Re: [PATCH v2] iommu/arm-smmu-v3: Match Stall behaviour for S2

[Robin Murphy <robin.murphy@arm.com>]

On 15/08/2024 1:59 pm, Jason Gunthorpe wrote:
> On Thu, Aug 15, 2024 at 12:26:46PM +0000, Mostafa Saleh wrote:
>> Hi Robin,
>>
>> On Thu, Aug 15, 2024 at 01:16:19PM +0100, Robin Murphy wrote:
>>> On 15/08/2024 12:30 pm, Mostafa Saleh wrote:
>>>> Hi Jason,
>>>>
>>>> On Wed, Aug 14, 2024 at 12:51:51PM -0300, Jason Gunthorpe wrote:
>>>>> On Wed, Aug 14, 2024 at 02:56:33PM +0000, Mostafa Saleh wrote:
>>>>>
>>>>>> Also described in the pseudocode “SteIllegal()”
>>>>>>       if eff_idr0_stall_model == '10' && STE.S2S == '0' then
>>>>>>           // stall_model forcing stall, but S2S == 0
>>>>>>           return TRUE;
>>>>>
>>>>> This clips out an important bit:
>>>>>
>>>>> if STE.Config == '11x' then
>>>>>     [..]
>>>>>     if eff_idr0_stall_model == '10' && STE.S2S == '0' then
>>>>>         // stall_model forcing stall, but S2S == 0
>>>>>         return TRUE;
>>>>>
>>>>> And here we are using STRTAB_STE_0_CFG_S1_TRANS which is 101 and won't
>>>>> match the STE.Config qualification.
>>>>>
>>>>> The plain text language said the S2S is only required if the S2 is
>>>>> translating, STRTAB_STE_0_CFG_S1_TRANS puts it in bypass.
>>>>
>>>> Yes, my bad, this should be for stage-2 only which is populated in
>>>> arm_smmu_make_s2_domain_ste()
>>>>
>>>>>
>>>>>> +	/*
>>>>>> +	 * S2S is ignored if stage-2 exists but not enabled.
>>>>>> +	 * S2S is not compatible with ATS.
>>>>>> +	 */
>>>>>> +	if (master->stall_enabled && !ats_enabled &&
>>>>>> +	    smmu->features & ARM_SMMU_FEAT_TRANS_S2)
>>>>>> +		target->data[2] |= STRTAB_STE_2_S2S;
>>>>>
>>>>> We can't ignore ATS if it was requested here.
>>>
>>> I don't see much value in adding effectively-dead checks for something which
>>> is already forbidden by the architecture. The definition of STALL_MODEL
>>> explicitly states:
>>>
>>> "An SMMU associated with a PCI system must not have STALL_MODEL == 0b10".
>>>
>>
>> Ah, I was expecting that as otherwise it's contradiction, but couldn't
>> find it while searching. Thanks for pointing it out, I will drop all
>> references to ATS then.
> 
> I was thinking this was also protecting against buggy FW since
> stall_enable can be set by:
>      device_property_read_bool(dev, "dma-can-stall"))

If firmware goes out of its way to describe the system incorrectly then 
I'd consider that all bets are off, and there's little point in us 
trying to guess how to cope with a contradiction. For all we know, it 
could be that the stall property is in fact genuine and it's the ATS 
capability that was advertised erroneously.

> Alternatively we could directly prevent the clash even earlier:
> 
> @@ -3292,8 +3292,13 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>   
>          if ((smmu->features & ARM_SMMU_FEAT_STALLS &&
>               device_property_read_bool(dev, "dma-can-stall")) ||
> -           smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
> -               master->stall_enabled = true;
> +           smmu->features & ARM_SMMU_FEAT_STALL_FORCE) {
> +               if (!dev_is_pci(dev))
> +                       master->stall_enabled = true;
> +               else
> +                       dev_err(dev, FW_BUG
> +                               "A SMMUv3 is required to run in stall mode for a PCI device\n");

Unfortunately we can't do that, because there *are* RCiEP devices whose 
data interfaces are native AMBA, and thus for whom stalling is not 
actually a protocol violation as it would be on a real PCIe transport 
layer; correspondingly, it's *because* they are not true PCIe devices 
that they can't support ATS, and thus need stall support in order to do 
SVA, so things should still work out OK in practice.

> +           }
>   
>          if (dev_is_pci(dev)) {
> 
> Though I have no idea how the GPU driver that wants to use this
> works - it doesn't seem to be intree :\

It's not a GPU: drivers/crypto/hisilicon/zip/

Thanks,
Robin.