Re: [PATCH v11 5/7] x86/kvm/hyper-v: enable hypercalls without hypercall page with syndbg

[Jon Doron <arilou@gmail.com>]

On 13/05/2020, Roman Kagan wrote:
>On Fri, Apr 24, 2020 at 02:37:44PM +0300, Jon Doron wrote:
>> Microsoft's kdvm.dll dbgtransport module does not respect the hypercall
>> page and simply identifies the CPU being used (AMD/Intel) and according
>> to it simply makes hypercalls with the relevant instruction
>> (vmmcall/vmcall respectively).
>>
>> The relevant function in kdvm is KdHvConnectHypervisor which first checks
>> if the hypercall page has been enabled via HV_X64_MSR_HYPERCALL_ENABLE,
>> and in case it was not it simply sets the HV_X64_MSR_GUEST_OS_ID to
>> 0x1000101010001 which means:
>> build_number = 0x0001
>> service_version = 0x01
>> minor_version = 0x01
>> major_version = 0x01
>> os_id = 0x00 (Undefined)
>> vendor_id = 1 (Microsoft)
>> os_type = 0 (A value of 0 indicates a proprietary, closed source OS)
>>
>> and starts issuing the hypercall without setting the hypercall page.
>
>I guess this is to avoid interfering with the OS being debugged
>requesting its own hypercall page at a different address.
>
>> To resolve this issue simply enable hypercalls also if the guest_os_id
>> is not 0 and the syndbg feature is enabled.
>>
>> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Signed-off-by: Jon Doron <arilou@gmail.com>
>> ---
>>  arch/x86/kvm/hyperv.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index 435516595090..524b5466a515 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1650,7 +1650,10 @@ static u64 kvm_hv_send_ipi(struct kvm_vcpu *current_vcpu, u64 ingpa, u64 outgpa,
>>
>>  bool kvm_hv_hypercall_enabled(struct kvm *kvm)
>>  {
>> -	return READ_ONCE(kvm->arch.hyperv.hv_hypercall) & HV_X64_MSR_HYPERCALL_ENABLE;
>> +	struct kvm_hv *hv = &kvm->arch.hyperv;
>> +
>> +	return READ_ONCE(hv->hv_hypercall) & HV_X64_MSR_HYPERCALL_ENABLE ||
>> +	       (hv->hv_syndbg.active && READ_ONCE(hv->hv_guest_os_id) != 0);
>
>This function is meant to tell if the hypercall should be interpreted as
>following KVM or HyperV conventions.  Quoting from the spec
>
>  3.5 Legal Hypercall Environments
>
>  ...
>  All hypercalls should be invoked through the architecturally-defined
>  hypercall interface. (See the following sections for instructions on
>  discovering and establishing this interface.) An attempt to invoke a
>  hypercall by any other means (for example, copying the code from the
>  hypercall code page to an alternate location and executing it from
>  there) might result in an undefined operation (#UD) exception.  The
>  hypervisor is not guaranteed to deliver this exception.
>
>so I think we can simply test for hv_guest_os_id != 0 and ignore
>HV_X64_MSR_HYPERCALL_ENABLE (it's about hypercall page being enabled,
>not the hypercalls per se).
>
>Thanks,
>Roman.
>
>>  }
>>
>>  static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
>> --
>> 2.24.1
>>

Hi Roman,

I agree this was the original implementation of this patchset (see v1) I 
will send a v12 with the suggested change, but I would prefer that you 
will review the mailing list previous comments which caused to this 
specific behaviour.

Thanks,
-- Jon.