Re: [libvirt] [PATCH v2 1/5] Introduce a "scsi_host" hostdev type

On 09/06/2016 08:58 AM, Eric Farman wrote: I assume from the review of v1 this is both HBA and vHBA? That is a vHBA would be useful for the NPIV support to pass through the entire vHBA rather than individual LUN's (something recently asked for at KVM Forum). The vHBA is created when one has a vport capable scsi_host (see http://wiki.libvirt.org/page/NPIV_in_libvirt). The "result" is a 'scsi_hostM' using a vport capable parent scsi_hostN. Now if a vHBA cannot (or should not) be passed through, then we'll need to be sure to test and avoid it. Unfortunately we had a lab issue and the system I normally use for my vHBA/NPIV work was affected - trying to get it resurected remotely... I took a peek at the v1, but suffice to say didn't follow it that closely since there's a number of changes in v2 related to patch ordering. You could get 3 different opinions on that matter too! Typically I'll try to add the qemu code first (capabilities first, then command in one patch and hotplug in the next one). Once that's in place, then I add the domain bits, docs, etc. Since you're extending the domain XML, you will need to add an xml2xml test in this patch. Use the XML from your patch 5 as a basis/guide. You'll need a "tests/qemuxml2argvdata/" file and a "tests/qemuxml2xmloutdata/" file as well as a changed to qemuxml2xmltest.c. If the data file is the same as the outdata file, then you can create a soft link to the data file in the outdata file directory. A wwpn doesn't generally have the "naa." prefix on it. So, why not just add that to the command line instead? That is a wwpn is a 16 hex digit value. I'm assuming that in order to support this feature a "naa." is prefixed and sent to qemu. Does that necessarily mean some other hypervisor would choose to place the "naa." prefix or would then code need to be added to strip it off for those other hypervisors? Looking at the qemu code - it seems fairly specific. It'll be at least 2.3.0 I see from the qemu code there's support for attributes 'num_queues', 'max_sectors', and 'cmd_per_lun' - since this is passing the scsi_host through and not a 'scsi' LUN with a 'scsi' controller, should those attributes be added as well? Not "required" for this version, but I would hope we could add caps and attributes for that as well because I can almost guarantee someone will ask. 2.3 s/HBI/HBA I don't think we mention the "naa." prefix... It would be nice to tell a user how to find that wwpn value via some command (virsh or otherwise) in order to fill in the wwpn attribute. Although there's always varying opinions on this since many times it's distro dependent. That's why I suggest virsh - at least if it's supportable we can display the wwpn there using nodedev-dumpxml. What/where is the 'vhost_scsi_wwpn'? IOW: It's not something defined yet - I'm assuming there's some output somewhere where that's pulled from. One final question - what is this exposed as on the guest? "Some" scsi_hostM? For a SCSI LUN it's some 'sgN' value. I see the qemu side seems to infer some amount of scsi_generic code (read_naa_id), so it's mostly curiosity/learning for me. If you go with the we'll add "naa." later, then you could just: <ref name='wwn'/> instead of <data ...> ... </data> Can't we produce something here? Not sure seeing "?" in the output of an audit message would be useful. Perhaps the path to the device similar to how we end up in virDomainAuditGenericDev... Notice how the virDomainHostdevSubsysSCSIProtocol adds 'adapter' first - this should do something similar (although the preference is to use "none" - I cannot recall why that was different). Remember that all structs are "calloc"'d and thus making sure this setting to something other than 0 will ensure we haven't missed something. This code would do the "protocol" parsing. I believe the way it's written now protocol goes unchecked. You need to add code to use the virDomainHostdevSubsysHostProtocolTypeFromString in order to validate what was supplied in the XML is correct. See virDomainHostdevSubsysSCSIDefParseXML - although make your ->protocol comparison <= 0 since we don't want a "0" value. (Yes, there's a bug in the SCSIDefParse I think - although it's caught later on). If we take the don't require a "naa." prefix, then virValidateWWN should be used to validate things. Even if naa. was kept, the same validation should be done for consistency. FWIW: If some day in the future type='scsi_host' added some other "protocol" (something other than vhost), then at that time this code would be extended to handle that. VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_NONE, Add the NONE here. hmmm... If you're going to change this here, then qemuDomainAttachHostDevice would be changed at the same time. Hence why this ordering stuff to patches is important. I cannot recall if removing would cause a build error in this case. Why is only security_dac.c changed? Usually _selinux and _apparmor.c are also changed, which I do see happening in your v1. Although I wouldn't classify a "vhost" and an "iSCSI" host as the same thing. The latter is a networked resource; whereas, IIUC this new device is essentially a file descriptor that's being passed along to qemu. So there must be other examples of that in the security_*.c modules. This particular area I usually don't get right either, but what I'll do is follow the history of the changes and see if something is "similar" to what I'm creating and then just copy that model. Makes me wonder how this plays in a non-privileged environment (e.g. session mode). Is it even allowed to open/read vhost-scsi. FWIW: I usually use cscope to find all VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_ occurrances and be sure the _HOST option is covered. I see that nothing in cgroup is modified. I also wonder if we need to track these in virhostdev.c. John