| Message ID | 20240327054255.24626-3-manali.shukla@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a test case for KVM_X86_DISABLE_EXIT | expand |
On Wed, Mar 27, 2024 at 05:42:54AM +0000, Manali Shukla wrote: ... > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index 4a40b332115d..00e37c376cf3 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -879,7 +879,7 @@ static inline vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, > */ > struct kvm_vm *____vm_create(struct vm_shape shape); > struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, > - uint64_t nr_extra_pages); > + uint64_t nr_extra_pages, bool is_in_kernel_apic); Adding boolean flag parameters to functions, which will 99% of the time be called with the same value set for them, is not nice. ... > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index adc51b0712ca..9c2a9e216d80 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -354,7 +354,7 @@ static uint64_t vm_nr_pages_required(enum vm_guest_mode mode, > } > > struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, > - uint64_t nr_extra_pages) > + uint64_t nr_extra_pages, bool is_in_kernel_apic) > { > uint64_t nr_pages = vm_nr_pages_required(shape.mode, nr_runnable_vcpus, > nr_extra_pages); > @@ -382,7 +382,12 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, > slot0 = memslot2region(vm, 0); > ucall_init(vm, slot0->region.guest_phys_addr + slot0->region.memory_size); > > - kvm_arch_vm_post_create(vm); > + if (is_in_kernel_apic) { > + kvm_arch_vm_post_create(vm); > + } else { > + sync_global_to_guest(vm, host_cpu_is_intel); > + sync_global_to_guest(vm, host_cpu_is_amd); > + } __vm_create() is shared with other architectures, and duplicating part of kvm_arch_vm_post_create() here is not a good approach, even if the framework was only for x86. I suggest: 1. Extend vm_shape to include a flags field and create a flag called NO_IRQCHIP 2. Add a flags member to kvm_vm and set it to the value of vm_shape.flags in ____vm_create() 3. Check !(vm.flags & NO_IRQCHIP) in x86's kvm_arch_vm_post_create() before calling vm_create_irqchip() Then, in your tests, you'll create your vm shape with the NO_IRQCHIP flag set. Thanks, drew
Hi Andrew, Thank you for reviewing my patches. On 3/27/2024 4:57 PM, Andrew Jones wrote: > On Wed, Mar 27, 2024 at 05:42:54AM +0000, Manali Shukla wrote: > ... >> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h >> index 4a40b332115d..00e37c376cf3 100644 >> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h >> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h >> @@ -879,7 +879,7 @@ static inline vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, >> */ >> struct kvm_vm *____vm_create(struct vm_shape shape); >> struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, >> - uint64_t nr_extra_pages); >> + uint64_t nr_extra_pages, bool is_in_kernel_apic); > > Adding boolean flag parameters to functions, which will 99% of the time be > called with the same value set for them, is not nice. Agreed. > > ... >> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c >> index adc51b0712ca..9c2a9e216d80 100644 >> --- a/tools/testing/selftests/kvm/lib/kvm_util.c >> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c >> @@ -354,7 +354,7 @@ static uint64_t vm_nr_pages_required(enum vm_guest_mode mode, >> } >> >> struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, >> - uint64_t nr_extra_pages) >> + uint64_t nr_extra_pages, bool is_in_kernel_apic) >> { >> uint64_t nr_pages = vm_nr_pages_required(shape.mode, nr_runnable_vcpus, >> nr_extra_pages); >> @@ -382,7 +382,12 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, >> slot0 = memslot2region(vm, 0); >> ucall_init(vm, slot0->region.guest_phys_addr + slot0->region.memory_size); >> >> - kvm_arch_vm_post_create(vm); >> + if (is_in_kernel_apic) { >> + kvm_arch_vm_post_create(vm); >> + } else { >> + sync_global_to_guest(vm, host_cpu_is_intel); >> + sync_global_to_guest(vm, host_cpu_is_amd); >> + } > > __vm_create() is shared with other architectures, and duplicating part of > kvm_arch_vm_post_create() here is not a good approach, even if the > framework was only for x86. > > I suggest: > > 1. Extend vm_shape to include a flags field and create a flag called > NO_IRQCHIP > > 2. Add a flags member to kvm_vm and set it to the value of vm_shape.flags > in ____vm_create() > > 3. Check !(vm.flags & NO_IRQCHIP) in x86's kvm_arch_vm_post_create() > before calling vm_create_irqchip() > > Then, in your tests, you'll create your vm shape with the NO_IRQCHIP flag > set. Sure. I will incorporate your suggestions in the next version. > > Thanks, > drew - Manali
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 6cbecf499767..667a83d67bfe 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -699,7 +699,7 @@ static struct kvm_vm *create_vm(enum vm_guest_mode mode, struct kvm_vcpu **vcpu, pr_info("Testing guest mode: %s\n", vm_guest_mode_string(mode)); - vm = __vm_create(VM_SHAPE(mode), 1, extra_mem_pages); + vm = __vm_create(VM_SHAPE(mode), 1, extra_mem_pages, true); log_mode_create_vm_done(vm); *vcpu = vm_vcpu_add(vm, 0, guest_code); diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 4a40b332115d..00e37c376cf3 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -879,7 +879,7 @@ static inline vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, */ struct kvm_vm *____vm_create(struct vm_shape shape); struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, - uint64_t nr_extra_pages); + uint64_t nr_extra_pages, bool is_in_kernel_apic); static inline struct kvm_vm *vm_create_barebones(void) { @@ -900,7 +900,7 @@ static inline struct kvm_vm *vm_create_barebones_protected_vm(void) static inline struct kvm_vm *vm_create(uint32_t nr_runnable_vcpus) { - return __vm_create(VM_SHAPE_DEFAULT, nr_runnable_vcpus, 0); + return __vm_create(VM_SHAPE_DEFAULT, nr_runnable_vcpus, 0, true); } struct kvm_vm *__vm_create_with_vcpus(struct vm_shape shape, uint32_t nr_vcpus, diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index adc51b0712ca..9c2a9e216d80 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -354,7 +354,7 @@ static uint64_t vm_nr_pages_required(enum vm_guest_mode mode, } struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, - uint64_t nr_extra_pages) + uint64_t nr_extra_pages, bool is_in_kernel_apic) { uint64_t nr_pages = vm_nr_pages_required(shape.mode, nr_runnable_vcpus, nr_extra_pages); @@ -382,7 +382,12 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, slot0 = memslot2region(vm, 0); ucall_init(vm, slot0->region.guest_phys_addr + slot0->region.memory_size); - kvm_arch_vm_post_create(vm); + if (is_in_kernel_apic) { + kvm_arch_vm_post_create(vm); + } else { + sync_global_to_guest(vm, host_cpu_is_intel); + sync_global_to_guest(vm, host_cpu_is_amd); + } return vm; } @@ -415,7 +420,7 @@ struct kvm_vm *__vm_create_with_vcpus(struct vm_shape shape, uint32_t nr_vcpus, TEST_ASSERT(!nr_vcpus || vcpus, "Must provide vCPU array"); - vm = __vm_create(shape, nr_vcpus, extra_mem_pages); + vm = __vm_create(shape, nr_vcpus, extra_mem_pages, true); for (i = 0; i < nr_vcpus; ++i) vcpus[i] = vm_vcpu_add(vm, i, guest_code); diff --git a/tools/testing/selftests/kvm/x86_64/ucna_injection_test.c b/tools/testing/selftests/kvm/x86_64/ucna_injection_test.c index 0ed32ec903d0..095188562709 100644 --- a/tools/testing/selftests/kvm/x86_64/ucna_injection_test.c +++ b/tools/testing/selftests/kvm/x86_64/ucna_injection_test.c @@ -271,7 +271,7 @@ int main(int argc, char *argv[]) kvm_check_cap(KVM_CAP_MCE); - vm = __vm_create(VM_SHAPE_DEFAULT, 3, 0); + vm = __vm_create(VM_SHAPE_DEFAULT, 3, 0, true); kvm_ioctl(vm->kvm_fd, KVM_X86_GET_MCE_CAP_SUPPORTED, &supported_mcg_caps);
Change __vm_create() to incorporate creation of a vm without in-kernel APIC. Currently, all the vms are created with in-kernel APIC support in KVM selftests because KVM_CREATE_IRQCHIP ioctl is called by default from kvm_arch_vm_post_create(). Add a flag in __vm_create() for a userspace to decide whether to start a vm with in-kernel APIC or without in-kernel APIC. It is a preparatory patch to create a vm without in-kernel APIC support for the KVM_X86_DISABLE_EXITS_HLT test. Signed-off-by: Manali Shukla <manali.shukla@amd.com> --- tools/testing/selftests/kvm/dirty_log_test.c | 2 +- tools/testing/selftests/kvm/include/kvm_util_base.h | 4 ++-- tools/testing/selftests/kvm/lib/kvm_util.c | 11 ++++++++--- .../selftests/kvm/x86_64/ucna_injection_test.c | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-)