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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hello Oleksandr-san,

Thank you for the patch!

> From: Oleksandr Tyshchenko, Sent: Sunday, November 28, 2021 2:52 AM
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> 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@renesas.com>
>  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@renesas.com>
> 
>  commit a8d93bc07da89a7fcf4d85f34d119a030310efa5
>  Author: Nam Nguyen <nam.nguyen.yh@renesas.com>
>  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@renesas.com>
> 
> **********
> 
> 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.

> 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.
So, we have to add a new member (maybe "control_offset_base" is a good name?)
to calculate the address.

> 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.

> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  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.

> @@ -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.

Best regards,
Yoshihiro Shimoda