| Message ID | 20201105060257.35269-2-vikas.gupta@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v0,1/3] vfio/platform: add support for msi | expand |
On Thu, 5 Nov 2020 11:32:55 +0530 Vikas Gupta <vikas.gupta@broadcom.com> wrote: > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 2f313a238a8f..aab051e8338d 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -203,6 +203,7 @@ struct vfio_device_info { > #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ > #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ > #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ > +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ > __u32 num_regions; /* Max region index + 1 */ > __u32 num_irqs; /* Max IRQ index + 1 */ > __u32 cap_offset; /* Offset within info struct of first cap */ This doesn't make any sense to me, MSIs are just edge triggered interrupts to userspace, so why isn't this fully described via VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, this seems incomplete, which indexes are MSI (IRQ_INFO can describe that)? We also already support MSI with vfio-pci, so a global flag for the device advertising this still seems wrong. Thanks, Alex
Hi Alex, On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson <alex.williamson@redhat.com> wrote: > > On Thu, 5 Nov 2020 11:32:55 +0530 > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > index 2f313a238a8f..aab051e8338d 100644 > > --- a/include/uapi/linux/vfio.h > > +++ b/include/uapi/linux/vfio.h > > @@ -203,6 +203,7 @@ struct vfio_device_info { > > #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ > > #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ > > #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ > > +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ > > __u32 num_regions; /* Max region index + 1 */ > > __u32 num_irqs; /* Max IRQ index + 1 */ > > __u32 cap_offset; /* Offset within info struct of first cap */ > > This doesn't make any sense to me, MSIs are just edge triggered > interrupts to userspace, so why isn't this fully described via > VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, > this seems incomplete, which indexes are MSI (IRQ_INFO can describe > that)? We also already support MSI with vfio-pci, so a global flag for > the device advertising this still seems wrong. Thanks, > > Alex > Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) cannot be described using indexes. In the patch set there is no difference between MSI and normal interrupt for VFIO_DEVICE_GET_IRQ_INFO. The patch set adds MSI(s), say as an extension, to the normal interrupts and handled accordingly. Do you see this is a violation? If yes, then we`ll think of other possible ways to support MSI for the platform devices. Macro VFIO_DEVICE_FLAGS_MSI can be changed to any other name if it collides with an already supported vfio-pci or if not necessary, we can remove this flag. Thanks, Vikas
On Fri, 6 Nov 2020 08:24:26 +0530 Vikas Gupta <vikas.gupta@broadcom.com> wrote: > Hi Alex, > > On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson > <alex.williamson@redhat.com> wrote: > > > > On Thu, 5 Nov 2020 11:32:55 +0530 > > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > index 2f313a238a8f..aab051e8338d 100644 > > > --- a/include/uapi/linux/vfio.h > > > +++ b/include/uapi/linux/vfio.h > > > @@ -203,6 +203,7 @@ struct vfio_device_info { > > > #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ > > > #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ > > > #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ > > > +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ > > > __u32 num_regions; /* Max region index + 1 */ > > > __u32 num_irqs; /* Max IRQ index + 1 */ > > > __u32 cap_offset; /* Offset within info struct of first cap */ > > > > This doesn't make any sense to me, MSIs are just edge triggered > > interrupts to userspace, so why isn't this fully described via > > VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, > > this seems incomplete, which indexes are MSI (IRQ_INFO can describe > > that)? We also already support MSI with vfio-pci, so a global flag for > > the device advertising this still seems wrong. Thanks, > > > > Alex > > > Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) > cannot be described using indexes. That would be news for vfio-pci which has been describing MSIs with sub-indexes within indexes since vfio started. > In the patch set there is no difference between MSI and normal > interrupt for VFIO_DEVICE_GET_IRQ_INFO. Then what exactly is a global device flag indicating? Does it indicate all IRQs are MSI? > The patch set adds MSI(s), say as an extension, to the normal > interrupts and handled accordingly. So we have both "normal" IRQs and MSIs? How does the user know which indexes are which? > Do you see this is a violation? If Seems pretty unclear and dubious use of a global device flag. > yes, then we`ll think of other possible ways to support MSI for the > platform devices. > Macro VFIO_DEVICE_FLAGS_MSI can be changed to any other name if it > collides with an already supported vfio-pci or if not necessary, we > can remove this flag. If nothing else you're using a global flag to describe a platform device specific augmentation. We've recently added capabilities on the device info return that would be more appropriate for this, but fundamentally I don't understand why the irq info isn't sufficient. Thanks, Alex
Hi Alex, On Fri, Nov 6, 2020 at 8:42 AM Alex Williamson <alex.williamson@redhat.com> wrote: > > On Fri, 6 Nov 2020 08:24:26 +0530 > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > Hi Alex, > > > > On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson > > <alex.williamson@redhat.com> wrote: > > > > > > On Thu, 5 Nov 2020 11:32:55 +0530 > > > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > > index 2f313a238a8f..aab051e8338d 100644 > > > > --- a/include/uapi/linux/vfio.h > > > > +++ b/include/uapi/linux/vfio.h > > > > @@ -203,6 +203,7 @@ struct vfio_device_info { > > > > #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ > > > > #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ > > > > #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ > > > > +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ > > > > __u32 num_regions; /* Max region index + 1 */ > > > > __u32 num_irqs; /* Max IRQ index + 1 */ > > > > __u32 cap_offset; /* Offset within info struct of first cap */ > > > > > > This doesn't make any sense to me, MSIs are just edge triggered > > > interrupts to userspace, so why isn't this fully described via > > > VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, > > > this seems incomplete, which indexes are MSI (IRQ_INFO can describe > > > that)? We also already support MSI with vfio-pci, so a global flag for > > > the device advertising this still seems wrong. Thanks, > > > > > > Alex > > > > > Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) > > cannot be described using indexes. > > That would be news for vfio-pci which has been describing MSIs with > sub-indexes within indexes since vfio started. > > > In the patch set there is no difference between MSI and normal > > interrupt for VFIO_DEVICE_GET_IRQ_INFO. > > Then what exactly is a global device flag indicating? Does it indicate > all IRQs are MSI? No, it's not indicating that all are MSI. The rationale behind adding the flag to tell user-space that platform device supports MSI as well. As you mentioned recently added capabilities can help on this, I`ll go through that. > > > The patch set adds MSI(s), say as an extension, to the normal > > interrupts and handled accordingly. > > So we have both "normal" IRQs and MSIs? How does the user know which > indexes are which? With this patch set, I think this is missing and user space cannot know that particular index is MSI interrupt. For platform devices there is no such mechanism, like index and sub-indexes to differentiate between legacy, MSI or MSIX as it’s there in PCI. I believe for a particular IRQ index if the flag VFIO_IRQ_INFO_NORESIZE is used then user space can know which IRQ index has MSI(s). Does it make sense? Suggestions on this would be helpful. Thanks, Vikas > > > Do you see this is a violation? If > > Seems pretty unclear and dubious use of a global device flag. > > > yes, then we`ll think of other possible ways to support MSI for the > > platform devices. > > Macro VFIO_DEVICE_FLAGS_MSI can be changed to any other name if it > > collides with an already supported vfio-pci or if not necessary, we > > can remove this flag. > > If nothing else you're using a global flag to describe a platform > device specific augmentation. We've recently added capabilities on the > device info return that would be more appropriate for this, but > fundamentally I don't understand why the irq info isn't sufficient. > Thanks, > > Alex >
Hi Vikas, On 11/6/20 3:54 AM, Vikas Gupta wrote: > Hi Alex, > > On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson > <alex.williamson@redhat.com> wrote: >> >> On Thu, 5 Nov 2020 11:32:55 +0530 >> Vikas Gupta <vikas.gupta@broadcom.com> wrote: >> >>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >>> index 2f313a238a8f..aab051e8338d 100644 >>> --- a/include/uapi/linux/vfio.h >>> +++ b/include/uapi/linux/vfio.h >>> @@ -203,6 +203,7 @@ struct vfio_device_info { >>> #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ >>> #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ >>> #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ >>> +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ >>> __u32 num_regions; /* Max region index + 1 */ >>> __u32 num_irqs; /* Max IRQ index + 1 */ >>> __u32 cap_offset; /* Offset within info struct of first cap */ >> >> This doesn't make any sense to me, MSIs are just edge triggered >> interrupts to userspace, so why isn't this fully described via >> VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, >> this seems incomplete, which indexes are MSI (IRQ_INFO can describe >> that)? We also already support MSI with vfio-pci, so a global flag for >> the device advertising this still seems wrong. Thanks, >> >> Alex >> > Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) > cannot be described using indexes. > In the patch set there is no difference between MSI and normal > interrupt for VFIO_DEVICE_GET_IRQ_INFO. in vfio_platform_irq_init() we first iterate on normal interrupts using get_irq(). Can't we add an MSI index at the end of this list with vdev->irqs[i].count > 1 and set vdev->num_irqs accordingly? Thanks Eric > The patch set adds MSI(s), say as an extension, to the normal > interrupts and handled accordingly. Do you see this is a violation? If > yes, then we`ll think of other possible ways to support MSI for the > platform devices. > Macro VFIO_DEVICE_FLAGS_MSI can be changed to any other name if it > collides with an already supported vfio-pci or if not necessary, we > can remove this flag. > > Thanks, > Vikas >
Hi Vikas, On 11/9/20 7:41 AM, Vikas Gupta wrote: > Hi Alex, > > On Fri, Nov 6, 2020 at 8:42 AM Alex Williamson > <alex.williamson@redhat.com> wrote: >> >> On Fri, 6 Nov 2020 08:24:26 +0530 >> Vikas Gupta <vikas.gupta@broadcom.com> wrote: >> >>> Hi Alex, >>> >>> On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson >>> <alex.williamson@redhat.com> wrote: >>>> >>>> On Thu, 5 Nov 2020 11:32:55 +0530 >>>> Vikas Gupta <vikas.gupta@broadcom.com> wrote: >>>> >>>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >>>>> index 2f313a238a8f..aab051e8338d 100644 >>>>> --- a/include/uapi/linux/vfio.h >>>>> +++ b/include/uapi/linux/vfio.h >>>>> @@ -203,6 +203,7 @@ struct vfio_device_info { >>>>> #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ >>>>> #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ >>>>> #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ >>>>> +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ >>>>> __u32 num_regions; /* Max region index + 1 */ >>>>> __u32 num_irqs; /* Max IRQ index + 1 */ >>>>> __u32 cap_offset; /* Offset within info struct of first cap */ >>>> >>>> This doesn't make any sense to me, MSIs are just edge triggered >>>> interrupts to userspace, so why isn't this fully described via >>>> VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, >>>> this seems incomplete, which indexes are MSI (IRQ_INFO can describe >>>> that)? We also already support MSI with vfio-pci, so a global flag for >>>> the device advertising this still seems wrong. Thanks, >>>> >>>> Alex >>>> >>> Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) >>> cannot be described using indexes. >> >> That would be news for vfio-pci which has been describing MSIs with >> sub-indexes within indexes since vfio started. >> >>> In the patch set there is no difference between MSI and normal >>> interrupt for VFIO_DEVICE_GET_IRQ_INFO. >> >> Then what exactly is a global device flag indicating? Does it indicate >> all IRQs are MSI? > > No, it's not indicating that all are MSI. > The rationale behind adding the flag to tell user-space that platform > device supports MSI as well. As you mentioned recently added > capabilities can help on this, I`ll go through that. > >> >>> The patch set adds MSI(s), say as an extension, to the normal >>> interrupts and handled accordingly. >> >> So we have both "normal" IRQs and MSIs? How does the user know which >> indexes are which? > > With this patch set, I think this is missing and user space cannot > know that particular index is MSI interrupt. > For platform devices there is no such mechanism, like index and > sub-indexes to differentiate between legacy, MSI or MSIX as it’s there > in PCI. Wht can't you use the count field (as per vfio_pci_get_irq_count())? > I believe for a particular IRQ index if the flag > VFIO_IRQ_INFO_NORESIZE is used then user space can know which IRQ > index has MSI(s). Does it make sense? I don't think it is the same semantics. Thanks Eric > Suggestions on this would be helpful. > > Thanks, > Vikas >> >>> Do you see this is a violation? If >> >> Seems pretty unclear and dubious use of a global device flag. >> >>> yes, then we`ll think of other possible ways to support MSI for the >>> platform devices. >>> Macro VFIO_DEVICE_FLAGS_MSI can be changed to any other name if it >>> collides with an already supported vfio-pci or if not necessary, we >>> can remove this flag. >> >> If nothing else you're using a global flag to describe a platform >> device specific augmentation. We've recently added capabilities on the >> device info return that would be more appropriate for this, but >> fundamentally I don't understand why the irq info isn't sufficient. >> Thanks, >> >> Alex >>
On Mon, 9 Nov 2020 12:11:15 +0530 Vikas Gupta <vikas.gupta@broadcom.com> wrote: > Hi Alex, > > On Fri, Nov 6, 2020 at 8:42 AM Alex Williamson > <alex.williamson@redhat.com> wrote: > > > > On Fri, 6 Nov 2020 08:24:26 +0530 > > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > > > Hi Alex, > > > > > > On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson > > > <alex.williamson@redhat.com> wrote: > > > > > > > > On Thu, 5 Nov 2020 11:32:55 +0530 > > > > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > > > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > > > index 2f313a238a8f..aab051e8338d 100644 > > > > > --- a/include/uapi/linux/vfio.h > > > > > +++ b/include/uapi/linux/vfio.h > > > > > @@ -203,6 +203,7 @@ struct vfio_device_info { > > > > > #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ > > > > > #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ > > > > > #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ > > > > > +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ > > > > > __u32 num_regions; /* Max region index + 1 */ > > > > > __u32 num_irqs; /* Max IRQ index + 1 */ > > > > > __u32 cap_offset; /* Offset within info struct of first cap */ > > > > > > > > This doesn't make any sense to me, MSIs are just edge triggered > > > > interrupts to userspace, so why isn't this fully described via > > > > VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, > > > > this seems incomplete, which indexes are MSI (IRQ_INFO can describe > > > > that)? We also already support MSI with vfio-pci, so a global flag for > > > > the device advertising this still seems wrong. Thanks, > > > > > > > > Alex > > > > > > > Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) > > > cannot be described using indexes. > > > > That would be news for vfio-pci which has been describing MSIs with > > sub-indexes within indexes since vfio started. > > > > > In the patch set there is no difference between MSI and normal > > > interrupt for VFIO_DEVICE_GET_IRQ_INFO. > > > > Then what exactly is a global device flag indicating? Does it indicate > > all IRQs are MSI? > > No, it's not indicating that all are MSI. > The rationale behind adding the flag to tell user-space that platform > device supports MSI as well. As you mentioned recently added > capabilities can help on this, I`ll go through that. It still seems questionable to me to use a device info capability to describe an interrupt index specific feature. The scope seems wrong. Why does userspace need to know that this IRQ is MSI rather than indicating it's simply an edge triggered interrupt? That can be done using only vfio_irq_info.flags. > > > The patch set adds MSI(s), say as an extension, to the normal > > > interrupts and handled accordingly. > > > > So we have both "normal" IRQs and MSIs? How does the user know which > > indexes are which? > > With this patch set, I think this is missing and user space cannot > know that particular index is MSI interrupt. > For platform devices there is no such mechanism, like index and > sub-indexes to differentiate between legacy, MSI or MSIX as it’s there > in PCI. Indexes and sub-indexes are a grouping mechanism of vfio to describe related interrupts. That terminology doesn't exist on PCI either, it's meant to be used generically. It's left to the vfio bus driver how userspace associates a given index to a device feature. > I believe for a particular IRQ index if the flag > VFIO_IRQ_INFO_NORESIZE is used then user space can know which IRQ > index has MSI(s). Does it make sense? No, no-resize is an implementation detail, not an indication of the interrupt mechanism. It's still not clear to me why it's important to expose to userspace that a given interrupt is MSI versus simply exposing it as an edge interrupt (ie. automasked = false). If it is necessary, the most direct approach might be to expose a capability extension in the vfio_irq_info structure to describe it. Even then though, I don't think simply exposing a index as MSI is very meaningful. What is userspace intended to do differently based on this information? Thanks, Alex
Hi Eric, On Mon, Nov 9, 2020 at 8:35 PM Auger Eric <eric.auger@redhat.com> wrote: > > Hi Vikas, > > On 11/6/20 3:54 AM, Vikas Gupta wrote: > > Hi Alex, > > > > On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson > > <alex.williamson@redhat.com> wrote: > >> > >> On Thu, 5 Nov 2020 11:32:55 +0530 > >> Vikas Gupta <vikas.gupta@broadcom.com> wrote: > >> > >>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > >>> index 2f313a238a8f..aab051e8338d 100644 > >>> --- a/include/uapi/linux/vfio.h > >>> +++ b/include/uapi/linux/vfio.h > >>> @@ -203,6 +203,7 @@ struct vfio_device_info { > >>> #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ > >>> #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ > >>> #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ > >>> +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ > >>> __u32 num_regions; /* Max region index + 1 */ > >>> __u32 num_irqs; /* Max IRQ index + 1 */ > >>> __u32 cap_offset; /* Offset within info struct of first cap */ > >> > >> This doesn't make any sense to me, MSIs are just edge triggered > >> interrupts to userspace, so why isn't this fully described via > >> VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, > >> this seems incomplete, which indexes are MSI (IRQ_INFO can describe > >> that)? We also already support MSI with vfio-pci, so a global flag for > >> the device advertising this still seems wrong. Thanks, > >> > >> Alex > >> > > Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) > > cannot be described using indexes. > > In the patch set there is no difference between MSI and normal > > interrupt for VFIO_DEVICE_GET_IRQ_INFO. > in vfio_platform_irq_init() we first iterate on normal interrupts using > get_irq(). Can't we add an MSI index at the end of this list with > vdev->irqs[i].count > 1 and set vdev->num_irqs accordingly? Yes, I think MSI can be added to the end of list with setting vdev->irqs[i].count > 1. I`ll consider changing in the next patch set. Thanks, Vikas > > Thanks > > Eric > > The patch set adds MSI(s), say as an extension, to the normal > > interrupts and handled accordingly. Do you see this is a violation? If > > yes, then we`ll think of other possible ways to support MSI for the > > platform devices. > > Macro VFIO_DEVICE_FLAGS_MSI can be changed to any other name if it > > collides with an already supported vfio-pci or if not necessary, we > > can remove this flag. > > > > Thanks, > > Vikas > > >
Hi Alex, On Mon, Nov 9, 2020 at 8:58 PM Alex Williamson <alex.williamson@redhat.com> wrote: > > On Mon, 9 Nov 2020 12:11:15 +0530 > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > Hi Alex, > > > > On Fri, Nov 6, 2020 at 8:42 AM Alex Williamson > > <alex.williamson@redhat.com> wrote: > > > > > > On Fri, 6 Nov 2020 08:24:26 +0530 > > > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > > > > > Hi Alex, > > > > > > > > On Thu, Nov 5, 2020 at 12:38 PM Alex Williamson > > > > <alex.williamson@redhat.com> wrote: > > > > > > > > > > On Thu, 5 Nov 2020 11:32:55 +0530 > > > > > Vikas Gupta <vikas.gupta@broadcom.com> wrote: > > > > > > > > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > > > > index 2f313a238a8f..aab051e8338d 100644 > > > > > > --- a/include/uapi/linux/vfio.h > > > > > > +++ b/include/uapi/linux/vfio.h > > > > > > @@ -203,6 +203,7 @@ struct vfio_device_info { > > > > > > #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ > > > > > > #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ > > > > > > #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ > > > > > > +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ > > > > > > __u32 num_regions; /* Max region index + 1 */ > > > > > > __u32 num_irqs; /* Max IRQ index + 1 */ > > > > > > __u32 cap_offset; /* Offset within info struct of first cap */ > > > > > > > > > > This doesn't make any sense to me, MSIs are just edge triggered > > > > > interrupts to userspace, so why isn't this fully described via > > > > > VFIO_DEVICE_GET_IRQ_INFO? If we do need something new to describe it, > > > > > this seems incomplete, which indexes are MSI (IRQ_INFO can describe > > > > > that)? We also already support MSI with vfio-pci, so a global flag for > > > > > the device advertising this still seems wrong. Thanks, > > > > > > > > > > Alex > > > > > > > > > Since VFIO platform uses indexes for IRQ numbers so I think MSI(s) > > > > cannot be described using indexes. > > > > > > That would be news for vfio-pci which has been describing MSIs with > > > sub-indexes within indexes since vfio started. > > > > > > > In the patch set there is no difference between MSI and normal > > > > interrupt for VFIO_DEVICE_GET_IRQ_INFO. > > > > > > Then what exactly is a global device flag indicating? Does it indicate > > > all IRQs are MSI? > > > > No, it's not indicating that all are MSI. > > The rationale behind adding the flag to tell user-space that platform > > device supports MSI as well. As you mentioned recently added > > capabilities can help on this, I`ll go through that. > > > It still seems questionable to me to use a device info capability to > describe an interrupt index specific feature. The scope seems wrong. > Why does userspace need to know that this IRQ is MSI rather than > indicating it's simply an edge triggered interrupt? That can be done > using only vfio_irq_info.flags. Ok. In the next patch set I`ll remove the device flag (VFIO_DEVICE_FLAGS_MSI) as vfio_irq_info.flags should have enough information for edge triggered interrupt. > > > > > > The patch set adds MSI(s), say as an extension, to the normal > > > > interrupts and handled accordingly. > > > > > > So we have both "normal" IRQs and MSIs? How does the user know which > > > indexes are which? > > > > With this patch set, I think this is missing and user space cannot > > know that particular index is MSI interrupt. > > For platform devices there is no such mechanism, like index and > > sub-indexes to differentiate between legacy, MSI or MSIX as it’s there > > in PCI. > > Indexes and sub-indexes are a grouping mechanism of vfio to describe > related interrupts. That terminology doesn't exist on PCI either, it's > meant to be used generically. It's left to the vfio bus driver how > userspace associates a given index to a device feature. > > > I believe for a particular IRQ index if the flag > > VFIO_IRQ_INFO_NORESIZE is used then user space can know which IRQ > > index has MSI(s). Does it make sense? > > > No, no-resize is an implementation detail, not an indication of the > interrupt mechanism. It's still not clear to me why it's important to > expose to userspace that a given interrupt is MSI versus simply > exposing it as an edge interrupt (ie. automasked = false). If it is > necessary, the most direct approach might be to expose a capability > extension in the vfio_irq_info structure to describe it. Even then > though, I don't think simply exposing a index as MSI is very > meaningful. What is userspace intended to do differently based on this > information? Thanks, The current patch set is not setting VFIO_IRQ_INFO_AUTOMASKED (automasked=false) for MSIs so I believe this much is information enough for user space to know that this is an edge triggered interrupt. I agree that exposing an index as MSI is not meaningful as user space has nothing special to do with this information. > > Alex >
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index c0771a9567fb..c713f4e4c552 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -26,6 +26,10 @@ #define VFIO_PLATFORM_IS_ACPI(vdev) ((vdev)->acpihid != NULL) static LIST_HEAD(reset_list); + +/* devices having MSI support */ +static LIST_HEAD(msi_list); + static DEFINE_MUTEX(driver_lock); static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat, @@ -47,6 +51,26 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat, return reset_fn; } +static bool vfio_platform_lookup_msi(struct vfio_platform_device *vdev) +{ + bool has_msi = false; + struct vfio_platform_msi_node *iter; + + mutex_lock(&driver_lock); + list_for_each_entry(iter, &msi_list, link) { + if (!strcmp(iter->compat, vdev->compat) && + try_module_get(iter->owner)) { + vdev->msi_module = iter->owner; + vdev->of_get_msi = iter->of_get_msi; + vdev->of_msi_write = iter->of_msi_write; + has_msi = true; + break; + } + } + mutex_unlock(&driver_lock); + return has_msi; +} + static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev, struct device *dev) { @@ -110,6 +134,11 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev) return vdev->of_reset ? true : false; } +static bool vfio_platform_has_msi(struct vfio_platform_device *vdev) +{ + return vdev->of_get_msi ? true : false; +} + static int vfio_platform_get_reset(struct vfio_platform_device *vdev) { if (VFIO_PLATFORM_IS_ACPI(vdev)) @@ -126,6 +155,19 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev) return vdev->of_reset ? 0 : -ENOENT; } +static int vfio_platform_get_msi(struct vfio_platform_device *vdev) +{ + bool has_msi; + + has_msi = vfio_platform_lookup_msi(vdev); + if (!has_msi) { + request_module("vfio-msi:%s", vdev->compat); + has_msi = vfio_platform_lookup_msi(vdev); + } + + return has_msi ? 0 : -ENOENT; +} + static void vfio_platform_put_reset(struct vfio_platform_device *vdev) { if (VFIO_PLATFORM_IS_ACPI(vdev)) @@ -135,6 +177,12 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev) module_put(vdev->reset_module); } +static void vfio_platform_put_msi(struct vfio_platform_device *vdev) +{ + if (vdev->of_get_msi) + module_put(vdev->msi_module); +} + static int vfio_platform_regions_init(struct vfio_platform_device *vdev) { int cnt = 0, i; @@ -313,6 +361,10 @@ static long vfio_platform_ioctl(void *device_data, if (vfio_platform_has_reset(vdev)) vdev->flags |= VFIO_DEVICE_FLAGS_RESET; + + if (vfio_platform_has_msi(vdev)) + vdev->flags |= VFIO_DEVICE_FLAGS_MSI; + info.flags = vdev->flags; info.num_regions = vdev->num_regions; info.num_irqs = vdev->num_irqs; @@ -679,11 +731,15 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, return ret; } + ret = vfio_platform_get_msi(vdev); + if (ret) + dev_info(vdev->device, "msi not supported\n"); + group = vfio_iommu_group_get(dev); if (!group) { dev_err(dev, "No IOMMU group for device %s\n", vdev->name); ret = -EINVAL; - goto put_reset; + goto put_msi; } ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); @@ -697,6 +753,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, put_iommu: vfio_iommu_group_put(group, dev); +put_msi: + vfio_platform_put_msi(vdev); put_reset: vfio_platform_put_reset(vdev); return ret; @@ -745,6 +803,30 @@ void vfio_platform_unregister_reset(const char *compat, } EXPORT_SYMBOL_GPL(vfio_platform_unregister_reset); +void __vfio_platform_register_msi(struct vfio_platform_msi_node *node) +{ + mutex_lock(&driver_lock); + list_add(&node->link, &msi_list); + mutex_unlock(&driver_lock); +} +EXPORT_SYMBOL_GPL(__vfio_platform_register_msi); + +void vfio_platform_unregister_msi(const char *compat) +{ + struct vfio_platform_msi_node *iter, *temp; + + mutex_lock(&driver_lock); + list_for_each_entry_safe(iter, temp, &msi_list, link) { + if (!strcmp(iter->compat, compat)) { + list_del(&iter->link); + break; + } + } + + mutex_unlock(&driver_lock); +} +EXPORT_SYMBOL_GPL(vfio_platform_unregister_msi); + MODULE_VERSION(DRIVER_VERSION); MODULE_LICENSE("GPL v2"); MODULE_AUTHOR(DRIVER_AUTHOR); diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c index c5b09ec0a3c9..14b544c3aa42 100644 --- a/drivers/vfio/platform/vfio_platform_irq.c +++ b/drivers/vfio/platform/vfio_platform_irq.c @@ -8,10 +8,12 @@ #include <linux/eventfd.h> #include <linux/interrupt.h> +#include <linux/eventfd.h> #include <linux/slab.h> #include <linux/types.h> #include <linux/vfio.h> #include <linux/irq.h> +#include <linux/msi.h> #include "vfio_platform_private.h" @@ -253,6 +255,176 @@ static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev, return 0; } +/* MSI/MSIX */ +static irqreturn_t vfio_msihandler(int irq, void *arg) +{ + struct eventfd_ctx *trigger = arg; + + eventfd_signal(trigger, 1); + return IRQ_HANDLED; +} + +static void msi_write(struct msi_desc *desc, struct msi_msg *msg) +{ + struct device *dev = msi_desc_to_dev(desc); + struct vfio_device *device = dev_get_drvdata(dev); + struct vfio_platform_device *vdev = (struct vfio_platform_device *) + vfio_device_data(device); + + vdev->of_msi_write(vdev, desc, msg); +} + +static int vfio_msi_enable(struct vfio_platform_device *vdev, int nvec) +{ + int ret; + int msi_idx = 0; + struct msi_desc *desc; + struct device *dev = vdev->device; + u32 msi_off = vdev->num_irqs - vdev->num_msis; + + /* Allocate platform MSIs */ + ret = platform_msi_domain_alloc_irqs(dev, nvec, msi_write); + if (ret < 0) + return ret; + + for_each_msi_entry(desc, dev) { + vdev->irqs[msi_off + msi_idx].hwirq = desc->irq; + msi_idx++; + } + + vdev->num_msis = nvec; + vdev->config_msi = 1; + + return 0; +} + +static int vfio_msi_set_vector_signal(struct vfio_platform_device *vdev, + int vector, int fd) +{ + struct eventfd_ctx *trigger; + int irq, ret; + u32 msi_off = vdev->num_irqs - vdev->num_msis; + + if (vector < 0 || vector >= vdev->num_msis) + return -EINVAL; + + irq = vdev->irqs[msi_off + vector].hwirq; + + if (vdev->irqs[vector].trigger) { + free_irq(irq, vdev->irqs[vector].trigger); + kfree(vdev->irqs[vector].name); + eventfd_ctx_put(vdev->irqs[vector].trigger); + vdev->irqs[vector].trigger = NULL; + } + + if (fd < 0) + return 0; + + vdev->irqs[vector].name = kasprintf(GFP_KERNEL, + "vfio-msi[%d]", vector); + if (!vdev->irqs[vector].name) + return -ENOMEM; + + trigger = eventfd_ctx_fdget(fd); + if (IS_ERR(trigger)) { + kfree(vdev->irqs[vector].name); + return PTR_ERR(trigger); + } + + ret = request_irq(irq, vfio_msihandler, 0, + vdev->irqs[vector].name, trigger); + if (ret) { + kfree(vdev->irqs[vector].name); + eventfd_ctx_put(trigger); + return ret; + } + + vdev->irqs[vector].trigger = trigger; + + return 0; +} + +static int vfio_msi_set_block(struct vfio_platform_device *vdev, unsigned int start, + unsigned int count, int32_t *fds) +{ + int i, j, ret = 0; + + if (start >= vdev->num_msis || start + count > vdev->num_msis) + return -EINVAL; + + for (i = 0, j = start; i < count && !ret; i++, j++) { + int fd = fds ? fds[i] : -1; + + ret = vfio_msi_set_vector_signal(vdev, j, fd); + } + + if (ret) { + for (--j; j >= (int)start; j--) + vfio_msi_set_vector_signal(vdev, j, -1); + } + + return ret; +} + +static void vfio_msi_disable(struct vfio_platform_device *vdev) +{ + struct device *dev = vdev->device; + + vfio_msi_set_block(vdev, 0, vdev->num_msis, NULL); + platform_msi_domain_free_irqs(dev); + + vdev->config_msi = 0; + vdev->num_msis = 0; +} + +static int vfio_set_msi_trigger(struct vfio_platform_device *vdev, + unsigned int index, unsigned int start, + unsigned int count, uint32_t flags, void *data) +{ + int i; + + if (start + count > vdev->num_msis) + return -EINVAL; + + if (!count && (flags & VFIO_IRQ_SET_DATA_NONE)) { + vfio_msi_disable(vdev); + return 0; + } + + if (flags & VFIO_IRQ_SET_DATA_EVENTFD) { + s32 *fds = data; + int ret; + + if (vdev->config_msi) + return vfio_msi_set_block(vdev, start, count, + fds); + ret = vfio_msi_enable(vdev, start + count); + if (ret) + return ret; + + ret = vfio_msi_set_block(vdev, start, count, fds); + if (ret) + vfio_msi_disable(vdev); + + return ret; + } + + for (i = start; i < start + count; i++) { + if (!vdev->irqs[i].trigger) + continue; + if (flags & VFIO_IRQ_SET_DATA_NONE) { + eventfd_signal(vdev->irqs[i].trigger, 1); + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) { + u8 *bools = data; + + if (bools[i - start]) + eventfd_signal(vdev->irqs[i].trigger, 1); + } + } + + return 0; +} + int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev, uint32_t flags, unsigned index, unsigned start, unsigned count, void *data) @@ -261,16 +433,29 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev, unsigned start, unsigned count, uint32_t flags, void *data) = NULL; - switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) { - case VFIO_IRQ_SET_ACTION_MASK: - func = vfio_platform_set_irq_mask; - break; - case VFIO_IRQ_SET_ACTION_UNMASK: - func = vfio_platform_set_irq_unmask; - break; - case VFIO_IRQ_SET_ACTION_TRIGGER: - func = vfio_platform_set_irq_trigger; - break; + struct vfio_platform_irq *irq = &vdev->irqs[index]; + + if (!irq->is_msi) { + switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) { + case VFIO_IRQ_SET_ACTION_MASK: + func = vfio_platform_set_irq_mask; + break; + case VFIO_IRQ_SET_ACTION_UNMASK: + func = vfio_platform_set_irq_unmask; + break; + case VFIO_IRQ_SET_ACTION_TRIGGER: + func = vfio_platform_set_irq_trigger; + break; + } + } else { + switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) { + case VFIO_IRQ_SET_ACTION_MASK: + case VFIO_IRQ_SET_ACTION_UNMASK: + break; + case VFIO_IRQ_SET_ACTION_TRIGGER: + func = vfio_set_msi_trigger; + break; + } } if (!func) @@ -282,11 +467,17 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev, int vfio_platform_irq_init(struct vfio_platform_device *vdev) { int cnt = 0, i; + int msi_cnt = 0; while (vdev->get_irq(vdev, cnt) >= 0) cnt++; - vdev->irqs = kcalloc(cnt, sizeof(struct vfio_platform_irq), GFP_KERNEL); + if (vdev->of_get_msi) + msi_cnt = vdev->of_get_msi(vdev); + vdev->num_msis = msi_cnt; + + vdev->irqs = kcalloc(cnt + msi_cnt, sizeof(struct vfio_platform_irq), + GFP_KERNEL); if (!vdev->irqs) return -ENOMEM; @@ -309,7 +500,18 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev) vdev->irqs[i].masked = false; } - vdev->num_irqs = cnt; + for (i = cnt; i < msi_cnt + cnt; i++) { + spin_lock_init(&vdev->irqs[i].lock); + + vdev->irqs[i].flags = VFIO_IRQ_INFO_EVENTFD; + + vdev->irqs[i].count = 1; + vdev->irqs[i].hwirq = 0; + vdev->irqs[i].masked = false; + vdev->irqs[i].is_msi = true; + } + + vdev->num_irqs = cnt + msi_cnt; return 0; err: @@ -320,10 +522,17 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev) void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev) { int i; + int non_msi_cnt = vdev->num_irqs - vdev->num_msis; - for (i = 0; i < vdev->num_irqs; i++) + for (i = 0; i < non_msi_cnt; i++) vfio_set_trigger(vdev, i, -1, NULL); + if (vdev->num_msis) { + vfio_set_msi_trigger(vdev, 0, 0, 0, + VFIO_IRQ_SET_DATA_NONE, NULL); + vdev->num_msis = 0; + } + vdev->num_irqs = 0; kfree(vdev->irqs); } diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h index 289089910643..2aea445e1071 100644 --- a/drivers/vfio/platform/vfio_platform_private.h +++ b/drivers/vfio/platform/vfio_platform_private.h @@ -9,6 +9,7 @@ #include <linux/types.h> #include <linux/interrupt.h> +#include <linux/msi.h> #define VFIO_PLATFORM_OFFSET_SHIFT 40 #define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1) @@ -26,6 +27,7 @@ struct vfio_platform_irq { char *name; struct eventfd_ctx *trigger; bool masked; + bool is_msi; spinlock_t lock; struct virqfd *unmask; struct virqfd *mask; @@ -46,13 +48,16 @@ struct vfio_platform_device { u32 num_regions; struct vfio_platform_irq *irqs; u32 num_irqs; + u32 num_msis; int refcnt; struct mutex igate; struct module *parent_module; const char *compat; const char *acpihid; struct module *reset_module; + struct module *msi_module; struct device *device; + int config_msi; /* * These fields should be filled by the bus specific binder @@ -65,11 +70,19 @@ struct vfio_platform_device { (*get_resource)(struct vfio_platform_device *vdev, int i); int (*get_irq)(struct vfio_platform_device *vdev, int i); int (*of_reset)(struct vfio_platform_device *vdev); + u32 (*of_get_msi)(struct vfio_platform_device *vdev); + int (*of_msi_write)(struct vfio_platform_device *vdev, + struct msi_desc *desc, + struct msi_msg *msg); bool reset_required; }; typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev); +typedef u32 (*vfio_platform_get_msi_fn_t)(struct vfio_platform_device *vdev); +typedef int (*vfio_platform_msi_write_fn_t)(struct vfio_platform_device *vdev, + struct msi_desc *desc, + struct msi_msg *msg); struct vfio_platform_reset_node { struct list_head link; @@ -78,6 +91,14 @@ struct vfio_platform_reset_node { vfio_platform_reset_fn_t of_reset; }; +struct vfio_platform_msi_node { + struct list_head link; + char *compat; + struct module *owner; + vfio_platform_get_msi_fn_t of_get_msi; + vfio_platform_msi_write_fn_t of_msi_write; +}; + extern int vfio_platform_probe_common(struct vfio_platform_device *vdev, struct device *dev); extern struct vfio_platform_device *vfio_platform_remove_common @@ -94,6 +115,8 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev, extern void __vfio_platform_register_reset(struct vfio_platform_reset_node *n); extern void vfio_platform_unregister_reset(const char *compat, vfio_platform_reset_fn_t fn); +void __vfio_platform_register_msi(struct vfio_platform_msi_node *n); +void vfio_platform_unregister_msi(const char *compat); #define vfio_platform_register_reset(__compat, __reset) \ static struct vfio_platform_reset_node __reset ## _node = { \ .owner = THIS_MODULE, \ diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 2f313a238a8f..aab051e8338d 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -203,6 +203,7 @@ struct vfio_device_info { #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */ #define VFIO_DEVICE_FLAGS_FSL_MC (1 << 6) /* vfio-fsl-mc device */ #define VFIO_DEVICE_FLAGS_CAPS (1 << 7) /* Info supports caps */ +#define VFIO_DEVICE_FLAGS_MSI (1 << 8) /* Device supports msi */ __u32 num_regions; /* Max region index + 1 */ __u32 num_irqs; /* Max IRQ index + 1 */ __u32 cap_offset; /* Offset within info struct of first cap */
MSI support for platform devices. Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com> --- drivers/vfio/platform/vfio_platform_common.c | 84 ++++++- drivers/vfio/platform/vfio_platform_irq.c | 235 +++++++++++++++++- drivers/vfio/platform/vfio_platform_private.h | 23 ++ include/uapi/linux/vfio.h | 1 + 4 files changed, 329 insertions(+), 14 deletions(-)