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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Oleksandr,

Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> writes:

> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> Based on the following commits from the Renesas BSP:
> 8fba83d97cca709a05139c38e29408e81ed4cf62
> a8d93bc07da89a7fcf4d85f34d119a030310efa5
> located at:
> https://urldefense.com/v3/__https://github.com/renesas-rcar/linux-bsp/tree/v5.10.41/rcar-5.1.3.rc5__;!!GF_29dbcQIUBPA!mB3ScUYdbD0s4mYzmb1Wu61fm6lRM1RhcvULXNjedfRRx0XhTk4HshhraUhZ3FRwxzSFY2I$
>  [github[.]com]
>
> 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.
>
> 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.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@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)
>  
> @@ -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);
> +
>      spin_lock(&ipmmu_devices_lock);
>      list_add(&mmu->list, &ipmmu_devices);
>      spin_unlock(&ipmmu_devices_lock);


-- 
Volodymyr Babchuk at EPAM