Re: [libvirt] [PATCH 3/9] Add new domain device: "controller"

Daniel P. Berrange wrote: > 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. using a generic format for PCi addresses surely makes sense, especially wrt. the common data structure and parsing routines - I'll add these into the next series. However, I'm not sure if using <address type='pci' domain='0x0000' bus='0x06' slot='0x12'/> instead of a simple string provides much of an advantage and does not just increase the typing strain (though there are maybe some small advantages for computerised parsing of libvirt configuration files by other apps).

There's already a generic definition of PCI addresses in docs/schemas/domain.rng:\approx 1140, I suppose it's your intention to extend if by a type field?