[PATCH 0/3] Various SVM Cleanups

[PATCH 0/3] Various SVM Cleanups

[Brian Woods]

This is a small series of SVM cleanup patches.  The first patch is
correcting a couple of checks relating to VGIF.  The other two are
ensuring that nested SVM functionality is emulated/setup more
correctly.

Brian Woods (3):
  x86/svm: update VGIF support
  x86/svm: add EFER SVME support for VGIF/VLOAD
  x86/svm: correct EFER.SVME intercept checks

 xen/arch/x86/hvm/svm/nestedsvm.c | 24 ++++++++++---
 xen/arch/x86/hvm/svm/svm.c       | 77 ++++++++++++++++++++++++++++++++++++++--
 xen/arch/x86/hvm/svm/vmcb.c      | 17 ---------
 3 files changed, 94 insertions(+), 24 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] x86/svm: update VGIF support

[Brian Woods]

There are places where the GIF value is checked.  A guest with VGIF
enabled can change the GIF value without the host being involved,
therefore it needs to check the GIF value in the VMCB rather the one in
the nestedsvm struct.

Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index b6f6449d75..1f7b0d3e88 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -800,8 +800,13 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct cpu_user_regs *regs,
     struct nestedvcpu *nv = &vcpu_nestedhvm(v);
     struct nestedsvm *svm = &vcpu_nestedsvm(v);
     struct vmcb_struct *ns_vmcb;
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+
+    if ( vmcb->_vintr.fields.vgif_enable )
+        ASSERT(vmcb->_vintr.fields.vgif == 0);
+    else
+        ASSERT(svm->ns_gif == 0);
 
-    ASSERT(svm->ns_gif == 0);
     ns_vmcb = nv->nv_vvmcx;
 
     if (nv->nv_vmexit_pending) {
@@ -1343,8 +1348,13 @@ nestedsvm_vmexit_defer(struct vcpu *v,
     uint64_t exitcode, uint64_t exitinfo1, uint64_t exitinfo2)
 {
     struct nestedsvm *svm = &vcpu_nestedsvm(v);
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+
+    if ( vmcb->_vintr.fields.vgif_enable )
+        vmcb->_vintr.fields.vgif = 0;
+    else
+        nestedsvm_vcpu_clgi(v);
 
-    nestedsvm_vcpu_clgi(v);
     svm->ns_vmexit.exitcode = exitcode;
     svm->ns_vmexit.exitinfo1 = exitinfo1;
     svm->ns_vmexit.exitinfo2 = exitinfo2;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] x86/svm: update VGIF support

[Boris Ostrovsky]

On 01/31/2018 03:35 PM, Brian Woods wrote:
> There are places where the GIF value is checked.  A guest with VGIF
> enabled can change the GIF value without the host being involved,
> therefore it needs to check the GIF value in the VMCB rather the one in
> the nestedsvm struct.
> 
> Signed-off-by: Brian Woods <brian.woods@amd.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/vmcb.c | 17 -----------
 2 files changed, 69 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..7864ee39ae 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -601,6 +601,73 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
     }
 }
 
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+static void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+    struct nestedsvm *svm = &vcpu_nestedsvm(v);
+    u32 general2_intercepts;
+    vintr_t vintr;
+
+   /*
+    * Need state for transfering the nested gif status so only write on
+    * the hvm_vcpu EFER.SVME changing.
+    */
+    if ( (v->arch.hvm_vcpu.guest_efer & EFER_SVME) &&
+         nestedhvm_enabled(v->domain))
+    {
+        if ( (vmcb->virt_ext.fields.vloadsave_enable == 0) &&
+             paging_mode_hap(v->domain) &&
+             cpu_has_svm_vloadsave )
+        {
+            vmcb->virt_ext.fields.vloadsave_enable = 1;
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+                                     GENERAL2_INTERCEPT_VMSAVE);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+
+        if ( (vmcb->_vintr.fields.vgif_enable == 0) &&
+             cpu_has_svm_vgif )
+        {
+            vintr = vmcb_get_vintr(vmcb);
+            vintr.fields.vgif = svm->ns_gif;
+            vintr.fields.vgif_enable = 1;
+            vmcb_set_vintr(vmcb, vintr);
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+                                     GENERAL2_INTERCEPT_CLGI);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+    }
+    else
+    {
+        if ( vmcb->virt_ext.fields.vloadsave_enable == 1 )
+        {
+            vmcb->virt_ext.fields.vloadsave_enable = 0;
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+                                    GENERAL2_INTERCEPT_VMSAVE);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+
+        if ( vmcb->_vintr.fields.vgif_enable == 1 )
+        {
+            vintr = vmcb_get_vintr(vmcb);
+            svm->ns_gif = vintr.fields.vgif;
+            vintr.fields.vgif_enable = 0;
+            vmcb_set_vintr(vmcb, vintr);
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+                                    GENERAL2_INTERCEPT_CLGI);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+    }
+}
+
 static void svm_update_guest_efer(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
@@ -611,6 +678,8 @@ static void svm_update_guest_efer(struct vcpu *v)
     if ( lma )
         new_efer |= EFER_LME;
     vmcb_set_efer(vmcb, new_efer);
+
+    svm_nested_features_on_efer_update(v);
 }
 
 static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
 
         /* PAT is under complete control of SVM when using nested paging. */
         svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-        /* Use virtual VMLOAD/VMSAVE if available. */
