[PATCH] KVM: selftests: fix supported_flags for aarch64

[PATCH] KVM: selftests: fix supported_flags for aarch64

[Paolo Bonzini]

KVM/Arm supports readonly memslots; fix the calculation of
supported_flags in set_memory_region_test.c, otherwise the
test fails.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tools/testing/selftests/kvm/set_memory_region_test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index 6637a0845acf..dfd1d1e22da3 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -333,9 +333,11 @@ static void test_invalid_memory_region_flags(void)
 	struct kvm_vm *vm;
 	int r, i;
 
-#ifdef __x86_64__
+#if defined __aarch64__ || defined __x86_64__
 	supported_flags |= KVM_MEM_READONLY;
+#endif
 
+#ifdef __x86_64__
 	if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM))
 		vm = vm_create_barebones_protected_vm();
 	else
-- 
2.39.1

Re: [PATCH] KVM: selftests: fix supported_flags for aarch64

[Sean Christopherson]

On Fri, Dec 08, 2023, Paolo Bonzini wrote:
> KVM/Arm supports readonly memslots; fix the calculation of
> supported_flags in set_memory_region_test.c, otherwise the
> test fails.

You got beat by a few hours, and by a better solution ;-)

https://lore.kernel.org/all/20231208033505.2930064-1-shahuang@redhat.com

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tools/testing/selftests/kvm/set_memory_region_test.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index 6637a0845acf..dfd1d1e22da3 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -333,9 +333,11 @@ static void test_invalid_memory_region_flags(void)
>  	struct kvm_vm *vm;
>  	int r, i;
>  
> -#ifdef __x86_64__
> +#if defined __aarch64__ || defined __x86_64__
>  	supported_flags |= KVM_MEM_READONLY;
> +#endif
>  
> +#ifdef __x86_64__
>  	if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM))
>  		vm = vm_create_barebones_protected_vm();
>  	else
> -- 
> 2.39.1
> 

Re: [PATCH] KVM: selftests: fix supported_flags for aarch64

[Paolo Bonzini]

On 12/9/23 03:29, Sean Christopherson wrote:
> On Fri, Dec 08, 2023, Paolo Bonzini wrote:
>> KVM/Arm supports readonly memslots; fix the calculation of
>> supported_flags in set_memory_region_test.c, otherwise the
>> test fails.
> 
> You got beat by a few hours, and by a better solution ;-)
> 
> https://lore.kernel.org/all/20231208033505.2930064-1-shahuang@redhat.com

Better but also wrong---and my patch has the debatable merit of more
clearly exposing the wrongness.  Testing individual architectures is bad,
but testing __KVM_HAVE_READONLY_MEM makes the test fail when running a new
test on an old kernel.

This scenario of course will fail when the test detects a bug, but readonly
memory is just new functionality (think of the case where RISC-V starts
defining __KVM_HAVE_READONLY_MEM in the future).  For new functionality,
the right thing to do is one of 1) skip the whole test 2) skip the individual
test case 3) code the test to adapt to the old kernel.  The third choice is
rarely possible, but this is one of the cases in which it _is_ possible.

So, the only good way to do this is to get _all_ supported_flags from
KVM_CHECK_EXTENSION(KVM_CAP_USER_MEMORY2).  We can change the value returned
by KVM_CHECK_EXTENSION because KVM_CAP_USER_MEMORY2 has not been included in
any released kernel.  Calling KVM_CHECK_EXTENSION subsumes

         supported_flags |= KVM_MEM_READONLY;
         if (kvm_check_cap(KVM_CAP_MEMORY_ATTRIBUTES) & KVM_MEMORY_ATTRIBUTE_PRIVATE)
                 supported_flags |= KVM_MEM_GUEST_MEMFD;

and v2_only_flags would be defined as

         const uint32_t v2_only_flags = ~(KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY);

(not guaranteed to work in the future, but good enough since new KVM_MEM_*
flags are a very rare occurrence).  Then, the test checks that the supported
flags are consistent with the value returned by KVM_CHECK_EXTENSION.

