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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/10/2021 08:59, Roger Pau Monne wrote:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 0167731ab0..faeb3eba76 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2967,6 +2967,7 @@ void __init create_domUs(void)
>              .max_evtchn_port = -1,
>              .max_grant_frames = -1,
>              .max_maptrack_frames = -1,
> +            .grant_opts = XEN_DOMCTL_GRANT_version_default,

These three will need to be opt_gnttab_max_version which will need
exporting.

See final comment for why "default" mustn't exist.

> 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
> @@ -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;
> +    }

I think this wants to live in sanitise_domain_config() along with all
the other auditing of flags and settings.  Also, it can be simplified:

if ( max_grant_version < 1 ||
    max_grant_version > opt_gnttab_max_version )
{
    dprintk(XENLOG_INFO, "Requested gnttab max version %u outside of
supported range [%u, %u]\n", ...);
}



> +    if ( unlikely(max_page >= PFN_DOWN(TB(16))) && is_pv_domain(d) &&
> +         max_grant_version < 2 )
> +        dprintk(XENLOG_INFO,
> +                "%pd: host memory above 16Tb and grant table v2 disabled\n",
> +                d);

This is rather more complicated.

For PV, this going wrong in the first place is conditional on CONFIG_BIGMEM.
For HVM, it the guest address size, not the host.
For ARM, I don't even know, because I've lost track of which bits of the
ABI are directmap in an otherwise translated domain.

I think it is probably useful to do something about it, but probably not
in this patch.

Perhaps modify domain_set_alloc_bitsize() to impose an upper limit for
the "host memory size matters" cases?

For the guest address size cases, this possibly wants to feed in to the
max policy calculations in the same way that shadow kinda does.

> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 51017b47bc..0ec57614bd 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -89,14 +89,20 @@ struct xen_domctl_createdomain {
>      /*
>       * Various domain limits, which impact the quantity of resources
>       * (global mapping space, xenheap, etc) a guest may consume.  For
> -     * max_grant_frames and max_maptrack_frames, < 0 means "use the
> -     * default maximum value in the hypervisor".
> +     * max_grant_frames, max_maptrack_frames and max_gnttab_version < 0
> +     * means "use the default maximum value in the hypervisor".
>       */
>      uint32_t max_vcpus;
>      uint32_t max_evtchn_port;
>      int32_t max_grant_frames;
>      int32_t max_maptrack_frames;
>  
> +/* Grant version, use low 4 bits. */
> +#define XEN_DOMCTL_GRANT_version_mask    0xf
> +#define XEN_DOMCTL_GRANT_version_default 0xf

This needs to be a toolstack decision, not something in Xen.  This
doesn't fix the case where VMs can't cope with change underfoot.

It is fine for the user say "use the default", but this must be turned
into an explicit 1 or 2 by the toolstack, so that the version(s) visible
to the guest remains invariant while it is booted.

Given the timescales, I'll put together a prereq patch exposing
gnttab-v1/2 in virt_caps for the toolstack to reason over.

~Andrew