| 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 |
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,
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
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, >
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
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
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,
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 --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
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(+)