[PATCH 03/14] Nested Virtualization: data structure

[PATCH 03/14] Nested Virtualization: data structure

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]


Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_nh03_structdata.diff --]
[-- Type: text/x-diff, Size: 4147 bytes --]

# HG changeset patch
# User cegger
# Date 1280925496 -7200
Data structures for Nested Virtualization

diff -r e75662c48917 -r 79a75ef7b7d0 xen/arch/x86/hvm/svm/vmcb.c
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -40,9 +40,6 @@
 
 extern int svm_dbg_on;
 
-#define IOPM_SIZE   (12 * 1024)
-#define MSRPM_SIZE  (8  * 1024)
-
 struct vmcb_struct *alloc_vmcb(void) 
 {
     struct vmcb_struct *vmcb;
diff -r e75662c48917 -r 79a75ef7b7d0 xen/include/asm-x86/hvm/hvm.h
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -52,7 +52,8 @@ enum hvm_intblk {
     hvm_intblk_shadow,    /* MOV-SS or STI shadow */
     hvm_intblk_rflags_ie, /* RFLAGS.IE == 0 */
     hvm_intblk_tpr,       /* LAPIC TPR too high */
-    hvm_intblk_nmi_iret   /* NMI blocked until IRET */
+    hvm_intblk_nmi_iret,  /* NMI blocked until IRET */
+    hvm_intblk_gif,       /* GIF cleared */
 };
 
 /* These happen to be the same as the VMX interrupt shadow definitions. */
@@ -180,6 +181,8 @@ int hvm_girq_dest_2_vcpu_id(struct domai
     (hvm_paging_enabled(v) && ((v)->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PAE))
 #define hvm_nx_enabled(v) \
     (!!((v)->arch.hvm_vcpu.guest_efer & EFER_NX))
+#define hvm_svm_enabled(v) \
+    (!!((v)->arch.hvm_vcpu.guest_efer & EFER_SVME))
 
 #define hvm_hap_has_1gb(d) \
     (hvm_funcs.hap_capabilities & HVM_HAP_SUPERPAGE_1GB)
diff -r e75662c48917 -r 79a75ef7b7d0 xen/include/asm-x86/hvm/svm/vmcb.h
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -364,6 +364,9 @@ typedef union
     } fields;
 } __attribute__ ((packed)) lbrctrl_t;
 
