Skip to contentSkip to conversation

iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes

PATCH New Robin Murphy 2 participants 3 messages Oct 16, 2018 – Oct 17, 2018 IOMMU

lore
Share & tools
lore

Robin Murphy

author reviewer
#0 lore raw

PATCH 1/1 iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes

When we insert the sync sequence number into the CMD_SYNC.MSIData field, we do so in CPU-native byte order, before writing out the whole command as explicitly little-endian dwords. Thus on big-endian systems, the SMMU will receive and write back a byteswapped version of sync_nr, which would be perfect if it were targeting a similarly-little-endian ITS, but since it's actually writing back to memory being polled by the CPUs, they're going to end up seeing the wrong thing.

Since the SMMU doesn't care what the MSIData actually contains, the minimal-overhead solution is to simply add an extra byteswap initially, such that it then writes back the big-endian format directly.

Fixes 37de98f8f1cf ("iommu/arm-smmu-v3: Use CMD_SYNC completion MSI")
---
 drivers/iommu/arm-smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/arm-smmu-v3.c +1 -1
index db402e8b068b..867ba548c2cc 100644
@@ -952,7 +952,7 @@ static int __arm_smmu_cmdq_issue_sync_msi(struct arm_smmu_device *smmu)
 	struct arm_smmu_cmdq_ent ent = {
 		.opcode = CMDQ_OP_CMD_SYNC,
 		.sync	= {
-			.msiaddr = virt_to_phys(&smmu->sync_count),
+			.msiaddr = cpu_to_le32(virt_to_phys(&smmu->sync_count)),
 		},
 	};
 
Signature · 1 line
2.19.1.dirty
#1 lore raw

PATCH 1/1 iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes

Quoted 23 lines · Robin Murphy

When we insert the sync sequence number into the CMD_SYNC.MSIData field, we do so in CPU-native byte order, before writing out the whole command as explicitly little-endian dwords. Thus on big-endian systems, the SMMU will receive and write back a byteswapped version of sync_nr, which would be perfect if it were targeting a similarly-little-endian ITS, but since it's actually writing back to memory being polled by the CPUs, they're going to end up seeing the wrong thing.

Since the SMMU doesn't care what the MSIData actually contains, the minimal-overhead solution is to simply add an extra byteswap initially, such that it then writes back the big-endian format directly.

Fixes 37de98f8f1cf ("iommu/arm-smmu-v3: Use CMD_SYNC completion MSI")
---
 drivers/iommu/arm-smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/arm-smmu-v3.c +0 -0
index db402e8b068b..867ba548c2cc 100644

drivers/iommu/arm-smmu-v3.c static int __arm_smmu_cmdq_issue_sync_msi(struct arm_smmu_device *smmu) · line 955

Quoted 5 lines
drivers/iommu/arm-smmu-v3.c +1 -1
@@ -952,7 +952,7 @@ static int __arm_smmu_cmdq_issue_sync_msi(struct arm_smmu_device *smmu)
 	struct arm_smmu_cmdq_ent ent = {
 		.opcode = CMDQ_OP_CMD_SYNC,
 		.sync	= {
-			.msiaddr = virt_to_phys(&smmu->sync_count),
+			.msiaddr = cpu_to_le32(virt_to_phys(&smmu->sync_count)),

Hmm, aren't you swabbing the address here? Don't we want to swab the data? We also need a comment to explain this!

Will

author reviewer
#2 lore raw

PATCH 1/1 iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes

Quoted 19 lines · Will Deacon
Quoted 18 lines · Robin Murphy

When we insert the sync sequence number into the CMD_SYNC.MSIData field, we do so in CPU-native byte order, before writing out the whole command as explicitly little-endian dwords. Thus on big-endian systems, the SMMU will receive and write back a byteswapped version of sync_nr, which would be perfect if it were targeting a similarly-little-endian ITS, but since it's actually writing back to memory being polled by the CPUs, they're going to end up seeing the wrong thing.

Since the SMMU doesn't care what the MSIData actually contains, the minimal-overhead solution is to simply add an extra byteswap initially, such that it then writes back the big-endian format directly.

Fixes 37de98f8f1cf ("iommu/arm-smmu-v3: Use CMD_SYNC completion MSI")
---
  drivers/iommu/arm-smmu-v3.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 13 lines

drivers/iommu/arm-smmu-v3.c static int __arm_smmu_cmdq_issue_sync_msi(struct arm_smmu_device *smmu) · line 955

Quoted 10 lines
drivers/iommu/arm-smmu-v3.c +1 -1
index db402e8b068b..867ba548c2cc 100644
@@ -952,7 +952,7 @@ static int __arm_smmu_cmdq_issue_sync_msi(struct arm_smmu_device *smmu)
  	struct arm_smmu_cmdq_ent ent = {
  		.opcode = CMDQ_OP_CMD_SYNC,
  		.sync	= {
-			.msiaddr = virt_to_phys(&smmu->sync_count),
+			.msiaddr = cpu_to_le32(virt_to_phys(&smmu->sync_count)),

Hmm, aren't you swabbing the address here? Don't we want to swab the data? We also need a comment to explain this!

It's entirely possible that I'm a spectacular idiot. <sigh>

Let me try again...

Robin.