Hi Julien, On 04/10/2024 19:24, Julien Grall wrote: > > > 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.cherech...@nxp.com> >>>> >>>> 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?
Sure: 1. This is an SoC issue, present in a subset of the S32CC processors family. 2. VAs whose bits [48:41] are not all zero. 3. Well, TLB corruption. 4. There are instructions affected at both levels 1 and 2 of translation. The guest will have to work its own way around it. In Xen, just `tlbi vae2is` (only used in flush_xen_tlb_range_va()) seems to be affected, and if we go with the 2nd approach from the ones proposed, it should not be very ugly to implement. I am of course open to any of your suggestions. > > >> 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. I'm not sure what to say here, we would need to have the layout limited under the mentioned VA range in our case, right? What about the GVA layout? > >>>> 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, Thank you for the review and suggestions. Regards, Andrei Cherechesu
