diff mbox series

[v1,2/3] KVM: selftests: Change __vm_create() to create a vm without in-kernel APIC

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

Commit Message

Manali Shukla March 27, 2024, 5:42 a.m. UTC
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(-)

Comments

Andrew Jones March 27, 2024, 11:27 a.m. UTC | #1
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
Manali Shukla March 28, 2024, 11:12 a.m. UTC | #2
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 mbox series

Patch

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);