Re: [Qemu-devel] [PATCH 4/7] linux-headers: import improved definition of KVM_GET/SET_NESTED_STATE structs

[Liran Alon]

> On 15 Jun 2019, at 3:42, Paolo Bonzini <address@hidden> wrote:
> 
> This patch improves the KVM_GET/SET_NESTED_STATE structs by detailing
> the format of VMX nested state in a struct.  The VMX nested state is
> accessible through struct kvm_vmx_nested_state though, to avoid
> changing the size of the structs, it has to be accessed as "vmx.data[0]"
> rather than just "vmx.data".
> 
> Also, the values of the "format" field are defined as macros.  This
> patch should be sent to Linus very shortly.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> linux-headers/asm-x86/kvm.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> 
> diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
> index 7a0e64ccd6..06b8727a3b 100644
> --- a/linux-headers/asm-x86/kvm.h
> +++ b/linux-headers/asm-x86/kvm.h
> @@ -383,6 +383,9 @@ struct kvm_sync_regs {
> #define KVM_X86_QUIRK_LAPIC_MMIO_HOLE (1 << 2)
> #define KVM_X86_QUIRK_OUT_7E_INC_RIP  (1 << 3)
> 
> +#define KVM_STATE_NESTED_FORMAT_VMX  0
> +#define KVM_STATE_NESTED_FORMAT_SVM  1
> +
> #define KVM_STATE_NESTED_GUEST_MODE   0x00000001
> #define KVM_STATE_NESTED_RUN_PENDING  0x00000002
> #define KVM_STATE_NESTED_EVMCS                0x00000004
> @@ -390,6 +393,11 @@ struct kvm_sync_regs {
> #define KVM_STATE_NESTED_SMM_GUEST_MODE       0x00000001
> #define KVM_STATE_NESTED_SMM_VMXON    0x00000002
> 
> +struct kvm_vmx_nested_state_data {
> +     __u8 vmcs12[0x1000];
> +     __u8 shadow_vmcs12[0x1000];
> +};

Do you think we should replace this 0x1000 with VMCS12_SIZE?

> +
> struct kvm_vmx_nested_state {
>       __u64 vmxon_pa;
>       __u64 vmcs_pa;
> @@ -397,6 +405,9 @@ struct kvm_vmx_nested_state {
>       struct {
>               __u16 flags;
>       } smm;
> +
> +     __u8 pad[120 - 18];
> +     struct kvm_vmx_nested_state_data data[0];
> };

I don’t like this pad[] thing.
It creates weird dependency between the padding in kvm_nested_state and this 
one.
Also, it doesn’t separate nicely between header & data regions.
What do you think on the following alternative patch?
(Note that it should still preserve kvm_nested_state struct size)

-struct kvm_vmx_nested_state {
+struct kvm_vmx_nested_state_data {
+       __u8 vmcs12[0x1000];
+       __u8 shadow_vmcs12[0x1000];
+};
+
+struct kvm_vmx_nested_state_hdr {
        __u64 vmxon_pa;
-       __u64 vmcs_pa;
+       __u64 vmcs12_pa;

        struct {
                __u16 flags;
        } smm;
 };

+struct kvm_svm_nested_state_data {
+       /* TODO: Implement */
+};
+
+struct kvm_svm_nested_state_hdr {
+       /* TODO: Implement */
+};
+
 /* for KVM_CAP_NESTED_STATE */
 struct kvm_nested_state {
-       /* KVM_STATE_* flags */
        __u16 flags;
-
-       /* 0 for VMX, 1 for SVM.  */
        __u16 format;
-
-       /* 128 for SVM, 128 + VMCS size for VMX.  */
        __u32 size;

        union {
-               /* VMXON, VMCS */
-               struct kvm_vmx_nested_state vmx;
+               struct kvm_vmx_nested_state_hdr vmx;
+               struct kvm_svm_nested_state_hdr svm;

                /* Pad the header to 128 bytes.  */
                __u8 pad[120];
-       };
+       } hdr;

-       __u8 data[0];
+       /*
+        * Define data region as 0 bytes to preserve backwards-compatability
+        * to old definition of kvm_nested_state in order to avoid changing
+        * KVM_{GET,PUT}_NESTED_STATE ioctl values.
+        */
+       union {
+               struct kvm_vmx_nested_state_data vmx[0];
+               struct kvm_svm_nested_state_data svm[0];
+       } data;
 };

I think this is cleaner.

-Liran