Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
Hi, Julien
I noticed there was already a panic() in iommu_setup() just in case
the user
force the use of IOMMU but they were not initialized. I was
half-tempted to set
iommu_force to true for Arm, but I think this is a different issue.
So here my take (not tested nor compiled).
Thank you. I will check it and come back with results.
I have preliminary tested it with my IPMMU series including new
modification of "deferred probing" patch (attached). Being honest my
series is not based on the *current* staging, but not too outdated.
So, your patch works as expected in the following scenarios:
1. [No panic] Without IOMMU driver in Xen (#CONFIG_IPMMU_VMSA is not set).
2. [No panic] IOMMU driver is present, but reports it is cannot be used
in Xen (P2M sharing not supported, etc) by returning -ENODEV.
3. [No panic] IOMMU is globally disabled in command line "iommu=0".
4. [No panic] IOMMU driver requests deferred probing until the last
device in DT is initialized, after that all deferred devices get
initialized.
5. [Panic] IOMMU driver returns an error (other than -EBADF and -ENODEV)
at the very beginning/when a part of devices are already initialized.
6. [Panic] IOMMU driver always returns deferred probing/returns an error
other than -EAGAIN after deferred probing.
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2c5d1372c0..8f94f618b0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -755,6 +755,7 @@ void __init start_xen(unsigned long
boot_phys_offset,
.max_grant_frames = gnttab_dom0_frames(),
.max_maptrack_frames = opt_max_maptrack_frames,
};
+ int rc;
dcache_line_bytes = read_dcache_line_bytes();
@@ -890,7 +891,9 @@ void __init start_xen(unsigned long
boot_phys_offset,
setup_virt_paging();
- iommu_setup();
+ rc = iommu_setup();
+ if ( !iommu_enabled && rc != -ENODEV )
+ panic("Couldn't configure correctly all the IOMMUs.");
"\n" should be added.
--
Regards,
Oleksandr Tyshchenko
>From ff0af4d3d332a4b7b1b9b7edd4dbd1b3a263cb70 Mon Sep 17 00:00:00 2001
From: Oleksandr Tyshchenko
Date: Thu, 15 Aug 2019 16:56:04 +0300
Subject: [PATCH] iommu/arm: Add ability to handle deferred probing request
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.
While here, order the headers alphabetically.
Signed-off-by: Oleksandr Tyshchenko
---
Changes V2 -> V3:
- removed deferred_probe field from struct dt_device_node,
re-used domain_list instead
- documented domain_list usage
- added ASSERT to check that np->domain_list is empty
before re-using it
- put deferred_probe_list to init section
- used more strict logic regarding processing devices in
the deferred list
- added more comments to code
- put headers in alphabetical order
---
xen/drivers/passthrough/arm/iommu.c | 55 ++---
xen/include/asm-arm/device.h| 6 +++-
xen/include/xen/device_tree.h | 7 +
3 files changed, 63 insertions(+), 5 deletions(-)
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index f8bb2c3..8569d2e 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -15,11 +15,20 @@
* GNU General Public License for more details.
*/
-#include
-#include
#include
+#include
+#include
+
#include
+/*
+ * Deferred probe list is used to keep track of devices for which driver
+ * requested deferred probing (returned -EAGAIN).
+ *
+ * We re-use device's domain_list to link the device in the deferred list.
+ */
+static __initdata LIST_HEAD(deferred_probe_list);
+
static const struct iommu_ops *iommu_ops;
const struct iommu_ops *iommu_get_ops(void)
@@ -42,7 +51,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 +60,17 @@ int __init iommu_hardware_setup(void)
rc = device_init(np, DEVICE_IOMMU, NULL);
if ( !rc )
num_iommus++;
+else if ( rc == -EAGAIN )
+{
+/* We expect nobody uses domain_list
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
On 15.08.19 15:54, Julien Grall wrote:
Hi,
Hi Julien
I noticed there was already a panic() in iommu_setup() just in case the user
force the use of IOMMU but they were not initialized. I was half-tempted to set
iommu_force to true for Arm, but I think this is a different issue.
So here my take (not tested nor compiled).
Thank you. I will check it and come back with results.
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2c5d1372c0..8f94f618b0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -755,6 +755,7 @@ void __init start_xen(unsigned long boot_phys_offset,
.max_grant_frames = gnttab_dom0_frames(),
.max_maptrack_frames = opt_max_maptrack_frames,
};
+int rc;
dcache_line_bytes = read_dcache_line_bytes();
@@ -890,7 +891,9 @@ void __init start_xen(unsigned long boot_phys_offset,
setup_virt_paging();
-iommu_setup();
+rc = iommu_setup();
+if ( !iommu_enabled && rc != -ENODEV )
+panic("Couldn't configure correctly all the IOMMUs.");
do_initcalls();
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 2135233736..f219de9ac3 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -51,6 +51,14 @@ int __init iommu_hardware_setup(void)
rc = device_init(np, DEVICE_IOMMU, NULL);
if ( !rc )
num_iommus++;
+/*
+ * Ignore the following error codes:
+ * - EBADF: Indicate the current not is not an IOMMU
+ * - ENODEV: The IOMMU is not present or cannot be used by
+ * Xen.
+ */
+else if ( rc != -EBADF && rc != -ENODEV )
+return rc;
}
return ( num_iommus > 0 ) ? 0 : -ENODEV;
Cheers,
--
Regards,
Oleksandr Tyshchenko
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
Hi,
On 15/08/2019 10:29, Julien Grall wrote:
> On 14/08/2019 20:25, Stefano Stabellini wrote:
>> On Wed, 14 Aug 2019, Julien Grall wrote:
>> I agree that we should enable all IOMMUs or none. I don't think we want
>> to deal with partially initialized IOMMUs systems.
>>
>> Failure to enable all IOMMUs should lead to returning an error from the
>> relevant function (arch_iommu_populate_page_table?) which should
>
> The patch is:
>
> |> start_xen()
> |> iommu_setup()
> |> iommu_hardware_setup()
>
>> translate into Xen failing to boot and crashing. Which I think it is
>> what you are suggesting, right?
>
> That's correct. At the moment the return value of iommu_setup() is ignored.
> What
> I would like to suggest is something along the following lines:
>
> rc = iommu_setup();
> if ( iommu_enable && rc != -ENODEV )
> panic("Unable to setup IOMMUs");
>
>>
>> (I wouldn't call panic() inside the IOMMU specific initializer, because
>> there might be iommu= command line options for Xen that specify a
>> different desired outcome. I would deal with this case the same way we
>> would deal with an error during initialization of a single IOMMU.)
>
> I am not sure to understand this. If you have an half initialized IOMMU (note
> the "single" here!), then continuing is likely to make things much worst.
>
> I don't advocate to put the panic() inside the IOMMU specific initializer
> (see
> above). But clearly, we should return an error no matter the content of
> 'iommu'
> command line if the user requested to enable the IOMMUs (if any). It wouldn't
> be
> right if the user can say "ignore IOMMU error" as most likely you will have
> unexpected error afterwards.
I noticed there was already a panic() in iommu_setup() just in case the user
force the use of IOMMU but they were not initialized. I was half-tempted to set
iommu_force to true for Arm, but I think this is a different issue.
So here my take (not tested nor compiled).
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2c5d1372c0..8f94f618b0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -755,6 +755,7 @@ void __init start_xen(unsigned long boot_phys_offset,
.max_grant_frames = gnttab_dom0_frames(),
.max_maptrack_frames = opt_max_maptrack_frames,
};
+int rc;
dcache_line_bytes = read_dcache_line_bytes();
@@ -890,7 +891,9 @@ void __init start_xen(unsigned long boot_phys_offset,
setup_virt_paging();
-iommu_setup();
+rc = iommu_setup();
+if ( !iommu_enabled && rc != -ENODEV )
+panic("Couldn't configure correctly all the IOMMUs.");
do_initcalls();
diff --git a/xen/drivers/passthrough/arm/iommu.c
b/xen/drivers/passthrough/arm/iommu.c
index 2135233736..f219de9ac3 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -51,6 +51,14 @@ int __init iommu_hardware_setup(void)
rc = device_init(np, DEVICE_IOMMU, NULL);
if ( !rc )
num_iommus++;
+/*
+ * Ignore the following error codes:
+ * - EBADF: Indicate the current not is not an IOMMU
+ * - ENODEV: The IOMMU is not present or cannot be used by
+ * Xen.
+ */
+else if ( rc != -EBADF && rc != -ENODEV )
+return rc;
}
return ( num_iommus > 0 ) ? 0 : -ENODEV;
>
> Cheers,
>
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
Hi Stefano,
On 14/08/2019 20:25, Stefano Stabellini wrote:
On Wed, 14 Aug 2019, Julien Grall wrote:
I agree that we should enable all IOMMUs or none. I don't think we want
to deal with partially initialized IOMMUs systems.
Failure to enable all IOMMUs should lead to returning an error from the
relevant function (arch_iommu_populate_page_table?) which should
The patch is:
|> start_xen()
|> iommu_setup()
|> iommu_hardware_setup()
translate into Xen failing to boot and crashing. Which I think it is
what you are suggesting, right?
That's correct. At the moment the return value of iommu_setup() is ignored. What
I would like to suggest is something along the following lines:
rc = iommu_setup();
if ( iommu_enable && rc != -ENODEV )
panic("Unable to setup IOMMUs");
(I wouldn't call panic() inside the IOMMU specific initializer, because
there might be iommu= command line options for Xen that specify a
different desired outcome. I would deal with this case the same way we
would deal with an error during initialization of a single IOMMU.)
I am not sure to understand this. If you have an half initialized IOMMU (note
the "single" here!), then continuing is likely to make things much worst.
I don't advocate to put the panic() inside the IOMMU specific initializer (see
above). But clearly, we should return an error no matter the content of 'iommu'
command line if the user requested to enable the IOMMUs (if any). It wouldn't be
right if the user can say "ignore IOMMU error" as most likely you will have
unexpected error afterwards.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
On Wed, 14 Aug 2019, Julien Grall wrote:
> Hi Oleksandr,
>
> On 13/08/2019 13:35, Oleksandr wrote:
> >
> > On 12.08.19 22:46, Julien Grall wrote:
> > > Hi Oleksandr,
> >
> > Hi, Julien
> >
> >
> > >
> > > 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
> > > > > >
> > > > > > 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
> > > > > > ---
> > > > > > 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(>domain_list);
> > > > > > + INIT_LIST_HEAD(>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 :).
> >
> > Got it. Thank you for clarification.
> >
> >
> > > > > > +
> > > > > > 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(>deferred_probe, _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
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
Hi Oleksandr,
On 13/08/2019 13:35, Oleksandr wrote:
On 12.08.19 22:46, Julien Grall wrote:
Hi Oleksandr,
Hi, Julien
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
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
---
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(>domain_list);
+ INIT_LIST_HEAD(>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 :).
Got it. Thank you for clarification.
+
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(>deferred_probe, _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.
It makes sense.
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...
Oh, agree.
As I understand, the new strict logic would be the following:
If initialization for at least one IOMMU device failed (device_init returns an
error other than -EAGAIN), we should stop and return an error to upper layer
(even if num_iommus > 0). No matter whether it is during the first attempt or
after deferring probe. We don't allow the "I/O virtualisation" to be enabled
(iommu_enabled == true) with only part of the IOMMU devices being initialized.
Is my understanding correct?
Let me
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
On 12.08.19 22:46, Julien Grall wrote:
Hi Oleksandr,
Hi, Julien
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
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
---
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(>domain_list);
+ INIT_LIST_HEAD(>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 :).
Got it. Thank you for clarification.
+
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(>deferred_probe, _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.
It makes sense.
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...
Oh, agree.
As I understand, the new strict logic would be the following:
If initialization for at least one IOMMU device failed (device_init
returns an error other than -EAGAIN), we should stop and return an error
to upper layer (even if num_iommus > 0). No matter whether it is during
the first attempt or after deferring probe. We don't allow the "I/O
virtualisation" to be enabled (iommu_enabled == true) with only part of
the IOMMU devices being initialized. Is my understanding correct?
Cheers,
--
Julien Grall
--
Regards,
Oleksandr
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
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
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
---
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(>domain_list);
+ INIT_LIST_HEAD(>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
#include
+/*
+ * 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(>deferred_probe, _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@lists.xenproject.org
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
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
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
---
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(>domain_list);
+ INIT_LIST_HEAD(>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
#include
+/*
+ * 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(>deferred_probe, _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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request
Hi Oleksandr,
On 02/08/2019 17:39, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko
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
---
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(>domain_list);
+INIT_LIST_HEAD(>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)?
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
#include
+/*
+ * 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.
+
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(>deferred_probe, _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.
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.
+while ( !list_empty(_probe_list) && num_iommus )
+{
+list_for_each_entry_safe ( np, tmp, _probe_list,
+ deferred_probe )
+{
+rc = device_init(np, DEVICE_IOMMU, NULL);
+if ( !rc )
+num_iommus++;
+if ( rc != -EAGAIN )
+/*
+ * Driver didn't request deferred probing, so remove this
device
+ * from the deferred list.
+ */
+list_del_init(>deferred_probe);
+}
}
return ( num_iommus > 0 ) ? 0 : -ENODEV;
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 63a0f36..ee1c3bc 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -44,7 +44,11 @@ struct device_desc {
enum device_class class;
/* List of devices supported by this driver */
const struct dt_device_match *dt_match;
-/* Device initialization */
+/*
+ * Device initialization.
+ *
+ * -EAGAIN is used to indicate that device probing is deferred.
+ */
int (*init)(struct dt_device_node *dev, const void *data);
};
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 8315629..71b0e47 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -93,6 +93,7 @@ struct dt_device_node {
/* IOMMU specific fields */
bool is_protected;
struct list_head
