Re: [PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Julien Grall <julien@xxxxxxx>]

On 04/10/2024 16:37, Andrei Cherechesu wrote:
> Hi Julien, Stefano,
Hi Andrei,

> On 01/10/2024 13:01, Julien Grall wrote:
> > Hi Andrei,
> >
> > On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> > > From: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
> > >
> > > Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).
> > >
> > > S32CC platforms use the NXP LINFlexD UART driver for
> > > console by default, and rely on Dom0 having access to
> > > SCMI services for system-level resources from firmware
> > > at EL3.
> > >
> > > Platform-related quirks will be addressed in following
> > > commits.
> > I don't see any specific quirks in the series. Are you intended to send 
> > follow-up?
> >
> > [...]
> Well, we could use an opinion on some SoC erratum implementation
> and if it would be acceptable for you or not, from any of
> the ARM maintainers.
>
> The erratum is about some TLB Invalidate (tlbi) instruction
> variants (by Virtual Address) which are handled incorrectly
> in some cases (for some VAs), and that need to be replaced
> with broader versions.
Can you provide more details:
  1. Is this a processor issue?
  2. Which VAs are affected?
  3. What is the impact if the erratum is hit?
  3. Do we need to do anything on the behalf of the guests?

> It doesn't apply for all S32CC platforms, but we can use the
> Host DT compatible to identify this. My idea is that we define
> a platform quirk for this and check for it using
> platform_has_quirk().
> Then, we either:>      - modify the definition for the affected 
'tlbi' variant
>      in arm64/flushtlb.h to actually execute the broader one
>      if needed
> or:
>      - define a new wrapper for the tlbi variant we want to
>      replace the affected one with
>      - check for this quirk before its usage and call the
>      more potent version if needed (there's only one
>      occurrence of usage for the affected version).
>
> Number 2 seems better to me, it's more customizable and
> it's similar to the existing handling for
> PLATFORM_QUIRK_GIC_64K_STRIDE for some other Arm platform.
I need a bit more details first (see my questions above). But if there 
are not many VAs affected, then we may be able to re-order the Xen 
memory layout to avoid the VAs. So we wouldn't need any specific change 
in the TLB flush operations.
> > > diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> > > index bec6e55d1f..2c304b964d 100644
> > > --- a/xen/arch/arm/platforms/Makefile
> > > +++ b/xen/arch/arm/platforms/Makefile
> > > @@ -10,5 +10,6 @@ obj-$(CONFIG_ALL64_PLAT) += thunderx.o
> > >    obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
> > >    obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
> > >    obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
> > > +obj-$(CONFIG_S32CC_PLATFORM)  += s32cc.o
> > >    obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
> > >    obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
> > > diff --git a/xen/arch/arm/platforms/s32cc.c b/xen/arch/arm/platforms/s32cc.c
> > > new file mode 100644
> > > index 0000000000..f99a2d56f6
> > > --- /dev/null
> > > +++ b/xen/arch/arm/platforms/s32cc.c
> > We only add a new platform if there are platform specific hook to implement. 
> > AFAICT, you don't implement any, so it should not be necessary.
> Like I mentioned above, there's some erratum workaround which
> could make use of the platform_quirk() callback, that we want
> to send in the near future.
I think it would be best to introduce platforms/s32cc.c at that point.

Cheers,

--
Julien Grall