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

[Julien Grall <julien@xxxxxxx>]

On 21/01/2021 18:50, Rahul Singh wrote:
> Hello Julien,
>
> > On 21 Jan 2021, at 6:31 pm, Julien Grall <julien@xxxxxxx> wrote:
> >
> > On 21/01/2021 17:18, Rahul Singh wrote:
> > > Hello Oleksandr,
> > Hi,
> >
> > > > On 20 Jan 2021, at 9:33 pm, Oleksandr <olekstysh@xxxxxxxxx> 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@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 ...
> > > 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);
> --
It should be a copy/paste from the other driver, so it would be nice to 
handle everything now (the code above sounds good).
We can handle the consolition afterwards.

Cheers,


--
Julien Grall