Re: [PATCH 2/2] KVM: selftests: Reuse kvm_setup_gdt in vcpu_init_descriptor_tables

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

 



On Wed, Jan 18, 2023 at 11:01 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Sat, Jan 14, 2023, Ackerley Tng wrote:
> > Refactor vcpu_init_descriptor_tables to use kvm_setup_gdt
> >
> > Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> > ---
> >  tools/testing/selftests/kvm/lib/x86_64/processor.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index 33ca7f5232a4..8d544e9237aa 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -1119,8 +1119,7 @@ void vcpu_init_descriptor_tables(struct kvm_vcpu *vcpu)
> >       vcpu_sregs_get(vcpu, &sregs);
> >       sregs.idt.base = vm->idt;
> >       sregs.idt.limit = NUM_INTERRUPTS * sizeof(struct idt_entry) - 1;
> > -     sregs.gdt.base = vm->gdt;
> > -     sregs.gdt.limit = getpagesize() - 1;
> > +     kvm_setup_gdt(vcpu->vm, &sregs.gdt);
>
> *sigh*
>
> The selftests infrastructure is so misguided.  Forcing tests to opt-in to
> installing an IDT just to avoid allocating two pages is such an awful tradeoff.
>
> Now that we have kvm_arch_vm_post_create(), I think we should always allocate
> the GDT, IDT, and handlers, and then vCPU setup/creation can simply grab the
> already-allocated values and stuff them into KVM.  That would then eliminate
> kvm_setup_gdt() entirely.

+1

I actually started going through the process of making the same clean
up last year, but never got around to posting it. My motivation at the
time was to provide better debuggability for test failures that were
due to unexpected exceptions in guest mode.

One thing to be aware of: vmx_pmu_caps_test and platform_info_test
both relied on *not* having an IDT installed (as of Sep 2022).
Specifically they relied on exception generating KVM_EXIT_SHUTDOWN.
Installing an IDT causes these tests instead to hit the unhandled
exception TEST_ASSERT(). Just a heads up when you do this clean up :)

>
> And much of the setup code is also backwards and unnecessarily thread-unsafe, e.g.
> vCPU initialization shouldn't need to fill GDT entries.
>
> So, while I agree that using kvm_setup_gdt() is a good change on its own, I'd
> rather go the more aggressive route and clean up the underlying mess.
>
> I'll send patches sometime this week, unfortunately typing up what I have in mind
> is harder than just reworking the code :-/

I would offer to post my patches but it doesn't cover any of the GDT
stuff so I'll let you post yours.



[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux