Hello,
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:
> > > 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'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.
> >
> > 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).
It's just that the check would be wrong if we allow Xen itself to use
a different page size than the one used by the grant table interface
to the guest.
> >
> > > 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?
Hm, possibly. It seems like debian-unstable arm32 gcc is building OK,
but I've got no idea if different compiler versions could complain.
Thanks, Roger.
