Hi Zenghui,
On 3/25/21 3:18 PM, Zenghui Yu wrote:
> On 2021/3/9 18:27, Eric Auger wrote:
>> If the whole SID range (32b) is invalidated (SMMU_CMD_CFGI_ALL),
>> @end overflows and we fail to handle the command properly.
>>
>> Once this gets fixed, the current code really is awkward in the
>> sense it loops over the whole range instead of removing the
>> currently cached configs through a hash table lookup.
>>
>> Fix both the overflow and the lookup.
>>
>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
>
> Not much to do with this patch, but maybe we can take the fix a little
> further. We now take StreamID as the start of the invalidation range,
> regardless of whatever the Range is, whilst the spec clearly states that
> "the StreamID parameter (of *CMD_CFGI_ALL* command) is IGNORED". If
> there are some random bits in the StreamID field (who knows), we'll fail
> to perform the full invalidation but get a strange range (e.g.,
> SMMUSIDRange={.start=1, .end=0}) instead.
>
> And having looked at the spec again, 4.3.2 CMD_CFGI_STE_RANGE:
>
> - "Invalidation is performed for an *aligned* range of 2^(Range+1)
> StreamIDs."
>
> - "The bottom Range+1 bits of the StreamID parameter are IGNORED,
> aligning the range to its size."
>
> which seems to be some bits that we had never taken into account. And
> what I'm saying is roughly something like below (compile tested), any
> thoughts?
>
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 3b87324ce2..8705612535 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -980,16 +980,20 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> }
> case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL */
> {
> - uint32_t start = CMD_SID(&cmd);
> + uint32_t sid = CMD_SID(&cmd), mask;
> uint8_t range = CMD_STE_RANGE(&cmd);
> - uint64_t end = start + (1ULL << (range + 1)) - 1;
> - SMMUSIDRange sid_range = {start, end};
> + SMMUSIDRange sid_range;
>
> if (CMD_SSEC(&cmd)) {
> cmd_error = SMMU_CERROR_ILL;
> break;
> }
> - trace_smmuv3_cmdq_cfgi_ste_range(start, end);
> +
> + mask = (1ULL << (range + 1)) - 1;
> + sid_range.start = sid & ~mask;
> + sid_range.end = sid_range.start + mask;
> +
> + trace_smmuv3_cmdq_cfgi_ste_range(sid_range.start,
> sid_range.end);
> g_hash_table_foreach_remove(bs->configs,
> smmuv3_invalidate_ste,
> &sid_range);
> break;
>
Thanks for spotting this discrepancy with the spec. This looks good to
me, please feel free to then the patch.
Thanks
Eric
