[libvirt] [PATCHv3 02/13] qemu: implement <model> subelement to <controller>
Martin Kletzander
mkletzan at redhat.com
Mon Aug 10 08:19:19 UTC 2015
On Sun, Aug 09, 2015 at 01:11:27AM -0400, Laine Stump wrote:
>On 08/03/2015 06:05 AM, Martin Kletzander wrote:
>> [I reduced the Cc list so I don't need to hear jtomko's whining again]
>>
>> On Sat, Jul 25, 2015 at 03:58:26PM -0400, Laine Stump wrote:
>>> This patch provides qemu support for the contents of <model> in
>>> <controller> for the two existing PCI controller types that need it
>>> (i.e. the two controller types that are backed by a device that must
>>> be specified on the qemu commandline):
>>>
>>> 1) pci-bridge - sets <model> name attribute default as "pci-bridge"
>>>
>>> 2) dmi-to-pci-bridge - sets <model> name attribute default as
>>> "i82801b11-bridge".
>>>
>>> These both match current hardcoded practice.
>>>
>>> The defaults are set at the end of qemuDomainAssignPCIAddresses(), It
>>> can't be done earlier because some of the options that will be
>>> autogenerated need full PCI address info for the controller and
>>> because qemuDomainAssignPCIAddresses() might create extra controllers
>>> which would need default settings added, and that hasn't been done at
>>> the time the PostParse callbacks are being
>>> run. qemuDomainAssignPCIAddresses() is still prior to the XML being
>>> written to disk, though, so the autogenerated defaults are persistent.
>>>
>>> qemu capabilities bits aren't checked until the commandline is
>>> actually created (so the domain can possibly be defined on a host that
>>> doesn't yet have support for the give n device, or a host different
>>> from the one where it will eventually be run). At that time we compare
>>> the type strings to known qemu device names and check for the
>>> capabilities bit for that device.
>>> ---
>>>
>>> Changes from V2: use enum instead of string model name.
>>>
>>> src/qemu/qemu_command.c | 81
>>> ++++++++++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 77 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 09f30c4..6a19d15 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -2251,11 +2251,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr
>>> def,
>>> virDomainControllerDefPtr cont = def->controllers[i];
>>> int idx = cont->idx;
>>> virDevicePCIAddressPtr addr;
>>> + virDomainPCIControllerOptsPtr options;
>>>
>>> if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>>> continue;
>>>
>>> addr = &cont->info.addr.pci;
>>> + options = &cont->opts.pciopts;
>>> +
>>> + /* set defaults for any other auto-generated config
>>> + * options for this controller that haven't been
>>> + * specified in config.
>>> + */
>>> + switch ((virDomainControllerModelPCI)cont->model) {
>>> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>>> + if (options->modelName ==
>>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE)
>>> + options->modelName =
>>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE;
>>> + break;
>>> + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
>>> + if (options->modelName ==
>>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE)
>>> + options->modelName =
>>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE;
>>> + break;
>>> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
>>> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
>>> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
>>> + break;
>>> + }
>>> +
>>> /* check if every PCI bridge controller's ID is
>>> greater than
>>> * the bus it is placed onto
>>> */
>>> @@ -4505,6 +4527,7 @@ qemuBuildControllerDevStr(virDomainDefPtr
>>> domainDef,
>>> {
>>> virBuffer buf = VIR_BUFFER_INITIALIZER;
>>> int model = def->model;
>>> + const char *modelName = NULL;
>>>
>>> if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
>>> if ((qemuSetSCSIControllerModel(domainDef, qemuCaps, &model))
>>> < 0)
>>> @@ -4626,17 +4649,67 @@ qemuBuildControllerDevStr(virDomainDefPtr
>>> domainDef,
>>> }
>>> switch (def->model) {
>>> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>>> - virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s",
>>> - def->idx, def->info.alias);
>>> + if (def->opts.pciopts.modelName
>>> + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> + _("autogenerated pci-bridge options
>>> not set"));
>>> + goto error;
>>> + }
>>> +
>>> + modelName =
>>> virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName);
>>> + if (!modelName) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("unknown pci-bridge model name
>>> value %d"),
>>> + def->opts.pciopts.modelName);
>>> + goto error;
>>> + }
>>> + if (def->opts.pciopts.modelName
>>> + != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> + _("PCI controller model name '%s' "
>>> + "is not valid for a pci-bridge"),
>>> + modelName);
>>> + goto error;
>>> + }
>>> + if (!virQEMUCapsGet(qemuCaps,
>>> QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("the pci-bridge controller "
>>> + "is not supported in this QEMU
>>> binary"));
>>> + goto error;
>>> + }
>>> + virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s",
>>> + modelName, def->idx, def->info.alias);
>>> break;
>>> case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
>>> + if (def->opts.pciopts.modelName
>>> + == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> + _("autogenerated dmi-to-pci-bridge
>>> options not set"));
>>> + goto error;
>>> + }
>>> +
>>> + modelName =
>>> virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName);
>>> + if (!modelName) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("unknown dmi-to-pci-bridge model
>>> name value %d"),
>>> + def->opts.pciopts.modelName);
>>> + goto error;
>>> + }
>>> + if (def->opts.pciopts.modelName
>>> + !=
>>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> + _("PCI controller model name '%s' "
>>> + "is not valid for a
>>> dmi-to-pci-bridge"),
>>> + modelName);
>>> + goto error;
>>> + }
>>
>> I see why you didn't care whether it will be implemented this way or
>> the other. That's because you're checking for stuff we usually leave
>> the parsing to handle.
>
>Checking whether the particular modelName given in the XML (or
>auto-assigned later) is supported by this QEMU isn't something that's
>done by the parser or post-parse callbacks. It's done while we're
>building the commandline because it's then that we have accurate and
>up-to-date capability info.
>
I meant all the checks except the one you mentioned :) Anyway, as I
said, there is no problem keeping it this way. I just wanted to say
that some space could be saved with methods similar to the ones used
with the following functions. But that can be cleaned up at any
point.
qemuControllerModelUSBToCaps()
qemuSoundCodecTypeToCaps()
>(Actually I just looked and up until just a few weeks ago, qemuCaps
>wasn't used anywhere in the post-parse callbacks; just recently code was
>added to the domain post-parse callback to retrieve the qemuCaps and use
>it when deciding whether to add an implicit pcie-root controller to an
>ARM virtual machine. This is done because apparently you need to check
>the capabilities to see if the machine has a pcie-root, but that seems
>troublesome to me, because a machine that has a pcie-root and one that
>don't should *not* have the same machinetype name in my opinion (that is
>a *very* serious difference in ABI). Also, I wonder if this ARM "virt-*"
>machinetype really supports the i82801b11-bridge controller (since it is
>modelled on a controller that in the real world is only available
>integrated into an Intel x86 chipset). The things you find while
>cscoping around at midnight on Saturday...)
>
Well, yes. IMHO this was pushed harshly and I said that I don't like
it. It won't work properly *and* it needs hack to work properly with
our test suite. Unfortunately nobody else looks like replying with
their opinion on what should be the next step (I'd say fixing, of
course).
>
>
>> I haven't checked that there is a possibility
>> of having domain parsed without assigning PCI addresses,
>
>not in the code as it exists now - PCI addresses are assigned by a
>separate function that gets called after return from the parser (and
>post parse callbacks), but every place that calls the parser also calls
>the function that assigns PCI addresses.
>
>> but the
>> def->opts.pciopts.modelName cannot be invalid in this case. Anyway,
>> being _too_ careful cannot hurt, no need to change it unless you want
>> to.
>
>See your previous sentence. Checking for an empty modelName is a sanity
>check. I like to be careful, especially when there is separation in
>space and time between when the value should have been set and when it
>is used. Today it's true that it's always set, but if someone
>unknowingly changes the code over in "the other place" some day, I'd
>rather get a clear error about it than a segv.
>
I understood that and it's sensible. It's just not what we do
everywhere else :) In the future I'd like to move at least part of the
parsing/formatting code into data, which would deal partly with this,
but that's a long shot for now.
>
>>
>>> if (!virQEMUCapsGet(qemuCaps,
>>> QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
>>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> - _("The dmi-to-pci-bridge
>>> (i82801b11-bridge) "
>>> + _("the dmi-to-pci-bridge
>>> (i82801b11-bridge) "
>>> "controller is not supported in this
>>> QEMU binary"));
>>> goto error;
>>> }
>>
>> You could also save a bunch of lines using what I suggested, that is
>> concentrating device->caps mapping in a separate function and using
>> one error message for all models, not one for every model, e.g.:
>>
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("the '%s' ('%s') controller is not supported "
>> "in this QEMU binary"),
>> blahTypeToString(def->model),
>> blehTypeToString(def->opts.pciopts.modelName));
>>
>> but that can be done later on, the code works properly as it is now.
>
>I missed that suggestion, but would welcome the consolidation. However
>we still have the situation that each modelName only works with one
>particular model, so it would take some extra thought to get the right
>amount of the duplicated code into the common function and in the right
>order (so I'll just leave it as is for now.
>
That could be dealt with some function that would map 'models' and
'model names' and check it, then this error could be used. I'll keep
this in mind and will eventually cook a patch. Unless I forget since
this is really just a cosmetic thing.
Thanks for the reply, I like that we're on the same note. I managed
to rebase my patch onto the pushed series and can send it now, thanks.
>>
>>
>> ACK
>
-------------- 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/20150810/9ef2ed9a/attachment-0001.sig>
More information about the libvir-list
mailing list