[libvirt] [PATCH/RFC]: hostdev passthrough support
Hi, attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via: <devices> <hostdev type='usb' vendor='0204' product='6025'/> <hostdev type='usb' bus='001' device='007'/> </devices> I didn't implement unplug yet since this needs some modifications to qemu/kvm to be able to identify the correct device to unplug. Does this look reasonable? -- Guido
On Fri, Jul 25, 2008 at 04:17:30PM -0400, Guido Günther wrote:
Hi, attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via:
<devices> <hostdev type='usb' vendor='0204' product='6025'/>
you meant type='pci' there right ?
<hostdev type='usb' bus='001' device='007'/> </devices>
Hum, on the format level I think this need a bit more discussion. the type is better caracterized as a bus informations. Also I'm not sure if we should keep a flat single element or (expecting possible further complex descriptions later) go for a more structured description like <hostdev type='pci'> <source vendor='0204' product='6025'/> </hostdev> I think an hypervisor could remap the actual port/addresses of a device so a target may make sense in the future (or not).
I didn't implement unplug yet since this needs some modifications to qemu/kvm to be able to identify the correct device to unplug. Does this look reasonable?
I also think we need to clarify the naming conventions, are numbers provided decimal, if yes then is an 0x hexadecimal version allowed too. I also see how a more high level description might be useful, <hostdev type='usb'> <source vendor='Sennheiser'/> </hostdev> or <hostdev type='usb'> <source product='headset'/> </hostdev> in that case this means that libvirt has some lookup to do, but in the end the XML format becomes more portable which is an important property IMHO. From an implementation perspective, the patch looks very clean to me aside maybe some Hostdev type vs. Bus naming for the enum, but maybe we need to give a bit more thoughs one the format first :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi Daniel, On Mon, Jul 28, 2008 at 09:26:57AM -0400, Daniel Veillard wrote:
On Fri, Jul 25, 2008 at 04:17:30PM -0400, Guido Günther wrote:
Hi, attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via:
<devices> <hostdev type='usb' vendor='0204' product='6025'/>
you meant type='pci' there right ? Qemu support host:bus.device syntax for usb devices (useful for the case of two identical devices). PCI can work like this too of course.
<hostdev type='usb' bus='001' device='007'/> </devices>
Hum, on the format level I think this need a bit more discussion. the type is better caracterized as a bus informations. Also I'm not sure if we should keep a flat single element or (expecting possible further complex descriptions later) go for a more structured description like
<hostdev type='pci'> <source vendor='0204' product='6025'/> </hostdev> I think this looks better. Actually I thought so already after sending the patch.
I think an hypervisor could remap the actual port/addresses of a device so a target may make sense in the future (or not). Hopefully it does. It would be nice if we could do that since we'd then have the necessary data for easy unmapping.
I didn't implement unplug yet since this needs some modifications to qemu/kvm to be able to identify the correct device to unplug. Does this look reasonable?
I also think we need to clarify the naming conventions, are numbers provided decimal, if yes then is an 0x hexadecimal version allowed too. Currently they're all hex as outputet by lsusb.
[..snip..]
From an implementation perspective, the patch looks very clean to me aside maybe some Hostdev type vs. Bus naming for the enum, but maybe we need to give a bit more thoughs one the format first :-) Sure. I'll see what I can come up with. Thanks for the comments. -- Guido
On Mon, Jul 28, 2008 at 09:26:57AM -0400, Daniel Veillard wrote:
On Fri, Jul 25, 2008 at 04:17:30PM -0400, Guido Günther wrote:
Hi, attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via:
<devices> <hostdev type='usb' vendor='0204' product='6025'/>
you meant type='pci' there right ?
<hostdev type='usb' bus='001' device='007'/> </devices>
Hum, on the format level I think this need a bit more discussion. the type is better caracterized as a bus informations. Also I'm not sure if we should keep a flat single element or (expecting possible further complex descriptions later) go for a more structured description like
Yes, we need deeper XML structure for the source info
<hostdev type='pci'> <source vendor='0204' product='6025'/> </hostdev>
I think an hypervisor could remap the actual port/addresses of a device so a target may make sense in the future (or not).
Yes, for SCSI at least we need target info, and if we can query the mapped target address from the underlying hypervisor we should display it.
I didn't implement unplug yet since this needs some modifications to qemu/kvm to be able to identify the correct device to unplug. Does this look reasonable?
I also think we need to clarify the naming conventions, are numbers provided decimal, if yes then is an 0x hexadecimal version allowed too. I also see how a more high level description might be useful,
<hostdev type='usb'> <source vendor='Sennheiser'/> </hostdev>
or
<hostdev type='usb'> <source product='headset'/> </hostdev>
We should stick to ID numbers in the domain XML - allowing decimal or hex is nice. For descriptive names 'Sennheiser' or 'headset', we should only provide that info in the device enumeration APIs http://www.redhat.com/archives/libvir-list/2008-April/msg00005.html Management tools / admins can use this to map to the ID number from a human readable name if relevant. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Jul 29, 2008 at 11:56:18AM +0100, Daniel P. Berrange wrote:
On Mon, Jul 28, 2008 at 09:26:57AM -0400, Daniel Veillard wrote:
I also think we need to clarify the naming conventions, are numbers provided decimal, if yes then is an 0x hexadecimal version allowed too. I also see how a more high level description might be useful,
<hostdev type='usb'> <source vendor='Sennheiser'/> </hostdev>
or
<hostdev type='usb'> <source product='headset'/> </hostdev>
We should stick to ID numbers in the domain XML - allowing decimal or hex is nice.
For descriptive names 'Sennheiser' or 'headset', we should only provide that info in the device enumeration APIs
Well, I was thinking of this as a way to keep the XML file static over a range of potential hardware, potentially allowing some kind of migration in spite of a dependancy to a local hardware device. But that's probably not very realistic, and if turn out possible sometimes then that can be added later.
http://www.redhat.com/archives/libvir-list/2008-April/msg00005.html
Management tools / admins can use this to map to the ID number from a human readable name if relevant.
Okay, that solves the problem at creation time, i remember this being discussed no problem. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Fri, Jul 25, 2008 at 04:17:30PM -0400, Guido Günther wrote:
attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via: On top of the hostdev passthrough (but it's actually totally independent) I added usb massstorage backed by a file. Very handy for installer testing where the preseed data is on the USB stick and the CD/DVD is the installation medium. At the moment I'm using a dummy target "usbdisk" so we don't have to check for target == NULL in that many places. Once qemu handles it we can fill in bus and device address for unplugging. To add a file as usb massstorage to the guest you can use:
<disk type='file' device='disk'> <source file='/foo/bar/usbmass.img'/> <target bus='usb'/> </disk> Does this make sense? -- Guido
On Mon, Jul 28, 2008 at 11:02:45AM -0400, Guido G?nther wrote:
On Fri, Jul 25, 2008 at 04:17:30PM -0400, Guido Günther wrote:
attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via: On top of the hostdev passthrough (but it's actually totally independent) I added usb massstorage backed by a file. Very handy for installer testing where the preseed data is on the USB stick and the CD/DVD is the installation medium. At the moment I'm using a dummy target "usbdisk" so we don't have to check for target == NULL in that many places. Once qemu handles it we can fill in bus and device address for unplugging. To add a file as usb massstorage to the guest you can use:
<disk type='file' device='disk'> <source file='/foo/bar/usbmass.img'/> <target bus='usb'/> </disk>
The way we do target for other disk types doesn't work so well - others we typically have a device name, eg hda, sdc, xvdf, etc. For VirtIO we let people pass in a dummy device name too 'vda', 'vdb', etc. Only some of the disk buses & guest OS honour these names though - in cases where they're not honoured, they at most provide a unique key, and allow for ordering of disks in our XML. USB will be much like SCSI / VirtIO where the device name in the XML is merely used for ordering, so we should just allow sda, sdb, etc as the target device name. This will avoid having to special case the code in the here too much.
--- src/domain_conf.c | 40 ++++++++++++++++++++++++++++------------ src/domain_conf.h | 1 + src/qemu_conf.c | 23 +++++++++++++++++++++-- src/qemu_driver.c | 41 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 91 insertions(+), 14 deletions(-)
diff --git a/src/domain_conf.c b/src/domain_conf.c index d36caeb..74ceecc 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -84,7 +84,8 @@ VIR_ENUM_IMPL(virDomainDiskBus, VIR_DOMAIN_DISK_BUS_LAST, "fdc", "scsi", "virtio", - "xen") + "xen", + "usb")
This is fine.
VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "user", @@ -540,6 +541,14 @@ virDomainDiskDefParseXML(virConnectPtr conn, def->device = VIR_DOMAIN_DISK_DEVICE_DISK; }
+ if (bus) { + if ((def->bus = virDomainDiskBusTypeFromString(bus)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown disk bus type '%s'"), bus); + goto error; + } + } +
No need to move this - see further down.
/* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present */ if (source == NULL && @@ -550,10 +559,16 @@ virDomainDiskDefParseXML(virConnectPtr conn, goto error; }
+ /* only USB devices are allowed missing target path since the hypervisor + * can assign bus and device number */ if (target == NULL) { - virDomainReportError(conn, VIR_ERR_NO_TARGET, - source ? "%s" : NULL, source); - goto error; + if (def->bus == VIR_DOMAIN_DISK_BUS_USB) { + target = strdup("usbdisk"); + } else { + virDomainReportError(conn, VIR_ERR_NO_TARGET, + source ? "%s" : NULL, source); + goto error; + }
No need - we'll just define that 'sdXXXX' is the prefix to use.
}
if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && @@ -571,19 +586,14 @@ virDomainDiskDefParseXML(virConnectPtr conn, !STRPREFIX((const char *)target, "hd") && !STRPREFIX((const char *)target, "sd") && !STRPREFIX((const char *)target, "vd") && - !STRPREFIX((const char *)target, "xvd")) { + !STRPREFIX((const char *)target, "xvd") && + !STREQ((const char*)target, "usbdisk")) {
Again, this is not needed.
virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, _("Invalid harddisk device name: %s"), target); goto error; }
- if (bus) { - if ((def->bus = virDomainDiskBusTypeFromString(bus)) < 0) { - virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("unknown disk bus type '%s'"), bus); - goto error; - } - } else { + if (!bus) { if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { def->bus = VIR_DOMAIN_DISK_BUS_FDC; } else { @@ -612,6 +622,12 @@ virDomainDiskDefParseXML(virConnectPtr conn, _("Invalid bus type '%s' for disk"), bus); goto error; } + if (def->device != VIR_DOMAIN_DISK_DEVICE_DISK && + def->bus == VIR_DOMAIN_DISK_BUS_USB) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Invalid bus type '%s' for usb disk"), bus); + goto error; + }
It is possible to have USB floppys and CDROMs. So this check should be made in the underlying drivers at time of use, rather than in the parser. eg, move this into qemudBuildCommandLine() method, or the device hotplug method for QEMU. Similarly for other hypervisor backends as needed.
diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 7678ac5..868b3dc 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -55,7 +55,8 @@ VIR_ENUM_IMPL(virDomainDiskQEMUBus, VIR_DOMAIN_DISK_BUS_LAST, "floppy", "scsi", "virtio", - "xen") + "xen", + "usb")
#define qemudLog(level, msg...) fprintf(stderr, msg) @@ -772,6 +773,13 @@ int qemudBuildCommandLine(virConnectPtr conn, goto no_memory; \ } while (0)
+#define ADD_USBDISK(thisarg) \ + do { \ + ADD_ARG_LIT("-usbdevice"); \ + if ((asprintf(&qargv[qargc++], "disk:%s", thisarg)) == -1) \ + goto no_memory; \ + } while (0) + snprintf(memory, sizeof(memory), "%lu", vm->def->memory/1024); snprintf(vcpus, sizeof(vcpus), "%lu", vm->def->vcpus);
@@ -883,6 +891,12 @@ int qemudBuildCommandLine(virConnectPtr conn, int idx = virDiskNameToIndex(disk->dst); const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus);
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { + ADD_USBDISK(disk->src); + disk = disk->next; + continue; + } +
The indentation is messed up here. Annoying that QEMU's -drive arg doesn't support USB :-(
if (idx < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unsupported disk type '%s'"), disk->dst); @@ -924,6 +938,12 @@ int qemudBuildCommandLine(virConnectPtr conn, char dev[NAME_MAX]; char file[PATH_MAX];
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { + ADD_USBDISK(disk->src); + disk = disk->next; + continue; + }
More indentation issues.
+ if (STREQ(disk->dst, "hdc") && disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (disk->src) { @@ -947,7 +967,6 @@ int qemudBuildCommandLine(virConnectPtr conn,
ADD_ARG_LIT(dev); ADD_ARG_LIT(file); - disk = disk->next; } } diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 3381d10..73b6da4 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2979,6 +2979,44 @@ static int qemudDomainAttachCdromDevice(virDomainPtr dom, return 0; }
+static int qemudDomainAttachUsbMassstorageDevice(virDomainPtr dom, virDomainDeviceDefPtr dev) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid); + int ret; + char *cmd, *reply; + + ret = asprintf(&cmd, "usb_add disk:%s", dev->data.disk->src); + + if (ret == -1) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("out of memory")); + return ret; + }
Minor indentation issue there.
+ + if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("cannot attach usb device")); + VIR_FREE(cmd); + return -1; + } + + DEBUG ("attach_usb reply: %s", reply); + /* If the command failed qemu prints: + * Could not add ... */ + if (strstr(reply, "Could not add ")) { + qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + "%s", + _("adding usb device failed")); + VIR_FREE(reply); + VIR_FREE(cmd); + return -1; + } + VIR_FREE(reply); + VIR_FREE(cmd); + return 0;
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Jul 28, 2008 at 11:02:45AM -0400, Guido G?nther wrote:
On Fri, Jul 25, 2008 at 04:17:30PM -0400, Guido Günther wrote:
attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via: On top of the hostdev passthrough (but it's actually totally independent) I added usb massstorage backed by a file. Very handy for installer testing where the preseed data is on the USB stick and the CD/DVD is the installation medium. At the moment I'm using a dummy target "usbdisk" so we don't have to check for target == NULL in that many places. Once qemu handles it we can fill in bus and device address for unplugging. To add a file as usb massstorage to the guest you can use:
<disk type='file' device='disk'> <source file='/foo/bar/usbmass.img'/> <target bus='usb'/> </disk>
The way we do target for other disk types doesn't work so well - others we typically have a device name, eg hda, sdc, xvdf, etc. For VirtIO we let people pass in a dummy device name too 'vda', 'vdb', etc. Only some of the disk buses & guest OS honour these names though - in cases where they're not honoured, they at most provide a unique key, and allow for ordering of disks in our XML. USB will be much like SCSI / VirtIO where the device name in the XML is merely used for ordering, so we should just allow sda, sdb, etc as the target device name. This will avoid having to special case the code in the here too much. Acutally I intended to use the target like target="bus:device" once qemu (and others) support this. The would also help on detach. Would you
On Wed, Jul 30, 2008 at 09:47:00AM +0100, Daniel P. Berrange wrote: prefer: <target bus='usb' target='sda' bus='0001' id='1234'> then? -- Guido
On Thu, Jul 31, 2008 at 04:17:39AM -0400, Guido G?nther wrote:
On Mon, Jul 28, 2008 at 11:02:45AM -0400, Guido G?nther wrote:
On Fri, Jul 25, 2008 at 04:17:30PM -0400, Guido Günther wrote:
attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via: On top of the hostdev passthrough (but it's actually totally independent) I added usb massstorage backed by a file. Very handy for installer testing where the preseed data is on the USB stick and the CD/DVD is the installation medium. At the moment I'm using a dummy target "usbdisk" so we don't have to check for target == NULL in that many places. Once qemu handles it we can fill in bus and device address for unplugging. To add a file as usb massstorage to the guest you can use:
<disk type='file' device='disk'> <source file='/foo/bar/usbmass.img'/> <target bus='usb'/> </disk>
The way we do target for other disk types doesn't work so well - others we typically have a device name, eg hda, sdc, xvdf, etc. For VirtIO we let people pass in a dummy device name too 'vda', 'vdb', etc. Only some of the disk buses & guest OS honour these names though - in cases where they're not honoured, they at most provide a unique key, and allow for ordering of disks in our XML. USB will be much like SCSI / VirtIO where the device name in the XML is merely used for ordering, so we should just allow sda, sdb, etc as the target device name. This will avoid having to special case the code in the here too much. Acutally I intended to use the target like target="bus:device" once qemu (and others) support this. The would also help on detach. Would you
On Wed, Jul 30, 2008 at 09:47:00AM +0100, Daniel P. Berrange wrote: prefer: <target bus='usb' target='sda' bus='0001' id='1234'>
Taking into account the other thread discussing USB/PCI device passthrough to the guest for host device, I think it'd be worth having an explicit 'address' element for the USB details using a matching syntax, eg <target dev='sda' bus='usb'> <address bus='0001' dev='1234'/> </target> Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hi, attached is a second version of the usb massstorage patch: Changes are: * use sd* dummy dev names instead of usbdisk * move the check for the "subtype" (hard disk/cdrom/...) into the driver An example is: <disk type='file' device='disk'> <target dev="sda" bus='usb'/> <source file='/foo/bar/usbmass.img'/> </disk> Patch is on top of the hostdev passthrough patch. Cheers, -- Guido
On Sun, Aug 03, 2008 at 01:50:44AM +0200, Guido G?nther wrote:
diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 73039c5..331ff9d 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -883,6 +891,13 @@ int qemudBuildCommandLine(virConnectPtr conn, int idx = virDiskNameToIndex(disk->dst); const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus);
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_USB && + disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + ADD_USBDISK(disk->src); + disk = disk->next; + continue; + }
Should do a 'qemudReportError' if the device type != DISK, so if someone mistakenly adds a floppy/cdrom it isn't silently ignored.
if (idx < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unsupported disk type '%s'"), disk->dst); @@ -922,6 +937,13 @@ int qemudBuildCommandLine(virConnectPtr conn, char dev[NAME_MAX]; char file[PATH_MAX];
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_USB && + disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + ADD_USBDISK(disk->src); + disk = disk->next; + continue; + }
Same here.
+ if (STREQ(disk->dst, "hdc") && disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (disk->src) { diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 1aff53e..018046e 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2978,6 +2978,44 @@ static int qemudDomainAttachCdromDevice(virDomainPtr dom, return 0; }
+static int qemudDomainAttachUsbMassstorageDevice(virDomainPtr dom, virDomainDeviceDefPtr dev) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid); + int ret; + char *cmd, *reply; + + ret = asprintf(&cmd, "usb_add disk:%s", dev->data.disk->src); + + if (ret == -1) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("out of memory"));
Should just be qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); As with the other patch we need to have an update to the RNG schema and at least one example test data file in tests/qemuxml2argvdata/ with XML and corresponding ARGV, plumbed into qemuxml2xmltest.c and qemuxml2argvtest.c With those changes I'd be happy to see this committed. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hi, attached is version three of the file backed usb massstorage patch. * handle type != DISK case * on OOM use VIR_ERR_NO_MEMORY instead of VIR_ERR_OPERAION_FAILED * RelaxNG schema update * testcase The patches are ment to be applied on top of the hostdev patches. Cheers, -- Guido
On Thu, Aug 07, 2008 at 07:16:30PM +0200, Guido G?nther wrote:
Hi, attached is version three of the file backed usb massstorage patch.
* handle type != DISK case * on OOM use VIR_ERR_NO_MEMORY instead of VIR_ERR_OPERAION_FAILED * RelaxNG schema update * testcase
The patches are ment to be applied on top of the hostdev patches.
ACK, also looks good to commit Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Guido Günther <agx@sigxcpu.org> wrote:
> attached is version three of the file backed usb massstorage patch.
>
> * handle type != DISK case
> * on OOM use VIR_ERR_NO_MEMORY instead of VIR_ERR_OPERAION_FAILED
> * RelaxNG schema update
> * testcase
...
ACK, once you fix the following:
>>From 123763836d1fe71e6180c36a75690ceec0174f8c Mon Sep 17 00:00:00 2001
> From: Guido Guenther <agx@sigxcpu.org>
> Date: Sun, 3 Aug 2008 01:07:00 +0200
> Subject: [PATCH] usbmass: use files as USB disks
...
> diff --git a/src/qemu_conf.c b/src/qemu_conf.c
...
> +#define ADD_USBDISK(thisarg) \
> + do { \
> + ADD_ARG_LIT("-usbdevice"); \
> + ADD_ARG_SPACE; \
> + if ((asprintf(&qargv[qargc++], "disk:%s", thisarg)) == -1) \
> + goto no_memory; \
Set qargv[qargc-1] = NULL before the "goto", since upon asprintf
failure it's undefined -- and the subsequent attempt to free it
may well evoke a segfault.
> + } while (0)
...
> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> index ef4e158..118d423 100644
> --- a/src/qemu_driver.c
> +++ b/src/qemu_driver.c
> @@ -2978,6 +2978,43 @@ static int qemudDomainAttachCdromDevice(virDomainPtr dom,
> return 0;
> }
>
> +static int qemudDomainAttachUsbMassstorageDevice(virDomainPtr dom, virDomainDeviceDefPtr dev)
> +{
> + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
> + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
This should handle the case in which vm == NULL.
Otherwise, qemudMonitorCommand will dereference NULL.
...
>>From a714b14a13f97b824c288fc40165ef9fe5fbaae0 Mon Sep 17 00:00:00 2001
> From: Guido Guenther <agx@sigxcpu.org>
> Date: Thu, 7 Aug 2008 14:39:35 +0200
> Subject: [PATCH] usbmass: testcase for file as src for usb disk
...
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb.args
> new file mode 100644
> index 0000000..60a8317
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb.args
> @@ -0,0 +1 @@
> +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -usbdevice disk:/tmp/usbdisk.img -net none -serial none -parallel none -usb
> \ No newline at end of file
Please add a newline and split the long line:
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi \
-boot c -hda /dev/HostVG/QEMUGuest1 -usbdevice disk:/tmp/usbdisk.img \
-net none -serial none -parallel none -usb
On Fri, Aug 08, 2008 at 02:07:49PM +0200, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
From a714b14a13f97b824c288fc40165ef9fe5fbaae0 Mon Sep 17 00:00:00 2001 From: Guido Guenther <agx@sigxcpu.org> Date: Thu, 7 Aug 2008 14:39:35 +0200 Subject: [PATCH] usbmass: testcase for file as src for usb disk ... diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-usb.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb.args new file mode 100644 index 0000000..60a8317 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-usb.args @@ -0,0 +1 @@ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -usbdevice disk:/tmp/usbdisk.img -net none -serial none -parallel none -usb \ No newline at end of file
Please add a newline and split the long line:
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi \ -boot c -hda /dev/HostVG/QEMUGuest1 -usbdevice disk:/tmp/usbdisk.img \ -net none -serial none -parallel none -usb
No, this would break the test case, since its being compared against generated data in the format shown by the patch. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Aug 08, 2008 at 02:07:49PM +0200, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
From a714b14a13f97b824c288fc40165ef9fe5fbaae0 Mon Sep 17 00:00:00 2001 From: Guido Guenther <agx@sigxcpu.org> ... Please add a newline and split the long line:
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi \ -boot c -hda /dev/HostVG/QEMUGuest1 -usbdevice disk:/tmp/usbdisk.img \ -net none -serial none -parallel none -usb
No, this would break the test case, since its being compared against generated data in the format shown by the patch.
Ahh... it looked like something to be executed (and maintained), rather than merely expected output. So no need to split long lines. However, including a trailing newline might be worthwhile (and adjust the code to emit the matching newline), since some editors have a hard time modifying or creating a file with no newline at EOF.
Jim Meyering wrote:
On Fri, Aug 08, 2008 at 02:07:49PM +0200, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
From a714b14a13f97b824c288fc40165ef9fe5fbaae0 Mon Sep 17 00:00:00 2001 From: Guido Guenther <agx@sigxcpu.org> ... Please add a newline and split the long line:
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi \ -boot c -hda /dev/HostVG/QEMUGuest1 -usbdevice disk:/tmp/usbdisk.img \ -net none -serial none -parallel none -usb No, this would break the test case, since its being compared against generated data in the format shown by the patch.
Ahh... it looked like something to be executed (and maintained), rather than merely expected output. So no need to split long lines.
However, including a trailing newline might be worthwhile (and adjust the code to emit the matching newline), since some editors have a hard time modifying or creating a file with no newline at EOF.
Heh. Unfortunately, that's not an option either. The tests just do a simple strcmp(), so if there is an extra trailing newline in the args file, the test will "fail". I ran into this before. Chris Lalancette
Chris Lalancette <clalance@redhat.com> wrote:
Jim Meyering wrote:
On Fri, Aug 08, 2008 at 02:07:49PM +0200, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
From a714b14a13f97b824c288fc40165ef9fe5fbaae0 Mon Sep 17 00:00:00 2001 From: Guido Guenther <agx@sigxcpu.org> ... Please add a newline and split the long line:
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi \ -boot c -hda /dev/HostVG/QEMUGuest1 -usbdevice disk:/tmp/usbdisk.img \ -net none -serial none -parallel none -usb No, this would break the test case, since its being compared against generated data in the format shown by the patch.
Ahh... it looked like something to be executed (and maintained), rather than merely expected output. So no need to split long lines.
However, including a trailing newline might be worthwhile (and adjust the code to emit the matching newline), since some editors have a hard time modifying or creating a file with no newline at EOF.
Heh. Unfortunately, that's not an option either. The tests just do a simple strcmp(), so if there is an extra trailing newline in the args file, the test will "fail". I ran into this before.
Maybe you're talking about something else? I proposed to add a trailing newline in the expected output as well as in the code to generate the "actual" output. This is the sort of change I was thinking of: diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d136a13..25465ae 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -53,7 +53,7 @@ static int testCompareXMLToArgvFiles(const char *xml, const char *cmd, int extra goto fail; tmp = argv; - len = 0; + len = 1; /* for trailing newline */ while (*tmp) { len += strlen(*tmp) + 1; tmp++; @@ -68,6 +68,7 @@ static int testCompareXMLToArgvFiles(const char *xml, const char *cmd, int extra strcat(actualargv, *tmp); tmp++; } + strcat(actualargv, "\n"); if (STRNEQ(expectargv, actualargv)) { virtTestDifference(stderr, expectargv, actualargv);
Jim Meyering <jim@meyering.net> wrote:
Maybe you're talking about something else? I proposed to add a trailing newline in the expected output as well as in the code to generate the "actual" output.
This is the sort of change I was thinking of:
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
At first I thought that might be incomplete. But it's all that's needed on the C side. I ran this one-liner to append the missing newlines, and we're all set: for i in $(find|grep '\.args$'); do echo >> $i;done "make check" and "make syntax-check" both pass.
From b548c2f7caf311b7acfa2ebd839067f674d89a40 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 8 Aug 2008 16:18:57 +0200 Subject: [PATCH] tests: append a newline to expected-output files lacking NL-at-EOF
* tests/qemuxml2argvtest.c (testCompareXMLToArgvFiles): Adjust the code that creates "actual" output, so that it too produces a newline-terminated buffer. * tests/qemuxml2argvdata/*.args: Append a newline to each, via: for i in $(find|grep '\.args$'); do echo >> $i;done --- .../qemuxml2argvdata/qemuxml2argv-boot-cdrom.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-boot-floppy.args | 2 +- .../qemuxml2argv-boot-network.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-bootloader.args | 2 +- .../qemuxml2argv-clock-localtime.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-clock-utc.args | 2 +- .../qemuxml2argv-console-compat.args | 2 +- .../qemuxml2argv-disk-cdrom-empty.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-disk-cdrom.args | 2 +- .../qemuxml2argv-disk-drive-boot-cdrom.args | 2 +- .../qemuxml2argv-disk-drive-boot-disk.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-disk-floppy.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-disk-many.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-disk-virtio.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-disk-xenvbd.args | 2 +- .../qemuxml2argv-graphics-sdl.args | 2 +- .../qemuxml2argv-graphics-vnc.args | 2 +- .../qemuxml2argv-input-usbmouse.args | 2 +- .../qemuxml2argv-input-usbtablet.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-input-xen.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-minimal.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-misc-acpi.args | 2 +- .../qemuxml2argv-misc-no-reboot.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-net-user.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-net-virtio.args | 2 +- .../qemuxml2argv-parallel-tcp.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-serial-dev.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-serial-file.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-serial-many.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-serial-pty.args | 2 +- .../qemuxml2argv-serial-tcp-telnet.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-serial-tcp.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-serial-udp.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-serial-unix.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-serial-vc.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-sound.args | 2 +- tests/qemuxml2argvtest.c | 3 ++- 37 files changed, 38 insertions(+), 37 deletions(-) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.args b/tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.args index 3f45b24..6c062ae 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.args @@ -1 +1 @@ -/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot d -cdrom /dev/cdrom -net none -serial none -parallel none -usb \ No newline at end of file +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot d -cdrom /dev/cdrom -net none -serial none -parallel none -usb ... [lots of generated changes elided] ... --- a/tests/qemuxml2argvdata/qemuxml2argv-sound.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound.args @@ -1 +1 @@ -/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -soundhw pcspk,es1370,sb16 \ No newline at end of file +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -soundhw pcspk,es1370,sb16 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d136a13..25465ae 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -53,7 +53,7 @@ static int testCompareXMLToArgvFiles(const char *xml, const char *cmd, int extra goto fail; tmp = argv; - len = 0; + len = 1; /* for trailing newline */ while (*tmp) { len += strlen(*tmp) + 1; tmp++; @@ -68,6 +68,7 @@ static int testCompareXMLToArgvFiles(const char *xml, const char *cmd, int extra strcat(actualargv, *tmp); tmp++; } + strcat(actualargv, "\n"); if (STRNEQ(expectargv, actualargv)) { virtTestDifference(stderr, expectargv, actualargv); -- 1.6.0.rc2.2.g59bf
Jim Meyering <jim@meyering.net> wrote:
Jim Meyering <jim@meyering.net> wrote:
Maybe you're talking about something else? I proposed to add a trailing newline in the expected output as well as in the code to generate the "actual" output.
This is the sort of change I was thinking of:
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
At first I thought that might be incomplete. But it's all that's needed on the C side. I ran this one-liner to append the missing newlines, and we're all set:
for i in $(find|grep '\.args$'); do echo >> $i;done
"make check" and "make syntax-check" both pass.
Planning to commit that, I rebased against the latest, but found "make check" then failed due to three new expected-output ".args" files without the now-required trailing newline. I added them, and now "make check" passes, so I've just committed it.
On Fri, Aug 08, 2008 at 02:07:49PM +0200, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote: [..snip..]
+static int qemudDomainAttachUsbMassstorageDevice(virDomainPtr dom, virDomainDeviceDefPtr dev) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
This should handle the case in which vm == NULL. Otherwise, qemudMonitorCommand will dereference NULL. ... The function gets called from qemudDomainAttachDevice which does the same call to virDomainFindbyUUid and checks for vm == NULL already but you're right: better safe than sorry.
[..snip..]
Please add a newline and split the long line:
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi \ -boot c -hda /dev/HostVG/QEMUGuest1 -usbdevice disk:/tmp/usbdisk.img \ -net none -serial none -parallel none -usb I'd rather not since this breaks the testcase, it compares the exact output.
New patches attached. Patches apply on top of the hostdev patches. -- Guido
On Fri, Aug 08, 2008 at 04:04:17PM +0200, Guido Günther wrote:
New patches attached. Patches apply on top of the hostdev patches.
All commited, I just removed an extra space at end of line :-) thanks a lot ! Again a small patch to the doc describing it would fix the last weak spot, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Fri, Jul 25, 2008 at 04:17:30PM -0400, Guido G?nther wrote:
Hi, attached is some basic support for host device passthrough. It enables you to passthrough usb devices in qemu/kvm via:
<devices> <hostdev type='usb' vendor='0204' product='6025'/> <hostdev type='usb' bus='001' device='007'/> </devices>
This stuff is obviously going to have a correlation with the host device enumeration support I'd offered a design for a few months back. As such I'd like to try and keep a consistent XML format between the two. For reference the original mesage was here: http://www.redhat.com/archives/libvir-list/2008-April/msg00005.html There were basically two ways to identify a device. Some devices are 'physical' mapped straight to a piece of hardware (USB device, or PCI card) and would have '<bus>' element with hardware details. eg a USB finger print reader appears as: <device> <name>usb_device_483_2016_noserial</name> <key>/org/freedesktop/Hal/devices/usb_device_483_2016_noserial</key> <parent>/org/freedesktop/Hal/devices/usb_device_0_0_0000_00_1d_3</parent> <bus type="usb"> <vendor id="1155">SGS Thomson Microelectronics</vendor> <product id="8214">Fingerprint Reader</product> <address bus="003" dev="005"/> </bus> </device> Other devices are 'logical' devices, which don't have hardware info directly associated with them. The reason for this is that one piece of hardware may present many logical devices each with varying capabilities. As an example, a wifi card typically exports at least 2 network device - one control interface, and one for traffic. eg a wireless network interface for data traffic <device> <name>net_00_13_02_b9_f9_d3_0</name> <key>/org/freedesktop/Hal/devices/net_00_13_02_b9_f9_d3_0</key> <parent>/org/freedesktop/Hal/devices/pci_8086_4227</parent> <capability type="net"> <hwaddr>00:13:02:b9:f9:d3</hwaddr> <name>eth0</name> <capability type="80211"/> </capability> </device> In this case the unique device identifier is the '<name>' field but this case varying depending on the capability type. Different virt solutions have different capabilties for device passthrough. KVM and Xen both support passthrough of physical devices, either USB or PCI cards. OpenVZ supports passthrough of logical devices - in particular network interfaces. We need to allow for both possibilities in the domain XML for this type of device. So, to expand on your proposal, I'd like to see the XML format have a top level attribute indicating whether we're passing a logical or physical device, and then the capability name or bus name respectively. The child elements then need to provide the appropriate naming. USB has the further annoyance you identified that one could conceivably do attachment based on USB bus address, or the vendor+product pair. The downside of former is that a bus address changes every time you plug a device in. The downside of the latter is that it is not neccessarily unique. We have no choice but to allow both I'm afraid :-( Finally, with some systems we may have the option of specifying a target address - eg PCI device ID seen in guest. So, some examples.... A USB device by vendor+product <hostdev mode='bus' bus='usb'> <source> <product id='1155'/> <vendor id='8214'/> </source> </hostdev> A USB device by address <hostdev mode='bus' bus='usb'> <source> <address bus='003' dev='005'/> </source> </hostdev> A PCI device by address <hostdev mode='bus' bus='pci'> <source> <address domain="0000" bus="00" slot="16" function="3"/> </source> </hostdev> A network card by name (ie for OpenVZ) <hostdev mode='capability'> <source name='eth0'/> </hostdev> A SCSI device by name (eg, SCSI PV passthrough), also specifying the target adress <hostdev mode='capability' type='scsi'> <source name='sg3'/> <target address='0:0:0:0'/> </hostdev> Conceivably we could allow PCI devices by vendor+product, but I don't see much call for that since PCI device's don't (yet) appear/disappear on the fly & have a consistent address. More to the point none of our underlying hypervisors use anything other than the PCI address for PCI device passthrough. For USB, if we're doing attachment based on vendor+product, then libvirt needs to query QEMU to find out the actual device it chose, so we can fill in the <address> tag. NB I know QEMU doesn't allow this, but we need it in order todo unplug reliably, so we'll likely need to do it anyway.
diff --git a/src/domain_conf.h b/src/domain_conf.h index b438bc8..1aa5c39 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -257,7 +257,35 @@ struct _virDomainGraphicsDef { } data; };
+enum virDomainHostdevType { + VIR_DOMAIN_HOSTDEV_TYPE_USB, + VIR_DOMAIN_HOSTDEV_TYPE_PCI,
+ VIR_DOMAIN_HOSTDEV_TYPE_LAST +}; + +typedef struct _virDomainHostdevDef virDomainHostdevDef; +typedef virDomainHostdevDef *virDomainHostdevDefPtr; +struct _virDomainHostdevDef { + int type; + union { + struct { + int byModel; + union { + unsigned vendor; + unsigned bus; + }; + union { + unsigned product; + unsigned device; + }; + } usb; + struct { + /* TBD */ + } pci; + }; + virDomainHostdevDefPtr next; +};
Taking into account the various options we need to cope with I think we'll need something a little larger, along the lines of enum virDomainHostdevMode { VIR_DOMAIN_HSOTDEV_MODE_BUS, VIR_DOMAIN_HSOTDEV_MODE_CAPABILITY, }; enum virDomainHostdevBusType VIR_DOMAIN_HSOTDEV_BUS_TYPE_PCI, VIR_DOMAIN_HSOTDEV_BUS_TYPE_USB, }; enum virDomainHostdevCapabilityType { VIR_DOMAIN_HSOTDEV_CAP_TYPE_NET, VIR_DOMAIN_HSOTDEV_CAP_TYPE_SCSI, }; struct _virDomainHostdevDef { int mode; /* enum virDomainHostdevMode */ union { struct { int type; /* enum virDomainHostdevBusType */ union { struct { unsigned bus; unsigned device; unsigned vendor; unsigned product; } usb; struct { unsigned domain; unsigned bus; unsigned slot; unsigned function; } pci; } data; } bus; struct { int type; /* enum virDomainHostdevCapabilityType */ union { struct { char *name; } net; struct { char *name; } scsi; }; } cap; } source; char *target; };
/* Flags for the 'type' field in next struct */ enum virDomainDeviceType { @@ -265,6 +293,7 @@ enum virDomainDeviceType { VIR_DOMAIN_DEVICE_NET, VIR_DOMAIN_DEVICE_INPUT, VIR_DOMAIN_DEVICE_SOUND, + VIR_DOMAIN_DEVICE_HOST, };
typedef struct _virDomainDeviceDef virDomainDeviceDef; @@ -276,6 +305,7 @@ struct _virDomainDeviceDef { virDomainNetDefPtr net; virDomainInputDefPtr input; virDomainSoundDefPtr sound; + virDomainHostdevDefPtr hostdev;
Careful with indentation - check the HACKING file for an emacs whitespace rule setting. Modulo, the comments about the XML format, I think your patch is basically sound & following all our guidelines which is great :-) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Jul 29, 2008 at 11:53:46AM +0100, Daniel P. Berrange wrote:
This stuff is obviously going to have a correlation with the host device enumeration support I'd offered a design for a few months back. As such I'd like to try and keep a consistent XML format between the two. For reference the original mesage was here:
http://www.redhat.com/archives/libvir-list/2008-April/msg00005.html
There were basically two ways to identify a device. Some devices are 'physical' mapped straight to a piece of hardware (USB device, or PCI card) and would have '<bus>' element with hardware details.
Oh, one thing I meant to say. Contrary to my message in the thread above, and my previous reply, we should actually use 'subsys' or 'subsystem' instead of 'bus', to follow the HAL naming.
eg a USB finger print reader appears as:
<device> <name>usb_device_483_2016_noserial</name> <key>/org/freedesktop/Hal/devices/usb_device_483_2016_noserial</key> <parent>/org/freedesktop/Hal/devices/usb_device_0_0_0000_00_1d_3</parent>
<bus type="usb">
eg <subsystem type='usb'>
<vendor id="1155">SGS Thomson Microelectronics</vendor> <product id="8214">Fingerprint Reader</product>
<address bus="003" dev="005"/> </bus> </device>
Other devices are 'logical' devices, which don't have hardware info directly associated with them. The reason for this is that one piece of hardware may present many logical devices each with varying capabilities. As an example, a wifi card typically exports at least 2 network device - one control interface, and one for traffic.
eg a wireless network interface for data traffic
<device> <name>net_00_13_02_b9_f9_d3_0</name> <key>/org/freedesktop/Hal/devices/net_00_13_02_b9_f9_d3_0</key> <parent>/org/freedesktop/Hal/devices/pci_8086_4227</parent>
<capability type="net"> <hwaddr>00:13:02:b9:f9:d3</hwaddr> <name>eth0</name>
<capability type="80211"/> </capability> </device>
In this case the unique device identifier is the '<name>' field but this case varying depending on the capability type.
Different virt solutions have different capabilties for device passthrough. KVM and Xen both support passthrough of physical devices, either USB or PCI cards. OpenVZ supports passthrough of logical devices - in particular network interfaces.
We need to allow for both possibilities in the domain XML for this type of device.
So, to expand on your proposal, I'd like to see the XML format have a top level attribute indicating whether we're passing a logical or physical device, and then the capability name or bus name respectively. The child elements then need to provide the appropriate naming.
USB has the further annoyance you identified that one could conceivably do attachment based on USB bus address, or the vendor+product pair. The downside of former is that a bus address changes every time you plug a device in. The downside of the latter is that it is not neccessarily unique. We have no choice but to allow both I'm afraid :-(
Finally, with some systems we may have the option of specifying a target address - eg PCI device ID seen in guest.
So, some examples....
A USB device by vendor+product
<hostdev mode='bus' bus='usb'>
<hostdev mode='subsystem' type='usb'>
<source> <product id='1155'/> <vendor id='8214'/> </source> </hostdev>
A USB device by address
<hostdev mode='bus' bus='usb'>
<hostdev mode='subsystem' type='usb'>
<source> <address bus='003' dev='005'/> </source> </hostdev>
A PCI device by address
<hostdev mode='bus' bus='pci'>
<hostdev mode='subsystem' type='pci'>
<source> <address domain="0000" bus="00" slot="16" function="3"/> </source> </hostdev>
A network card by name (ie for OpenVZ)
<hostdev mode='capability'> <source name='eth0'/> </hostdev>
A SCSI device by name (eg, SCSI PV passthrough), also specifying the target adress
<hostdev mode='capability' type='scsi'> <source name='sg3'/> <target address='0:0:0:0'/> </hostdev>
Taking into account the various options we need to cope with I think we'll need something a little larger, along the lines of
enum virDomainHostdevMode { VIR_DOMAIN_HSOTDEV_MODE_BUS, VIR_DOMAIN_HSOTDEV_MODE_CAPABILITY, };
enum virDomainHostdevBusType VIR_DOMAIN_HSOTDEV_BUS_TYPE_PCI, VIR_DOMAIN_HSOTDEV_BUS_TYPE_USB, };
s/BUS/SUBSYS/
enum virDomainHostdevCapabilityType { VIR_DOMAIN_HSOTDEV_CAP_TYPE_NET, VIR_DOMAIN_HSOTDEV_CAP_TYPE_SCSI, };
struct _virDomainHostdevDef { int mode; /* enum virDomainHostdevMode */ union { struct { int type; /* enum virDomainHostdevBusType */ union { struct { unsigned bus; unsigned device;
unsigned vendor; unsigned product; } usb; struct { unsigned domain; unsigned bus; unsigned slot; unsigned function; } pci; } data; } bus;
s/bus/subsys/
struct { int type; /* enum virDomainHostdevCapabilityType */ union { struct { char *name; } net; struct { char *name; } scsi; }; } cap; } source; char *target; };
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hi, attached is a second version. Changes are: * s/bus/subsystem/ * support hexadecimal and decimal attributes * introduce device and source elements. I decided to not drop vendor and product id into their own elements since the structure would then become very nested for no good reason. Some examples: <hostdev mode="subsys" type='usb'> <source> <device vendor="0x0204" product="0x6025"/> </source> </hostdev> <hostdev mode="subsys" type='usb'> <source> <address bus="001" device="003"/> </source> </hostdev> Support for <target> will be coming once I got around to adjust qemu/kvm. Cheers, -- Guido
On Sun, Aug 03, 2008 at 01:41:28AM +0200, Guido G?nther wrote:
Hi, attached is a second version. Changes are:
* s/bus/subsystem/ * support hexadecimal and decimal attributes * introduce device and source elements.
I decided to not drop vendor and product id into their own elements since the structure would then become very nested for no good reason.
The reason is that I want it to match the host device enumeration XML format. This will be using explicit <product> and <vendor> tags, with an 'id' attribute, because there will be text content in the body giving the human readable name.
Some examples:
<hostdev mode="subsys" type='usb'> <source> <device vendor="0x0204" product="0x6025"/>
So this needs to be changed to <vendor id="0x0204"/> <product id="0x6025"/>
</source> </hostdev>
<hostdev mode="subsys" type='usb'> <source> <address bus="001" device="003"/> </source> </hostdev>
This one is fine.
Support for <target> will be coming once I got around to adjust qemu/kvm.
Ok, that's not critical, but the other things we need before we can commit this are - Code to format XML for output, so that the devices are included when dumping the XML description of a guest. - Update to the RNG schema in docs/libvirt.rng - Example XML data files in tests/qemuxml2argvdata/, along with the corresponding CLI args for QEMU. THis is then hooked into the qemuxml2argvtest.c and qemuxml2xmltest.c files The latter test suite requires point the XML formatting & RNG updates. The currrent code you have for parsing XML & attaching device/creating CLI flags is basically correct with only very minor bugs
+static int +virDomainHostdevSubsysUsbDefParseXML(virConnectPtr conn, + const xmlNodePtr node, + virDomainHostdevDefPtr def) {
Minor indentation bug.
+ + int ret = -1; + xmlNodePtr cur; + + def->source.subsys.usb.vendor = 0; + def->source.subsys.usb.product = 0; + def->source.subsys.usb.bus = 0; + def->source.subsys.usb.device = 0;
This isn't needed, since the VIR_ALLOC contract says that the memory range has been memset() to all zero.
+static int qemudDomainAttachHostDevice(virDomainPtr dom, virDomainDeviceDefPtr dev) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid); + int ret; + char *cmd, *reply; + + if (dev->data.hostdev->source.subsys.usb.vendor) { + ret = asprintf(&cmd, "usb_add host:%.4x:%.4x", + dev->data.hostdev->source.subsys.usb.vendor, + dev->data.hostdev->source.subsys.usb.product); + } else { + ret = asprintf(&cmd, "usb_add host:%.3d.%.3d", + dev->data.hostdev->source.subsys.usb.bus, + dev->data.hostdev->source.subsys.usb.device); + } + if (ret == -1) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("out of memory")); + return -1;
This should be reporting OOM, and not pass the 'dom' parameter, and no need for a string message, eg qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Aug 04, 2008 at 10:34:31AM +0100, Daniel P. Berrange wrote:
On Sun, Aug 03, 2008 at 01:41:28AM +0200, Guido G?nther wrote:
Hi, attached is a second version. Changes are:
* s/bus/subsystem/ * support hexadecimal and decimal attributes * introduce device and source elements.
I decided to not drop vendor and product id into their own elements since the structure would then become very nested for no good reason.
The reason is that I want it to match the host device enumeration XML format. This will be using explicit <product> and <vendor> tags, with an 'id' attribute, because there will be text content in the body giving the human readable name. Ahh...o.k.
Some examples:
<hostdev mode="subsys" type='usb'> <source> <device vendor="0x0204" product="0x6025"/>
So this needs to be changed to
<vendor id="0x0204"/> <product id="0x6025"/>
Sure.
Ok, that's not critical, but the other things we need before we can commit this are
- Code to format XML for output, so that the devices are included when dumping the XML description of a guest. - Update to the RNG schema in docs/libvirt.rng - Example XML data files in tests/qemuxml2argvdata/, along with the corresponding CLI args for QEMU. THis is then hooked into the qemuxml2argvtest.c and qemuxml2xmltest.c files Sure. I delayed those since they'd have to be adjusted when the XML changes, I'll add them now and do the other minor touchups you suggested...might be a couple of days though. -- Guido
Hi, attached is version three of the hostdev passthrough patch. It adds: * code to format the XML for output * RelaxNG schema update * testcases Cheers, -- Guido
On Thu, Aug 07, 2008 at 06:25:56PM +0200, Guido G?nther wrote:
Hi, attached is version three of the hostdev passthrough patch. It adds:
* code to format the XML for output * RelaxNG schema update * testcases
ACK, this looks ready to commit now - great work getting this done. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Guido Günther <agx@sigxcpu.org> wrote:
attached is version three of the hostdev passthrough patch. It adds:
* code to format the XML for output * RelaxNG schema update * testcases
ACK, modulo the details below.
From cfcfc85accdcc7be7a5fbfd2c8dde435646d5ab2 Mon Sep 17 00:00:00 2001 From: Guido Guenther <agx@sigxcpu.org> Date: Fri, 25 Jul 2008 15:18:16 -0400 Subject: [PATCH] hostdev: pass host devices to the guest ... diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 7fe3903..ef4e158 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2951,12 +2951,83 @@ static int qemudDomainChangeCDROM(virDomainPtr dom, return 0; }
+static int qemudDomainAttachCdromDevice(virDomainPtr dom, + virDomainDeviceDefPtr dev) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid); + virDomainDiskDefPtr disk; + + disk = vm->def->disks;
Check for vm == NULL before dereferencing it. ...
+static int qemudDomainAttachHostDevice(virDomainPtr dom, virDomainDeviceDefPtr dev) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid); ... + if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
Likewise. ...
From 6fd13e2faa49eadf2db4a7e4a3a28192b4c61622 Mon Sep 17 00:00:00 2001 From: Guido Guenther <agx@sigxcpu.org> Date: Thu, 7 Aug 2008 14:25:50 +0200 Subject: [PATCH] hostdev: add testcases ... diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args new file mode 100644 index 0000000..0b89999 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args @@ -0,0 +1 @@ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 \ No newline at end of file
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args new file mode 100644 index 0000000..b993ae5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args @@ -0,0 +1 @@ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:0204:6025 \ No newline at end of file
Please split the long lines and add a newline at EOF.
On Fri, Aug 08, 2008 at 02:30:15PM +0200, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote: [..snip..]
+static int qemudDomainAttachCdromDevice(virDomainPtr dom, + virDomainDeviceDefPtr dev) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid); + virDomainDiskDefPtr disk; + + disk = vm->def->disks;
Check for vm == NULL before dereferencing it.
...
+static int qemudDomainAttachHostDevice(virDomainPtr dom, virDomainDeviceDefPtr dev) +{ + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid); ... + if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
Likewise. The same applies here: qemudDomainAttachDevice checked that already, but again - better safe than sorry. Updated patches attached. -- Guido
On Fri, Aug 08, 2008 at 04:05:10PM +0200, Guido Günther wrote:
Likewise. The same applies here: qemudDomainAttachDevice checked that already, but again - better safe than sorry. Updated patches attached.
Okidoc, all look good ! Applied and commited to CVS, thanks a lot for the work, I think the only thing missing is extending the descrition in the documentation would you mind adding a description in formatdomain.html(.in) Probably an "USB devices" section added below the elementsDisks part, explaining the syntaxes and stating that it was added in versions after 0.4.4 If you don't feel comfortable with that I can do it, but you know better :) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi, On Fri, Aug 08, 2008 at 10:34:00AM -0400, Daniel Veillard wrote: [..snip..]
I think the only thing missing is extending the descrition in the documentation would you mind adding a description in formatdomain.html(.in) Probably an "USB devices" section added below the elementsDisks part, explaining the syntaxes and stating that it was added in versions after 0.4.4
If you don't feel comfortable with that I can do it, but you know better :) Attached is a short patch for this. This might be a bit terse but we'll have to change this anyhow once pci passthrough for xen arrives. Cheers, -- Guido
On Fri, Aug 08, 2008 at 05:59:56PM +0200, Guido Günther wrote:
Hi, On Fri, Aug 08, 2008 at 10:34:00AM -0400, Daniel Veillard wrote: [..snip..]
I think the only thing missing is extending the descrition in the documentation would you mind adding a description in formatdomain.html(.in) Probably an "USB devices" section added below the elementsDisks part, explaining the syntaxes and stating that it was added in versions after 0.4.4
If you don't feel comfortable with that I can do it, but you know better :) Attached is a short patch for this. This might be a bit terse but we'll have to change this anyhow once pci passthrough for xen arrives. Cheers,
That's fine, just fixed a couple of typos and commited, thanks a lot :-) ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (5)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Guido Günther -
Jim Meyering