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

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On Fri, Dec 16, 2022 at 04:34:50PM +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.

Thanks Sean. But I do not get it.
May I ask why ABI usages are different?  Or is there any documentation
describing the requirement? Thanks!

>                                                                                 
> So, sadly, we can't simply introduce INVALID_GFN here, and instead need to do
> something like:
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 20522d4ba1e0..2d31caaf812c 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1766,6 +1766,7 @@ struct kvm_xen_hvm_attr {
>                 __u8 vector;
>                 __u8 runstate_update_flag;
>                 struct {
> +#define KVM_XEN_INVALID_GFN    (~0ull)
>                         __u64 gfn;
>                 } shared_info;

I guess above policy shall also be applied for the gpa inside struct
kvm_xen_vcpu_attr. Instead of using INVALID_GPA (in patch 2), should
be like:

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 61c052d51a64..c06ef8ed9680 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1823,6 +1823,7 @@ struct kvm_xen_vcpu_attr {
        __u16 type;
        __u16 pad[3];
        union {
+#define KVM_XEN_INVALID_GPA            (~0ull)
                __u64 gpa;
                __u64 pad[8];
                struct {

Also, xen.c should use KVM_XEN_INVALID_GPA for GPA values...

B.R.
Yu