* [PATCH] iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes
@ 2018-10-16 11:31 ` Robin Murphy
0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2018-10-16 11:31 UTC (permalink / raw)
To: will.deacon-5wv7dgnIgG8, joro-zLv9SwRftAIdnm+yROfE0A
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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")
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
drivers/iommu/arm-smmu-v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index db402e8b068b..867ba548c2cc 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -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)),
},
};
--
2.19.1.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes
@ 2018-10-16 11:31 ` Robin Murphy
0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2018-10-16 11:31 UTC (permalink / raw)
To: linux-arm-kernel
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")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/iommu/arm-smmu-v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index db402e8b068b..867ba548c2cc 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -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)),
},
};
--
2.19.1.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes
2018-10-16 11:31 ` Robin Murphy
@ 2018-10-17 17:07 ` Will Deacon
-1 siblings, 0 replies; 6+ messages in thread
From: Will Deacon @ 2018-10-17 17:07 UTC (permalink / raw)
To: Robin Murphy
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Tue, Oct 16, 2018 at 12:31:07PM +0100, Robin Murphy wrote:
> 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")
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> drivers/iommu/arm-smmu-v3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index db402e8b068b..867ba548c2cc 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes
@ 2018-10-17 17:07 ` Will Deacon
0 siblings, 0 replies; 6+ messages in thread
From: Will Deacon @ 2018-10-17 17:07 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 16, 2018 at 12:31:07PM +0100, Robin Murphy wrote:
> 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")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/iommu/arm-smmu-v3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index db402e8b068b..867ba548c2cc 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes
2018-10-17 17:07 ` Will Deacon
@ 2018-10-17 17:10 ` Robin Murphy
-1 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2018-10-17 17:10 UTC (permalink / raw)
To: Will Deacon
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 17/10/18 18:07, Will Deacon wrote:
> On Tue, Oct 16, 2018 at 12:31:07PM +0100, Robin Murphy wrote:
>> 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")
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>> drivers/iommu/arm-smmu-v3.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index db402e8b068b..867ba548c2cc 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes
@ 2018-10-17 17:10 ` Robin Murphy
0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2018-10-17 17:10 UTC (permalink / raw)
To: linux-arm-kernel
On 17/10/18 18:07, Will Deacon wrote:
> On Tue, Oct 16, 2018 at 12:31:07PM +0100, Robin Murphy wrote:
>> 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")
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/iommu/arm-smmu-v3.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index db402e8b068b..867ba548c2cc 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-17 17:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-16 11:31 [PATCH] iommu/arm-smmu-v3: Fix big-endian CMD_SYNC writes Robin Murphy
2018-10-16 11:31 ` Robin Murphy
[not found] ` <e14feb6f7dedc90d181e20fbeaaba49156b03b09.1539689371.git.robin.murphy-5wv7dgnIgG8@public.gmane.org>
2018-10-17 17:07 ` Will Deacon
2018-10-17 17:07 ` Will Deacon
[not found] ` <20181017170712.GA4570-5wv7dgnIgG8@public.gmane.org>
2018-10-17 17:10 ` Robin Murphy
2018-10-17 17:10 ` Robin Murphy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.