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

[Julien Grall <julien@xxxxxxx>]

On 29/10/2021 10:41, Roger Pau Monné wrote:
> On Fri, Oct 29, 2021 at 09:58:55AM +0100, Julien Grall wrote:
> > Hi Roger,
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,
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 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.
> > 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 haven't tried. But I remember in the past seen report for always 
true/false check. Maybe that was just on coverity?
> I've pushed it to gitlab and will adjust as needed.
Thanks!

Cheers,

--
Julien Grall