xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien.grall@arm.com>
To: Oleksandr Tyshchenko <olekstysh@gmail.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 14:49:52 +0100	[thread overview]
Message-ID: <90352292-9218-e682-dd8a-4cf66c0d5c60@arm.com> (raw)
In-Reply-To: <1564763985-20312-6-git-send-email-olekstysh@gmail.com>

Hi Oleksandr,

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.

> 
> [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.

> +    {
> +        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.

!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.

> +
> +    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.

> +    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.

> +        if ( rc )
> +            break;
> +
> +        index++;
> +    }
> +
> +    /*
> +     * Add DT device to the IOMMU if latter is present and available.
> +     * The IOMMU driver's responsibility is to check whether dev->iommu_fwspec
> +     * field is initialized and mark that device as protected.
> +     */
> +    if ( !rc )
> +        rc = ops->add_device(0, dev);
> +
> +    if ( rc < 0 )
> +        iommu_fwspec_free(dev);
> +
> +    return rc < 0 ? rc : 0;
> +}
> diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
> index 1853bd9..06b07fa 100644
> --- a/xen/include/asm-arm/iommu.h
> +++ b/xen/include/asm-arm/iommu.h
> @@ -28,6 +28,9 @@ struct arch_iommu
>   const struct iommu_ops *iommu_get_ops(void);
>   void iommu_set_ops(const struct iommu_ops *ops);
>   
> +/* helper to add DT device to the IOMMU */
> +int iommu_add_dt_device(struct dt_device_node *np);
> +
>   /* mapping helpers */
>   int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
>                                       unsigned int flags,
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

  reply	other threads:[~2019-08-13 13:50 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 [this message]
2019-08-13 16:05     ` Oleksandr
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=90352292-9218-e682-dd8a-4cf66c0d5c60@arm.com \
    --to=julien.grall@arm.com \
    --cc=oleksandr_tyshchenko@epam.com \
    --cc=olekstysh@gmail.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).