Hello Julien,
> On 21 Jan 2021, at 6:31 pm, Julien Grall <jul...@xen.org> wrote:
>
> On 21/01/2021 17:18, Rahul Singh wrote:
>> Hello Oleksandr,
>
> Hi,
>
>>> On 20 Jan 2021, at 9:33 pm, Oleksandr <olekst...@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.si...@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
