Re: [libvirt] [PATCHv2 06/17] qemu: implement <model> subelement to <controller>
On 07/23/2015 09:41 AM, Ján Tomko wrote:
[Reducing the cc-list for this one too]
On Fri, Jul 17, 2015 at 02:43:33PM -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> type attribute default as "pci-bridge"
>
> 2) dmi-to-pci-bridge - sets <model> type 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. This 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.
> ---
> new in V2 (previously was a part of the patch to add pcie-root-port)
>
> src/qemu/qemu_command.c | 70 ++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 64 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 74f02f5..8868e18 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2243,11 +2243,37 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> virDomainControllerDefPtr cont = def->controllers[i];
> int idx = cont->idx;
> virDevicePCIAddressPtr addr;
> + virDomainPCIControllerOptsPtr options;
> + const char *deviceName = NULL;
>
> 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.
> + */
Setting the defaults here feels out of place.
I agree, and originally tried to set them in the PostParse callbacks,
but then figured out that 1) no new devices (including new controllers)
have a PCI address yet at that time and 2) any extra auto-added
pci-bridge controllers haven't been added yet either, so putting any
sort of logic to set these values (which are completely dependent on the
PCI address, which hasn't yet been assigned) in the PostParse is pointless.
You can see the order of this in, e.g., qemuDomainDefineXMLFlags() - it
calls virDomainDefParseString() (which will end up calling the
PostParse), then several other things, and then finally gets around to
calling qemuDomainAssignAddresses().
Short of completely reorganizing when or how PCI addresses are assigned
(which I think is way beyond the scope of this patch series, and has the
potential to upset quite a lot in between PostParse and
qemuDomainAssignAddresses()), this is the most reasonable place I could
find.
If we set them in the PostParse callbacks, they will also get
formatted back
(which should not matter when migrating to older libvirt, as it does not
parse it anyway), which is what we do for lots of other default values.
That way only the controllers auto-added a few lines above this code
would need their chassis filled.
Even that is enough to cause problems. The real killer is that even
controllers that were added explicitly in the XML (but without PCI
addresses) would need something done here. So essentially "all controllers".
Jan