diff mbox series

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

Message ID 20240918175102.223076-5-ayan.kumar.halder@amd.com (mailing list archive)
State Superseded
Headers show
Series Enable early bootup of AArch64 MPU systems | expand

Commit Message

Ayan Kumar Halder Sept. 18, 2024, 5:51 p.m. UTC
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@amd.com>
---
Changes from :-

v1 - 1. NR_CPUS is defined as 1 for MPU

2. Added a message in enable_secondary_cpu_mm()

 xen/arch/Kconfig              |  1 +
 xen/arch/arm/arm64/mpu/head.S | 10 ++++++++++
 2 files changed, 11 insertions(+)

Comments

Julien Grall Sept. 19, 2024, 1:20 p.m. UTC | #1
Hi,

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@amd.com>
> ---
> Changes from :-
> 
> v1 - 1. NR_CPUS is defined as 1 for MPU
> 
> 2. Added a message in enable_secondary_cpu_mm()
> 
>   xen/arch/Kconfig              |  1 +
>   xen/arch/arm/arm64/mpu/head.S | 10 ++++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
> index 308ce129a8..8640b7ec8b 100644
> --- 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
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> index ef55c8765c..3dfbbf8879 100644
> --- a/xen/arch/arm/arm64/mpu/head.S
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -168,6 +168,16 @@ FUNC(enable_boot_cpu_mm)
>       b   1b
>   END(enable_boot_cpu_mm)
>   
> +/*
> + * Secondary cpu has not yet been supported on MPU systems. We will block the
> + * secondary cpu bringup at this stage.

Given that NR_CPUS is 1, this should not be reachable. How about the 
following comment:

"We don't yet support secondary CPUs bring-up. Implement a dummy helper 
to please the common code."

> + */
> +ENTRY(enable_secondary_cpu_mm)
> +1:  PRINT("- SMP not enabled yet -\r\n")

You want the print to be outside of the loop. Otherwise, it will spam 
the console in the unlikely case the code is reached.

> +    wfe
> +    b 1b
> +ENDPROC(enable_secondary_cpu_mm)
> +
>   /*
>    * Local variables:
>    * mode: ASM

Cheers,
Jan Beulich Sept. 23, 2024, 10:23 a.m. UTC | #2
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@amd.com>
> ---
> 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.

Jan
Ayan Kumar Halder Sept. 24, 2024, 11:34 a.m. UTC | #3
On 19/09/2024 14:20, Julien Grall wrote:
> Hi,
Hi Julien,
>
> 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@amd.com>
>> ---
>> Changes from :-
>>
>> v1 - 1. NR_CPUS is defined as 1 for MPU
>>
>> 2. Added a message in enable_secondary_cpu_mm()
>>
>>   xen/arch/Kconfig              |  1 +
>>   xen/arch/arm/arm64/mpu/head.S | 10 ++++++++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
>> index 308ce129a8..8640b7ec8b 100644
>> --- 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
>> diff --git a/xen/arch/arm/arm64/mpu/head.S 
>> b/xen/arch/arm/arm64/mpu/head.S
>> index ef55c8765c..3dfbbf8879 100644
>> --- a/xen/arch/arm/arm64/mpu/head.S
>> +++ b/xen/arch/arm/arm64/mpu/head.S
>> @@ -168,6 +168,16 @@ FUNC(enable_boot_cpu_mm)
>>       b   1b
>>   END(enable_boot_cpu_mm)
>>   +/*
>> + * Secondary cpu has not yet been supported on MPU systems. We will 
>> block the
>> + * secondary cpu bringup at this stage.
>
> Given that NR_CPUS is 1, this should not be reachable. How about the 
> following comment:
>
> "We don't yet support secondary CPUs bring-up. Implement a dummy 
> helper to please the common code."
Ack.
>
>> + */
>> +ENTRY(enable_secondary_cpu_mm)
>> +1:  PRINT("- SMP not enabled yet -\r\n")
>
> You want the print to be outside of the loop. Otherwise, it will spam 
> the console in the unlikely case the code is reached.

Ack.

- Ayan

>
>> +    wfe
>> +    b 1b
>> +ENDPROC(enable_secondary_cpu_mm)
>> +
>>   /*
>>    * Local variables:
>>    * mode: ASM
>
> Cheers,
>
Ayan Kumar Halder Sept. 24, 2024, 11:54 a.m. UTC | #4
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@amd.com>
>> ---
>> 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 ?

- Ayan
Jan Beulich Sept. 24, 2024, 12:22 p.m. UTC | #5
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@amd.com>
>>> ---
>>> 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?

Jan
Julien Grall Oct. 7, 2024, 9:25 p.m. UTC | #6
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@amd.com>
>>>> ---
>>>> 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).

Cheers,
Jan Beulich Oct. 8, 2024, 6:21 a.m. UTC | #7
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@amd.com>
>>>>> ---
>>>>> 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
diff mbox series

Patch

diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 308ce129a8..8640b7ec8b 100644
--- 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
diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
index ef55c8765c..3dfbbf8879 100644
--- a/xen/arch/arm/arm64/mpu/head.S
+++ b/xen/arch/arm/arm64/mpu/head.S
@@ -168,6 +168,16 @@  FUNC(enable_boot_cpu_mm)
     b   1b
 END(enable_boot_cpu_mm)
 
+/*
+ * Secondary cpu has not yet been supported on MPU systems. We will block the
+ * secondary cpu bringup at this stage.
+ */
+ENTRY(enable_secondary_cpu_mm)
+1:  PRINT("- SMP not enabled yet -\r\n")
+    wfe
+    b 1b
+ENDPROC(enable_secondary_cpu_mm)
+
 /*
  * Local variables:
  * mode: ASM