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

[Rahul Singh <Rahul.Singh@arm.com>]

Hello Julien,

> On 21 Jan 2021, at 6:31 pm, Julien Grall <julien@xen.org> wrote:
> 
> 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.

Ok. I agree with you I will send the next version to fix the same.  Please suggest do you want me to check command line also in SMMUv3 now and later remove the code once I will send the patch to move duplicate code. 

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

--


Regards,
Rahul

> 
> Cheers,
> 
> -- 
> Julien Grall