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

[Auger Eric <eric.auger@redhat.com>]

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