Date:   Mon, 16 Nov 2020 14:14:40 +0100
From:   Auger Eric <eric.auger@...hat.com>
To:     Vikas Gupta <vikas.gupta@...adcom.com>
Cc:     Alex Williamson <alex.williamson@...hat.com>,
        Cornelia Huck <cohuck@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Vikram Prakash <vikram.prakash@...adcom.com>,
        Srinath Mannam <srinath.mannam@...adcom.com>
Subject: Re: [RFC, v1 0/3] msi support for platform devices

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@...hat.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
>>>
>>

Powered by blists - more mailing lists