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

[Jon Doron <arilou@gmail.com>]

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
>