Shaoqin, would you give it a shot?

Thanks,

Paolo

> 

>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   tools/testing/selftests/kvm/set_memory_region_test.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
>> index 6637a0845acf..dfd1d1e22da3 100644
>> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
>> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
>> @@ -333,9 +333,11 @@ static void test_invalid_memory_region_flags(void)
>>   	struct kvm_vm *vm;
>>   	int r, i;
>>   
>> -#ifdef __x86_64__
>> +#if defined __aarch64__ || defined __x86_64__
>>   	supported_flags |= KVM_MEM_READONLY;
>> +#endif
>>   
>> +#ifdef __x86_64__
>>   	if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM))
>>   		vm = vm_create_barebones_protected_vm();
>>   	else
>> -- 
>> 2.39.1
>>

Re: [PATCH] KVM: selftests: fix supported_flags for aarch64

[Sean Christopherson]

On Tue, Dec 12, 2023, Paolo Bonzini wrote:
> On 12/9/23 03:29, Sean Christopherson wrote:
> > On Fri, Dec 08, 2023, Paolo Bonzini wrote:
> > > KVM/Arm supports readonly memslots; fix the calculation of
> > > supported_flags in set_memory_region_test.c, otherwise the
> > > test fails.
> > 
> > You got beat by a few hours, and by a better solution ;-)
> > 
> > https://lore.kernel.org/all/20231208033505.2930064-1-shahuang@redhat.com
> 
> Better but also wrong---and my patch has the debatable merit of more
> clearly exposing the wrongness.  Testing individual architectures is bad,
> but testing __KVM_HAVE_READONLY_MEM makes the test fail when running a new
> test on an old kernel.

But we already crossed that bridge and burned it for good measure by switching
to KVM_SET_USER_MEMORY_REGION2, i.e. as of commit

  8d99e347c097 ("KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2")

selftests built against a new kernel can't run on an old kernel.  Building KVM
selftests requires kernel headers, so while not having a hard requirement that
the uapi headers are fresh would be nice, I don't think it buys all that much.

If we wanted to assert that x86, arm64, etc. enumerate __KVM_HAVE_READONLY_MEM,
i.e. ensure that read-only memory is supported as expected, then that can be done
as a completely unrelated test.

IMO, one of the big selling points of selftests over KUT is that we can punt on
supporting old kernels since selftests are in-tree.  I don't think it's at all
unreasonable to require that selftests be built against the target kernel, and
by doing so we can signficantly reduce the maintenance burden.  The kernel needs
to be backwards compatibile, but I don't see why selftests need to be backwards
compatible.

Re: [PATCH] KVM: selftests: fix supported_flags for aarch64

[Paolo Bonzini]

On 12/13/23 18:21, Sean Christopherson wrote:
> On Tue, Dec 12, 2023, Paolo Bonzini wrote:
>> On 12/9/23 03:29, Sean Christopherson wrote:
>>> On Fri, Dec 08, 2023, Paolo Bonzini wrote:
>>>> KVM/Arm supports readonly memslots; fix the calculation of
>>>> supported_flags in set_memory_region_test.c, otherwise the
>>>> test fails.
>>>
>>> You got beat by a few hours, and by a better solution ;-)
>>>
>>> https://lore.kernel.org/all/20231208033505.2930064-1-shahuang@redhat.com
>>
>> Better but also wrong---and my patch has the debatable merit of more
>> clearly exposing the wrongness.  Testing individual architectures is bad,
>> but testing __KVM_HAVE_READONLY_MEM makes the test fail when running a new
>> test on an old kernel.
> 
> But we already crossed that bridge and burned it for good measure by switching
> to KVM_SET_USER_MEMORY_REGION2, i.e. as of commit
> 
>    8d99e347c097 ("KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2")
> 
> selftests built against a new kernel can't run on an old kernel.  Building KVM
> selftests requires kernel headers, so while not having a hard requirement that
> the uapi headers are fresh would be nice, I don't think it buys all that much.
> 
> If we wanted to assert that x86, arm64, etc. enumerate __KVM_HAVE_READONLY_MEM,
> i.e. ensure that read-only memory is supported as expected, then that can be done
> as a completely unrelated test.

