libxl: tooling expects wrong errno

Commit Message

Grzegorz Uriasz June 14, 2020, 4:17 p.m. UTC

When iommu is not enabled for a given domain then pci passthrough
hypercalls such as xc_test_assign_device return EOPNOTSUPP.
The code responsible for this is in "iommu_do_domctl" inside
xen/drivers/passthrough/iommu.c
This patch fixes the error message reported by libxl when assigning
pci devices to domains without iommu.

Signed-off-by: Grzegorz Uriasz <[e-mail]>
Tested-by: Grzegorz Uriasz <[e-mail]>
---
 tools/libxl/libxl_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ian Jackson June 15, 2020, 2:59 p.m. UTC | #1

Grzegorz Uriasz writes ("[PATCH] libxl: tooling expects wrong errno"):
> When iommu is not enabled for a given domain then pci passthrough
> hypercalls such as xc_test_assign_device return EOPNOTSUPP.
> The code responsible for this is in "iommu_do_domctl" inside
> xen/drivers/passthrough/iommu.c
> This patch fixes the error message reported by libxl when assigning
> pci devices to domains without iommu.
> 
> Signed-off-by: Grzegorz Uriasz <[e-mail]>
> Tested-by: Grzegorz Uriasz <[e-mail]>
> ---
>  tools/libxl/libxl_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 957ff5c8e9..bc5843b137 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -1561,7 +1561,7 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
>              LOGD(ERROR, domid,
>                   "PCI device %04x:%02x:%02x.%u %s?",
>                   pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
> -                 errno == ENOSYS ? "cannot be assigned - no IOMMU"
> +                 errno == EOPNOTSUPP ? "cannot be assigned - no IOMMU"
>                   : "already assigned to a different guest");
>              goto out;
>          }

Thanks.  I have addressed some Xen IOMMU maintainers.  Can you confirm
whether this is right ?

Regards,
Ian.

Roger Pau Monne June 15, 2020, 3:56 p.m. UTC | #2

On Mon, Jun 15, 2020 at 03:59:50PM +0100, Ian Jackson wrote:
> Grzegorz Uriasz writes ("[PATCH] libxl: tooling expects wrong errno"):
> > When iommu is not enabled for a given domain then pci passthrough
> > hypercalls such as xc_test_assign_device return EOPNOTSUPP.
> > The code responsible for this is in "iommu_do_domctl" inside
> > xen/drivers/passthrough/iommu.c
> > This patch fixes the error message reported by libxl when assigning
> > pci devices to domains without iommu.
> > 
> > Signed-off-by: Grzegorz Uriasz <[e-mail]>
> > Tested-by: Grzegorz Uriasz <[e-mail]>
> > ---
> >  tools/libxl/libxl_pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 957ff5c8e9..bc5843b137 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -1561,7 +1561,7 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
> >              LOGD(ERROR, domid,
> >                   "PCI device %04x:%02x:%02x.%u %s?",
> >                   pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
> > -                 errno == ENOSYS ? "cannot be assigned - no IOMMU"
> > +                 errno == EOPNOTSUPP ? "cannot be assigned - no IOMMU"
> >                   : "already assigned to a different guest");
> >              goto out;
> >          }
> 
> Thanks.  I have addressed some Xen IOMMU maintainers.  Can you confirm
> whether this is right ?

Not an IOMMU maintainer myself, but I've taken a look at the code and
I think Grzegorz is right. iommu_do_domctl will return -EOPNOTSUPP if
the IOMMU is not enabled for the domain. Another option would be to
check for EBUSY (which will certainly be returned when the device is
busy) and log the error code with a message when it's different than
EBUSY?

There are many possible error here, for example the device itself
might not be behind an IOMMU, in which case Xen will return -ENODEV at
least on the Intel case.

Thanks, Roger.

Paul Durrant June 15, 2020, 4:04 p.m. UTC | #3

