[PATCH 0/2] Various SVM Patches

[PATCH 0/2] Various SVM Patches

[Brian Woods]

This is small patch series which does some basic maintenance work in the
SVM section.  

1. Add support for checking the SVME bit before doing nested SVM
   operations
2. Add support for checking if nestedhvm is enabled before enabling
   nested SVM features in the VMCB. 

Brian Woods (2):
  x86/svm: Add SVME checking for SVM intercepts
  x86/svm: Add checks for nested HW features

 xen/arch/x86/hvm/svm/svm.c  | 28 ++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/vmcb.c |  5 +++--
 2 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.11.0


Brian Woods (2):
  x86/svm: Add SVME checking for SVM intercepts
  x86/svm: Add checks for nested HW features

 xen/arch/x86/hvm/svm/svm.c  | 29 +++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/vmcb.c |  5 +++--
 2 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.11.0


Brian Woods (2):
  x86/svm: Add SVME checking for SVM intercepts
  x86/svm: Add checks for nested HW features

 xen/arch/x86/hvm/svm/svm.c  | 29 +++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/vmcb.c |  5 +++--
 2 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.11.0


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

[PATCH 1/2] x86/svm: Add SVME checking for SVM intercepts

[Brian Woods]

Checks the hvm EFER.SVME bit to make sure the EFER.SVME bit it high
before allowing nested SVM intercepts to be handled successfully.  On
SVME being low, it generates a #UD as per the AMD APM vol2 15.4.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 2e62b9bb6d..2d0a82ae77 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2587,6 +2587,35 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
                                      eventinj.fields.vector) )
         vmcb->eventinj = eventinj;
 
