| Message ID | 20230620151736.3720850-1-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] xen/arm: arm32: Add support to identify the Cortex-R52 processor | expand |
Hi, On 20/06/2023 16:17, Ayan Kumar Halder wrote: > Add a special configuration (CONFIG_AARCH32_V8R) to setup the Cortex-R52 > specifics. > > Cortex-R52 is an Arm-V8R AArch32 processor. > > Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, > bits[31:24] = 0x41 , Arm Ltd > bits[23:20] = Implementation defined > bits[19:16] = 0xf , Arch features are individually identified > bits[15:4] = Implementation defined > bits[3:0] = Implementation defined > > Thus, the processor id is 0x410f0000 and the processor id mask is > 0xff0f0000 > > Also, there is no special initialization required for R52. Are you saying that Xen upstream + this patch will boot on Cortex-R52? > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > xen/arch/arm/Kconfig | 7 +++++++ > xen/arch/arm/arm32/Makefile | 1 + > xen/arch/arm/arm32/proc-v8.S | 32 ++++++++++++++++++++++++++++++++ > 3 files changed, 40 insertions(+) > create mode 100644 xen/arch/arm/arm32/proc-v8.S > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index 61e581b8c2..c45753a2dd 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -3,6 +3,13 @@ config ARM_32 > depends on "$(ARCH)" = "arm32" > select ARCH_MAP_DOMAIN_PAGE > > +config AARCH32_V8R > + bool "AArch32 Arm V8R Support (UNSUPPORTED)" if UNSUPPORTED > + def_bool n > + depends on ARM_32 > + help > + This option enables Armv8-R profile for AArch32. > + > config ARM_64 > def_bool y > depends on !ARM_32 > diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile > index 520fb42054..2ab808a7a8 100644 > --- a/xen/arch/arm/arm32/Makefile > +++ b/xen/arch/arm/arm32/Makefile > @@ -8,6 +8,7 @@ obj-y += head.o > obj-y += insn.o > obj-$(CONFIG_LIVEPATCH) += livepatch.o > obj-y += proc-v7.o proc-caxx.o > +obj-$(CONFIG_AARCH32_V8R) += proc-v8.o > obj-y += smpboot.o > obj-y += traps.o > obj-y += vfp.o > diff --git a/xen/arch/arm/arm32/proc-v8.S b/xen/arch/arm/arm32/proc-v8.S > new file mode 100644 > index 0000000000..c5a566b165 > --- /dev/null > +++ b/xen/arch/arm/arm32/proc-v8.S Below you say the file will contain v8r specific initialization. So please rename it to proc-v8r.S. Cheers,
On 20/06/2023 17:41, Julien Grall wrote: > Hi, Hi Julien, > > On 20/06/2023 16:17, Ayan Kumar Halder wrote: >> Add a special configuration (CONFIG_AARCH32_V8R) to setup the Cortex-R52 >> specifics. >> >> Cortex-R52 is an Arm-V8R AArch32 processor. >> >> Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, >> bits[31:24] = 0x41 , Arm Ltd >> bits[23:20] = Implementation defined >> bits[19:16] = 0xf , Arch features are individually identified >> bits[15:4] = Implementation defined >> bits[3:0] = Implementation defined >> >> Thus, the processor id is 0x410f0000 and the processor id mask is >> 0xff0f0000 >> >> Also, there is no special initialization required for R52. > > Are you saying that Xen upstream + this patch will boot on Cortex-R52? This patch will help for earlyboot of Xen. With this patch, cpu_init() will work on Cortex-R52. There will be changes required for the MPU configuration, but that will be sent after Penny's patch serie "[PATCH v2 00/41] xen/arm: Add Armv8-R64 MPU support to Xen - Part#1" is upstreamed. My aim is to extract the non-dependent changes and send them for review. > >> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> xen/arch/arm/Kconfig | 7 +++++++ >> xen/arch/arm/arm32/Makefile | 1 + >> xen/arch/arm/arm32/proc-v8.S | 32 ++++++++++++++++++++++++++++++++ >> 3 files changed, 40 insertions(+) >> create mode 100644 xen/arch/arm/arm32/proc-v8.S >> >> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >> index 61e581b8c2..c45753a2dd 100644 >> --- a/xen/arch/arm/Kconfig >> +++ b/xen/arch/arm/Kconfig >> @@ -3,6 +3,13 @@ config ARM_32 >> depends on "$(ARCH)" = "arm32" >> select ARCH_MAP_DOMAIN_PAGE >> +config AARCH32_V8R >> + bool "AArch32 Arm V8R Support (UNSUPPORTED)" if UNSUPPORTED >> + def_bool n >> + depends on ARM_32 >> + help >> + This option enables Armv8-R profile for AArch32. >> + >> config ARM_64 >> def_bool y >> depends on !ARM_32 >> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile >> index 520fb42054..2ab808a7a8 100644 >> --- a/xen/arch/arm/arm32/Makefile >> +++ b/xen/arch/arm/arm32/Makefile >> @@ -8,6 +8,7 @@ obj-y += head.o >> obj-y += insn.o >> obj-$(CONFIG_LIVEPATCH) += livepatch.o >> obj-y += proc-v7.o proc-caxx.o >> +obj-$(CONFIG_AARCH32_V8R) += proc-v8.o >> obj-y += smpboot.o >> obj-y += traps.o >> obj-y += vfp.o >> diff --git a/xen/arch/arm/arm32/proc-v8.S b/xen/arch/arm/arm32/proc-v8.S >> new file mode 100644 >> index 0000000000..c5a566b165 >> --- /dev/null >> +++ b/xen/arch/arm/arm32/proc-v8.S > > Below you say the file will contain v8r specific initialization. So > please rename it to proc-v8r.S. Ack. - Ayan > > Cheers, >
Hi Ayan, On 20/06/2023 19:28, Ayan Kumar Halder wrote: > > On 20/06/2023 17:41, Julien Grall wrote: >> Hi, > Hi Julien, >> >> On 20/06/2023 16:17, Ayan Kumar Halder wrote: >>> Add a special configuration (CONFIG_AARCH32_V8R) to setup the Cortex-R52 >>> specifics. >>> >>> Cortex-R52 is an Arm-V8R AArch32 processor. >>> >>> Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, >>> bits[31:24] = 0x41 , Arm Ltd >>> bits[23:20] = Implementation defined >>> bits[19:16] = 0xf , Arch features are individually identified >>> bits[15:4] = Implementation defined >>> bits[3:0] = Implementation defined >>> >>> Thus, the processor id is 0x410f0000 and the processor id mask is >>> 0xff0f0000 >>> >>> Also, there is no special initialization required for R52. >> >> Are you saying that Xen upstream + this patch will boot on Cortex-R52? > > This patch will help for earlyboot of Xen. With this patch, cpu_init() > will work on Cortex-R52. > > There will be changes required for the MPU configuration, but that will > be sent after Penny's patch serie "[PATCH v2 00/41] xen/arm: Add > Armv8-R64 MPU support to Xen - Part#1" is upstreamed. > > My aim is to extract the non-dependent changes and send them for review. I can review the patch. But I am not willing to merge it as it gives the false impression that Xen would boot on Cortex-R52. In fact, I think this patch should only be merged once we have all the MPU merged. IMHO, patches are independent are rework (e.g. code split...) that would help the MPU. Cheers,
On 20/06/2023 21:43, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 20/06/2023 19:28, Ayan Kumar Halder wrote: >> >> On 20/06/2023 17:41, Julien Grall wrote: >>> Hi, >> Hi Julien, >>> >>> On 20/06/2023 16:17, Ayan Kumar Halder wrote: >>>> Add a special configuration (CONFIG_AARCH32_V8R) to setup the >>>> Cortex-R52 >>>> specifics. >>>> >>>> Cortex-R52 is an Arm-V8R AArch32 processor. >>>> >>>> Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, >>>> bits[31:24] = 0x41 , Arm Ltd >>>> bits[23:20] = Implementation defined >>>> bits[19:16] = 0xf , Arch features are individually identified >>>> bits[15:4] = Implementation defined >>>> bits[3:0] = Implementation defined >>>> >>>> Thus, the processor id is 0x410f0000 and the processor id mask is >>>> 0xff0f0000 >>>> >>>> Also, there is no special initialization required for R52. >>> >>> Are you saying that Xen upstream + this patch will boot on Cortex-R52? >> >> This patch will help for earlyboot of Xen. With this patch, >> cpu_init() will work on Cortex-R52. >> >> There will be changes required for the MPU configuration, but that >> will be sent after Penny's patch serie "[PATCH v2 00/41] xen/arm: Add >> Armv8-R64 MPU support to Xen - Part#1" is upstreamed. >> >> My aim is to extract the non-dependent changes and send them for review. > > I can review the patch. But I am not willing to merge it as it gives > the false impression that Xen would boot on Cortex-R52. > > In fact, I think this patch should only be merged once we have all the > MPU merged. > > IMHO, patches are independent are rework (e.g. code split...) that > would help the MPU. Yes, that's exactly what I intend to do. I will send out the patches (similar to this) which will not be impacted by the MPU changes. So, that both as an author/reviewer, it helps to restrict MPU serie to only MPU specific changes. Can you suggest some change to the commit message, so that we can commit it (without giving any false impression that Xen boots on Cortex-R52) ? May be adding this line to the commit message helps ? "However, Xen does not fully boot on Cortex-R52 as it requires MPU support which is under review. Once Xen is supported on Cortex-R52, SUPPORT.md will be amended to reflect it." - Ayan > > Cheers, >
Hi Ayan, On 22/06/2023 09:59, Ayan Kumar Halder wrote: > > On 20/06/2023 21:43, Julien Grall wrote: >> Hi Ayan, > Hi Julien, >> >> On 20/06/2023 19:28, Ayan Kumar Halder wrote: >>> >>> On 20/06/2023 17:41, Julien Grall wrote: >>>> Hi, >>> Hi Julien, >>>> >>>> On 20/06/2023 16:17, Ayan Kumar Halder wrote: >>>>> Add a special configuration (CONFIG_AARCH32_V8R) to setup the >>>>> Cortex-R52 >>>>> specifics. >>>>> >>>>> Cortex-R52 is an Arm-V8R AArch32 processor. >>>>> >>>>> Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, >>>>> bits[31:24] = 0x41 , Arm Ltd >>>>> bits[23:20] = Implementation defined >>>>> bits[19:16] = 0xf , Arch features are individually identified >>>>> bits[15:4] = Implementation defined >>>>> bits[3:0] = Implementation defined >>>>> >>>>> Thus, the processor id is 0x410f0000 and the processor id mask is >>>>> 0xff0f0000 >>>>> >>>>> Also, there is no special initialization required for R52. >>>> >>>> Are you saying that Xen upstream + this patch will boot on Cortex-R52? >>> >>> This patch will help for earlyboot of Xen. With this patch, >>> cpu_init() will work on Cortex-R52. >>> >>> There will be changes required for the MPU configuration, but that >>> will be sent after Penny's patch serie "[PATCH v2 00/41] xen/arm: Add >>> Armv8-R64 MPU support to Xen - Part#1" is upstreamed. >>> >>> My aim is to extract the non-dependent changes and send them for review. >> >> I can review the patch. But I am not willing to merge it as it gives >> the false impression that Xen would boot on Cortex-R52. >> >> In fact, I think this patch should only be merged once we have all the >> MPU merged. >> >> IMHO, patches are independent are rework (e.g. code split...) that >> would help the MPU. > > Yes, that's exactly what I intend to do. I will send out the patches > (similar to this) which will not be impacted by the MPU changes. > > So, that both as an author/reviewer, it helps to restrict MPU serie to > only MPU specific changes. > > Can you suggest some change to the commit message, so that we can commit > it (without giving any false impression that Xen boots on Cortex-R52) > > May be adding this line to the commit message helps ? > > "However, Xen does not fully boot on Cortex-R52 as it requires MPU > support which is under review. > > Once Xen is supported on Cortex-R52, SUPPORT.md will be amended to > reflect it." While writing an answer for this patch, I was actually wondering whether the CPU allowlist still make sense for the 32-bit ARMv8-R? From Armv7-A, we needed it because some CPUs need specific quirk when booting. But it would be best if we can get rid of it for 32-bit ARMv8-R. Looking at your patch, your merely providing stubs. Do you have any plan to fill them up? Cheers,
On 22/06/2023 10:22, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 22/06/2023 09:59, Ayan Kumar Halder wrote: >> >> On 20/06/2023 21:43, Julien Grall wrote: >>> Hi Ayan, >> Hi Julien, >>> >>> On 20/06/2023 19:28, Ayan Kumar Halder wrote: >>>> >>>> On 20/06/2023 17:41, Julien Grall wrote: >>>>> Hi, >>>> Hi Julien, >>>>> >>>>> On 20/06/2023 16:17, Ayan Kumar Halder wrote: >>>>>> Add a special configuration (CONFIG_AARCH32_V8R) to setup the >>>>>> Cortex-R52 >>>>>> specifics. >>>>>> >>>>>> Cortex-R52 is an Arm-V8R AArch32 processor. >>>>>> >>>>>> Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, >>>>>> bits[31:24] = 0x41 , Arm Ltd >>>>>> bits[23:20] = Implementation defined >>>>>> bits[19:16] = 0xf , Arch features are individually identified >>>>>> bits[15:4] = Implementation defined >>>>>> bits[3:0] = Implementation defined >>>>>> >>>>>> Thus, the processor id is 0x410f0000 and the processor id mask is >>>>>> 0xff0f0000 >>>>>> >>>>>> Also, there is no special initialization required for R52. >>>>> >>>>> Are you saying that Xen upstream + this patch will boot on >>>>> Cortex-R52? >>>> >>>> This patch will help for earlyboot of Xen. With this patch, >>>> cpu_init() will work on Cortex-R52. >>>> >>>> There will be changes required for the MPU configuration, but that >>>> will be sent after Penny's patch serie "[PATCH v2 00/41] xen/arm: >>>> Add Armv8-R64 MPU support to Xen - Part#1" is upstreamed. >>>> >>>> My aim is to extract the non-dependent changes and send them for >>>> review. >>> >>> I can review the patch. But I am not willing to merge it as it gives >>> the false impression that Xen would boot on Cortex-R52. >>> >>> In fact, I think this patch should only be merged once we have all >>> the MPU merged. >>> >>> IMHO, patches are independent are rework (e.g. code split...) that >>> would help the MPU. >> >> Yes, that's exactly what I intend to do. I will send out the patches >> (similar to this) which will not be impacted by the MPU changes. >> >> So, that both as an author/reviewer, it helps to restrict MPU serie >> to only MPU specific changes. > >> Can you suggest some change to the commit message, so that we can >> commit it (without giving any false impression that Xen boots on >> Cortex-R52) > >> May be adding this line to the commit message helps ? > >> "However, Xen does not fully boot on Cortex-R52 as it requires MPU >> support which is under review. > >> Once Xen is supported on Cortex-R52, SUPPORT.md will be amended to >> reflect it." > > While writing an answer for this patch, I was actually wondering > whether the CPU allowlist still make sense for the 32-bit ARMv8-R? > > From Armv7-A, we needed it because some CPUs need specific quirk when > booting. But it would be best if we can get rid of it for 32-bit ARMv8-R. > > Looking at your patch, your merely providing stubs. Do you have any > plan to fill them up? Actually, I would use cr52_init in a later patch to write to CNTFRQ. See below :- +#define XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ 800000U + cr52_init: + /* + * Set counter frequency 800 KHZ + * + * Set counter frequency, CNTFRQ + */ + ldr r0,=XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ + mrc 15,0,r1,c14,c0,0 /* Read CNTFRQ */ + cmp r1,#0 + /* Set only if the frequency read is 0 */ + mcreq 15,0,r0,c14,c0,0 /* Write CNTFRQ */ + mov pc, lr - Ayan > > Cheers, >
Hi, On 22/06/2023 16:41, Ayan Kumar Halder wrote: > > On 22/06/2023 10:22, Julien Grall wrote: >> Hi Ayan, > Hi Julien, >> >> On 22/06/2023 09:59, Ayan Kumar Halder wrote: >>> >>> On 20/06/2023 21:43, Julien Grall wrote: >>>> Hi Ayan, >>> Hi Julien, >>>> >>>> On 20/06/2023 19:28, Ayan Kumar Halder wrote: >>>>> >>>>> On 20/06/2023 17:41, Julien Grall wrote: >>>>>> Hi, >>>>> Hi Julien, >>>>>> >>>>>> On 20/06/2023 16:17, Ayan Kumar Halder wrote: >>>>>>> Add a special configuration (CONFIG_AARCH32_V8R) to setup the >>>>>>> Cortex-R52 >>>>>>> specifics. >>>>>>> >>>>>>> Cortex-R52 is an Arm-V8R AArch32 processor. >>>>>>> >>>>>>> Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, >>>>>>> bits[31:24] = 0x41 , Arm Ltd >>>>>>> bits[23:20] = Implementation defined >>>>>>> bits[19:16] = 0xf , Arch features are individually identified >>>>>>> bits[15:4] = Implementation defined >>>>>>> bits[3:0] = Implementation defined >>>>>>> >>>>>>> Thus, the processor id is 0x410f0000 and the processor id mask is >>>>>>> 0xff0f0000 >>>>>>> >>>>>>> Also, there is no special initialization required for R52. >>>>>> >>>>>> Are you saying that Xen upstream + this patch will boot on >>>>>> Cortex-R52? >>>>> >>>>> This patch will help for earlyboot of Xen. With this patch, >>>>> cpu_init() will work on Cortex-R52. >>>>> >>>>> There will be changes required for the MPU configuration, but that >>>>> will be sent after Penny's patch serie "[PATCH v2 00/41] xen/arm: >>>>> Add Armv8-R64 MPU support to Xen - Part#1" is upstreamed. >>>>> >>>>> My aim is to extract the non-dependent changes and send them for >>>>> review. >>>> >>>> I can review the patch. But I am not willing to merge it as it gives >>>> the false impression that Xen would boot on Cortex-R52. >>>> >>>> In fact, I think this patch should only be merged once we have all >>>> the MPU merged. >>>> >>>> IMHO, patches are independent are rework (e.g. code split...) that >>>> would help the MPU. >>> >>> Yes, that's exactly what I intend to do. I will send out the patches >>> (similar to this) which will not be impacted by the MPU changes. >>> >>> So, that both as an author/reviewer, it helps to restrict MPU serie >>> to only MPU specific changes. > >>> Can you suggest some change to the commit message, so that we can >>> commit it (without giving any false impression that Xen boots on >>> Cortex-R52) > >>> May be adding this line to the commit message helps ? > >>> "However, Xen does not fully boot on Cortex-R52 as it requires MPU >>> support which is under review. > >>> Once Xen is supported on Cortex-R52, SUPPORT.md will be amended to >>> reflect it." >> >> While writing an answer for this patch, I was actually wondering >> whether the CPU allowlist still make sense for the 32-bit ARMv8-R? >> >> From Armv7-A, we needed it because some CPUs need specific quirk when >> booting. But it would be best if we can get rid of it for 32-bit ARMv8-R. >> >> Looking at your patch, your merely providing stubs. Do you have any >> plan to fill them up? > > Actually, I would use cr52_init in a later patch to write to CNTFRQ. See > below :- > > +#define XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ 800000U > + > cr52_init: > + /* > + * Set counter frequency 800 KHZ > + * > + * Set counter frequency, CNTFRQ > + */ > + ldr r0,=XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ > + mrc 15,0,r1,c14,c0,0 /* Read CNTFRQ */ > + cmp r1,#0 > + /* Set only if the frequency read is 0 */ > + mcreq 15,0,r0,c14,c0,0 /* Write CNTFRQ */ > + > mov pc, lr Why can't you use the device-tree (see clock-frequency) as all the other buggy platform does? Cheers,
On 22/06/2023 17:32, Julien Grall wrote: > Hi, Hi Julien, > > On 22/06/2023 16:41, Ayan Kumar Halder wrote: >> >> On 22/06/2023 10:22, Julien Grall wrote: >>> Hi Ayan, >> Hi Julien, >>> >>> On 22/06/2023 09:59, Ayan Kumar Halder wrote: >>>> >>>> On 20/06/2023 21:43, Julien Grall wrote: >>>>> Hi Ayan, >>>> Hi Julien, >>>>> >>>>> On 20/06/2023 19:28, Ayan Kumar Halder wrote: >>>>>> >>>>>> On 20/06/2023 17:41, Julien Grall wrote: >>>>>>> Hi, >>>>>> Hi Julien, >>>>>>> >>>>>>> On 20/06/2023 16:17, Ayan Kumar Halder wrote: >>>>>>>> Add a special configuration (CONFIG_AARCH32_V8R) to setup the >>>>>>>> Cortex-R52 >>>>>>>> specifics. >>>>>>>> >>>>>>>> Cortex-R52 is an Arm-V8R AArch32 processor. >>>>>>>> >>>>>>>> Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, >>>>>>>> bits[31:24] = 0x41 , Arm Ltd >>>>>>>> bits[23:20] = Implementation defined >>>>>>>> bits[19:16] = 0xf , Arch features are individually identified >>>>>>>> bits[15:4] = Implementation defined >>>>>>>> bits[3:0] = Implementation defined >>>>>>>> >>>>>>>> Thus, the processor id is 0x410f0000 and the processor id mask is >>>>>>>> 0xff0f0000 >>>>>>>> >>>>>>>> Also, there is no special initialization required for R52. >>>>>>> >>>>>>> Are you saying that Xen upstream + this patch will boot on >>>>>>> Cortex-R52? >>>>>> >>>>>> This patch will help for earlyboot of Xen. With this patch, >>>>>> cpu_init() will work on Cortex-R52. >>>>>> >>>>>> There will be changes required for the MPU configuration, but >>>>>> that will be sent after Penny's patch serie "[PATCH v2 00/41] >>>>>> xen/arm: Add Armv8-R64 MPU support to Xen - Part#1" is upstreamed. >>>>>> >>>>>> My aim is to extract the non-dependent changes and send them for >>>>>> review. >>>>> >>>>> I can review the patch. But I am not willing to merge it as it >>>>> gives the false impression that Xen would boot on Cortex-R52. >>>>> >>>>> In fact, I think this patch should only be merged once we have all >>>>> the MPU merged. >>>>> >>>>> IMHO, patches are independent are rework (e.g. code split...) that >>>>> would help the MPU. >>>> >>>> Yes, that's exactly what I intend to do. I will send out the >>>> patches (similar to this) which will not be impacted by the MPU >>>> changes. >>>> >>>> So, that both as an author/reviewer, it helps to restrict MPU serie >>>> to only MPU specific changes. > >>>> Can you suggest some change to the commit message, so that we can >>>> commit it (without giving any false impression that Xen boots on >>>> Cortex-R52) > >>>> May be adding this line to the commit message helps ? > >>>> "However, Xen does not fully boot on Cortex-R52 as it requires MPU >>>> support which is under review. > >>>> Once Xen is supported on Cortex-R52, SUPPORT.md will be amended to >>>> reflect it." >>> >>> While writing an answer for this patch, I was actually wondering >>> whether the CPU allowlist still make sense for the 32-bit ARMv8-R? >>> >>> From Armv7-A, we needed it because some CPUs need specific quirk >>> when booting. But it would be best if we can get rid of it for >>> 32-bit ARMv8-R. >>> >>> Looking at your patch, your merely providing stubs. Do you have any >>> plan to fill them up? >> >> Actually, I would use cr52_init in a later patch to write to CNTFRQ. >> See below :- >> >> +#define XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ 800000U >> + >> cr52_init: >> + /* >> + * Set counter frequency 800 KHZ >> + * >> + * Set counter frequency, CNTFRQ >> + */ >> + ldr r0,=XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ >> + mrc 15,0,r1,c14,c0,0 /* Read CNTFRQ */ >> + cmp r1,#0 >> + /* Set only if the frequency read is 0 */ >> + mcreq 15,0,r0,c14,c0,0 /* Write CNTFRQ */ >> + >> mov pc, lr > > Why can't you use the device-tree (see clock-frequency) as all the > other buggy platform does? That's a good suggestion :). Also, I can do this in the platform specific .init(). Then, can I add the empty stubs for R52 (similar to https://elixir.bootlin.com/linux/latest/source/arch/arm/mm/proc-v7m.S#L165 Cortex-M7, cpu_v7m_proc_init(), cpu_v7m_switch_mm() are stubs). Or do you propose something like this :- --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -322,7 +322,7 @@ cpu_init: PRINT("- Setting up control registers -\r\n") mov r5, lr /* r5 := return address */ - +#ifndef CONFIG_ARM_NO_PROC_INIT /* Get processor specific proc info into r1 */ bl __lookup_processor_type teq r1, #0 @@ -337,7 +337,7 @@ cpu_init: ldr r1, [r1, #PROCINFO_cpu_init] /* r1 := vaddr(init func) */ adr lr, cpu_init_done /* Save return address */ add pc, r1, r10 /* Call paddr(init func) */ - +#endif cpu_init_done: And we introduce this new bool Kconfig option "CONFIG_ARM_NO_PROC_INIT" which will be defined for processors that do not need any special init (eg R52). - Ayan > > Cheers, >
Hi Ayan, On 23/06/2023 14:43, Ayan Kumar Halder wrote: > > On 22/06/2023 17:32, Julien Grall wrote: >> Hi, > Hi Julien, >> >> On 22/06/2023 16:41, Ayan Kumar Halder wrote: >>> >>> On 22/06/2023 10:22, Julien Grall wrote: >>>> Hi Ayan, >>> Hi Julien, >>>> >>>> On 22/06/2023 09:59, Ayan Kumar Halder wrote: >>>>> >>>>> On 20/06/2023 21:43, Julien Grall wrote: >>>>>> Hi Ayan, >>>>> Hi Julien, >>>>>> >>>>>> On 20/06/2023 19:28, Ayan Kumar Halder wrote: >>>>>>> >>>>>>> On 20/06/2023 17:41, Julien Grall wrote: >>>>>>>> Hi, >>>>>>> Hi Julien, >>>>>>>> >>>>>>>> On 20/06/2023 16:17, Ayan Kumar Halder wrote: >>>>>>>>> Add a special configuration (CONFIG_AARCH32_V8R) to setup the >>>>>>>>> Cortex-R52 >>>>>>>>> specifics. >>>>>>>>> >>>>>>>>> Cortex-R52 is an Arm-V8R AArch32 processor. >>>>>>>>> >>>>>>>>> Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, >>>>>>>>> bits[31:24] = 0x41 , Arm Ltd >>>>>>>>> bits[23:20] = Implementation defined >>>>>>>>> bits[19:16] = 0xf , Arch features are individually identified >>>>>>>>> bits[15:4] = Implementation defined >>>>>>>>> bits[3:0] = Implementation defined >>>>>>>>> >>>>>>>>> Thus, the processor id is 0x410f0000 and the processor id mask is >>>>>>>>> 0xff0f0000 >>>>>>>>> >>>>>>>>> Also, there is no special initialization required for R52. >>>>>>>> >>>>>>>> Are you saying that Xen upstream + this patch will boot on >>>>>>>> Cortex-R52? >>>>>>> >>>>>>> This patch will help for earlyboot of Xen. With this patch, >>>>>>> cpu_init() will work on Cortex-R52. >>>>>>> >>>>>>> There will be changes required for the MPU configuration, but >>>>>>> that will be sent after Penny's patch serie "[PATCH v2 00/41] >>>>>>> xen/arm: Add Armv8-R64 MPU support to Xen - Part#1" is upstreamed. >>>>>>> >>>>>>> My aim is to extract the non-dependent changes and send them for >>>>>>> review. >>>>>> >>>>>> I can review the patch. But I am not willing to merge it as it >>>>>> gives the false impression that Xen would boot on Cortex-R52. >>>>>> >>>>>> In fact, I think this patch should only be merged once we have all >>>>>> the MPU merged. >>>>>> >>>>>> IMHO, patches are independent are rework (e.g. code split...) that >>>>>> would help the MPU. >>>>> >>>>> Yes, that's exactly what I intend to do. I will send out the >>>>> patches (similar to this) which will not be impacted by the MPU >>>>> changes. >>>>> >>>>> So, that both as an author/reviewer, it helps to restrict MPU serie >>>>> to only MPU specific changes. > >>>>> Can you suggest some change to the commit message, so that we can >>>>> commit it (without giving any false impression that Xen boots on >>>>> Cortex-R52) > >>>>> May be adding this line to the commit message helps ? > >>>>> "However, Xen does not fully boot on Cortex-R52 as it requires MPU >>>>> support which is under review. > >>>>> Once Xen is supported on Cortex-R52, SUPPORT.md will be amended to >>>>> reflect it." >>>> >>>> While writing an answer for this patch, I was actually wondering >>>> whether the CPU allowlist still make sense for the 32-bit ARMv8-R? >>>> >>>> From Armv7-A, we needed it because some CPUs need specific quirk >>>> when booting. But it would be best if we can get rid of it for >>>> 32-bit ARMv8-R. >>>> >>>> Looking at your patch, your merely providing stubs. Do you have any >>>> plan to fill them up? >>> >>> Actually, I would use cr52_init in a later patch to write to CNTFRQ. >>> See below :- >>> >>> +#define XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ 800000U >>> + >>> cr52_init: >>> + /* >>> + * Set counter frequency 800 KHZ >>> + * >>> + * Set counter frequency, CNTFRQ >>> + */ >>> + ldr r0,=XPAR_CPU_CORTEXR52_0_TIMESTAMP_CLK_FREQ >>> + mrc 15,0,r1,c14,c0,0 /* Read CNTFRQ */ >>> + cmp r1,#0 >>> + /* Set only if the frequency read is 0 */ >>> + mcreq 15,0,r0,c14,c0,0 /* Write CNTFRQ */ >>> + >>> mov pc, lr >> >> Why can't you use the device-tree (see clock-frequency) as all the >> other buggy platform does? > > That's a good suggestion :). Also, I can do this in the platform > specific .init(). I am not sure I understand why you are referring to .init() when you can simply add the property in the device-tree. We should not add new .init() if platform-agnostic way to do it. > > Then, can I add the empty stubs for R52 (similar to > https://elixir.bootlin.com/linux/latest/source/arch/arm/mm/proc-v7m.S#L165 Cortex-M7, cpu_v7m_proc_init(), cpu_v7m_switch_mm() are stubs). > > Or do you propose something like this :- > > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -322,7 +322,7 @@ cpu_init: > PRINT("- Setting up control registers -\r\n") > > mov r5, lr /* r5 := return address */ > - > +#ifndef CONFIG_ARM_NO_PROC_INIT > /* Get processor specific proc info into r1 */ > bl __lookup_processor_type > teq r1, #0 > @@ -337,7 +337,7 @@ cpu_init: > ldr r1, [r1, #PROCINFO_cpu_init] /* r1 := vaddr(init func) */ > adr lr, cpu_init_done /* Save return address */ > add pc, r1, r10 /* Call paddr(init func) */ > - > +#endif I think it would be best if you just #ifdef the fail below. So if the config selected, then you will still be able to have a Xen that can boot Cortex-A15 or a core that don't need _init. Note that for now, we should only select this new config for Armv8-R because there are some work to confirm it would be safe for us to boot Xen 32-bit Arm on any CPUs. I vaguely remember that we were making some assumptions on the cache type in the past. But maybe we other check in place to check such assumption. If this can be confirm (I am not ask you to do it, but you can) then we could even get rid of the #ifdef. Cheers,
Hi, On 23/06/2023 22:26, Julien Grall wrote: >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -322,7 +322,7 @@ cpu_init: >> PRINT("- Setting up control registers -\r\n") >> >> mov r5, lr /* r5 := return address */ >> - >> +#ifndef CONFIG_ARM_NO_PROC_INIT >> /* Get processor specific proc info into r1 */ >> bl __lookup_processor_type >> teq r1, #0 >> @@ -337,7 +337,7 @@ cpu_init: >> ldr r1, [r1, #PROCINFO_cpu_init] /* r1 := vaddr(init >> func) */ >> adr lr, cpu_init_done /* Save return address */ >> add pc, r1, r10 /* Call paddr(init func) */ >> - >> +#endif > > I think it would be best if you just #ifdef the fail below. So if the > config selected, then you will still be able to have a Xen that can boot > Cortex-A15 or a core that don't need _init. > > Note that for now, we should only select this new config for Armv8-R > because there are some work to confirm it would be safe for us to boot > Xen 32-bit Arm on any CPUs. I vaguely remember that we were making some > assumptions on the cache type in the past. But maybe we other check in > place to check such assumption. > > If this can be confirm (I am not ask you to do it, but you can) then we > could even get rid of the #ifdef. I had a look through the code. We have a check in the 32-bit version of setup_mm() for the instruction cache type. So I think it would be OK to relax the check in head.S. Bertrand, Stefano, what do you think? Cheers,
On 24/06/2023 08:04, Julien Grall wrote: > Hi, Hi Julien, > > On 23/06/2023 22:26, Julien Grall wrote: >>> --- a/xen/arch/arm/arm32/head.S >>> +++ b/xen/arch/arm/arm32/head.S >>> @@ -322,7 +322,7 @@ cpu_init: >>> PRINT("- Setting up control registers -\r\n") >>> >>> mov r5, lr /* r5 := return address */ >>> - >>> +#ifndef CONFIG_ARM_NO_PROC_INIT >>> /* Get processor specific proc info into r1 */ >>> bl __lookup_processor_type >>> teq r1, #0 >>> @@ -337,7 +337,7 @@ cpu_init: >>> ldr r1, [r1, #PROCINFO_cpu_init] /* r1 := vaddr(init >>> func) */ >>> adr lr, cpu_init_done /* Save return address */ >>> add pc, r1, r10 /* Call paddr(init >>> func) */ >>> - >>> +#endif >> >> I think it would be best if you just #ifdef the fail below. So if the >> config selected, then you will still be able to have a Xen that can >> boot Cortex-A15 or a core that don't need _init. >> >> Note that for now, we should only select this new config for Armv8-R >> because there are some work to confirm it would be safe for us to >> boot Xen 32-bit Arm on any CPUs. I vaguely remember that we were >> making some assumptions on the cache type in the past. But maybe we >> other check in place to check such assumption. >> >> If this can be confirm (I am not ask you to do it, but you can) then >> we could even get rid of the #ifdef. > > I had a look through the code. We have a check in the 32-bit version > of setup_mm() for the instruction cache type. So I think it would be > OK to relax the check in head.S. > > Bertrand, Stefano, what do you think? As per discussion, I have sent "[XEN v2] xen/arm: arm32: Allow Xen to boot on unidentified CPUs" with the comment addressed. - Ayan > > Cheers, >
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 61e581b8c2..c45753a2dd 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -3,6 +3,13 @@ config ARM_32 depends on "$(ARCH)" = "arm32" select ARCH_MAP_DOMAIN_PAGE +config AARCH32_V8R + bool "AArch32 Arm V8R Support (UNSUPPORTED)" if UNSUPPORTED + def_bool n + depends on ARM_32 + help + This option enables Armv8-R profile for AArch32. + config ARM_64 def_bool y depends on !ARM_32 diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile index 520fb42054..2ab808a7a8 100644 --- a/xen/arch/arm/arm32/Makefile +++ b/xen/arch/arm/arm32/Makefile @@ -8,6 +8,7 @@ obj-y += head.o obj-y += insn.o obj-$(CONFIG_LIVEPATCH) += livepatch.o obj-y += proc-v7.o proc-caxx.o +obj-$(CONFIG_AARCH32_V8R) += proc-v8.o obj-y += smpboot.o obj-y += traps.o obj-y += vfp.o diff --git a/xen/arch/arm/arm32/proc-v8.S b/xen/arch/arm/arm32/proc-v8.S new file mode 100644 index 0000000000..c5a566b165 --- /dev/null +++ b/xen/arch/arm/arm32/proc-v8.S @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * xen/arch/arm/arm32/proc-v8.S + * + * AArch32 V8R specific initialization + * + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved. + */ + +#include <asm/arm32/processor.h> +#include <asm/sysregs.h> + +cr52_init: + mov pc, lr + + .section ".proc.info", #alloc + .type __v8_cr52_proc_info, #object +__v8_cr52_proc_info: + .long 0x410F0000 /* Cortex-R52 */ + .long 0xFF0F0000 /* Mask */ + .long cr52_init + .size __v8_cr52_proc_info, . - __v8_cr52_proc_info + + .section ".proc.info", #alloc + .type __v8_cr52_proc_info, #object + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */
Add a special configuration (CONFIG_AARCH32_V8R) to setup the Cortex-R52 specifics. Cortex-R52 is an Arm-V8R AArch32 processor. Refer ARM DDI 0487I.a ID081822, G8-9647, G8.2.112 MIDR, bits[31:24] = 0x41 , Arm Ltd bits[23:20] = Implementation defined bits[19:16] = 0xf , Arch features are individually identified bits[15:4] = Implementation defined bits[3:0] = Implementation defined Thus, the processor id is 0x410f0000 and the processor id mask is 0xff0f0000 Also, there is no special initialization required for R52. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- xen/arch/arm/Kconfig | 7 +++++++ xen/arch/arm/arm32/Makefile | 1 + xen/arch/arm/arm32/proc-v8.S | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+) create mode 100644 xen/arch/arm/arm32/proc-v8.S