Hi Eric,

On Fri, Nov 13, 2020 at 12:10 AM Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Vikas,
>
> On 11/12/20 6:58 PM, Vikas Gupta wrote:
> > This RFC adds support for MSI for platform devices.
> > a) MSI(s) is/are added in addition to the normal interrupts.
> > b) The vendor specific MSI configuration can be done using
> >    callbacks which is implemented as msi module.
> > c) Adds a msi handling module for the Broadcom platform devices.
> >
> > Changes from:
> > -------------
> >  v0 to v1:
> >    i)  Removed MSI device flag VFIO_DEVICE_FLAGS_MSI.
> >    ii) Add MSI(s) at the end of the irq list of platform IRQs.
> >        MSI(s) with first entry of MSI block has count and flag
> >        information.
> >        IRQ list: Allocation for IRQs + MSIs are allocated as below
> >        Example: if there are 'n' IRQs and 'k' MSIs
> >        -------------------------------------------------------
> >        |IRQ-0|IRQ-1|....|IRQ-n|MSI-0|MSI-1|MSI-2|......|MSI-k|
> >        -------------------------------------------------------
> I have not taken time yet to look at your series, but to me you should have
> |IRQ-0|IRQ-1|....|IRQ-n|MSI|MSIX
> then for setting a given MSIX (i) you would select the MSIx index and
> then set start=i count=1.

As per your suggestion, we should have, if there are n-IRQs, k-MSIXs
and m-MSIs, allocation of IRQs should be done as below

|IRQ0|IRQ1|......|IRQ-(n-1)|MSI|MSIX|
                                             |        |
                                             |
|MSIX0||MSIX1||MSXI2|....|MSIX-(k-1)|
                                             |MSI0||MSI1||MSI2|....|MSI-(m-1)|
With this implementation user space can know that, at indexes n and
n+1, edge triggered interrupts are present.
   We may add an element in vfio_platform_irq itself to allocate MSIs/MSIXs
   struct vfio_platform_irq{
   .....
   .....
   struct vfio_platform_irq *block; => this points to the block
allocation for MSIs/MSIXs and all msi/msix are type of IRQs.
   };
                         OR
Another structure can be defined in 'vfio_pci_private.h'
struct vfio_msi_ctx {
        struct eventfd_ctx      *trigger;
        char                    *name;
};
and
struct vfio_platform_irq {
  .....
  .....
  struct vfio_msi_ctx *block; => this points to the block allocation
for MSIs/MSIXs
};
Which of the above two options sounds OK to you? Please suggest.

> to me individual MSIs are encoded in the subindex and not in the index.
> The index just selects the "type" of interrupt.
>
> For PCI you just have:
>         VFIO_PCI_INTX_IRQ_INDEX,
>         VFIO_PCI_MSI_IRQ_INDEX, -> MSI index and then you play with
> start/count
>         VFIO_PCI_MSIX_IRQ_INDEX,
>         VFIO_PCI_ERR_IRQ_INDEX,
>         VFIO_PCI_REQ_IRQ_INDEX,
>
> (include/uapi/linux/vfio.h)

In pci case, type of interrupts is fixed so they can be 'indexed' by
these enums but for VFIO platform user space will need to iterate all
(num_irqs) indexes to know at which indexes edge triggered interrupts
are present.

Thanks,
Vikas
>
> Thanks
>
> Eric
> >        MSI-0 will have count=k set and flags set accordingly.
> >
> > Vikas Gupta (3):
> >   vfio/platform: add support for msi
> >   vfio/platform: change cleanup order
> >   vfio/platform: add Broadcom msi module
> >
> >  drivers/vfio/platform/Kconfig                 |   1 +
> >  drivers/vfio/platform/Makefile                |   1 +
> >  drivers/vfio/platform/msi/Kconfig             |   9 +
> >  drivers/vfio/platform/msi/Makefile            |   2 +
> >  .../vfio/platform/msi/vfio_platform_bcmplt.c  |  74 ++++++
> >  drivers/vfio/platform/vfio_platform_common.c  |  86 ++++++-
> >  drivers/vfio/platform/vfio_platform_irq.c     | 238 +++++++++++++++++-
> >  drivers/vfio/platform/vfio_platform_private.h |  23 ++
> >  8 files changed, 419 insertions(+), 15 deletions(-)
> >  create mode 100644 drivers/vfio/platform/msi/Kconfig
> >  create mode 100644 drivers/vfio/platform/msi/Makefile
> >  create mode 100644 drivers/vfio/platform/msi/vfio_platform_bcmplt.c
> >
>