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

[Julien Grall <julien@xxxxxxx>]

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.
Cheers,

--
Julien Grall