Re: [Xen-devel] [PATCH V2 5/6] iommu/arm: Introduce iommu_add_dt_device API

[Oleksandr <olekstysh@xxxxxxxxx>]

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@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel