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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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.

> --- 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.

Jan