Re: [libvirt] [PATCH 3/9] Add new domain device: "controller"
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 :|