On Mon, Jun 22, 2015 at 02:44:05PM -0400, Laine Stump wrote:
The first 4 patches are bugfixes/reorganizations that have no controversy.
The sets of 5-7, 8-10, and 11-13 each implement a new model of PCI
controller:
5-7 - <controller type='pci' model='pcie-root-port'/>
This is based on qemu's ioh3420.
8-10 - <controller type='pci' model='pcie-switch-upstream-port'/>
Based on qemu's x3130-upstream
11-13 - <controller type='pci' model='pcie-switch-downstream-port'/>
(xio3130-downstream)
The first patch of each set adds a capability bit for qemu (again
non-controversial), the 2nd adds the new pci controller model, and the
3rd implements that model in qemu (by checking for the capability and
adding a commandline arg or failing).
The "controversial"/RFC bit is this - talking to Alex Williamson
(after I'd rwritten these patches, which is why I'm presenting them in
a form that I *don't* want to push) about the possibility of qemu
adding generic root-port, switch-upstream-port, and
switch-downstream-port controllers, and possibly also a generic
dmi-to-pci-bridge (i.e. controllers not tied to any particular
hardware implementation as with those currently available), I'm
realizing that, while it was a correct decision to make all of the
existing pci controllers "type='pci'" (since they share an address
space), using the "model" attribute to set the kind of controller was
probably a mistake. The problem - if:
<controller type='pci' model='dmi-to-pci-bridge'/>
currently means to add an i82801b11-bridge controller to the domain,
once qemu implements a generic dmi-to-pci-bridge, how will *that* be
denoted, and how will we avoid replacing the existing i81801b11-bridge
in a particular domain with the generic version when a guest is
restarted following a qemu/libvirt upgrade?
In hindsight, it probably would have been better to do something like
this with the four existing pci controllers:
<controller type='pci' subType='dmi-to-pci-bridge'
model='i82801b11-bridge'/>
<controller type='pci' subType='pci-bridge'
model='pci-bridge'/> (or maybe blank?)
<controller type='pci' subType='pci-root'/> (again maybe model is blank)
<controller type='pci' subType='pcie-root'/>(and again)
(instead, what is shown above as "subType" is currently placed in the "model" attribute).
Then we could add the 3 new types like this:
<controller type='pci' subType='pcie-root-port' model='ioh3420'/>
<controller type='pci' subType='pcie-switch-upstream-port'
model='x3130-upstream/>
<controller type='pci' subType='pcie-switch-downstream-port'
model='xio3130-downstream/>
and we would easily be able to add support for new generic controllers
that behaved identically, by just adding a new model. But we haven't
done that, and anything we do in the future must be backwards
compatible with what's already there (right?). I'm trying to think of
how to satisfy backward compatibility while making things work better
in the future.
Some ideas, in no particular order:
===
Idea 1: multiplex the meaning of the "model" attribute, so we currently have:
<controller type='pci' model='dmi-to-pci-bridge'/>
which means "add an i82801b11-bridge device" and when we add a generic
version of this type of controller, we would do it with something like:
<controller type='pci' model='generic-dmi-to-pci-bridge'/>
and for another vendor's mythical controller:
<controller type='pci' model='xyz-dmi-to-pci-bridge'/>
Cons: This will make for ugliness in switch statements where a new
case will have to be added whenever a different controller with
similar behavior/usage is supported. And it's generally not a good idea to
have a single attribute be used for two different functions.
===
Idea 2: implement new controllers as suggested in "20/20 hindsight"
above. For controllers in existing domains (dmi-to-pci-bridge,
pic-bridge, pci-root, and pcie-root) imply it into the controller
definition of an existing domain when only model has been given (but
don't write it out that way, to preserve the ability to downgrade). So
this:
[1] <controller type='pci' model='dmi-to-pci-bridge'/>
would internally mean this:
[2] <controller type='pci' subType='dmi-to-pci-bridge'
model='i82801b11-bridge'/>
(but would remain as [1] when config is rewritten/migrated) while
this:
[3] <controller type='pci' subType='dmi-to-pci-bridge'
model='anything whatsoever/>
would mean exactly what it says.
Cons: Keeping this straight would mean having some sort of
"oldStyleCompat" flag in the controller object, to be sure that [1]
wasn't sent in migration status as [2] (since the destination might
not recognize it). It would also mean keeping the code in the parser
and formatter to deal with this flag. Forever.
===
Idea 3: interpret controllers with missing subType as above, but
actually write it out to the config/migration/etc in the new modified
format.
Cons: This would prevent downgrading libvirt or migrating from a host
with newer libvirt to one with older libvirt. (Although preserving
compatibility at some level when downgrading may be a stated
requirement of some downstream distros' builds of libvirt, I think for
upstream it is only a "best effort"; I'm just not certain how much "best" is considered to be :-)
===
Idea 4: Unlike other uses of "model" in libvirt, for pci controllers,
continue to use "model" to denote the subtype/class/whatever of
controller, and create a new attribute to denote the different
specific implementations of each one. So for example:
[4] <controller type='pci' model='dmi-to-pci-bridge'/>
would become:
[5] <controller type='pci' model='dmi-to-pci-bridge'
implementation='i82801b11-bridge'/>
(or some other name in place of "implementation" - ideas? I'm horrible
at thinkin up names)
Pros: wouldn't create compatibility problems when downgrading or
migrating cross version.
Cons: Is inconsistent with every other use of "model" attribute in
libvirt, and each new addition of a PCI controller further propagates
this misuse.
I must say that I thought of this ^^ exactly when reading first three ideas. I wouldn't necessarily say it's a "misuse" of the 'model' naming. We can say it's 'model' and 'subModel' or 'driver' or whatever. One thing to add to pros is that if you don't care about the implementation (submodel/driver), then you don't need to increase the size of the XML. Pus the code complexity added is not greater than the benefit gained. Having said that, I don't insist on that, it's merely my two cents that I like the last idea the best.
==== I currently like either option 2 or 3 (depending on how good we want to be about supporting downgrade/intra-version migration), but as is obvious by the fact that it was me that suggested putting the type of pci controller into "model", I am very good at making the wrong decision on matters like this. Whatever everyone thinks is best, patches 5-13 of this series would be changed accordingly, and possibly a couple new patches would be made to adjust the 4 existing controller types. Note that this will also effect the libvirt support for the upcoming qemu "pxb" controller, which is a PCI root bus that can be placed in its own numa node (my description may be incorrect, but I think it gets the idea across). Anyway, with idea 2 or 3 it would be: <controller type='pci' subType='pci-root' model='pxb'/> (along with some options to setup numa info). So, along with any comments on the individual patches (including whether the specific args added to the qemu commandline are correct - I'm looking at you, qemu people!), I would appreciate opinions on how I can save us from this misuse of "model" that I've created. Laine Stump (13): qemu: refactor qemuBuildControllerDevStr to eliminate future duplicate code qemu: always permit PCI devices to be manually assigned to a PCIe bus qemu: ignore assumptions about hotplug requirement when address is from config docs: document when pcie-root/dmi-to-pci-bridge support was added qemu: add capabilities bit for device ioh3420 conf: new pci controller model "pcie-root-port" qemu: support new pci controller model "pcie-root-port" qemu: add capabilities bit for device x3130-upstream conf: new pci controller model "pcie-switch-upstream-port" qemu: support new pci controller model "pcie-switch-upstream-port" qemu: add capabilities bit for device xio3130-downstream conf: new pcie-controller model "pcie-switch-downstream-port" qemu: support new pci controller model "pcie-switch-downstream-port" docs/formatdomain.html.in | 48 +++++++++--- docs/schemas/domaincommon.rng | 3 + src/conf/domain_addr.c | 47 ++++++++++-- src/conf/domain_addr.h | 23 ++++-- src/conf/domain_conf.c | 5 +- src/conf/domain_conf.h | 3 + src/qemu/qemu_capabilities.c | 8 +- src/qemu/qemu_capabilities.h | 5 +- src/qemu/qemu_command.c | 86 +++++++++++++++++----- tests/qemucapabilitiesdata/caps_1.2.2-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.3.1-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.4.2-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.5.3-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.6.0-1.caps | 3 + tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 3 + tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 3 + tests/qemuhelptest.c | 10 ++- .../qemuxml2argv-pcie-root-port.args | 10 +++ .../qemuxml2argv-pcie-root-port.xml | 33 +++++++++ .../qemuxml2argv-pcie-switch-downstream-port.args | 13 ++++ .../qemuxml2argv-pcie-switch-downstream-port.xml | 36 +++++++++ .../qemuxml2argv-pcie-switch-upstream-port.args | 9 +++ .../qemuxml2argv-pcie-switch-upstream-port.xml | 32 ++++++++ tests/qemuxml2argvtest.c | 25 +++++++ tests/qemuxml2xmltest.c | 3 + 25 files changed, 375 insertions(+), 45 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml -- 2.1.0 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list