Re: [PATCH for-4.19 v2 2/3] xen/x86: enable altp2m at create domain domctl

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/05/2024 12:23 pm, Roger Pau Monne wrote:
> Enabling it using an HVM param is fragile, and complicates the logic when
> deciding whether options that interact with altp2m can also be enabled.
>
> Leave the HVM param value for consumption by the guest, but prevent it from
> being set.  Enabling is now done using the misc_flags field in
> xen_arch_domainconfig.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - New in this version.

Ha.  So this is actually work that Petr has been wanting to do.

Petr has a series hoping to make it into 4.19 (x86: Make MAX_ALTP2M
configurable), which just missed out on this side of things.

altp2m is not architecture specific at all, and there's even support for
ARM out on the mailing list.  Therefore, the altp2m mode wants to be
common, just like the new MAX_ALTP2M setting already is.

Both fields can reasonably share uint32_t, but could you work with Petr
to make both halfs of this land cleanly.


As to the HVMPARAM, I'd really quite like to delete it.  It was always a
bodge, and there's a full set of HVMOP_altp2m_* for a guest to use.

> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 20e83cf38bbd..dff790060605 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -708,13 +711,33 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          }
>      }
>  
> -    if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
> +    if ( config->arch.misc_flags & ~XEN_X86_MISC_FLAGS_ALL )
>      {
>          dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
>                  config->arch.misc_flags);
>          return -EINVAL;
>      }
>  
> +    if ( altp2m && (altp2m & (altp2m - 1)) )
> +    {
> +        dprintk(XENLOG_INFO, "Multiple altp2m options selected in flags: 
> %#x\n",
> +                config->flags);
> +        return -EINVAL;

I think this would be clearer to follow by having a 2 bit field called
altp2m_mode and check for <= 2.

> +    }
> +
> +    if ( altp2m && nested_virt )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "Nested virt and altp2m are mutually incompatible\n");

There's nothing inherently incompatible.  I think it's more that noone
had any interest in trying to make it work in combination with nested p2ms.

I'd phrase it as "not supported", rather than incompatible.

> +        return -EINVAL;
> +    }
> +
> +    if ( altp2m && !hap )
> +    {
> +        dprintk(XENLOG_INFO, "altp2m requires HAP\n");
> +        return -EINVAL;
> +    }

altp2m ought to work fine with shadow.  It's only if you want VMFUNC/#VE
acceleration that you depend on EPT.

Again, I'd phrase this as "not supported".

~Andrew