Re: [PATCH v3 2/5] KVM: x86: invert KVM_HYPERCALL to default to VMMCALL

[Kalra, Ashish]

> On Aug 20, 2021, at 3:38 AM, Kalra, Ashish <Ashish.Kalra@xxxxxxx> wrote:
> 
> Hello Sean,
> 
>> On Aug 20, 2021, at 2:15 AM, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>> 
>> Preferred shortlog prefix for KVM guest changes is "x86/kvm".  "KVM: x86" is for
>> host changes.
>> 
>>>> On Tue, Jun 08, 2021, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>>> 
>>> KVM hypercall framework relies on alternative framework to patch the
>>> VMCALL -> VMMCALL on AMD platform. If a hypercall is made before
>>> apply_alternative() is called then it defaults to VMCALL. The approach
>>> works fine on non SEV guest. A VMCALL would causes #UD, and hypervisor
>>> will be able to decode the instruction and do the right things. But
>>> when SEV is active, guest memory is encrypted with guest key and
>>> hypervisor will not be able to decode the instruction bytes.
>>> 
>>> So invert KVM_HYPERCALL and X86_FEATURE_VMMCALL to default to VMMCALL
>>> and opt into VMCALL.
>> 
>> The changelog needs to explain why SEV hypercalls need to be made before
>> apply_alternative(), why it's ok to make Intel CPUs take #UDs on the unknown
>> VMMCALL, and why this is not creating the same conundrum for TDX.
> 
> I think it makes more sense to stick to the original approach/patch, i.e., introducing a new private hypercall interface like kvm_sev_hypercall3() and let early paravirtualized kernel code invoke this private hypercall interface wherever required.
> 
> This helps avoiding Intel CPUs taking unnecessary #UDs and also avoid using hacks as below.
> 
> TDX code can introduce similar private hypercall interface for their early para virtualized kernel code if required.

Actually, if we are using this kvm_sev_hypercall3() and not modifying KVM_HYPERCALL() then Intel CPUs avoid unnecessary #UDs and TDX code does not need any new interface. Only early AMD/SEV specific code will use this kvm_sev_hypercall3() interface. TDX code will always work with KVM_HYPERCALL().

Thanks,
Ashish

> 
>> 
>> Actually, I don't think making Intel CPUs take #UDs is acceptable.  This patch
>> breaks Linux on upstream KVM on Intel due a bug in upstream KVM.  KVM attempts
>> to patch the "wrong" hypercall to the "right" hypercall, but stupidly does so
>> via an emulated write.  I.e. KVM honors the guest page table permissions and
>> injects a !WRITABLE #PF on the VMMCALL RIP if the kernel code is mapped RX.
>> 
>> In other words, trusting the VMM to not screw up the #UD is a bad idea.  This also
>> makes documenting the "why does SEV need super early hypercalls" extra important.
>> 
> 
> Makes sense.
> 
> Thanks,
> Ashish
> 
>> This patch doesn't work because X86_FEATURE_VMCALL is a synthetic flag and is
>> only set by VMware paravirt code, which is why the patching doesn't happen as
>> would be expected.  The obvious solution would be to manually set X86_FEATURE_VMCALL
>> where appropriate, but given that defaulting to VMCALL has worked for years,
>> defaulting to VMMCALL makes me nervous, e.g. even if we splatter X86_FEATURE_VMCALL
>> into Intel, Centaur, and Zhaoxin, there's a possibility we'll break existing VMs
>> that run on hypervisors that do something weird with the vendor string.
>> 
>> Rather than look for X86_FEATURE_VMCALL, I think it makes sense to have this be
>> a "pure" inversion, i.e. patch in VMCALL if VMMCALL is not supported, as opposed
>> to patching in VMCALL if VMCALL is supproted.
>> 
>> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
>> index 69299878b200..61641e69cfda 100644
>> --- a/arch/x86/include/asm/kvm_para.h
>> +++ b/arch/x86/include/asm/kvm_para.h
>> @@ -17,7 +17,7 @@ static inline bool kvm_check_and_clear_guest_paused(void)
>> #endif /* CONFIG_KVM_GUEST */
>> 
>> #define KVM_HYPERCALL \
>> -        ALTERNATIVE("vmcall", "vmmcall", X86_FEATURE_VMMCALL)
>> +        ALTERNATIVE("vmmcall", "vmcall", ALT_NOT(X86_FEATURE_VMMCALL))
>> 
>> /* For KVM hypercalls, a three-byte sequence of either the vmcall or the vmmcall
>> * instruction.  The hypervisor may replace it with something else but only the
>> 
>>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>>> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>> Cc: Joerg Roedel <joro@xxxxxxxxxx>
>>> Cc: Borislav Petkov <bp@xxxxxxx>
>>> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
>>> Cc: x86@xxxxxxxxxx
>>> Cc: kvm@xxxxxxxxxxxxxxx
>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> 
>> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>> 
>>> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
>> 
>> Is Brijesh the author?  Co-developed-by for a one-line change would be odd...
>> 
>>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>>> ---
>>> arch/x86/include/asm/kvm_para.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
>>> index 69299878b200..0267bebb0b0f 100644
>>> --- a/arch/x86/include/asm/kvm_para.h
>>> +++ b/arch/x86/include/asm/kvm_para.h
>>> @@ -17,7 +17,7 @@ static inline bool kvm_check_and_clear_guest_paused(void)
>>> #endif /* CONFIG_KVM_GUEST */
>>> 
>>> #define KVM_HYPERCALL \
>>> -        ALTERNATIVE("vmcall", "vmmcall", X86_FEATURE_VMMCALL)
>>> +    ALTERNATIVE("vmmcall", "vmcall", X86_FEATURE_VMCALL)
>>> 
>>> /* For KVM hypercalls, a three-byte sequence of either the vmcall or the vmmcall
>>> * instruction.  The hypervisor may replace it with something else but only the
>>> -- 
>>> 2.17.1
>>>