+    /*
+     * Making sure SVME is enabled see AMD APM vol2 section 15.4
+     * Nested Xen needs VMMCALL to boot.  It hasn't set SVME by the time
+     * it uses it, therefore it isn't checked
+     */
+    switch ( exit_reason )
+    {
+    case VMEXIT_VMRUN:
+    case VMEXIT_VMLOAD:
+    case VMEXIT_VMSAVE:
+    case VMEXIT_CLGI:
+    case VMEXIT_INVLPGA:
+        if ( !(nestedhvm_enabled(v->domain) &&
+             nsvm_efer_svm_enabled(v)) )
+        {
+            gdprintk(XENLOG_ERR, "nestedhvm nested/SVME disabled, injecting #UD\n");
+            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+	    goto out;
+        }
+    case VMEXIT_STGI:
+    case VMEXIT_SKINIT:
+        if ( !nestedhvm_enabled(v->domain) )
+        {
+            gdprintk(XENLOG_ERR, "nestedhvm nested disabled, injecting #UD\n");
+            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+	    goto out;
+        }
+    }
+
     switch ( exit_reason )
     {
     case VMEXIT_INTR:
-- 
2.11.0


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

Re: [PATCH 1/2] x86/svm: Add SVME checking for SVM intercepts

[Boris Ostrovsky]

On 12/21/2017 10:26 AM, Brian Woods wrote:
> Checks the hvm EFER.SVME bit to make sure the EFER.SVME bit it high

s/it/is

> before allowing nested SVM intercepts to be handled successfully.  On
> SVME being low, it generates a #UD as per the AMD APM vol2 15.4.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Brian Woods <brian.woods@amd.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 2e62b9bb6d..2d0a82ae77 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2587,6 +2587,35 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>                                       eventinj.fields.vector) )
>          vmcb->eventinj = eventinj;
>  
> +    /*
> +     * Making sure SVME is enabled see AMD APM vol2 section 15.4
> +     * Nested Xen needs VMMCALL to boot.  It hasn't set SVME by the time
> +     * it uses it, therefore it isn't checked
> +     */
> +    switch ( exit_reason )
> +    {
> +    case VMEXIT_VMRUN:
> +    case VMEXIT_VMLOAD:
> +    case VMEXIT_VMSAVE:
> +    case VMEXIT_CLGI:
> +    case VMEXIT_INVLPGA:
> +        if ( !(nestedhvm_enabled(v->domain) &&
> +             nsvm_efer_svm_enabled(v)) )


I haven't checked all of them but at least for the first two
(svm_vmexit_do_vmrun() and smv_vmexit_do_vmload()) we check EFER and
print a similar error message. So it seems they can be handled in the
switch statement below.

-boris

> +        {
> +            gdprintk(XENLOG_ERR, "nestedhvm nested/SVME disabled, injecting #UD\n");
> +            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> +	    goto out;
> +        }
> +    case VMEXIT_STGI:
> +    case VMEXIT_SKINIT:
> +        if ( !nestedhvm_enabled(v->domain) )
> +        {
> +            gdprintk(XENLOG_ERR, "nestedhvm nested disabled, injecting #UD\n");
> +            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> +	    goto out;
> +        }
> +    }
> +
>      switch ( exit_reason )
>      {
>      case VMEXIT_INTR:




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

Re: [PATCH 1/2] x86/svm: Add SVME checking for SVM intercepts

[Brian Woods]

On Thu, Dec 21, 2017 at 11:52:27PM -0500, Boris Ostrovsky wrote:
> s/it/is

Oopsy, thank you. 

> I haven't checked all of them but at least for the first two
> (svm_vmexit_do_vmrun() and smv_vmexit_do_vmload()) we check EFER and
> print a similar error message. So it seems they can be handled in the
> switch statement below.
> 
> -boris

They are indeed checked in those functions.  I also need to check to
make sure that the switching from L2 to L1 is done correctly in cause of
SVME being low.  I tested it with Xen and it worked correctly but it
might not be failing correctly.  The joys of being under the weather and
forgetting all your mental notes.  I'll try to send out a v2 of this
patch today with the corrections. 

Thank you for your input.

-- 
Brian Woods

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

[PATCH 2/2] x86/svm: Add checks for nested HW features

[Brian Woods]

Add a nestedhvm_enable() check to the existing checks for setting the
virtual GIF and virtual VMLOAD/VMSAVE features.  If it isn't a nestedhvm
guest, do not enable the features in the VMCB.

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

diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..62f7d7f7dd 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -27,6 +27,7 @@
 #include <asm/msr-index.h>
 #include <asm/p2m.h>
 #include <asm/hvm/support.h>
+#include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/svm/svmdebug.h>
 
@@ -202,7 +203,7 @@ static int construct_vmcb(struct vcpu *v)
         svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
 
         /* Use virtual VMLOAD/VMSAVE if available. */
-        if ( cpu_has_svm_vloadsave )
+        if ( cpu_has_svm_vloadsave && nestedhvm_enabled(v->domain) )
         {
             vmcb->virt_ext.fields.vloadsave_enable = 1;
             vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
@@ -215,7 +216,7 @@ static int construct_vmcb(struct vcpu *v)
     }
 
     /* if available, enable and configure virtual gif */
-    if ( cpu_has_svm_vgif )
+    if ( cpu_has_svm_vgif && nestedhvm_enabled(v->domain) )
     {
         vmcb->_vintr.fields.vgif = 1;
         vmcb->_vintr.fields.vgif_enable = 1;
-- 
2.11.0


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

Re: [PATCH 2/2] x86/svm: Add checks for nested HW features

[Boris Ostrovsky]

On 12/21/2017 10:26 AM, Brian Woods wrote:
> Add a nestedhvm_enable() check to the existing checks for setting the
> virtual GIF and virtual VMLOAD/VMSAVE features.  If it isn't a nestedhvm
> guest, do not enable the features in the VMCB.
>
> 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/2] x86/svm: Add checks for nested HW features

[Andrew Cooper]

On 21/12/17 15:26, Brian Woods wrote:
> Add a nestedhvm_enable() check to the existing checks for setting the
> virtual GIF and virtual VMLOAD/VMSAVE features.  If it isn't a nestedhvm
> guest, do not enable the features in the VMCB.
>
> Signed-off-by: Brian Woods <brian.woods@amd.com>

Unfortunately, nestedhvm_enabled() is guaranteed to be false at the
point that construct_vmcb() is called (due the order in which
information appears while constructing the VM), which means we will
never enable these optimisations.

Combined with the observation of EFER in the pipeline, the logic to
enable/disable these optimisations needs to be in
svm_update_guest_efer(), and need to trigger when EFER.SVME changes.

~Andrew

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

Re: [PATCH 2/2] x86/svm: Add checks for nested HW features

[Brian Woods]

On Fri, Dec 22, 2017 at 03:15:48PM +0000, Andrew Cooper wrote:
> Unfortunately, nestedhvm_enabled() is guaranteed to be false at the
> point that construct_vmcb() is called (due the order in which
> information appears while constructing the VM), which means we will
> never enable these optimisations.
> 
> Combined with the observation of EFER in the pipeline, the logic to
> enable/disable these optimisations needs to be in
> svm_update_guest_efer(), and need to trigger when EFER.SVME changes.
> 
> ~Andrew

Sorry for the late reply.  I tired working this before vacation but it
turned out to be a little bit longer than that... I have a set of
patches that _should_ work, but there are other issues.  Turns out there
are interrupt issues with nestted SVM HVM and I'm trying to hunt those
down and fix them so I can properly test the patches I've done.  Oddly
enough you can at least get a system booted on 17h family systems even
if it's fragile but 15h just fails to even boot.  Not sure how it even
worked when I tested previous patches on the 15h system. 

-- 
Brian Woods

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