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

[Manali Shukla <manali.shukla@amd.com>]

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