Re: [EXTERNAL] Re: [PATCH] iommu/arm-smmu-v3: Fix incorrect fields being passed to prefetch command

[Pratyush Yadav]

On 28/06/19 3:22 PM, Will Deacon wrote:
> On Fri, Jun 28, 2019 at 02:39:53PM +0530, Pratyush Yadav wrote:
>> According to the SMMUv3 spec [0] section 4.2.1, command 0x1
>> (CMD_PREFETCH_CONFIG) does not take address and size as parameters. It
>> only takes  StreamID, SSec, SubstreamID, and SSV. Address and Size are
>> parameters for command 0x2 (CMD_PREFETCH_ADDR).
>>
>> Tested on kernel 4.19 on TI J721E SOC.
>>
>> [0] https://static.docs.arm.com/ihi0070/a/IHI_0070A_SMMUv3.pdf
>>
>> Signed-off-by: Pratyush Yadav <p-yadav1@xxxxxx>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 4d5a694f02c2..2d4dfd909436 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -413,6 +413,7 @@ struct arm_smmu_cmdq_ent {
>>  	/* Command-specific fields */
>>  	union {
>>  		#define CMDQ_OP_PREFETCH_CFG	0x1
>> +		#define CMDQ_OP_PREFETCH_ADDR	0x2
>>  		struct {
>>  			u32			sid;
>>  			u8			size;
>> @@ -805,10 +806,12 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
>>  	case CMDQ_OP_TLBI_EL2_ALL:
>>  	case CMDQ_OP_TLBI_NSNH_ALL:
>>  		break;
>> -	case CMDQ_OP_PREFETCH_CFG:
>> -		cmd[0] |= FIELD_PREP(CMDQ_PREFETCH_0_SID, ent->prefetch.sid);
>> +	case CMDQ_OP_PREFETCH_ADDR:
>>  		cmd[1] |= FIELD_PREP(CMDQ_PREFETCH_1_SIZE, ent->prefetch.size);
>>  		cmd[1] |= ent->prefetch.addr & CMDQ_PREFETCH_1_ADDR_MASK;
>> +		/* Fallthrough */
>> +	case CMDQ_OP_PREFETCH_CFG:
>> +		cmd[0] |= FIELD_PREP(CMDQ_PREFETCH_0_SID, ent->prefetch.sid);
> 
> Hmm, but there's no issue here because the onus is on the caller not to
> initialise size and addr if they are using PREFETCH_CFG (and this is
> currently the case). Are you seeing a problem in practice?

Nope, no problems in practice. I just noticed this discrepancy when
working on something else.

The spec section 4.1.5 says that an implementation can either ignore the
reserved fields or raise CERROR_ILL. The SMMU in our board is kinder and
just ignores those fields, but some other implementations might not.

So if the caller does initialise size and addr, they would either not
take effect or would cause an CERROR_ILL. Either way, not the desired
behaviour.

> I'm happy to take a patch adding support for PREFETCH_ADDR, but we'd need
> a caller first.

I don't have a use case for PREFETCH_ADDR. So how about removing addr
and size for now? It would avoid silent (or not-silent, depending on the
SMMU) failures in the future. If someone wants, they can easily add them
back.

> Also -- fancy sending me a board? ;)

Just as soon as we get some spare ones :)

> Will
> 

-- 
Regards,
Pratyush Yadav