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

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

On Fri, Oct 15, 2021 at 12:05:06PM +0200, Jan Beulich wrote:
> On 22.09.2021 10:21, Roger Pau Monne wrote:
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2649,7 +2649,8 @@ void __init create_domUs(void)
> >              .max_evtchn_port = -1,
> >              .max_grant_frames = -1,
> >              .max_maptrack_frames = -1,
> > -            .grant_opts = XEN_DOMCTL_GRANT_version_default,
> > +            .grant_opts = XEN_DOMCTL_GRANT_version_default |
> > +                          XEN_DOMCTL_GRANT_transitive,
> >          };
> >  
> >          if ( !dt_device_is_compatible(node, "xen,domain") )
> > @@ -2757,7 +2758,8 @@ void __init create_dom0(void)
> >          .max_evtchn_port = -1,
> >          .max_grant_frames = gnttab_dom0_frames(),
> >          .max_maptrack_frames = -1,
> > -        .grant_opts = XEN_DOMCTL_GRANT_version_default,
> > +        .grant_opts = XEN_DOMCTL_GRANT_version_default |
> > +                      XEN_DOMCTL_GRANT_transitive,
> >      };
> >  
> >      /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -750,7 +750,8 @@ static struct domain *__init create_dom0(const module_t 
> > *image,
> >          .max_evtchn_port = -1,
> >          .max_grant_frames = -1,
> >          .max_maptrack_frames = -1,
> > -        .grant_opts = XEN_DOMCTL_GRANT_version_default,
> > +        .grant_opts = XEN_DOMCTL_GRANT_version_default |
> > +                      XEN_DOMCTL_GRANT_transitive,
> >          .max_vcpus = dom0_max_vcpus(),
> >          .arch = {
> >              .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
> 
> While I can see that you make these adjustments for retaining backwards
> compatibility, I wonder if we need to, at least for Dom0. Dom0 doesn't
> normally grant anything anyway and hence would even less so use
> transitive grants. Of course then there's need to be a command line
> control to re-enable that, just in case.

dom0=gnttab-transitive, or should it be placed somewhere else?

> > @@ -1965,6 +1969,8 @@ int grant_table_init(struct domain *d, int 
> > max_grant_frames,
> >      gt->max_grant_frames = max_grant_frames;
> >      gt->max_maptrack_frames = max_maptrack_frames;
> >      gt->max_grant_version = max_grant_version;
> > +    gt->allow_transitive = !!(options & XEN_DOMCTL_GRANT_transitive) &&
> > +                           opt_transitive_grants;
> 
> No need for !! here afaics. Not even if there weren't the &&.
> 
> As to combining with the global option: I think if an admin requested
> them for a domain, this should overrule the command line option. Which
> in turn suggests that the command line option could go away at this
> point. Otherwise, if you AND both together and the guest is known to
> not work without this functionality, domain creation would instead
> better fail (or at the very least it should be logged by the tool
> stack that the request wasn't satisfied, which would require a means
> to retrieve the effective setting). IOW I would see the command line
> turning this off to trump the per-guest enabling request.

How should we go about deprecating it? It would be a bit antisocial
IMO to just drop the option, since people using it would then be
exposed to guests using transient grants if they didn't realize it
should be set in xl.conf or xl.cfg now.

> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -98,8 +98,11 @@ struct xen_domctl_createdomain {
> >  /* Grant version, use low 4 bits. */
> >  #define XEN_DOMCTL_GRANT_version_mask    0xf
> >  #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)
> 
> Omit the former and have the latter be 0x10 or (1U << 4)?
> 
> > -#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
> > +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_transitive
> 
> I didn't even spot this in patch 2 - what is this intended to be used
> for? Neither there nor here I can spot any use.

Yeah, AFAICT those _MAX definitions are used by the ocaml stubs to
assert the max option available. Given how these new options are handled
in ocaml the _MAX check is not implemented, so I could like drop those
(unless there's some other tool that also depends on them).

Thanks, Roger.