> -----Original Message-----
> From: Roger Pau Monné <[e-mail]>
> Sent: 15 June 2020 16:57
> To: Ian Jackson <[e-mail]>
> Cc: Grzegorz Uriasz <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> <[e-mail]>; Kevin Tian <[e-mail]>; Paul Durrant <[e-mail]>; Wei Liu
> <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; Anthony
> Perard <[e-mail]>; [e-mail]; [e-mail]
> Subject: Re: [PATCH] libxl: tooling expects wrong errno
> 
> On Mon, Jun 15, 2020 at 03:59:50PM +0100, Ian Jackson wrote:
> > Grzegorz Uriasz writes ("[PATCH] libxl: tooling expects wrong errno"):
> > > When iommu is not enabled for a given domain then pci passthrough
> > > hypercalls such as xc_test_assign_device return EOPNOTSUPP.
> > > The code responsible for this is in "iommu_do_domctl" inside
> > > xen/drivers/passthrough/iommu.c
> > > This patch fixes the error message reported by libxl when assigning
> > > pci devices to domains without iommu.
> > >
> > > Signed-off-by: Grzegorz Uriasz <[e-mail]>
> > > Tested-by: Grzegorz Uriasz <[e-mail]>
> > > ---
> > >  tools/libxl/libxl_pci.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > > index 957ff5c8e9..bc5843b137 100644
> > > --- a/tools/libxl/libxl_pci.c
> > > +++ b/tools/libxl/libxl_pci.c
> > > @@ -1561,7 +1561,7 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
> > >              LOGD(ERROR, domid,
> > >                   "PCI device %04x:%02x:%02x.%u %s?",
> > >                   pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
> > > -                 errno == ENOSYS ? "cannot be assigned - no IOMMU"
> > > +                 errno == EOPNOTSUPP ? "cannot be assigned - no IOMMU"
> > >                   : "already assigned to a different guest");
> > >              goto out;
> > >          }
> >
> > Thanks.  I have addressed some Xen IOMMU maintainers.  Can you confirm
> > whether this is right ?
> 
> Not an IOMMU maintainer myself, but I've taken a look at the code and
> I think Grzegorz is right. iommu_do_domctl will return -EOPNOTSUPP if
> the IOMMU is not enabled for the domain. Another option would be to
> check for EBUSY (which will certainly be returned when the device is
> busy) and log the error code with a message when it's different than
> EBUSY?
> 
> There are many possible error here, for example the device itself
> might not be behind an IOMMU, in which case Xen will return -ENODEV at
> least on the Intel case.

ENOSYS is certainly wrong; it should only be used to indicate an unimplemented hypercall. I think Roger's suggestion of avoiding EBUSY and use LOGED would be better though.

  Paul

> 
> Thanks, Roger.

Ian Jackson June 15, 2020, 4:37 p.m. UTC | #4

Roger Pau Monne writes ("Re: [PATCH] libxl: tooling expects wrong errno"):
> On Mon, Jun 15, 2020 at 03:59:50PM +0100, Ian Jackson wrote:
> > Grzegorz Uriasz writes ("[PATCH] libxl: tooling expects wrong errno"):
> > > When iommu is not enabled for a given domain then pci passthrough
> > > hypercalls such as xc_test_assign_device return EOPNOTSUPP.
> > > The code responsible for this is in "iommu_do_domctl" inside
> > > xen/drivers/passthrough/iommu.c
> > > This patch fixes the error message reported by libxl when assigning
> > > pci devices to domains without iommu.
> > > 
> > > Signed-off-by: Grzegorz Uriasz <[e-mail]>
> > > Tested-by: Grzegorz Uriasz <[e-mail]>
> > > ---
> > >  tools/libxl/libxl_pci.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > > index 957ff5c8e9..bc5843b137 100644
> > > --- a/tools/libxl/libxl_pci.c
> > > +++ b/tools/libxl/libxl_pci.c
> > > @@ -1561,7 +1561,7 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
> > >              LOGD(ERROR, domid,
> > >                   "PCI device %04x:%02x:%02x.%u %s?",
> > >                   pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
> > > -                 errno == ENOSYS ? "cannot be assigned - no IOMMU"
> > > +                 errno == EOPNOTSUPP ? "cannot be assigned - no IOMMU"
> > >                   : "already assigned to a different guest");
> > >              goto out;
> > >          }
> > 
> > Thanks.  I have addressed some Xen IOMMU maintainers.  Can you confirm
> > whether this is right ?
> 
> Not an IOMMU maintainer myself, but I've taken a look at the code and
> I think Grzegorz is right. iommu_do_domctl will return -EOPNOTSUPP if
> the IOMMU is not enabled for the domain. Another option would be to
> check for EBUSY (which will certainly be returned when the device is
> busy) and log the error code with a message when it's different than
> EBUSY?
> 
> There are many possible error here, for example the device itself
> might not be behind an IOMMU, in which case Xen will return -ENODEV at
> least on the Intel case.

