Hi Eric, On Mon, Nov 16, 2020 at 6:44 PM Auger Eric <[email protected]> wrote: > > Hi Vikas, > > On 11/13/20 6:24 PM, Vikas Gupta wrote: > > Hi Eric, > > > > On Fri, Nov 13, 2020 at 12:10 AM Auger Eric <[email protected]> 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)| > No I really meant this list of indices: IRQ0|IRQ1|......|IRQ-(n-1)|MSI|MSIX| > and potentially later on IRQ0|IRQ1|......|IRQ-(n-1)|MSI|MSIX| ERR| REQ > if ERR/REQ were to be added. I agree on this. Actually the map I drew incorrectly above but wanted to demonstrate the same. It was a child-parent relationship for MSI and its members and similarly for MSIX as well. > > I think the userspace could query the total number of indices using > VFIO_DEVICE_GET_INFO and retrieve num_irqs (corresponding to the n wire > interrupts + MSI index + MSIX index) > > Then userspace can loop on all the indices using > VFIO_DEVICE_GET_IRQ_INFO. For each index it uses count to determine the > first indices related to wire interrupts (count = 1). Then comes the MSI > index, and after the MSI index. If any of those is supported, count >1, > otherwise count=0. The only thing I am dubious about is can the device > use a single MSI/MSIX? Because my hypothesis here is we use count to > discriminate between wire first indices and other indices. I believe count can be one as well, especially for ERR/REQ as you mentioned above.I think we can not rely on the count > 1. Now, this is blocking and we are not left with options unless we consider adding more enums in flags in vfio_irq_info to tell userspace that particular index is wired, MSI, MSIX etc. for the platform device. What do you think? > > > > > With this implementation user space can know that, at indexes n and > > n+1, edge triggered interrupts are present. > note wired interrupts can also be edge ones. > > 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.As wired > > interrupts and MSI interrupts coexist, I would store in vdev an > array of wired interrupts (the existing vdev->irqs) and a new array for > MSI(x) as done in the PCI code. > > vdev->ctx = kcalloc(nvec, sizeof(struct vfio_pci_irq_ctx), GFP_KERNEL); > > Does it make sense? Yes, we can use similar kinds of allocations.
Thanks,
Vikas
>
> > };
> > 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.
> indeed, but can't you loop over all indices looking until count !=1? At
> this point you know if have finished emurating the wires. Holds if
> MSI(x) count !=1 of course.
>
> Thanks
>
> Eric
>
> >
> > 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
> >>>
> >>
>
smime.p7s
Description: S/MIME Cryptographic Signature
