[V2,08/10] iommu/ipmmu-vmsa: Set IPMMU bit IMSCTLR_USE_SECGRP to 0

Commit Message

Oleksandr Tyshchenko Dec. 20, 2021, 9:15 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

Based on the following commits from the Renesas BSP:
8fba83d97cca709a05139c38e29408e81ed4cf62
a8d93bc07da89a7fcf4d85f34d119a030310efa5
located at:
https://github.com/renesas-rcar/linux-bsp/tree/v5.10.41/rcar-5.1.3.rc5

Original commit messages:
 commit 8fba83d97cca709a05139c38e29408e81ed4cf62
 Author: Nam Nguyen <[e-mail]>
 Date:   Wed Apr 28 18:54:44 2021 +0700

  iommu/ipmmu-vmsa: Set IPMMU bit IMSCTLR_USE_SECGRP to 0

  Need to set bit IMSCTLR_USE_SECGRP to 0
  because H/W initial value is unknown, without this
  dma-transfer cannot be done due to address translation doesn't work.

  Signed-off-by: Nam Nguyen <[e-mail]>

 commit a8d93bc07da89a7fcf4d85f34d119a030310efa5
 Author: Nam Nguyen <[e-mail]>
 Date:   Tue Sep 7 14:46:12 2021 +0700

  iommu/ipmmu-vmsa: Update IMSCTLR register offset address for R-Car S4

  Update IMSCTLR register offset address to align with R-Car S4 H/W UM.

  Signed-off-by: Nam Nguyen <[e-mail]>

**********

It is still a question whether this really needs to be done in Xen,
rather in firmware, but better to be on the safe side. After all,
if firmware already takes care of clearing this bit, nothing bad
will happen.

Please note the following:
1. I decided to squash both commits since the first commit adds clearing
code and only the second one makes it functional on S4. Moreover, this is
not a direct port. So it would be better to introduce complete solution
by a single patch.
2. Although patch indeed does what it claims in the subject,
the implementation is different in comparison with original changes.
On Linux the clearing is done at runtime in ipmmu_domain_setup_context().
On Xen the clearing is done at boot time in ipmmu_probe().
The IMSCTLR is not a MMU "context" register at all, so I think there is
no point in performing the clearing each time we initialize the context,
instead perform the clearing at once during initialization. Also do not
abuse ctx_offset_stride_adj field for the register's offset calculation,
instead use recently added control_offset_base field.

Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
Reviewed-by: Volodymyr Babchuk <[e-mail]>
---
Changes V1 -> V2:
   - add R-b
   - update patch description
   - replace ctx_offset_stride_adj with control_offset_base for
     register's offset calculation
---
 xen/drivers/passthrough/arm/ipmmu-vmsa.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Yoshihiro Shimoda Dec. 29, 2021, 12:53 a.m. UTC | #1

Hello Oleksandr-san,

> From: Oleksandr Tyshchenko, Sent: Tuesday, December 21, 2021 6:16 AM
> 
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> Based on the following commits from the Renesas BSP:
> 8fba83d97cca709a05139c38e29408e81ed4cf62
> a8d93bc07da89a7fcf4d85f34d119a030310efa5
> located at:
<snip>
> 
> Original commit messages:
>  commit 8fba83d97cca709a05139c38e29408e81ed4cf62
>  Author: Nam Nguyen <[e-mail]>
>  Date:   Wed Apr 28 18:54:44 2021 +0700
> 
>   iommu/ipmmu-vmsa: Set IPMMU bit IMSCTLR_USE_SECGRP to 0
> 
>   Need to set bit IMSCTLR_USE_SECGRP to 0
>   because H/W initial value is unknown, without this
>   dma-transfer cannot be done due to address translation doesn't work.
> 
>   Signed-off-by: Nam Nguyen <[e-mail]>
> 
>  commit a8d93bc07da89a7fcf4d85f34d119a030310efa5
>  Author: Nam Nguyen <[e-mail]>
>  Date:   Tue Sep 7 14:46:12 2021 +0700
> 
>   iommu/ipmmu-vmsa: Update IMSCTLR register offset address for R-Car S4
> 
>   Update IMSCTLR register offset address to align with R-Car S4 H/W UM.
> 
>   Signed-off-by: Nam Nguyen <[e-mail]>
> 
> **********
> 
> It is still a question whether this really needs to be done in Xen,
> rather in firmware, but better to be on the safe side. After all,
> if firmware already takes care of clearing this bit, nothing bad
> will happen.
> 
> Please note the following:
> 1. I decided to squash both commits since the first commit adds clearing
> code and only the second one makes it functional on S4. Moreover, this is
> not a direct port. So it would be better to introduce complete solution
> by a single patch.
> 2. Although patch indeed does what it claims in the subject,
> the implementation is different in comparison with original changes.
> On Linux the clearing is done at runtime in ipmmu_domain_setup_context().
> On Xen the clearing is done at boot time in ipmmu_probe().
> The IMSCTLR is not a MMU "context" register at all, so I think there is
> no point in performing the clearing each time we initialize the context,
> instead perform the clearing at once during initialization. Also do not
> abuse ctx_offset_stride_adj field for the register's offset calculation,
> instead use recently added control_offset_base field.
> 
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> Reviewed-by: Volodymyr Babchuk <[e-mail]>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <[e-mail]>

Best regards,
Yoshihiro Shimoda

Patch

diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index 14848ce..649b9f6 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -222,6 +222,9 @@  static DEFINE_SPINLOCK(ipmmu_devices_lock);
 #define IMUASID0(n)            (0x0308 + ((n) * 16))
 #define IMUASID32(n)           (0x0608 + (((n) - 32) * 16))
 
+#define IMSCTLR             0x0500
+#define IMSCTLR_USE_SECGRP  (1 << 28)
+
 #define IMSAUXCTLR          0x0504
 #define IMSAUXCTLR_S2PTE    (1 << 3)
 
@@ -966,6 +969,10 @@  static int ipmmu_probe(struct dt_device_node *node)
         set_bit(0, mmu->ctx);
     }
 
+    /* Do not use security group function. */
+    reg = IMSCTLR + mmu->features->control_offset_base;
+    ipmmu_write(mmu, reg, ipmmu_read(mmu, reg) & ~IMSCTLR_USE_SECGRP);
+
     spin_lock(&ipmmu_devices_lock);
     list_add(&mmu->list, &ipmmu_devices);
     spin_unlock(&ipmmu_devices_lock);