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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2023 23:07, 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

Like in v6: Unfinished sentence?

> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> Changes since v6:
> - Make invalid-cacheability= a subflag of pv=.

While you did this, you've retained the standalone option, and documentation
also continues to describe that one instead of the new sub-option. You will
then also want to move where invalid_cacheability is defined, I think.

> @@ -1343,7 +1392,9 @@ 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];
> +
> +            switch ( ret = get_page_from_l1e(l1e, d, d) )
>              {
>              default:
>                  goto fail;

Stale (and now pointless) change?

> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -28,9 +28,21 @@ static int __init cf_check parse_pv(const char *s)
>      do {
>          ss = strchr(s, ',');
>          if ( !ss )
> -            ss = strchr(s, '\0');
> -
> -        if ( (val = parse_boolean("32", s, ss)) >= 0 )
> +            ss += strlen(s);
> +        if ( !strncmp("invalid-cacheability=", s,
> +                      sizeof("invalid-cacheability=") - 1) )
> +        {
> +            const char *p = s + (sizeof("invalid-cacheability=") - 1);
> +            if (ss - p == 5 && !memcmp(p, "allow", 5))

Nit: Blank line please between declaration(s) and statement(s).

Jan