Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver

[Julien Grall <julien@xen.org>]

On 21/01/2021 17:18, Rahul Singh wrote:
> Hello Oleksandr,

Hi,

>> On 20 Jan 2021, at 9:33 pm, Oleksandr <olekstysh@gmail.com> wrote:
>>
>>
>> On 20.01.21 16:52, Rahul Singh wrote:
>>
>> Hi Rahul
>>
>>> Add support for ARM architected SMMUv3 implementation. It is based on
>>> the Linux SMMUv3 driver.
>>>
>>> Driver is currently supported as Tech Preview.
>>>
>>> Major differences with regard to Linux driver are as follows:
>>> 2. Only Stage-2 translation is supported as compared to the Linux driver
>>>     that supports both Stage-1 and Stage-2 translations.
>>> 3. Use P2M  page table instead of creating one as SMMUv3 has the
>>>     capability to share the page tables with the CPU.
>>> 4. Tasklets are used in place of threaded IRQ's in Linux for event queue
>>>     and priority queue IRQ handling.
>>> 5. Latest version of the Linux SMMUv3 code implements the commands queue
>>>     access functions based on atomic operations implemented in Linux.
>>>     Atomic functions used by the commands queue access functions are not
>>>     implemented in XEN therefore we decided to port the earlier version
>>>     of the code. Atomic operations are introduced to fix the bottleneck
>>>     of the SMMU command queue insertion operation. A new algorithm for
>>>     inserting commands into the queue is introduced, which is lock-free
>>>     on the fast-path.
>>>     Consequence of reverting the patch is that the command queue
>>>     insertion will be slow for large systems as spinlock will be used to
>>>     serializes accesses from all CPUs to the single queue supported by
>>>     the hardware. Once the proper atomic operations will be available in
>>>     XEN the driver can be updated.
>>> 6. Spin lock is used in place of mutex when attaching a device to the
>>>     SMMU, as there is no blocking locks implementation available in XEN.
>>>     This might introduce latency in XEN. Need to investigate before
>>>     driver is out for tech preview.
>>> 7. PCI ATS functionality is not supported, as there is no support
>>>     available in XEN to test the functionality. Code is not tested and
>>>     compiled. Code is guarded by the flag CONFIG_PCI_ATS.
>>> 8. MSI interrupts are not supported as there is no support available in
>>>     XEN to request MSI interrupts. Code is not tested and compiled. Code
>>>     is guarded by the flag CONFIG_MSI.
>>>
>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>>> ---
>>> Changes since v2:
>>> - added return statement for readx_poll_timeout function.
>>> - remove iommu_get_dma_cookie and iommu_put_dma_cookie.
>>> - remove struct arm_smmu_xen_device as not required.
>>> - move dt_property_match_string to device_tree.c file.
>>> - replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion.
>>> - use ARM_SMMU_REG_SZ as size when map memory to XEN.
>>> - remove bypass keyword to make sure when device-tree probe is failed we
>>>    are reporting error and not continuing to configure SMMU in bypass
>>>    mode.
>>> - fixed minor comments.
>>> Changes since v3:
>>> - Fixed typo for CONFIG_MSI
>>> - Added back the mutex code
>>> - Rebase the patch on top of newly added WARN_ON().
>>> - Remove the direct read of register VTCR_EL2.
>>> - Fixed minor comments.
>>> Changes since v4:
>>> - Replace the ffsll() with ffs64() function.
>>> - Add code to free resources when probe failed.
>>
>> Thank you for addressing, patch looks ok to me, just one small remark below:
>>
>>
>>> +
>>> +static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
>>> +{
>>> +}
>>
>> We discussed in V4 about adding some code here which all IOMMUs on Arm already have, copy it below for the convenience:
>>
>>
>>       /* Set to false options not supported on ARM. */
>>       if ( iommu_hwdom_inclusive )
>>           printk(XENLOG_WARNING
>>           "map-inclusive dom0-iommu option is not supported on ARM\n");
>>       iommu_hwdom_inclusive = false;
>>       if ( iommu_hwdom_reserved == 1 )
>>           printk(XENLOG_WARNING
>>           "map-reserved dom0-iommu option is not supported on ARM\n");
>>       iommu_hwdom_reserved = 0;
>>
>>       arch_iommu_hwdom_init(d);
>>
>>
>> Also we discussed about possibility to fold the part of code (which disables unsupported options) in arch_iommu_hwdom_init() to avoid duplication as a follow-up.
>> At least, I expected to see arch_iommu_hwdom_init() to be called by arm_smmu_iommu_hwdom_init() it current patch... Please note, this is *not* a request for change immediately,
>> I think, driver is functional even without this code (hopefully arch_iommu_hwdom_init is empty now, etc).  But, if you happen to do V6 or probably it could be done on commit ...
>>
> 
> Yes I will send the patch to move the code to arch_iommu_hwdom_init() to avoid duplication once SMMUv3 driver will be merged.
> I thought anyway I have to remove the code from SMMUv1 and IPMMU I will take care of all the IOMMU driver at the same time because of that I didn’t modify the SMMUv3 driver.

There are two part in the problem here:
   1) Code duplication
   2) The SMMUv3 not checking the command line and calling 
arch_iommu_hwdom_init(d)

I agree that 1) can be deferred because it is a clean-up. However, I 
consider 2) a (latent) bug because it means that we risk unintentionally 
breaking the SMMUv3 driver is we need to add code in 
arch_iommu_hwdom_init() as part of a future bug fix for 4.15.

Therefore...

> Yes if there is a need for v6 I will add the arch_iommu_hwdom_init(d) in arm_smmu_iommu_hwdom_init().

... I think calling arch_iommu_hwdom_init() should be the strict 
minimum. So please address it. Although, no need to resend the full 
series, you can only resend patch #10.

Cheers,

-- 
Julien Grall