Re: [libvirt] [PATCHv2 05/17] conf: add new <model> subelement with type attribute to <controller>

On Thu, Jul 23, 2015 at 03:18:04PM +0200, Ján Tomko wrote: > On Fri, Jul 17, 2015 at 02:43:32PM -0400, Laine Stump wrote: >> This new subelement is used in PCI controllers: the toplevel >> *attribute* "model" of a controller denotes what kind of PCI >> controller is being described, e.g. a "dmi-to-pci-bridge", >> "pci-bridge", or "pci-root". But in the future there will be different >> implementations of some of those types of PCI controllers, which >> behave similarly from libvirt's point of view (and so should have the >> same model), but use a different device in qemu (and present >> themselves as a different piece of hardware in the guest). In an ideal >> world we (i.e. "I") would have thought of that back when the pci >> controllers were added, and used some sort of type/class/model >> notation (where class was used in the way we are now using model, and >> model was used for the actual manufacturer's model number of a >> particular family of PCI controller), but that opportunity is long >> past, so as an alternative, this patch allows selecting a particular >> implementation of a pci controller with the "type" attribute of the >> <model> subelement, e.g.: >> >> <controller type='pci' model='dmi-to-pci-bridge' index='1'> >> <model type='i82801b11-bridge'/> >> </controller> >> > > I'd say 'type' would be more fitting for the generic class of the device > (dmi-to-pci-bridge), not the exact device model (i82801b11-bridge) so > <model name='i82801b11-bridge'/> looks nicer to me, but I'm not going to > suggest it again. > Well, you just did. But I must say I like "model name" better too. When I pitched the "model type" I did that to show the creation of the new element, not the spelling itself.

>> In this case, "dmi-to-pci-bridge" is the kind of controller (one that >> has a single PCIe port upstream, and 32 standard PCI ports downstream, >> which are not hotpluggable), and the qemu device to be used to >> implement this kind of controller is named "i82801b11-bridge". >> >> Implementing the above now will allow us in the future to add a new >> kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge >> device, but instead uses something else (which doesn't yet exist, but >> qemu people have been discussing it), all without breaking existing >> configs. >> >> (note that for the existing "pci-bridge" type of PCI controller, both >> the model attribute and <model> type are 'pci-bridge'. This is just a >> coincidence, since it turns out that in this case the device name in >> qemu really is a generic 'pci-bridge' rather than being the name of >> some real-world chip) >> --- >> new in V2 (previously was a part of the patch to add pcie-root-port) >> >> docs/formatdomain.html.in | 12 ++++++++++++ >> docs/schemas/domaincommon.rng | 13 +++++++++++++ >> src/conf/domain_conf.c | 23 >> +++++++++++++++++++++-- >> src/conf/domain_conf.h | 8 ++++++++ >> tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 ++++++-- >> tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 ++++++-- >> 6 files changed, 66 insertions(+), 6 deletions(-) > >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h >> index 50750c1..09fe3c0 100644 >> --- a/src/conf/domain_conf.h >> +++ b/src/conf/domain_conf.h >> @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts >> *virDomainPCIControllerOptsPtr; >> struct _virDomainPCIControllerOpts { >> bool pcihole64; >> unsigned long pcihole64size; >> + >> + /* the type is in the "model" subelement, e.g.: >> + * <controller type='pci' model='pcie-root-port'> >> + * <model type='ioh3420''/> >> + * ... >> + * similar to the model of <interface> devices. >> + */ >> + char *type; /* the exact name of the device in hypervisor */ > > This would be nicer as an enum - we don't allow arbitrary strings there > anyway. > The only place where we allow arbitrary strings is the interface model and due to that we're having some problems. Not allowing strings is good, storing the enum value is even better.