Re: [PATCH for-4.16 v4] gnttab: allow setting max version per-domain

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Oct 29, 2021 at 02:25:22PM +0100, Julien Grall wrote:
> On 29/10/2021 12:04, Roger Pau Monné wrote:
> > Hello,
> 
> Hi Roger,
> 
> > On Fri, Oct 29, 2021 at 11:01:29AM +0100, Julien Grall wrote:
> > > 
> > > 
> > > On 29/10/2021 10:41, Roger Pau Monné wrote:
> > > > On Fri, Oct 29, 2021 at 09:58:55AM +0100, Julien Grall wrote:
> > > > > On 29/10/2021 08:59, Roger Pau Monne wrote:
> > > > > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > > > > > index e510395d08..f94f0f272c 100644
> > > > > > --- a/xen/common/grant_table.c
> > > > > > +++ b/xen/common/grant_table.c
> > > > > > @@ -53,6 +53,7 @@ struct grant_table {
> > > > > >         percpu_rwlock_t       lock;
> > > > > >         /* Lock protecting the maptrack limit */
> > > > > >         spinlock_t            maptrack_lock;
> > > > > > +    unsigned int          max_version;
> > > > > >         /*
> > > > > >          * Defaults to v1.  May be changed with 
> > > > > > GNTTABOP_set_version.  All other
> > > > > >          * values are invalid.
> > > > > > @@ -1917,11 +1918,33 @@ active_alloc_failed:
> > > > > >     }
> > > > > >     int grant_table_init(struct domain *d, int max_grant_frames,
> > > > > > -                     int max_maptrack_frames)
> > > > > > +                     int max_maptrack_frames, unsigned int options)
> > > > > >     {
> > > > > >         struct grant_table *gt;
> > > > > > +    unsigned int max_grant_version = options & 
> > > > > > XEN_DOMCTL_GRANT_version_mask;
> > > > > >         int ret = -ENOMEM;
> > > > > > +    if ( max_grant_version == XEN_DOMCTL_GRANT_version_default )
> > > > > > +        max_grant_version = opt_gnttab_max_version;
> > > > > > +    if ( !max_grant_version )
> > > > > > +    {
> > > > > > +        dprintk(XENLOG_INFO, "%pd: invalid grant table version 0 
> > > > > > requested\n",
> > > > > > +                d);
> > > > > > +        return -EINVAL;
> > > > > > +    }
> > > > > > +    if ( max_grant_version > opt_gnttab_max_version )
> > > > > > +    {
> > > > > > +        dprintk(XENLOG_INFO,
> > > > > > +                "%pd: requested grant version (%u) greater than 
> > > > > > supported (%u)\n",
> > > > > > +                d, max_grant_version, opt_gnttab_max_version);
> > > > > > +        return -EINVAL;
> > > > > > +    }
> > > > > > +    if ( unlikely(max_page >= PFN_DOWN(TB(16))) && is_pv_domain(d) 
> > > > > > &&
> > > > > 
> > > > >   From my understanding, the limit for the grant table v1 is based on 
> > > > > the page
> > > > > granularity used and the size of the fields.
> > > > > 
> > > > > So the limit you add is valid for 4KB but not 16KB/64KB. Therefore, I 
> > > > > think
> > > > > it would be better to use:
> > > > > 
> > > > > 'max_page >= (1U << 32)'
> > > > 
> > > > I'm slightly confused. Isn't Xen always using a 4KB page granularity,
> > > 
> > > Yes. We only support 4KB today. But most of Xen is agnostic to the page
> > > granularity. I have actually started to look to allow 64KB/16KB page
> > > granularity for Xen on Arm in my spare time.
> > > 
> > > > and that also applies to the grant table entries?
> > > The page granularity for the hypercall interface is whatever the page
> > > granularity Xen is using. So...
> > 
> > I've somehow assumed that the current hypercall ABI was strictly tied
> > to 4KB pages, as that's for example already hardcoded in Linux
> > as XEN_PAGE_SIZE.
> 
> It is a mess. Before, XEN_PAGE_SIZE was introduced, we were assuming that
> Linux and Xen were using the same page granularity.
> 
> When I introduced the support to run guest with 16KB and 64KB, then we
> decided to introduce the define. Looking back at the decision, this was a
> mistake and we should have introduced an hypercall to fetch the ABI
> granularity instead.
> 
> > 
> > > > 
> > > > I don't think it's possible to use correctly use a 16KB or 64KB page
> > > > as an entry for the grant table, as Xen assumes those to always be 4KB
> > > > based.
> > > 
> > > ... if you build Xen with 16KB, then the grant table entries will be using
> > > 16KB.
> > > 
> > > So I would like to avoid making the assumption that we are always using 
> > > 4KB.
> > > That said, the worse that can happen is a spurious message. So this is 
> > > more
> > > to get an accurate check.
> > 
> > I don't have strong objections to using max_page >> 32, it might even
> > be clearer than checking against TB(16).

FWIW, I've changed the check to use >> 32 and limited it to
CONFIG_64BIT, since on 32bit arches max_page will be a 32bit value.

Thanks, Roger.