Re: [PATCH v6 4/5] x86/mm: Reject invalid cacheability in PV guests by default

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.12.2022 23:31, Demi Marie Obenour wrote:
> Setting cacheability flags that are not ones specified by Xen is a bug
> in the guest.  By default, return -EINVAL if a guests attempts to do
> this.  The invalid-cacheability= Xen command-line flag allows the
> administrator to allow such attempts or to produce

Unfinished sentence?

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1324,6 +1324,37 @@ static int put_page_from_l4e(l4_pgentry_t l4e, mfn_t 
> l4mfn, unsigned int flags)
>      return put_pt_page(l4e_get_page(l4e), mfn_to_page(l4mfn), flags);
>  }
>  
> +enum {
> +    INVALID_CACHEABILITY_ALLOW,
> +    INVALID_CACHEABILITY_DENY,
> +    INVALID_CACHEABILITY_TRAP,
> +};
> +
> +#ifdef NDEBUG
> +#define INVALID_CACHEABILITY_DEFAULT INVALID_CACHEABILITY_DENY
> +#else
> +#define INVALID_CACHEABILITY_DEFAULT INVALID_CACHEABILITY_TRAP
> +#endif
> +
> +static __ro_after_init uint8_t invalid_cacheability =
> +    INVALID_CACHEABILITY_DEFAULT;
> +
> +static int __init cf_check set_invalid_cacheability(const char *str)
> +{
> +    if (strcmp("allow", str) == 0)
> +        invalid_cacheability = INVALID_CACHEABILITY_ALLOW;
> +    else if (strcmp("deny", str) == 0)
> +        invalid_cacheability = INVALID_CACHEABILITY_DENY;
> +    else if (strcmp("trap", str) == 0)
> +        invalid_cacheability = INVALID_CACHEABILITY_TRAP;

Style: Missing blanks immediately inside if(). Also note that generally
we prefer '!' over "== 0".

> +    else
> +        return -EINVAL;
> +
> +    return 0;
> +}
> +
> +custom_param("invalid-cacheability", set_invalid_cacheability);

Nit: Generally we avoid blank lines between the handler of a
custom_param() and the actual param definition.

> @@ -1343,7 +1374,34 @@ static int promote_l1_table(struct page_info *page)
>          }
>          else
>          {
> -            switch ( ret = get_page_from_l1e(pl1e[i], d, d) )
> +            l1_pgentry_t l1e = pl1e[i];
> +
> +            if ( invalid_cacheability != INVALID_CACHEABILITY_ALLOW )
> +            {
> +                switch ( l1e.l1 & PAGE_CACHE_ATTRS )

You want to use l1e_get_flags() here.

Jan