[libvirt] [PATCH 1/2] qemu: assign correct type of PCI address for vhost-scsi when using pcie-root

[John Ferlan]

On 12/18/2017 10:35 AM, Laine Stump wrote:
> Commit 10c73bf1 fixed a bug that I had introduced back in commit
> 70249927 - if a vhost-scsi device had no manually assigned PCI
> address, one wouldn't be assigned automatically. There was a slight
> problem with the logic of the fix though - in the case of domains with
> pcie-root (e.g. those with a q35 machinetype),
> qemuDomainDeviceCalculatePCIConnectFlags() will attempt to determine
> if the host-side PCI device is Express or legacy by examining sysfs
> based on the host-side PCI address stored in
> hostdev->source.subsys.u.pci.addr, but that part of the union is only
> valid for PCI hostdevs, *not* for SCSI hostdevs. So we end up trying
> to read sysfs for some probably-non-existent device, which fails, and
> the function virPCIDeviceIsPCIExpress() returns failure (-1). By
> coincidence, the return value is being examined as a boolean, and
> since -1 is true, we still end up assigning the vhost-scsi device to
> an Express slot, but that is just be chance (and could fail in the
> case that the gibberish in the "hostside PCI address" was the address
> of a real device that happened to be legacy PCI).

^^  Probably something that should be fixed as a separate patch?
Hazards of the undocumented virPCIDeviceIsPCIExpress return values.

> 
> Since (according to Paolo Bonzini at least) vhost-scsi devices appear
> just like virtio-scsi devices in the guest, they should follow the
> same rules as virtio devices when deciding whether they should be
> placed in an Express or a legacy slot. That's accomplished in this
> patch by returning early with virtioFlags, rather than erroneously
> using hostdev->source.subsys.u.pci.addr. It also adds a test case for
> PCIe to assure it doesn't get broken in the future.
> ---
>  src/qemu/qemu_domain_address.c                     |  7 ++++
>  .../hostdev-scsi-vhost-scsi-pcie.args              | 25 ++++++++++++++
>  .../hostdev-scsi-vhost-scsi-pcie.xml               | 23 +++++++++++++
>  tests/qemuxml2argvtest.c                           |  7 ++++
>  .../hostdev-scsi-vhost-scsi-pcie.xml               | 40 ++++++++++++++++++++++
>  tests/qemuxml2xmltest.c                            |  7 ++++
>  6 files changed, 109 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.args
>  create mode 100644 tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.xml
>  create mode 100644 tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pcie.xml
> 

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

Thanks - I was wondering about PCIE, but figured/assumed that since it's
an HBA sitting on a bus as opposed to a LUN that we're making it look
like a disk on the guest, that the code would magically work. Just
seemed different enough from a MDEV which is to me more like how a vHBA
LUN would be handled as a "child" (so to speak) of some device that's
sitting on a bus.