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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.05.2024 10:23, Roger Pau Monné wrote:
> On Wed, May 08, 2024 at 08:38:07PM +0100, Andrew Cooper wrote:
>> 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.
> 
> Initially I had it as a set of XEN_DOMCTL_CDF_* flags, but it wasn't
> clear to me whether the modes could be shared between arches.
> 
>> Both fields can reasonably share uint32_t, but could you work with Petr
>> to make both halfs of this land cleanly.
> 
> I'm happy for Petr to pick this patch as part of the series if he
> feels like.
> 
> I assume the plan would be to add an XEN_DOMCTL_CDF_altp2m flag, and
> then a new field to signal the mode.
> 
>>
>> 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.
> 
> I've assumed we must keep HVM_PARAM_ALTP2M for backwards
> compatibility.
> 
>>> 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.
> 
> Don't we need 3 bits, for mixed, external and limited modes?
> 
> We could do with 2 bits if we signal altp2m enabled in a different
> field, and then introduce a field to just contain the mode.

I think what Andrew meant is

#define XEN_X86_ALTP2M_MIXED   (1U << 1)
/* Enable altp2m external mode. */
#define XEN_X86_ALTP2M_EXT     (2U << 1)
/* Enable altp2m limited mode. */
#define XEN_X86_ALTP2M_LIMITED (3U << 1)

(leaving aside the x86-only vs common aspect). That would also eliminate
the ability to request multiple (conflicting) modes.

Jan