From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien.grall@arm.com>, xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
sstabellini@kernel.org
Subject: Re: [Xen-devel] [PATCH V2 5/6] iommu/arm: Introduce iommu_add_dt_device API
Date: Tue, 13 Aug 2019 19:05:43 +0300 [thread overview]
Message-ID: <4b802df9-abc8-d918-eb39-5ef6a426cb43@gmail.com> (raw)
In-Reply-To: <90352292-9218-e682-dd8a-4cf66c0d5c60@arm.com>
On 13.08.19 16:49, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien
>
> On 8/2/19 5:39 PM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This patch adds new iommu_add_dt_device API for adding DT device
>> to the IOMMU using generic IOMMU DT binding [1] and previously
>> added "iommu_fwspec" support.
>>
>> New function parses the DT binding, prepares "dev->iommu_fwspec"
>> with correct information and calls the IOMMU driver using "add_device"
>> callback to register new DT device.
>> The IOMMU driver's responsibility is to check whether
>> "dev->iommu_fwspec"
>> is initialized and mark that device as protected.
>>
>> The additional benefit here is to avoid to go through the whole DT
>> multiple times in IOMMU driver trying to locate master devices which
>> belong to each IOMMU device being probed.
>>
>> The upcoming IPMMU driver will have "add_device" callback implemented.
>>
>> I hope, this patch won't break SMMU driver's functionality,
>> which doesn't have this callback implemented.
>
> The last two sentence does not really belong to the commit message. So
> I think they should go after ---.
>
> Anyway, the only concern for the SMMU is to not break the old
> bindings. New bindings are not supported, so it does not matter
> whether they are broken or not. Once this series is merged, we can
> have a look how new bindings for the SMMU can be supported.
Sounds reasonable.
>
>>
>> [1]
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> xen/arch/arm/domain_build.c | 12 ++++++++++
>> xen/drivers/passthrough/arm/iommu.c | 45
>> +++++++++++++++++++++++++++++++++++++
>> xen/include/asm-arm/iommu.h | 3 +++
>> 3 files changed, 60 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index d983677..d67f7d4 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1241,6 +1241,18 @@ static int __init handle_device(struct domain
>> *d, struct dt_device_node *dev,
>> u64 addr, size;
>> bool need_mapping = !dt_device_for_passthrough(dev);
>> + if ( dt_parse_phandle(dev, "iommus", 0) )
>
> I don't particularly like this check. dt_parse_phandle is non-trivial
> to execute.
>
> TBH, what we should do is trying to call iommu_add_dt_device if IOMMU
> is enabled. We can then return a recognizable value to tell the device
> is not protected.
Well. Don't really mind.
>
>> + {
>> + dt_dprintk("%s add to iommu\n", dt_node_full_name(dev));
>> + res = iommu_add_dt_device(dev);
>> + if ( res )
>> + {
>> + printk(XENLOG_ERR "Failed to add %s to the IOMMU\n",
>> + dt_node_full_name(dev));
>> + return res;
>> + }
>> + }
>> +
>> nirq = dt_number_of_irq(dev);
>> naddr = dt_number_of_address(dev);
>> diff --git a/xen/drivers/passthrough/arm/iommu.c
>> b/xen/drivers/passthrough/arm/iommu.c
>> index 3195919..19516af 100644
>> --- a/xen/drivers/passthrough/arm/iommu.c
>> +++ b/xen/drivers/passthrough/arm/iommu.c
>> @@ -113,3 +113,48 @@ int arch_iommu_populate_page_table(struct domain
>> *d)
>> void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>> {
>> }
>> +
>> +int __init iommu_add_dt_device(struct dt_device_node *np)
>> +{
>> + const struct iommu_ops *ops = iommu_get_ops();
>> + struct dt_phandle_args iommu_spec;
>> + struct device *dev = dt_to_dev(np);
>> + int rc = 1, index = 0;
>> +
>> + if ( !iommu_enabled || !ops || !ops->add_device )
>> + return 0;
>
> While I agree that !iommu_enabled should return 0, for the two others
> I am not entirely sure this is the right thing to do.
>
> !ops is definitely an error because if you have the IOMMU enabled then
> you should have ops installed.
Agree.
>
>
> !ops->add_device means that you will not be able to add any device
> using the new bindings. IOW, the device will be unusable later one as
> most likely the IOMMU was configured to deny any transaction.
> Therefore, this should return an error.
The initial reason *was* to not break SMMU which hasn't had add_device
callback implemented yet. But, I got your point regarding SMMU above
(the only concern for the SMMU is to not break the old bindings), so
agree here.
>
>> +
>> + if ( dev_iommu_fwspec_get(dev) )
>> + return -EEXIST;
>> +
>> + /* According to the
>> Documentation/devicetree/bindings/iommu/iommu.txt */
>
> This file does not exist in Xen, so Linux should at least be mentioned
> in the comment.
Will do.
>
>> + while ( !dt_parse_phandle_with_args(np, "iommus", "#iommu-cells",
>> + index, &iommu_spec) )
>> + {
>> + if ( !dt_device_is_available(iommu_spec.np) )
>> + break;
>> +
>> + rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
>> + if ( rc )
>> + break;
>> +
>> + rc = iommu_fwspec_add_ids(dev, iommu_spec.args, 1);
>
> Here you assume that there will at least always be one cells and the
> first cell is the IDs. For a first, #iommu-cells may be 0 (and
> therefore no cells) when the master IOMMU device cannot be configured.
>
> Furthermore, the content of the #iommu-cells depends on the driver.
> This is why Linux provides a callback of_xlate to let the driver
> decide how to interpret it.
>
> For instance, the SMMU can support either 1 or 2 cells. It also may
> need to look-up for other properties in the node (e.g stream-match-mask).
>
> So I think we also want to provide the of_xlate in Xen.
Hmm, I was thinking how to end up with only one callback re-used
(add_device), really didn't want to add a new one (of_xlate). But, I
didn't take into the account that this stuff is a really
driver-depended. So, likely yes, we need to provide of_xlate callback.
May I ask some questions to clarify:
1. Do you want me to introduce of_xlate callback in a separate patch
(under CONFIG_ARM?)?
2. Can we avoid introducing new API for that callback?
iommu_add_dt_device will be able to call it directly.
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-08-13 16:06 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-02 16:39 [Xen-devel] [PATCH V2 0/6] iommu/arm: Add Renesas IPMMU-VMSA support + Linux's iommu_fwspec Oleksandr Tyshchenko
2019-08-02 16:39 ` [Xen-devel] [PATCH V2 1/6] iommu/arm: Add iommu_helpers.c file to keep common for IOMMUs stuff Oleksandr Tyshchenko
2019-08-09 17:35 ` Julien Grall
2019-08-09 18:10 ` Oleksandr
2019-08-02 16:39 ` [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request Oleksandr Tyshchenko
2019-08-12 11:11 ` Julien Grall
2019-08-12 12:01 ` Oleksandr
2019-08-12 19:46 ` Julien Grall
2019-08-13 12:35 ` Oleksandr
2019-08-14 17:34 ` Julien Grall
2019-08-14 19:25 ` Stefano Stabellini
2019-08-15 9:29 ` Julien Grall
2019-08-15 12:54 ` Julien Grall
2019-08-15 13:14 ` Oleksandr
2019-08-15 16:39 ` Oleksandr
2019-08-02 16:39 ` [Xen-devel] [PATCH V2 3/6] [RFC] xen/common: Introduce _xrealloc function Oleksandr Tyshchenko
2019-08-05 10:02 ` Jan Beulich
2019-08-06 18:50 ` Oleksandr
2019-08-07 6:22 ` Jan Beulich
2019-08-07 17:31 ` Oleksandr
2019-08-06 19:51 ` Volodymyr Babchuk
2019-08-07 6:26 ` Jan Beulich
2019-08-07 18:36 ` Oleksandr
2019-08-08 6:08 ` Jan Beulich
2019-08-08 7:05 ` Jan Beulich
2019-08-08 11:05 ` Oleksandr
2019-08-02 16:39 ` [Xen-devel] [PATCH V2 4/6] iommu/arm: Add lightweight iommu_fwspec support Oleksandr Tyshchenko
2019-08-13 12:39 ` Julien Grall
2019-08-13 15:17 ` Oleksandr
2019-08-13 15:28 ` Julien Grall
2019-08-13 16:18 ` Oleksandr
2019-08-13 13:40 ` Julien Grall
2019-08-13 16:28 ` Oleksandr
2019-08-02 16:39 ` [Xen-devel] [PATCH V2 5/6] iommu/arm: Introduce iommu_add_dt_device API Oleksandr Tyshchenko
2019-08-13 13:49 ` Julien Grall
2019-08-13 16:05 ` Oleksandr [this message]
2019-08-13 17:13 ` Julien Grall
2019-08-02 16:39 ` [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support Oleksandr Tyshchenko
2019-08-07 2:41 ` Yoshihiro Shimoda
2019-08-07 16:01 ` Oleksandr
2019-08-07 19:15 ` Julien Grall
2019-08-07 20:28 ` Oleksandr Tyshchenko
2019-08-08 9:05 ` Julien Grall
2019-08-08 10:14 ` Oleksandr
2019-08-08 12:44 ` Julien Grall
2019-08-08 15:04 ` Oleksandr
2019-08-08 17:16 ` Julien Grall
2019-08-08 19:29 ` Oleksandr
2019-08-08 20:32 ` Julien Grall
2019-08-08 23:32 ` Oleksandr Tyshchenko
2019-08-09 9:56 ` Julien Grall
2019-08-09 18:38 ` Oleksandr
2019-08-08 12:28 ` Oleksandr
2019-08-08 14:23 ` Lars Kurth
2019-08-08 4:05 ` Yoshihiro Shimoda
2019-08-14 17:38 ` Julien Grall
2019-08-14 18:45 ` Oleksandr
2019-08-05 7:58 ` [Xen-devel] [PATCH V2 0/6] iommu/arm: Add Renesas IPMMU-VMSA support + Linux's iommu_fwspec Oleksandr
2019-08-05 8:29 ` 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=4b802df9-abc8-d918-eb39-5ef6a426cb43@gmail.com \
--to=olekstysh@gmail.com \
--cc=julien.grall@arm.com \
--cc=oleksandr_tyshchenko@epam.com \
--cc=sstabellini@kernel.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).