Re: [Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-04-02 at 11:47 +0100, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> # Date 1333363500 -3600
> # Node ID 62b1030a2485536caf995b825bee9e8255f914b3
> # Parent  5386937e6c5c9afaa8a3cd56d391dcc9e40d0596
> xl,libxl: Add per-device and global permissive config options for pci 
> passthrough
> 
> By default pciback only allows PV guests to write "known safe" values into
> PCI config space.  But many devices require writes to other areas of config
> space in order to operate properly.  One way to do that is with the "quirks"
> interface, which specifies areas known safe to a particular device; the
> other way is to mark a device as "permissive", which tells pciback to allow
> all config space writes for that domain and device.
> 
> This adds a "permissive" flag to the libxl_pci struct and teaches libxl how
> to write the appropriate value into sysfs to enable the permissive feature for
> devices being passed through.  It also adds the permissive config options 
> either
> on a per-device basis, or as a global option in the xl command-line.
> 
> Because of the potential stability and security implications of enabling
> permissive, the flag is left off by default.
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> diff -r 5386937e6c5c -r 62b1030a2485 docs/man/xl.cfg.pod.5
> --- a/docs/man/xl.cfg.pod.5   Mon Apr 02 11:29:34 2012 +0100
> +++ b/docs/man/xl.cfg.pod.5   Mon Apr 02 11:45:00 2012 +0100
> @@ -294,10 +294,25 @@ XXX
>  
>  XXX
>  
> +=item B<permissive=BOOLEAN>
> +
> +By default pciback only allows PV guests to write "known safe" values into
> +PCI config space.  But many devices require writes to other areas of config
> +space in order to operate properly.  This tells the pciback driver to
> +allow all writes to PCI config space for this domain and this device.  This
> +option should be enabled with caution, as there may be stability or security 
> +implications of doing so.
> +
>  =back
>  
>  =back
>  
> +=item B<pci_permissive=BOOLEAN>
> +
> +Changes the default value of 'permissive' for all PCI devices for this
> +VM.  This can still be overriden on a per-device basis. See the
> +"pci=" section for more information on the "permissive" flag.
> +
>  =back
>  
>  =head2 Paravirtualised (PV) Guest Specific Options
> diff -r 5386937e6c5c -r 62b1030a2485 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c Mon Apr 02 11:29:34 2012 +0100
> +++ b/tools/libxl/libxl_pci.c Mon Apr 02 11:45:00 2012 +0100
> @@ -55,7 +55,10 @@ static void libxl_create_pci_backend_dev
>      if (pcidev->vdevfn)
>          flexarray_append_pair(back, libxl__sprintf(gc, "vdevfn-%d", num), 
> libxl__sprintf(gc, "%x", pcidev->vdevfn));
>      flexarray_append(back, libxl__sprintf(gc, "opts-%d", num));
> -    flexarray_append(back, libxl__sprintf(gc, 
> "msitranslate=%d,power_mgmt=%d", pcidev->msitranslate, pcidev->power_mgmt));
> +    flexarray_append(back,
> +              libxl__sprintf(gc, 
> "msitranslate=%d,power_mgmt=%d,permissive=%d",
> +                             pcidev->msitranslate, pcidev->power_mgmt,
> +                             pcidev->permissive));

Since we are already writing this key, and AFAICT this matches xend's
behaviour, I think it is correct to add the new parameter here. But...

Does anything actually read this key? I can't find anything in pciback
or qemu. 

>      flexarray_append_pair(back, libxl__sprintf(gc, "state-%d", num), 
> libxl__sprintf(gc, "%d", 1));
>  }
>  
> @@ -565,6 +568,29 @@ static int do_pci_add(libxl__gc *gc, uin
>              }
>          }
>          fclose(f);
> +
> +        /* Don't restrict writes to the PCI config space from this VM */
> +        if (pcidev->permissive) {
> +            int fd;
> +            char *buf;
> +
> +            sysfs_path = libxl__sprintf(gc, 
> SYSFS_PCIBACK_DRIVER"/permissive");
> +            fd = open(sysfs_path, O_WRONLY);
> +            if (fd < 0) {
> +                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
> +                                 sysfs_path);
> +                goto out;

Why not either fall through (i.e. accept this as a soft failure) or
return an actual error here?

FWIW I think the case where we cannot open the sysfs "irq" node and
"goto out" is also similarly wrong.

> +            }
> + 
> +            buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus,
> +                                 pcidev->dev, pcidev->func);
> +            rc = write(fd, buf, strlen(buf));
> +            if (rc < 0)
> +                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "write to %s 
> returned %d",
> +                                 sysfs_path, rc);
> +            close(fd);
> +            goto out;

I don't think this makes sense, out is the next thing we actually get to
anyway and there is a "break" out of the switch right below.

> +        }
>          break;
>      }
>      default:

Ian.



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