-        if ( cpu_has_svm_vloadsave )
-        {
-            vmcb->virt_ext.fields.vloadsave_enable = 1;
-            vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-            vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-        }
     }
     else
     {
         vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
     }
 
-    /* if available, enable and configure virtual gif */
-    if ( cpu_has_svm_vgif )
-    {
-        vmcb->_vintr.fields.vgif = 1;
-        vmcb->_vintr.fields.vgif_enable = 1;
-        vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_STGI;
-        vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_CLGI;
-    }
-
     if ( cpu_has_pause_filter )
     {
         vmcb->_pause_filter_count = SVM_PAUSEFILTER_INIT;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Boris Ostrovsky]

On 01/31/2018 03:35 PM, Brian Woods wrote:
> Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Brian Woods <brian.woods@amd.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Jan Beulich]

>>> On 31.01.18 at 21:35, <brian.woods@amd.com> wrote:
> Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Brian Woods <brian.woods@amd.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c  | 69 
> +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/svm/vmcb.c | 17 -----------
>  2 files changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index c48fdfaa5d..7864ee39ae 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -601,6 +601,73 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
>      }
>  }
>  
> +/*
> + * This runs on EFER change to see if nested features need to either be
> + * turned off or on.
> + */
> +static void svm_nested_features_on_efer_update(struct vcpu *v)
> +{
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
> +    u32 general2_intercepts;
> +    vintr_t vintr;
> +
> +   /*
> +    * Need state for transfering the nested gif status so only write on
> +    * the hvm_vcpu EFER.SVME changing.
> +    */
> +    if ( (v->arch.hvm_vcpu.guest_efer & EFER_SVME) &&
> +         nestedhvm_enabled(v->domain))

If the latter check was moved to the caller, the whole function
would perhaps be better placed in nestedsvm.c?

> +    {
> +        if ( (vmcb->virt_ext.fields.vloadsave_enable == 0) &&

Can you please avoid "== 0" and "== 1" on boolean fields (even
if, like in the case here, the bitfield has u64 as underlying type,
which is sort of pointless)?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

On Mon, Feb 05, 2018 at 02:09:15AM -0700, Jan Beulich wrote:
> If the latter check was moved to the caller, the whole function
> would perhaps be better placed in nestedsvm.c?

I thought about putting it in nestedsvm.c but I thought having it as a
static function would be better.  I could move it to nestedsvm.c 
though.

> Can you please avoid "== 0" and "== 1" on boolean fields (even
> if, like in the case here, the bitfield has u64 as underlying type,
> which is sort of pointless)?
> 
> Jan

svm.c is older and uses "== X", and I was trying to keep it consistent
within the file.  For things like if () {\n,} \{, which are
inconsistent, I always use if () \n \{.  Should I only be using boolean?

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Jan Beulich]

>>> On 05.02.18 at 17:47, <brian.woods@amd.com> wrote:
> On Mon, Feb 05, 2018 at 02:09:15AM -0700, Jan Beulich wrote:
>> If the latter check was moved to the caller, the whole function
>> would perhaps be better placed in nestedsvm.c?
> 
> I thought about putting it in nestedsvm.c but I thought having it as a
> static function would be better.  I could move it to nestedsvm.c 
> though.
> 
>> Can you please avoid "== 0" and "== 1" on boolean fields (even
>> if, like in the case here, the bitfield has u64 as underlying type,
>> which is sort of pointless)?
> 
> svm.c is older and uses "== X", and I was trying to keep it consistent
> within the file.  For things like if () {\n,} \{, which are
> inconsistent, I always use if () \n \{.  Should I only be using boolean?

Well, unless one of the maintainers of the file objects, I think it
would be better to adjust your patch.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Andrew Cooper]

On 31/01/18 20:35, Brian Woods wrote:
> Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Brian Woods <brian.woods@amd.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/svm/vmcb.c | 17 -----------
>  2 files changed, 69 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index c48fdfaa5d..7864ee39ae 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -601,6 +601,73 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
>      }
>  }
>  
> +/*
> + * This runs on EFER change to see if nested features need to either be
> + * turned off or on.
> + */
> +static void svm_nested_features_on_efer_update(struct vcpu *v)
> +{
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
> +    u32 general2_intercepts;
> +    vintr_t vintr;
> +
> +   /*
> +    * Need state for transfering the nested gif status so only write on
> +    * the hvm_vcpu EFER.SVME changing.
> +    */

Indenting is off, but that can be fixed on commit.

> +    if ( (v->arch.hvm_vcpu.guest_efer & EFER_SVME) &&
> +         nestedhvm_enabled(v->domain))
> +    {
> +        if ( (vmcb->virt_ext.fields.vloadsave_enable == 0) &&
> +             paging_mode_hap(v->domain) &&
> +             cpu_has_svm_vloadsave )
> +        {
> +            vmcb->virt_ext.fields.vloadsave_enable = 1;
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
> +                                     GENERAL2_INTERCEPT_VMSAVE);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +
> +        if ( (vmcb->_vintr.fields.vgif_enable == 0) &&
> +             cpu_has_svm_vgif )
> +        {
> +            vintr = vmcb_get_vintr(vmcb);
> +            vintr.fields.vgif = svm->ns_gif;
> +            vintr.fields.vgif_enable = 1;
> +            vmcb_set_vintr(vmcb, vintr);
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
> +                                     GENERAL2_INTERCEPT_CLGI);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +    }
> +    else
> +    {
> +        if ( vmcb->virt_ext.fields.vloadsave_enable == 1 )
> +        {
> +            vmcb->virt_ext.fields.vloadsave_enable = 0;
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
> +                                    GENERAL2_INTERCEPT_VMSAVE);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +
> +        if ( vmcb->_vintr.fields.vgif_enable == 1 )
> +        {
> +            vintr = vmcb_get_vintr(vmcb);
> +            svm->ns_gif = vintr.fields.vgif;
> +            vintr.fields.vgif_enable = 0;
> +            vmcb_set_vintr(vmcb, vintr);
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
> +                                    GENERAL2_INTERCEPT_CLGI);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +    }
> +}
> +

As some extra cleanup, what about folding this diff in?  It avoids
repeatedly hitting the cleanbits, and is clearer to follow IMO.

~Andrew

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index e750bed..a0f8e76 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -609,63 +609,58 @@ static void
svm_nested_features_on_efer_update(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     struct nestedsvm *svm = &vcpu_nestedsvm(v);
-    u32 general2_intercepts;
-    vintr_t vintr;
-
-   /*
-    * Need state for transfering the nested gif status so only write on
-    * the hvm_vcpu EFER.SVME changing.
-    */
-    if ( (v->arch.hvm_vcpu.guest_efer & EFER_SVME) &&
-         nestedhvm_enabled(v->domain))
-    {
-        if ( (vmcb->virt_ext.fields.vloadsave_enable == 0) &&
-             paging_mode_hap(v->domain) &&
-             cpu_has_svm_vloadsave )
+    uint32_t general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+    uint32_t orig_general2_intercepts = general2_intercepts;
+    vintr_t vintr = vmcb_get_vintr(vmcb), orig_vintr = vintr;
+
+    if ( !nestedhvm_enabled(v->domain) )
+        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
+
+    /*
+     * Need state for transfering the nested gif status so only write on
+     * the hvm_vcpu EFER.SVME changing.
+     */
+    if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
+    {
+        if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+             paging_mode_hap(v->domain) && cpu_has_svm_vloadsave )
         {
             vmcb->virt_ext.fields.vloadsave_enable = 1;
-            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
             general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
                                      GENERAL2_INTERCEPT_VMSAVE);
-            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
         }
 
-        if ( (vmcb->_vintr.fields.vgif_enable == 0) &&
-             cpu_has_svm_vgif )
+        if ( !vintr.fields.vgif_enable && cpu_has_svm_vgif )
         {
-            vintr = vmcb_get_vintr(vmcb);
             vintr.fields.vgif = svm->ns_gif;
             vintr.fields.vgif_enable = 1;
-            vmcb_set_vintr(vmcb, vintr);
-            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
             general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
                                      GENERAL2_INTERCEPT_CLGI);
-            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
         }
     }
     else
     {
-        if ( vmcb->virt_ext.fields.vloadsave_enable == 1 )
+        if ( vmcb->virt_ext.fields.vloadsave_enable )
         {
             vmcb->virt_ext.fields.vloadsave_enable = 0;
-            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
             general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
                                     GENERAL2_INTERCEPT_VMSAVE);
-            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
         }
 
-        if ( vmcb->_vintr.fields.vgif_enable == 1 )
+        if ( vintr.fields.vgif_enable )
         {
-            vintr = vmcb_get_vintr(vmcb);
             svm->ns_gif = vintr.fields.vgif;
             vintr.fields.vgif_enable = 0;
-            vmcb_set_vintr(vmcb, vintr);
-            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
             general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
                                     GENERAL2_INTERCEPT_CLGI);
-            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
         }
     }
