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

[Oleksandr <olekstysh@gmail.com>]

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

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

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