Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 22/09/2020 19:56, Andrew Cooper wrote:
> On 22/09/2020 19:20, Julien Grall wrote:
> > > > +
> > > >    #endif /* __ASM_DOMAIN_H__ */
> > > >      /*
> > > > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > > > index 5c5e55ebcb76..7564df5e8374 100644
> > > > --- a/xen/include/public/domctl.h
> > > > +++ b/xen/include/public/domctl.h
> > > > @@ -136,6 +136,12 @@ struct xen_domctl_getdomaininfo {
> > > >        uint64_aligned_t outstanding_pages;
> > > >        uint64_aligned_t shr_pages;
> > > >        uint64_aligned_t paged_pages;
> > > > +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)
> > > We've already got INVALID_GFN as a constant used in the interface.  Lets
> > > not proliferate more.
> > This was my original approach (see [1]) but this was reworked because:
> >     1) INVALID_GFN is not technically defined in the ABI. So the
> > toolstack has to hardcode the value in the check.
> >     2) The value is different between 32-bit and 64-bit Arm as
> > INVALID_GFN is defined as an unsigned long.
> >
> > So providing a new define is the right way to go.
> There is nothing special about this field.  It should not have a
> dedicated constant, when a general one is the appropriate one to use.
>
> libxl already has LIBXL_INVALID_GFN, which is already used.
Right, but that's imply it cannot be used by libxc as this would be a 
layer violation.
> If this isn't good enough, them the right thing to do is put a proper
> INVALID_GFN in the tools interface.
That would be nice but I can see some issue on x86 given that we don't 
consistenly define a GFN in the interface as a 64-bit value.
So would you still be happy to consider introducing XEN_INVALID_GFN in 
the interface with some caveats?
> > > >        uint64_aligned_t cpu_time;
> > > >        uint32_t nr_online_vcpus;    /* Number of VCPUs currently
> > > > online. */
> > > > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> > > > index cde0d9c7fe63..7281eb7b36c7 100644
> > > > --- a/xen/include/xen/domain.h
> > > > +++ b/xen/include/xen/domain.h
> > > > @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma);
> > > >    static inline void vnuma_destroy(struct vnuma_info *vnuma) {
> > > > ASSERT(!vnuma); }
> > > >    #endif
> > > >    +#ifdef CONFIG_HAS_M2P
> > > > +#define domain_shared_info_gfn(d) ({                            \
> > > > +    const struct domain *d_ = (d);                              \
> > > > +    gfn_t gfn_;                                                 \
> > > > +                                                                \
> > > > +    gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\
> > > > +    BUG_ON(SHARED_M2P(gfn_x(gfn_)));                            \
> > > > +                                                                \
> > > > +    gfn_;                                                       \
> > > > +})
> > > ... this wants to be
> > >
> > > #ifndef arch_shared_info_gfn
> > > static inline gfn_t arch_shared_info_gfn(const struct domain *d) {
> > > return INVALID_GFN; }
> > > #endif
> > >
> > > with
> > >
> > > gfn_t arch_shared_info_gfn(const struct domain *d);
> > > #define arch_shared_info_gfn arch_shared_info_gfn
> > >
> > > in asm-x86/domain.h
> > >
> > > and the actual implementation in arch/x86/domain.c
> > What's wrong with implement it in xen/domain.h? After all there is
> > nothing x86 specific in the implementation...
> d->shared_info is allocated in arch specific code, not common code.
> This macro assumes that __virt_to_mfn() is safe to call on the pointer.
That's a fair point. I will move the code in an x86 specific files.

Cheers,

--
Julien Grall