* Re: [PATCH v1 1/3] x86/kvm/hyper-v: Add support for synthetic debugger capability
[not found] ` <20200303130356.50405-2-arilou@gmail.com>
@ 2020-03-04 13:51 ` Vitaly Kuznetsov
2020-03-05 13:56 ` Jon Doron
0 siblings, 1 reply; 5+ messages in thread
From: Vitaly Kuznetsov @ 2020-03-04 13:51 UTC (permalink / raw)
To: kvm; +Cc: Jon Doron, linux-hyperv
Jon Doron <arilou@gmail.com> writes:
> Add support for Hyper-V synthetic debugger (syndbg) interface.
> The syndbg interface is using MSRs to emulate a way to send/recv packets
> data.
>
> The debug transport dll (kdvm/kdnet) will identify if Hyper-V is enabled
> and if it supports the synthetic debugger interface it will attempt to
> use it, instead of trying to initialize a network adapter.
>
Cc: linux-hyperv@ list where Hyper-V folks live :-) They're in charge of
'hyperv-tlfs.h' so an ACK from them will be needed.
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> arch/x86/include/asm/hyperv-tlfs.h | 16 +++++
> arch/x86/include/asm/kvm_host.h | 11 +++
> arch/x86/kvm/hyperv.c | 109 +++++++++++++++++++++++++++++
> arch/x86/kvm/hyperv.h | 5 ++
> arch/x86/kvm/trace.h | 22 ++++++
> arch/x86/kvm/x86.c | 8 +++
> include/uapi/linux/kvm.h | 9 +++
> 7 files changed, 180 insertions(+)
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 92abc1e42bfc..8efdf974c23f 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -33,6 +33,9 @@
> #define HYPERV_CPUID_ENLIGHTMENT_INFO 0x40000004
> #define HYPERV_CPUID_IMPLEMENT_LIMITS 0x40000005
> #define HYPERV_CPUID_NESTED_FEATURES 0x4000000A
> +#define HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS 0x40000080
> +#define HYPERV_CPUID_SYNDBG_INTERFACE 0x40000081
> +#define HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES 0x40000082
Out of pure curiosity, are these CPUIDs and MSRs documented somewhere?
I'm looking at TLFS v6.0 and failing to see them...
>
> #define HYPERV_HYPERVISOR_PRESENT_BIT 0x80000000
> #define HYPERV_CPUID_MIN 0x40000005
> @@ -131,6 +134,8 @@
> #define HV_FEATURE_FREQUENCY_MSRS_AVAILABLE BIT(8)
> /* Crash MSR available */
> #define HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE BIT(10)
> +/* Support for debug MSRs available */
> +#define HV_FEATURE_DEBUG_MSRS_AVAILABLE BIT(11)
> /* stimer Direct Mode is available */
> #define HV_STIMER_DIRECT_MODE_AVAILABLE BIT(19)
>
> @@ -194,6 +199,9 @@
> #define HV_X64_NESTED_GUEST_MAPPING_FLUSH BIT(18)
> #define HV_X64_NESTED_MSR_BITMAP BIT(19)
>
> +/* Hyper-V synthetic debugger platform capabilities */
> +#define HV_X64_SYNDBG_CAP_ALLOW_KERNEL_DEBUGGING BIT(1)
> +
> /* Hyper-V specific model specific registers (MSRs) */
>
> /* MSR used to identify the guest OS. */
> @@ -267,6 +275,14 @@
> /* Hyper-V guest idle MSR */
> #define HV_X64_MSR_GUEST_IDLE 0x400000F0
>
> +/* Hyper-V Synthetic debug options MSR */
> +#define HV_X64_MSR_SYNDBG_CONTROL 0x400000F1
> +#define HV_X64_MSR_SYNDBG_STATUS 0x400000F2
> +#define HV_X64_MSR_SYNDBG_SEND_BUFFER 0x400000F3
> +#define HV_X64_MSR_SYNDBG_RECV_BUFFER 0x400000F4
> +#define HV_X64_MSR_SYNDBG_PENDING_BUFFER 0x400000F5
> +#define HV_X64_MSR_SYNDBG_OPTIONS 0x400000FF
> +
> /* Hyper-V guest crash notification MSR's */
> #define HV_X64_MSR_CRASH_P0 0x40000100
> #define HV_X64_MSR_CRASH_P1 0x40000101
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 98959e8cd448..2b755174d683 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -854,6 +854,15 @@ struct kvm_apic_map {
> struct kvm_lapic *phys_map[];
> };
>
> +/* Hyper-V synthetic debugger (SynDbg)*/
> +struct kvm_hv_syndbg {
> + u64 control;
> + u64 status;
> + u64 send_page;
> + u64 recv_page;
> + u64 pending_page;
> +};
> +
> /* Hyper-V emulation context */
> struct kvm_hv {
> struct mutex hv_lock;
> @@ -877,6 +886,8 @@ struct kvm_hv {
> atomic_t num_mismatched_vp_indexes;
>
> struct hv_partition_assist_pg *hv_pa_pg;
> + u64 hv_syndbg_options;
> + struct kvm_hv_syndbg hv_syndbg;
I would've encapsulated both to struct kvm_hv_syndbg, e.g.
struct kvm_hv_syndbg {
struct {
u64 control;
u64 status;
u64 send_page;
u64 recv_page;
u64 pending_page;
} control;
u64 options;
}
To make it clear they're part of the same thing (are they?) I see you
handle HV_X64_MSR_SYNDBG_OPTIONS differently (outside of
syndbg_set_msr()).
> };
>
> enum kvm_irqchip_mode {
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index a86fda7a1d03..13176ec23496 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -266,6 +266,66 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
> return ret;
> }
>
> +static int kvm_hv_syndbg_complete_userspace(struct kvm_vcpu *vcpu)
> +{
> + struct kvm *kvm = vcpu->kvm;
> + struct kvm_hv *hv = &kvm->arch.hyperv;
> +
> + if (vcpu->run->hyperv.u.syndbg.msr == HV_X64_MSR_SYNDBG_CONTROL)
> + hv->hv_syndbg.status = vcpu->run->hyperv.u.syndbg.status;
> + return 1;
> +}
> +
> +static void syndbg_exit(struct kvm_vcpu *vcpu, u32 msr)
> +{
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> + struct kvm_vcpu_hv *hv_vcpu = &vcpu->arch.hyperv;
> +
> + hv_vcpu->exit.type = KVM_EXIT_HYPERV_SYNDBG;
> + hv_vcpu->exit.u.syndbg.msr = msr;
> + hv_vcpu->exit.u.syndbg.control = syndbg->control;
> + hv_vcpu->exit.u.syndbg.send_page = syndbg->send_page;
> + hv_vcpu->exit.u.syndbg.recv_page = syndbg->recv_page;
> + hv_vcpu->exit.u.syndbg.pending_page = syndbg->pending_page;
> + vcpu->arch.complete_userspace_io =
> + kvm_hv_syndbg_complete_userspace;
> +
> + kvm_make_request(KVM_REQ_HV_EXIT, vcpu);
This new interface requires userspace support apparently so we can't
enable it unconditionally (userspaces which don't support it will be
very confused). You need to introduce a capability
(KVM_CAP_HYPERV_DEBUGGING?)
> +}
> +
> +static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> +{
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> + int ret;
> +
> + trace_kvm_hv_syndbg_set_msr(vcpu->vcpu_id, msr, data);
> + ret = 0;
> + switch (msr) {
> + case HV_X64_MSR_SYNDBG_CONTROL:
> + syndbg->control = data;
> + syndbg_exit(vcpu, msr);
> + break;
> + case HV_X64_MSR_SYNDBG_STATUS:
> + syndbg->status = data;
> + break;
> + case HV_X64_MSR_SYNDBG_SEND_BUFFER:
> + syndbg->send_page = data;
> + break;
> + case HV_X64_MSR_SYNDBG_RECV_BUFFER:
> + syndbg->recv_page = data;
> + break;
> + case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + syndbg->pending_page = data;
> + syndbg_exit(vcpu, msr);
> + break;
> + default:
> + ret = 1;
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int synic_get_msr(struct kvm_vcpu_hv_synic *synic, u32 msr, u64 *pdata,
> bool host)
> {
> @@ -800,6 +860,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr)
> case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
> case HV_X64_MSR_TSC_EMULATION_CONTROL:
> case HV_X64_MSR_TSC_EMULATION_STATUS:
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> r = true;
> break;
> }
> @@ -1061,6 +1123,11 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
> if (!host)
> return 1;
> break;
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + hv->hv_syndbg_options = data;
> + break;
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + return syndbg_set_msr(vcpu, msr, data);
> default:
> vcpu_unimpl(vcpu, "Hyper-V unhandled wrmsr: 0x%x data 0x%llx\n",
> msr, data);
> @@ -1227,6 +1294,24 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> case HV_X64_MSR_TSC_EMULATION_STATUS:
> data = hv->hv_tsc_emulation_status;
> break;
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + data = hv->hv_syndbg_options;
> + break;
> + case HV_X64_MSR_SYNDBG_CONTROL:
> + data = hv->hv_syndbg.control;
> + break;
> + case HV_X64_MSR_SYNDBG_STATUS:
> + data = hv->hv_syndbg.status;
> + break;
> + case HV_X64_MSR_SYNDBG_SEND_BUFFER:
> + data = hv->hv_syndbg.send_page;
> + break;
> + case HV_X64_MSR_SYNDBG_RECV_BUFFER:
> + data = hv->hv_syndbg.recv_page;
> + break;
> + case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + data = hv->hv_syndbg.pending_page;
> + break;
> default:
> vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
> return 1;
> @@ -1797,6 +1882,9 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> { .function = HYPERV_CPUID_ENLIGHTMENT_INFO },
> { .function = HYPERV_CPUID_IMPLEMENT_LIMITS },
> { .function = HYPERV_CPUID_NESTED_FEATURES },
> + { .function = HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS },
> + { .function = HYPERV_CPUID_SYNDBG_INTERFACE },
> + { .function = HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES },
HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS now returns
'HYPERV_CPUID_NESTED_FEATURES' as the last available leaf and I don't
see you adjusting it - is this expected?
> };
> int i, nent = ARRAY_SIZE(cpuid_entries);
>
> @@ -1856,9 +1944,12 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>
> ent->ebx |= HV_X64_POST_MESSAGES;
> ent->ebx |= HV_X64_SIGNAL_EVENTS;
> + ent->ebx |= HV_X64_DEBUGGING;
>
> ent->edx |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE;
> ent->edx |= HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE;
> + ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE;
> + ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE;
>
> /*
> * Direct Synthetic timers only make sense with in-kernel
> @@ -1903,6 +1994,24 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>
> break;
>
> + case HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS:
> + memcpy(signature, "Microsoft VS", 12);
Does this string matter? E.g. for HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS
we call ourselves "Linux KVM Hv", I think we should do the same here.
> +
> + ent->eax = 0;
> + ent->ebx = signature[0];
> + ent->ecx = signature[1];
> + ent->edx = signature[2];
> + break;
> +
> + case HYPERV_CPUID_SYNDBG_INTERFACE:
> + memcpy(signature, "VS#1\0\0\0\0\0\0\0\0", 12);
> + ent->eax = signature[0];
> + break;
> +
> + case HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES:
> + ent->eax |= HV_X64_SYNDBG_CAP_ALLOW_KERNEL_DEBUGGING;
> + break;
> +
> default:
> break;
> }
> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> index 757cb578101c..6a86151fac53 100644
> --- a/arch/x86/kvm/hyperv.h
> +++ b/arch/x86/kvm/hyperv.h
> @@ -46,6 +46,11 @@ static inline struct kvm_vcpu *synic_to_vcpu(struct kvm_vcpu_hv_synic *synic)
> return hv_vcpu_to_vcpu(container_of(synic, struct kvm_vcpu_hv, synic));
> }
>
> +static inline struct kvm_hv_syndbg *vcpu_to_hv_syndbg(struct kvm_vcpu *vcpu)
> +{
> + return &vcpu->kvm->arch.hyperv.hv_syndbg;
> +}
> +
> int kvm_hv_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host);
> int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host);
>
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index f194dd058470..235b9ab673a2 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -1515,6 +1515,28 @@ TRACE_EVENT(kvm_nested_vmenter_failed,
> __print_symbolic(__entry->err, VMX_VMENTER_INSTRUCTION_ERRORS))
> );
>
> +/*
> + * Tracepoint for syndbg_set_msr.
> + */
> +TRACE_EVENT(kvm_hv_syndbg_set_msr,
> + TP_PROTO(int vcpu_id, u32 msr, u64 data),
> + TP_ARGS(vcpu_id, msr, data),
> +
> + TP_STRUCT__entry(
> + __field(int, vcpu_id)
> + __field(u32, msr)
> + __field(u64, data)
> + ),
> +
> + TP_fast_assign(
> + __entry->vcpu_id = vcpu_id;
> + __entry->msr = msr;
> + __entry->data = data;
> + ),
> +
> + TP_printk("vcpu_id %d msr 0x%x data 0x%llx",
> + __entry->vcpu_id, __entry->msr, __entry->data)
This doesn't give us any additional data trace_kvm_msr_* points are more
or less the same. I think we can do better, e.g. for Hyper-V specific
things log the processor's VP index.
> +);
> #endif /* _TRACE_KVM_H */
>
> #undef TRACE_INCLUDE_PATH
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5de200663f51..9d4d72a88572 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1214,6 +1214,10 @@ static const u32 emulated_msrs_all[] = {
> HV_X64_MSR_VP_ASSIST_PAGE,
> HV_X64_MSR_REENLIGHTENMENT_CONTROL, HV_X64_MSR_TSC_EMULATION_CONTROL,
> HV_X64_MSR_TSC_EMULATION_STATUS,
> + HV_X64_MSR_SYNDBG_OPTIONS,
> + HV_X64_MSR_SYNDBG_CONTROL, HV_X64_MSR_SYNDBG_STATUS,
> + HV_X64_MSR_SYNDBG_SEND_BUFFER, HV_X64_MSR_SYNDBG_RECV_BUFFER,
> + HV_X64_MSR_SYNDBG_PENDING_BUFFER,
>
> MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME,
> MSR_KVM_PV_EOI_EN,
> @@ -2906,6 +2910,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> */
> break;
> case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
> case HV_X64_MSR_CRASH_CTL:
> case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
> @@ -3151,6 +3157,8 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> msr_info->data = 0x20000000;
> break;
> case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
> case HV_X64_MSR_CRASH_CTL:
> case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4b95f9a31a2f..fae8cf608976 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -188,6 +188,7 @@ struct kvm_s390_cmma_log {
> struct kvm_hyperv_exit {
> #define KVM_EXIT_HYPERV_SYNIC 1
> #define KVM_EXIT_HYPERV_HCALL 2
> +#define KVM_EXIT_HYPERV_SYNDBG 3
> __u32 type;
> union {
> struct {
> @@ -201,6 +202,14 @@ struct kvm_hyperv_exit {
> __u64 result;
> __u64 params[2];
> } hcall;
> + struct {
> + __u32 msr;
> + __u64 control;
> + __u64 status;
> + __u64 send_page;
> + __u64 recv_page;
> + __u64 pending_page;
> + } syndbg;
> } u;
> };
Not your fault but I just noticed that 'struct kvm_hyperv_exit' is not
properly padded. 'synic' struct is OK, however, 'hcall' is not as
there's gonna be a gap between '__u32 type' and it. Your 'struct syndbg'
is also OK as it starts with '__u32 msr' but we should do something
about hcall.
--
Vitaly
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 2/3] x86/kvm/hyper-v: enable hypercalls regardless of hypercall page
[not found] ` <20200303130356.50405-3-arilou@gmail.com>
@ 2020-03-04 13:58 ` Vitaly Kuznetsov
2020-03-05 13:57 ` Jon Doron
0 siblings, 1 reply; 5+ messages in thread
From: Vitaly Kuznetsov @ 2020-03-04 13:58 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv
Jon Doron <arilou@gmail.com> writes:
> 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.
>
> To resolve this issue simply enable hypercalls if the guest_os_id is
> not 0.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> arch/x86/kvm/hyperv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 13176ec23496..7ec962d433af 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1615,7 +1615,7 @@ 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;
> + return READ_ONCE(kvm->arch.hyperv.hv_guest_os_id) != 0;
> }
>
> static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
I would've enabled it in both cases,
return (READ_ONCE(kvm->arch.hyperv.hv_hypercall) &
HV_X64_MSR_HYPERCALL_ENABLE) || (READ_ONCE(kvm->arch.hyperv.hv_guest_os_id) != 0);
to be safe. We can also check what genuine Hyper-V does but I bet it has
hypercalls always enabled. Also, the function can be made inline,
there's a single caller.
--
Vitaly
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/3] x86/kvm/hyper-v: Add support for synthetic debugger capability
2020-03-04 13:51 ` [PATCH v1 1/3] x86/kvm/hyper-v: Add support for synthetic debugger capability Vitaly Kuznetsov
@ 2020-03-05 13:56 ` Jon Doron
0 siblings, 0 replies; 5+ messages in thread
From: Jon Doron @ 2020-03-05 13:56 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: kvm, linux-hyperv
On Wed, Mar 4, 2020 at 3:51 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Jon Doron <arilou@gmail.com> writes:
>
> > Add support for Hyper-V synthetic debugger (syndbg) interface.
> > The syndbg interface is using MSRs to emulate a way to send/recv packets
> > data.
> >
> > The debug transport dll (kdvm/kdnet) will identify if Hyper-V is enabled
> > and if it supports the synthetic debugger interface it will attempt to
> > use it, instead of trying to initialize a network adapter.
> >
>
> Cc: linux-hyperv@ list where Hyper-V folks live :-) They're in charge of
> 'hyperv-tlfs.h' so an ACK from them will be needed.
>
Thanks will do in next version of this patchset.
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> > arch/x86/include/asm/hyperv-tlfs.h | 16 +++++
> > arch/x86/include/asm/kvm_host.h | 11 +++
> > arch/x86/kvm/hyperv.c | 109 +++++++++++++++++++++++++++++
> > arch/x86/kvm/hyperv.h | 5 ++
> > arch/x86/kvm/trace.h | 22 ++++++
> > arch/x86/kvm/x86.c | 8 +++
> > include/uapi/linux/kvm.h | 9 +++
> > 7 files changed, 180 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> > index 92abc1e42bfc..8efdf974c23f 100644
> > --- a/arch/x86/include/asm/hyperv-tlfs.h
> > +++ b/arch/x86/include/asm/hyperv-tlfs.h
> > @@ -33,6 +33,9 @@
> > #define HYPERV_CPUID_ENLIGHTMENT_INFO 0x40000004
> > #define HYPERV_CPUID_IMPLEMENT_LIMITS 0x40000005
> > #define HYPERV_CPUID_NESTED_FEATURES 0x4000000A
> > +#define HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS 0x40000080
> > +#define HYPERV_CPUID_SYNDBG_INTERFACE 0x40000081
> > +#define HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES 0x40000082
>
> Out of pure curiosity, are these CPUIDs and MSRs documented somewhere?
> I'm looking at TLFS v6.0 and failing to see them...
>
Basically a lot of playing around with KDVM/KDNet by setting a kernel
debugger, there are
also few hints about how the transport for KD works in the DDK (there
are samples that
show how to code a new kdtransport)
> >
> > #define HYPERV_HYPERVISOR_PRESENT_BIT 0x80000000
> > #define HYPERV_CPUID_MIN 0x40000005
> > @@ -131,6 +134,8 @@
> > #define HV_FEATURE_FREQUENCY_MSRS_AVAILABLE BIT(8)
> > /* Crash MSR available */
> > #define HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE BIT(10)
> > +/* Support for debug MSRs available */
> > +#define HV_FEATURE_DEBUG_MSRS_AVAILABLE BIT(11)
> > /* stimer Direct Mode is available */
> > #define HV_STIMER_DIRECT_MODE_AVAILABLE BIT(19)
> >
> > @@ -194,6 +199,9 @@
> > #define HV_X64_NESTED_GUEST_MAPPING_FLUSH BIT(18)
> > #define HV_X64_NESTED_MSR_BITMAP BIT(19)
> >
> > +/* Hyper-V synthetic debugger platform capabilities */
> > +#define HV_X64_SYNDBG_CAP_ALLOW_KERNEL_DEBUGGING BIT(1)
> > +
> > /* Hyper-V specific model specific registers (MSRs) */
> >
> > /* MSR used to identify the guest OS. */
> > @@ -267,6 +275,14 @@
> > /* Hyper-V guest idle MSR */
> > #define HV_X64_MSR_GUEST_IDLE 0x400000F0
> >
> > +/* Hyper-V Synthetic debug options MSR */
> > +#define HV_X64_MSR_SYNDBG_CONTROL 0x400000F1
> > +#define HV_X64_MSR_SYNDBG_STATUS 0x400000F2
> > +#define HV_X64_MSR_SYNDBG_SEND_BUFFER 0x400000F3
> > +#define HV_X64_MSR_SYNDBG_RECV_BUFFER 0x400000F4
> > +#define HV_X64_MSR_SYNDBG_PENDING_BUFFER 0x400000F5
> > +#define HV_X64_MSR_SYNDBG_OPTIONS 0x400000FF
> > +
> > /* Hyper-V guest crash notification MSR's */
> > #define HV_X64_MSR_CRASH_P0 0x40000100
> > #define HV_X64_MSR_CRASH_P1 0x40000101
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 98959e8cd448..2b755174d683 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -854,6 +854,15 @@ struct kvm_apic_map {
> > struct kvm_lapic *phys_map[];
> > };
> >
> > +/* Hyper-V synthetic debugger (SynDbg)*/
> > +struct kvm_hv_syndbg {
> > + u64 control;
> > + u64 status;
> > + u64 send_page;
> > + u64 recv_page;
> > + u64 pending_page;
> > +};
> > +
> > /* Hyper-V emulation context */
> > struct kvm_hv {
> > struct mutex hv_lock;
> > @@ -877,6 +886,8 @@ struct kvm_hv {
> > atomic_t num_mismatched_vp_indexes;
> >
> > struct hv_partition_assist_pg *hv_pa_pg;
> > + u64 hv_syndbg_options;
> > + struct kvm_hv_syndbg hv_syndbg;
>
> I would've encapsulated both to struct kvm_hv_syndbg, e.g.
>
> struct kvm_hv_syndbg {
> struct {
> u64 control;
> u64 status;
> u64 send_page;
> u64 recv_page;
> u64 pending_page;
> } control;
> u64 options;
> }
>
> To make it clear they're part of the same thing (are they?) I see you
> handle HV_X64_MSR_SYNDBG_OPTIONS differently (outside of
> syndbg_set_msr()).
>
Done.
> > };
> >
> > enum kvm_irqchip_mode {
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index a86fda7a1d03..13176ec23496 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -266,6 +266,66 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
> > return ret;
> > }
> >
> > +static int kvm_hv_syndbg_complete_userspace(struct kvm_vcpu *vcpu)
> > +{
> > + struct kvm *kvm = vcpu->kvm;
> > + struct kvm_hv *hv = &kvm->arch.hyperv;
> > +
> > + if (vcpu->run->hyperv.u.syndbg.msr == HV_X64_MSR_SYNDBG_CONTROL)
> > + hv->hv_syndbg.status = vcpu->run->hyperv.u.syndbg.status;
> > + return 1;
> > +}
> > +
> > +static void syndbg_exit(struct kvm_vcpu *vcpu, u32 msr)
> > +{
> > + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> > + struct kvm_vcpu_hv *hv_vcpu = &vcpu->arch.hyperv;
> > +
> > + hv_vcpu->exit.type = KVM_EXIT_HYPERV_SYNDBG;
> > + hv_vcpu->exit.u.syndbg.msr = msr;
> > + hv_vcpu->exit.u.syndbg.control = syndbg->control;
> > + hv_vcpu->exit.u.syndbg.send_page = syndbg->send_page;
> > + hv_vcpu->exit.u.syndbg.recv_page = syndbg->recv_page;
> > + hv_vcpu->exit.u.syndbg.pending_page = syndbg->pending_page;
> > + vcpu->arch.complete_userspace_io =
> > + kvm_hv_syndbg_complete_userspace;
> > +
> > + kvm_make_request(KVM_REQ_HV_EXIT, vcpu);
>
> This new interface requires userspace support apparently so we can't
> enable it unconditionally (userspaces which don't support it will be
> very confused). You need to introduce a capability
> (KVM_CAP_HYPERV_DEBUGGING?)
>
Done.
> > +}
> > +
> > +static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> > +{
> > + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> > + int ret;
> > +
> > + trace_kvm_hv_syndbg_set_msr(vcpu->vcpu_id, msr, data);
> > + ret = 0;
> > + switch (msr) {
> > + case HV_X64_MSR_SYNDBG_CONTROL:
> > + syndbg->control = data;
> > + syndbg_exit(vcpu, msr);
> > + break;
> > + case HV_X64_MSR_SYNDBG_STATUS:
> > + syndbg->status = data;
> > + break;
> > + case HV_X64_MSR_SYNDBG_SEND_BUFFER:
> > + syndbg->send_page = data;
> > + break;
> > + case HV_X64_MSR_SYNDBG_RECV_BUFFER:
> > + syndbg->recv_page = data;
> > + break;
> > + case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> > + syndbg->pending_page = data;
> > + syndbg_exit(vcpu, msr);
> > + break;
> > + default:
> > + ret = 1;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int synic_get_msr(struct kvm_vcpu_hv_synic *synic, u32 msr, u64 *pdata,
> > bool host)
> > {
> > @@ -800,6 +860,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr)
> > case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
> > case HV_X64_MSR_TSC_EMULATION_CONTROL:
> > case HV_X64_MSR_TSC_EMULATION_STATUS:
> > + case HV_X64_MSR_SYNDBG_OPTIONS:
> > + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> > r = true;
> > break;
> > }
> > @@ -1061,6 +1123,11 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
> > if (!host)
> > return 1;
> > break;
> > + case HV_X64_MSR_SYNDBG_OPTIONS:
> > + hv->hv_syndbg_options = data;
> > + break;
> > + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> > + return syndbg_set_msr(vcpu, msr, data);
> > default:
> > vcpu_unimpl(vcpu, "Hyper-V unhandled wrmsr: 0x%x data 0x%llx\n",
> > msr, data);
> > @@ -1227,6 +1294,24 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> > case HV_X64_MSR_TSC_EMULATION_STATUS:
> > data = hv->hv_tsc_emulation_status;
> > break;
> > + case HV_X64_MSR_SYNDBG_OPTIONS:
> > + data = hv->hv_syndbg_options;
> > + break;
> > + case HV_X64_MSR_SYNDBG_CONTROL:
> > + data = hv->hv_syndbg.control;
> > + break;
> > + case HV_X64_MSR_SYNDBG_STATUS:
> > + data = hv->hv_syndbg.status;
> > + break;
> > + case HV_X64_MSR_SYNDBG_SEND_BUFFER:
> > + data = hv->hv_syndbg.send_page;
> > + break;
> > + case HV_X64_MSR_SYNDBG_RECV_BUFFER:
> > + data = hv->hv_syndbg.recv_page;
> > + break;
> > + case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> > + data = hv->hv_syndbg.pending_page;
> > + break;
> > default:
> > vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
> > return 1;
> > @@ -1797,6 +1882,9 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> > { .function = HYPERV_CPUID_ENLIGHTMENT_INFO },
> > { .function = HYPERV_CPUID_IMPLEMENT_LIMITS },
> > { .function = HYPERV_CPUID_NESTED_FEATURES },
> > + { .function = HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS },
> > + { .function = HYPERV_CPUID_SYNDBG_INTERFACE },
> > + { .function = HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES },
>
> HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS now returns
> 'HYPERV_CPUID_NESTED_FEATURES' as the last available leaf and I don't
> see you adjusting it - is this expected?
>
Good catch my bad :)
> > };
> > int i, nent = ARRAY_SIZE(cpuid_entries);
> >
> > @@ -1856,9 +1944,12 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> >
> > ent->ebx |= HV_X64_POST_MESSAGES;
> > ent->ebx |= HV_X64_SIGNAL_EVENTS;
> > + ent->ebx |= HV_X64_DEBUGGING;
> >
> > ent->edx |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE;
> > ent->edx |= HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE;
> > + ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE;
> > + ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE;
> >
> > /*
> > * Direct Synthetic timers only make sense with in-kernel
> > @@ -1903,6 +1994,24 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> >
> > break;
> >
> > + case HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS:
> > + memcpy(signature, "Microsoft VS", 12);
>
> Does this string matter? E.g. for HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS
> we call ourselves "Linux KVM Hv", I think we should do the same here.
>
Nope does not really matter since Userspace will override this anyway
> > +
> > + ent->eax = 0;
> > + ent->ebx = signature[0];
> > + ent->ecx = signature[1];
> > + ent->edx = signature[2];
> > + break;
> > +
> > + case HYPERV_CPUID_SYNDBG_INTERFACE:
> > + memcpy(signature, "VS#1\0\0\0\0\0\0\0\0", 12);
> > + ent->eax = signature[0];
> > + break;
> > +
> > + case HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES:
> > + ent->eax |= HV_X64_SYNDBG_CAP_ALLOW_KERNEL_DEBUGGING;
> > + break;
> > +
> > default:
> > break;
> > }
> > diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> > index 757cb578101c..6a86151fac53 100644
> > --- a/arch/x86/kvm/hyperv.h
> > +++ b/arch/x86/kvm/hyperv.h
> > @@ -46,6 +46,11 @@ static inline struct kvm_vcpu *synic_to_vcpu(struct kvm_vcpu_hv_synic *synic)
> > return hv_vcpu_to_vcpu(container_of(synic, struct kvm_vcpu_hv, synic));
> > }
> >
> > +static inline struct kvm_hv_syndbg *vcpu_to_hv_syndbg(struct kvm_vcpu *vcpu)
> > +{
> > + return &vcpu->kvm->arch.hyperv.hv_syndbg;
> > +}
> > +
> > int kvm_hv_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host);
> > int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host);
> >
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > index f194dd058470..235b9ab673a2 100644
> > --- a/arch/x86/kvm/trace.h
> > +++ b/arch/x86/kvm/trace.h
> > @@ -1515,6 +1515,28 @@ TRACE_EVENT(kvm_nested_vmenter_failed,
> > __print_symbolic(__entry->err, VMX_VMENTER_INSTRUCTION_ERRORS))
> > );
> >
> > +/*
> > + * Tracepoint for syndbg_set_msr.
> > + */
> > +TRACE_EVENT(kvm_hv_syndbg_set_msr,
> > + TP_PROTO(int vcpu_id, u32 msr, u64 data),
> > + TP_ARGS(vcpu_id, msr, data),
> > +
> > + TP_STRUCT__entry(
> > + __field(int, vcpu_id)
> > + __field(u32, msr)
> > + __field(u64, data)
> > + ),
> > +
> > + TP_fast_assign(
> > + __entry->vcpu_id = vcpu_id;
> > + __entry->msr = msr;
> > + __entry->data = data;
> > + ),
> > +
> > + TP_printk("vcpu_id %d msr 0x%x data 0x%llx",
> > + __entry->vcpu_id, __entry->msr, __entry->data)
>
> This doesn't give us any additional data trace_kvm_msr_* points are more
> or less the same. I think we can do better, e.g. for Hyper-V specific
> things log the processor's VP index.
>
Done.
> > +);
> > #endif /* _TRACE_KVM_H */
> >
> > #undef TRACE_INCLUDE_PATH
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 5de200663f51..9d4d72a88572 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1214,6 +1214,10 @@ static const u32 emulated_msrs_all[] = {
> > HV_X64_MSR_VP_ASSIST_PAGE,
> > HV_X64_MSR_REENLIGHTENMENT_CONTROL, HV_X64_MSR_TSC_EMULATION_CONTROL,
> > HV_X64_MSR_TSC_EMULATION_STATUS,
> > + HV_X64_MSR_SYNDBG_OPTIONS,
> > + HV_X64_MSR_SYNDBG_CONTROL, HV_X64_MSR_SYNDBG_STATUS,
> > + HV_X64_MSR_SYNDBG_SEND_BUFFER, HV_X64_MSR_SYNDBG_RECV_BUFFER,
> > + HV_X64_MSR_SYNDBG_PENDING_BUFFER,
> >
> > MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME,
> > MSR_KVM_PV_EOI_EN,
> > @@ -2906,6 +2910,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > */
> > break;
> > case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
> > + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> > + case HV_X64_MSR_SYNDBG_OPTIONS:
> > case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
> > case HV_X64_MSR_CRASH_CTL:
> > case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
> > @@ -3151,6 +3157,8 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > msr_info->data = 0x20000000;
> > break;
> > case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
> > + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> > + case HV_X64_MSR_SYNDBG_OPTIONS:
> > case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
> > case HV_X64_MSR_CRASH_CTL:
> > case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 4b95f9a31a2f..fae8cf608976 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -188,6 +188,7 @@ struct kvm_s390_cmma_log {
> > struct kvm_hyperv_exit {
> > #define KVM_EXIT_HYPERV_SYNIC 1
> > #define KVM_EXIT_HYPERV_HCALL 2
> > +#define KVM_EXIT_HYPERV_SYNDBG 3
> > __u32 type;
> > union {
> > struct {
> > @@ -201,6 +202,14 @@ struct kvm_hyperv_exit {
> > __u64 result;
> > __u64 params[2];
> > } hcall;
> > + struct {
> > + __u32 msr;
> > + __u64 control;
> > + __u64 status;
> > + __u64 send_page;
> > + __u64 recv_page;
> > + __u64 pending_page;
> > + } syndbg;
> > } u;
> > };
>
> Not your fault but I just noticed that 'struct kvm_hyperv_exit' is not
> properly padded. 'synic' struct is OK, however, 'hcall' is not as
> there's gonna be a gap between '__u32 type' and it. Your 'struct syndbg'
> is also OK as it starts with '__u32 msr' but we should do something
> about hcall.
>
Created another patch to address this.
> --
> Vitaly
>
Cheers,
-- Jon.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 2/3] x86/kvm/hyper-v: enable hypercalls regardless of hypercall page
2020-03-04 13:58 ` [PATCH v1 2/3] x86/kvm/hyper-v: enable hypercalls regardless of hypercall page Vitaly Kuznetsov
@ 2020-03-05 13:57 ` Jon Doron
0 siblings, 0 replies; 5+ messages in thread
From: Jon Doron @ 2020-03-05 13:57 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: kvm, linux-hyperv
On Wed, Mar 4, 2020 at 3:58 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Jon Doron <arilou@gmail.com> writes:
>
> > 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.
> >
> > To resolve this issue simply enable hypercalls if the guest_os_id is
> > not 0.
> >
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> > arch/x86/kvm/hyperv.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 13176ec23496..7ec962d433af 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -1615,7 +1615,7 @@ 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;
> > + return READ_ONCE(kvm->arch.hyperv.hv_guest_os_id) != 0;
> > }
> >
> > static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
>
> I would've enabled it in both cases,
>
> return (READ_ONCE(kvm->arch.hyperv.hv_hypercall) &
> HV_X64_MSR_HYPERCALL_ENABLE) || (READ_ONCE(kvm->arch.hyperv.hv_guest_os_id) != 0);
>
> to be safe. We can also check what genuine Hyper-V does but I bet it has
> hypercalls always enabled. Also, the function can be made inline,
> there's a single caller.
I dont have any Hyper-V setup at the moment to validate this, i
believe your hunch is correct but ill do the
implementation you have suggested.
>
> --
> Vitaly
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 3/3] x86/kvm/hyper-v: Add support for synthetic debugger via hypercalls
[not found] ` <87mu8wdxtt.fsf@vitty.brq.redhat.com>
@ 2020-03-05 13:58 ` Jon Doron
0 siblings, 0 replies; 5+ messages in thread
From: Jon Doron @ 2020-03-05 13:58 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: kvm, linux-hyperv
On Wed, Mar 4, 2020 at 4:00 PM Vitaly Kuznetsov <vkuznets@redhat.com> 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 | 11 +++++++++++
> > 2 files changed, 16 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)
> > +
>
> BIT(2) of what? :-) Also, you don't seem to use this define anywhere.
>
Will use it now :) and it's a syndbg MSR option bit
> > /* 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 7ec962d433af..593e0f3f4dba 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -1794,6 +1794,17 @@ 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:
> > + 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-05 13:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200303130356.50405-1-arilou@gmail.com>
[not found] ` <20200303130356.50405-2-arilou@gmail.com>
2020-03-04 13:51 ` [PATCH v1 1/3] x86/kvm/hyper-v: Add support for synthetic debugger capability Vitaly Kuznetsov
2020-03-05 13:56 ` Jon Doron
[not found] ` <20200303130356.50405-3-arilou@gmail.com>
2020-03-04 13:58 ` [PATCH v1 2/3] x86/kvm/hyper-v: enable hypercalls regardless of hypercall page Vitaly Kuznetsov
2020-03-05 13:57 ` Jon Doron
[not found] ` <20200303130356.50405-4-arilou@gmail.com>
[not found] ` <87mu8wdxtt.fsf@vitty.brq.redhat.com>
2020-03-05 13:58 ` [PATCH v1 3/3] x86/kvm/hyper-v: Add support for synthetic debugger via hypercalls Jon Doron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).