lkml.org 
[lkml]   [2020]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC, v0 1/3] vfio/platform: add support for msi
    Hi Alex,

    On Mon, Nov 9, 2020 at 8:58 PM Alex Williamson
    <alex.williamson@redhat.com> wrote:
    >
    > 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.

    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
    >
    [unhandled content-type:application/pkcs7-signature]
    \
     
     \ /
      Last update: 2020-11-10 12:07    [W:4.965 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site