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

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

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.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?

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