+
+    if ( general2_intercepts != orig_general2_intercepts )
+        vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+
+    if ( vintr.bytes != orig_vintr.bytes )
+        vmcb_set_vintr(vmcb, vintr);
 }
 
 static void svm_update_guest_efer(struct vcpu *v)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

On Mon, Feb 05, 2018 at 03:37:06PM +0000, Andrew Cooper wrote:
> Indenting is off, but that can be fixed on commit.

Oopsy, sorry about that.  

> As some extra cleanup, what about folding this diff in?  It avoids
> repeatedly hitting the cleanbits, and is clearer to follow IMO.
> 
> ~Andrew
> 

It's functionally the same.  It's a trade off of whether you want to
write/read to the VMCB twice (assuming both are changing) or go through
more if statements every time.  The clean bits are just a field in the
VMCB so.  Some of the VMCB fields are only read on change with my
version but to be honest, I don't think it matters either way.  I like
the locality of mine better, but I'm perfectly fine with either.

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c  | 71 +++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/vmcb.c | 17 -----------
 2 files changed, 71 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..3e9c81b0e4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -601,6 +601,75 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
     }
 }
 
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+static void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+    struct nestedsvm *svm = &vcpu_nestedsvm(v);
+    u32 general2_intercepts;
+    vintr_t vintr;
+
+    if ( !nestedhvm_enabled(v->domain) )
+        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
+
+    /*
+     * Need state for transfering the nested gif status so only write on
+     * the hvm_vcpu EFER.SVME changing.
+     */
+    if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
+    {
+        if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+             paging_mode_hap(v->domain) &&
+             cpu_has_svm_vloadsave )
+        {
+            vmcb->virt_ext.fields.vloadsave_enable = 1;
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+                                     GENERAL2_INTERCEPT_VMSAVE);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+
+        if ( !vmcb->_vintr.fields.vgif_enable &&
+             cpu_has_svm_vgif )
+        {
+            vintr = vmcb_get_vintr(vmcb);
+            vintr.fields.vgif = svm->ns_gif;
+            vintr.fields.vgif_enable = 1;
+            vmcb_set_vintr(vmcb, vintr);
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+                                     GENERAL2_INTERCEPT_CLGI);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+    }
+    else
+    {
+        if ( vmcb->virt_ext.fields.vloadsave_enable )
+        {
+            vmcb->virt_ext.fields.vloadsave_enable = 0;
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+                                    GENERAL2_INTERCEPT_VMSAVE);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+
+        if ( vmcb->_vintr.fields.vgif_enable )
+        {
+            vintr = vmcb_get_vintr(vmcb);
+            svm->ns_gif = vintr.fields.vgif;
+            vintr.fields.vgif_enable = 0;
+            vmcb_set_vintr(vmcb, vintr);
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+                                    GENERAL2_INTERCEPT_CLGI);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+    }
+}
+
 static void svm_update_guest_efer(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
@@ -611,6 +680,8 @@ static void svm_update_guest_efer(struct vcpu *v)
     if ( lma )
         new_efer |= EFER_LME;
     vmcb_set_efer(vmcb, new_efer);
+
+    svm_nested_features_on_efer_update(v);
 }
 
 static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
 
         /* PAT is under complete control of SVM when using nested paging. */
         svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-        /* Use virtual VMLOAD/VMSAVE if available. */
