| Message ID | 20231208184628.2297994-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: fix supported_flags for aarch64 | expand |
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 >
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 >>
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.
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
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.
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. >
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
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(-)