Re: [PATCH 2/6] gnttab: allow per-domain control over transitive grants

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

On Mon, Sep 20, 2021 at 10:32:24AM +0100, Andrew Cooper wrote:
> On 17/09/2021 16:46, Roger Pau Monne wrote:
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index 7f8456c50e..fe2201fca1 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -96,6 +96,14 @@ struct xen_domctl_createdomain {
> >      int32_t max_maptrack_frames;
> >      int32_t max_grant_version;
> >  
> > +/* Allow transitive grants. */
> > +#define _XEN_DOMCTL_GRANT_transitive  0
> > +#define XEN_DOMCTL_GRANT_transitive   (1U << _XEN_DOMCTL_GRANT_transitive)
> 
> There's no need for bit position variables.
> 
> > +
> > +#define XEN_DOMCTL_GRANT_MAX XEN_DOMCTL_GRANT_transitive
> > +
> > +    uint32_t grant_opts;
> 
> So far, we've got 3 bits of information, v1, v2 and transitive, and
> we're tight on space in the structure with loads more to fit in.
> 
> I was thinking grant_flags or equiv to contain these 3 settings, and any
> further which might appear.

What about using something like the below?

We also need to consider selecting the default version (whatever is
set on the hypervisor) and no grant table at all.

/* Grant version, use low 4 bits. */
#define XEN_DOMCTL_GRANT_disable         0
#define XEN_DOMCTL_GRANT_version_v1      1
#define XEN_DOMCTL_GRANT_version_v2      2
#define XEN_DOMCTL_GRANT_version_default 0xf
/* Allow transitive grants. */
#define _XEN_DOMCTL_GRANT_transitive  4
#define XEN_DOMCTL_GRANT_transitive   (1U << _XEN_DOMCTL_GRANT_transitive)

#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_transitive

    uint32_t grant_opts;

> 
> 
> One thing which is missing however is the enumeration of which settings
> are available, and rejection of bad settings.  If v2 is disabled
> globally, trying to create a VM with v2 needs to fail.

Right, I think this is already the case with the current
implementation. This doesn't happen however with the transitive
option, as I implemented it and'ing the hypervisor selection to the
tools provided one, partially due to the lack of a 'use hypervisor
default' option.

Thanks, Roger.