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

On 06/03/2020, Vitaly Kuznetsov wrote:

Jon Doron [off-list ref] writes:

quoted

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 <redacted>
---
 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.

quoted

+
 /* 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.

quoted

+			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