Thanks for the analysis.  So:

Acked-by: Ian Jackson <[e-mail]>

This would seem to be a backport candidate.  AFAICT check has been
there, looking for ENOSYS, since this code was introduced in
   826eb17271d3c647516d9944c47b0779afedea25
   libxl: suppress device assignment to HVM guest when there is no IOMMU
?

But that commit has a Tested-by.  Maybe Xen changed its error return
at some point ?

Ian.

Paul Durrant June 15, 2020, 5:11 p.m. UTC | #5

> -----Original Message-----
> From: Ian Jackson <[e-mail]>
> Sent: 15 June 2020 17:38
> To: Roger Pau Monne <[e-mail]>
> Cc: Grzegorz Uriasz <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> <[e-mail]>; Kevin Tian <[e-mail]>; Paul Durrant <[e-mail]>; Wei Liu
> <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; Anthony
> Perard <[e-mail]>; [e-mail]; [e-mail]
> Subject: Re: [PATCH] libxl: tooling expects wrong errno
> 
> Roger Pau Monne writes ("Re: [PATCH] libxl: tooling expects wrong errno"):
> > On Mon, Jun 15, 2020 at 03:59:50PM +0100, Ian Jackson wrote:
> > > Grzegorz Uriasz writes ("[PATCH] libxl: tooling expects wrong errno"):
> > > > When iommu is not enabled for a given domain then pci passthrough
> > > > hypercalls such as xc_test_assign_device return EOPNOTSUPP.
> > > > The code responsible for this is in "iommu_do_domctl" inside
> > > > xen/drivers/passthrough/iommu.c
> > > > This patch fixes the error message reported by libxl when assigning
> > > > pci devices to domains without iommu.
> > > >
> > > > Signed-off-by: Grzegorz Uriasz <[e-mail]>
> > > > Tested-by: Grzegorz Uriasz <[e-mail]>
> > > > ---
> > > >  tools/libxl/libxl_pci.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > > > index 957ff5c8e9..bc5843b137 100644
> > > > --- a/tools/libxl/libxl_pci.c
> > > > +++ b/tools/libxl/libxl_pci.c
> > > > @@ -1561,7 +1561,7 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
> > > >              LOGD(ERROR, domid,
> > > >                   "PCI device %04x:%02x:%02x.%u %s?",
> > > >                   pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
> > > > -                 errno == ENOSYS ? "cannot be assigned - no IOMMU"
> > > > +                 errno == EOPNOTSUPP ? "cannot be assigned - no IOMMU"
> > > >                   : "already assigned to a different guest");
> > > >              goto out;
> > > >          }
> > >
> > > Thanks.  I have addressed some Xen IOMMU maintainers.  Can you confirm
> > > whether this is right ?
> >
> > Not an IOMMU maintainer myself, but I've taken a look at the code and
> > I think Grzegorz is right. iommu_do_domctl will return -EOPNOTSUPP if
> > the IOMMU is not enabled for the domain. Another option would be to
> > check for EBUSY (which will certainly be returned when the device is
> > busy) and log the error code with a message when it's different than
> > EBUSY?
> >
> > There are many possible error here, for example the device itself
> > might not be behind an IOMMU, in which case Xen will return -ENODEV at
> > least on the Intel case.
> 
> Thanks for the analysis.  So:
> 
> Acked-by: Ian Jackson <[e-mail]>
> 
> This would seem to be a backport candidate.  AFAICT check has been
> there, looking for ENOSYS, since this code was introduced in
>    826eb17271d3c647516d9944c47b0779afedea25
>    libxl: suppress device assignment to HVM guest when there is no IOMMU
> ?
> 
> But that commit has a Tested-by.  Maybe Xen changed its error return
> at some point ?
> 

Yes, it happened in 71e617a6b8f69849c70eda1b3c58f1ff6b244e5a
use is_iommu_enabled() where appropriate...

  Paul

> Ian.

Ian Jackson June 15, 2020, 5:39 p.m. UTC | #6

