[PATCH] libxl: Don't ignore error when we fail to give access to ioport/irq

[PATCH] libxl: Don't ignore error when we fail to give access to ioport/irq

[Julien Grall]

If we fail to give the access, the domain will unlikely work correctly.
So we should bail out at the first error.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>

---
This patch is candidate for a backport for at least Xen 4.4 and Xen 4.5.
---
 tools/libxl/libxl_create.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1198225..09d481a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1167,6 +1167,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
                  "failed give dom%d access to ioports %"PRIx32"-%"PRIx32,
                  domid, io->first, io->first + io->number - 1);
             ret = ERROR_FAIL;
+            goto error_out;
         }
     }
 
@@ -1182,6 +1183,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         if (ret < 0) {
             LOGE(ERROR, "failed give dom%d access to irq %d", domid, irq);
             ret = ERROR_FAIL;
+            goto error_out;
         }
     }
 
-- 
2.1.4

Re: [PATCH] libxl: Don't ignore error when we fail to give access to ioport/irq

[Wei Liu]

On Fri, Jan 09, 2015 at 01:54:22PM +0000, Julien Grall wrote:
> If we fail to give the access, the domain will unlikely work correctly.
> So we should bail out at the first error.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
> ---
> This patch is candidate for a backport for at least Xen 4.4 and Xen 4.5.

The handling of io mem has same issue. You may also want to fix that?

Also do you need to clean up (revoke permission) the ports, irqs and io
mems that have already been assigned? I presume that's done when domain
is destroyed?

Wei.

> ---
>  tools/libxl/libxl_create.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 1198225..09d481a 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1167,6 +1167,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>                   "failed give dom%d access to ioports %"PRIx32"-%"PRIx32,
>                   domid, io->first, io->first + io->number - 1);
>              ret = ERROR_FAIL;
> +            goto error_out;
>          }
>      }
>  
> @@ -1182,6 +1183,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>          if (ret < 0) {
>              LOGE(ERROR, "failed give dom%d access to irq %d", domid, irq);
>              ret = ERROR_FAIL;
> +            goto error_out;
>          }
>      }
>  
> -- 
> 2.1.4

Re: [PATCH] libxl: Don't ignore error when we fail to give access to ioport/irq

[Julien Grall]

Hi Wei,

On 09/01/15 14:15, Wei Liu wrote:
> On Fri, Jan 09, 2015 at 01:54:22PM +0000, Julien Grall wrote:
>> If we fail to give the access, the domain will unlikely work correctly.
>> So we should bail out at the first error.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>>
>> ---
>> This patch is candidate for a backport for at least Xen 4.4 and Xen 4.5.
> 
> The handling of io mem has same issue. You may also want to fix that?

I forgot this one. I will fix in the next version.

> Also do you need to clean up (revoke permission) the ports, irqs and io
> mems that have already been assigned?

We don't need to clean up ioport/irqs/iomems. It's already done
implicitly when the domain is destroyed.

> I presume that's done when domain
> is destroyed?

If domain destroyed doesn't correctly revoke the permission that would
mean the code is buggy in the normal behavior.

Regards,


-- 
Julien Grall