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