Re: [PATCH v2 4/4] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.10.2024 23:25, Julien Grall wrote:
> Hi Jan,
> 
> On 24/09/2024 13:22, Jan Beulich wrote:
>> On 24.09.2024 13:54, Ayan Kumar Halder wrote:
>>> Hi Jan,
>>>
>>> On 23/09/2024 11:23, Jan Beulich wrote:
>>>> On 18.09.2024 19:51, Ayan Kumar Halder wrote:
>>>>> Secondary cpus initialization is not yet supported. Thus, we print an
>>>>> appropriate message and put the secondary cpus in WFE state.
>>>>>
>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>>>>> ---
>>>>> Changes from :-
>>>>>
>>>>> v1 - 1. NR_CPUS is defined as 1 for MPU
>>>> Not quite, what you do is ...
>>>>
>>>>> --- a/xen/arch/Kconfig
>>>>> +++ b/xen/arch/Kconfig
>>>>> @@ -11,6 +11,7 @@ config NR_CPUS
>>>>>           default "8" if ARM && RCAR3
>>>>>           default "4" if ARM && QEMU
>>>>>           default "4" if ARM && MPSOC
>>>>> + default "1" if ARM && MPU
>>>>>           default "128" if ARM
>>>>>           help
>>>>>             Controls the build-time size of various arrays and bitmaps
>>>> ... merely set the default. I wonder how useful that is, the more that
>>>> - as per the description - this is temporary only anyway.
>>>
>>> Yes, I can enforce a build time check like this.
>>>
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -295,6 +295,12 @@ void asmlinkage __init start_xen(unsigned long
>>> fdt_paddr)
>>>        struct domain *d;
>>>        int rc, i;
>>>
>>> +    /*
>>> +     * Currently MPU does not support SMP.
>>> +     */
>>> +#ifdef CONFIG_MPU
>>> +    BUILD_BUG_ON(NR_CPUS > 1);
>>> +#endif
>>>        dcache_line_bytes = read_dcache_line_bytes();
>>>
>>> Does this look ok ?
>>
>> Well, I'd still want to understand the purpose (if I was maintainer of
>> this code at least). You can't bring up secondary processors yet - fine.
>> But why does that mean you want to limit the build to NR_CPUS=1? Any
>> number is fine, but just not useful?
> 
> I have suggested to restrict NR_CPUS because it is not clear whether 
> cpu_up() would even work on MPU. In its current state, it may return an 
> error but could also corrupt the system.
> 
> At least for Xen on Arm, we have no other way to temporarly disable SMP 
> support (aside modify the provided DTB, but I would like to avoid it).

I see. Ayan: Could this be made a little more clear in the description
and / or code comment?

Furthermore I'm then puzzled by the use of BUILD_BUG_ON(). If you want
to prevent people building SMP configs, wouldn't you be better off doing
so right in Kconfig:

config NR_CPUS
        int "Maximum number of CPUs"
        range 1 16383 if !MPU
        range 1 1

or

config NR_CPUS
        int "Maximum number of CPUs"
        range 1 1 if MPU
        range 1 16383

? Since the 2nd form leaves the primary line untouched, I guess I like
this slightly better, despite the outlier then coming first. (The above
would presumably still require the default adjustment that you already
have.)

Finally, having observed interesting build issues with newer gcc when
trying to build UP configs, did you check that Arm actually builds fine
that way with recent-ish gcc?

Jan