selftests have the luxury of having sync-ed kernel headers, but in 
general userspace won't, and that means __KVM_HAVE_READONLY_MEM would be 
a very poor userspace API.  Fortunately it has "__" so it is not 
userspace API at all, and I don't want selftests to treat it as one.

> IMO, one of the big selling points of selftests over KUT is that we can punt on
> supporting old kernels since selftests are in-tree.  I don't think it's at all
> unreasonable to require that selftests be built against the target kernel, and
> by doing so we can signficantly reduce the maintenance burden.  The kernel needs
> to be backwards compatibile, but I don't see why selftests need to be backwards
> compatible.

It does help sometimes to be able to run old tests on new kernel or vice 
versa.  So even without making that a requirement, it is a nice thing to 
have whenever possible.

Paolo

Re: [PATCH] KVM: selftests: fix supported_flags for aarch64

[Sean Christopherson]

On Wed, Dec 13, 2023, Paolo Bonzini wrote:
> On 12/13/23 18:21, Sean Christopherson wrote:
> > On Tue, Dec 12, 2023, Paolo Bonzini wrote:
> > > On 12/9/23 03:29, Sean Christopherson wrote:
> > > > On Fri, Dec 08, 2023, Paolo Bonzini wrote:
> > > > > KVM/Arm supports readonly memslots; fix the calculation of
> > > > > supported_flags in set_memory_region_test.c, otherwise the
> > > > > test fails.
> > > > 
> > > > You got beat by a few hours, and by a better solution ;-)
> > > > 
> > > > https://lore.kernel.org/all/20231208033505.2930064-1-shahuang@redhat.com
> > > 
> > > Better but also wrong---and my patch has the debatable merit of more
> > > clearly exposing the wrongness.  Testing individual architectures is bad,
> > > but testing __KVM_HAVE_READONLY_MEM makes the test fail when running a new
> > > test on an old kernel.
> > 
> > But we already crossed that bridge and burned it for good measure by switching
> > to KVM_SET_USER_MEMORY_REGION2, i.e. as of commit
> > 
> >    8d99e347c097 ("KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2")
> > 
> > selftests built against a new kernel can't run on an old kernel.  Building KVM
> > selftests requires kernel headers, so while not having a hard requirement that
> > the uapi headers are fresh would be nice, I don't think it buys all that much.
> > 
> > If we wanted to assert that x86, arm64, etc. enumerate __KVM_HAVE_READONLY_MEM,
> > i.e. ensure that read-only memory is supported as expected, then that can be done
> > as a completely unrelated test.
> 
> selftests have the luxury of having sync-ed kernel headers, but in general
> userspace won't, and that means __KVM_HAVE_READONLY_MEM would be a very poor
> userspace API.  Fortunately it has "__" so it is not userspace API at all,
> and I don't want selftests to treat it as one.

Wait, what?  How does double underscores exempt it from being uAPI?  AIUI, the C
standard effectively ensures that userspace won't define/declare symbols with
double underscores, i.e. ensures there won't be conflicts.  But pretty much all
of the kernel-defined types are prefixed with "__", e.g. __u8 and friends, so I
don't see how prefixing with "__" exempts something from becoming uAPI.

I completely agree that __KVM_HAVE_READONLY_MEM shouldn't be uAPI, but then it
really, really shouldn't be defined in arch/x86/include/uapi/asm/kvm.h.

Re: [PATCH] KVM: selftests: fix supported_flags for aarch64

[Paolo Bonzini]

