Re: [RFC, v1 0/3] msi support for platform devices

From: Vikas Gupta
Date: Tue Nov 17 2020 - 11:37:14 EST

Next message: Bjorn Helgaas: "Re: Time to re-enable Runtime PM per default for PCI devcies?"
Previous message: Marco Elver: "[PATCH] kcsan: Avoid scheduler recursion by using non-instrumented preempt_disable()"
In reply to: Auger Eric: "Re: [RFC, v1 0/3] msi support for platform devices"
Next in thread: Auger Eric: "Re: [RFC, v1 0/3] msi support for platform devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Bjorn Helgaas: "Re: Time to re-enable Runtime PM per default for PCI devcies?"
Previous message: Marco Elver: "[PATCH] kcsan: Avoid scheduler recursion by using non-instrumented preempt_disable()"
In reply to: Auger Eric: "Re: [RFC, v1 0/3] msi support for platform devices"
Next in thread: Auger Eric: "Re: [RFC, v1 0/3] msi support for platform devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]