| Message ID | 20230114161557.499685-3-ackerleytng@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix kvm_setup_gdt and reuse in vcpu_init_descriptor_tables | expand |
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 :-/
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.
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 --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;
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(-)