| Message ID | 20210503150854.1144255-3-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Fix migration of nested guests when eVMCS is in use | expand |
On Mon, 2021-05-03 at 17:08 +0200, Vitaly Kuznetsov wrote: > Eliminate the probably unwanted hole in 'struct kvm_vmx_nested_state_hdr': > > Pre-patch: > struct kvm_vmx_nested_state_hdr { > __u64 vmxon_pa; /* 0 8 */ > __u64 vmcs12_pa; /* 8 8 */ > struct { > __u16 flags; /* 16 2 */ > } smm; /* 16 2 */ > > /* XXX 2 bytes hole, try to pack */ > > __u32 flags; /* 20 4 */ > __u64 preemption_timer_deadline; /* 24 8 */ > }; > > Post-patch: > struct kvm_vmx_nested_state_hdr { > __u64 vmxon_pa; /* 0 8 */ > __u64 vmcs12_pa; /* 8 8 */ > struct { > __u16 flags; /* 16 2 */ > } smm; /* 16 2 */ > __u16 pad; /* 18 2 */ > __u32 flags; /* 20 4 */ > __u64 preemption_timer_deadline; /* 24 8 */ > }; > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/include/uapi/asm/kvm.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h > index 5a3022c8af82..0662f644aad9 100644 > --- a/arch/x86/include/uapi/asm/kvm.h > +++ b/arch/x86/include/uapi/asm/kvm.h > @@ -437,6 +437,8 @@ struct kvm_vmx_nested_state_hdr { > __u16 flags; > } smm; > > + __u16 pad; > + > __u32 flags; > __u64 preemption_timer_deadline; > }; Looks good to me. I wonder if we can enable the -Wpadded GCC warning to warn about such cases. Probably can't be enabled for the whole kernel but maybe we can enable it for KVM codebase at least, like we did with -Werror. From GCC manual: "-Wpadded Warn if padding is included in a structure, either to align an element of the structure or to align the whole structure. Sometimes when this happens it is possible to rearrange the fields of the structure to reduce the padding and so make the structure smaller." Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On Wed, May 05, 2021, Maxim Levitsky wrote: > On Mon, 2021-05-03 at 17:08 +0200, Vitaly Kuznetsov wrote: > > Eliminate the probably unwanted hole in 'struct kvm_vmx_nested_state_hdr': > > > > Pre-patch: > > struct kvm_vmx_nested_state_hdr { > > __u64 vmxon_pa; /* 0 8 */ > > __u64 vmcs12_pa; /* 8 8 */ > > struct { > > __u16 flags; /* 16 2 */ > > } smm; /* 16 2 */ > > > > /* XXX 2 bytes hole, try to pack */ > > > > __u32 flags; /* 20 4 */ > > __u64 preemption_timer_deadline; /* 24 8 */ > > }; > > > > Post-patch: > > struct kvm_vmx_nested_state_hdr { > > __u64 vmxon_pa; /* 0 8 */ > > __u64 vmcs12_pa; /* 8 8 */ > > struct { > > __u16 flags; /* 16 2 */ > > } smm; /* 16 2 */ > > __u16 pad; /* 18 2 */ > > __u32 flags; /* 20 4 */ > > __u64 preemption_timer_deadline; /* 24 8 */ > > }; > > > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > --- > > arch/x86/include/uapi/asm/kvm.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h > > index 5a3022c8af82..0662f644aad9 100644 > > --- a/arch/x86/include/uapi/asm/kvm.h > > +++ b/arch/x86/include/uapi/asm/kvm.h > > @@ -437,6 +437,8 @@ struct kvm_vmx_nested_state_hdr { > > __u16 flags; > > } smm; > > > > + __u16 pad; > > + > > __u32 flags; > > __u64 preemption_timer_deadline; > > }; > > > Looks good to me. > > I wonder if we can enable the -Wpadded GCC warning to warn about such cases. > Probably can't be enabled for the whole kernel but maybe we can enable it > for KVM codebase at least, like we did with -Werror. It'll never work, there are far, far too many structs throughout the kernel and KVM that have implicit padding. And for kernel-internal structs, that's perfectly ok and even desirable since the kernel generally shouldn't make assumptions about the layouts of its structs, i.e. it's a good thing the compiler pads structs so that accesses are optimally aligned. The padding behavior is only problematic for structs that are exposed to userspace, because if userspace pads differently then we've got problems. But even then, building the kernel with -Wpadded wouldn't prevent userspace from using a broken/goofy compiler that inserts unusual padding and misinterprets the intended layout. AFAIK, the C standard only expicitly disallows padding at the beginning of a struct, i.e. the kernel's ABI is heavily reliant on existing compiler convention. The only way to ensure exact layouts without relying on compiler convention would be to tagged structs as packed, but "packed" also causes the compiler to generate sub-optimal code since "packed" has strict requirements, and so the kernel relies on sane compiler padding to provide a stable ABI.
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h index 5a3022c8af82..0662f644aad9 100644 --- a/arch/x86/include/uapi/asm/kvm.h +++ b/arch/x86/include/uapi/asm/kvm.h @@ -437,6 +437,8 @@ struct kvm_vmx_nested_state_hdr { __u16 flags; } smm; + __u16 pad; + __u32 flags; __u64 preemption_timer_deadline; };
Eliminate the probably unwanted hole in 'struct kvm_vmx_nested_state_hdr': Pre-patch: struct kvm_vmx_nested_state_hdr { __u64 vmxon_pa; /* 0 8 */ __u64 vmcs12_pa; /* 8 8 */ struct { __u16 flags; /* 16 2 */ } smm; /* 16 2 */ /* XXX 2 bytes hole, try to pack */ __u32 flags; /* 20 4 */ __u64 preemption_timer_deadline; /* 24 8 */ }; Post-patch: struct kvm_vmx_nested_state_hdr { __u64 vmxon_pa; /* 0 8 */ __u64 vmcs12_pa; /* 8 8 */ struct { __u16 flags; /* 16 2 */ } smm; /* 16 2 */ __u16 pad; /* 18 2 */ __u32 flags; /* 20 4 */ __u64 preemption_timer_deadline; /* 24 8 */ }; Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/include/uapi/asm/kvm.h | 2 ++ 1 file changed, 2 insertions(+)