-        if ( cpu_has_svm_vloadsave )
-        {
-            vmcb->virt_ext.fields.vloadsave_enable = 1;
-            vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-            vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-        }
     }
     else
     {
         vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
     }
 
-    /* if available, enable and configure virtual gif */
-    if ( cpu_has_svm_vgif )
-    {
-        vmcb->_vintr.fields.vgif = 1;
-        vmcb->_vintr.fields.vgif_enable = 1;
-        vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_STGI;
-        vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_CLGI;
-    }
-
     if ( cpu_has_pause_filter )
     {
         vmcb->_pause_filter_count = SVM_PAUSEFILTER_INIT;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

If Andy wants to use his version of this, that's fine also.  This is
just a newer version based on Jan's input.

v1 -> v2
    Got rid of "== X"s
    Added an assert and got rid of a check in an if

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Jan Beulich]

>>> On 07.02.18 at 22:06, <brian.woods@amd.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -601,6 +601,75 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
>      }
>  }
>  
> +/*
> + * This runs on EFER change to see if nested features need to either be
> + * turned off or on.
> + */
> +static void svm_nested_features_on_efer_update(struct vcpu *v)

I'm afraid I continue to be confused: A function with this name should
imo, as said earlier, live in nestedsvm.c. However ...

> +{
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
> +    u32 general2_intercepts;
> +    vintr_t vintr;
> +
> +    if ( !nestedhvm_enabled(v->domain) )
> +        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));

... this indicates that the function does something even for the
non-nested case. In particular ...

> +    /*
> +     * Need state for transfering the nested gif status so only write on
> +     * the hvm_vcpu EFER.SVME changing.
> +     */
> +    if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
> +    {
> +        if ( !vmcb->virt_ext.fields.vloadsave_enable &&
> +             paging_mode_hap(v->domain) &&
> +             cpu_has_svm_vloadsave )
> +        {
> +            vmcb->virt_ext.fields.vloadsave_enable = 1;
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
> +                                     GENERAL2_INTERCEPT_VMSAVE);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +
> +        if ( !vmcb->_vintr.fields.vgif_enable &&
> +             cpu_has_svm_vgif )
> +        {
> +            vintr = vmcb_get_vintr(vmcb);
> +            vintr.fields.vgif = svm->ns_gif;
> +            vintr.fields.vgif_enable = 1;
> +            vmcb_set_vintr(vmcb, vintr);
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
> +                                     GENERAL2_INTERCEPT_CLGI);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +    }
> +    else
> +    {
> +        if ( vmcb->virt_ext.fields.vloadsave_enable )
> +        {
> +            vmcb->virt_ext.fields.vloadsave_enable = 0;
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
> +                                    GENERAL2_INTERCEPT_VMSAVE);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +
> +        if ( vmcb->_vintr.fields.vgif_enable )
> +        {
> +            vintr = vmcb_get_vintr(vmcb);
> +            svm->ns_gif = vintr.fields.vgif;
> +            vintr.fields.vgif_enable = 0;
> +            vmcb_set_vintr(vmcb, vintr);
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
> +                                    GENERAL2_INTERCEPT_CLGI);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +    }

