> From: Alex Williamson <alex.williamson@xxxxxxxxxx> > Sent: Thursday, November 2, 2023 2:07 AM > > On Tue, 31 Oct 2023 07:31:24 +0000 > "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote: > > > > From: Chatre, Reinette <reinette.chatre@xxxxxxxxx> > > > Sent: Saturday, October 28, 2023 1:01 AM > > > > > > Changes since RFC V2: > > > - RFC V2: > > > > https://lore.kernel.org/lkml/cover.1696609476.git.reinette.chatre@xxxxxxxxx > > > / > > > - Still submiting this as RFC series. I believe that this now matches the > > > expectatations raised during earlier reviews. If you agree this is > > > the right direction then I can drop the RFC prefix on next submission. > > > If you do not agree then please do let me know where I missed > > > expectations. > > > > Overall this matches my expectation. Let's wait for Alex/Jason's thoughts > > before moving to next-level refinement. > > It feels like there's a lot of gratuitous change without any clear > purpose. We create an ops structure so that a variant/mdev driver can > make use of the vfio-pci-core set_irqs ioctl piecemeal, but then the > two entry points that are actually implemented by the ims version are > the same as the core version, so the ops appear to be at the wrong > level. The use of the priv pointer for the core callbacks looks like > it's just trying to justify the existence of the opaque pointer, it > should really just be using container_of(). We drill down into various > support functions for MSI (ie. enable, disable, request_interrupt, > free_interrupt, device name), but INTx is largely ignored, where we > haven't even kept is_intx() consistent with the other helpers. All above are good points. The main interest of this series is to share MSI frontend interface with various backends (PCI MSI/X, IMS, and purely emulated). From this angle the current ops abstraction does sound to sit in a wrong level. But if counting your suggestion to also refactor mdev sample driver (e.g. mtty emulates INTx) then there might be a different outcome. > > Without an in-tree user of this code, we're just chopping up code for > no real purpose. There's no reason that a variant driver requiring IMS > couldn't initially implement their own SET_IRQS ioctl. Doing that this is an interesting idea. We haven't seen a real usage which wants such MSI emulation on IMS for variant drivers. but if the code is simple enough to demonstrate the 1st user of IMS it might not be a bad choice. There are additional trap-emulation required in the device MMIO bar (mostly copying MSI permission entry which contains PASID info to the corresponding IMS entry). At a glance that area is 4k-aligned so should be doable. let's explore more into this option. > might lead to a more organic solution where we create interfaces where > they're actually needed. The existing mdev sample drivers should also > be considered in any schemes to refactor the core code into a generic > SET_IRQS helper for devices exposing a vfio-pci API. Thanks, > In this case we'll have mtty to demonstrate an emulated INTx backend and intel vgpu to contain an emulated MSI backend. and moving forward all drivers with a vfio-pci API should share a same frontend interface.
