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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Oleksandr,

On 8/12/19 1:01 PM, Oleksandr wrote:
> On 12.08.19 14:11, Julien Grall wrote:
> > 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).
What I meant is adding an ASSERT to check that np->domain_list is at 
empty at least before trying to add in the list. This would help to 
debug any potential issue if we end up to use domain_list earlier in the 
future. I can't see why it would as iommu is called earlier, but who 
knows :).
> > >             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).
I think adding an IOMMU is enough. If you return an error other than 
-EAGAIN here after deferring probing, then you are likely going to fail 
at the next loop. So better to stop early.
I realize this not what the current code is doing (I know I wrote it 
;)). But I am not sure it is sane to continue if only part of the IOMMUs 
are initialized. Most likely you will see an error much later that may 
be not trivial to find out.
Imagine you want to passthrough you network card to a guest but the 
IOMMU initialization failed...
Cheers,

--
Julien Grall

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