Re: [PATCH 4/5] KVM: selftests: Reserve memslot 1 for the ucall MMIO address

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: selftests: memslot 1 collision in vgic_v5_lpi_ist_vm_create()
--

commit 9df9f60479f8451e3b508d09149c3249f3de5c98
Author: Tharit Tangkijwanichakul <tharitt97@gmail.com>

KVM: selftests: Reserve memslot 1 for the ucall MMIO address

This commit reserves memslot 1 for a read-only ucall MMIO region in
__vm_create() when KVM_CAP_READONLY_MEM is supported, and bumps
TEST_MEM_SLOT_BASE from 1 to 2.

> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 9ddc047d5c275..f387100b1a19a 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -20,6 +20,7 @@
>  #include <linux/kernel.h>
>  
>  #define KVM_UTIL_MIN_PFN	2
> +#define KVM_UCALL_MEMSLOT	1
>  
>  u32 kvm_random_seed;
>  struct kvm_random_state kvm_rng;
> @@ -492,6 +493,7 @@ struct kvm_vm *__vm_create(struct vm_shape shape, u32 nr_runnable_vcpus,
>  	struct userspace_mem_region *slot0;
>  	struct kvm_vm *vm;
>  	int i, flags;
> +	gpa_t mmio_gpa;
>  
>  	kvm_set_files_rlimit(nr_runnable_vcpus);
>  
> @@ -514,14 +516,19 @@ struct kvm_vm *__vm_create(struct vm_shape shape, u32 nr_runnable_vcpus,
>  
>  	kvm_vm_elf_load(vm, program_invocation_name);
>  
> -	/*
> -	 * TODO: Add proper defines to protect the library's memslots, and then
> -	 * carve out memslot1 for the ucall MMIO address.  KVM treats writes to
> -	 * read-only memslots as MMIO, and creating a read-only memslot for the
> -	 * MMIO region would prevent silently clobbering the MMIO region.
> +	/* KVM treats writes to read-only memslots as MMIO, and creating
> +	 * a read-only memslot for the MMIO region would prevent silently
> +	 * clobbering the MMIO region.
>  	 */
>  	slot0 = memslot2region(vm, 0);
> -	ucall_init(vm, slot0->region.guest_phys_addr + slot0->region.memory_size);
> +	mmio_gpa = slot0->region.guest_phys_addr + slot0->region.memory_size;
> +	ucall_init(vm, mmio_gpa);
> +
> +	if (vm_check_cap(vm, KVM_CAP_READONLY_MEM))
> +		vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
> +					    mmio_gpa, KVM_UCALL_MEMSLOT,
> +					    vm_adjust_num_guest_pages(vm->mode, 1),
> +					    KVM_MEM_READONLY);

[Severity: Medium]
Does reserving KVM_UCALL_MEMSLOT (slot 1) in __vm_create() collide with
VGIC_V5_LPI_MEMSLOT in tools/testing/selftests/kvm/arm64/vgic_v5.c?

In tools/testing/selftests/kvm/arm64/vgic_v5.c, VGIC_V5_LPI_MEMSLOT is
hardcoded to 1 instead of using TEST_MEM_SLOT_BASE:

    #define VGIC_V5_LPI_MEMSLOT	1

When test_vgic_v5_lpis() or test_vgic_v5_ist_save_restore() runs,
vgic_v5_lpi_ist_vm_create() creates the VM via __vm_create() (which
populates slot 1 because arm64 unconditionally supports
KVM_CAP_READONLY_MEM) and then attempts to add a memslot at
VGIC_V5_LPI_MEMSLOT:

    v->vm = __vm_create(VM_SHAPE_DEFAULT, VGIC_V5_LPI_NR_VCPUS, 0);
    ...
    vm_userspace_mem_region_add(v->vm, VM_MEM_SRC_ANONYMOUS,
                                VGIC_V5_LPI_IST_BASE_GPA,
                                VGIC_V5_LPI_MEMSLOT,
                                nr_lpi_ist_pages, 0);

Inside vm_userspace_mem_region_add() -> vm_mem_add(), the duplicate slot
check in vm->regions.slot_hash finds KVM_UCALL_MEMSLOT in slot 1 and
aborts the test with TEST_FAIL("A mem region with the requested slot
already exists...").

Should VGIC_V5_LPI_MEMSLOT be defined in terms of TEST_MEM_SLOT_BASE as
well?

>  
>  	if (kvm_random_seed != last_kvm_seed)
>  		kvm_seed_rng(kvm_random_seed);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017110213.46199-1-tharitt97@gmail.com?part=4