* [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context
@ 2019-06-17 12:54 Andrew Cooper
2019-06-17 13:55 ` Jan Beulich
2019-06-19 16:23 ` Woods, Brian
0 siblings, 2 replies; 4+ messages in thread
From: Andrew Cooper @ 2019-06-17 12:54 UTC (permalink / raw)
To: Xen-devel
Cc: Jan Beulich, Wei Liu, Andrew Cooper, Suravee Suthikulpanit,
Boris Ostrovsky, Brian Woods, Roger Pau Monné
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context
2019-06-17 12:54 [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context Andrew Cooper
@ 2019-06-17 13:55 ` Jan Beulich
2019-06-17 13:59 ` Andrew Cooper
2019-06-19 16:23 ` Woods, Brian
1 sibling, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2019-06-17 13:55 UTC (permalink / raw)
To: Andrew Cooper
Cc: WeiLiu, Suravee Suthikulpanit, xen-devel, Boris Ostrovsky,
Brian Woods, Roger Pau Monne
>>> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context
2019-06-17 13:55 ` Jan Beulich
@ 2019-06-17 13:59 ` Andrew Cooper
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Cooper @ 2019-06-17 13:59 UTC (permalink / raw)
To: Jan Beulich
Cc: WeiLiu, Suravee Suthikulpanit, xen-devel, Boris Ostrovsky,
Brian Woods, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context
2019-06-17 12:54 [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context Andrew Cooper
2019-06-17 13:55 ` Jan Beulich
@ 2019-06-19 16:23 ` Woods, Brian
1 sibling, 0 replies; 4+ messages in thread
From: Woods, Brian @ 2019-06-19 16:23 UTC (permalink / raw)
To: Andrew Cooper
Cc: Jan Beulich, Wei Liu, Suthikulpanit, Suravee, Xen-devel,
Boris Ostrovsky, Woods, Brian, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-19 16:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 12:54 [Xen-devel] [PATCH] x86/svm: Fix svm_vmcb_dump() when used in current context Andrew Cooper
2019-06-17 13:55 ` Jan Beulich
2019-06-17 13:59 ` Andrew Cooper
2019-06-19 16:23 ` Woods, Brian
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).