Re: [PATCH v3 6/7] hw/arm/smmuv3: Fix SMMU_CMD_CFGI_STE_RANGE handling

[Zenghui Yu]

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.auger@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@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;