| Message ID | fa199b00cff6e9be1645a8ec10f77ba0bcaf3f4e.1547746867.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu: virtio-{non-}transitional support | expand |
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: [...] > @@ -1108,6 +1110,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { > { "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP }, > { "zpci", QEMU_CAPS_DEVICE_ZPCI }, > { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD }, > + {"virtio-blk-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL}, > + {"virtio-blk-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL}, There should be whitespace before and after curly braces, for consistency with existing entries. [...] > +++ b/src/qemu/qemu_capabilities.h > @@ -504,6 +504,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ > /* 325 */ > QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file,pmem= */ > QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */ > + QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL, /* -device virtio-blk-pci-transitional */ > + QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL, /* -device virtio-blk-pci-non-transitional */ A bit more verbose, but I think we should go for QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_{,NON_}TRANSITIONAL since there are several non-PCI variants of VirtIO devices. It'd also be nice if adding the capabilities and wiring up the command line generation bits happened in separate patches. [...] > +static int > +qemuBuildVirtioTransitional(virBufferPtr buf, > + const char *baseName, > + virQEMUCapsPtr qemuCaps, > + virDomainDeviceAddressType type, > + int model, > + virDomainDeviceType devtype) > +{ > + int tmodel_cap, ntmodel_cap; > + bool has_tmodel, has_ntmodel; > + > + if (qemuBuildVirtioDevStr(buf, baseName, type) < 0) > + return -1; > + > + switch (devtype) { > + case VIR_DOMAIN_DEVICE_DISK: > + has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL; > + has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL; > + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL; > + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL; > + break; I like this approach much better than the one used in the RFC, eg. passing two booleans to the function. Nice! What I don't like is that you're building this fairly thin wrapper around qemuBuildVirtioDevStr() when IMHO you should rather be agumenting the original function - mostly because the new name is not nearly as good :) Do you think you could make that happen? [...] > + if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && > + (has_tmodel || has_ntmodel)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("virtio transitional models are not supported " > + "for address type=%s"), s/transitional/(non-)transitional/ [...] > + if (has_tmodel) { > + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) > + virBufferAddLit(buf, "-transitional"); > + > + /* No error for if -transitional is not supported: our address > + * allocation will force the device into plain PCI bus, which > + * is functionally identical to standard 'virtio-XXX' behavior > + */ > + } else if (has_ntmodel) { > + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { > + virBufferAddLit(buf, "-non-transitional"); > + } else if (virQEMUCapsGet(qemuCaps, > + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { > + virBufferAddLit(buf, ",disable-legacy=on"); > + } else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("virtio non-transitional model not supported " > + "for this qemu")); > + return -1; > + } > + } Would it make sense to be more explicit here? Current versions of QEMU default to disable-modern=off,disable-legacy=off for virtio-pci devices plugged into conventional PCI slots, but unless I'm mistaken that was not always the case, so it would perhaps be preferrable to not rely on that behavior and always explicitly set both disable-* options when the new devices are not available; if the options themselves are not available, then we should error out. [...] > @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, > case VIR_DOMAIN_DEVICE_DISK: > switch ((virDomainDiskBus) dev->data.disk->bus) { > case VIR_DOMAIN_DISK_BUS_VIRTIO: > + if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL) > + return pciFlags; Perhaps a short comment about how transitional VirtIO devices can only be plugged into conventional PCI slots would be appropriate here, for the benefit of those looking at the code years from now :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/18/2019 07:33 AM, Andrea Bolognani wrote: > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > [...] >> @@ -1108,6 +1110,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { >> { "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP }, >> { "zpci", QEMU_CAPS_DEVICE_ZPCI }, >> { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD }, >> + {"virtio-blk-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL}, >> + {"virtio-blk-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL}, > > There should be whitespace before and after curly braces, for > consistency with existing entries. > Indeed, I'll fix that in all the patches > [...] > >> +++ b/src/qemu/qemu_capabilities.h >> @@ -504,6 +504,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ >> /* 325 */ >> QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file,pmem= */ >> QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */ >> + QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL, /* -device virtio-blk-pci-transitional */ >> + QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL, /* -device virtio-blk-pci-non-transitional */ > > A bit more verbose, but I think we should go for > > QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_{,NON_}TRANSITIONAL > > since there are several non-PCI variants of VirtIO devices. > Sure sounds good > It'd also be nice if adding the capabilities and wiring up the > command line generation bits happened in separate patches. > > [...] > OK >> +static int >> +qemuBuildVirtioTransitional(virBufferPtr buf, >> + const char *baseName, >> + virQEMUCapsPtr qemuCaps, >> + virDomainDeviceAddressType type, >> + int model, >> + virDomainDeviceType devtype) >> +{ >> + int tmodel_cap, ntmodel_cap; >> + bool has_tmodel, has_ntmodel; >> + >> + if (qemuBuildVirtioDevStr(buf, baseName, type) < 0) >> + return -1; >> + >> + switch (devtype) { >> + case VIR_DOMAIN_DEVICE_DISK: >> + has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL; >> + has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL; >> + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL; >> + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL; >> + break; > > I like this approach much better than the one used in the RFC, > eg. passing two booleans to the function. Nice! > > What I don't like is that you're building this fairly thin wrapper > around qemuBuildVirtioDevStr() when IMHO you should rather be > agumenting the original function - mostly because the new name is > not nearly as good :) Do you think you could make that happen? > Hmm, seems weird to make call sites that will never support the transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into this function, passing DEVICE_TYPE etc. I can split the transitional bits into their own function and not have it wrap BuildVirtioDevStr but be a follow on, so for example: if (qemuBuildVirtioDevStr(...) < 0) goto error; if (qemuBuildVirtioTransitionalStr(...) < ) goto error; Does that work? > [...] >> + if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && >> + (has_tmodel || has_ntmodel)) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> + _("virtio transitional models are not supported " >> + "for address type=%s"), > > s/transitional/(non-)transitional/ > > [...] >> + if (has_tmodel) { >> + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) >> + virBufferAddLit(buf, "-transitional"); >> + >> + /* No error for if -transitional is not supported: our address >> + * allocation will force the device into plain PCI bus, which >> + * is functionally identical to standard 'virtio-XXX' behavior >> + */ >> + } else if (has_ntmodel) { >> + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { >> + virBufferAddLit(buf, "-non-transitional"); >> + } else if (virQEMUCapsGet(qemuCaps, >> + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { >> + virBufferAddLit(buf, ",disable-legacy=on"); >> + } else { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("virtio non-transitional model not supported " >> + "for this qemu")); >> + return -1; >> + } >> + } > > Would it make sense to be more explicit here? Current versions of > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci > devices plugged into conventional PCI slots, but unless I'm mistaken > that was not always the case, so it would perhaps be preferrable to > not rely on that behavior and always explicitly set both disable-* > options when the new devices are not available; if the options > themselves are not available, then we should error out. > I'll respond separately > [...] >> @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, >> case VIR_DOMAIN_DEVICE_DISK: >> switch ((virDomainDiskBus) dev->data.disk->bus) { >> case VIR_DOMAIN_DISK_BUS_VIRTIO: >> + if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL) >> + return pciFlags; > > Perhaps a short comment about how transitional VirtIO devices can > only be plugged into conventional PCI slots would be appropriate > here, for the benefit of those looking at the code years from now :) > This same pattern is duplicated in the rest of the series, seems weird to comment one site, but commenting all of them is overkill. I guess commenting one site is the middle ground unless you have a better idea of where to put the comment Thanks for the review - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/18/2019 07:33 AM, Andrea Bolognani wrote: > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: >> + if (has_tmodel) { >> + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) >> + virBufferAddLit(buf, "-transitional"); >> + >> + /* No error for if -transitional is not supported: our address >> + * allocation will force the device into plain PCI bus, which >> + * is functionally identical to standard 'virtio-XXX' behavior >> + */ >> + } else if (has_ntmodel) { >> + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { >> + virBufferAddLit(buf, "-non-transitional"); >> + } else if (virQEMUCapsGet(qemuCaps, >> + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { >> + virBufferAddLit(buf, ",disable-legacy=on"); >> + } else { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("virtio non-transitional model not supported " >> + "for this qemu")); >> + return -1; >> + } >> + } > > Would it make sense to be more explicit here? Current versions of > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci > devices plugged into conventional PCI slots, but unless I'm mistaken > that was not always the case, so it would perhaps be preferrable to > not rely on that behavior and always explicitly set both disable-* > options when the new devices are not available; if the options > themselves are not available, then we should error out. > I don't know enough to say, CCing ehabkost and danpb for more eyes - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote: > On 01/18/2019 07:33 AM, Andrea Bolognani wrote: > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > >> + if (has_tmodel) { > >> + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) > >> + virBufferAddLit(buf, "-transitional"); > >> + > >> + /* No error for if -transitional is not supported: our address > >> + * allocation will force the device into plain PCI bus, which > >> + * is functionally identical to standard 'virtio-XXX' behavior > >> + */ > >> + } else if (has_ntmodel) { > >> + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { > >> + virBufferAddLit(buf, "-non-transitional"); > >> + } else if (virQEMUCapsGet(qemuCaps, > >> + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { > >> + virBufferAddLit(buf, ",disable-legacy=on"); > >> + } else { > >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > >> + _("virtio non-transitional model not supported " > >> + "for this qemu")); > >> + return -1; > >> + } > >> + } > > > > Would it make sense to be more explicit here? Current versions of > > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci > > devices plugged into conventional PCI slots, but unless I'm mistaken > > that was not always the case, so it would perhaps be preferrable to > > not rely on that behavior and always explicitly set both disable-* > > options when the new devices are not available; if the options > > themselves are not available, then we should error out. > > > > I don't know enough to say, CCing ehabkost and danpb for more eyes The QEMU code for the devices has - Original devs: disable-modern=off disable-legacy=auto - Transitional devs: disable-modern=off disable-legacy=off - Non-transitional devs: disable-modern=off disable-legacy=on IOW, in the case that -transitional is not available, we could set disable-legacy=off. Provided that we always place -transitional devices into PCI slots, never PCI-e slots, whether we set disable-legacy=off or not doesn't have any effect. None the less it would make sense to set it explicitly though as that would cause us to catch bugs if we mistakenly had a -transitional dev in a PCI-e slot. I don't see a need to set disable-modern at all, since it is defaulting to "off" in all cases. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote: > On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote: > > On 01/18/2019 07:33 AM, Andrea Bolognani wrote: > > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > > > > + if (has_tmodel) { > > > > + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) > > > > + virBufferAddLit(buf, "-transitional"); > > > > + > > > > + /* No error for if -transitional is not supported: our address > > > > + * allocation will force the device into plain PCI bus, which > > > > + * is functionally identical to standard 'virtio-XXX' behavior > > > > + */ > > > > + } else if (has_ntmodel) { > > > > + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { > > > > + virBufferAddLit(buf, "-non-transitional"); > > > > + } else if (virQEMUCapsGet(qemuCaps, > > > > + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { > > > > + virBufferAddLit(buf, ",disable-legacy=on"); > > > > + } else { > > > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > > > + _("virtio non-transitional model not supported " > > > > + "for this qemu")); > > > > + return -1; > > > > + } > > > > + } > > > > > > Would it make sense to be more explicit here? Current versions of > > > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci > > > devices plugged into conventional PCI slots, but unless I'm mistaken > > > that was not always the case, so it would perhaps be preferrable to > > > not rely on that behavior and always explicitly set both disable-* > > > options when the new devices are not available; if the options > > > themselves are not available, then we should error out. > > > > I don't know enough to say, CCing ehabkost and danpb for more eyes > > The QEMU code for the devices has > > - Original devs: disable-modern=off disable-legacy=auto > - Transitional devs: disable-modern=off disable-legacy=off > - Non-transitional devs: disable-modern=off disable-legacy=on > > IOW, in the case that -transitional is not available, we could > set disable-legacy=off. Provided that we always place -transitional > devices into PCI slots, never PCI-e slots, whether we set > disable-legacy=off or not doesn't have any effect. None the less it > would make sense to set it explicitly though as that would cause us > to catch bugs if we mistakenly had a -transitional dev in a PCI-e > slot. > > I don't see a need to set disable-modern at all, since it is > defaulting to "off" in all cases. Wasn't there some old QEMU release where disable-modern didn't default to off? I seem to remember that. More generally, I don't see a reason *not* to specify disable-modern along with disable-legacy. Why be implicit when you can very easily be explicit instead?
On Mon, 2019-01-21 at 17:40 -0500, Cole Robinson wrote: > On 01/18/2019 07:33 AM, Andrea Bolognani wrote: > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > > > +static int > > > +qemuBuildVirtioTransitional(virBufferPtr buf, > > > + const char *baseName, > > > + virQEMUCapsPtr qemuCaps, > > > + virDomainDeviceAddressType type, > > > + int model, > > > + virDomainDeviceType devtype) > > > +{ > > > + int tmodel_cap, ntmodel_cap; > > > + bool has_tmodel, has_ntmodel; > > > + > > > + if (qemuBuildVirtioDevStr(buf, baseName, type) < 0) > > > + return -1; > > > + > > > + switch (devtype) { > > > + case VIR_DOMAIN_DEVICE_DISK: > > > + has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL; > > > + has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL; > > > + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL; > > > + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL; > > > + break; > > > > I like this approach much better than the one used in the RFC, > > eg. passing two booleans to the function. Nice! > > > > What I don't like is that you're building this fairly thin wrapper > > around qemuBuildVirtioDevStr() when IMHO you should rather be > > agumenting the original function - mostly because the new name is > > not nearly as good :) Do you think you could make that happen? > > Hmm, seems weird to make call sites that will never support the > transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into > this function, passing DEVICE_TYPE etc. I can split the transitional > bits into their own function and not have it wrap BuildVirtioDevStr but > be a follow on, so for example: > > if (qemuBuildVirtioDevStr(...) < 0) > goto error; > if (qemuBuildVirtioTransitionalStr(...) < ) > goto error; > > Does that work? The split version is more likely to end up being misused, so I wouldn't go down that road. As for the naming, my suggestion was to stick with the original qemuBuildVirtioDevStr() name, add parameters to it, and not introduce qemuBuildVirtioTransitional() at all, so it wouldn't look weird when called for devices that are modern only. I don't see a problem with passing devType even when you're not ultimately going to make decisions based on it for certain devices. > > [...] > > > @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, > > > case VIR_DOMAIN_DEVICE_DISK: > > > switch ((virDomainDiskBus) dev->data.disk->bus) { > > > case VIR_DOMAIN_DISK_BUS_VIRTIO: > > > + if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL) > > > + return pciFlags; > > > > Perhaps a short comment about how transitional VirtIO devices can > > only be plugged into conventional PCI slots would be appropriate > > here, for the benefit of those looking at the code years from now :) > > This same pattern is duplicated in the rest of the series, seems weird > to comment one site, but commenting all of them is overkill. I guess > commenting one site is the middle ground unless you have a better idea > of where to put the comment I don't think having a short comment such as /* Transitional devices only work in conventional PCI slots */ repeated a few times really counts as overkill :) So I'd go that way. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 22, 2019 at 12:45:59PM +0100, Andrea Bolognani wrote: > On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote: > > On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote: > > > On 01/18/2019 07:33 AM, Andrea Bolognani wrote: > > > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > > > > > + if (has_tmodel) { > > > > > + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) > > > > > + virBufferAddLit(buf, "-transitional"); > > > > > + > > > > > + /* No error for if -transitional is not supported: our address > > > > > + * allocation will force the device into plain PCI bus, which > > > > > + * is functionally identical to standard 'virtio-XXX' behavior > > > > > + */ > > > > > + } else if (has_ntmodel) { > > > > > + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { > > > > > + virBufferAddLit(buf, "-non-transitional"); > > > > > + } else if (virQEMUCapsGet(qemuCaps, > > > > > + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { > > > > > + virBufferAddLit(buf, ",disable-legacy=on"); > > > > > + } else { > > > > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > > > > + _("virtio non-transitional model not supported " > > > > > + "for this qemu")); > > > > > + return -1; > > > > > + } > > > > > + } > > > > > > > > Would it make sense to be more explicit here? Current versions of > > > > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci > > > > devices plugged into conventional PCI slots, but unless I'm mistaken > > > > that was not always the case, so it would perhaps be preferrable to > > > > not rely on that behavior and always explicitly set both disable-* > > > > options when the new devices are not available; if the options > > > > themselves are not available, then we should error out. > > > > > > I don't know enough to say, CCing ehabkost and danpb for more eyes > > > > The QEMU code for the devices has > > > > - Original devs: disable-modern=off disable-legacy=auto > > - Transitional devs: disable-modern=off disable-legacy=off > > - Non-transitional devs: disable-modern=off disable-legacy=on > > > > IOW, in the case that -transitional is not available, we could > > set disable-legacy=off. Provided that we always place -transitional > > devices into PCI slots, never PCI-e slots, whether we set > > disable-legacy=off or not doesn't have any effect. None the less it > > would make sense to set it explicitly though as that would cause us > > to catch bugs if we mistakenly had a -transitional dev in a PCI-e > > slot. > > > > I don't see a need to set disable-modern at all, since it is > > defaulting to "off" in all cases. > > Wasn't there some old QEMU release where disable-modern didn't > default to off? I seem to remember that. Yes, disable-modern is "on" on 2.6 and older machine-types. > > More generally, I don't see a reason *not* to specify disable-modern > along with disable-legacy. Why be implicit when you can very easily > be explicit instead? Agreed. -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 22, 2019 at 10:44:23AM -0200, Eduardo Habkost wrote: > On Tue, Jan 22, 2019 at 12:45:59PM +0100, Andrea Bolognani wrote: > > On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote: > > > On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote: > > > > On 01/18/2019 07:33 AM, Andrea Bolognani wrote: > > > > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > > > > > > + if (has_tmodel) { > > > > > > + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) > > > > > > + virBufferAddLit(buf, "-transitional"); > > > > > > + > > > > > > + /* No error for if -transitional is not supported: our address > > > > > > + * allocation will force the device into plain PCI bus, which > > > > > > + * is functionally identical to standard 'virtio-XXX' behavior > > > > > > + */ > > > > > > + } else if (has_ntmodel) { > > > > > > + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { > > > > > > + virBufferAddLit(buf, "-non-transitional"); > > > > > > + } else if (virQEMUCapsGet(qemuCaps, > > > > > > + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { > > > > > > + virBufferAddLit(buf, ",disable-legacy=on"); > > > > > > + } else { > > > > > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > > > > > + _("virtio non-transitional model not supported " > > > > > > + "for this qemu")); > > > > > > + return -1; > > > > > > + } > > > > > > + } > > > > > > > > > > Would it make sense to be more explicit here? Current versions of > > > > > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci > > > > > devices plugged into conventional PCI slots, but unless I'm mistaken > > > > > that was not always the case, so it would perhaps be preferrable to > > > > > not rely on that behavior and always explicitly set both disable-* > > > > > options when the new devices are not available; if the options > > > > > themselves are not available, then we should error out. > > > > > > > > I don't know enough to say, CCing ehabkost and danpb for more eyes > > > > > > The QEMU code for the devices has > > > > > > - Original devs: disable-modern=off disable-legacy=auto > > > - Transitional devs: disable-modern=off disable-legacy=off > > > - Non-transitional devs: disable-modern=off disable-legacy=on > > > > > > IOW, in the case that -transitional is not available, we could > > > set disable-legacy=off. Provided that we always place -transitional > > > devices into PCI slots, never PCI-e slots, whether we set > > > disable-legacy=off or not doesn't have any effect. None the less it > > > would make sense to set it explicitly though as that would cause us > > > to catch bugs if we mistakenly had a -transitional dev in a PCI-e > > > slot. > > > > > > I don't see a need to set disable-modern at all, since it is > > > defaulting to "off" in all cases. > > > > Wasn't there some old QEMU release where disable-modern didn't > > default to off? I seem to remember that. > > Yes, disable-modern is "on" on 2.6 and older machine-types. > > > > > More generally, I don't see a reason *not* to specify disable-modern > > along with disable-legacy. Why be implicit when you can very easily > > be explicit instead? > > Agreed. Ok, I've no objection to an explicit disable-modern=off based on this info. Regards, Daniel
On 01/22/2019 09:00 AM, Daniel P. Berrangé wrote: > On Tue, Jan 22, 2019 at 10:44:23AM -0200, Eduardo Habkost wrote: >> On Tue, Jan 22, 2019 at 12:45:59PM +0100, Andrea Bolognani wrote: >>> On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote: >>>> On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote: >>>>> On 01/18/2019 07:33 AM, Andrea Bolognani wrote: >>>>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: >>>>>>> + if (has_tmodel) { >>>>>>> + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) >>>>>>> + virBufferAddLit(buf, "-transitional"); >>>>>>> + >>>>>>> + /* No error for if -transitional is not supported: our address >>>>>>> + * allocation will force the device into plain PCI bus, which >>>>>>> + * is functionally identical to standard 'virtio-XXX' behavior >>>>>>> + */ >>>>>>> + } else if (has_ntmodel) { >>>>>>> + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { >>>>>>> + virBufferAddLit(buf, "-non-transitional"); >>>>>>> + } else if (virQEMUCapsGet(qemuCaps, >>>>>>> + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { >>>>>>> + virBufferAddLit(buf, ",disable-legacy=on"); >>>>>>> + } else { >>>>>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >>>>>>> + _("virtio non-transitional model not supported " >>>>>>> + "for this qemu")); >>>>>>> + return -1; >>>>>>> + } >>>>>>> + } >>>>>> >>>>>> Would it make sense to be more explicit here? Current versions of >>>>>> QEMU default to disable-modern=off,disable-legacy=off for virtio-pci >>>>>> devices plugged into conventional PCI slots, but unless I'm mistaken >>>>>> that was not always the case, so it would perhaps be preferrable to >>>>>> not rely on that behavior and always explicitly set both disable-* >>>>>> options when the new devices are not available; if the options >>>>>> themselves are not available, then we should error out. >>>>> >>>>> I don't know enough to say, CCing ehabkost and danpb for more eyes >>>> >>>> The QEMU code for the devices has >>>> >>>> - Original devs: disable-modern=off disable-legacy=auto >>>> - Transitional devs: disable-modern=off disable-legacy=off >>>> - Non-transitional devs: disable-modern=off disable-legacy=on >>>> >>>> IOW, in the case that -transitional is not available, we could >>>> set disable-legacy=off. Provided that we always place -transitional >>>> devices into PCI slots, never PCI-e slots, whether we set >>>> disable-legacy=off or not doesn't have any effect. None the less it >>>> would make sense to set it explicitly though as that would cause us >>>> to catch bugs if we mistakenly had a -transitional dev in a PCI-e >>>> slot. >>>> >>>> I don't see a need to set disable-modern at all, since it is >>>> defaulting to "off" in all cases. >>> >>> Wasn't there some old QEMU release where disable-modern didn't >>> default to off? I seem to remember that. >> >> Yes, disable-modern is "on" on 2.6 and older machine-types. >> >>> >>> More generally, I don't see a reason *not* to specify disable-modern >>> along with disable-legacy. Why be implicit when you can very easily >>> be explicit instead? >> >> Agreed. > > Ok, I've no objection to an explicit disable-modern=off based on this > info. Cool I'll make this change for the next round Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/22/2019 06:53 AM, Andrea Bolognani wrote: > On Mon, 2019-01-21 at 17:40 -0500, Cole Robinson wrote: >> On 01/18/2019 07:33 AM, Andrea Bolognani wrote: >>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: >>>> +static int >>>> +qemuBuildVirtioTransitional(virBufferPtr buf, >>>> + const char *baseName, >>>> + virQEMUCapsPtr qemuCaps, >>>> + virDomainDeviceAddressType type, >>>> + int model, >>>> + virDomainDeviceType devtype) >>>> +{ >>>> + int tmodel_cap, ntmodel_cap; >>>> + bool has_tmodel, has_ntmodel; >>>> + >>>> + if (qemuBuildVirtioDevStr(buf, baseName, type) < 0) >>>> + return -1; >>>> + >>>> + switch (devtype) { >>>> + case VIR_DOMAIN_DEVICE_DISK: >>>> + has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL; >>>> + has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL; >>>> + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL; >>>> + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL; >>>> + break; >>> >>> I like this approach much better than the one used in the RFC, >>> eg. passing two booleans to the function. Nice! >>> >>> What I don't like is that you're building this fairly thin wrapper >>> around qemuBuildVirtioDevStr() when IMHO you should rather be >>> agumenting the original function - mostly because the new name is >>> not nearly as good :) Do you think you could make that happen? >> >> Hmm, seems weird to make call sites that will never support the >> transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into >> this function, passing DEVICE_TYPE etc. I can split the transitional >> bits into their own function and not have it wrap BuildVirtioDevStr but >> be a follow on, so for example: >> >> if (qemuBuildVirtioDevStr(...) < 0) >> goto error; >> if (qemuBuildVirtioTransitionalStr(...) < ) >> goto error; >> >> Does that work? > > The split version is more likely to end up being misused, so I > wouldn't go down that road. > > As for the naming, my suggestion was to stick with the original > qemuBuildVirtioDevStr() name, add parameters to it, and not > introduce qemuBuildVirtioTransitional() at all, so it wouldn't look > weird when called for devices that are modern only. > > I don't see a problem with passing devType even when you're not > ultimately going to make decisions based on it for certain devices. > Okay I'll go that route >>> [...] >>>> @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, >>>> case VIR_DOMAIN_DEVICE_DISK: >>>> switch ((virDomainDiskBus) dev->data.disk->bus) { >>>> case VIR_DOMAIN_DISK_BUS_VIRTIO: >>>> + if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL) >>>> + return pciFlags; >>> >>> Perhaps a short comment about how transitional VirtIO devices can >>> only be plugged into conventional PCI slots would be appropriate >>> here, for the benefit of those looking at the code years from now :) >> >> This same pattern is duplicated in the rest of the series, seems weird >> to comment one site, but commenting all of them is overkill. I guess >> commenting one site is the middle ground unless you have a better idea >> of where to put the comment > > I don't think having a short comment such as > > /* Transitional devices only work in conventional PCI slots */ > > repeated a few times really counts as overkill :) So I'd go that way. > Cool, I'll add it to the series Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f504db7d05..b7c0387f8e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -520,6 +520,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 325 */ "memory-backend-file.pmem", "nvdimm.unarmed", + "virtio-blk-pci-transitional", + "virtio-blk-pci-non-transitional", ); @@ -1108,6 +1110,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP }, { "zpci", QEMU_CAPS_DEVICE_ZPCI }, { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD }, + {"virtio-blk-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL}, + {"virtio-blk-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL}, }; static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 6d5ed8a3cc..34265d7cc0 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -504,6 +504,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 325 */ QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file,pmem= */ QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */ + QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL, /* -device virtio-blk-pci-transitional */ + QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL, /* -device virtio-blk-pci-non-transitional */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 822d5f8669..608cd65806 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -443,6 +443,91 @@ qemuBuildVirtioDevStr(virBufferPtr buf, return 0; } +static int +qemuBuildVirtioTransitional(virBufferPtr buf, + const char *baseName, + virQEMUCapsPtr qemuCaps, + virDomainDeviceAddressType type, + int model, + virDomainDeviceType devtype) +{ + int tmodel_cap, ntmodel_cap; + bool has_tmodel, has_ntmodel; + + if (qemuBuildVirtioDevStr(buf, baseName, type) < 0) + return -1; + + switch (devtype) { + case VIR_DOMAIN_DEVICE_DISK: + has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL; + has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL; + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL; + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL; + break; + + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + case VIR_DOMAIN_DEVICE_LAST: + default: + return 0; + } + + if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + (has_tmodel || has_ntmodel)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("virtio transitional models are not supported " + "for address type=%s"), + virDomainDeviceAddressTypeToString(type)); + return -1; + } + + if (has_tmodel) { + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) + virBufferAddLit(buf, "-transitional"); + + /* No error for if -transitional is not supported: our address + * allocation will force the device into plain PCI bus, which + * is functionally identical to standard 'virtio-XXX' behavior + */ + } else if (has_ntmodel) { + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { + virBufferAddLit(buf, "-non-transitional"); + } else if (virQEMUCapsGet(qemuCaps, + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { + virBufferAddLit(buf, ",disable-legacy=on"); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio non-transitional model not supported " + "for this qemu")); + return -1; + } + } + + return 0; +} + static int qemuBuildVirtioOptionsStr(virBufferPtr buf, @@ -2049,7 +2134,10 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, break; case VIR_DOMAIN_DISK_BUS_VIRTIO: - if (qemuBuildVirtioDevStr(&opt, "virtio-blk", disk->info.type) < 0) + if (qemuBuildVirtioTransitional(&opt, "virtio-blk", qemuCaps, + disk->info.type, + disk->model, + VIR_DOMAIN_DEVICE_DISK) < 0) goto error; if (disk->iothread) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index bd6c4031e0..4a7c71d76d 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, case VIR_DOMAIN_DEVICE_DISK: switch ((virDomainDiskBus) dev->data.disk->bus) { case VIR_DOMAIN_DISK_BUS_VIRTIO: + if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL) + return pciFlags; return virtioFlags; /* only virtio disks use PCI */ case VIR_DOMAIN_DISK_BUS_IDE: diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index c2db392e83..8cf9083035 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -212,6 +212,8 @@ <flag name='memory-backend-file.align'/> <flag name='memory-backend-file.pmem'/> <flag name='nvdimm.unarmed'/> + <flag name='virtio-blk-pci-transitional'/> + <flag name='virtio-blk-pci-non-transitional'/> <version>3001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>446361</microcodeVersion> diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args index 9e11e900da..9c5d553077 100644 --- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args +++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args @@ -27,8 +27,8 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ addr=0x1 \ -device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\ -id=virtio-disk0,bootindex=1 \ +-device virtio-blk-pci,disable-legacy=on,scsi=off,bus=pci.1,addr=0x0,\ +drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args index 070b4b8334..37078765bc 100644 --- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args +++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args @@ -27,8 +27,8 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ addr=0x1 \ -device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\ -id=virtio-disk0,bootindex=1 \ +-device virtio-blk-pci-non-transitional,scsi=off,bus=pci.1,addr=0x0,\ +drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args index 9e11e900da..356e8fdf4c 100644 --- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args +++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args @@ -25,9 +25,10 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -boot strict=on \ -device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ addr=0x1 \ --device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ +-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \ +-device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\ +-device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x1,drive=drive-virtio-disk0,\ id=virtio-disk0,bootindex=1 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args index 070b4b8334..e78223eac8 100644 --- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args +++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args @@ -25,10 +25,11 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -boot strict=on \ -device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ addr=0x1 \ --device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ +-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \ +-device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\ -id=virtio-disk0,bootindex=1 \ +-device virtio-blk-pci-transitional,scsi=off,bus=pci.2,addr=0x1,\ +drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2xmloutdata/virtio-transitional.xml b/tests/qemuxml2xmloutdata/virtio-transitional.xml index 1d28af9abb..c19e133bb3 100644 --- a/tests/qemuxml2xmloutdata/virtio-transitional.xml +++ b/tests/qemuxml2xmloutdata/virtio-transitional.xml @@ -18,7 +18,7 @@ <driver name='qemu' type='raw'/> <source dev='/dev/HostVG/QEMUGuest1'/> <target dev='vda' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> </disk> <controller type='usb' index='0' model='none'/> <controller type='sata' index='0'> @@ -30,9 +30,13 @@ <target chassis='1' port='0x8'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> </controller> - <controller type='pci' index='2' model='pcie-root-port'> + <controller type='pci' index='2' model='pcie-to-pci-bridge'> + <model name='pcie-pci-bridge'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> <model name='pcie-root-port'/> - <target chassis='2' port='0x9'/> + <target chassis='3' port='0x9'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> <input type='mouse' bus='ps2'/>
Add new <disk> model values for virtio transitional devices. When combined with bus='virtio': * "virtio-transitional" maps to qemu "virtio-blk-pci-transitional" * "virtio-non-transitional" maps to qemu "virtio-blk-pci-non-transitional" Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 90 ++++++++++++++++++- src/qemu/qemu_domain_address.c | 2 + .../caps_4.0.0.x86_64.xml | 2 + .../virtio-non-transitional.x86_64-3.1.0.args | 4 +- ...virtio-non-transitional.x86_64-latest.args | 4 +- .../virtio-transitional.x86_64-3.1.0.args | 5 +- .../virtio-transitional.x86_64-latest.args | 7 +- .../virtio-transitional.xml | 10 ++- 10 files changed, 117 insertions(+), 13 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list