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

[Paolo Bonzini]

On Wed, Dec 13, 2023 at 7:15 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx
> > > >
> > > > 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.
>