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
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,
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
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!
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;
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!
linux-kselftest-mirror@lists.linaro.org
-
Ackerley Tng -
David Matlack -
Sean Christopherson