On Tue, Aug 09, 2016 at 10:44:44AM -0400, Eric Farman wrote: > > > 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@xxxxxxxxxxxxxxxxxx> > > > > Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx> > > > > --- > > > > .../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. No, it should be type=scsi_host, so that in the future if we want to pass through entire SCSI or iSCSI HBAs without vhost-scsi, then we can do it. > (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?) The IQN name has to include the LUN in it - if that's left out, QEMU would complain that no LUN was specified. > > 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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list