[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




On 16.12.21 14:48, Yoshihiro Shimoda wrote:
Hello Oleksandr-san,

Hello Shimoda-san,



Thank you for the patch!

Thank you for the review!



From: Oleksandr Tyshchenko, Sent: Sunday, November 28, 2021 2:52 AM

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

Based on the following commits from the Renesas BSP:
8fba83d97cca709a05139c38e29408e81ed4cf62
a8d93bc07da89a7fcf4d85f34d119a030310efa5
located at:
<snip>
Original commit messages:
  commit 8fba83d97cca709a05139c38e29408e81ed4cf62
  Author: Nam Nguyen <nam.nguyen.yh@xxxxxxxxxxx>
  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 <nam.nguyen.yh@xxxxxxxxxxx>

  commit a8d93bc07da89a7fcf4d85f34d119a030310efa5
  Author: Nam Nguyen <nam.nguyen.yh@xxxxxxxxxxx>
  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 <nam.nguyen.yh@xxxxxxxxxxx>

**********

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.
IIUC, we need this for IPMMU-DS0.

Yes, we have confirmed that by running dmatest app over SYS-DMAC1/2 channels in the Guest on S4 w/ and w/o current patch.
So this clearing is definitely needed.



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.
I agree.
However, I realized IMSCTLR and IMSAUXCTLR registers' offset differs
between Gen3 and Gen4. About BSP patch of 07/10, it seems to take care
of the offset. But, Linux upstream based code doesn't take care of it.


Yes, I assume this is because Linux upstream driver doesn't support S4 yet, so there is no need to clear the USE_SECGRP bit in IMSCTLR so far and Linux upstream driver doesn't use stage2 translation table format, so there is no need to set the S2PTE bit in IMSAUXCTLR.


So, we have to add a new member (maybe "control_offset_base" is a good name?)
to calculate the address.

Agree here, control_offset_base sounds perfectly fine to me, will do. I already had to diverge from Linux in 07/10 patch by introducing imuctr_ttsel_mask member (which is (15 << 4) on Gen3 and (31 << 4) on S4 due to the larger number of context supported by the latter) as Xen driver has an additional hardening code in ipmmu_utlb_enable().



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.
ipmmu_domain_setup_context() is called in probing and resuming.
So, it's enough to clear in ipmmu_probe() I think.

great, thank you for confirming.



Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
  xen/drivers/passthrough/arm/ipmmu-vmsa.c | 7 +++++++
  1 file changed, 7 insertions(+)

diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index 8dfdae8..22dd84e 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -229,6 +229,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)
As I mentioned above, we have to adjust these registers' offset for
both Gen3 (+0) and Gen4 (+0x1000) somehow.

Yes, I will take care of it.



@@ -979,6 +982,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->ctx_offset_stride_adj;
+    ipmmu_write(mmu, reg, ipmmu_read(mmu, reg) & ~IMSCTLR_USE_SECGRP);
If we modify the 07/10 patch, we cannot use ctx_offset_stride_adj.
# But, using "ctx_offset" here seems to be abused though because
# the register is not context.

I agree, it is an abuse. I believe, this will be solved by your suggestion to introduce control_offset_base member with proper values for Gen3 and S4, will do.



Best regards,
Yoshihiro Shimoda

--
Regards,

Oleksandr Tyshchenko




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.