[libvirt] [PATCH 00/13] PCIe fixes + new PCI controllers w/RFC
Martin Kletzander
mkletzan at redhat.com
Tue Jun 23 08:40:24 UTC 2015
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 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/20150623/0089712e/attachment-0001.sig>
More information about the libvir-list
mailing list