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&data=02%7C01%7 > > > > > Cbabu.moger%40amd.com%7C4cedcb3567194883601e08d8124b6be7%7C3dd8 > > > > > 961fe4884e608e11a82d994e183d%7C0%7C0%7C637279463210520563&s > > > > > data=oRQq0hj0O43A4lnl8JEb%2BHt8oCFHWxcqvLaA1%2BacTJc%3D&reser > > > ved=0 > > > > Link: > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla. > > > > > kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.m > > > > > oger%40amd.com%7C4cedcb3567194883601e08d8124b6be7%7C3dd8961fe48 > > > > > 84e608e11a82d994e183d%7C0%7C0%7C637279463210520563&sdata=EtA > > > rCUBB8etloN%2B%2Blx42RZqai12QFvtJefnxBn1ryMQ%3D&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.
