Re: [libvirt] [PATCH 1/3] qemu: Need to assign PCI address to vhost-scsi

On 12/13/2017 08:28 AM, Eric Farman wrote: > > > On 12/12/2017 09:22 AM, John Ferlan wrote: >> Commit id '70249927b' neglected to cover this case because the test >> had taken the "shortcut" to already add the <address>, so I altered >> that too just to make sure it doesn't happen again. > > My bad.  :( > No worries - I probably encouraged it as part of a test files reduction plan. It only became a problem once the code to assign PCI addresses got adjusted. Of course I neglected to make that attribution, so I adjusted this first paragraph to: Commit id '70249927b' neglected to cover this case because the test had taken the "shortcut" to already add the <address>; however, when the PCI address assignment code was adjusted by commit id '70249927' the vhost-scsi (VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) wasn't covered thus returning a 0 for pciFlags. So I altered the tests too to make sure it doesn't happen again.

Thanks for the quick review!

John >> >> Previously the qemuxml2xmloutdata was a softlink to the source >> qemuxml2argvdata, so I unlinked and recreated the output file to >> force generation of the adddress. Without the test changes, an >> address generation returns: >> >>      libvirt: Domain Config error : internal error: Cannot automatically >>      add a new PCI bus for a device with connect flags 00 >> >> if an address was supplied in the test, a restart of libvirtd or >> edit of a guest would display the following opaque message: >> >>      warning : qemuDomainCollectPCIAddress:1237 : >>      qemuDomainDeviceCalculatePCIConnectFlags() thinks that the device >>       with PCI address 0000:00:09.0 should not have a PCI address >> >> where the address is related to the guest PCI address provided. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > > Reviewed-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; > >> --- >>   src/qemu/qemu_domain_address.c                     |  3 +- >>   .../hostdev-scsi-vhost-scsi-ccw.xml                |  1 - >>   .../hostdev-scsi-vhost-scsi-pci.xml                |  1 - >>   .../hostdev-scsi-vhost-scsi-ccw.xml                | 35 >> +++++++++++++++++- >>   .../hostdev-scsi-vhost-scsi-pci.xml                | 43 >> +++++++++++++++++++++- >>   5 files changed, 78 insertions(+), 5 deletions(-) >>   mode change 120000 => 100644 >> tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-ccw.xml >>   mode change 120000 => 100644 >> tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pci.xml >> >> diff --git a/src/qemu/qemu_domain_address.c >> b/src/qemu/qemu_domain_address.c >> index a40cdb399..6e7561d39 100644 >> --- a/src/qemu/qemu_domain_address.c >> +++ b/src/qemu/qemu_domain_address.c >> @@ -623,7 +623,8 @@ >> qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, >> >>           if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || >>               (hostdev->source.subsys.type != >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && >> -             hostdev->source.subsys.type != >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV)) { >> +             hostdev->source.subsys.type != >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV && >> +             hostdev->source.subsys.type != >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST)) { >>               return 0; >>           } >> >> diff --git a/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-ccw.xml >> b/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-ccw.xml >> index 6012d241c..33234bc3d 100644 >> --- a/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-ccw.xml >> +++ b/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-ccw.xml >> @@ -24,7 +24,6 @@ >>       </controller> >>       <hostdev mode='subsystem' type='scsi_host' managed='no'> >>         <source protocol='vhost' wwpn='naa.5123456789abcde0'/> >> -      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0002'/> >>       </hostdev> >>       <memballoon model='virtio'> >>         <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0003'/> >> diff --git a/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pci.xml >> b/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pci.xml >> index cb97544d3..13455f672 100644 >> --- a/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pci.xml >> +++ b/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pci.xml >> @@ -33,7 +33,6 @@ >>       <input type='keyboard' bus='ps2'/> >>       <hostdev mode='subsystem' type='scsi_host' managed='no'> >>         <source protocol='vhost' wwpn='naa.5123456789abcde0'/> >> -      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' >> function='0x0'/> >>       </hostdev> >>       <memballoon model='virtio'> >>         <address type='pci' domain='0x0000' bus='0x00' slot='0x04' >> function='0x0'/> >> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-ccw.xml >> b/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-ccw.xml >> deleted file mode 120000 >> index 39ff65968..000000000 >> --- a/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-ccw.xml >> +++ /dev/null >> @@ -1 +0,0 @@ >> -../qemuxml2argvdata/hostdev-scsi-vhost-scsi-ccw.xml >> \ No newline at end of file >> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-ccw.xml >> b/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-ccw.xml >> new file mode 100644 >> index 000000000..6012d241c >> --- /dev/null >> +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-ccw.xml >> @@ -0,0 +1,34 @@ >> +<domain type='qemu'> >> +  <name>QEMUGuest2</name> >> +  <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> >> +  <memory unit='KiB'>219100</memory> >> +  <currentMemory unit='KiB'>219100</currentMemory> >> +  <vcpu placement='static'>1</vcpu> >> +  <os> >> +    <type arch='s390x' machine='s390-ccw'>hvm</type> >> +    <boot dev='hd'/> >> +  </os> >> +  <clock offset='utc'/> >> +  <on_poweroff>destroy</on_poweroff> >> +  <on_reboot>restart</on_reboot> >> +  <on_crash>destroy</on_crash> >> +  <devices> >> +    <emulator>/usr/bin/qemu-system-s390x</emulator> >> +    <disk type='block' device='disk'> >> +      <source dev='/dev/HostVG/QEMUGuest2'/> >> +      <target dev='hda' bus='virtio'/> >> +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> >> +    </disk> >> +    <controller type='scsi' index='0' model='virtio-scsi'> >> +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/> >> +    </controller> >> +    <hostdev mode='subsystem' type='scsi_host' managed='no'> >> +      <source protocol='vhost' wwpn='naa.5123456789abcde0'/> >> +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0002'/> >> +    </hostdev> >> +    <memballoon model='virtio'> >> +      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0003'/> >> +    </memballoon> >> +    <panic model='s390'/> >> +  </devices> >> +</domain> >> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pci.xml >> b/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pci.xml >> deleted file mode 120000 >> index b17ea17c8..000000000 >> --- a/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pci.xml >> +++ /dev/null >> @@ -1 +0,0 @@ >> -../qemuxml2argvdata/hostdev-scsi-vhost-scsi-pci.xml >> \ No newline at end of file >> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pci.xml >> b/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pci.xml >> new file mode 100644 >> index 000000000..cb97544d3 >> --- /dev/null >> +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pci.xml >> @@ -0,0 +1,42 @@ >> +<domain type='qemu'> >> +  <name>QEMUGuest2</name> >> +  <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> >> +  <memory unit='KiB'>219100</memory> >> +  <currentMemory unit='KiB'>219100</currentMemory> >> +  <vcpu placement='static'>1</vcpu> >> +  <os> >> +    <type arch='i686' machine='pc'>hvm</type> >> +    <boot dev='hd'/> >> +  </os> >> +  <clock offset='utc'/> >> +  <on_poweroff>destroy</on_poweroff> >> +  <on_reboot>restart</on_reboot> >> +  <on_crash>destroy</on_crash> >> +  <devices> >> +    <emulator>/usr/bin/qemu-system-i686</emulator> >> +    <disk type='block' device='disk'> >> +      <source dev='/dev/HostVG/QEMUGuest2'/> >> +      <target dev='hda' bus='ide'/> >> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/> >> +    </disk> >> +    <controller type='scsi' index='0' model='virtio-scsi'> >> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' >> function='0x0'/> >> +    </controller> >> +    <controller type='usb' index='0'> >> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' >> function='0x2'/> >> +    </controller> >> +    <controller type='ide' index='0'> >> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' >> function='0x1'/> >> +    </controller> >> +    <controller type='pci' index='0' model='pci-root'/> >> +    <input type='mouse' bus='ps2'/> >> +    <input type='keyboard' bus='ps2'/> >> +    <hostdev mode='subsystem' type='scsi_host' managed='no'> >> +      <source protocol='vhost' wwpn='naa.5123456789abcde0'/> >> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' >> function='0x0'/> >> +    </hostdev> >> +    <memballoon model='virtio'> >> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' >> function='0x0'/> >> +    </memballoon> >> +  </devices> >> +</domain> >> >