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. 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 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, Alex
