Re: [PATCH v2 4/7] x86: Make the maximum number of altp2m views configurable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.04.2024 18:00, Petr Beneš wrote:
> On Tue, Apr 30, 2024 at 4:27 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -685,6 +685,12 @@ int arch_sanitise_domain_config(struct 
>>> xen_domctl_createdomain *config)
>>>          return -EINVAL;
>>>      }
>>>
>>> +    if ( config->max_altp2m > MAX_EPTP )
>>> +    {
>>> +        dprintk(XENLOG_INFO, "max_altp2m must be <= %u\n", MAX_EPTP);
>>> +        return -EINVAL;
>>> +    }
>>
>> ... using MAX_EPTP here feels like a layering violation to me. Imo there want
>> to be separate constants, tied together with a suitably placed 
>> BUILD_BUG_ON().
>>
>> Furthermore comparisons like this (there are further ones elsewhere) suggest
>> there is a (continued) naming issue: A max_ or MAX_ prefix ought to name a
>> "maximum valid value", not "number of permitted values". This is not a
>> request to alter MAX_EPTP, but one to make sure the new struct fields really
>> have matching names and purposes.
> 
> Do you have any proposals? I was considering nr_altp2m. Another
> question is what it should be named in xl.cfg - also nr_altp2m? I was
> too hesitant to name it like that, since there aren't any nr_* fields
> currently.

Internally nr_ or num_ are going to be fine. For xl whether either of
those would be, or maybe altp2ms= (along the lines of e.g. vcpus=), or
altp2m_count, or yet something else I simply don't know. That'll be
the maintainers there to help with.

Jan