Re: [RFC, v0 1/3] vfio/platform: add support for msi

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 9 Nov 2020 12:11:15 +0530
Vikas Gupta <vikas.gupta@broadcom.com> wrote:

> Hi Alex,
> 
> On Fri, Nov 6, 2020 at 8:42 AM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Fri, 6 Nov 2020 08:24:26 +0530
> > Vikas Gupta <vikas.gupta@broadcom.com> wrote:
> >  
> > > Hi Alex,
> > >
> > > On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson
> > > <alex.williamson@redhat.com> wrote:  
> > > >
> > > > On Thu,  5 Nov 2020 11:32:55 +0530
> > > > Vikas Gupta <vikas.gupta@broadcom.com> 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.


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

Alex