Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Sean Christopherson <seanjc@google.com>]

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@infradead.org>
> > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> > > ---
> > >  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.