Hi Eric, On Tue, Nov 17, 2020 at 1:55 PM Auger Eric <eric.auger@xxxxxxxxxx> wrote: > > Hi Vikas, > > On 11/17/20 9:05 AM, Auger Eric wrote: > > Hi Vikas, > > > > On 11/17/20 7:25 AM, Vikas Gupta wrote: > >> Hi Eric, > >> > >> On Mon, Nov 16, 2020 at 6:44 PM Auger Eric <eric.auger@xxxxxxxxxx> 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 <eric.auger@xxxxxxxxxx> 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. > > Given ERR and REQ indices would follow MSI and MSIX ones, MSI index > > could be recognized by the first index whose count != 1. But indeed I am > > not sure the number of supported vectors cannot be 1. In your case it is > > induced by the size of the ring so it is OK but for other devices this > > may be different. > > > > 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? > > If count is not reliable to discriminate the first n wired interrupts > > from the subsequen MSI and MSIx index, Alex suggested to add a > > capability extension in the vfio_irq_info structure. Something similar > > to what was done for vfio_region_info. > > > > Such kind of thing was attempted in > > https://lore.kernel.org/kvmarm/20201116110030.32335-8-eric.auger@xxxxxxxxxx/T/#u > > > > ` [PATCH v11 07/13] vfio: Use capability chains to handle device > > specific irq > > ` [PATCH v11 08/13] vfio/pci: Add framework for custom interrupt indices > > ` [PATCH v11 09/13] vfio: Add new IRQ for DMA fault reporting > > By the way I was mentionning MSI/MSIx in my previous reply but, as Alex > pointed out, with platform device only a single MSI index does make > sense, no? Yes, I think single MSI should be OK. This single MSI index should be implemented as ext_irqs, similar to, as you implemented in the mentioned patch. Is my understanding correct? Thanks, Vikas > > Thanks > > Eric > > > > Note this has not been reviewed yet. > > > > Thanks > > > > Eric > > > >>> > >>> > >>> > >>>> 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 > >>>>>> > >>>>> > >>> >
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
