Re: [PATCH] x86/hyper-v: define structures from TLFS as packed
Roman Kagan writes: > On Fri, Nov 30, 2018 at 02:44:54PM +0100, Vitaly Kuznetsov wrote: > >> I also checked this patch before sending out, WS2016 guest boots without >> issues. Any particular places you're worried about? > > It's Linux guests on Hyper-V that need to be checked. > That's exactly what I've tested, Linux guest on Windows Server 2016 with Hyper-V. (Actually, I checked WS2016 guest on KVM with hv_stimer and hv_stimer_direct too but afaiu we're not worried about the alignment requirements there). -- Vitaly
Re: [PATCH] x86/hyper-v: define structures from TLFS as packed
On Fri, Nov 30, 2018 at 02:44:54PM +0100, Vitaly Kuznetsov wrote: > Roman Kagan writes: > > > On Fri, Nov 30, 2018 at 01:15:11PM +0100, Vitaly Kuznetsov wrote: > >> Without 'packed' compiler is free to add optimization paddings and re-order > >> structure fields for randomization/optimization. And structures from > >> hyperv-tlfs.h are used for hypervisor-guest communication, we need to > >> ultimately forbid such practices. > > > > Note that __packed also reduces the structure alignment to 1, which is > > not necessarily what you want. > > > > E.g. some of these structures are passed by pointer to the hypercall, > > which requires its arguments to be 8byte-aligned. > > Hm, > > I thought we always take precautions for Hyper-V hypercall arguments, in > particular > > PV IPI/TLB flush use pre-allocated hyperv_pcpu_input_arg, > hv_post_message() uses pre-allocated message page, other call sites use > fast hypercalls where we use registers. Looks so indeed. > I also checked this patch before sending out, WS2016 guest boots without > issues. Any particular places you're worried about? It's Linux guests on Hyper-V that need to be checked. > > I'm also not sure > > that passing unaligned argument to [rw]msr is ok, need to > > double-check. > > My understanding is that rdmsr/wrmsr instuctions are registers-only. Indeed. > We can, of course, just add __aligned(8) to some structures but I'd like > to find the reason first. I guess you're right, in the current code the relevant structures are made sufficiently aligned by other means. False alarm, sorry. Roman.
Re: [PATCH] x86/hyper-v: define structures from TLFS as packed
Roman Kagan writes: > On Fri, Nov 30, 2018 at 01:15:11PM +0100, Vitaly Kuznetsov wrote: >> Without 'packed' compiler is free to add optimization paddings and re-order >> structure fields for randomization/optimization. And structures from >> hyperv-tlfs.h are used for hypervisor-guest communication, we need to >> ultimately forbid such practices. > > Note that __packed also reduces the structure alignment to 1, which is > not necessarily what you want. > > E.g. some of these structures are passed by pointer to the hypercall, > which requires its arguments to be 8byte-aligned. Hm, I thought we always take precautions for Hyper-V hypercall arguments, in particular PV IPI/TLB flush use pre-allocated hyperv_pcpu_input_arg, hv_post_message() uses pre-allocated message page, other call sites use fast hypercalls where we use registers. I also checked this patch before sending out, WS2016 guest boots without issues. Any particular places you're worried about? > I'm also not sure > that passing unaligned argument to [rw]msr is ok, need to > double-check. My understanding is that rdmsr/wrmsr instuctions are registers-only. We can, of course, just add __aligned(8) to some structures but I'd like to find the reason first. -- Vitaly
Re: [PATCH] x86/hyper-v: define structures from TLFS as packed
On Fri, 30 Nov 2018, Roman Kagan wrote: > On Fri, Nov 30, 2018 at 01:15:11PM +0100, Vitaly Kuznetsov wrote: > > Without 'packed' compiler is free to add optimization paddings and re-order > > structure fields for randomization/optimization. And structures from > > hyperv-tlfs.h are used for hypervisor-guest communication, we need to > > ultimately forbid such practices. > > Note that __packed also reduces the structure alignment to 1, which is > not necessarily what you want. > > E.g. some of these structures are passed by pointer to the hypercall, > which requires its arguments to be 8byte-aligned. I'm also not sure > that passing unaligned argument to [rw]msr is ok, need to double-check. If you have alignment requirements then you need to express them too. Thanks, tglx
Re: [PATCH] x86/hyper-v: define structures from TLFS as packed
On Fri, Nov 30, 2018 at 01:15:11PM +0100, Vitaly Kuznetsov wrote:
> Without 'packed' compiler is free to add optimization paddings and re-order
> structure fields for randomization/optimization. And structures from
> hyperv-tlfs.h are used for hypervisor-guest communication, we need to
> ultimately forbid such practices.
Note that __packed also reduces the structure alignment to 1, which is
not necessarily what you want.
E.g. some of these structures are passed by pointer to the hypercall,
which requires its arguments to be 8byte-aligned. I'm also not sure
that passing unaligned argument to [rw]msr is ok, need to double-check.
Roman.
> Suggested-by: Nadav Amit
> Signed-off-by: Vitaly Kuznetsov
> ---
> - This is a follow-up to my "[PATCH v2 0/4] x86/kvm/hyper-v: Implement
> Direct Mode for synthetic timers" series, as suggested by Thomas I'm
> routing it to KVM tree to avoid merge conflicts.
> ---
> arch/x86/include/asm/hyperv-tlfs.h | 50 +++---
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> b/arch/x86/include/asm/hyperv-tlfs.h
> index ebfed56976d2..6a60fa17f6f2 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -271,7 +271,7 @@ union hv_x64_msr_hypercall_contents {
> u64 enable:1;
> u64 reserved:11;
> u64 guest_physical_address:52;
> - };
> + } __packed;
> };
>
> /*
> @@ -283,7 +283,7 @@ struct ms_hyperv_tsc_page {
> volatile u64 tsc_scale;
> volatile s64 tsc_offset;
> u64 reserved2[509];
> -};
> +} __packed;
>
> /*
> * The guest OS needs to register the guest ID with the hypervisor.
> @@ -324,7 +324,7 @@ struct hv_reenlightenment_control {
> __u64 enabled:1;
> __u64 reserved2:15;
> __u64 target_vp:32;
> -};
> +} __packed;
>
> #define HV_X64_MSR_TSC_EMULATION_CONTROL 0x4107
> #define HV_X64_MSR_TSC_EMULATION_STATUS 0x4108
> @@ -332,12 +332,12 @@ struct hv_reenlightenment_control {
> struct hv_tsc_emulation_control {
> __u64 enabled:1;
> __u64 reserved:63;
> -};
> +} __packed;
>
> struct hv_tsc_emulation_status {
> __u64 inprogress:1;
> __u64 reserved:63;
> -};
> +} __packed;
>
> #define HV_X64_MSR_HYPERCALL_ENABLE 0x0001
> #define HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT 12
> @@ -409,7 +409,7 @@ typedef struct _HV_REFERENCE_TSC_PAGE {
> __u32 res1;
> __u64 tsc_scale;
> __s64 tsc_offset;
> -} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
> +} __packed HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
>
> /* Define the number of synthetic interrupt sources. */
> #define HV_SYNIC_SINT_COUNT (16)
> @@ -466,7 +466,7 @@ union hv_message_flags {
> struct {
> __u8 msg_pending:1;
> __u8 reserved:7;
> - };
> + } __packed;
> };
>
> /* Define port identifier type. */
> @@ -488,7 +488,7 @@ struct hv_message_header {
> __u64 sender;
> union hv_port_id port;
> };
> -};
> +} __packed;
>
> /* Define synthetic interrupt controller message format. */
> struct hv_message {
> @@ -496,12 +496,12 @@ struct hv_message {
> union {
> __u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT];
> } u;
> -};
> +} __packed;
>
> /* Define the synthetic interrupt message page layout. */
> struct hv_message_page {
> struct hv_message sint_message[HV_SYNIC_SINT_COUNT];
> -};
> +} __packed;
>
> /* Define timer message payload structure. */
> struct hv_timer_message_payload {
> @@ -509,7 +509,7 @@ struct hv_timer_message_payload {
> __u32 reserved;
> __u64 expiration_time; /* When the timer expired */
> __u64 delivery_time;/* When the message was delivered */
> -};
> +} __packed;
>
> /* Define virtual processor assist page structure. */
> struct hv_vp_assist_page {
> @@ -519,7 +519,7 @@ struct hv_vp_assist_page {
> __u64 nested_enlightenments_control[2];
> __u32 enlighten_vmentry;
> __u64 current_nested_vmcs;
> -};
> +} __packed;
>
> struct hv_enlightened_vmcs {
> u32 revision_id;
> @@ -693,7 +693,7 @@ struct hv_enlightened_vmcs {
> u32 nested_flush_hypercall:1;
> u32 msr_bitmap:1;
> u32 reserved:30;
> - } hv_enlightenments_control;
> + } __packed hv_enlightenments_control;
> u32 hv_vp_id;
>
> u64 hv_vm_id;
> @@ -703,7 +703,7 @@ struct hv_enlightened_vmcs {
> u64 padding64_5[7];
> u64 xss_exit_bitmap;
> u64 padding64_6[7];
> -};
> +} __packed;
>
> #define HV_VMX_ENLIGHTENED_CLEAN_FIELD_NONE 0
> #define HV_VMX_ENLIGHTENED_CLEAN_FIELD_IO_BITMAP BIT(0)
> @@ -744,7 +744,7 @@ union hv_stimer_config {
> u64 reserved_z0:3;
> u64 sintx:4;
> u64 reserved_z1:44;
> - };
> + } __packed
Re: [PATCH] x86/hyper-v: define structures from TLFS as packed
Thomas Gleixner writes: > On Fri, 30 Nov 2018, Vitaly Kuznetsov wrote: > >> Subject: x86/hyper-v: define structures from TLFS as packed > > Please start the first word after the prefix colon with an uppercase > letter. Also structures from TLFS doesn't make sense to me. Something like > this: > > Subject: x86/hyper-v: Mark TLFS structures packed > >> Without 'packed' compiler is free to add optimization paddings and re-order >> structure fields for randomization/optimization. And structures from > > s/And/As/ ? > >> hyperv-tlfs.h are used for hypervisor-guest communication, we need to >> ultimately forbid such practices. > > That whole paragraph reads a bit strange. Something like: > > The TLFS structures are used for hypervisor-guest communication and must > exactly meet the specification. > > Compilers can add alignment padding to structures or reorder struct > members for randomization and optimization, which would break the > hypervisor ABI. > > Mark the structures as packed to prevent this. > > Hmm? Sure, will do v2! > >> Suggested-by: Nadav Amit >> Signed-off-by: Vitaly Kuznetsov > > Other than that: Acked-by: Thomas Gleixner Thanks! -- Vitaly
Re: [PATCH] x86/hyper-v: define structures from TLFS as packed
On Fri, 30 Nov 2018, Vitaly Kuznetsov wrote: > Subject: x86/hyper-v: define structures from TLFS as packed Please start the first word after the prefix colon with an uppercase letter. Also structures from TLFS doesn't make sense to me. Something like this: Subject: x86/hyper-v: Mark TLFS structures packed > Without 'packed' compiler is free to add optimization paddings and re-order > structure fields for randomization/optimization. And structures from s/And/As/ ? > hyperv-tlfs.h are used for hypervisor-guest communication, we need to > ultimately forbid such practices. That whole paragraph reads a bit strange. Something like: The TLFS structures are used for hypervisor-guest communication and must exactly meet the specification. Compilers can add alignment padding to structures or reorder struct members for randomization and optimization, which would break the hypervisor ABI. Mark the structures as packed to prevent this. Hmm? > Suggested-by: Nadav Amit > Signed-off-by: Vitaly Kuznetsov Other than that: Acked-by: Thomas Gleixner
