x86/svm: Fix svm_vmcb_dump() when used in current context

[Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context

Posted by Andrew Cooper 4 years, 10 months ago

VMExit doesn't switch all state.  The FS/GS/TS/LDTR/GSBASE segment
information, and SYSCALL/SYSENTER MSRs may still be cached in hardware, rather
than up-to-date in the VMCB.

Export svm_sync_vmcb() via svmdebug.h so svm_vmcb_dump() can use it, and bring
the VMCB into sync in current context.

As a minor optimisation, switch svm_sync_vmcb() to use svm_vm{load,save}_pa(),
as svm->vmcb_pa is always in correct, and this avoids a redundant __pa()
translation behind the scenes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c             | 6 +++---
 xen/arch/x86/hvm/svm/svmdebug.c        | 9 +++++++++
 xen/include/asm-x86/hvm/svm/svmdebug.h | 1 +
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index cd6a6b3..0eac9ce 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -627,21 +627,21 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
                       cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
 }
 
-static void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state)
+void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state)
 {
     struct svm_vcpu *svm = &v->arch.hvm.svm;
 
     if ( new_state == vmcb_needs_vmsave )
     {
         if ( svm->vmcb_sync_state == vmcb_needs_vmload )
-            svm_vmload(svm->vmcb);
+            svm_vmload_pa(svm->vmcb_pa);
 
         svm->vmcb_sync_state = new_state;
     }
     else
     {
         if ( svm->vmcb_sync_state == vmcb_needs_vmsave )
-            svm_vmsave(svm->vmcb);
+            svm_vmsave_pa(svm->vmcb_pa);
 
         if ( svm->vmcb_sync_state != vmcb_needs_vmload )
             svm->vmcb_sync_state = new_state;
diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index d35e405..4293d8d 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -29,6 +29,15 @@ static void svm_dump_sel(const char *name, const struct segment_register *s)
 
 void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
 {
+    struct vcpu *curr = current;
+
+    /*
+     * If we are dumping the VMCB currently in context, some guest state may
+     * still be cached in hardware.  Retrieve it.
+     */
+    if ( vmcb == curr->arch.hvm.svm.vmcb )
+        svm_sync_vmcb(curr, vmcb_in_sync);
+
     printk("Dumping guest's current state at %s...\n", from);
     printk("Size of VMCB = %zu, paddr = %"PRIpaddr", vaddr = %p\n",
            sizeof(struct vmcb_struct), virt_to_maddr(vmcb), vmcb);
diff --git a/xen/include/asm-x86/hvm/svm/svmdebug.h b/xen/include/asm-x86/hvm/svm/svmdebug.h
index 658cdd3..330c1d9 100644
--- a/xen/include/asm-x86/hvm/svm/svmdebug.h
+++ b/xen/include/asm-x86/hvm/svm/svmdebug.h
@@ -22,6 +22,7 @@
 #include <asm/types.h>
 #include <asm/hvm/svm/vmcb.h>
 
+void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state);
 void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb);
 bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
                       const struct vcpu *v, bool verbose);
-- 
2.1.4


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

