From: Rahul Singh <Rahul.Singh@arm.com>
To: Julien Grall <julien@xen.org>
Cc: Oleksandr <olekstysh@gmail.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
Bertrand Marquis <Bertrand.Marquis@arm.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Ian Jackson <iwj@xenproject.org>, Jan Beulich <jbeulich@suse.com>,
Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
Paul Durrant <paul@xen.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
Date: Thu, 21 Jan 2021 18:50:14 +0000 [thread overview]
Message-ID: <35D8F2EE-8875-4941-8C5A-2BDB8B29F421@arm.com> (raw)
In-Reply-To: <d5c1f75a-8e5c-a938-0d10-a0d276643052@xen.org>
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
next prev parent reply other threads:[~2021-01-21 18:50 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-20 14:52 [PATCH v5 00/10] xen/arm: Add support for SMMUv3 driver Rahul Singh
2021-01-20 14:52 ` [PATCH v5 01/10] xen/arm: smmuv3: Import the SMMUv3 driver from Linux Rahul Singh
2021-01-20 14:52 ` [PATCH v5 02/10] xen/arm: Revert atomic operation related command-queue insertion patch Rahul Singh
2021-01-20 14:52 ` [PATCH v5 03/10] xen/arm: smmuv3: Revert patch related to XArray Rahul Singh
2021-01-20 14:52 ` [PATCH v5 04/10] xen/arm: smmuv3: Remove support for Stage-1 translation on SMMUv3 Rahul Singh
2021-01-20 14:52 ` [PATCH v5 05/10] xen/arm: smmuv3: Remove Linux specific code that is not usable in XEN Rahul Singh
2021-01-20 14:52 ` [PATCH v5 06/10] xen/device-tree: Add dt_property_match_string helper Rahul Singh
2021-01-20 14:52 ` [PATCH v5 07/10] xen/compiler: import 'fallthrough' keyword from linux Rahul Singh
2021-01-20 14:52 ` [PATCH v5 08/10] xen/arm: smmuv3: Use fallthrough pseudo-keyword Rahul Singh
2021-01-20 15:36 ` Bertrand Marquis
2021-01-20 19:56 ` Stefano Stabellini
2021-01-20 14:52 ` [PATCH v5 09/10] xen/arm: smmuv3: Replace linux functions with xen functions Rahul Singh
2021-01-20 14:52 ` [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver Rahul Singh
2021-01-20 15:35 ` Bertrand Marquis
2021-01-20 20:31 ` Stefano Stabellini
2021-01-21 17:10 ` Rahul Singh
2021-01-21 18:43 ` Julien Grall
2021-01-21 20:28 ` Rahul Singh
2021-01-21 21:20 ` Stefano Stabellini
2021-01-20 21:33 ` Oleksandr
2021-01-21 17:18 ` Rahul Singh
2021-01-21 18:31 ` Julien Grall
2021-01-21 18:50 ` Rahul Singh [this message]
2021-01-21 20:19 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=35D8F2EE-8875-4941-8C5A-2BDB8B29F421@arm.com \
--to=rahul.singh@arm.com \
--cc=Bertrand.Marquis@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=olekstysh@gmail.com \
--cc=paul@xen.org \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).