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 <[email protected]>
Based on the following commits from the Renesas BSP:
8fba83d97cca709a05139c38e29408e81ed4cf62
a8d93bc07da89a7fcf4d85f34d119a030310efa5
located at:
<snip>
Original commit messages:
commit 8fba83d97cca709a05139c38e29408e81ed4cf62
Author: Nam Nguyen <[email protected]>
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 <[email protected]>
commit a8d93bc07da89a7fcf4d85f34d119a030310efa5
Author: Nam Nguyen <[email protected]>
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 <[email protected]>
**********
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 <[email protected]>
---
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
