Re: [PATCH v4] x86/hyper-v: Mark TLFS structures packed

[Paolo Bonzini]

On 12/12/18 18:57, Vitaly Kuznetsov wrote:
> 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. 'struct hv_vp_assist_page'
> and 'struct hv_enlightened_vmcs' need to be properly padded to support the
> change.
> 
> Suggested-by: Nadav Amit <nadav.amit@xxxxxxxxx>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> Acked-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Acked-by: Nadav Amit <nadav.amit@xxxxxxxxx>
> Reviewed-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> ---
> - Changes since v3:
>  - Properly pad 'struct hv_vp_assist_page' and 'struct hv_enlightened_vmcs'.
>  - Add Michael's Reviewed-by tag.
> 
> - 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 | 57 ++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index ebfed56976d2..64d2b1914d37 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	0x40000107
>  #define HV_X64_MSR_TSC_EMULATION_STATUS		0x40000108
> @@ -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		0x00000001
>  #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. */
> @@ -475,7 +475,7 @@ union hv_port_id {
>  	struct {
>  		__u32 id:24;
>  		__u32 reserved:8;
> -	} u;
> +	} __packed u;
>  };
>  
>  /* Define synthetic interrupt controller message header. */
> @@ -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 {
> @@ -518,8 +518,9 @@ struct hv_vp_assist_page {
>  	__u64 vtl_control[2];
>  	__u64 nested_enlightenments_control[2];
>  	__u32 enlighten_vmentry;
> +	__u32 padding;
>  	__u64 current_nested_vmcs;
> -};
> +} __packed;
>  
>  struct hv_enlightened_vmcs {
>  	u32 revision_id;
> @@ -533,6 +534,8 @@ struct hv_enlightened_vmcs {
>  	u16 host_gs_selector;
>  	u16 host_tr_selector;
>  
> +	u16 padding16_1;
> +
>  	u64 host_ia32_pat;
>  	u64 host_ia32_efer;
>  
> @@ -651,7 +654,7 @@ struct hv_enlightened_vmcs {
>  	u64 ept_pointer;
>  
>  	u16 virtual_processor_id;
> -	u16 padding16[3];
> +	u16 padding16_2[3];
>  
>  	u64 padding64_2[5];
>  	u64 guest_physical_address;
> @@ -693,7 +696,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 +706,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 +747,7 @@ union hv_stimer_config {
>  		u64 reserved_z0:3;
>  		u64 sintx:4;
>  		u64 reserved_z1:44;
> -	};
> +	} __packed;
>  };
>  
>  
> @@ -759,7 +762,7 @@ union hv_synic_scontrol {
>  	struct {
>  		u64 enable:1;
>  		u64 reserved:63;
> -	};
> +	} __packed;
>  };
>  
>  /* Define synthetic interrupt source. */
> @@ -771,7 +774,7 @@ union hv_synic_sint {
>  		u64 masked:1;
>  		u64 auto_eoi:1;
>  		u64 reserved2:46;
> -	};
> +	} __packed;
>  };
>  
>  /* Define the format of the SIMP register */
> @@ -781,7 +784,7 @@ union hv_synic_simp {
>  		u64 simp_enabled:1;
>  		u64 preserved:11;
>  		u64 base_simp_gpa:52;
> -	};
> +	} __packed;
>  };
>  
>  /* Define the format of the SIEFP register */
> @@ -791,34 +794,34 @@ union hv_synic_siefp {
>  		u64 siefp_enabled:1;
>  		u64 preserved:11;
>  		u64 base_siefp_gpa:52;
> -	};
> +	} __packed;
>  };
>  
>  struct hv_vpset {
>  	u64 format;
>  	u64 valid_bank_mask;
>  	u64 bank_contents[];
> -};
> +} __packed;
>  
>  /* HvCallSendSyntheticClusterIpi hypercall */
>  struct hv_send_ipi {
>  	u32 vector;
>  	u32 reserved;
>  	u64 cpu_mask;
> -};
> +} __packed;
>  
>  /* HvCallSendSyntheticClusterIpiEx hypercall */
>  struct hv_send_ipi_ex {
>  	u32 vector;
>  	u32 reserved;
>  	struct hv_vpset vp_set;
> -};
> +} __packed;
>  
>  /* HvFlushGuestPhysicalAddressSpace hypercalls */
>  struct hv_guest_mapping_flush {
>  	u64 address_space;
>  	u64 flags;
> -};
> +} __packed;
>  
>  /* HvFlushVirtualAddressSpace, HvFlushVirtualAddressList hypercalls */
>  struct hv_tlb_flush {
> @@ -826,7 +829,7 @@ struct hv_tlb_flush {
>  	u64 flags;
>  	u64 processor_mask;
>  	u64 gva_list[];
> -};
> +} __packed;
>  
>  /* HvFlushVirtualAddressSpaceEx, HvFlushVirtualAddressListEx hypercalls */
>  struct hv_tlb_flush_ex {
> @@ -834,6 +837,6 @@ struct hv_tlb_flush_ex {
>  	u64 flags;
>  	struct hv_vpset hv_vp_set;
>  	u64 gva_list[];
> -};
> +} __packed;
>  
>  #endif
> 

Queued, thanks.  I squashed the SynIC parts into "x86/hyper-v: move
synic/stimer control structures definitions to hyperv-tlfs.h" and kept
the rest separate.

Paolo