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

[Andrew Jones]

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