Re: [PATCH 2/4] KVM: nVMX: Properly pad 'struct kvm_vmx_nested_state_hdr'

[Maxim Levitsky]

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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

Best regards,
	Maxim Levitsky