[PATCH] irqchip/gicv3-its: Add workaround for HIP09/HIP10/HIP10C erratum 162100803/162200807/162400807

[Zhou Wang]

On 2025/9/15 20:23, Marc Zyngier wrote:
> On Wed, 10 Sep 2025 04:27:15 +0100,
> Zhou Wang <wangzhou1 at hisilicon.com> wrote:
>>

[...]

>>>
>>> You are telling the ITS that it is allowed to go and access
>>> unspecified data (16 bit worth of translations). That's not
>>> acceptable. If you *have* to do that, then override the size in the
>>> driver code to actually allocate the corresponding memory.
>>
>> Then we have to override the ITT memory to 2 ^ 16 * 8 for one device
>> :(
> 
> But what's the alternative? Letting the ITS speculate in random
> memory, possibly under the control of userspace or a guest? I don't
> think that's acceptable.

Seems that only a bad device who could send an interrupt which is not be
declared may bring a problem here.

Not sure how dose userspace or a guest make a problem here?

> 
> [...]
> 
>>>> +	/*
>>>> +	 * Cache invalidate by a private register GITS_FUNC_EN, whose offset
>>>> +	 * is 0x20080 of ITS base address. GICv4.1 already maps sgir_base
>>>> +	 * (offset is 0x20000), so address of GITS_FUNC_EN can be got by
>>>> +	 * sgir_base + 0x80. Bit 16 is used to clear DT cache, the flip of
>>>> +	 * bit 19 indicates that DT cache has been cleared.
>>>> +	 */
>>>> +	while (--i) {
>>>> +		tmp = readl_relaxed(its_func_en) & mask;
>>>> +		writel_relaxed(tmp | (1 << 16), its_func_en);
>>>> +		tmp1 = readl_relaxed(its_func_en) & mask;
>>>> +		if (tmp != tmp1)
>>>> +			break;
>>>> +	}
>>>
>>> Please define these bits so that they have names instead of using raw
>>> values. Why do you need to write bit 16 every time? Surely once you
>>> have written it *once*, the HW remember what it has been told to do?
>>
>> HW will not remember it, so we have to write bit 16 every time.
>>
>>>
>>> And frankly, using readl_relaxed_poll_timeout_atomic should be the
>>> right thing to do.
>>
>> The HW logic is that write bit 16 will trigger DT cache clear in one HW cycle
>> or not. NOT that waiting bit 19 to flip.
> 
> Huh. This sounds crazy... Please document this.

OK.

> 
> [...]
> 
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_162100803
>>>> +	{
>>>> +		.desc = "ITS: Hip09 erratum 162100803",
>>>> +		.iidr = 0x00051736,
>>>> +		.mask = 0xffffffff,
>>>> +		.init = its_enable_quirk_162100803,
>>>> +	},
>>>> +	{
>>>> +		.desc = "ITS: Hip10 erratum 162200807",
>>>> +		.iidr = 0x01051736,
>>>> +		.mask = 0xffffffff,
>>>> +		.init = its_enable_quirk_162100803,
>>>> +	},
>>>> +	{
>>>> +		.desc = "ITS: Hip10c erratum 162400807",
>>>> +		.iidr = 0x00061736,
>>>> +		.mask = 0xffffffff,
>>>> +		.init = its_enable_quirk_162100803,
>>>
>>> Can you try to merge these three entries and adjust the mask
>>> accordingly? mask=0xeffcffff should do the trick, assuming that you
>>> don't have any other hardware overlapping this.
>>
>> The iidrs are different, how to merge these entries?
> 
> By using the mask:
> 
> 	{
> 		.desc = "ITS: Hip10 erratum, will eat your vSGIs",
> 		.iidr = 0x01051736,
> 		.mask = 0xeffcffff,
> 		.init = its_enable_quirk_162100803,
> 	},
> 
> will match all three platforms, assuming I got the mask correctly.

iidrs are differet, so one iidr can not match all. e.g.
for iidr = 0x00061736, mask with 0xeffcffff will be 0x00041736, which
will not be 0x01051736.

>  
>>
>>>
>>>> +	},
>>>> +#endif
>>>>  #ifdef CONFIG_ROCKCHIP_ERRATUM_3588001
>>>>  	{
>>>>  		.desc   = "ITS: Rockchip erratum RK3588001",
>>>
>>> If the HW is this bad, and that this only affects virtual interrupts,
>>> why don't you simply disable GICv4 support? Messing with the internals
>>> of the ITS tables feels like a pretty bad idea. Or as I suggested
>>> above, get the firmware to ignore RAS events from the ITS.
>>
>> The hack is ugly, but we want to try to save vSGI direct
>> injection...
> 
> Well, if you think this is worth it...
> 
> 	M.
>