Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

On Mon, 2018-07-23 at 13:37 +0200, Ján Tomko wrote: > On Tue, Jul 10, 2018 at 04:02:14PM +0800, Yi Min Zhao wrote: >> Abstract >> ======== >> The PCI representation in QEMU has recently been extended for S390 >> allowing configuration of zPCI attributes like uid (user-defined >> identifier) and fid (PCI function identifier). >> The details can be found here: >> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html >> >> To support the new zPCI feature of the S390 platform, two new XML >> attributes, @uid and @fid, are introduced for device addresses of type >> 'pci', i.e.: >> <hostdev mode='subsystem' type='pci'> >> <driver name='vfio'/> >> <source> >> <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> >> </source> >> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' >> uid='0x0003' fid='0x00000027'/> >> </hostdev> >> >> uid and fid are optional attributes. If they are defined by the user, >> unique values within the guest domain must be used. If they are not >> specified and the architecture requires them, they are automatically >> generated with non-conflicting values. >> >> Current implementation is the most seamless one for the user as it >> unites the address specific data of a PCI device on one XML element. >> It could accommodate both specifying our special parameters (uid and fid) >> and re-using standard statements (domain, bus, slot and function) for >> PCI devices. User can still specify bus/slot/function for the virtualized >> PCI devices in the XML. >> >> Thus uid/fid act as an extension to the PCI address and are stored in >> a new structure 'virZPCIDeviceAddress' which is a member of common PCI >> Address structure. Additionally, two hashtables are used for assignment >> and reservation of uid/fid. >> >> In support of extending the PCI address, a new PCI address extension flag is >> introduced. This extension flag allows is not only dedicated for the S390 >> platform but also other architectures needing certain extensions to PCI >> address space. [...] > > The patches look OK to me, therefore: > > Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; > > But I'd like to hear from some other developer who touched the PCI code > last (Laine? Andrea?) Thanks for bringing this to my attention - it had somehow managed to escape my radar so far :) I've quickly gone through the patches and spotted some minor code style issues that I'd like to see addressed (I'll point them out separately); first, though, I have a couple of questions about the general idea behind the feature. Looking at both the generated QEMU command line and the qemu-devel message linked above, I seem to understand the zpci device is basically some sort of adapter that sits between a regular PCI device (emulated or otherwise) and an s390 guest and presents an ID-based interface to the latter; so IIUC the s390 guest doesn't actually see a PCI device, but a couple of IDs that can (somehow) be used to access the underlying PCI device. >From whatever little s390 knowledge I have, I recall the architecture using peculiar ways to address and access devices, so PCI not being usable wouldn't surprise me that much; what I find odd, however, is that regular PCI seems to be available at least on the host side, because otherwise stuff like -device zpci,uid=1,fid=1,target=vpci0 -device vfio-pci,host=0000:00:00.0,id=vpci0 wouldn't be possible. So, would anyone with s390 knowledge please spend a few words explaining how the various pieces fit together? Assuming the above is a correct reading of the situation, it would seem the IDs are the only guest-visible part that we need to make sure doesn't change during the lifetime of the guest, while the usual bus/slot/function triplet assigned to the underlying PCI device doesn't actually matter. And if that's the case, wouldn't something like <address type='zpci' uid='1' fid='1'/>

be a better representation of the arrangement? We could leave it up to QEMU to assign addresses to the PCI devices in this case... But maybe they still matter for things such as migration? Or maybe I've just gotten it wrong altogether? :)