On Fri, Sep 18, 2009 at 05:26:10PM +0200, Wolfgang Mauerer wrote:
This augments virDomainDevice with a <controller> element that is used to represent disk controllers (e.g., scsi controllers). The XML format is given by
<controller type="scsi" id="<my_id>"> <bus addr="<Domain>:<Bus>:<Slot>"> </controller>
where type denotes the disk interface (scsi, ide,...), id is an arbitrary string that identifies the controller for disk hotadd/remove, and bus addr denotes the controller address on the PCI bus.
The bus element can be omitted; in this case, an address will be assigned automatically. Currently, only hotplugging scsi controllers on a PCI bus is supported, and this only for qemu guests
As mentioned in the previous patch, I reckon 'id' is better called 'name'. For PCI addresses, it is desirable to fully normalize the XML format, by which I mean have separate attributes for domain, bus and slot. We already have a syntax for PCI addresses used for host device passthrough, so it'd make sense to use the same syntax here for controllers. More broadly, we're probably going to have to add a PCI address element to all our devices. While it is unlikely we'll need non-PCI addresses, it doesn't hurt to make it explicit by adding a type='pci' attribute Thus I'd suggest <address type='pci' domain='0x0000' bus='0x06' slot='0x12'/> Instead of <bus addr="<Domain>:<Bus>:<Slot>"> In the domain_conf.c/.h parser, we could have a datatype like enum { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST }; typedef struct _virDomainDevicePCIAddress virDomainDevicePCIAddress; struct _virDomainDevicePCIAddress { unsigned domain; unsigned bus; unsigned slot; unsigned function; }; typedef struct _virDomainDeviceAddress virDomainDeviceAddress; struct _virDomainDeviceAddress { int type; union { virDomainDevicePCIAddress pci; } data; }; Which we then use in all the places in domain_conf.h wanting address information. Obviously we'd not use the 'function' field in most places, but doesn't hurt to have it. And a pair of methods for parsing/formatting this address info we can call from all the appropriate locations.
diff --git a/src/domain_conf.h b/src/domain_conf.h index 898f6c9..6b3cb09 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -111,6 +111,11 @@ struct _virDomainDiskDef { char *src; char *dst; char *controller_id; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } controller_pci_addr;
I think we should stick to just using the controller name as the mandatory identifier for cross-referencing disks to controllers.
char *driverName; char *driverType; char *serial; @@ -125,6 +130,19 @@ struct _virDomainDiskDef { virStorageEncryptionPtr encryption; };
+/* Stores the virtual disk controller configuration */ +typedef struct _virDomainControllerDef virDomainControllerDef; +typedef virDomainControllerDef *virDomainControllerDefPtr; +struct _virDomainControllerDef { + int type; + char *id; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } pci_addr; +};
With the generic address data type and s/id/name/, this would be just struct _virDomainControllerDef { int type; char *name; virDomainDeviceAddress addr; }; 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 :|