* Re: [PATCH v9 4/6] x86/kvm/hyper-v: Add support for synthetic debugger capability
From: Vitaly Kuznetsov @ 2020-03-23 10:36 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv
In-Reply-To: <20200320172839.1144395-5-arilou@gmail.com>
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.
>
This commit seem to break tests, e.g.
# ./tools/testing/selftests/kvm/x86_64/state_test
==== Test Assertion Failure ====
lib/x86_64/processor.c:1023: r == nmsrs
pid=26436 tid=26436 - Argument list too long
1 0x000000000040a87f: vcpu_save_state at processor.c:1022 (discriminator 3)
2 0x00000000004011b3: main at state_test.c:171 (discriminator 4)
3 0x00007f0315cc63d4: ?? ??:0
4 0x0000000000401357: _start at ??:?
Unexpected result from KVM_GET_MSRS, r: 60 (failed MSR was 0x400000ff)
The problem is that the newly introduced MSRs don't allow
reading/writing by the host when KVM_CAP_HYPERV_SYNDBG is disabled, I
think we'll need to relax the requirement and allow at reading/writing by
the host. Something like
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index f17156d36419..ca1f10498705 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -301,11 +301,11 @@ static void syndbg_exit(struct kvm_vcpu *vcpu, u32 msr)
kvm_make_request(KVM_REQ_HV_EXIT, vcpu);
}
-static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
+static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
{
struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
- if (!syndbg->active)
+ if (!syndbg->active && !host)
return 1;
trace_kvm_hv_syndbg_set_msr(vcpu->vcpu_id,
@@ -313,7 +313,8 @@ static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
switch (msr) {
case HV_X64_MSR_SYNDBG_CONTROL:
syndbg->control.control = data;
- syndbg_exit(vcpu, msr);
+ if (!host)
+ syndbg_exit(vcpu, msr);
break;
case HV_X64_MSR_SYNDBG_STATUS:
syndbg->control.status = data;
@@ -326,7 +327,8 @@ static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
break;
case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
syndbg->control.pending_page = data;
- syndbg_exit(vcpu, msr);
+ if (!host)
+ syndbg_exit(vcpu, msr);
break;
case HV_X64_MSR_SYNDBG_OPTIONS:
syndbg->options = data;
@@ -338,11 +340,11 @@ static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
return 0;
}
-static int syndbg_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
+static int syndbg_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
{
struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
- if (!syndbg->active)
+ if (!syndbg->active && !host)
return 1;
switch (msr) {
@@ -1174,7 +1176,7 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
break;
case HV_X64_MSR_SYNDBG_OPTIONS:
case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
- return syndbg_set_msr(vcpu, msr, data);
+ return syndbg_set_msr(vcpu, msr, data, host);
default:
vcpu_unimpl(vcpu, "Hyper-V unhandled wrmsr: 0x%x data 0x%llx\n",
msr, data);
@@ -1304,7 +1306,7 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
return 0;
}
-static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
+static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
{
u64 data = 0;
struct kvm *kvm = vcpu->kvm;
@@ -1343,7 +1345,7 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
break;
case HV_X64_MSR_SYNDBG_OPTIONS:
case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
- return syndbg_get_msr(vcpu, msr, pdata);
+ return syndbg_get_msr(vcpu, msr, pdata, host);
default:
vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
return 1;
@@ -1433,7 +1435,7 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
int r;
mutex_lock(&vcpu->kvm->arch.hyperv.hv_lock);
- r = kvm_hv_get_msr_pw(vcpu, msr, pdata);
+ r = kvm_hv_get_msr_pw(vcpu, msr, pdata, host);
mutex_unlock(&vcpu->kvm->arch.hyperv.hv_lock);
return r;
} else
Note that I also omit syndbg_exit() when
HV_X64_MSR_SYNDBG_PENDING_BUFFER/HV_X64_MSR_SYNDBG_CONTROL are
written by the host to not get a self-induced exit to userspace, this is
likely not what we want on e.g. migration. Without it, we will fail
again:
==== Test Assertion Failure ====
x86_64/state_test.c:151: run->exit_reason == KVM_EXIT_IO
pid=7827 tid=7827 - Argument list too long
1 0x0000000000401109: main at state_test.c:148
2 0x00007ff5308d63d4: ?? ??:0
3 0x0000000000401357: _start at ??:?
Stage 2: unexpected exit reason: 27 (Unknown),
Alternatively, we may introduce a way to hide unsupported MSRs from
KVM_GET_MSRS but we don't do it for existing Hyper-V MSRs as of yet.
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> Documentation/virt/kvm/api.rst | 16 ++++
> arch/x86/include/asm/kvm_host.h | 14 +++
> arch/x86/kvm/hyperv.c | 158 +++++++++++++++++++++++++++++++-
> arch/x86/kvm/hyperv.h | 6 ++
> arch/x86/kvm/trace.h | 51 +++++++++++
> arch/x86/kvm/x86.c | 13 +++
> include/uapi/linux/kvm.h | 11 +++
> 7 files changed, 268 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 4872c47bbcff..fe992dcf4f93 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5024,6 +5024,7 @@ EOI was received.
> struct kvm_hyperv_exit {
> #define KVM_EXIT_HYPERV_SYNIC 1
> #define KVM_EXIT_HYPERV_HCALL 2
> + #define KVM_EXIT_HYPERV_SYNDBG 3
> __u32 type;
> __u32 pad1;
> union {
> @@ -5039,6 +5040,15 @@ EOI was received.
> __u64 result;
> __u64 params[2];
> } hcall;
> + struct {
> + __u32 msr;
> + __u32 pad2;
> + __u64 control;
> + __u64 status;
> + __u64 send_page;
> + __u64 recv_page;
> + __u64 pending_page;
> + } syndbg;
> } u;
> };
> /* KVM_EXIT_HYPERV */
> @@ -5055,6 +5065,12 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
> event/message pages and to enable/disable SynIC messages/events processing
> in userspace.
>
> + - KVM_EXIT_HYPERV_SYNDBG -- synchronously notify user-space about
> +
> +Hyper-V Synthetic debugger state change. Notification is used to either update
> +the pending_page location or to send a control command (send the buffer located
> +in send_page or recv a buffer to recv_page).
> +
> ::
>
> /* KVM_EXIT_ARM_NISV */
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 98959e8cd448..c09fa7401b13 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -854,6 +854,19 @@ struct kvm_apic_map {
> struct kvm_lapic *phys_map[];
> };
>
> +/* Hyper-V synthetic debugger (SynDbg)*/
> +struct kvm_hv_syndbg {
> + struct {
> + u64 control;
> + u64 status;
> + u64 send_page;
> + u64 recv_page;
> + u64 pending_page;
> + } control;
> + u64 options;
> + bool active;
> +};
> +
> /* Hyper-V emulation context */
> struct kvm_hv {
> struct mutex hv_lock;
> @@ -877,6 +890,7 @@ struct kvm_hv {
> atomic_t num_mismatched_vp_indexes;
>
> struct hv_partition_assist_pg *hv_pa_pg;
> + struct kvm_hv_syndbg hv_syndbg;
> };
>
> enum kvm_irqchip_mode {
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 7383c7e7d4af..cd8d0142a841 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -266,6 +266,115 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
> return ret;
> }
>
> +void kvm_hv_activate_syndbg(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> +
> + syndbg->active = true;
> +}
> +
> +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.control.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.control;
> + hv_vcpu->exit.u.syndbg.send_page = syndbg->control.send_page;
> + hv_vcpu->exit.u.syndbg.recv_page = syndbg->control.recv_page;
> + hv_vcpu->exit.u.syndbg.pending_page = syndbg->control.pending_page;
> + vcpu->arch.complete_userspace_io =
> + kvm_hv_syndbg_complete_userspace;
> +
> + kvm_make_request(KVM_REQ_HV_EXIT, vcpu);
> +}
> +
> +static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> +{
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> +
> + if (!syndbg->active)
> + return 1;
> +
> + trace_kvm_hv_syndbg_set_msr(vcpu->vcpu_id,
> + vcpu_to_hv_vcpu(vcpu)->vp_index, msr, data);
> + switch (msr) {
> + case HV_X64_MSR_SYNDBG_CONTROL:
> + syndbg->control.control = data;
> + syndbg_exit(vcpu, msr);
> + break;
> + case HV_X64_MSR_SYNDBG_STATUS:
> + syndbg->control.status = data;
> + break;
> + case HV_X64_MSR_SYNDBG_SEND_BUFFER:
> + syndbg->control.send_page = data;
> + break;
> + case HV_X64_MSR_SYNDBG_RECV_BUFFER:
> + syndbg->control.recv_page = data;
> + break;
> + case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + syndbg->control.pending_page = data;
> + syndbg_exit(vcpu, msr);
> + break;
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + syndbg->options = data;
> + break;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int syndbg_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> +{
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> +
> + if (!syndbg->active)
> + return 1;
> +
> + switch (msr) {
> + case HV_X64_MSR_SYNDBG_CONTROL:
> + *pdata = syndbg->control.control;
> + break;
> + case HV_X64_MSR_SYNDBG_STATUS:
> + *pdata = syndbg->control.status;
> + break;
> + case HV_X64_MSR_SYNDBG_SEND_BUFFER:
> + *pdata = syndbg->control.send_page;
> + break;
> + case HV_X64_MSR_SYNDBG_RECV_BUFFER:
> + *pdata = syndbg->control.recv_page;
> + break;
> + case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + *pdata = syndbg->control.pending_page;
> + break;
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + *pdata = syndbg->options;
> + break;
> + default:
> + break;
> + }
> +
> + trace_kvm_hv_syndbg_get_msr(vcpu->vcpu_id,
> + vcpu_to_hv_vcpu(vcpu)->vp_index, msr,
> + *pdata);
> +
> + return 0;
> +}
> +
> static int synic_get_msr(struct kvm_vcpu_hv_synic *synic, u32 msr, u64 *pdata,
> bool host)
> {
> @@ -800,6 +909,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 +1172,9 @@ 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:
> + 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 +1341,9 @@ 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:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + return syndbg_get_msr(vcpu, msr, pdata);
> default:
> vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
> return 1;
> @@ -1799,9 +1916,16 @@ static struct kvm_cpuid_entry2 evmcs_cpuid_entries[] = {
> { .function = HYPERV_CPUID_NESTED_FEATURES },
> };
>
> +static struct kvm_cpuid_entry2 syndbg_cpuid_entries[] = {
> + { .function = HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS },
> + { .function = HYPERV_CPUID_SYNDBG_INTERFACE },
> + { .function = HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES },
> +};
> +
> #define HV_MAX_CPUID_ENTRIES \
> ARRAY_SIZE(core_cpuid_entries) +\
> - ARRAY_SIZE(evmcs_cpuid_entries)
> + ARRAY_SIZE(evmcs_cpuid_entries) +\
> + ARRAY_SIZE(syndbg_cpuid_entries)
>
> int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> struct kvm_cpuid_entry2 __user *entries)
> @@ -1809,6 +1933,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> uint16_t evmcs_ver = 0;
> struct kvm_cpuid_entry2 cpuid_entries[HV_MAX_CPUID_ENTRIES];
> int i, nent = 0;
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
>
> /* Set the core cpuid entries required for Hyper-V */
> memcpy(&cpuid_entries[nent], &core_cpuid_entries,
> @@ -1825,6 +1950,13 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> nent += ARRAY_SIZE(evmcs_cpuid_entries);
> }
>
> + if (syndbg->active) {
> + /* Syndbg is enabled, add the required Syndbg CPUID leafs */
> + memcpy(&cpuid_entries[nent], &syndbg_cpuid_entries,
> + sizeof(syndbg_cpuid_entries));
> + nent += ARRAY_SIZE(syndbg_cpuid_entries);
> + }
> +
> if (cpuid->nent < nent)
> return -E2BIG;
>
> @@ -1878,6 +2010,12 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> ent->edx |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE;
> ent->edx |= HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE;
>
> + if (syndbg->active) {
> + ent->ebx |= HV_X64_DEBUGGING;
> + ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE;
> + ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE;
> + }
> +
> /*
> * Direct Synthetic timers only make sense with in-kernel
> * LAPIC
> @@ -1921,6 +2059,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, "Linux KVM Hv", 12);
> +
> + 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 5e4780bf6dd7..45aaaf08bf15 100644
> --- a/arch/x86/kvm/hyperv.h
> +++ b/arch/x86/kvm/hyperv.h
> @@ -73,6 +73,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);
>
> @@ -83,6 +88,7 @@ void kvm_hv_irq_routing_update(struct kvm *kvm);
> int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint);
> void kvm_hv_synic_send_eoi(struct kvm_vcpu *vcpu, int vector);
> int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages);
> +void kvm_hv_activate_syndbg(struct kvm_vcpu *vcpu);
>
> void kvm_hv_vcpu_init(struct kvm_vcpu *vcpu);
> void kvm_hv_vcpu_postcreate(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index f194dd058470..bf6c3852868d 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -1515,6 +1515,57 @@ 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 vp_index, u32 msr, u64 data),
> + TP_ARGS(vcpu_id, vp_index, msr, data),
> +
> + TP_STRUCT__entry(
> + __field(int, vcpu_id)
> + __field(u32, vp_index)
> + __field(u32, msr)
> + __field(u64, data)
> + ),
> +
> + TP_fast_assign(
> + __entry->vcpu_id = vcpu_id;
> + __entry->vp_index = vp_index;
> + __entry->msr = msr;
> + __entry->data = data;
> + ),
> +
> + TP_printk("vcpu_id %d vp_index %u msr 0x%x data 0x%llx",
> + __entry->vcpu_id, __entry->vp_index, __entry->msr,
> + __entry->data)
> +);
> +
> +/*
> + * Tracepoint for syndbg_get_msr.
> + */
> +TRACE_EVENT(kvm_hv_syndbg_get_msr,
> + TP_PROTO(int vcpu_id, u32 vp_index, u32 msr, u64 data),
> + TP_ARGS(vcpu_id, vp_index, msr, data),
> +
> + TP_STRUCT__entry(
> + __field(int, vcpu_id)
> + __field(u32, vp_index)
> + __field(u32, msr)
> + __field(u64, data)
> + ),
> +
> + TP_fast_assign(
> + __entry->vcpu_id = vcpu_id;
> + __entry->vp_index = vp_index;
> + __entry->msr = msr;
> + __entry->data = data;
> + ),
> +
> + TP_printk("vcpu_id %d vp_index %u msr 0x%x data 0x%llx",
> + __entry->vcpu_id, __entry->vp_index, __entry->msr,
> + __entry->data)
> +);
> #endif /* _TRACE_KVM_H */
>
> #undef TRACE_INCLUDE_PATH
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3156e25b0774..7bfa30d7a525 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:
> @@ -3323,6 +3331,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_HYPERV_TLBFLUSH:
> case KVM_CAP_HYPERV_SEND_IPI:
> case KVM_CAP_HYPERV_CPUID:
> + case KVM_CAP_HYPERV_SYNDBG:
> case KVM_CAP_PCI_SEGMENT:
> case KVM_CAP_DEBUGREGS:
> case KVM_CAP_X86_ROBUST_SINGLESTEP:
> @@ -4178,6 +4187,10 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
> return -EINVAL;
>
> switch (cap->cap) {
> + case KVM_CAP_HYPERV_SYNDBG:
> + kvm_hv_activate_syndbg(vcpu);
> + return 0;
> +
> case KVM_CAP_HYPERV_SYNIC2:
> if (cap->args[0])
> return -EINVAL;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 7ee0ddc4c457..0b573e1155c8 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;
> __u32 pad1;
> union {
> @@ -203,6 +204,15 @@ struct kvm_hyperv_exit {
> __u64 result;
> __u64 params[2];
> } hcall;
> + struct {
> + __u32 msr;
> + __u32 pad2;
> + __u64 control;
> + __u64 status;
> + __u64 send_page;
> + __u64 recv_page;
> + __u64 pending_page;
> + } syndbg;
> } u;
> };
>
> @@ -1012,6 +1022,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_ARM_NISV_TO_USER 177
> #define KVM_CAP_ARM_INJECT_EXT_DABT 178
> #define KVM_CAP_S390_VCPU_RESETS 179
> +#define KVM_CAP_HYPERV_SYNDBG 180
>
> #ifdef KVM_CAP_IRQ_ROUTING
--
Vitaly
^ permalink raw reply related
* Re: [PATCH v9 3/6] x86/hyper-v: Add synthetic debugger definitions
From: Vitaly Kuznetsov @ 2020-03-23 10:17 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv
In-Reply-To: <20200320172839.1144395-4-arilou@gmail.com>
Jon Doron <arilou@gmail.com> writes:
> Hyper-V synthetic debugger has two modes, one that uses MSRs and
> the other that use Hypercalls.
>
> Add all the required definitions to both types of synthetic debugger
> interface.
>
> Some of the required new CPUIDs and MSRs are not documented in the TLFS
> so they are in hyperv.h instead.
>
> The reason they are not documented is because they are subjected to be
> removed in future versions of Windows.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
You seem to have lost Michael's R-b tag from v8
> ---
> arch/x86/include/asm/hyperv-tlfs.h | 6 ++++++
> arch/x86/kvm/hyperv.h | 27 +++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 92abc1e42bfc..671ce2a39d4b 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -131,6 +131,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)
>
> @@ -376,6 +378,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
>
> @@ -419,6 +424,7 @@ enum HV_GENERIC_SET_FORMAT {
> #define HV_STATUS_INVALID_HYPERCALL_INPUT 3
> #define HV_STATUS_INVALID_ALIGNMENT 4
> #define HV_STATUS_INVALID_PARAMETER 5
> +#define HV_STATUS_OPERATION_DENIED 8
> #define HV_STATUS_INSUFFICIENT_MEMORY 11
> #define HV_STATUS_INVALID_PORT_ID 17
> #define HV_STATUS_INVALID_CONNECTION_ID 18
> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> index 757cb578101c..5e4780bf6dd7 100644
> --- a/arch/x86/kvm/hyperv.h
> +++ b/arch/x86/kvm/hyperv.h
> @@ -23,6 +23,33 @@
>
> #include <linux/kvm_host.h>
>
> +/*
> + * The #defines related to the synthetic debugger are required by KDNet, but
> + * they are not documented in the Hyper-V TLFS because the synthetic debugger
> + * functionality has been deprecated and is subject to removal in future versions
> + * of Windows.
> + */
> +#define HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS 0x40000080
> +#define HYPERV_CPUID_SYNDBG_INTERFACE 0x40000081
> +#define HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES 0x40000082
> +
> +/*
> + * Hyper-V synthetic debugger platform capabilities
> + * These are HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES.EAX bits.
> + */
> +#define HV_X64_SYNDBG_CAP_ALLOW_KERNEL_DEBUGGING BIT(1)
> +
> +/* 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 HV_X64_MSR_SYNDBG_OPTIONS bits */
> +#define HV_X64_SYNDBG_OPTION_USE_HCALLS BIT(2)
> +
> static inline struct kvm_vcpu_hv *vcpu_to_hv_vcpu(struct kvm_vcpu *vcpu)
> {
> return &vcpu->arch.hyperv;
--
Vitaly
^ permalink raw reply
* Re: [PATCH v9 2/6] x86/kvm/hyper-v: Simplify addition for custom cpuid leafs
From: Vitaly Kuznetsov @ 2020-03-23 10:16 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv
In-Reply-To: <20200320172839.1144395-3-arilou@gmail.com>
Jon Doron <arilou@gmail.com> writes:
> Simlify the code to define a new cpuid leaf group by enabled feature.
>
> This also fixes a bug in which the max cpuid leaf was always set to
> HYPERV_CPUID_NESTED_FEATURES regardless if nesting is supported or not.
>
> Any new CPUID group needs to consider the max leaf and be added in the
> correct order, in this method there are two rules:
> 1. Each cpuid leaf group must be order in an ascending order
> 2. The appending for the cpuid leafs by features also needs to happen by
> ascending order.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> arch/x86/kvm/hyperv.c | 46 ++++++++++++++++++++++++++++++-------------
> 1 file changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index a86fda7a1d03..7383c7e7d4af 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1785,27 +1785,45 @@ int kvm_vm_ioctl_hv_eventfd(struct kvm *kvm, struct kvm_hyperv_eventfd *args)
> return kvm_hv_eventfd_assign(kvm, args->conn_id, args->fd);
> }
>
> +// Must be sorted in ascending order by function
scripts/checkpatch.pl should've complained here, kernel coding style
always requires /* */
> +static struct kvm_cpuid_entry2 core_cpuid_entries[] = {
> + { .function = HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS },
> + { .function = HYPERV_CPUID_INTERFACE },
> + { .function = HYPERV_CPUID_VERSION },
> + { .function = HYPERV_CPUID_FEATURES },
> + { .function = HYPERV_CPUID_ENLIGHTMENT_INFO },
> + { .function = HYPERV_CPUID_IMPLEMENT_LIMITS },
> +};
> +
> +static struct kvm_cpuid_entry2 evmcs_cpuid_entries[] = {
> + { .function = HYPERV_CPUID_NESTED_FEATURES },
> +};
> +
> +#define HV_MAX_CPUID_ENTRIES \
> + ARRAY_SIZE(core_cpuid_entries) +\
> + ARRAY_SIZE(evmcs_cpuid_entries)
> +
> int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> struct kvm_cpuid_entry2 __user *entries)
> {
> uint16_t evmcs_ver = 0;
> - struct kvm_cpuid_entry2 cpuid_entries[] = {
> - { .function = HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS },
> - { .function = HYPERV_CPUID_INTERFACE },
> - { .function = HYPERV_CPUID_VERSION },
> - { .function = HYPERV_CPUID_FEATURES },
> - { .function = HYPERV_CPUID_ENLIGHTMENT_INFO },
> - { .function = HYPERV_CPUID_IMPLEMENT_LIMITS },
> - { .function = HYPERV_CPUID_NESTED_FEATURES },
> - };
> - int i, nent = ARRAY_SIZE(cpuid_entries);
> + struct kvm_cpuid_entry2 cpuid_entries[HV_MAX_CPUID_ENTRIES];
> + int i, nent = 0;
> +
> + /* Set the core cpuid entries required for Hyper-V */
> + memcpy(&cpuid_entries[nent], &core_cpuid_entries,
> + sizeof(core_cpuid_entries));
> + nent += ARRAY_SIZE(core_cpuid_entries);
Strictly speaking "+=" is not needed here as nent is zero.
>
> if (kvm_x86_ops->nested_get_evmcs_version)
> evmcs_ver = kvm_x86_ops->nested_get_evmcs_version(vcpu);
>
> - /* Skip NESTED_FEATURES if eVMCS is not supported */
> - if (!evmcs_ver)
> - --nent;
> + if (evmcs_ver) {
> + /* EVMCS is enabled, add the required EVMCS CPUID leafs */
> + memcpy(&cpuid_entries[nent], &evmcs_cpuid_entries,
> + sizeof(evmcs_cpuid_entries));
> + nent += ARRAY_SIZE(evmcs_cpuid_entries);
> + }
>
> if (cpuid->nent < nent)
> return -E2BIG;
> @@ -1821,7 +1839,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> case HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS:
> memcpy(signature, "Linux KVM Hv", 12);
>
> - ent->eax = HYPERV_CPUID_NESTED_FEATURES;
> + ent->eax = cpuid_entries[nent - 1].function;
> ent->ebx = signature[0];
> ent->ecx = signature[1];
> ent->edx = signature[2];
With the nitpicks mentioned above:
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
--
Vitaly
^ permalink raw reply
* Re: [PATCH v9 1/6] x86/kvm/hyper-v: Explicitly align hcall param for kvm_hyperv_exit
From: Vitaly Kuznetsov @ 2020-03-23 9:53 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv
In-Reply-To: <20200320172839.1144395-2-arilou@gmail.com>
Jon Doron <arilou@gmail.com> writes:
> The problem the patch is trying to address is the fact that 'struct
> kvm_hyperv_exit' has different layout on when compiling in 32 and 64 bit
> modes.
>
> In 64-bit mode the default alignment boundary is 64 bits thus
> forcing extra gaps after 'type' and 'msr' but in 32-bit mode the
> boundary is at 32 bits thus no extra gaps.
>
> This is an issue as even when the kernel is 64 bit, the userspace using
> the interface can be both 32 and 64 bit but the same 32 bit userspace has
> to work with 32 bit kernel.
>
> The issue is fixed by forcing the 64 bit layout, this leads to ABI
> change for 32 bit builds and while we are obviously breaking '32 bit
> userspace with 32 bit kernel' case, we're fixing the '32 bit userspace
> with 64 bit kernel' one.
>
> As the interface has no (known) users and 32 bit KVM is rather baroque
> nowadays, this seems like a reasonable decision.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> Documentation/virt/kvm/api.rst | 2 ++
> include/uapi/linux/kvm.h | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index ebd383fba939..4872c47bbcff 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5025,9 +5025,11 @@ EOI was received.
> #define KVM_EXIT_HYPERV_SYNIC 1
> #define KVM_EXIT_HYPERV_HCALL 2
> __u32 type;
> + __u32 pad1;
> union {
> struct {
> __u32 msr;
> + __u32 pad2;
> __u64 control;
> __u64 evt_page;
> __u64 msg_page;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4b95f9a31a2f..7ee0ddc4c457 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -189,9 +189,11 @@ struct kvm_hyperv_exit {
> #define KVM_EXIT_HYPERV_SYNIC 1
> #define KVM_EXIT_HYPERV_HCALL 2
> __u32 type;
> + __u32 pad1;
> union {
> struct {
> __u32 msr;
> + __u32 pad2;
> __u64 control;
> __u64 evt_page;
> __u64 msg_page;
Already said that on v8 but the tag got lost,
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
--
Vitaly
^ permalink raw reply
* [PATCH v9 7/6] KVM: selftests: update hyperv_cpuid with SynDBG tests
From: Vitaly Kuznetsov @ 2020-03-23 9:51 UTC (permalink / raw)
To: kvm; +Cc: linux-hyperv, Jon Doron
In-Reply-To: <20200320172839.1144395-1-arilou@gmail.com>
Test all four combinations with eVMCS and SynDBG capabilities,
check that we get the right number of entries and that
0x40000000.EAX always returns the correct max leaf.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
.../selftests/kvm/x86_64/hyperv_cpuid.c | 138 ++++++++++++------
1 file changed, 96 insertions(+), 42 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
index 83323f3d7ca0..4a2ef7e1b1f9 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
@@ -26,18 +26,18 @@ static void guest_code(void)
{
}
-static int smt_possible(void)
+static bool smt_possible(void)
{
char buf[16];
FILE *f;
- bool res = 1;
+ bool res = true;
f = fopen("/sys/devices/system/cpu/smt/control", "r");
if (f) {
if (fread(buf, sizeof(*buf), sizeof(buf), f) > 0) {
if (!strncmp(buf, "forceoff", 8) ||
!strncmp(buf, "notsupported", 12))
- res = 0;
+ res = false;
}
fclose(f);
}
@@ -45,30 +45,48 @@ static int smt_possible(void)
return res;
}
+void vcpu_enable_syndbg(struct kvm_vm *vm, int vcpu_id)
+{
+ struct kvm_enable_cap enable_syndbg_cap = {
+ .cap = KVM_CAP_HYPERV_SYNDBG,
+ };
+
+ vcpu_ioctl(vm, vcpu_id, KVM_ENABLE_CAP, &enable_syndbg_cap);
+}
+
static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries,
- int evmcs_enabled)
+ bool evmcs_enabled, bool syndbg_enabled)
{
int i;
+ int nent = 6;
+ u32 test_val;
+
+ if (evmcs_enabled)
+ nent += 1; /* 0x4000000A */
- if (!evmcs_enabled)
- TEST_ASSERT(hv_cpuid_entries->nent == 6,
- "KVM_GET_SUPPORTED_HV_CPUID should return 6 entries"
- " when Enlightened VMCS is disabled (returned %d)",
- hv_cpuid_entries->nent);
- else
- TEST_ASSERT(hv_cpuid_entries->nent == 7,
- "KVM_GET_SUPPORTED_HV_CPUID should return 7 entries"
- " when Enlightened VMCS is enabled (returned %d)",
- hv_cpuid_entries->nent);
+ if (syndbg_enabled)
+ nent += 3; /* 0x40000080 - 0x40000082 */
+
+ TEST_ASSERT(hv_cpuid_entries->nent == nent,
+ "KVM_GET_SUPPORTED_HV_CPUID should return %d entries"
+ " with evmcs=%d syndbg=%d (returned %d)",
+ nent, evmcs_enabled, syndbg_enabled,
+ hv_cpuid_entries->nent);
for (i = 0; i < hv_cpuid_entries->nent; i++) {
struct kvm_cpuid_entry2 *entry = &hv_cpuid_entries->entries[i];
TEST_ASSERT((entry->function >= 0x40000000) &&
- (entry->function <= 0x4000000A),
+ (entry->function <= 0x40000082),
"function %x is our of supported range",
entry->function);
+ TEST_ASSERT(evmcs_enabled || (entry->function != 0x4000000A),
+ "0x4000000A leaf should not be reported");
+
+ TEST_ASSERT(syndbg_enabled || (entry->function <= 0x4000000A),
+ "SYNDBG leaves should not be reported");
+
TEST_ASSERT(entry->index == 0,
".index field should be zero");
@@ -78,12 +96,27 @@ static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries,
TEST_ASSERT(!entry->padding[0] && !entry->padding[1] &&
!entry->padding[2], "padding should be zero");
- if (entry->function == 0x40000004) {
- int nononarchcs = !!(entry->eax & (1UL << 18));
-
- TEST_ASSERT(nononarchcs == !smt_possible(),
+ switch (entry->function) {
+ case 0x40000000:
+ test_val = 0x40000005;
+ if (evmcs_enabled)
+ test_val = 0x4000000A;
+ if (syndbg_enabled)
+ test_val = 0x40000082;
+
+ TEST_ASSERT(entry->eax == test_val,
+ "Wrong max leaf report in 0x40000000.EAX: %x"
+ " (evmcs=%d syndbg=%d)",
+ entry->eax, evmcs_enabled, syndbg_enabled
+ );
+ break;
+ case 0x40000004:
+ test_val = entry->eax & (1UL << 18);
+
+ TEST_ASSERT(!!test_val == !smt_possible(),
"NoNonArchitecturalCoreSharing bit"
" doesn't reflect SMT setting");
+ break;
}
/*
@@ -133,8 +166,9 @@ struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(struct kvm_vm *vm)
int main(int argc, char *argv[])
{
struct kvm_vm *vm;
- int rv;
+ int rv, stage;
struct kvm_cpuid2 *hv_cpuid_entries;
+ bool evmcs_enabled, syndbg_enabled;
/* Tell stdout not to buffer its content */
setbuf(stdout, NULL);
@@ -150,31 +184,51 @@ int main(int argc, char *argv[])
test_hv_cpuid_e2big(vm);
- hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm);
- if (!hv_cpuid_entries)
- return 1;
-
- test_hv_cpuid(hv_cpuid_entries, 0);
+ kvm_vm_free(vm);
- free(hv_cpuid_entries);
+ for (stage = 0; stage < 5; stage++) {
+ evmcs_enabled = false;
+ syndbg_enabled = false;
+
+ vm = vm_create_default(VCPU_ID, 0, guest_code);
+ switch (stage) {
+ case 0:
+ test_hv_cpuid_e2big(vm);
+ continue;
+ case 1:
+ break;
+ case 2:
+ if (!kvm_check_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS)) {
+ print_skip("Enlightened VMCS is unsupported");
+ continue;
+ }
+ vcpu_enable_evmcs(vm, VCPU_ID);
+ evmcs_enabled = true;
+ break;
+ case 3:
+ if (!kvm_check_cap(KVM_CAP_HYPERV_SYNDBG)) {
+ print_skip("Synthetic debugger is unsupported");
+ continue;
+ }
+ vcpu_enable_syndbg(vm, VCPU_ID);
+ syndbg_enabled = true;
+ break;
+ case 4:
+ if (!kvm_check_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS) ||
+ !kvm_check_cap(KVM_CAP_HYPERV_SYNDBG))
+ continue;
+ vcpu_enable_evmcs(vm, VCPU_ID);
+ vcpu_enable_syndbg(vm, VCPU_ID);
+ evmcs_enabled = true;
+ syndbg_enabled = true;
+ break;
+ }
- if (!kvm_check_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS)) {
- print_skip("Enlightened VMCS is unsupported");
- goto vm_free;
+ hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm);
+ test_hv_cpuid(hv_cpuid_entries, evmcs_enabled, syndbg_enabled);
+ free(hv_cpuid_entries);
+ kvm_vm_free(vm);
}
- vcpu_enable_evmcs(vm, VCPU_ID);
-
- hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm);
- if (!hv_cpuid_entries)
- return 1;
-
- test_hv_cpuid(hv_cpuid_entries, 1);
-
- free(hv_cpuid_entries);
-
-vm_free:
- kvm_vm_free(vm);
-
return 0;
}
--
2.25.1
^ permalink raw reply related
* Re: [PATCH][next] hv: hyperv_vmbus.h: Replace zero-length array with flexible-array member
From: Wei Liu @ 2020-03-21 16:26 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, linux-kernel
In-Reply-To: <20200319213226.GA9536@embeddedor.com>
On Thu, Mar 19, 2020 at 04:32:26PM -0500, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Queued, thanks.
Wei.
^ permalink raw reply
* [PATCH v9 4/6] x86/kvm/hyper-v: Add support for synthetic debugger capability
From: Jon Doron @ 2020-03-20 17:28 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, Jon Doron
In-Reply-To: <20200320172839.1144395-1-arilou@gmail.com>
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.
Signed-off-by: Jon Doron <arilou@gmail.com>
---
Documentation/virt/kvm/api.rst | 16 ++++
arch/x86/include/asm/kvm_host.h | 14 +++
arch/x86/kvm/hyperv.c | 158 +++++++++++++++++++++++++++++++-
arch/x86/kvm/hyperv.h | 6 ++
arch/x86/kvm/trace.h | 51 +++++++++++
arch/x86/kvm/x86.c | 13 +++
include/uapi/linux/kvm.h | 11 +++
7 files changed, 268 insertions(+), 1 deletion(-)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 4872c47bbcff..fe992dcf4f93 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -5024,6 +5024,7 @@ EOI was received.
struct kvm_hyperv_exit {
#define KVM_EXIT_HYPERV_SYNIC 1
#define KVM_EXIT_HYPERV_HCALL 2
+ #define KVM_EXIT_HYPERV_SYNDBG 3
__u32 type;
__u32 pad1;
union {
@@ -5039,6 +5040,15 @@ EOI was received.
__u64 result;
__u64 params[2];
} hcall;
+ struct {
+ __u32 msr;
+ __u32 pad2;
+ __u64 control;
+ __u64 status;
+ __u64 send_page;
+ __u64 recv_page;
+ __u64 pending_page;
+ } syndbg;
} u;
};
/* KVM_EXIT_HYPERV */
@@ -5055,6 +5065,12 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
event/message pages and to enable/disable SynIC messages/events processing
in userspace.
+ - KVM_EXIT_HYPERV_SYNDBG -- synchronously notify user-space about
+
+Hyper-V Synthetic debugger state change. Notification is used to either update
+the pending_page location or to send a control command (send the buffer located
+in send_page or recv a buffer to recv_page).
+
::
/* KVM_EXIT_ARM_NISV */
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 98959e8cd448..c09fa7401b13 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -854,6 +854,19 @@ struct kvm_apic_map {
struct kvm_lapic *phys_map[];
};
+/* Hyper-V synthetic debugger (SynDbg)*/
+struct kvm_hv_syndbg {
+ struct {
+ u64 control;
+ u64 status;
+ u64 send_page;
+ u64 recv_page;
+ u64 pending_page;
+ } control;
+ u64 options;
+ bool active;
+};
+
/* Hyper-V emulation context */
struct kvm_hv {
struct mutex hv_lock;
@@ -877,6 +890,7 @@ struct kvm_hv {
atomic_t num_mismatched_vp_indexes;
struct hv_partition_assist_pg *hv_pa_pg;
+ struct kvm_hv_syndbg hv_syndbg;
};
enum kvm_irqchip_mode {
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 7383c7e7d4af..cd8d0142a841 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -266,6 +266,115 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
return ret;
}
+void kvm_hv_activate_syndbg(struct kvm_vcpu *vcpu)
+{
+ struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
+
+ syndbg->active = true;
+}
+
+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.control.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.control;
+ hv_vcpu->exit.u.syndbg.send_page = syndbg->control.send_page;
+ hv_vcpu->exit.u.syndbg.recv_page = syndbg->control.recv_page;
+ hv_vcpu->exit.u.syndbg.pending_page = syndbg->control.pending_page;
+ vcpu->arch.complete_userspace_io =
+ kvm_hv_syndbg_complete_userspace;
+
+ kvm_make_request(KVM_REQ_HV_EXIT, vcpu);
+}
+
+static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
+{
+ struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
+
+ if (!syndbg->active)
+ return 1;
+
+ trace_kvm_hv_syndbg_set_msr(vcpu->vcpu_id,
+ vcpu_to_hv_vcpu(vcpu)->vp_index, msr, data);
+ switch (msr) {
+ case HV_X64_MSR_SYNDBG_CONTROL:
+ syndbg->control.control = data;
+ syndbg_exit(vcpu, msr);
+ break;
+ case HV_X64_MSR_SYNDBG_STATUS:
+ syndbg->control.status = data;
+ break;
+ case HV_X64_MSR_SYNDBG_SEND_BUFFER:
+ syndbg->control.send_page = data;
+ break;
+ case HV_X64_MSR_SYNDBG_RECV_BUFFER:
+ syndbg->control.recv_page = data;
+ break;
+ case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
+ syndbg->control.pending_page = data;
+ syndbg_exit(vcpu, msr);
+ break;
+ case HV_X64_MSR_SYNDBG_OPTIONS:
+ syndbg->options = data;
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static int syndbg_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
+{
+ struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
+
+ if (!syndbg->active)
+ return 1;
+
+ switch (msr) {
+ case HV_X64_MSR_SYNDBG_CONTROL:
+ *pdata = syndbg->control.control;
+ break;
+ case HV_X64_MSR_SYNDBG_STATUS:
+ *pdata = syndbg->control.status;
+ break;
+ case HV_X64_MSR_SYNDBG_SEND_BUFFER:
+ *pdata = syndbg->control.send_page;
+ break;
+ case HV_X64_MSR_SYNDBG_RECV_BUFFER:
+ *pdata = syndbg->control.recv_page;
+ break;
+ case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
+ *pdata = syndbg->control.pending_page;
+ break;
+ case HV_X64_MSR_SYNDBG_OPTIONS:
+ *pdata = syndbg->options;
+ break;
+ default:
+ break;
+ }
+
+ trace_kvm_hv_syndbg_get_msr(vcpu->vcpu_id,
+ vcpu_to_hv_vcpu(vcpu)->vp_index, msr,
+ *pdata);
+
+ return 0;
+}
+
static int synic_get_msr(struct kvm_vcpu_hv_synic *synic, u32 msr, u64 *pdata,
bool host)
{
@@ -800,6 +909,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 +1172,9 @@ 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:
+ 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 +1341,9 @@ 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:
+ case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
+ return syndbg_get_msr(vcpu, msr, pdata);
default:
vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
return 1;
@@ -1799,9 +1916,16 @@ static struct kvm_cpuid_entry2 evmcs_cpuid_entries[] = {
{ .function = HYPERV_CPUID_NESTED_FEATURES },
};
+static struct kvm_cpuid_entry2 syndbg_cpuid_entries[] = {
+ { .function = HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS },
+ { .function = HYPERV_CPUID_SYNDBG_INTERFACE },
+ { .function = HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES },
+};
+
#define HV_MAX_CPUID_ENTRIES \
ARRAY_SIZE(core_cpuid_entries) +\
- ARRAY_SIZE(evmcs_cpuid_entries)
+ ARRAY_SIZE(evmcs_cpuid_entries) +\
+ ARRAY_SIZE(syndbg_cpuid_entries)
int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
struct kvm_cpuid_entry2 __user *entries)
@@ -1809,6 +1933,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
uint16_t evmcs_ver = 0;
struct kvm_cpuid_entry2 cpuid_entries[HV_MAX_CPUID_ENTRIES];
int i, nent = 0;
+ struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
/* Set the core cpuid entries required for Hyper-V */
memcpy(&cpuid_entries[nent], &core_cpuid_entries,
@@ -1825,6 +1950,13 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
nent += ARRAY_SIZE(evmcs_cpuid_entries);
}
+ if (syndbg->active) {
+ /* Syndbg is enabled, add the required Syndbg CPUID leafs */
+ memcpy(&cpuid_entries[nent], &syndbg_cpuid_entries,
+ sizeof(syndbg_cpuid_entries));
+ nent += ARRAY_SIZE(syndbg_cpuid_entries);
+ }
+
if (cpuid->nent < nent)
return -E2BIG;
@@ -1878,6 +2010,12 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
ent->edx |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE;
ent->edx |= HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE;
+ if (syndbg->active) {
+ ent->ebx |= HV_X64_DEBUGGING;
+ ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE;
+ ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE;
+ }
+
/*
* Direct Synthetic timers only make sense with in-kernel
* LAPIC
@@ -1921,6 +2059,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, "Linux KVM Hv", 12);
+
+ 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 5e4780bf6dd7..45aaaf08bf15 100644
--- a/arch/x86/kvm/hyperv.h
+++ b/arch/x86/kvm/hyperv.h
@@ -73,6 +73,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);
@@ -83,6 +88,7 @@ void kvm_hv_irq_routing_update(struct kvm *kvm);
int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint);
void kvm_hv_synic_send_eoi(struct kvm_vcpu *vcpu, int vector);
int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages);
+void kvm_hv_activate_syndbg(struct kvm_vcpu *vcpu);
void kvm_hv_vcpu_init(struct kvm_vcpu *vcpu);
void kvm_hv_vcpu_postcreate(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index f194dd058470..bf6c3852868d 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -1515,6 +1515,57 @@ 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 vp_index, u32 msr, u64 data),
+ TP_ARGS(vcpu_id, vp_index, msr, data),
+
+ TP_STRUCT__entry(
+ __field(int, vcpu_id)
+ __field(u32, vp_index)
+ __field(u32, msr)
+ __field(u64, data)
+ ),
+
+ TP_fast_assign(
+ __entry->vcpu_id = vcpu_id;
+ __entry->vp_index = vp_index;
+ __entry->msr = msr;
+ __entry->data = data;
+ ),
+
+ TP_printk("vcpu_id %d vp_index %u msr 0x%x data 0x%llx",
+ __entry->vcpu_id, __entry->vp_index, __entry->msr,
+ __entry->data)
+);
+
+/*
+ * Tracepoint for syndbg_get_msr.
+ */
+TRACE_EVENT(kvm_hv_syndbg_get_msr,
+ TP_PROTO(int vcpu_id, u32 vp_index, u32 msr, u64 data),
+ TP_ARGS(vcpu_id, vp_index, msr, data),
+
+ TP_STRUCT__entry(
+ __field(int, vcpu_id)
+ __field(u32, vp_index)
+ __field(u32, msr)
+ __field(u64, data)
+ ),
+
+ TP_fast_assign(
+ __entry->vcpu_id = vcpu_id;
+ __entry->vp_index = vp_index;
+ __entry->msr = msr;
+ __entry->data = data;
+ ),
+
+ TP_printk("vcpu_id %d vp_index %u msr 0x%x data 0x%llx",
+ __entry->vcpu_id, __entry->vp_index, __entry->msr,
+ __entry->data)
+);
#endif /* _TRACE_KVM_H */
#undef TRACE_INCLUDE_PATH
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3156e25b0774..7bfa30d7a525 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:
@@ -3323,6 +3331,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_HYPERV_TLBFLUSH:
case KVM_CAP_HYPERV_SEND_IPI:
case KVM_CAP_HYPERV_CPUID:
+ case KVM_CAP_HYPERV_SYNDBG:
case KVM_CAP_PCI_SEGMENT:
case KVM_CAP_DEBUGREGS:
case KVM_CAP_X86_ROBUST_SINGLESTEP:
@@ -4178,6 +4187,10 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
return -EINVAL;
switch (cap->cap) {
+ case KVM_CAP_HYPERV_SYNDBG:
+ kvm_hv_activate_syndbg(vcpu);
+ return 0;
+
case KVM_CAP_HYPERV_SYNIC2:
if (cap->args[0])
return -EINVAL;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 7ee0ddc4c457..0b573e1155c8 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;
__u32 pad1;
union {
@@ -203,6 +204,15 @@ struct kvm_hyperv_exit {
__u64 result;
__u64 params[2];
} hcall;
+ struct {
+ __u32 msr;
+ __u32 pad2;
+ __u64 control;
+ __u64 status;
+ __u64 send_page;
+ __u64 recv_page;
+ __u64 pending_page;
+ } syndbg;
} u;
};
@@ -1012,6 +1022,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_ARM_NISV_TO_USER 177
#define KVM_CAP_ARM_INJECT_EXT_DABT 178
#define KVM_CAP_S390_VCPU_RESETS 179
+#define KVM_CAP_HYPERV_SYNDBG 180
#ifdef KVM_CAP_IRQ_ROUTING
--
2.24.1
^ permalink raw reply related
* [PATCH v9 5/6] x86/kvm/hyper-v: enable hypercalls without hypercall page with syndbg
From: Jon Doron @ 2020-03-20 17:28 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, Jon Doron
In-Reply-To: <20200320172839.1144395-1-arilou@gmail.com>
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 also if the guest_os_id
is not 0 and the syndbg feature is enabled.
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 cd8d0142a841..c130a386f4c1 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1647,7 +1647,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);
}
static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
--
2.24.1
^ permalink raw reply related
* [PATCH v9 6/6] x86/kvm/hyper-v: Add support for synthetic debugger via hypercalls
From: Jon Doron @ 2020-03-20 17:28 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, Jon Doron
In-Reply-To: <20200320172839.1144395-1-arilou@gmail.com>
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/kvm/hyperv.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index c130a386f4c1..f17156d36419 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1829,6 +1829,34 @@ 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:
+ if (unlikely(fast)) {
+ ret = HV_STATUS_INVALID_PARAMETER;
+ break;
+ }
+ fallthrough;
+ case HVCALL_RESET_DEBUG_SESSION: {
+ struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
+
+ if (!syndbg->active) {
+ ret = HV_STATUS_INVALID_HYPERCALL_CODE;
+ break;
+ }
+
+ if (!(syndbg->options & HV_X64_SYNDBG_OPTION_USE_HCALLS)) {
+ ret = HV_STATUS_OPERATION_DENIED;
+ 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;
--
2.24.1
^ permalink raw reply related
* [PATCH v9 3/6] x86/hyper-v: Add synthetic debugger definitions
From: Jon Doron @ 2020-03-20 17:28 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, Jon Doron
In-Reply-To: <20200320172839.1144395-1-arilou@gmail.com>
Hyper-V synthetic debugger has two modes, one that uses MSRs and
the other that use Hypercalls.
Add all the required definitions to both types of synthetic debugger
interface.
Some of the required new CPUIDs and MSRs are not documented in the TLFS
so they are in hyperv.h instead.
The reason they are not documented is because they are subjected to be
removed in future versions of Windows.
Signed-off-by: Jon Doron <arilou@gmail.com>
---
arch/x86/include/asm/hyperv-tlfs.h | 6 ++++++
arch/x86/kvm/hyperv.h | 27 +++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 92abc1e42bfc..671ce2a39d4b 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -131,6 +131,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)
@@ -376,6 +378,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
@@ -419,6 +424,7 @@ enum HV_GENERIC_SET_FORMAT {
#define HV_STATUS_INVALID_HYPERCALL_INPUT 3
#define HV_STATUS_INVALID_ALIGNMENT 4
#define HV_STATUS_INVALID_PARAMETER 5
+#define HV_STATUS_OPERATION_DENIED 8
#define HV_STATUS_INSUFFICIENT_MEMORY 11
#define HV_STATUS_INVALID_PORT_ID 17
#define HV_STATUS_INVALID_CONNECTION_ID 18
diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
index 757cb578101c..5e4780bf6dd7 100644
--- a/arch/x86/kvm/hyperv.h
+++ b/arch/x86/kvm/hyperv.h
@@ -23,6 +23,33 @@
#include <linux/kvm_host.h>
+/*
+ * The #defines related to the synthetic debugger are required by KDNet, but
+ * they are not documented in the Hyper-V TLFS because the synthetic debugger
+ * functionality has been deprecated and is subject to removal in future versions
+ * of Windows.
+ */
+#define HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS 0x40000080
+#define HYPERV_CPUID_SYNDBG_INTERFACE 0x40000081
+#define HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES 0x40000082
+
+/*
+ * Hyper-V synthetic debugger platform capabilities
+ * These are HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES.EAX bits.
+ */
+#define HV_X64_SYNDBG_CAP_ALLOW_KERNEL_DEBUGGING BIT(1)
+
+/* 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 HV_X64_MSR_SYNDBG_OPTIONS bits */
+#define HV_X64_SYNDBG_OPTION_USE_HCALLS BIT(2)
+
static inline struct kvm_vcpu_hv *vcpu_to_hv_vcpu(struct kvm_vcpu *vcpu)
{
return &vcpu->arch.hyperv;
--
2.24.1
^ permalink raw reply related
* [PATCH v9 2/6] x86/kvm/hyper-v: Simplify addition for custom cpuid leafs
From: Jon Doron @ 2020-03-20 17:28 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, Jon Doron
In-Reply-To: <20200320172839.1144395-1-arilou@gmail.com>
Simlify the code to define a new cpuid leaf group by enabled feature.
This also fixes a bug in which the max cpuid leaf was always set to
HYPERV_CPUID_NESTED_FEATURES regardless if nesting is supported or not.
Any new CPUID group needs to consider the max leaf and be added in the
correct order, in this method there are two rules:
1. Each cpuid leaf group must be order in an ascending order
2. The appending for the cpuid leafs by features also needs to happen by
ascending order.
Signed-off-by: Jon Doron <arilou@gmail.com>
---
arch/x86/kvm/hyperv.c | 46 ++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index a86fda7a1d03..7383c7e7d4af 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1785,27 +1785,45 @@ int kvm_vm_ioctl_hv_eventfd(struct kvm *kvm, struct kvm_hyperv_eventfd *args)
return kvm_hv_eventfd_assign(kvm, args->conn_id, args->fd);
}
+// Must be sorted in ascending order by function
+static struct kvm_cpuid_entry2 core_cpuid_entries[] = {
+ { .function = HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS },
+ { .function = HYPERV_CPUID_INTERFACE },
+ { .function = HYPERV_CPUID_VERSION },
+ { .function = HYPERV_CPUID_FEATURES },
+ { .function = HYPERV_CPUID_ENLIGHTMENT_INFO },
+ { .function = HYPERV_CPUID_IMPLEMENT_LIMITS },
+};
+
+static struct kvm_cpuid_entry2 evmcs_cpuid_entries[] = {
+ { .function = HYPERV_CPUID_NESTED_FEATURES },
+};
+
+#define HV_MAX_CPUID_ENTRIES \
+ ARRAY_SIZE(core_cpuid_entries) +\
+ ARRAY_SIZE(evmcs_cpuid_entries)
+
int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
struct kvm_cpuid_entry2 __user *entries)
{
uint16_t evmcs_ver = 0;
- struct kvm_cpuid_entry2 cpuid_entries[] = {
- { .function = HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS },
- { .function = HYPERV_CPUID_INTERFACE },
- { .function = HYPERV_CPUID_VERSION },
- { .function = HYPERV_CPUID_FEATURES },
- { .function = HYPERV_CPUID_ENLIGHTMENT_INFO },
- { .function = HYPERV_CPUID_IMPLEMENT_LIMITS },
- { .function = HYPERV_CPUID_NESTED_FEATURES },
- };
- int i, nent = ARRAY_SIZE(cpuid_entries);
+ struct kvm_cpuid_entry2 cpuid_entries[HV_MAX_CPUID_ENTRIES];
+ int i, nent = 0;
+
+ /* Set the core cpuid entries required for Hyper-V */
+ memcpy(&cpuid_entries[nent], &core_cpuid_entries,
+ sizeof(core_cpuid_entries));
+ nent += ARRAY_SIZE(core_cpuid_entries);
if (kvm_x86_ops->nested_get_evmcs_version)
evmcs_ver = kvm_x86_ops->nested_get_evmcs_version(vcpu);
- /* Skip NESTED_FEATURES if eVMCS is not supported */
- if (!evmcs_ver)
- --nent;
+ if (evmcs_ver) {
+ /* EVMCS is enabled, add the required EVMCS CPUID leafs */
+ memcpy(&cpuid_entries[nent], &evmcs_cpuid_entries,
+ sizeof(evmcs_cpuid_entries));
+ nent += ARRAY_SIZE(evmcs_cpuid_entries);
+ }
if (cpuid->nent < nent)
return -E2BIG;
@@ -1821,7 +1839,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
case HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS:
memcpy(signature, "Linux KVM Hv", 12);
- ent->eax = HYPERV_CPUID_NESTED_FEATURES;
+ ent->eax = cpuid_entries[nent - 1].function;
ent->ebx = signature[0];
ent->ecx = signature[1];
ent->edx = signature[2];
--
2.24.1
^ permalink raw reply related
* [PATCH v9 1/6] x86/kvm/hyper-v: Explicitly align hcall param for kvm_hyperv_exit
From: Jon Doron @ 2020-03-20 17:28 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, Jon Doron
In-Reply-To: <20200320172839.1144395-1-arilou@gmail.com>
The problem the patch is trying to address is the fact that 'struct
kvm_hyperv_exit' has different layout on when compiling in 32 and 64 bit
modes.
In 64-bit mode the default alignment boundary is 64 bits thus
forcing extra gaps after 'type' and 'msr' but in 32-bit mode the
boundary is at 32 bits thus no extra gaps.
This is an issue as even when the kernel is 64 bit, the userspace using
the interface can be both 32 and 64 bit but the same 32 bit userspace has
to work with 32 bit kernel.
The issue is fixed by forcing the 64 bit layout, this leads to ABI
change for 32 bit builds and while we are obviously breaking '32 bit
userspace with 32 bit kernel' case, we're fixing the '32 bit userspace
with 64 bit kernel' one.
As the interface has no (known) users and 32 bit KVM is rather baroque
nowadays, this seems like a reasonable decision.
Signed-off-by: Jon Doron <arilou@gmail.com>
---
Documentation/virt/kvm/api.rst | 2 ++
include/uapi/linux/kvm.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index ebd383fba939..4872c47bbcff 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -5025,9 +5025,11 @@ EOI was received.
#define KVM_EXIT_HYPERV_SYNIC 1
#define KVM_EXIT_HYPERV_HCALL 2
__u32 type;
+ __u32 pad1;
union {
struct {
__u32 msr;
+ __u32 pad2;
__u64 control;
__u64 evt_page;
__u64 msg_page;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4b95f9a31a2f..7ee0ddc4c457 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -189,9 +189,11 @@ struct kvm_hyperv_exit {
#define KVM_EXIT_HYPERV_SYNIC 1
#define KVM_EXIT_HYPERV_HCALL 2
__u32 type;
+ __u32 pad1;
union {
struct {
__u32 msr;
+ __u32 pad2;
__u64 control;
__u64 evt_page;
__u64 msg_page;
--
2.24.1
^ permalink raw reply related
* [PATCH v9 0/6] x86/kvm/hyper-v: add support for synthetic debugger
From: Jon Doron @ 2020-03-20 17:28 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, Jon Doron
Add support for the synthetic debugger interface of hyper-v, the
synthetic debugger has 2 modes.
1. Use a set of MSRs to send/recv information (undocumented so it's not
going to the hyperv-tlfs.h)
2. Use hypercalls
The first mode is based the following MSRs:
1. Control/Status MSRs which either asks for a send/recv .
2. Send/Recv MSRs each holds GPA where the send/recv buffers are.
3. Pending MSR, holds a GPA to a PAGE that simply has a boolean that
indicates if there is data pending to issue a recv VMEXIT.
The first mode implementation is to simply exit to user-space when
either the control MSR or the pending MSR are being set.
Then it's up-to userspace to implement the rest of the logic of sending/recving.
In the second mode instead of using MSRs KNet will simply issue
Hypercalls with the information to send/recv, in this mode the data
being transferred is UDP encapsulated, unlike in the previous mode in
which you get just the data to send.
The new hypercalls will exit to userspace which will be incharge of
re-encapsulating if needed the UDP packets to be sent.
There is an issue though in which KDNet does not respect the hypercall
page and simply issues vmcall/vmmcall instructions depending on the cpu
type expecting them to be handled as it a real hypercall was issued.
It's important to note that part of this feature has been subject to be
removed in future versions of Windows, which is why some of the
defintions will not be present the the TLFS but in the kvm hyperv header
instead.
Jon Doron (6):
x86/kvm/hyper-v: Explicitly align hcall param for kvm_hyperv_exit
x86/kvm/hyper-v: Simplify addition for custom cpuid leafs
x86/hyper-v: Add synthetic debugger definitions
x86/kvm/hyper-v: Add support for synthetic debugger capability
x86/kvm/hyper-v: enable hypercalls without hypercall page with syndbg
x86/kvm/hyper-v: Add support for synthetic debugger via hypercalls
Documentation/virt/kvm/api.rst | 18 +++
arch/x86/include/asm/hyperv-tlfs.h | 6 +
arch/x86/include/asm/kvm_host.h | 14 ++
arch/x86/kvm/hyperv.c | 235 +++++++++++++++++++++++++++--
arch/x86/kvm/hyperv.h | 33 ++++
arch/x86/kvm/trace.h | 51 +++++++
arch/x86/kvm/x86.c | 13 ++
include/uapi/linux/kvm.h | 13 ++
8 files changed, 368 insertions(+), 15 deletions(-)
--
2.24.1
^ permalink raw reply
* RE: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages
From: Michael Kelley @ 2020-03-20 17:22 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Marc Zyngier, gregkh, Will Deacon, Ard Biesheuvel,
Catalin Marinas, Mark Rutland, Linux ARM,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-efi, linux-arch, olaf@aepfle.de, Andy Whitcroft, vkuznets,
Jason Wang, marcelo.cerri@canonical.com, KY Srinivasan,
Sunil Muthuswamy, Boqun Feng
In-Reply-To: <CAK8P3a1MfYDTiQ9j0o3tnd=ymZukPoSmuExLhEMRR+GRwVD7xA@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de> Sent: Friday, March 20, 2020 9:28 AM
>
> On Thu, Mar 19, 2020 at 10:43 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:58 AM
> > > On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > > My point was to keep the functions but use alloc_pages() internally,
> > > so you can deal with the hypervisor having a larger page size than
> > > the guest, which seems to be a more important scenario if I correctly
> > > understand the differences between the way Windows and Linux
> > > deal with page cache.
> >
> > OK, I see now what you are getting at. I should spell out my
> > assumption, which is the opposite. Hyper-V won't have a page
> > size other than 4K anytime in the foreseeable future. The
> > code is too wedded to the x86 4K page size, and the host-guest
> > interfaces have a lot of implicit assumptions that the page size is
> > 4K (unfortunately). But the last time I looked, RHEL for ARM64 is
> > delivered with a 64K page size. So my assumption is that the only
> > combination that really matters is the guest page size being larger
> > than the Hyper-V page size. The other way around just won't
> > happen without a major overhaul to Hyper-V, including a rework
> > of the guest-host interface.
>
> Ok, got it. We should really ask Red Hat to change the page size,
> but as long as you care existing systems, and you expect this to
> result in gigabytes of allocation on future systems, having the
> wrapper seems reasonable.
>
> Maybe you could fall back to alloc_page when PAGE_SIZE equals
> HV_HYP_PAGE_SIZE though? I'm not sure what the tradeoff
> between kmalloc and alloc_page is these days, other than the
> feeling that alloc_page is the more obvious choice when you know
> you always want exactly a page here.
>
Yes, that works for me.
Michael
^ permalink raw reply
* Re: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files
From: Arnd Bergmann @ 2020-03-20 16:38 UTC (permalink / raw)
To: Michael Kelley
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <MW2PR2101MB1052686237C57955148F173ED7F40@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Thu, Mar 19, 2020 at 10:31 PM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 3:10 AM
> > Just drop the __packed annotations then, they just confuse the compiler
> > in this case. In particular, when the compiler thinks that a structure is
> > misaligned, it tries to avoid using load/store instructions on it that are
> > inefficient or trap with misaligned code, so having default alignment
> > produces better object code.
>
> So I'm confused a bit. Were the original concerns in the above LKML
> discussion bogus? Is it legal for the compiler to reorder fields or add
> padding, even if the layout of fields in the structure doesn't require it?
> If the compiler *could* do such, then it seems like keeping the __packed
> would be appropriate per the LKML discussion.
The padding is defined in the ELF psABI document for a particular
architecture. In theory an architecture might require padding around
smaller members, but they generally don't when you look at the ones
that Linux runs on. The few odd ones are those that require less
padding, only aligning members to 16 or 32 bit rather than natural
alignment, or padding the size of the structure to 32 bit even if it
only contains 8-bit or 16-bit members. When you have structures
in which every member is naturally aligned and the size it a multiple
of 32 bit, better leave out the __packed.
Aside from generating sub-optimal code, the __packed annotation
can also lead to undefined behavior, if you pass a pointer to
an unaligned member into a function call that takes an aligned
pointer. Newer compilers warn about this.
> > > Unfortunately, changing to a bit mask ripples into
> > > architecture independent code and into the x86
> > > implementation. I'd prefer not to drag that complexity
> > > into this patch set.
> >
> > How so? If this file is arm64 specific, there should be no need to make
> > x86 do the same change.
>
> This file, hyperv-tlfs.h, is duplicating some definitions on the x86 and
> ARM64 sides that are used by arch independent code, and this is one
> of those definitions. I had held off on breaking the file into arch
> independent and arch specific portions because the Hyper-V team has
> left some gray areas for functionality that isn't yet used on the ARM64
> side. So in some cases, it's hard to know what functionality to put
> into the arch independent portion.
>
> But I think I'll go ahead and make the separation with reasonably good
> accuracy, and update the x86 side accordingly. That will reduce the size
> of this patch set to contain only the things that we know are ARM64
> specific and which are actually used by the ARM64 code. Things like the
> hv_message_flags will go into the arch independent portion so that
> they can be used by the arch independent code without cluttering up
> the arch specific code. Making the change will help reduce any
> confusion about what is ARM64-specific. The other core #include file,
> mshyperv.h, has already been done this way.
Ok, sounds good.
Arnd
^ permalink raw reply
* Re: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages
From: Arnd Bergmann @ 2020-03-20 16:28 UTC (permalink / raw)
To: Michael Kelley
Cc: Marc Zyngier, gregkh, Will Deacon, Ard Biesheuvel,
Catalin Marinas, Mark Rutland, Linux ARM,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-efi, linux-arch, olaf@aepfle.de, Andy Whitcroft, vkuznets,
Jason Wang, marcelo.cerri@canonical.com, KY Srinivasan,
Sunil Muthuswamy, Boqun Feng
In-Reply-To: <MW2PR2101MB10520CEF065A41EEBC17FFC2D7F40@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Thu, Mar 19, 2020 at 10:43 PM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:58 AM
> > On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > My point was to keep the functions but use alloc_pages() internally,
> > so you can deal with the hypervisor having a larger page size than
> > the guest, which seems to be a more important scenario if I correctly
> > understand the differences between the way Windows and Linux
> > deal with page cache.
>
> OK, I see now what you are getting at. I should spell out my
> assumption, which is the opposite. Hyper-V won't have a page
> size other than 4K anytime in the foreseeable future. The
> code is too wedded to the x86 4K page size, and the host-guest
> interfaces have a lot of implicit assumptions that the page size is
> 4K (unfortunately). But the last time I looked, RHEL for ARM64 is
> delivered with a 64K page size. So my assumption is that the only
> combination that really matters is the guest page size being larger
> than the Hyper-V page size. The other way around just won't
> happen without a major overhaul to Hyper-V, including a rework
> of the guest-host interface.
Ok, got it. We should really ask Red Hat to change the page size,
but as long as you care existing systems, and you expect this to
result in gigabytes of allocation on future systems, having the
wrapper seems reasonable.
Maybe you could fall back to alloc_page when PAGE_SIZE equals
HV_HYP_PAGE_SIZE though? I'm not sure what the tradeoff
between kmalloc and alloc_page is these days, other than the
feeling that alloc_page is the more obvious choice when you know
you always want exactly a page here.
Arnd
^ permalink raw reply
* Re: [PATCH v3 0/8] mm/memory_hotplug: allow to specify a default online_type
From: Baoquan He @ 2020-03-20 10:01 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv,
Vitaly Kuznetsov, Yumei Huang, Igor Mammedov, Eduardo Habkost,
Milan Zamazal, Andrew Morton, Benjamin Herrenschmidt,
Greg Kroah-Hartman, Haiyang Zhang, K. Y. Srinivasan,
Michael Ellerman, Michal Hocko, Michal Hocko, Oscar Salvador,
Paul Mackerras, Rafael J. Wysocki, Stephen Hemminger, Wei Liu,
Wei Yang
In-Reply-To: <20200319131221.14044-1-david@redhat.com>
On 03/19/20 at 02:12pm, David Hildenbrand wrote:
> Distributions nowadays use udev rules ([1] [2]) to specify if and
> how to online hotplugged memory. The rules seem to get more complex with
> many special cases. Due to the various special cases,
> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
> is handled via udev rules.
>
> Everytime we hotplug memory, the udev rule will come to the same
> conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
> memory in separate memory blocks and wait for memory to get onlined by user
> space before continuing to add more memory blocks (to not add memory faster
> than it is getting onlined). This of course slows down the whole memory
> hotplug process.
>
> To make the job of distributions easier and to avoid udev rules that get
> more and more complicated, let's extend the mechanism provided by
> - /sys/devices/system/memory/auto_online_blocks
> - "memhp_default_state=" on the kernel cmdline
> to be able to specify also "online_movable" as well as "online_kernel"
>
> v2 -> v3:
> - "hv_balloon: don't check for memhp_auto_online manually"
> -- init_completion() before register_memory_notifier()
> - Minor typo fix
>
> v1 -> v2:
> - Tweaked some patch descriptions
> - Added
> -- "powernv/memtrace: always online added memory blocks"
> -- "hv_balloon: don't check for memhp_auto_online manually"
> -- "mm/memory_hotplug: unexport memhp_auto_online"
> - "mm/memory_hotplug: convert memhp_auto_online to store an online_type"
> -- No longer touches hv/memtrace code
Ack the series.
Reviewed-by: Baoquan He <bhe@redhat.com>
^ permalink raw reply
* Re: [PATCH v3 3/8] drivers/base/memory: store mapping between MMOP_* and string in an array
From: Baoquan He @ 2020-03-20 9:59 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
Oscar Salvador, Rafael J. Wysocki
In-Reply-To: <166f7f03-eda9-00a8-bd18-128898526313@redhat.com>
On 03/20/20 at 10:50am, David Hildenbrand wrote:
> On 20.03.20 08:36, Baoquan He wrote:
> > On 03/19/20 at 02:12pm, David Hildenbrand wrote:
> >> Let's use a simple array which we can reuse soon. While at it, move the
> >> string->mmop conversion out of the device hotplug lock.
> >>
> >> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> >> Acked-by: Michal Hocko <mhocko@suse.com>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: Michal Hocko <mhocko@kernel.org>
> >> Cc: Oscar Salvador <osalvador@suse.de>
> >> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> >> Cc: Baoquan He <bhe@redhat.com>
> >> Cc: Wei Yang <richard.weiyang@gmail.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >> drivers/base/memory.c | 38 +++++++++++++++++++++++---------------
> >> 1 file changed, 23 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> >> index e7e77cafef80..8a7f29c0bf97 100644
> >> --- a/drivers/base/memory.c
> >> +++ b/drivers/base/memory.c
> >> @@ -28,6 +28,24 @@
> >>
> >> #define MEMORY_CLASS_NAME "memory"
> >>
> >> +static const char *const online_type_to_str[] = {
> >> + [MMOP_OFFLINE] = "offline",
> >> + [MMOP_ONLINE] = "online",
> >> + [MMOP_ONLINE_KERNEL] = "online_kernel",
> >> + [MMOP_ONLINE_MOVABLE] = "online_movable",
> >> +};
> >> +
> >> +static int memhp_online_type_from_str(const char *str)
> >> +{
> >> + int i;
> >
> > I would change it as:
> >
> > for (int i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
> >
>
> That's not allowed by the C90 standard (and -std=gnu89).
>
> $ gcc main.c -std=gnu89
> main.c: In function 'main':
> main.c:3:2: error: 'for' loop initial declarations are only allowed in
> C99 or C11 mode
> 3 | for (int i = 0; i < 8; i++) {
> | ^~~
Good to know, thanks.
>
> One of the reasons why
> git grep "for (int "
>
> will result in very little hits (IOW, only 5 in driver code only).
>
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply
* Re: [PATCH v3 3/8] drivers/base/memory: store mapping between MMOP_* and string in an array
From: David Hildenbrand @ 2020-03-20 9:50 UTC (permalink / raw)
To: Baoquan He
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
Oscar Salvador, Rafael J. Wysocki
In-Reply-To: <20200320073653.GE2987@MiWiFi-R3L-srv>
On 20.03.20 08:36, Baoquan He wrote:
> On 03/19/20 at 02:12pm, David Hildenbrand wrote:
>> Let's use a simple array which we can reuse soon. While at it, move the
>> string->mmop conversion out of the device hotplug lock.
>>
>> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>> Acked-by: Michal Hocko <mhocko@suse.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Oscar Salvador <osalvador@suse.de>
>> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>> Cc: Baoquan He <bhe@redhat.com>
>> Cc: Wei Yang <richard.weiyang@gmail.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> drivers/base/memory.c | 38 +++++++++++++++++++++++---------------
>> 1 file changed, 23 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index e7e77cafef80..8a7f29c0bf97 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -28,6 +28,24 @@
>>
>> #define MEMORY_CLASS_NAME "memory"
>>
>> +static const char *const online_type_to_str[] = {
>> + [MMOP_OFFLINE] = "offline",
>> + [MMOP_ONLINE] = "online",
>> + [MMOP_ONLINE_KERNEL] = "online_kernel",
>> + [MMOP_ONLINE_MOVABLE] = "online_movable",
>> +};
>> +
>> +static int memhp_online_type_from_str(const char *str)
>> +{
>> + int i;
>
> I would change it as:
>
> for (int i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
>
That's not allowed by the C90 standard (and -std=gnu89).
$ gcc main.c -std=gnu89
main.c: In function 'main':
main.c:3:2: error: 'for' loop initial declarations are only allowed in
C99 or C11 mode
3 | for (int i = 0; i < 8; i++) {
| ^~~
One of the reasons why
git grep "for (int "
will result in very little hits (IOW, only 5 in driver code only).
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v3 3/8] drivers/base/memory: store mapping between MMOP_* and string in an array
From: Baoquan He @ 2020-03-20 7:36 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
Oscar Salvador, Rafael J. Wysocki
In-Reply-To: <20200319131221.14044-4-david@redhat.com>
On 03/19/20 at 02:12pm, David Hildenbrand wrote:
> Let's use a simple array which we can reuse soon. While at it, move the
> string->mmop conversion out of the device hotplug lock.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> drivers/base/memory.c | 38 +++++++++++++++++++++++---------------
> 1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index e7e77cafef80..8a7f29c0bf97 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -28,6 +28,24 @@
>
> #define MEMORY_CLASS_NAME "memory"
>
> +static const char *const online_type_to_str[] = {
> + [MMOP_OFFLINE] = "offline",
> + [MMOP_ONLINE] = "online",
> + [MMOP_ONLINE_KERNEL] = "online_kernel",
> + [MMOP_ONLINE_MOVABLE] = "online_movable",
> +};
> +
> +static int memhp_online_type_from_str(const char *str)
> +{
> + int i;
I would change it as:
for (int i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
> +
> + for (i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
> + if (sysfs_streq(str, online_type_to_str[i]))
> + return i;
> + }
> + return -EINVAL;
> +}
> +
> #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
>
> static int sections_per_block;
> @@ -236,26 +254,17 @@ static int memory_subsys_offline(struct device *dev)
> static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> + const int online_type = memhp_online_type_from_str(buf);
> struct memory_block *mem = to_memory_block(dev);
> - int ret, online_type;
> + int ret;
> +
> + if (online_type < 0)
> + return -EINVAL;
>
> ret = lock_device_hotplug_sysfs();
> if (ret)
> return ret;
>
> - if (sysfs_streq(buf, "online_kernel"))
> - online_type = MMOP_ONLINE_KERNEL;
> - else if (sysfs_streq(buf, "online_movable"))
> - online_type = MMOP_ONLINE_MOVABLE;
> - else if (sysfs_streq(buf, "online"))
> - online_type = MMOP_ONLINE;
> - else if (sysfs_streq(buf, "offline"))
> - online_type = MMOP_OFFLINE;
> - else {
> - ret = -EINVAL;
> - goto err;
> - }
> -
> switch (online_type) {
> case MMOP_ONLINE_KERNEL:
> case MMOP_ONLINE_MOVABLE:
> @@ -271,7 +280,6 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> ret = -EINVAL; /* should never happen */
> }
>
> -err:
> unlock_device_hotplug();
>
> if (ret < 0)
> --
> 2.24.1
>
^ permalink raw reply
* Re: [PATCH 0/4] x86/Hyper-V: Panic code path fixes
From: Tianyu Lan @ 2020-03-20 2:21 UTC (permalink / raw)
To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Wei Liu, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
hpa@zytor.com, x86@kernel.org
Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, vkuznets
In-Reply-To: <MW2PR2101MB1052A7AFC5222CC9D78FA77ED7F40@MW2PR2101MB1052.namprd21.prod.outlook.com>
On 3/20/2020 12:07 AM, Michael Kelley wrote:
> From: Michael Kelley <mikelley@microsoft.com> Sent: Thursday, March 19, 2020 8:15 AM
>>
>> From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Thursday, March 19, 2020 7:08 AM
>>>>>
>>>>> This patchset fixes some issues in the Hyper-V panic code path.
>>>>> Patch 1 resolves issue that panic system still responses network
>>>>> packets.
>>>>> Patch 2-3 resolves crash enlightenment issues.
>>>>> Patch 4 is to set crash_kexec_post_notifiers to true for Hyper-V
>>>>> VM in order to report crash data or kmsg to host before running
>>>>> kdump kernel.
>>>>
>>>> I still see an issue that isn't addressed by these patches. The VMbus
>>>> driver registers a "die notifier" and a "panic notifier". But die() will
>>>> eventually call panic() if panic_on_oops is set (which I think it typically
>>>> is). If the CRASH_NOTIFY_MSG option is *not* enabled, then
>>>> hyperv_report_panic() could get called by the die notifier, and then
>>>> again by the panic notifier.
>>>>
>>>> Do we even need the "die notifier"? If it was removed, there would
>>>> not be any notification to Hyper-V via the die() path unless panic_on_oops
>>>> is set, which I think is actually the correct behavior. I'm not
>>>> completely clear on what is supposed to happen in general to the
>>>> Linux kernel if panic_on_oops is not set. Does it try to continue to run?
>>>> If so, then we should not be notifying Hyper-V if panic_on_oops is not
>>>> set, and removing the die notifier is the right thing to do.
>>>>
>>>
>>> hyperv_report_panic() has re-enter check inside and so kernel only
>>> reports crash register data once during die().
>>
>> Ah, yes, you are right.
>>
>>> From comment in the
>>> hyperv_report_panic(), register value reported in die chain is more
>>> exact than value in panic chain. The register value in die chain is
>>> passed by die() caller. Register value reported in panic chain
>>> is collected in the hyperv_panic_event().
>>>
>>> If panic_on_oops is not set, the task should be killed and kernel
>>> still runs. In this case, we may not trigger crash enlightenment.
>>
>> I'm not completely clear on your last statement. It seems like there
>> is still a problem in that die() will call hyperv_report_panic() even if
>> panic_on_oops is not set. We will have reported a panic to Hyper-V
>> even though the VM did not stop running.
>
Yes, the die callback is still necessary and we should skip report
if panic_on_oops isn't set.
>
> There's one more issue to consider. hv_kmsg_dump() skips calling
> hyperv_report_panic_msg() if sysctl_record_panic_msg has been cleared
> by a sysctl command. (This sysctl option gives a customer the ability to
> increase privacy by not having the VM's dmesg contents sent to Hyper-V.)
> In this case, the earlier hyperv_report_panic() call should be used. Otherwise,
> there would not be any notification to Hyper-V about the panic.
>
Nice catch. I will fix this in the next version.
Thanks.
^ permalink raw reply
* [PATCH][next] hv: hyperv_vmbus.h: Replace zero-length array with flexible-array member
From: Gustavo A. R. Silva @ 2020-03-19 21:32 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel, Gustavo A. R. Silva
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/hv/hyperv_vmbus.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index f5fa3b3c9baf..70b30e223a57 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -292,7 +292,7 @@ struct vmbus_msginfo {
struct list_head msglist_entry;
/* The message itself */
- unsigned char msg[0];
+ unsigned char msg[];
};
--
2.23.0
^ permalink raw reply related
* RE: [PATCH v6 09/10] arm64: efi: Export screen_info
From: Michael Kelley @ 2020-03-19 21:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <CAK8P3a1YCtc3LJ-_3iT90_Srehb96gLHvTXsbJ0wT6NFYCG=TQ@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
>
> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > > >
> > > > The Hyper-V frame buffer driver may be built as a module, and
> > > > it needs access to screen_info. So export screen_info.
> > > >
> > > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > >
> > > Is there any chance of using a more modern KMS based driver for the screen
> > > than the old fbdev subsystem? I had hoped to one day completely remove
> > > support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> > > architectures.
> > >
> >
> > The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
> > frame buffer device. That driver builds and runs on both ARM64 and x86.
> >
> > I'm not knowledgeable about video/graphics drivers, but when you
> > say "a more modern KMS based driver", are you meaning one based on
> > DRM & KMS? Does DRM make sense for a "dumb" frame buffer device?
> > Are there any drivers that would be a good pattern to look at?
>
> It used to be a lot harder to write a DRM driver compared to an fbdev
> driver, but this has changed to the opposite over the years.
>
> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
>
Thanks for the pointers, especially for the other hypervisors.
Michael
^ permalink raw reply
* RE: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages
From: Michael Kelley @ 2020-03-19 21:43 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Marc Zyngier, gregkh, Will Deacon, Ard Biesheuvel,
Catalin Marinas, Mark Rutland, Linux ARM,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-efi, linux-arch, olaf@aepfle.de, Andy Whitcroft, vkuznets,
Jason Wang, marcelo.cerri@canonical.com, KY Srinivasan,
Sunil Muthuswamy, Boqun Feng
In-Reply-To: <CAK8P3a2AO4k3vJ7WuJQz7ick+XPgGY3Jfk8-ROqtwyNs0nWkDA@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:58 AM
>
> On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:33 AM
> > >
> > > On Mon, Mar 16, 2020 at 9:30 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > On 2020-03-16 08:22, Arnd Bergmann wrote:
> > > > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com>
> > > > > wrote:
> > > > >> /*
> > > > >> + * Functions for allocating and freeing memory with size and
> > > > >> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> > > > >> + * the guest page size may not be the same as the Hyper-V page
> > > > >> + * size. We depend upon kmalloc() aligning power-of-two size
> > > > >> + * allocations to the allocation size boundary, so that the
> > > > >> + * allocated memory appears to Hyper-V as a page of the size
> > > > >> + * it expects.
> > > > >> + *
> > > > >> + * These functions are used by arm64 specific code as well as
> > > > >> + * arch independent Hyper-V drivers.
> > > > >> + */
> > > > >> +
> > > > >> +void *hv_alloc_hyperv_page(void)
> > > > >> +{
> > > > >> + BUILD_BUG_ON(PAGE_SIZE < HV_HYP_PAGE_SIZE);
> > > > >> + return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> > > > >> +}
> > > > >> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> > > > >
> > > > > I don't think there is any guarantee that kmalloc() returns
> > > > > page-aligned
> > > > > allocations in general.
> > > >
> > > > I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b:
> > > > guarantee
> > > > natural alignment for kmalloc(power-of-two)").
> > > >
> > > > > How about using get_free_pages() to implement this?
> > > >
> > > > This would certainly work, at the expense of a lot of wasted memory when
> > > > PAGE_SIZE isn't 4k.
> > >
> > > I'm sure this is the least of your problems when the guest runs with
> > > a large base page size, you've already wasted most of your memory
> > > otherwise then.
> > >
> >
> > I think there's value in keeping these functions. There are 8 uses in
> > architecture independent code at the moment, which admittedly saves
> > only ~1/2 Mbyte of memory with a 64K page size, but we will have
> > additional uses with more memory savings as we get all of the
> > Hyper-V synthetic drivers to work with 64K page size. Furthermore,
> > there's coming work that will require additional steps to share a page
> > between a guest and the Hyper-V host. These functions are the right
> > place to put the code for the additional sharing steps. Removing them
> > now in favor of a bare kmalloc() and then adding them back doesn't
> > seem worthwhile.
>
> My point was to keep the functions but use alloc_pages() internally,
> so you can deal with the hypervisor having a larger page size than
> the guest, which seems to be a more important scenario if I correctly
> understand the differences between the way Windows and Linux
> deal with page cache.
OK, I see now what you are getting at. I should spell out my
assumption, which is the opposite. Hyper-V won't have a page
size other than 4K anytime in the foreseeable future. The
code is too wedded to the x86 4K page size, and the host-guest
interfaces have a lot of implicit assumptions that the page size is
4K (unfortunately). But the last time I looked, RHEL for ARM64 is
delivered with a 64K page size. So my assumption is that the only
combination that really matters is the guest page size being larger
than the Hyper-V page size. The other way around just won't
happen without a major overhaul to Hyper-V, including a rework
of the guest-host interface.
Michael
>
> As far as I understand, using 64kb pages on Windows is generally
> a win as the VFS code already deals with units of that size, while
> on Linux the 4kb page size is too deeply entrenched within the
> file system code to mess with it: Whatever you gain in terms of
> TLB pressure on workloads that cannot use huge pages is all lost
> again through extra I/O and wasted physical memory.
>
> Arnd
^ permalink raw reply
* RE: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files
From: Michael Kelley @ 2020-03-19 21:31 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <CAK8P3a02EULGxyuKFq8YnbG8BQ_m-RKciaNEc9ZbdP2yz9dt+Q@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 3:10 AM
>
> On Wed, Mar 18, 2020 at 1:12 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:48 AM
> > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > >
> > > > +
> > > > +/* Define input and output layout for Get VP Register hypercall */
> > > > +struct hv_get_vp_register_input {
> > > > + u64 partitionid;
> > > > + u32 vpindex;
> > > > + u8 inputvtl;
> > > > + u8 padding[3];
> > > > + u32 name0;
> > > > + u32 name1;
> > > > +} __packed;
> > >
> > > Are you sure these need to be made byte-aligned according to the
> > > specification? If the structure itself is aligned to 64 bit, better mark only
> > > the individual fields that are misaligned as __packed.
> > >
> > > If the structure is aligned to only 32-bit addresses instead of
> > > 64-bit, mark it as "__packed __aligned(4)" to let the compiler
> > > generate better code for accessing it.
> >
> > None of the fields are misaligned and it will always be aligned to 64-bit
> > addresses, so there should be no padding needed or added. There was
> > a discussion of __packed and the Hyper-V data structures in general on
> > LKML here: https://lkml.org/lkml/2018/11/30/848 Adding __packed was
> > done as a preventative measure, not because anything was actually
> > broken. Marking as __aligned(8) here would indicate the correct intent,
> > though the use of the structure always ensures 64-bit alignment.
>
> Just drop the __packed annotations then, they just confuse the compiler
> in this case. In particular, when the compiler thinks that a structure is
> misaligned, it tries to avoid using load/store instructions on it that are
> inefficient or trap with misaligned code, so having default alignment
> produces better object code.
So I'm confused a bit. Were the original concerns in the above LKML
discussion bogus? Is it legal for the compiler to reorder fields or add
padding, even if the layout of fields in the structure doesn't require it?
If the compiler *could* do such, then it seems like keeping the __packed
would be appropriate per the LKML discussion.
>
> > > Also, in order to write portable code, it would be helpful to mark
> > > all the fields as explicitly little-endian, and use __le32_to_cpu()
> > > etc for accessing them.
> >
> > There's an opening comment in this file stating that all data
> > structures shared between Hyper-V and a guest VM are little
> > endian. Is there some other marking to consider using?
>
> Yes, device drivers should generally define data structures using
> the __le32, __le64 etc types, and use the conversion functions
> to access them. Building with 'make C=1' usually tells you when
> you have mismatched annotations.
>
> > We have definitely not allowed for the case of Hyper-V running on
> > a big endian architecture. There are a *lot* of messages and data
> > structures passed between the guest and Hyper-V, and coding
> > to handle either endianness is a big project. I'm doubtful
> > of the value until and unless we actually have a need for it.
>
> In general, the use of big-endian software on Linux is declining, however
>
> - arm64 as an architecture is meant to support both endian types,
> and we still try to ensure it works either way as long as there are
> users that depend on it.
>
> - The remaining users of big-endian software are probably
> more likely to run on virtual machines than on real hardware
>
> - Any device driver should generally be written against portable
> interfaces, even if you think you know how it will be used. As
> driver writers tend to look at existing code for new drivers, it's
> better to have them all be portable. (This is a similar argument
> to the irqchip interface).
>
> Even if you don't convert any of the existing architecture independent
> code to run both ways, I see no reason to not do it for new drivers.
OK, let me look into this. Given how the major Linux distros on
ARM64 have all gone little-endian, I'm a bit skeptical of the value
for the big server environments in which Hyper-V would be used.
>
> > > > +/* Define synthetic interrupt controller message flags. */
> > > > +union hv_message_flags {
> > > > + __u8 asu8;
> > > > + struct {
> > > > + __u8 msg_pending:1;
> > > > + __u8 reserved:7;
> > > > + } __packed;
> > > > +};
> > >
> > > For similar reasons, please avoid bit fields and just use a
> > > bit mask on the first member of the union.
> >
> > Unfortunately, changing to a bit mask ripples into
> > architecture independent code and into the x86
> > implementation. I'd prefer not to drag that complexity
> > into this patch set.
>
> How so? If this file is arm64 specific, there should be no need to make
> x86 do the same change.
This file, hyperv-tlfs.h, is duplicating some definitions on the x86 and
ARM64 sides that are used by arch independent code, and this is one
of those definitions. I had held off on breaking the file into arch
independent and arch specific portions because the Hyper-V team has
left some gray areas for functionality that isn't yet used on the ARM64
side. So in some cases, it's hard to know what functionality to put
into the arch independent portion.
But I think I'll go ahead and make the separation with reasonably good
accuracy, and update the x86 side accordingly. That will reduce the size
of this patch set to contain only the things that we know are ARM64
specific and which are actually used by the ARM64 code. Things like the
hv_message_flags will go into the arch independent portion so that
they can be used by the arch independent code without cluttering up
the arch specific code. Making the change will help reduce any
confusion about what is ARM64-specific. The other core #include file,
mshyperv.h, has already been done this way.
Michael
>
> > > > + * Use the Hyper-V provided stimer0 as the timer that is made
> > > > + * available to the architecture independent Hyper-V drivers.
> > > > + */
> > > > +#define hv_init_timer(timer, tick) \
> > > > + hv_set_vpreg(HV_REGISTER_STIMER0_COUNT + (2*timer), tick)
> > > > +#define hv_init_timer_config(timer, val) \
> > > > + hv_set_vpreg(HV_REGISTER_STIMER0_CONFIG + (2*timer), val)
> > > > +#define hv_get_current_tick(tick) \
> > > > + (tick = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))
> > >
> > > In general, we prefer inline functions over macros in header files.
> >
> > I can change the "set" calls to inline functions. As you can see, the "get"
> > functions are coded and used in architecture independent code and on
> > the x86 side in a way that won't convert to inline functions.
>
> Ok.
>
> Arnd
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox