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

[Auger Eric]

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

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