Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
References: <20231208184628.2297994-1-pbonzini@redhat.com> <ZXPRGzgWFqFdI_ep@google.com>
 <184e253d-06c4-419e-b2b4-7cce1f875ba5@redhat.com> <ZXnoCadq2J3cPz2r@google.com>
 <84ad3082-794b-443f-874a-d304934a395b@redhat.com> <ZXn0sR6IyzLzVHW-@google.com>
In-Reply-To: <ZXn0sR6IyzLzVHW-@google.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed, 13 Dec 2023 19:44:42 +0100
Message-ID: <CABgObfa9y3dwLjduS8UmGDRxLBfFVJ9T8GhHx=22arM+U6kBzw@mail.gmail.com>
Subject: Re: [PATCH] KVM: selftests: fix supported_flags for aarch64
To:     Sean Christopherson <seanjc@google.com>
Cc:     Shaoqin Huang <shahuang@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Dec 13, 2023 at 7:15=E2=80=AFPM Sean Christopherson <seanjc@google.=
com> wrote:
> > selftests have the luxury of having sync-ed kernel headers, but in gene=
ral
> > 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 a=
ll,
> > 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 mu=
ch 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 th=
en it
> really, really shouldn't be defined in arch/x86/include/uapi/asm/kvm.h.
>

On Wed, Dec 13, 2023 at 7:15=E2=80=AFPM 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@red=
hat.com
> > > >
> > > > Better but also wrong---and my patch has the debatable merit of mor=
e
> > > > clearly exposing the wrongness.  Testing individual architectures i=
s bad,
> > > > but testing __KVM_HAVE_READONLY_MEM makes the test fail when runnin=
g 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_SE=
T_USER_MEMORY_REGION2")
> > >
> > > selftests built against a new kernel can't run on an old kernel.  Bui=
lding KVM
> > > selftests requires kernel headers, so while not having a hard require=
ment that
> > > the uapi headers are fresh would be nice, I don't think it buys all t=
hat much.
> > >
> > > If we wanted to assert that x86, arm64, etc. enumerate __KVM_HAVE_REA=
DONLY_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 gene=
ral
> > 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 a=
ll,
> > 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 mu=
ch 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 th=
en it
> really, really shouldn't be defined in arch/x86/include/uapi/asm/kvm.h.
>