... this entire else block. Is it necessary to do this in the non-nested
case? IOW - do these settings ever change there (I would have
thought that the two *_enable fields checked by the two if()s should
never be true for nested-disabled guests)? Otherwise, as also said
before, the caller should call here only when
nestedhvm_enabled(v->domain), and the function would better
move.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

On Thu, Feb 08, 2018 at 02:45:31AM -0700, Jan Beulich wrote:
> I'm afraid I continue to be confused: A function with this name should
> imo, as said earlier, live in nestedsvm.c. However ...

I'll move it to nestedsvm.c then. 

> ... this indicates that the function does something even for the
> non-nested case. In particular ...

It makes sure that SVME isn't set when nested isn't enabled.  

If SVME is set it does certain checks to enable features if enabled.
Else it makes sure nested features are turned off.

The reason for this is that, with VGIF/VVMLOAD, they can still work even
with SVME not being set.  This sets it where they get intercepted to Xen
so that Xen can correctly emulate what should be happening.  If SVME
isn't set then nested guest shouldn't be able to do VGIF/VVMLOAD. 

> ... this entire else block. Is it necessary to do this in the non-nested
> case? IOW - do these settings ever change there (I would have
> thought that the two *_enable fields checked by the two if()s should
> never be true for nested-disabled guests)? Otherwise, as also said
> before, the caller should call here only when
> nestedhvm_enabled(v->domain), and the function would better
> move.
> 
> Jan
 
It only checks two things (two if statements) in the VMCB per EFER
update.  Suppose you add an if to check if it's a nested guest and then
run the nested_features func.  You're only saving a total of one if and
going in and out a function but you add a small divergence in the
codepath.  If this was a long computer/IO intense function, I'd
completely agree but this is two very simple checks.

I'll change it though, since it'll be easier than going back and forth
about a minor detail.

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Jan Beulich]

>>> On 08.02.18 at 18:01, <brian.woods@amd.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
>      if ( lma )
>          new_efer |= EFER_LME;
>      vmcb_set_efer(vmcb, new_efer);
> +
> +    if ( !nestedhvm_enabled(v->domain) )
> +        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
> +
> +    if ( nestedhvm_enabled(v->domain) )
> +        svm_nested_features_on_efer_update(v);
>  }

Why not

    if ( nestedhvm_enabled(v->domain) )
        svm_nested_features_on_efer_update(v);
    else
        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));

?

Jan




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Woods, Brian]

Pardon any weird formatting, I'm replying on my phone. 

Because they are two different things.  One is an assert to make sure nothing wrong is happening with the EFER.SVME bit, and the other changes what features are enabled.  

IIRC, most asserts are on their on ifs and not in a if statement with something else.  I guess I should have put the assert higher in the function though but that's a small detail.  

Yes, you can squeeze both in one if statement but, but it being cleaner and easier to read (at least more logical) is better than getting rid of one if in my opinion.  Plus assuming asserts are disabled for release, I'd assume the extra if would get optimized out by gcc anyway. 

Brian


On February 13, 2018 03:31:40 Jan Beulich <JBeulich@suse.com> wrote:

>>>> On 08.02.18 at 18:01, <brian.woods@amd.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
>>      if ( lma )
>>          new_efer |= EFER_LME;
>>      vmcb_set_efer(vmcb, new_efer);
>> +
>> +    if ( !nestedhvm_enabled(v->domain) )
>> +        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
>> +
>> +    if ( nestedhvm_enabled(v->domain) )
>> +        svm_nested_features_on_efer_update(v);
>>  }
>
> Why not
>
>     if ( nestedhvm_enabled(v->domain) )
>         svm_nested_features_on_efer_update(v);
>     else
>         ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
>
> ?
>
> Jan
>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Jan Beulich]

>>> On 13.02.18 at 19:37, <Brian.Woods@amd.com> wrote:
> Pardon any weird formatting, I'm replying on my phone. 
> 
> Because they are two different things.  One is an assert to make sure 
> nothing wrong is happening with the EFER.SVME bit, and the other changes what 
> features are enabled.  
> 
> IIRC, most asserts are on their on ifs and not in a if statement with 
> something else.  I guess I should have put the assert higher in the function 
> though but that's a small detail.  
> 
> Yes, you can squeeze both in one if statement but, but it being cleaner and 
> easier to read (at least more logical) is better than getting rid of one if 
> in my opinion.  Plus assuming asserts are disabled for release, I'd assume 
> the extra if would get optimized out by gcc anyway. 

In that case it would better be

ASSERT(nestedhvm_enabled(v->domain) || !(v->arch.hvm_vcpu.guest_efer & EFER_SVME));

(suitably line wrapped if necessary). But I continue to think the
if/else variant looks better overall. It'll be the SVM maintainers to
decide, though.

