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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 02/04/12 12:30, Ian Campbell wrote:
> On Mon, 2012-04-02 at 11:47 +0100, George Dunlap wrote:
>> # HG changeset patch
>> # User George Dunlap<george.dunlap@eu.citrix.com>
>> # 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@eu.citrix.com>
>>
>> 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.
No idea -- I was just following suit.  In any case, it's probably not a 
bad idea to have this kind of thing in there to help debug stuff.

Let me know if you want to do something else, otherwise I'll keep it in 
for my next patch series.
>>       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.
Sorry -- yeah, I'm not seeing how that code got generated. :-/

I think what I'm going to do is have it return ERROR_FAIL if either the 
open or the write fails.  In general, if the user has asked specifically 
for something to happen and it can't happen, there should be an error, 
so the user can decide if he doesn't care so much, or fix things so it 
can happen.

  -George