Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request

[Oleksandr <olekstysh@xxxxxxxxx>]

On 12.08.19 14:11, Julien Grall wrote:
> Hi Oleksandr,
Hi, Julien


> On 02/08/2019 17:39, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > This patch adds minimal required support to General IOMMU framework
> > to be able to handle a case when IOMMU driver requesting deferred
> > probing for a device.
> >
> > In order not to pull Linux's error code (-EPROBE_DEFER) to Xen
> > we have chosen -EAGAIN to be used for indicating that device
> > probing is deferred.
> >
> > This is needed for the upcoming IPMMU driver which may request
> > deferred probing depending on what device will be probed the first
> > (there is some dependency between these devices, Root device must be
> > registered before Cache devices. If not the case, driver will deny
> > further Cache device probes until Root device is registered).
> > As we can't guarantee a fixed pre-defined order for the device nodes
> > in DT, we need to be ready for the situation where devices being
> > probed in "any" order.
> >
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> >   xen/common/device_tree.c            |  1 +
> >   xen/drivers/passthrough/arm/iommu.c | 35 
> > ++++++++++++++++++++++++++++++++++-
> >   xen/include/asm-arm/device.h        |  6 +++++-
> >   xen/include/xen/device_tree.h       |  1 +
> >   4 files changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index e107c6f..6f37448 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -1774,6 +1774,7 @@ static unsigned long __init 
> > unflatten_dt_node(const void *fdt,
> >           /* By default the device is not protected */
> >           np->is_protected = false;
> >           INIT_LIST_HEAD(&np->domain_list);
> > +        INIT_LIST_HEAD(&np->deferred_probe);
> I am not entirely happy to add a new list_head field per node just for 
> the benefits of boot code. Could we re-use domain_list (with a comment 
> in the code and appropriate ASSERT)?
Agree that only boot code uses deferred_probe field. I will consider 
re-using domain_list. Could you please clarify regarding ASSERT (where 
to put and what to check).

> >             if ( new_format )
> >           {
> > diff --git a/xen/drivers/passthrough/arm/iommu.c 
> > b/xen/drivers/passthrough/arm/iommu.c
> > index 2135233..3195919 100644
> > --- a/xen/drivers/passthrough/arm/iommu.c
> > +++ b/xen/drivers/passthrough/arm/iommu.c
> > @@ -20,6 +20,12 @@
> >   #include <xen/device_tree.h>
> >   #include <asm/device.h>
> >   +/*
> > + * Used to keep track of devices for which driver requested deferred 
> > probing
> > + * (returns -EAGAIN).
> > + */
> > +static LIST_HEAD(deferred_probe_list);
> This wants to be in init section as this is only used at boot.
Will do.


> > +
> >   static const struct iommu_ops *iommu_ops;
> >     const struct iommu_ops *iommu_get_ops(void)
> > @@ -42,7 +48,7 @@ void __init iommu_set_ops(const struct iommu_ops *ops)
> >     int __init iommu_hardware_setup(void)
> >   {
> > -    struct dt_device_node *np;
> > +    struct dt_device_node *np, *tmp;
> >       int rc;
> >       unsigned int num_iommus = 0;
> >   @@ -51,6 +57,33 @@ int __init iommu_hardware_setup(void)
> >           rc = device_init(np, DEVICE_IOMMU, NULL);
> >           if ( !rc )
> >               num_iommus++;
> > +        else if (rc == -EAGAIN)
> > +            /*
> > +             * Driver requested deferred probing, so add this device to
> > +             * the deferred list for further processing.
> > +             */
> > +            list_add(&np->deferred_probe, &deferred_probe_list);
> > +    }
> > +
> > +    /*
> > +     * Process devices in the deferred list if at least one 
> > successfully
> > +     * probed device is present.
> > +     */
> I think this can turn into an infinite loop if all device in 
> deferred_probe_list still return -EDEFER_PROBE and num_iommus is a 
> non-zero.
Agree.


> A better condition would be to check that at least one IOMMU is added 
> at each loop. If not, then we should bail with an error because it 
> likely means something is buggy.
Sounds reasonable. Will do.


Just to clarify:

>>> A better condition would be to check that at least one IOMMU is 
added at each loop.
Maybe, not only added (rc == 0), but driver didn't request deferred 
probe (rc != -EAGAIN).


--
Regards,

Oleksandr Tyshchenko


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