Jan

> On February 13, 2018 03:31:40 Jan Beulich <JBeulich@suse.com> wrote:
> 
>>>>> On 08.02.18 at 18:01, <brian.woods@amd.com> wrote:
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
>>>      if ( lma )
>>>          new_efer |= EFER_LME;
>>>      vmcb_set_efer(vmcb, new_efer);
>>> +
>>> +    if ( !nestedhvm_enabled(v->domain) )
>>> +        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
>>> +
>>> +    if ( nestedhvm_enabled(v->domain) )
>>> +        svm_nested_features_on_efer_update(v);
>>>  }
>>
>> Why not
>>
>>     if ( nestedhvm_enabled(v->domain) )
>>         svm_nested_features_on_efer_update(v);
>>     else
>>         ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
>>
>> ?
>>
>> Jan
>>
>>
>>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c        | 66 +++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c              |  6 +++
 xen/arch/x86/hvm/svm/vmcb.c             | 17 ---------
 xen/include/asm-x86/hvm/svm/nestedsvm.h |  1 +
 4 files changed, 73 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 1f7b0d3e88..9295e583d7 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1659,3 +1659,69 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
 
     __update_guest_eip(regs, inst_len);
 }
+
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+    struct nestedsvm *svm = &vcpu_nestedsvm(v);
+    u32 general2_intercepts;
+    vintr_t vintr;
+
+    /*
+     * Need state for transfering the nested gif status so only write on
+     * the hvm_vcpu EFER.SVME changing.
+     */
+    if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
+    {
+        if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+             paging_mode_hap(v->domain) &&
+             cpu_has_svm_vloadsave )
+        {
+            vmcb->virt_ext.fields.vloadsave_enable = 1;
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+                                     GENERAL2_INTERCEPT_VMSAVE);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+
+        if ( !vmcb->_vintr.fields.vgif_enable &&
+             cpu_has_svm_vgif )
+        {
+            vintr = vmcb_get_vintr(vmcb);
+            vintr.fields.vgif = svm->ns_gif;
+            vintr.fields.vgif_enable = 1;
+            vmcb_set_vintr(vmcb, vintr);
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+                                     GENERAL2_INTERCEPT_CLGI);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+    }
+    else
+    {
+        if ( vmcb->virt_ext.fields.vloadsave_enable )
+        {
+            vmcb->virt_ext.fields.vloadsave_enable = 0;
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+                                    GENERAL2_INTERCEPT_VMSAVE);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+
+        if ( vmcb->_vintr.fields.vgif_enable )
+        {
+            vintr = vmcb_get_vintr(vmcb);
+            svm->ns_gif = vintr.fields.vgif;
+            vintr.fields.vgif_enable = 0;
+            vmcb_set_vintr(vmcb, vintr);
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+                                    GENERAL2_INTERCEPT_CLGI);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+    }
+}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..be08a5aa5e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
     if ( lma )
         new_efer |= EFER_LME;
     vmcb_set_efer(vmcb, new_efer);
+
+    if ( !nestedhvm_enabled(v->domain) )
+        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
+
+    if ( nestedhvm_enabled(v->domain) )
+        svm_nested_features_on_efer_update(v);
 }
 
 static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
 
         /* PAT is under complete control of SVM when using nested paging. */
         svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-        /* Use virtual VMLOAD/VMSAVE if available. */
-        if ( cpu_has_svm_vloadsave )
-        {
-            vmcb->virt_ext.fields.vloadsave_enable = 1;
-            vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-            vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-        }
     }
     else
     {
         vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
     }
 
-    /* if available, enable and configure virtual gif */
-    if ( cpu_has_svm_vgif )
-    {
-        vmcb->_vintr.fields.vgif = 1;
-        vmcb->_vintr.fields.vgif_enable = 1;
-        vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_STGI;
-        vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_CLGI;
-    }
-
     if ( cpu_has_pause_filter )
     {
         vmcb->_pause_filter_count = SVM_PAUSEFILTER_INIT;
diff --git a/xen/include/asm-x86/hvm/svm/nestedsvm.h b/xen/include/asm-x86/hvm/svm/nestedsvm.h
index a619b6131b..abcf2e7c9c 100644
--- a/xen/include/asm-x86/hvm/svm/nestedsvm.h
+++ b/xen/include/asm-x86/hvm/svm/nestedsvm.h
@@ -104,6 +104,7 @@ nestedsvm_vmexit_n2n1(struct vcpu *v, struct cpu_user_regs *regs);
 enum nestedhvm_vmexits
 nestedsvm_check_intercepts(struct vcpu *v, struct cpu_user_regs *regs,
     uint64_t exitcode);
+void svm_nested_features_on_efer_update(struct vcpu *v);
 
 /* Interface methods */
 void nsvm_vcpu_destroy(struct vcpu *v);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c        | 66 +++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c              |  6 +++
 xen/arch/x86/hvm/svm/vmcb.c             | 17 ---------
 xen/include/asm-x86/hvm/svm/nestedsvm.h |  1 +
 4 files changed, 73 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 1f7b0d3e88..9295e583d7 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1659,3 +1659,69 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
 
     __update_guest_eip(regs, inst_len);
 }
+
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+    struct nestedsvm *svm = &vcpu_nestedsvm(v);
+    u32 general2_intercepts;
+    vintr_t vintr;
+
+    /*
+     * Need state for transfering the nested gif status so only write on
+     * the hvm_vcpu EFER.SVME changing.
+     */
+    if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
+    {
+        if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+             paging_mode_hap(v->domain) &&
+             cpu_has_svm_vloadsave )
+        {
+            vmcb->virt_ext.fields.vloadsave_enable = 1;
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+                                     GENERAL2_INTERCEPT_VMSAVE);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+
+        if ( !vmcb->_vintr.fields.vgif_enable &&
+             cpu_has_svm_vgif )
+        {
+            vintr = vmcb_get_vintr(vmcb);
+            vintr.fields.vgif = svm->ns_gif;
+            vintr.fields.vgif_enable = 1;
+            vmcb_set_vintr(vmcb, vintr);
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+                                     GENERAL2_INTERCEPT_CLGI);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+    }
+    else
+    {
+        if ( vmcb->virt_ext.fields.vloadsave_enable )
+        {
+            vmcb->virt_ext.fields.vloadsave_enable = 0;
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+                                    GENERAL2_INTERCEPT_VMSAVE);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+
+        if ( vmcb->_vintr.fields.vgif_enable )
+        {
+            vintr = vmcb_get_vintr(vmcb);
+            svm->ns_gif = vintr.fields.vgif;
+            vintr.fields.vgif_enable = 0;
+            vmcb_set_vintr(vmcb, vintr);
+            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
+            general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+                                    GENERAL2_INTERCEPT_CLGI);
+            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+        }
+    }
+}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..89140f02f3 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
     if ( lma )
         new_efer |= EFER_LME;
     vmcb_set_efer(vmcb, new_efer);
+
+    ASSERT(nestedhvm_enabled(v->domain) || 
+           !(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
+
+    if ( nestedhvm_enabled(v->domain) )
+        svm_nested_features_on_efer_update(v);
 }
 
 static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
 
         /* PAT is under complete control of SVM when using nested paging. */
         svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-        /* Use virtual VMLOAD/VMSAVE if available. */
-        if ( cpu_has_svm_vloadsave )
-        {
-            vmcb->virt_ext.fields.vloadsave_enable = 1;
-            vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-            vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-        }
     }
     else
     {
         vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
     }
 
-    /* if available, enable and configure virtual gif */
-    if ( cpu_has_svm_vgif )
-    {
-        vmcb->_vintr.fields.vgif = 1;
-        vmcb->_vintr.fields.vgif_enable = 1;
-        vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_STGI;
-        vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_CLGI;
-    }
-
     if ( cpu_has_pause_filter )
     {
         vmcb->_pause_filter_count = SVM_PAUSEFILTER_INIT;
diff --git a/xen/include/asm-x86/hvm/svm/nestedsvm.h b/xen/include/asm-x86/hvm/svm/nestedsvm.h
index a619b6131b..abcf2e7c9c 100644
--- a/xen/include/asm-x86/hvm/svm/nestedsvm.h
+++ b/xen/include/asm-x86/hvm/svm/nestedsvm.h
@@ -104,6 +104,7 @@ nestedsvm_vmexit_n2n1(struct vcpu *v, struct cpu_user_regs *regs);
 enum nestedhvm_vmexits
 nestedsvm_check_intercepts(struct vcpu *v, struct cpu_user_regs *regs,
     uint64_t exitcode);
+void svm_nested_features_on_efer_update(struct vcpu *v);
 
 /* Interface methods */
 void nsvm_vcpu_destroy(struct vcpu *v);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Boris Ostrovsky]

On 02/20/2018 05:27 PM, Brian Woods wrote:
> Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Brian Woods <brian.woods@amd.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

I've seen patch 1 and 3 are in but this one isn't.  Any status on it?

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Boris Ostrovsky]

On 02/20/2018 05:00 PM, Brian Woods wrote:
> I've seen patch 1 and 3 are in but this one isn't.  Any status on it?
>

That's possibly because you needed an SVM maintainer ACK.

I think Jan was waiting for decision on how to present the ASSERT. From
the 3 options I slightly more prefer

ASSERT(nestedhvm_enabled(v->domain) || !(v->arch.hvm_vcpu.guest_efer & EFER_SVME));

