Re: [PATCH v2 2/3] KVM:SVM: Add extended intercept support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 17, 2020 at 11:11 AM Babu Moger <babu.moger@xxxxxxx> wrote:
>
> Jim,
>
> > -----Original Message-----
> > From: kvm-owner@xxxxxxxxxxxxxxx <kvm-owner@xxxxxxxxxxxxxxx> On Behalf
> > Of Babu Moger
> > Sent: Wednesday, June 17, 2020 9:31 AM
> > To: Jim Mattson <jmattson@xxxxxxxxxx>
> > Cc: Wanpeng Li <wanpengli@xxxxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>;
> > the arch/x86 maintainers <x86@xxxxxxxxxx>; Sean Christopherson
> > <sean.j.christopherson@xxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>;
> > Borislav Petkov <bp@xxxxxxxxx>; H . Peter Anvin <hpa@xxxxxxxxx>; Paolo
> > Bonzini <pbonzini@xxxxxxxxxx>; Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>;
> > Thomas Gleixner <tglx@xxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>;
> > kvm list <kvm@xxxxxxxxxxxxxxx>
> > Subject: RE: [PATCH v2 2/3] KVM:SVM: Add extended intercept support
> >
> >
> >
> > > -----Original Message-----
> > > From: Jim Mattson <jmattson@xxxxxxxxxx>
> > > Sent: Tuesday, June 16, 2020 6:17 PM
> > > To: Moger, Babu <Babu.Moger@xxxxxxx>
> > > Cc: Wanpeng Li <wanpengli@xxxxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>;
> > > the arch/x86 maintainers <x86@xxxxxxxxxx>; Sean Christopherson
> > > <sean.j.christopherson@xxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>;
> > > Borislav Petkov <bp@xxxxxxxxx>; H . Peter Anvin <hpa@xxxxxxxxx>; Paolo
> > > Bonzini <pbonzini@xxxxxxxxxx>; Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>;
> > > Thomas Gleixner <tglx@xxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>;
> > > kvm list <kvm@xxxxxxxxxxxxxxx>
> > > Subject: Re: [PATCH v2 2/3] KVM:SVM: Add extended intercept support
> > >
> > > On Tue, Jun 16, 2020 at 3:03 PM Babu Moger <babu.moger@xxxxxxx>
> > wrote:
> > > >
> > > > The new intercept bits have been added in vmcb control
> > > > area to support the interception of INVPCID instruction.
> > > >
> > > > The following bit is added to the VMCB layout control area
> > > > to control intercept of INVPCID:
> > > >
> > > > Byte Offset     Bit(s)          Function
> > > > 14h             2               intercept INVPCID
> > > >
> > > > Add the interfaces to support these extended interception.
> > > > Also update the tracing for extended intercepts.
> > > >
> > > > AMD documentation for INVPCID feature is available at "AMD64
> > > > Architecture Programmer’s Manual Volume 2: System Programming,
> > > > Pub. 24593 Rev. 3.34(or later)"
> > > >
> > > > The documentation can be obtained at the links below:
> > > > Link:
> > >
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.a
> > >
> > md.com%2Fsystem%2Ffiles%2FTechDocs%2F24593.pdf&amp;data=02%7C01%7
> > >
> > Cbabu.moger%40amd.com%7C4cedcb3567194883601e08d8124b6be7%7C3dd8
> > >
> > 961fe4884e608e11a82d994e183d%7C0%7C0%7C637279463210520563&amp;s
> > >
> > data=oRQq0hj0O43A4lnl8JEb%2BHt8oCFHWxcqvLaA1%2BacTJc%3D&amp;reser
> > > ved=0
> > > > Link:
> > >
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> > >
> > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&amp;data=02%7C01%7Cbabu.m
> > >
> > oger%40amd.com%7C4cedcb3567194883601e08d8124b6be7%7C3dd8961fe48
> > >
> > 84e608e11a82d994e183d%7C0%7C0%7C637279463210520563&amp;sdata=EtA
> > > rCUBB8etloN%2B%2Blx42RZqai12QFvtJefnxBn1ryMQ%3D&amp;reserved=0
> > >
> > > Not your change, but this documentation is terrible. There is no
> > > INVLPCID instruction, nor is there a PCID instruction.
> >
> > Sorry about that. I will bring this to their notice.
> >
> > >
> > > > Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> > > > ---
> > > >  arch/x86/include/asm/svm.h |    3 ++-
> > > >  arch/x86/kvm/svm/nested.c  |    6 +++++-
> > > >  arch/x86/kvm/svm/svm.c     |    1 +
> > > >  arch/x86/kvm/svm/svm.h     |   18 ++++++++++++++++++
> > > >  arch/x86/kvm/trace.h       |   12 ++++++++----
> > > >  5 files changed, 34 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> > > > index 8a1f5382a4ea..62649fba8908 100644
> > > > --- a/arch/x86/include/asm/svm.h
> > > > +++ b/arch/x86/include/asm/svm.h
> > > > @@ -61,7 +61,8 @@ struct __attribute__ ((__packed__))
> > vmcb_control_area {
> > > >         u32 intercept_dr;
> > > >         u32 intercept_exceptions;
> > > >         u64 intercept;
> > > > -       u8 reserved_1[40];
> > > > +       u32 intercept_extended;
> > > > +       u8 reserved_1[36];
> > >
> > > It seems like a more straightforward implementation would simply
> > > change 'u64 intercept' to 'u32 intercept[3].'
> >
> > Sure. Will change it.
>
> This involves much more changes than I originally thought.  All these
> following code needs to be modified. Here is my cscope output for the C
> symbol intercept.
>
> 0 nested.c recalc_intercepts                123 c->intercept = h->intercept;
> 1 nested.c recalc_intercepts                135 c->intercept &= ~(1ULL <<
> INTERCEPT_VINTR);
> 2 nested.c recalc_intercepts                139 c->intercept &= ~(1ULL <<
> INTERCEPT_VMMCALL);
> 3 nested.c recalc_intercepts                144 c->intercept |= g->intercept;
> 4 nested.c copy_vmcb_control_area           153 dst->intercept =
> from->intercept;
> 5 nested.c nested_svm_vmrun_msrpm           186 if
> (!(svm->nested.ctl.intercept & (1ULL << INTERCEPT_MSR_PROT)))
> 6 nested.c nested_vmcb_check_controls 212 if ((control->intercept & (1ULL
> << INTERCEPT_VMRUN)) == 0)NIT));
> 7 nested.c nested_svm_vmrun                 436
> nested_vmcb->control.intercept);
> 8 nested.c nested_svm_exit_handled_msr      648 if
> (!(svm->nested.ctl.intercept & (1ULL << INTERCEPT_MSR_PROT)))
> 9 nested.c nested_svm_intercept_ioio        675 if
> (!(svm->nested.ctl.intercept & (1ULL << INTERCEPT_IOIO_PROT)))
> a nested.c nested_svm_intercept             732 if
> (svm->nested.ctl.intercept & exit_bits)
> b nested.c nested_exit_on_init              840 return
> (svm->nested.ctl.intercept & (1ULL << INTERCEPT_INIT));
> c svm.c    check_selective_cr0_intercepted 2205 u64 intercept;
> d svm.c    check_selective_cr0_intercepted 2207 intercept =
> svm->nested.ctl.intercept;
> e svm.c    check_selective_cr0_intercepted 2210 (!(intercept & (1ULL <<
> INTERCEPT_SELECTIVE_CR0))))
> f svm.c    dump_vmcb                       2803 pr_err("%-20s%016llx\n",
> "intercepts:", control->intercept);
> m svm.c    svm_check_intercept             3687 intercept =
> svm->nested.ctl.intercept;
> n svm.c    svm_check_intercept             3689 if (!(intercept & (1ULL <<
> INTERCEPT_SELECTIVE_CR0)))
> 6 svm.c    svm_apic_init_signal_blocked    3948
> (svm->vmcb->control.intercept & (1ULL << INTERCEPT_INIT));
> 7 svm.h    set_intercept                    300 vmcb->control.intercept |=
> (1ULL << bit);
> 8 svm.h    clr_intercept                    309 vmcb->control.intercept &=
> ~(1ULL << bit);
> 9 svm.h    is_intercept tercept_ioio        316 return
> (svm->vmcb->control.intercept & (1ULL << bit)) != 0;
> a svm.h    nested_exit_on_smi               377 return
> (svm->nested.ctl.intercept & (1ULL << INTERCEPT_SMI));
> b svm.h    nested_exit_on_intr              382 return
> (svm->nested.ctl.intercept & (1ULL << INTERCEPT_INTR));
> c svm.h    nested_exit_on_nmi               387 return
> (svm->nested.ctl.intercept & (1ULL << INTERCEPT_NMI));
>
> I will have to test extensively if I go ahead with these changes.  What do
> you think?

I see a lot of open-coding of the nested version of is_intercept(),
which would be a good preparatory cleanup.  It also looks like it
might be useful to introduce __set_intercept() and __clr_intercept()
which do the same thing as set_intercept() and clr_intercept(),
without calling recalc_intercepts(), for use *in* recalc_intercepts.
This code needs a little love. While your original proposal is more
expedient, taking the time to fix up the existing mess will be more
beneficial in the long run.




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux