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

[Julien Grall <julien@xxxxxxx>]

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)'

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?

> +         max_grant_version < 2 )
> +        dprintk(XENLOG_INFO,
> +                "%pd: host memory above 16Tb and grant table v2 disabled\n",
> +                d);
I would switch this one to a printk().

Cheers,

--
Julien Grall