diff mbox series

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

Message ID 20230114161557.499685-3-ackerleytng@google.com (mailing list archive)
State New, archived
Headers show
Series Fix kvm_setup_gdt and reuse in vcpu_init_descriptor_tables | expand

Commit Message

Ackerley Tng Jan. 14, 2023, 4:15 p.m. UTC
Refactor vcpu_init_descriptor_tables to use kvm_setup_gdt

Signed-off-by: Ackerley Tng <ackerleytng@google.com>
---
 tools/testing/selftests/kvm/lib/x86_64/processor.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Sean Christopherson Jan. 18, 2023, 7:01 p.m. UTC | #1
On Sat, Jan 14, 2023, Ackerley Tng wrote:
> Refactor vcpu_init_descriptor_tables to use kvm_setup_gdt
> 
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> ---
>  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.

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 :-/
David Matlack Jan. 18, 2023, 7:15 p.m. UTC | #2
On Wed, Jan 18, 2023 at 11:01 AM Sean Christopherson <seanjc@google.com> 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@google.com>
> > ---
> >  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.
Sean Christopherson Jan. 18, 2023, 7:36 p.m. UTC | #3
On Wed, Jan 18, 2023, David Matlack wrote:
> 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().

LOL, I'm just _shocked_ that we would have such code.

> Just a heads up when you do this clean up :)

Thanks, definitely saved me some time!
diff mbox series

Patch

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);
 	kvm_seg_set_kernel_data_64bit(NULL, DEFAULT_DATA_SELECTOR, &sregs.gs);
 	vcpu_sregs_set(vcpu, &sregs);
 	*(vm_vaddr_t *)addr_gva2hva(vm, (vm_vaddr_t)(&exception_handlers)) = vm->handlers;