Re: [libvirt] [PATCH v1 17/19] tests: Introduce basic vhost-scsi test

On 08/09/2016 08:22 AM, Daniel P. Berrange wrote: > On Tue, Aug 09, 2016 at 02:13:29PM +0200, Michal Privoznik wrote: > > On 25.07.2016 22:48, Eric Farman wrote: > > > The qemuxml2argv test was cloned from hostdev-scsi-virtio-scsi > > > > > > Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; > > > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; > > > --- > > > .../qemuxml2argv-hostdev-scsi-vhost-scsi.args | 24 ++++++++++++++++ > > > .../qemuxml2argv-hostdev-scsi-vhost-scsi.xml | 33 ++++++++++++++++++++++ > > > tests/qemuxml2argvmock.c | 12 ++++++++ > > > tests/qemuxml2argvtest.c | 3 ++ > > > 4 files changed, 72 insertions(+) > > > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args > > > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml > > > > > > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args > > > new file mode 100644 > > > index 0000000..a2c7c18 > > > --- /dev/null > > > +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args > > > @@ -0,0 +1,24 @@ > > > +LC_ALL=C \ > > > +PATH=/bin \ > > > +HOME=/home/test \ > > > +USER=test \ > > > +LOGNAME=test \ > > > +QEMU_AUDIO_DRV=none \ > > > +/usr/bin/qemu \ > > > +-name QEMUGuest2 \ > > > +-S \ > > > +-M pc \ > > > +-m 214 \ > > > +-smp 1 \ > > Because this patch has been sitting on the list for too long, this part > > of command line is now being generated slightly differently. Otherwise > > looking good. Fair enough, that's probably a common thread throughout your comments. Easy to fixup. > > > > > +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \ > > > +-nographic \ > > > +-nodefaults \ > > > +-monitor unix:/tmp/lib/domain--1-QEMUGuest2/monitor.sock,server,nowait \ > > > +-no-acpi \ > > > +-boot c \ > > > +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ > > > +-usb \ > > > +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ > > > +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ > > > +-device vhost-scsi-pci,wwpn=naa.5123456789abcde0,vhostfd=3,id=hostdev0 \ > > > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 > > > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml > > > new file mode 100644 > > > index 0000000..3fc4ef0 > > > --- /dev/null > > > +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml > > > @@ -0,0 +1,33 @@ > > > +<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</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'/> > > > + <controller type='usb' index='0'/> > > > + <controller type='ide' index='0'/> > > > + <controller type='pci' index='0' model='pci-root'/> > > > + <input type='mouse' bus='ps2'/> > > > + <input type='keyboard' bus='ps2'/> > > > + <hostdev mode='subsystem' type='scsi'> > > > + <source protocol='vhost' wwpn='naa.5123456789abcde0'/> > > > + </hostdev> > I'm not sure this syntax really makes sense. > > IIRC, currently <hostdev type="scsi"> is used to passthrough an individual > SCSI LUNs from the host to the guest. > > With vhost-scsi, IIUC, you are passing through an entire (virtual) SCSI HBA > to the guest. So I think it better for us to not reuse type="scsi" for that > purpose. Instead it feels like we should be adding a type="scsi_host" for > passthrough of entire HBAs Would that cause confusion amongst an existing type='scsi' hostdev, since a "scsi_hostX" turns up in the source tag? Example from the libvirt doc: <devices> <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'> <source> <adapter name='scsi_host0'/> <address bus='0' target='0' unit='0'/> </source> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </hostdev> </devices> Could make it be "type='vhost_scsi'" or something else, if that clarifies things.

(Question from my own ignorance; does an iSCSI target with more than one LUN behind it get rejected if specified here, instead of using a <pool> tag?)

> Obviously this has major implications for almost all patches in this series Splitting this out as a new hostdev type would mean it's not being shoehorned into the existing SCSI parts. That would probably make the entire series less unwieldy by dropping the separate protocol that exists herein.