Re: [PATCH v2 4/4] x86/kvm/hyper-v: Add support for synthetic debugger via hypercalls

Jon Doron
2020-03-06T16:36:06Z

On 06/03/2020, Vitaly Kuznetsov wrote:
>Jon Doron <arilou@gmail.com> writes:
>
>> There is another mode for the synthetic debugger which uses hypercalls
>> to send/recv network data instead of the MSR interface.
>>
>> This interface is much slower and less recommended since you might get
>> a lot of VMExits while KDVM polling for new packets to recv, rather
>> than simply checking the pending page to see if there is data avialble
>> and then request.
>>
>> Signed-off-by: Jon Doron <arilou@gmail.com>
>> ---
>> arch/x86/include/asm/hyperv-tlfs.h | 5 +++++
>> arch/x86/kvm/hyperv.c | 17 +++++++++++++++++
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
>> index 8efdf974c23f..4fa6bf3732a6 100644
>> --- a/arch/x86/include/asm/hyperv-tlfs.h
>> +++ b/arch/x86/include/asm/hyperv-tlfs.h
>> @@ -283,6 +283,8 @@
>> #define HV_X64_MSR_SYNDBG_PENDING_BUFFER 0x400000F5
>> #define HV_X64_MSR_SYNDBG_OPTIONS 0x400000FF
>>
>> +#define HV_X64_SYNDBG_OPTION_USE_HCALLS BIT(2)
>
>Nitpick: please add a comment like
>"These are HV_X64_MSR_SYNDBG_OPTIONS bits"
>just before the definition to make it to bluntly obvious.
>

Done.

>> +
>> /* Hyper-V guest crash notification MSR's */
>> #define HV_X64_MSR_CRASH_P0 0x40000100
>> #define HV_X64_MSR_CRASH_P1 0x40000101
>> @@ -392,6 +394,9 @@ struct hv_tsc_emulation_status {
>> #define HVCALL_SEND_IPI_EX 0x0015
>> #define HVCALL_POST_MESSAGE 0x005c
>> #define HVCALL_SIGNAL_EVENT 0x005d
>> +#define HVCALL_POST_DEBUG_DATA 0x0069
>> +#define HVCALL_RETRIEVE_DEBUG_DATA 0x006a
>> +#define HVCALL_RESET_DEBUG_SESSION 0x006b
>> #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
>> #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index d657a312004a..52517e11e643 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1800,6 +1800,23 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>> }
>> ret = kvm_hv_send_ipi(vcpu, ingpa, outgpa, true, false);
>> break;
>> + case HVCALL_POST_DEBUG_DATA:
>> + case HVCALL_RETRIEVE_DEBUG_DATA:
>> + case HVCALL_RESET_DEBUG_SESSION: {
>> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
>> + if (!(syndbg->options & HV_X64_SYNDBG_OPTION_USE_HCALLS)) {
>> + ret = HV_STATUS_INVALID_HYPERCALL_INPUT;
>
>In TLFS it is said that only HvResetDebugSession of these three can be
>'fast', others are regular hypercalls. We need to add something like
>
> if (unlikely(fast && code != HVCALL_RESET_DEBUG_SESSION)) {
> ret = HV_STATUS_INVALID_HYPERCALL_INPUT;
> break;
> }
>
>also, I'm not sure HV_STATUS_INVALID_HYPERCALL_INPUT is always the right
>return value as TLFS describes this as
>
>"The rep count was incorrect (for example, a non-zero rep count was
>passed to a non-rep call or a zero rep count was passed to a rep call) or
>a reserved bit in the specified hypercall input value was non-zero."
>
>(we may actually be wrong even for existing hypercalls)
>

You are right I believe in the next version I'm using a more proper
return code.

>> + break;
>> + }
>> + vcpu->run->exit_reason = KVM_EXIT_HYPERV;
>> + vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
>> + vcpu->run->hyperv.u.hcall.input = param;
>> + vcpu->run->hyperv.u.hcall.params[0] = ingpa;
>> + vcpu->run->hyperv.u.hcall.params[1] = outgpa;
>> + vcpu->arch.complete_userspace_io =
>> + kvm_hv_hypercall_complete_userspace;
>> + return 0;
>> + }
>> default:
>> ret = HV_STATUS_INVALID_HYPERCALL_CODE;
>> break;
>
>--
>Vitaly
>