12:30 p.m.
On 12/09/2015 09:36 AM, John Ferlan wrote:
On 11/19/2015 01:25 PM, Laine Stump wrote:
> This new function will add a single controller of the given model,
> except the case of ich9-usb-ehci1 (the master controller for a USB2
> controller set) in which case a set of related controllers will be
> added (EHCI1, UHCI1, UHCI2, UHCI3). These controllers will not be
> given PCI addresses, but should be otherwise ready to use.
>
> "-1" is allowed for controller model, and means "default for this
> machinetype". This matches the existing practice in
> qemuDomainDefPostParse(), which always adds the default controller
> with model = -1, and relies on the commandline builder to set a model
> (that is wrong, but will be fixed later).
> ---
> src/conf/domain_conf.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 2 ++
> src/libvirt_private.syms | 1 +
> tests/qemuxml2argvtest.c | 1 +
> 4 files changed, 52 insertions(+)
>
Wasn't able to "git am -3" this patch - so I assume you have some merge
conflicts coming... Safe to assume from your side that I wasn't able to
compile this - so I'll further assume this and the next patch will have
gone through check/check-syntax processing
'git rebase -i -x "make -j8 check && make -j8 syntax-check"
master' is
my friend :-)
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ab05e7f..63888b1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14410,6 +14410,54 @@ virDomainDefAddController(virDomainDefPtr def, int type, int
idx, int model)
> }
>
>
> +/*
> + * virDomainDefAddUSBController() - add a USB controller of the
> + * specified model. If model is ich9-usb-ehci, also add companion
> + * uhci1, uhci2, and uhci3 controllers at the same index. Note that a
> + * model of "-1" is allowed for backward compatibility, and means
> + * "default USB controller for this machinetype".
> + */
Nice! comments, but the format I've seen lately has been:
/* functionName
* @arg1: description
* @argn: description
*
* Function description
*
* Returns description
*/
Actually, more like this:
/**
* functionName:
*
* @arg1: description
* @argn: description
*
* Function description
*
* Returns description
*/
I changed it accordingly.
> +int
> +virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model)
> +{
> + virDomainControllerDefPtr cont; /* this is a *copy* of the DefPtr */
> +
> + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx, model);
> + if (!cont)
> + return -1;
> +
> + if (model != VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1)
> + return 0;
> +
> + /* When the initial controller is ich9-usb-ehci, also add the
> + * companion controllers
> + */
> +
> + idx = cont->idx; /* in case original request was "-1" */
> +
And the operating assumption being that none of the following exist?
IOW: Would it be possible for someone to add these, but not the above?
Not that anyone (qa) would do that...
This is only called if there are no other USB controllers in the config,
so no that could never happen.
> + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx,
VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1)))
> + return -1;
> + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> + cont->info.master.usb.startport = 0;
> +
> + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx,
VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2)))
> + return -1;
> + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> + cont->info.master.usb.startport = 2;
> +
> + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx,
VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3)))
> + return -1;
> + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> + cont->info.master.usb.startport = 4;
> +
> + return 0;
> +}
> +
> +
> int
> virDomainDefMaybeAddController(virDomainDefPtr def,
> int type,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8d43ee6..c34bfd0 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3169,6 +3169,8 @@ int virDomainObjListConvert(virDomainObjListPtr domlist,
> bool skip_missing);
>
> int
> +virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model);
> +int
> virDomainDefMaybeAddController(virDomainDefPtr def,
> int type,
> int idx,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7e60d87..b7008e0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -200,6 +200,7 @@ virDomainControllerTypeToString;
> virDomainCpuPlacementModeTypeFromString;
> virDomainCpuPlacementModeTypeToString;
> virDomainDefAddImplicitControllers;
> +virDomainDefAddUSBController;
> virDomainDefCheckABIStability;
> virDomainDefCheckDuplicateDiskInfo;
> virDomainDefCheckUnsupportedMemoryHotplug;
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 5fe52b0..25ffbea 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1474,6 +1474,7 @@ mymain(void)
> QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE,
> QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1,
Should this perhaps have been merged into patch 2?
No, it should be a part of patch 5. Must have gotten mixed up during a
rebase. I moved it.
Just seems out of place here.
ACK - seems reasonable to me.
John
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
> DO_TEST("q35-usb2",
>