(which wasn't the first choice for either of you ;-))

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

[Brian Woods]

On Tue, Feb 20, 2018 at 05:09:24PM -0500, Boris Ostrovsky wrote:
> That's possibly because you needed an SVM maintainer ACK.
> 
> I think Jan was waiting for decision on how to present the ASSERT. From
> the 3 options I slightly more prefer
> 
> ASSERT(nestedhvm_enabled(v->domain) || !(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
> 
> (which wasn't the first choice for either of you ;-))
> 
> -boris

I'll change it and send out a new version then. 

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/3] x86/svm: correct EFER.SVME intercept checks

[Brian Woods]

Corrects some EFER.SVME checks in intercepts.  See AMD APM vol2 section
15.4 for more details.  VMMCALL isn't checked due to guests needing it
to boot.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 10 ++++++++--
 xen/arch/x86/hvm/svm/svm.c       |  8 +++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 1f7b0d3e88..1f5981fc18 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1620,7 +1620,12 @@ void svm_vmexit_do_stgi(struct cpu_user_regs *regs, struct vcpu *v)
 {
     unsigned int inst_len;
 
-    if ( !nestedhvm_enabled(v->domain) ) {
+    /*
+     * STGI doesn't require SVME to be set to be used.  See AMD APM vol
+     * 2 section 15.4 for details.
+     */
+    if ( !nestedhvm_enabled(v->domain) )
+    {
         hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         return;
     }
@@ -1640,7 +1645,8 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
     uint32_t general1_intercepts = vmcb_get_general1_intercepts(vmcb);
     vintr_t intr;
 
-    if ( !nestedhvm_enabled(v->domain) ) {
+    if ( !nsvm_efer_svm_enabled(v) )
+    {
         hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         return;
     }
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 7864ee39ae..2599f02ab6 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2255,7 +2255,6 @@ svm_vmexit_do_vmrun(struct cpu_user_regs *regs,
 {
     if ( !nsvm_efer_svm_enabled(v) )
     {
-        gdprintk(XENLOG_ERR, "VMRUN: nestedhvm disabled, injecting #UD\n");
         hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         return;
     }
@@ -2310,7 +2309,6 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
 
     if ( !nsvm_efer_svm_enabled(v) ) 
     {
-        gdprintk(XENLOG_ERR, "VMLOAD: nestedhvm disabled, injecting #UD\n");
         hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         return;
     }
@@ -2346,7 +2344,6 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
 
     if ( !nsvm_efer_svm_enabled(v) ) 
     {
-        gdprintk(XENLOG_ERR, "VMSAVE: nestedhvm disabled, injecting #UD\n");
         hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         return;
     }
@@ -2820,6 +2817,11 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     case VMEXIT_INVLPGA:
+        if ( !nsvm_efer_svm_enabled(v) )
+        {
+            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+            break;
+        }
         if ( (inst_len = __get_instruction_length(v, INSTR_INVLPGA)) == 0 )
             break;
         svm_invlpga_intercept(v, regs->rax, regs->ecx);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] x86/svm: correct EFER.SVME intercept checks

[Boris Ostrovsky]

On 01/31/2018 03:35 PM, Brian Woods wrote:
> Corrects some EFER.SVME checks in intercepts.  See AMD APM vol2 section
> 15.4 for more details.  VMMCALL isn't checked due to guests needing it
> to boot.
> 

Don't you need SVME be on for VMMCALL?

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] x86/svm: correct EFER.SVME intercept checks

[Andrew Cooper]

On 03/02/18 17:03, Boris Ostrovsky wrote:
>
>
> On 01/31/2018 03:35 PM, Brian Woods wrote:
>> Corrects some EFER.SVME checks in intercepts.  See AMD APM vol2 section
>> 15.4 for more details.  VMMCALL isn't checked due to guests needing it
>> to boot.
>>
>
> Don't you need SVME be on for VMMCALL?

In real hardware, yes.

However, it's quite unreasonable to require the guest to have activated
its idea of EFER.SVME before it can make hypercalls :)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] x86/svm: correct EFER.SVME intercept checks

[Boris Ostrovsky]

On 02/03/2018 12:10 PM, Andrew Cooper wrote:
> On 03/02/18 17:03, Boris Ostrovsky wrote:
>>
>>
>> On 01/31/2018 03:35 PM, Brian Woods wrote:
>>> Corrects some EFER.SVME checks in intercepts.  See AMD APM vol2 section
>>> 15.4 for more details.  VMMCALL isn't checked due to guests needing it
>>> to boot.
>>>
>>
>> Don't you need SVME be on for VMMCALL?
> 
> In real hardware, yes.
> 
> However, it's quite unreasonable to require the guest to have activated
> its idea of EFER.SVME before it can make hypercalls :)



Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] x86/svm: correct EFER.SVME intercept checks

[Jan Beulich]

>>> On 03.02.18 at 18:15, <boris.ostrovsky@oracle.com> wrote:
> On 02/03/2018 12:10 PM, Andrew Cooper wrote:
>> On 03/02/18 17:03, Boris Ostrovsky wrote:
>>>
>>>
>>> On 01/31/2018 03:35 PM, Brian Woods wrote:
>>>> Corrects some EFER.SVME checks in intercepts.  See AMD APM vol2 section
>>>> 15.4 for more details.  VMMCALL isn't checked due to guests needing it
>>>> to boot.
>>>>
>>>
>>> Don't you need SVME be on for VMMCALL?
>> 
>> In real hardware, yes.
>> 
>> However, it's quite unreasonable to require the guest to have activated
>> its idea of EFER.SVME before it can make hypercalls :)
> 
> 
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

I've applied this patch together with patch 1 on the assumption
that it doesn't depend on patch 2 (which I've commented on).
Let me know if this was a mistake and there is a dependency (in
which case I'll revert).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel