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

[Oleksandr <olekstysh@xxxxxxxxx>]

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

> +
> +static void arm_smmu_iommu_xen_domain_teardown(struct domain *d)
> +{
> +       struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
> +
> +       ASSERT(list_empty(&xen_domain->contexts));
> +       xfree(xen_domain);
> +}
> +
> +static const struct iommu_ops arm_smmu_iommu_ops = {
> +       .init           = arm_smmu_iommu_xen_domain_init,
> +       .hwdom_init             = arm_smmu_iommu_hwdom_init,
> +       .teardown               = arm_smmu_iommu_xen_domain_teardown,
> +       .iotlb_flush            = arm_smmu_iotlb_flush,
> +       .iotlb_flush_all        = arm_smmu_iotlb_flush_all,
> +       .assign_device          = arm_smmu_assign_dev,
> +       .reassign_device        = arm_smmu_reassign_dev,
> +       .map_page               = arm_iommu_map_page,
> +       .unmap_page             = arm_iommu_unmap_page,
> +       .dt_xlate               = arm_smmu_dt_xlate,
> +       .add_device             = arm_smmu_add_device,
> +};
> +
> +static __init int arm_smmu_dt_init(struct dt_device_node *dev,
> +                               const void *data)
> +{
> +       int rc;
> +
> +       /*
> +        * Even if the device can't be initialized, we don't want to
> +        * give the SMMU device to dom0.
> +        */
> +       dt_device_set_used_by(dev, DOMID_XEN);
> +
> +       rc = arm_smmu_device_probe(dt_to_dev(dev));
> +       if (rc)
> +               return rc;
> +
> +       iommu_set_ops(&arm_smmu_iommu_ops);
> +
> +       return 0;
> +}
> +
> +DT_DEVICE_START(smmuv3, "ARM SMMU V3", DEVICE_IOMMU)
> +.dt_match = arm_smmu_of_match,
> +.init = arm_smmu_dt_init,
> +DT_DEVICE_END
--
Regards,

Oleksandr Tyshchenko