mbox series

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

Message ID 20201112175852.21572-1-vikas.gupta@broadcom.com (mailing list archive)
Headers show
Series msi support for platform devices | expand

Message

Vikas Gupta Nov. 12, 2020, 5:58 p.m. UTC
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|
       -------------------------------------------------------
       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

Comments

Auger Eric Nov. 12, 2020, 6:40 p.m. UTC | #1
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.
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)

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
>
Vikas Gupta Nov. 13, 2020, 5:24 p.m. UTC | #2
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
> >
>
Auger Eric Nov. 16, 2020, 1:14 p.m. UTC | #3
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@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)|
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 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.



> 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?

>    };
>                          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
>>>
>>
Vikas Gupta Nov. 17, 2020, 6:25 a.m. UTC | #4
Hi Eric,

On Mon, Nov 16, 2020 at 6:44 PM Auger Eric <eric.auger@redhat.com> 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@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)|
> 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
> >>>
> >>
>
Auger Eric Nov. 17, 2020, 8:05 a.m. UTC | #5
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@redhat.com> 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@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)|
>> 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@redhat.com/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

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
>>>>>
>>>>
>>
Auger Eric Nov. 17, 2020, 8:25 a.m. UTC | #6
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@redhat.com> 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@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)|
>>> 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@redhat.com/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?

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
>>>>>>
>>>>>
>>>
Vikas Gupta Nov. 17, 2020, 4:36 p.m. UTC | #7
Hi Eric,

On Tue, Nov 17, 2020 at 1:55 PM Auger Eric <eric.auger@redhat.com> 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@redhat.com> 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@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)|
> >>> 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@redhat.com/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
> >>>>>>
> >>>>>
> >>>
>
Auger Eric Nov. 18, 2020, 11 a.m. UTC | #8
Hi Vikas,

On 11/17/20 5:36 PM, Vikas Gupta wrote:
> Hi Eric,
> 
> On Tue, Nov 17, 2020 at 1:55 PM Auger Eric <eric.auger@redhat.com> 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@redhat.com> 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@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)|
>>>>> 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@redhat.com/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?
Yes, if count !=1 cannot be used to detect the MSI index, I think using
a capability would do the job and this is aligned with last Alex'
suggestion.

Thanks

Eric
> 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
>>>>>>>>
>>>>>>>
>>>>>
>>