Re: [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context

Posted by Jan Beulich 4 years, 10 months ago

>>> On 17.06.19 at 14:54, <andrew.cooper3@citrix.com> wrote:
> VMExit doesn't switch all state.  The FS/GS/TS/LDTR/GSBASE segment
> information, and SYSCALL/SYSENTER MSRs may still be cached in hardware, rather
> than up-to-date in the VMCB.
> 
> Export svm_sync_vmcb() via svmdebug.h so svm_vmcb_dump() can use it, and bring
> the VMCB into sync in current context.
> 
> As a minor optimisation, switch svm_sync_vmcb() to use svm_vm{load,save}_pa(),
> as svm->vmcb_pa is always in correct, and this avoids a redundant __pa()

Is the "in" here a leftover from some earlier, different wording?

> translation behind the scenes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Only as a remark: This removes the last use of svm_vmload(), but
perhaps it and svm_vmsave() would better be dropped together,
once the one remaining use of the latter has also disappeared
(assuming that's doable).

Jan



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

Re: [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context

Posted by Andrew Cooper 4 years, 10 months ago

On 17/06/2019 14:55, Jan Beulich wrote:
>>>> On 17.06.19 at 14:54, <andrew.cooper3@citrix.com> wrote:
>> VMExit doesn't switch all state.  The FS/GS/TS/LDTR/GSBASE segment
>> information, and SYSCALL/SYSENTER MSRs may still be cached in hardware, rather
>> than up-to-date in the VMCB.
>>
>> Export svm_sync_vmcb() via svmdebug.h so svm_vmcb_dump() can use it, and bring
>> the VMCB into sync in current context.
>>
>> As a minor optimisation, switch svm_sync_vmcb() to use svm_vm{load,save}_pa(),
>> as svm->vmcb_pa is always in correct, and this avoids a redundant __pa()
> Is the "in" here a leftover from some earlier, different wording?

Yes.  It can be dropped.

>
>> translation behind the scenes.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Only as a remark: This removes the last use of svm_vmload(), but
> perhaps it and svm_vmsave() would better be dropped together,
> once the one remaining use of the latter has also disappeared
> (assuming that's doable).

I noticed and tried to do just that, but removing the final svm_vmsave()
isn't trivial.  It is confined to the nested virt code, but I think it
is calling virt_to_maddr() on a domheap mapping, which needs adjusting
not to explode on a system with more than 5T of RAM.

Its in my todo list, but I don't have time to address that right now.

~Andrew

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

Re: [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context

Posted by Woods, Brian 4 years, 10 months ago

On Mon, Jun 17, 2019 at 01:54:39PM +0100, Andy Cooper wrote:
> VMExit doesn't switch all state.  The FS/GS/TS/LDTR/GSBASE segment
> information, and SYSCALL/SYSENTER MSRs may still be cached in hardware, rather
> than up-to-date in the VMCB.
> 
> Export svm_sync_vmcb() via svmdebug.h so svm_vmcb_dump() can use it, and bring
> the VMCB into sync in current context.
> 
> As a minor optimisation, switch svm_sync_vmcb() to use svm_vm{load,save}_pa(),
> as svm->vmcb_pa is always in correct, and this avoids a redundant __pa()
> translation behind the scenes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Brian Woods <brian.woods@amd.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c             | 6 +++---
>  xen/arch/x86/hvm/svm/svmdebug.c        | 9 +++++++++
>  xen/include/asm-x86/hvm/svm/svmdebug.h | 1 +
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index cd6a6b3..0eac9ce 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -627,21 +627,21 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
>                        cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
>  }
>  
> -static void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state)
> +void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state)
>  {
>      struct svm_vcpu *svm = &v->arch.hvm.svm;
>  
>      if ( new_state == vmcb_needs_vmsave )
>      {
>          if ( svm->vmcb_sync_state == vmcb_needs_vmload )
> -            svm_vmload(svm->vmcb);
> +            svm_vmload_pa(svm->vmcb_pa);
>  
>          svm->vmcb_sync_state = new_state;
>      }
>      else
>      {
>          if ( svm->vmcb_sync_state == vmcb_needs_vmsave )
> -            svm_vmsave(svm->vmcb);
> +            svm_vmsave_pa(svm->vmcb_pa);
>  
>          if ( svm->vmcb_sync_state != vmcb_needs_vmload )
>              svm->vmcb_sync_state = new_state;
> diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
> index d35e405..4293d8d 100644
> --- a/xen/arch/x86/hvm/svm/svmdebug.c
> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
> @@ -29,6 +29,15 @@ static void svm_dump_sel(const char *name, const struct segment_register *s)
>  
>  void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb)
>  {
> +    struct vcpu *curr = current;
> +
> +    /*
> +     * If we are dumping the VMCB currently in context, some guest state may
> +     * still be cached in hardware.  Retrieve it.
> +     */
> +    if ( vmcb == curr->arch.hvm.svm.vmcb )
> +        svm_sync_vmcb(curr, vmcb_in_sync);
> +
>      printk("Dumping guest's current state at %s...\n", from);
>      printk("Size of VMCB = %zu, paddr = %"PRIpaddr", vaddr = %p\n",
>             sizeof(struct vmcb_struct), virt_to_maddr(vmcb), vmcb);
> diff --git a/xen/include/asm-x86/hvm/svm/svmdebug.h b/xen/include/asm-x86/hvm/svm/svmdebug.h
> index 658cdd3..330c1d9 100644
> --- a/xen/include/asm-x86/hvm/svm/svmdebug.h
> +++ b/xen/include/asm-x86/hvm/svm/svmdebug.h
> @@ -22,6 +22,7 @@
>  #include <asm/types.h>
>  #include <asm/hvm/svm/vmcb.h>
>  
> +void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state);
>  void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb);
>  bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
>                        const struct vcpu *v, bool verbose);
> -- 
> 2.1.4
> 

-- 
Brian Woods

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