Re: [Xen-devel] [PATCH v10 11/12] tools/libxl: explicitly grant access to needed I/O-memory ranges

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Tue, 2014-07-29 at 00:12 +0200, Arianna Avanzini wrote:

> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 1217310..b5bc07a 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -19,6 +19,7 @@
>  
>  #include "libxl_internal.h"
>  #include "libxl_arch.h"
> +#include "libxl_pci.h"
>  
>  #include <xc_dom.h>
>  #include <xenguest.h>
> @@ -1194,6 +1195,65 @@ static void domcreate_launch_dm(libxl__egc *egc, 
> libxl__multidev *multidev,
>              libxl__spawn_stub_dm(egc, &dcs->dmss);
>          else
>              libxl__spawn_local_dm(egc, &dcs->dmss.dm);
> +
> +        /*
> +         * If VGA passthru is enabled by domain config and one of the
> +         * passthru GPUs is of VGA class, be sure that the related
> +         * stubdomain and the domain itself can access the VGA
> +         * framebuffer.
> +         */

Please move this hole block into a new internal helper (libxl__foo)
function in libxl_pci.c. This will avoid the need to refactor the
headers in the previous patch too.

> +        for (i = 0 ; d_config->b_info.u.hvm.gfx_passthru.val &&
> +                     i < d_config->num_pcidevs ; i++) {

d_config->b_info.u.hvm.gfx_passthru.val (which is static here) in a for
condition is a bit odd. When you refactor this into a function I think
this would quite naturally become a test and return at the head of the
function.

> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 21ecab1..1524170 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -841,10 +841,13 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, 
> uint32_t domid,
>  static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci 
> *pcidev, int starting)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
> +    libxl_domain_type type = libxl__domain_type(gc, domid);
>      int rc, hvm = 0;
>  
> -    switch (libxl__domain_type(gc, domid)) {
> -    case LIBXL_DOMAIN_TYPE_HVM:
> +    if (type == LIBXL_DOMAIN_TYPE_INVALID)
> +        return ERROR_FAIL;
> +
> +    if (type == LIBXL_DOMAIN_TYPE_HVM) {
>          hvm = 1;
>          if (libxl__wait_for_device_model_deprecated(gc, domid, "running",
>                                           NULL, NULL, NULL) < 0) {
> @@ -862,8 +865,7 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, 
> libxl_device_pci *pcidev, i
>          }
>          if ( rc )
>              return ERROR_FAIL;
> -        break;
> -    case LIBXL_DOMAIN_TYPE_PV:
> +    }

If you don't want to bring the following block back a level please still
include a blank line between the } and {.

> @@ -1176,8 +1175,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
>      }
>  
>      rc = ERROR_FAIL;
> -    switch (libxl__domain_type(gc, domid)) {
> -    case LIBXL_DOMAIN_TYPE_HVM:
> +    if (type == LIBXL_DOMAIN_TYPE_HVM) {
>          hvm = 1;
>          if (libxl__wait_for_device_model_deprecated(gc, domid, "running",
>                                           NULL, NULL, NULL) < 0)
> @@ -1198,8 +1196,8 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
>              rc = ERROR_FAIL;
>              goto out_fail;
>          }
> -        break;
> -    case LIBXL_DOMAIN_TYPE_PV:
> +    } else if (type != LIBXL_DOMAIN_TYPE_PV)
> +        abort();

Likewise a blank line after this.

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index e035e97..7f1bb6b 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1422,6 +1422,7 @@ skip_vfb:
>      if (!xlu_cfg_get_list (config, "pci", &pcis, 0, 0)) {
>          d_config->num_pcidevs = 0;
>          d_config->pcidevs = NULL;
> +        fprintf(stderr, "XEN parsing pci list\n");

Leftover debug?



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel