On Tue, Dec 20, 2022, David Woodhouse wrote:
> On Fri, 2022-12-16 at 16:34 +0000, Sean Christopherson wrote:
> > On Fri, Dec 16, 2022, Yu Zhang wrote:
> > > Currently, KVM xen and its shared info selftest code uses
> > > 'GPA_INVALID' for GFN values, but actually it is more accurate
> > > to use the name 'INVALID_GFN'. So just add a new definition
> > > and use it.
> > >
> > > No functional changes intended.
> > >
> > > Suggested-by: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> > > Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
> > > ---
> > > arch/x86/kvm/xen.c | 4 ++--
> > > include/linux/kvm_types.h | 1 +
> > > tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 4 ++--
> > > 3 files changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > > index d7af40240248..6908a74ab303 100644
> > > --- a/arch/x86/kvm/xen.c
> > > +++ b/arch/x86/kvm/xen.c
> > > @@ -41,7 +41,7 @@ static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn)
> > > int ret = 0;
> > > int idx = srcu_read_lock(&kvm->srcu);
> > >
> > > - if (gfn == GPA_INVALID) {
> > > + if (gfn == INVALID_GFN) {
> >
> > Grrr! This magic value is ABI, as "gfn == -1" yields different behavior than a
> > random, garbage gfn.
> >
> > So, sadly, we can't simply introduce INVALID_GFN here, and instead need to do
> > something like:
> >
>
> Well... you can still use INVALID_GFN as long as its value remains the
> same ((uint64_t)-1).
>
> But yes, the KVM API differs here from Xen because Xen only allows a
> guest to *set* these (and later they invented SHUTDOWN_soft_reset).
> While KVM lets the userspace VMM handle soft reset, and needs to allow
> them to be *unset*. And since zero is a valid GPA/GFN, -1 is a
> reasonable value for 'INVALID'.
Oh, yeah, I'm not arguing against using '-1', just calling out that there is
special meaning given to '-1' and so it needs to be formalized so that KVM doesn't
accidentally break userspace.
> But I do think that detail escaped the documentation and the uapi headers, so
> your suggestion below is a good one, although strictly we need a GPA one too.
Ah, right, for struct kvm_xen_vcpu_attr.
