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
