[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.