[PATCH 0/2] Fix kvm_setup_gdt and reuse in vcpu_init_descriptor_tables

[PATCH 0/2] Fix kvm_setup_gdt and reuse in vcpu_init_descriptor_tables

[Ackerley Tng]

Remove discrepancy between kvm_setup_gdt and vcpu_init_descriptor_tables
by fixing initialization of limit in kvm_setup_gdt and reusing
kvm_setup_gdt in vcpu_init_descriptor_tables.

Ackerley Tng (2):
  KVM: selftests: Fix initialization of GDT limit
  KVM: selftests: Reuse kvm_setup_gdt in vcpu_init_descriptor_tables

 tools/testing/selftests/kvm/lib/x86_64/processor.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

--
2.39.0.314.g84b9a713c41-goog

[PATCH 1/2] KVM: selftests: Fix initialization of GDT limit

[Ackerley Tng]

Subtract 1 to initialize GDT limit according to spec.

Signed-off-by: Ackerley Tng <ackerleytng@google.com>
---
 tools/testing/selftests/kvm/lib/x86_64/processor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index acfa1d01e7df..33ca7f5232a4 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -502,7 +502,12 @@ static void kvm_setup_gdt(struct kvm_vm *vm, struct kvm_dtable *dt)
 		vm->gdt = __vm_vaddr_alloc_page(vm, MEM_REGION_DATA);
 
 	dt->base = vm->gdt;
-	dt->limit = getpagesize();
+
+	/*
+	 * Intel SDM Volume 3, 3.5.1: "the GDT limit should always be one less
+	 * than an integral multiple of eight"
+	 */
+	dt->limit = getpagesize() - 1;
 }
 
 static void kvm_setup_tss_64bit(struct kvm_vm *vm, struct kvm_segment *segp,
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH 1/2] KVM: selftests: Fix initialization of GDT limit

[Sean Christopherson]

On Sat, Jan 14, 2023, Ackerley Tng wrote:
> Subtract 1 to initialize GDT limit according to spec.

Nit, make changelogs standalone, i.e. don't make me read the code just to
understand the changelog.  "Subtract 1" is meaningless without seeing the
existing code.  The changelog doesn't need to be a play-by-play, e.g. describing
the change as "inclusive vs. exclusive" is also fine, the important thing is that
readers can gain a basic understanding of the change without needing to read code.

> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> ---
>  tools/testing/selftests/kvm/lib/x86_64/processor.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index acfa1d01e7df..33ca7f5232a4 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -502,7 +502,12 @@ static void kvm_setup_gdt(struct kvm_vm *vm, struct kvm_dtable *dt)
>  		vm->gdt = __vm_vaddr_alloc_page(vm, MEM_REGION_DATA);
>  
>  	dt->base = vm->gdt;
> -	dt->limit = getpagesize();
> +
> +	/*
> +	 * Intel SDM Volume 3, 3.5.1:

As a general rule, especially in code comments, never reference manual sections
by their index/numbers, there's a 99% chance the comment will be stale within a
few years.

Quoting manuals is ok, because if the quote because stale then that in and of
itself is an interesting datapoint.  If referencing a specific section is the
easiest way to convey something, then use then name of the section, as that's less
likely to be arbitrarily change.

In this case, I'd just omit the comment entirely.  We have to assume readers have
a minimum level of knowledge, and IMO this is firmly below (above?) the threshold.

> "the GDT limit should always be one less
> +	 * than an integral multiple of eight"
> +	 */
> +	dt->limit = getpagesize() - 1;
>  }
>  
>  static void kvm_setup_tss_64bit(struct kvm_vm *vm, struct kvm_segment *segp,
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

Re: [PATCH 1/2] KVM: selftests: Fix initialization of GDT limit

[Sean Christopherson]

On Wed, Jan 18, 2023, Sean Christopherson wrote:
> On Sat, Jan 14, 2023, Ackerley Tng wrote:
> > Subtract 1 to initialize GDT limit according to spec.
> 
> Nit, make changelogs standalone, i.e. don't make me read the code just to
> understand the changelog.  "Subtract 1" is meaningless without seeing the
> existing code.  The changelog doesn't need to be a play-by-play, e.g. describing
> the change as "inclusive vs. exclusive" is also fine, the important thing is that
> readers can gain a basic understanding of the change without needing to read code.
> 
> > Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> > ---
> >  tools/testing/selftests/kvm/lib/x86_64/processor.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index acfa1d01e7df..33ca7f5232a4 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -502,7 +502,12 @@ static void kvm_setup_gdt(struct kvm_vm *vm, struct kvm_dtable *dt)
> >  		vm->gdt = __vm_vaddr_alloc_page(vm, MEM_REGION_DATA);
> >  
> >  	dt->base = vm->gdt;
> > -	dt->limit = getpagesize();
> > +
> > +	/*
> > +	 * Intel SDM Volume 3, 3.5.1:
> 
> As a general rule, especially in code comments, never reference manual sections
> by their index/numbers, there's a 99% chance the comment will be stale within a
> few years.
> 
> Quoting manuals is ok, because if the quote because stale then that in and of
> itself is an interesting datapoint.  If referencing a specific section is the
> easiest way to convey something, then use then name of the section, as that's less
> likely to be arbitrarily change.
> 
> In this case, I'd just omit the comment entirely.  We have to assume readers have
> a minimum level of knowledge, and IMO this is firmly below (above?) the threshold.

No need to post a v2, I'll fold this patch into a larger cleanup of the descriptor
table code.

Thanks!

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

[Ackerley Tng]

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);
 	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;
-- 
2.39.0.314.g84b9a713c41-goog

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

[Sean Christopherson]

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 :-/

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

[David Matlack]

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.

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

[Sean Christopherson]

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!