RE: [PATCH v2] x86/hyper-v: Mark TLFS structures packed
Michael Kelley writes:
> From: Vitaly Kuznetsov Sent: Friday, November 30, 2018
> 4:54 AM
>>
>> @@ -466,7 +466,7 @@ union hv_message_flags {
>> struct {
>> __u8 msg_pending:1;
>> __u8 reserved:7;
>> -};
>> +} __packed;
>> };
>>
>> /* Define port identifier type. */
>
> I think __packed is also needed in the definition
> of struct u defined within union hv_port_id. That
> struct has bit fields, so shouldn't it be done just like
> union hv_message_flags above?
>
True, missed it, thanks! Will send v3 out shortly.
--
Vitaly
Re: [PATCH v2] x86/hyper-v: Mark TLFS structures packed
On Mon, Dec 03, 2018 at 12:35:35AM +0100, Vitaly Kuznetsov wrote:
> Nadav Amit writes:
>
> [skip]
>
> >
> > Having said that, something else is sort of strange in the TLFS definitions,
> > I think (I really know little about this whole protocol). Look at the
> > following definitions from hyperv-tlfs.h:
> >
> >> struct hv_vpset {
> >> u64 format;
> >> u64 valid_bank_mask;
> >> u64 bank_contents[];
> >> };
> >>
> >> struct hv_tlb_flush_ex {
> >> u64 address_space;
> >> u64 flags;
> >> struct hv_vpset hv_vp_set;
> >> u64 gva_list[];
> >> };
> >
> > It seems you have two flexible array members at the end of hv_tlb_flush_ex.
> > This causes bank_contents[x] and gva_list[x] to overlap. So unless they have
> > the same meaning, this asks for trouble IMHO.
> >
>
> This is weird but intentional :-) We're just following Hyper-V spec
> here.
>
> E.g. HvFlushVirtualAddressListEx hypercall has the following input ABI:
>
> [Fixed len head][[Fixed len VP set spec]Var len VP set][Var len addr List]
>
> "Fixed len VP set spec" defines the true length of "Var len VP set" and
> "Address List" starts right after that. The length of the whole
> structure is also known.
>
> So bank_contents[] and gva_list[] do overlap (and have different
> meaning). We take special precautions when forming the structure
> (e.g. fill_gva_list() takes 'offset').
This basically means that the argument of this hypercall can't be
represented by a C struct. gva_list just can't be used. So I'd rather
remove it from the struct (but leave a comment to that end perhaps), and
construct the message in place (as is done now anyway).
Roman.
RE: [PATCH v2] x86/hyper-v: Mark TLFS structures packed
From: Vitaly Kuznetsov Sent: Friday, November 30, 2018
4:54 AM
>
> @@ -466,7 +466,7 @@ union hv_message_flags {
> struct {
> __u8 msg_pending:1;
> __u8 reserved:7;
> - };
> + } __packed;
> };
>
> /* Define port identifier type. */
I think __packed is also needed in the definition
of struct u defined within union hv_port_id. That
struct has bit fields, so shouldn't it be done just like
union hv_message_flags above?
Michael
> @@ -488,7 +488,7 @@ struct hv_message_header {
> __u64 sender;
> union hv_port_id port;
> };
> -};
> +} __packed;
>
Re: [PATCH v2] x86/hyper-v: Mark TLFS structures packed
Nadav Amit writes:
[skip]
>
> Having said that, something else is sort of strange in the TLFS definitions,
> I think (I really know little about this whole protocol). Look at the
> following definitions from hyperv-tlfs.h:
>
>> struct hv_vpset {
>> u64 format;
>> u64 valid_bank_mask;
>> u64 bank_contents[];
>> };
>>
>> struct hv_tlb_flush_ex {
>> u64 address_space;
>> u64 flags;
>> struct hv_vpset hv_vp_set;
>> u64 gva_list[];
>> };
>
> It seems you have two flexible array members at the end of hv_tlb_flush_ex.
> This causes bank_contents[x] and gva_list[x] to overlap. So unless they have
> the same meaning, this asks for trouble IMHO.
>
This is weird but intentional :-) We're just following Hyper-V spec
here.
E.g. HvFlushVirtualAddressListEx hypercall has the following input ABI:
[Fixed len head][[Fixed len VP set spec]Var len VP set][Var len addr List]
"Fixed len VP set spec" defines the true length of "Var len VP set" and
"Address List" starts right after that. The length of the whole
structure is also known.
So bank_contents[] and gva_list[] do overlap (and have different
meaning). We take special precautions when forming the structure
(e.g. fill_gva_list() takes 'offset').
--
Vitaly
Re: [PATCH v2] x86/hyper-v: Mark TLFS structures packed
> On Nov 30, 2018, at 4:54 AM, 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.
Seems good to me (I made sure you remembered to set __packed for the nested
structs ;-) )
If needed:
Acked-by: Nadav Amit
Having said that, something else is sort of strange in the TLFS definitions,
I think (I really know little about this whole protocol). Look at the
following definitions from hyperv-tlfs.h:
> struct hv_vpset {
> u64 format;
> u64 valid_bank_mask;
> u64 bank_contents[];
> };
>
> struct hv_tlb_flush_ex {
> u64 address_space;
> u64 flags;
> struct hv_vpset hv_vp_set;
> u64 gva_list[];
> };
It seems you have two flexible array members at the end of hv_tlb_flush_ex.
This causes bank_contents[x] and gva_list[x] to overlap. So unless they have
the same meaning, this asks for trouble IMHO.
[PATCH v2] x86/hyper-v: Mark TLFS structures packed
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.
Suggested-by: Nadav Amit
Signed-off-by: Vitaly Kuznetsov
Acked-by: Thomas Gleixner
---
- Changes since v1:
- Re-worded commit message for clarity. [Thomas Gleixner]
- 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_STATUS0x4108
@@ -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_ENABLE0x0001
#define HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT12
@@ -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_NONE0
#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;
};
@@ -759,7 +759,7 @@ union hv_synic_scontrol {
struct {
u64 enable:1;
u64 reserved:63;
- };
+ } __packed;
};
/* Define synthetic interrupt source. */
@@ -771,7 +771,7 @@ union hv_synic_sint {
u64 masked:1;
u64 auto_eoi:1;
u64 reserved2:46;
- };
+ } __packed;
};
/* Defin