On Wed, Dec 13, 2023 at 7:15 PM Sean Christopherson <seanjc@google.com> wrote:
> > selftests have the luxury of having sync-ed kernel headers, but in general
> > userspace won't, and that means __KVM_HAVE_READONLY_MEM would be a very poor
> > userspace API.  Fortunately it has "__" so it is not userspace API at all,
> > and I don't want selftests to treat it as one.
>
> Wait, what?  How does double underscores exempt it from being uAPI?  AIUI, the C
> standard effectively ensures that userspace won't define/declare symbols with
> double underscores, i.e. ensures there won't be conflicts.  But pretty much all
> of the kernel-defined types are prefixed with "__", e.g. __u8 and friends, so I
> don't see how prefixing with "__" exempts something from becoming uAPI.

Userspace is generally not supposed to know that double underscore
symbols exist, though in some cases it has to (see for example
_UFFDIO_*). Looking at yesterday's patch from Dionna, userspace is
very much not supposed to use _BITUL, and even less so for _UL.

In particular, __KVM_HAVE_* symbols are meant to mask definitions in
include/uapi/linux/kvm.h.
__KVM_HAVE_READONLY_MEM was a very misguided mean to define
KVM_CAP_READONLY_MEM only on architectures where it could have
possibly be true (see commit 0f8a4de3e088, "KVM: Unconditionally
export KVM_CAP_READONLY_MEM", 2014-08-29). Which does not make sense
at all, as the commit message points out. So I'm willing to test my
chances, kill it and see if anyone complains (while crossing fingers
that Google and Amazon don't :)).

Paolo

> I completely agree that __KVM_HAVE_READONLY_MEM shouldn't be uAPI, but then it
> really, really shouldn't be defined in arch/x86/include/uapi/asm/kvm.h.
>

On Wed, Dec 13, 2023 at 7:15 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Dec 13, 2023, Paolo Bonzini wrote:
> > On 12/13/23 18:21, Sean Christopherson wrote:
> > > On Tue, Dec 12, 2023, Paolo Bonzini wrote:
> > > > On 12/9/23 03:29, Sean Christopherson wrote:
> > > > > On Fri, Dec 08, 2023, Paolo Bonzini wrote:
> > > > > > KVM/Arm supports readonly memslots; fix the calculation of
> > > > > > supported_flags in set_memory_region_test.c, otherwise the
> > > > > > test fails.
> > > > >
> > > > > You got beat by a few hours, and by a better solution ;-)
> > > > >
> > > > > https://lore.kernel.org/all/20231208033505.2930064-1-shahuang@redhat.com
> > > >
> > > > Better but also wrong---and my patch has the debatable merit of more
> > > > clearly exposing the wrongness.  Testing individual architectures is bad,
> > > > but testing __KVM_HAVE_READONLY_MEM makes the test fail when running a new
> > > > test on an old kernel.
> > >
> > > But we already crossed that bridge and burned it for good measure by switching
> > > to KVM_SET_USER_MEMORY_REGION2, i.e. as of commit
> > >
> > >    8d99e347c097 ("KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2")
> > >
> > > selftests built against a new kernel can't run on an old kernel.  Building KVM
> > > selftests requires kernel headers, so while not having a hard requirement that
> > > the uapi headers are fresh would be nice, I don't think it buys all that much.
> > >
> > > If we wanted to assert that x86, arm64, etc. enumerate __KVM_HAVE_READONLY_MEM,
> > > i.e. ensure that read-only memory is supported as expected, then that can be done
> > > as a completely unrelated test.
> >
> > selftests have the luxury of having sync-ed kernel headers, but in general
> > userspace won't, and that means __KVM_HAVE_READONLY_MEM would be a very poor
> > userspace API.  Fortunately it has "__" so it is not userspace API at all,
> > and I don't want selftests to treat it as one.
>
> Wait, what?  How does double underscores exempt it from being uAPI?  AIUI, the C
> standard effectively ensures that userspace won't define/declare symbols with
> double underscores, i.e. ensures there won't be conflicts.  But pretty much all
> of the kernel-defined types are prefixed with "__", e.g. __u8 and friends, so I
> don't see how prefixing with "__" exempts something from becoming uAPI.
>
> I completely agree that __KVM_HAVE_READONLY_MEM shouldn't be uAPI, but then it
> really, really shouldn't be defined in arch/x86/include/uapi/asm/kvm.h.
>