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

Martin Kletzander mkletzan at redhat.com
Fri Jul 24 12:42:30 UTC 2015


On Fri, Jul 24, 2015 at 08:01:05AM -0400, Laine Stump wrote:
>On 07/24/2015 07:28 AM, Martin Kletzander wrote:
>> 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.
>
>
>Using "model type" is based on the existing "model type" for
><interface> and <video>. I also think "model name" is more fitting, but
>it breaks with precedent. Should we knowingly be inconsistent in order
>to make the new one better? I'm undecided.
>

We already have a model name='' too ;)  Although it's not for devices,
it's for a cpu...  To be honest, I don't care, I'm rather thinking
about how to make changes for inconsistencies work in the future
without messing up the code, but with keeping the back-compat.  I'm
still naive enough that I believe there has to be a way.

>
>>
>>>> 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.
>
>Again, I used a string because that's the way it is in existing code
>(interface), but will change it to an enum for efficiency's sake. (I'm
>curious what problem you're refering to though - is it the fact that
>qemuBuildNicDevStr() doesn't qualify the string before using it in the
>commandline? That is taken care of in the case of this code.)
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150724/5a3fc93e/attachment-0001.sig>


More information about the libvir-list mailing list