Message ID | 1560776079-9866-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/svm: Fix svm_vmcb_dump() when used in current context | expand |
>>> 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
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
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 >
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);
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(-)