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

Commit Message

Oleksandr Tyshchenko Nov. 27, 2021, 5:51 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.

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

Comments

Volodymyr Babchuk Dec. 15, 2021, 2:44 a.m. UTC | #1

Hi Oleksandr,

Oleksandr Tyshchenko <[e-mail]> writes:

> From: Oleksandr Tyshchenko <[e-mail]>
>
> 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 <[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.
>
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>

Reviewed-by: Volodymyr Babchuk <[e-mail]>

> ---
>  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);

Yoshihiro Shimoda Dec. 16, 2021, 12:48 p.m. UTC | #2

Hello Oleksandr-san,

Thank you for the patch!

> From: Oleksandr Tyshchenko, Sent: Sunday, November 28, 2021 2:52 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.

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 <[e-mail]>
> ---
>  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

Oleksandr Tyshchenko Dec. 16, 2021, 4:45 p.m. UTC | #3

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 <[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.
> 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 <[e-mail]>
>> ---
>>   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
>

Patch

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);