On 22.05.2024 18:21, Roger Pau Monné wrote:
> On Wed, May 22, 2024 at 03:34:29PM +0200, Jan Beulich wrote:
>> On 22.05.2024 15:16, Roger Pau Monné wrote:
>>> On Tue, May 21, 2024 at 12:30:32PM +0200, Jan Beulich wrote:
>>>> On 17.05.2024 15:33, 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 and additional altp2m specific
>>>>> field in
>>>>> xen_domctl_createdomain.
>>>>>
>>>>> Note that albeit only currently implemented in x86, altp2m could be
>>>>> implemented
>>>>> in other architectures, hence why the field is added to
>>>>> xen_domctl_createdomain
>>>>> instead of xen_arch_domainconfig.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
>>>>
>>>> Reviewed-by: Jan Beulich <jbeul...@suse.com> # hypervisor
>>>> albeit with one question:
>>>>
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -637,6 +637,8 @@ int arch_sanitise_domain_config(struct
>>>>> xen_domctl_createdomain *config)
>>>>> bool hap = config->flags & XEN_DOMCTL_CDF_hap;
>>>>> bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
>>>>> unsigned int max_vcpus;
>>>>> + unsigned int altp2m_mode = MASK_EXTR(config->altp2m_opts,
>>>>> + XEN_DOMCTL_ALTP2M_mode_mask);
>>>>>
>>>>> if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
>>>>> {
>>>>> @@ -715,6 +717,26 @@ int arch_sanitise_domain_config(struct
>>>>> xen_domctl_createdomain *config)
>>>>> return -EINVAL;
>>>>> }
>>>>>
>>>>> + if ( config->altp2m_opts & ~XEN_DOMCTL_ALTP2M_mode_mask )
>>>>> + {
>>>>> + dprintk(XENLOG_INFO, "Invalid altp2m options selected: %#x\n",
>>>>> + config->flags);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if ( altp2m_mode && nested_virt )
>>>>> + {
>>>>> + dprintk(XENLOG_INFO,
>>>>> + "Nested virt and altp2m are not supported together\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if ( altp2m_mode && !hap )
>>>>> + {
>>>>> + dprintk(XENLOG_INFO, "altp2m is only supported with HAP\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>
>>>> Should this last one perhaps be further extended to permit altp2m with EPT
>>>> only?
>>>
>>> Hm, yes, that would be more accurate as:
>>>
>>> if ( altp2m_mode && (!hap || !hvm_altp2m_supported()) )
>>
>> Wouldn't
>>
>> if ( altp2m_mode && !hvm_altp2m_supported() )
>>
>> suffice? hvm_funcs.caps.altp2m is not supposed to be set when no HAP,
>> as long as HAP continues to be a pre-condition?
>
> No, `hap` here signals whether the domain is using HAP, and we need to
> take this int account, otherwise we would allow enabling altp2m for
> domains using shadow.
Oh, right. But then the original for is good enough HAP-wise, as a request
to use HAP when HAP isn't available is deal with elsewhere. The
!hvm_altp2m_supported() is still wanted imo (for there potentially being
other restrictions), but then in a separate check, not resulting in a HAP-
specific log message. I'll commit the patch in its original form, and that
further addition can then be an incremental change.
Jan