+#define IOPM_SIZE   (12 * 1024)
+#define MSRPM_SIZE  (8  * 1024)
+
 struct vmcb_struct {
     u32 cr_intercepts;          /* offset 0x00 */
     u32 dr_intercepts;          /* offset 0x04 */
diff -r e75662c48917 -r 79a75ef7b7d0 xen/include/asm-x86/hvm/vcpu.h
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -35,6 +35,57 @@ enum hvm_io_state {
     HVMIO_completed
 };
 
+struct nestedhvm {
+    bool_t nh_gif; /* vcpu's GIF, always true on VMX */
+    bool_t nh_guestmode; /* vcpu in guestmode? */
+    void *nh_vm; /* VMCB/VMCS */
+    size_t nh_vmsize; /* size of VMCB/VMCS */
+
+    /* guest vm address of 1st level guest, needed for VMEXIT */
+    uint64_t nh_vmaddr;
+    uint64_t nh_vmmaxaddr; /* Maximum supported address */
+    void *nh_hostsave;
+
+    void *nh_arch; /* SVM/VMX specific data */
+    size_t nh_arch_size;
+
+    /* Cached real MSR permission bitmaps of the nested guest */
+    unsigned long *nh_cached_msrpm;
+    size_t nh_cached_msrpm_size;
+    /* Merged MSR permission bitmap */
+    unsigned long *nh_merged_msrpm;
+    size_t nh_merged_msrpm_size;
+
+    /* Cached cr3/hcr3 the guest sets up for the nested guest.
+     * Used by Shadow-on-Shadow and Nested-on-Nested. */
+    uint64_t nh_vmcb_cr3, nh_vmcb_hcr3;
+    uint32_t nh_guest_asid;
+    bool_t nh_flushp2m;
+    struct p2m_domain *nh_p2m; /* used p2m table for this vcpu */
+
+    /* Only meaningful when forcevmexit flag is set */
+    struct {
+        uint64_t exitcode;  /* generic exitcode */
+        uint64_t exitinfo1; /* additional information to the exitcode */
+        uint64_t exitinfo2; /* additional information to the exitcode */
+    } nh_forcevmexit;
+    union {
+        uint32_t bytes;
+        struct {
+            uint32_t rflagsif : 1;
+            uint32_t vintrmask : 1; /* always cleared on VMX */
+            uint32_t forcevmexit : 1;
+            uint32_t vmentry : 1;   /* true during vmentry/vmexit emulation */
+            uint32_t reserved : 28;	
+        } fields;
+    } nh_hostflags;
+
+    bool_t nh_hap_enabled;
+};
+
+#define VCPU_HVM(v)       ((v)->arch.hvm_vcpu)
+#define VCPU_NESTEDHVM(v) (VCPU_HVM((v)).nestedhvm)
+
 struct hvm_vcpu {
     /* Guest control-register and EFER values, just as the guest sees them. */
     unsigned long       guest_cr[5];
@@ -86,6 +137,8 @@ struct hvm_vcpu {
 
     struct tasklet      assert_evtchn_irq_tasklet;
 
+    struct nestedhvm    nestedhvm;
+
     struct mtrr_state   mtrr;
     u64                 pat_cr;
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH 03/14] Nested Virtualization: data structure

[Dong, Eddie]

> # HG changeset patch
> # User cegger
> # Date 1280925496 -7200
> Data structures for Nested Virtualization
> 
> diff -r e75662c48917 -r 79a75ef7b7d0 xen/arch/x86/hvm/svm/vmcb.c
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -40,9 +40,6 @@
> 
>  extern int svm_dbg_on;
> 
> -#define IOPM_SIZE   (12 * 1024)
> -#define MSRPM_SIZE  (8  * 1024)
> -
>  struct vmcb_struct *alloc_vmcb(void)
>  {
>      struct vmcb_struct *vmcb;
> diff -r e75662c48917 -r 79a75ef7b7d0 xen/include/asm-x86/hvm/hvm.h
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -52,7 +52,8 @@ enum hvm_intblk {
>      hvm_intblk_shadow,    /* MOV-SS or STI shadow */
>      hvm_intblk_rflags_ie, /* RFLAGS.IE == 0 */
>      hvm_intblk_tpr,       /* LAPIC TPR too high */
> -    hvm_intblk_nmi_iret   /* NMI blocked until IRET */
> +    hvm_intblk_nmi_iret,  /* NMI blocked until IRET */
> +    hvm_intblk_gif,       /* GIF cleared */
>  };

Should we move that to svm.h? We can had a wrap sub-structure here if you want to accomodate both situation.

> 
>  /* These happen to be the same as the VMX interrupt shadow
> definitions. */ @@ -180,6 +181,8 @@ int
>      hvm_girq_dest_2_vcpu_id(struct domai (hvm_paging_enabled(v) &&
>  ((v)->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PAE)) #define
>      hvm_nx_enabled(v) \ (!!((v)->arch.hvm_vcpu.guest_efer & EFER_NX))
> +#define hvm_svm_enabled(v) \
> +    (!!((v)->arch.hvm_vcpu.guest_efer & EFER_SVME))
> 

ditto.

>  #define hvm_hap_has_1gb(d) \
>      (hvm_funcs.hap_capabilities & HVM_HAP_SUPERPAGE_1GB)
> diff -r e75662c48917 -r 79a75ef7b7d0
> xen/include/asm-x86/hvm/svm/vmcb.h ---
> a/xen/include/asm-x86/hvm/svm/vmcb.h +++
> b/xen/include/asm-x86/hvm/svm/vmcb.h @@ -364,6 +364,9 @@ typedef union
>      } fields;
>  } __attribute__ ((packed)) lbrctrl_t;
> 
> +#define IOPM_SIZE   (12 * 1024)
> +#define MSRPM_SIZE  (8  * 1024)
> +
>  struct vmcb_struct {
>      u32 cr_intercepts;          /* offset 0x00 */
>      u32 dr_intercepts;          /* offset 0x04 */
> diff -r e75662c48917 -r 79a75ef7b7d0 xen/include/asm-x86/hvm/vcpu.h
> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -35,6 +35,57 @@ enum hvm_io_state {
>      HVMIO_completed
>  };
> 
> +struct nestedhvm {
> +    bool_t nh_gif; /* vcpu's GIF, always true on VMX */
> +    bool_t nh_guestmode; /* vcpu in guestmode? */
> +    void *nh_vm; /* VMCB/VMCS */
> +    size_t nh_vmsize; /* size of VMCB/VMCS */
> +
> +    /* guest vm address of 1st level guest, needed for VMEXIT */
> +    uint64_t nh_vmaddr;
> +    uint64_t nh_vmmaxaddr; /* Maximum supported address */
> +    void *nh_hostsave;
> +
> +    void *nh_arch; /* SVM/VMX specific data */
> +    size_t nh_arch_size;
> +
> +    /* Cached real MSR permission bitmaps of the nested guest */
> +    unsigned long *nh_cached_msrpm;
> +    size_t nh_cached_msrpm_size;
> +    /* Merged MSR permission bitmap */
> +    unsigned long *nh_merged_msrpm;
> +    size_t nh_merged_msrpm_size;
> +
> +    /* Cached cr3/hcr3 the guest sets up for the nested guest.
> +     * Used by Shadow-on-Shadow and Nested-on-Nested. */
> +    uint64_t nh_vmcb_cr3, nh_vmcb_hcr3;
> +    uint32_t nh_guest_asid;
> +    bool_t nh_flushp2m;
> +    struct p2m_domain *nh_p2m; /* used p2m table for this vcpu */
> +
> +    /* Only meaningful when forcevmexit flag is set */
> +    struct {
> +        uint64_t exitcode;  /* generic exitcode */
> +        uint64_t exitinfo1; /* additional information to the
> exitcode */ +        uint64_t exitinfo2; /* additional information to
> the exitcode */ +    } nh_forcevmexit;
> +    union {
> +        uint32_t bytes;
> +        struct {
> +            uint32_t rflagsif : 1;
> +            uint32_t vintrmask : 1; /* always cleared on VMX */
> +            uint32_t forcevmexit : 1;
> +            uint32_t vmentry : 1;   /* true during vmentry/vmexit
> emulation */ +            uint32_t reserved : 28;
> +        } fields;
> +    } nh_hostflags;
> +
> +    bool_t nh_hap_enabled;
> +};

ditto.
We can split above structure into common part, and SVM/VMX specific sub-structure.

> +
> +#define VCPU_HVM(v)       ((v)->arch.hvm_vcpu)
> +#define VCPU_NESTEDHVM(v) (VCPU_HVM((v)).nestedhvm)
> +
>  struct hvm_vcpu {
>      /* Guest control-register and EFER values, just as the guest
>      sees them. */ unsigned long       guest_cr[5];
> @@ -86,6 +137,8 @@ struct hvm_vcpu {
> 
>      struct tasklet      assert_evtchn_irq_tasklet;
> 
> +    struct nestedhvm    nestedhvm;
> +
>      struct mtrr_state   mtrr;
>      u64                 pat_cr;

Thx, Eddie

Re: [PATCH 03/14] Nested Virtualization: data structure

[Keir Fraser]

On 17/08/2010 08:47, "Dong, Eddie" <eddie.dong@intel.com> wrote:

>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -52,7 +52,8 @@ enum hvm_intblk {
>>      hvm_intblk_shadow,    /* MOV-SS or STI shadow */
>>      hvm_intblk_rflags_ie, /* RFLAGS.IE == 0 */
>>      hvm_intblk_tpr,       /* LAPIC TPR too high */
>> -    hvm_intblk_nmi_iret   /* NMI blocked until IRET */
>> +    hvm_intblk_nmi_iret,  /* NMI blocked until IRET */
>> +    hvm_intblk_gif,       /* GIF cleared */
>>  };
> 
> Should we move that to svm.h? We can had a wrap sub-structure here if you want
> to accomodate both situation.

Arguably not worth it. I'd call it hvm_intblk_svm_gif or somesuch however.

>>  /* These happen to be the same as the VMX interrupt shadow
>> definitions. */ @@ -180,6 +181,8 @@ int
>>      hvm_girq_dest_2_vcpu_id(struct domai (hvm_paging_enabled(v) &&
>>  ((v)->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PAE)) #define
>>      hvm_nx_enabled(v) \ (!!((v)->arch.hvm_vcpu.guest_efer & EFER_NX))
>> +#define hvm_svm_enabled(v) \
>> +    (!!((v)->arch.hvm_vcpu.guest_efer & EFER_SVME))
>> 
> 
> ditto.

Again arguably not worth it. Although it is easy to move...

>> +struct nestedhvm {
>> +    bool_t nh_gif; /* vcpu's GIF, always true on VMX */
>> +    bool_t nh_guestmode; /* vcpu in guestmode? */
>> +    void *nh_vm; /* VMCB/VMCS */
>> +    size_t nh_vmsize; /* size of VMCB/VMCS */
>> +
>> +    /* guest vm address of 1st level guest, needed for VMEXIT */
>> +    uint64_t nh_vmaddr;
>> +    uint64_t nh_vmmaxaddr; /* Maximum supported address */
>> +    void *nh_hostsave;
>> +
>> +    void *nh_arch; /* SVM/VMX specific data */
>> +    size_t nh_arch_size;
>> +
>> +    /* Cached real MSR permission bitmaps of the nested guest */
>> +    unsigned long *nh_cached_msrpm;
>> +    size_t nh_cached_msrpm_size;
>> +    /* Merged MSR permission bitmap */
>> +    unsigned long *nh_merged_msrpm;
>> +    size_t nh_merged_msrpm_size;
>> +
>> +    /* Cached cr3/hcr3 the guest sets up for the nested guest.
>> +     * Used by Shadow-on-Shadow and Nested-on-Nested. */
>> +    uint64_t nh_vmcb_cr3, nh_vmcb_hcr3;
>> +    uint32_t nh_guest_asid;
>> +    bool_t nh_flushp2m;
>> +    struct p2m_domain *nh_p2m; /* used p2m table for this vcpu */
>> +
>> +    /* Only meaningful when forcevmexit flag is set */
>> +    struct {
>> +        uint64_t exitcode;  /* generic exitcode */
>> +        uint64_t exitinfo1; /* additional information to the
>> exitcode */ +        uint64_t exitinfo2; /* additional information to
>> the exitcode */ +    } nh_forcevmexit;
>> +    union {
>> +        uint32_t bytes;
>> +        struct {
>> +            uint32_t rflagsif : 1;
>> +            uint32_t vintrmask : 1; /* always cleared on VMX */
>> +            uint32_t forcevmexit : 1;
>> +            uint32_t vmentry : 1;   /* true during vmentry/vmexit
>> emulation */ +            uint32_t reserved : 28;
>> +        } fields;
>> +    } nh_hostflags;
>> +
>> +    bool_t nh_hap_enabled;
>> +};
> 
> ditto.
> We can split above structure into common part, and SVM/VMX specific
> sub-structure.

Yes, we should be strict on the layout of this structure. SVM/VMX-specific
stuff goes into a sub-structure in a union. Absolutely. And you would only
go peeking at the SVM sub-structure if hvm_svm_enabled(v)==TRUE. And we'd
have a similar predicate hvm_vmx_enabled(v)==TRUE, presumably. And maybe a
generic hvm_nestedvirt_enabled(v) too.

 -- Keir


>> +
>> +#define VCPU_HVM(v)       ((v)->arch.hvm_vcpu)
>> +#define VCPU_NESTEDHVM(v) (VCPU_HVM((v)).nestedhvm)
>> +
>>  struct hvm_vcpu {
>>      /* Guest control-register and EFER values, just as the guest
>>      sees them. */ unsigned long       guest_cr[5];
>> @@ -86,6 +137,8 @@ struct hvm_vcpu {
>> 
>>      struct tasklet      assert_evtchn_irq_tasklet;
>> 
>> +    struct nestedhvm    nestedhvm;
>> +
>>      struct mtrr_state   mtrr;
>>      u64                 pat_cr;
> 
> Thx, Eddie
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 03/14] Nested Virtualization: data structure

[Christoph Egger]

On Tuesday 17 August 2010 10:03:28 Keir Fraser wrote:
> On 17/08/2010 08:47, "Dong, Eddie" <eddie.dong@intel.com> wrote:
> >> --- a/xen/include/asm-x86/hvm/hvm.h
> >> +++ b/xen/include/asm-x86/hvm/hvm.h
> >> @@ -52,7 +52,8 @@ enum hvm_intblk {
> >>      hvm_intblk_shadow,    /* MOV-SS or STI shadow */
> >>      hvm_intblk_rflags_ie, /* RFLAGS.IE == 0 */
> >>      hvm_intblk_tpr,       /* LAPIC TPR too high */
> >> -    hvm_intblk_nmi_iret   /* NMI blocked until IRET */
> >> +    hvm_intblk_nmi_iret,  /* NMI blocked until IRET */
> >> +    hvm_intblk_gif,       /* GIF cleared */
> >>  };
> >
> > Should we move that to svm.h? We can had a wrap sub-structure here if you
> > want to accomodate both situation.
>
> Arguably not worth it. I'd call it hvm_intblk_svm_gif or somesuch however.

Done.

>
> >>  /* These happen to be the same as the VMX interrupt shadow
> >> definitions. */ @@ -180,6 +181,8 @@ int
> >>      hvm_girq_dest_2_vcpu_id(struct domai (hvm_paging_enabled(v) &&
> >>  ((v)->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PAE)) #define
> >>      hvm_nx_enabled(v) \ (!!((v)->arch.hvm_vcpu.guest_efer & EFER_NX))
> >> +#define hvm_svm_enabled(v) \
> >> +    (!!((v)->arch.hvm_vcpu.guest_efer & EFER_SVME))
> >
> > ditto.
>
> Again arguably not worth it. Although it is easy to move...
>
> >> +struct nestedhvm {
> >> +    bool_t nh_gif; /* vcpu's GIF, always true on VMX */
> >> +    bool_t nh_guestmode; /* vcpu in guestmode? */
> >> +    void *nh_vm; /* VMCB/VMCS */
> >> +    size_t nh_vmsize; /* size of VMCB/VMCS */
> >> +
> >> +    /* guest vm address of 1st level guest, needed for VMEXIT */
> >> +    uint64_t nh_vmaddr;
> >> +    uint64_t nh_vmmaxaddr; /* Maximum supported address */
> >> +    void *nh_hostsave;
> >> +
> >> +    void *nh_arch; /* SVM/VMX specific data */
> >> +    size_t nh_arch_size;
> >> +
> >> +    /* Cached real MSR permission bitmaps of the nested guest */
> >> +    unsigned long *nh_cached_msrpm;
> >> +    size_t nh_cached_msrpm_size;
> >> +    /* Merged MSR permission bitmap */
> >> +    unsigned long *nh_merged_msrpm;
> >> +    size_t nh_merged_msrpm_size;
> >> +
> >> +    /* Cached cr3/hcr3 the guest sets up for the nested guest.
> >> +     * Used by Shadow-on-Shadow and Nested-on-Nested. */
> >> +    uint64_t nh_vmcb_cr3, nh_vmcb_hcr3;

I renamed these to nh_vm_guestcr3 and nh_vm_hostcr3 for better clarity
how it is used.

> >> +    uint32_t nh_guest_asid;
> >> +    bool_t nh_flushp2m;
> >> +    struct p2m_domain *nh_p2m; /* used p2m table for this vcpu */
> >> +
> >> +    /* Only meaningful when forcevmexit flag is set */
> >> +    struct {
> >> +        uint64_t exitcode;  /* generic exitcode */
> >> +        uint64_t exitinfo1; /* additional information to the
> >> exitcode */ +        uint64_t exitinfo2; /* additional information to
> >> the exitcode */ +    } nh_forcevmexit;
> >> +    union {
> >> +        uint32_t bytes;
> >> +        struct {
> >> +            uint32_t rflagsif : 1;
> >> +            uint32_t vintrmask : 1; /* always cleared on VMX */
> >> +            uint32_t forcevmexit : 1;
> >> +            uint32_t vmentry : 1;   /* true during vmentry/vmexit
> >> emulation */ +            uint32_t reserved : 28;
> >> +        } fields;
> >> +    } nh_hostflags;
> >> +
> >> +    bool_t nh_hap_enabled;
> >> +};
> >
> > ditto.
> > We can split above structure into common part, and SVM/VMX specific
> > sub-structure.
>
> Yes, we should be strict on the layout of this structure. SVM/VMX-specific
> stuff goes into a sub-structure in a union. Absolutely.

I have moved the SVM/VMX-specific pieces into the 'void *nh_arch' field above.
It is initialized in the svm/vmx specific vcpu initialization.

When you look into the svm specific patch, you will find a 'struct nestedsvm'
in xen/include/asm-x86/hvm/svm/vmcb.h

> And you would only go peeking at the SVM sub-structure
> if hvm_svm_enabled(v)==TRUE.

Correct. On a Intel CPU Xen should never allow the guest to
set the EFER.SVME bit.

> And we'd have a similar predicate hvm_vmx_enabled(v)==TRUE, presumably.
> And maybe a generic hvm_nestedvirt_enabled(v) too.

What you call hvm_nestedvirt_enabled() actually exists as nestedhvm_enabled().

>  -- Keir
>
> >> +
> >> +#define VCPU_HVM(v)       ((v)->arch.hvm_vcpu)
> >> +#define VCPU_NESTEDHVM(v) (VCPU_HVM((v)).nestedhvm)
> >> +
> >>  struct hvm_vcpu {
> >>      /* Guest control-register and EFER values, just as the guest
> >>      sees them. */ unsigned long       guest_cr[5];
> >> @@ -86,6 +137,8 @@ struct hvm_vcpu {
> >>
> >>      struct tasklet      assert_evtchn_irq_tasklet;
> >>
> >> +    struct nestedhvm    nestedhvm;
> >> +
> >>      struct mtrr_state   mtrr;
> >>      u64                 pat_cr;
> >
> > Thx, Eddie



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 03/14] Nested Virtualization: data structure

[Keir Fraser]

On 17/08/2010 11:01, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

>> Yes, we should be strict on the layout of this structure. SVM/VMX-specific
>> stuff goes into a sub-structure in a union. Absolutely.
> 
> I have moved the SVM/VMX-specific pieces into the 'void *nh_arch' field above.
> It is initialized in the svm/vmx specific vcpu initialization.

I suggest to make this a union including SVM/VMX-specific struct pointers.
It will avoid unnecessary explicit casting, and you can use an anonymous
union if you want. Is using pointers better than actually including the
structures in the union, do you think?

So I mean something like: union {
    void *nh_arch;
    struct nestedsvm *nh_svm;
    struct nestedvmx *nh_vmx;
};

Or: union {
    struct nestedsvm nh_svm;
    struct nestedvmx nh_vmx;
};

What is the nh_arch_size field for? Well I can guess what it represents, but
why do you need such a thing?

> When you look into the svm specific patch, you will find a 'struct nestedsvm'
> in xen/include/asm-x86/hvm/svm/vmcb.h
> 
>> And you would only go peeking at the SVM sub-structure
>> if hvm_svm_enabled(v)==TRUE.
> 
> Correct. On a Intel CPU Xen should never allow the guest to
> set the EFER.SVME bit.
> 
>> And we'd have a similar predicate hvm_vmx_enabled(v)==TRUE, presumably.
>> And maybe a generic hvm_nestedvirt_enabled(v) too.
> 
> What you call hvm_nestedvirt_enabled() actually exists as nestedhvm_enabled().

Fine.

 -- Keir

Re: [PATCH 03/14] Nested Virtualization: data structure

[Christoph Egger]

On Tuesday 17 August 2010 12:28:17 Keir Fraser wrote:
> On 17/08/2010 11:01, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> >> Yes, we should be strict on the layout of this structure.
> >> SVM/VMX-specific stuff goes into a sub-structure in a union. Absolutely.
> >
> > I have moved the SVM/VMX-specific pieces into the 'void *nh_arch' field
> > above. It is initialized in the svm/vmx specific vcpu initialization.
>
> I suggest to make this a union including SVM/VMX-specific struct pointers.
> It will avoid unnecessary explicit casting, and you can use an anonymous
> union if you want. Is using pointers better than actually including the
> structures in the union, do you think?
>
> So I mean something like: union {
>     void *nh_arch;
>     struct nestedsvm *nh_svm;
>     struct nestedvmx *nh_vmx;
> };
>
> Or: union {
>     struct nestedsvm nh_svm;
>     struct nestedvmx nh_vmx;
> };

All of this is possible but a union is actually only needed if you want
to access svm or vmx specific data from common code which is
bad from the software engineering side.
The advantage of using a pointer is that gcc can point you to
such a mistake.

In svm/vmx code you don't need explicit casts with a pointer.
Have a look at the top of the nsvm_vmcb_prepare4vmrun() function
in the nh_svm patch. There you see how I access 'struct nestedsvm'
w/o a cast.

> What is the nh_arch_size field for? Well I can guess what it represents,
> but why do you need such a thing?

It's purpose is to allow to copy svm/vmx specific data to somewhere else
w/o knowing them. It is currently nowhere needed. Once it turns out
it is neither needed for VMX it can go away.

> > When you look into the svm specific patch, you will find a 'struct
> > nestedsvm' in xen/include/asm-x86/hvm/svm/vmcb.h
> >
> >> And you would only go peeking at the SVM sub-structure
> >> if hvm_svm_enabled(v)==TRUE.
> >
> > Correct. On a Intel CPU Xen should never allow the guest to
> > set the EFER.SVME bit.
> >
> >> And we'd have a similar predicate hvm_vmx_enabled(v)==TRUE, presumably.
> >> And maybe a generic hvm_nestedvirt_enabled(v) too.
> >
> > What you call hvm_nestedvirt_enabled() actually exists as
> > nestedhvm_enabled().
>
> Fine.
>
>  -- Keir



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 03/14] Nested Virtualization: data structure

[Keir Fraser]

On 17/08/2010 11:48, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> All of this is possible but a union is actually only needed if you want
> to access svm or vmx specific data from common code which is
> bad from the software engineering side.
> The advantage of using a pointer is that gcc can point you to
> such a mistake.
>
> In svm/vmx code you don't need explicit casts with a pointer.
> Have a look at the top of the nsvm_vmcb_prepare4vmrun() function
> in the nh_svm patch. There you see how I access 'struct nestedsvm'
> w/o a cast.

Hm, I see. Well that's okay I guess. Two further comments then, by the by:
(1) Not sure why you hide it behind macro VCPU_NESTEDHVM(). That seems quite
pointless and you may as well reference v->...nh_arch directly. Apart from
that I don't like capitalised macros much anyway. If you must keep a macro
(why?) then at least don't capitalise it.
(2) No text speak in function names please. I get fed up enough with
blah2blah for conversion functions. Don't bring the numeral 4 into the fray
as well. This function would be just as clear with the '4' changed to '_'.

>> What is the nh_arch_size field for? Well I can guess what it represents,
>> but why do you need such a thing?
> 
> It's purpose is to allow to copy svm/vmx specific data to somewhere else
> w/o knowing them.

Yeah, it's pointless then. Noone's going to want to copy gobs of anonymous
arcbitrary vcpu-specific data.

 -- Keir

> It is currently nowhere needed. Once it turns out
> it is neither needed for VMX it can go away.