Paul Durrant writes ("RE: [PATCH] libxl: tooling expects wrong errno"):
> > -----Original Message-----
> > From: Ian Jackson <[e-mail]>
> > Thanks for the analysis.  So:
> > 
> > Acked-by: Ian Jackson <[e-mail]>
> > 
> > This would seem to be a backport candidate.  AFAICT check has been
> > there, looking for ENOSYS, since this code was introduced in
> >    826eb17271d3c647516d9944c47b0779afedea25
> >    libxl: suppress device assignment to HVM guest when there is no IOMMU
> > ?
> > 
> > But that commit has a Tested-by.  Maybe Xen changed its error return
> > at some point ?
> > 
> 
> Yes, it happened in 71e617a6b8f69849c70eda1b3c58f1ff6b244e5a
> use is_iommu_enabled() where appropriate...

So,

Backport: 4.13

Thanks!

Ian.

Wei Liu June 26, 2020, 11:25 a.m. UTC | #7

On Mon, Jun 15, 2020 at 06:39:30PM +0100, Ian Jackson wrote:
> Paul Durrant writes ("RE: [PATCH] libxl: tooling expects wrong errno"):
> > > -----Original Message-----
> > > From: Ian Jackson <[e-mail]>
> > > Thanks for the analysis.  So:
> > > 
> > > Acked-by: Ian Jackson <[e-mail]>
> > > 
> > > This would seem to be a backport candidate.  AFAICT check has been
> > > there, looking for ENOSYS, since this code was introduced in
> > >    826eb17271d3c647516d9944c47b0779afedea25
> > >    libxl: suppress device assignment to HVM guest when there is no IOMMU
> > > ?
> > > 
> > > But that commit has a Tested-by.  Maybe Xen changed its error return
> > > at some point ?
> > > 
> > 
> > Yes, it happened in 71e617a6b8f69849c70eda1b3c58f1ff6b244e5a
> > use is_iommu_enabled() where appropriate...
> 
> So,
> 
> Backport: 4.13
> 
> Thanks!

This is not yet committed. Paul, can I get a release ack from you?

Wei.

> 
> Ian.

Paul Durrant June 26, 2020, 11:28 a.m. UTC | #8

> -----Original Message-----
> From: Wei Liu <[e-mail]>
> Sent: 26 June 2020 12:25
> To: Ian Jackson <[e-mail]>
> Cc: [e-mail]; Roger Pau Monne <[e-mail]>; 'Grzegorz Uriasz' <[e-mail]>; 'Jan
> Beulich' <[e-mail]>; Andrew Cooper <[e-mail]>; Kevin Tian
> <[e-mail]>; 'Wei Liu' <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; Anthony Perard <[e-mail]>; xen-
> [e-mail]; [e-mail]
> Subject: Re: [PATCH] libxl: tooling expects wrong errno
> 
> On Mon, Jun 15, 2020 at 06:39:30PM +0100, Ian Jackson wrote:
> > Paul Durrant writes ("RE: [PATCH] libxl: tooling expects wrong errno"):
> > > > -----Original Message-----
> > > > From: Ian Jackson <[e-mail]>
> > > > Thanks for the analysis.  So:
> > > >
> > > > Acked-by: Ian Jackson <[e-mail]>
> > > >
> > > > This would seem to be a backport candidate.  AFAICT check has been
> > > > there, looking for ENOSYS, since this code was introduced in
> > > >    826eb17271d3c647516d9944c47b0779afedea25
> > > >    libxl: suppress device assignment to HVM guest when there is no IOMMU
> > > > ?
> > > >
> > > > But that commit has a Tested-by.  Maybe Xen changed its error return
> > > > at some point ?
> > > >
> > >
> > > Yes, it happened in 71e617a6b8f69849c70eda1b3c58f1ff6b244e5a
> > > use is_iommu_enabled() where appropriate...
> >
> > So,
> >
> > Backport: 4.13
> >
> > Thanks!
> 
> This is not yet committed. Paul, can I get a release ack from you?
> 
> Wei.

Yes.

Release-acked-by: Paul Durrant <[e-mail]>

> 
> >
> > Ian.

Patch

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 957ff5c8e9..bc5843b137 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1561,7 +1561,7 @@  void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
             LOGD(ERROR, domid,
                  "PCI device %04x:%02x:%02x.%u %s?",
                  pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
-                 errno == ENOSYS ? "cannot be assigned - no IOMMU"
+                 errno == EOPNOTSUPP ? "cannot be assigned - no IOMMU"
                  : "already assigned to a different guest");
             goto out;
         }