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 09:58:55AM +0100, Julien Grall wrote:
> Hi Roger,
> 
> 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,
and that also applies to the grant table entries?

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.

> Furthermore, it would add a comment explaining where this limit comes from.
> 
> Lastly, did you check the compiler wouldn't throw an error on arm32?

I've tested a previous version (v2), but not this one. I assume it
doesn't build?

I've pushed it to gitlab and will adjust as needed.

> > +         max_grant_version < 2 )
> > +        dprintk(XENLOG_INFO,
> > +                "%pd: host memory above 16Tb and grant table v2 
> > disabled\n",
> > +                d);
> 
> I would switch this one to a printk().

That's fine, will adjust unless someone has objections.

Thanks, Roger.