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?
> Would you be fine adjusting at commit, or would you prefer me to send
> an updated version?
I'd be happy to fold in whatever we settle on.
Jan
