Re: [PATCH 2/2] KVM: selftests: Test guest_memfd binding overlap without GPA overlap

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

>
> [...snip...]
>
>>
>> Since this test program is meant to test set_memory_region, should we be
>> retaining the original test? The original test is wrong in that it
>> doesn't test guest_memfd's binding, but it does test that
>> set_memory_region returns -EEXIST on overlapping GPAs.
>>
>> Perhaps to just test overlapping GPAs we can use anonymous memory
>> instead of guest_memfd.
>
> Eh, I see no harm in having both.  E.g. if we do this, then we don't have to
> explain why we're not testing the other case :-)
>

Makes sense to have both :)

> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index 9b919a231c93..283392bcc3a0 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -510,7 +510,7 @@ static void test_add_overlapping_private_memory_regions(void)
>
>         vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM);
>
> -       memfd = vm_create_guest_memfd(vm, MEM_REGION_SIZE * 4, 0);
> +       memfd = vm_create_guest_memfd(vm, MEM_REGION_SIZE * 5, 0);
>
>         vm_set_user_memory_region2(vm, MEM_REGION_SLOT, KVM_MEM_GUEST_MEMFD,
>                                    MEM_REGION_GPA, MEM_REGION_SIZE * 2, 0, memfd, 0);
> @@ -542,6 +542,26 @@ static void test_add_overlapping_private_memory_regions(void)
>         TEST_ASSERT(r == -1 && errno == EEXIST, "%s",
>                     "Overlapping guest_memfd() bindings should fail with EEXIST");
>
> +       /*
> +        * Repeat the overlap tests, but so that there is overlap in the
> +        * guest_memfd bindings (i.e. in guest_memfd file offsets), but _not_
> +        * in the GPA space.  Regardless of where there's overlap, KVM should
> +        * return -EEXIST.
> +        */
> +       r = __vm_set_user_memory_region2(vm, MEM_REGION_SLOT, KVM_MEM_GUEST_MEMFD,
> +                                        MEM_REGION_GPA,
> +                                        MEM_REGION_SIZE * 2,
> +                                        0, memfd, MEM_REGION_SIZE);
> +       TEST_ASSERT(r == -1 && errno == EEXIST, "%s",
> +                   "Overlapping guest_memfd() bindings should fail with EEXIST");
> +
> +       /* And now the back half of the other slot's guest_memfd binding. */
> +       r = __vm_set_user_memory_region2(vm, MEM_REGION_SLOT, KVM_MEM_GUEST_MEMFD,
> +                                        MEM_REGION_GPA,
> +                                        MEM_REGION_SIZE * 2,
> +                                        0, memfd, MEM_REGION_SIZE * 3);
> +       TEST_ASSERT(r == -1 && errno == EEXIST, "%s",
> +                   "Overlapping guest_memfd() bindings should fail with EEXIST");

I just noticed this is kind of odd, what is the purpose of "%s" and then
filling the string in with a hardcoded string?

>         close(memfd);
>         kvm_vm_free(vm);
>  }