Hi Alex, On Mon, Nov 9, 2020 at 8:58 PM Alex Williamson wrote: > > On Mon, 9 Nov 2020 12:11:15 +0530 > Vikas Gupta wrote: > > > Hi Alex, > > > > On Fri, Nov 6, 2020 at 8:42 AM Alex Williamson > > wrote: > > > > > > On Fri, 6 Nov 2020 08:24:26 +0530 > > > Vikas Gupta wrote: > > > > > > > Hi Alex, > > > > > > > > On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson > > > > wrote: > > > > > > > > > > On Thu, 5 Nov 2020 11:32:55 +0530 > > > > > Vikas Gupta wrote: > > > > > > > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > > > > index 2f313a238a8f..aab051e8338d 100644 > > > > > > --- a/include/uapi/linux/vfio.h > > > > > > +++ b/include/uapi/linux/vfio.h > > > > > > @@ -203,6 +203,7 @@ struct vfio_device_info { > > > > > > #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ > > > > > > #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ > > > > > > #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ > > > > > > +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ > > > > > > __u32 num_regions; /* Max region index + 1 */ > > > > > > __u32 num_irqs; /* Max IRQ index + 1 */ > > > > > > __u32 cap_offset; /* Offset within info struct of first cap */ > > > > > > > > > > This doesn't make any sense to me, MSIs are just edge triggered > > > > > interrupts to userspace, so why isn't this fully described via > > > > > VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, > > > > > this seems incomplete, which indexes are MSI (IRQ_INFO can describe > > > > > that)? We also already support MSI with vfio-pci, so a global flag for > > > > > the device advertising this still seems wrong. Thanks, > > > > > > > > > > Alex > > > > > > > > > Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) > > > > cannot be described using indexes. > > > > > > That would be news for vfio-pci which has been describing MSIs with > > > sub-indexes within indexes since vfio started. > > > > > > > In the patch set there is no difference between MSI and normal > > > > interrupt for VFIO_DEVICE_GET_IRQ_INFO. > > > > > > Then what exactly is a global device flag indicating? Does it indicate > > > all IRQs are MSI? > > > > No, it's not indicating that all are MSI. > > The rationale behind adding the flag to tell user-space that platform > > device supports MSI as well. As you mentioned recently added > > capabilities can help on this, I`ll go through that. > > > It still seems questionable to me to use a device info capability to > describe an interrupt index specific feature. The scope seems wrong. > Why does userspace need to know that this IRQ is MSI rather than > indicating it's simply an edge triggered interrupt? That can be done > using only vfio_irq_info.flags. Ok. In the next patch set I`ll remove the device flag (VFIO_DEVICE_FLAGS_MSI) as vfio_irq_info.flags should have enough information for edge triggered interrupt. > > > > > > The patch set adds MSI(s), say as an extension, to the normal > > > > interrupts and handled accordingly. > > > > > > So we have both "normal" IRQs and MSIs? How does the user know which > > > indexes are which? > > > > With this patch set, I think this is missing and user space cannot > > know that particular index is MSI interrupt. > > For platform devices there is no such mechanism, like index and > > sub-indexes to differentiate between legacy, MSI or MSIX as it’s there > > in PCI. > > Indexes and sub-indexes are a grouping mechanism of vfio to describe > related interrupts. That terminology doesn't exist on PCI either, it's > meant to be used generically. It's left to the vfio bus driver how > userspace associates a given index to a device feature. > > > I believe for a particular IRQ index if the flag > > VFIO_IRQ_INFO_NORESIZE is used then user space can know which IRQ > > index has MSI(s). Does it make sense? > > > No, no-resize is an implementation detail, not an indication of the > interrupt mechanism. It's still not clear to me why it's important to > expose to userspace that a given interrupt is MSI versus simply > exposing it as an edge interrupt (ie. automasked = false). If it is > necessary, the most direct approach might be to expose a capability > extension in the vfio_irq_info structure to describe it. Even then > though, I don't think simply exposing a index as MSI is very > meaningful. What is userspace intended to do differently based on this > information? Thanks, The current patch set is not setting VFIO_IRQ_INFO_AUTOMASKED (automasked=false) for MSIs so I believe this much is information enough for user space to know that this is an edge triggered interrupt. I agree that exposing an index as MSI is not meaningful as user space has nothing special to do with this information. > > Alex >