pci_msix_alloc_irq_at() enables an individual MSI-X interrupt to be
allocated after MSI-X enabling.
Use dynamic MSI-X (if supported by the device) to allocate an interrupt
after MSI-X is enabled. An MSI-X interrupt is dynamically allocated at
the time a valid eventfd is assigned. This is different behavior from
a range provided during MSI-X enabling where interrupts are allocated
for the entire range whether a valid eventfd is provided for each
interrupt or not.
The PCI-MSIX API requires that some number of irqs are allocated for
an initial set of vectors when enabling MSI-X on the device. When
dynamic MSIX allocation is not supported, the vector table, and thus
the allocated irq set can only be resized by disabling and re-enabling
MSI-X with a different range. In that case the irq allocation is
essentially a cache for configuring vectors within the previously
allocated vector range. When dynamic MSI-X allocation is supported,
the API still requires some initial set of irqs to be allocated, but
also supports allocating and freeing specific irq vectors both
within and beyond the initially allocated range.
For consistency between modes, as well as to reduce latency and improve
reliability of allocations, and also simplicity, this implementation
only releases irqs via pci_free_irq_vectors() when either the interrupt
mode changes or the device is released.
Signed-off-by: Reinette Chatre <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
---
Changes since V3:
- Remove vfio_msi_free_irq(). (Alex)
- Rework changelog. (Alex)
Changes since V2:
- Move vfio_irq_ctx_free() to earlier in series to support
earlier usage. (Alex)
- Use consistent terms in changelog: MSI-x changed to MSI-X.
- Make dynamic interrupt context creation generic across all
MSI/MSI-X interrupts. This resulted in code moving to earlier
in series as part of xarray introduction patch. (Alex)
- Remove the local allow_dyn_alloc and direct calling of
pci_msix_can_alloc_dyn(), use the new vdev->has_dyn_msix
introduced earlier instead. (Alex)
- Stop tracking new allocations (remove "new_ctx"). (Alex)
- Introduce new wrapper that returns Linux interrupt number or
dynamically allocate a new interrupt. Wrapper can be used for
all interrupt cases. (Alex)
- Only free dynamic MSI-X interrupts on MSI-X teardown. (Alex)
Changes since RFC V1:
- Add pointer to interrupt context as function parameter to
vfio_irq_ctx_free(). (Alex)
- Initialize new_ctx to false. (Dan Carpenter)
- Only support dynamic allocation if device supports it. (Alex)
drivers/vfio/pci/vfio_pci_intrs.c | 47 +++++++++++++++++++++++++++----
1 file changed, 41 insertions(+), 6 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index bdda7f46c2be..8340135b09fa 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -372,27 +372,56 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi
return 0;
}
+/*
+ * Return Linux IRQ number of an MSI or MSI-X device interrupt vector.
+ * If a Linux IRQ number is not available then a new interrupt will be
+ * allocated if dynamic MSI-X is supported.
+ */
+static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev,
+ unsigned int vector, bool msix)
+{
+ struct pci_dev *pdev = vdev->pdev;
+ struct msi_map map;
+ int irq;
+ u16 cmd;
+
+ irq = pci_irq_vector(pdev, vector);
+ if (irq > 0 || !msix || !vdev->has_dyn_msix)
+ return irq;
+
+ cmd = vfio_pci_memory_lock_and_enable(vdev);
+ map = pci_msix_alloc_irq_at(pdev, vector, NULL);
+ vfio_pci_memory_unlock_and_restore(vdev, cmd);
+
+ return map.index < 0 ? map.index : map.virq;
+}
+
+/*
+ * Where is vfio_msi_free_irq() ?
+ *
+ * Allocated interrupts are maintained, essentially forming a cache that
+ * subsequent allocations can draw from. Interrupts are freed using
+ * pci_free_irq_vectors() when MSI/MSI-X is disabled.
+ */
+
static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
unsigned int vector, int fd, bool msix)
{
struct pci_dev *pdev = vdev->pdev;
struct vfio_pci_irq_ctx *ctx;
struct eventfd_ctx *trigger;
- int irq, ret;
+ int irq = -EINVAL, ret;
u16 cmd;
- irq = pci_irq_vector(pdev, vector);
- if (irq < 0)
- return -EINVAL;
-
ctx = vfio_irq_ctx_get(vdev, vector);
if (ctx) {
irq_bypass_unregister_producer(&ctx->producer);
-
+ irq = pci_irq_vector(pdev, vector);
cmd = vfio_pci_memory_lock_and_enable(vdev);
free_irq(irq, ctx->trigger);
vfio_pci_memory_unlock_and_restore(vdev, cmd);
+ /* Interrupt stays allocated, will be freed at MSI-X disable. */
kfree(ctx->name);
eventfd_ctx_put(ctx->trigger);
vfio_irq_ctx_free(vdev, ctx, vector);
@@ -401,6 +430,12 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
if (fd < 0)
return 0;
+ if (irq == -EINVAL) {
+ irq = vfio_msi_alloc_irq(vdev, vector, msix);
+ if (irq < 0)
+ return irq;
+ }
+
ctx = vfio_irq_ctx_alloc(vdev, vector);
if (!ctx)
return -ENOMEM;
--
2.34.1
> From: Chatre, Reinette <[email protected]>
> Sent: Friday, April 28, 2023 1:36 AM
>
> pci_msix_alloc_irq_at() enables an individual MSI-X interrupt to be
> allocated after MSI-X enabling.
>
> Use dynamic MSI-X (if supported by the device) to allocate an interrupt
> after MSI-X is enabled. An MSI-X interrupt is dynamically allocated at
> the time a valid eventfd is assigned. This is different behavior from
> a range provided during MSI-X enabling where interrupts are allocated
> for the entire range whether a valid eventfd is provided for each
> interrupt or not.
>
> The PCI-MSIX API requires that some number of irqs are allocated for
> an initial set of vectors when enabling MSI-X on the device. When
> dynamic MSIX allocation is not supported, the vector table, and thus
> the allocated irq set can only be resized by disabling and re-enabling
> MSI-X with a different range. In that case the irq allocation is
> essentially a cache for configuring vectors within the previously
> allocated vector range. When dynamic MSI-X allocation is supported,
> the API still requires some initial set of irqs to be allocated, but
> also supports allocating and freeing specific irq vectors both
> within and beyond the initially allocated range.
>
> For consistency between modes, as well as to reduce latency and improve
> reliability of allocations, and also simplicity, this implementation
> only releases irqs via pci_free_irq_vectors() when either the interrupt
> mode changes or the device is released.
It improves the reliability of allocations from the calling device p.o.v.
But system-wide this is not efficient use of irqs and not releasing them
timely may affect the reliability of allocations for other devices.
Should this behavior be something configurable?
>
> +/*
> + * Return Linux IRQ number of an MSI or MSI-X device interrupt vector.
> + * If a Linux IRQ number is not available then a new interrupt will be
> + * allocated if dynamic MSI-X is supported.
> + */
> +static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev,
> + unsigned int vector, bool msix)
> +{
> + struct pci_dev *pdev = vdev->pdev;
> + struct msi_map map;
> + int irq;
> + u16 cmd;
> +
> + irq = pci_irq_vector(pdev, vector);
> + if (irq > 0 || !msix || !vdev->has_dyn_msix)
> + return irq;
if (irq >= 0 || ...)
> +
> +/*
> + * Where is vfio_msi_free_irq() ?
> + *
> + * Allocated interrupts are maintained, essentially forming a cache that
> + * subsequent allocations can draw from. Interrupts are freed using
> + * pci_free_irq_vectors() when MSI/MSI-X is disabled.
> + */
Probably merge it with the comment of vfio_msi_alloc_irq()?
> @@ -401,6 +430,12 @@ static int vfio_msi_set_vector_signal(struct
> vfio_pci_core_device *vdev,
> if (fd < 0)
> return 0;
>
> + if (irq == -EINVAL) {
> + irq = vfio_msi_alloc_irq(vdev, vector, msix);
> + if (irq < 0)
> + return irq;
> + }
> +
> ctx = vfio_irq_ctx_alloc(vdev, vector);
> if (!ctx)
> return -ENOMEM;
This doesn't read clean that an irq is allocated but not released
in the error unwind.
Hi Kevin,
On 4/27/2023 11:50 PM, Tian, Kevin wrote:
>> From: Chatre, Reinette <[email protected]>
>> Sent: Friday, April 28, 2023 1:36 AM
>>
>> pci_msix_alloc_irq_at() enables an individual MSI-X interrupt to be
>> allocated after MSI-X enabling.
>>
>> Use dynamic MSI-X (if supported by the device) to allocate an interrupt
>> after MSI-X is enabled. An MSI-X interrupt is dynamically allocated at
>> the time a valid eventfd is assigned. This is different behavior from
>> a range provided during MSI-X enabling where interrupts are allocated
>> for the entire range whether a valid eventfd is provided for each
>> interrupt or not.
>>
>> The PCI-MSIX API requires that some number of irqs are allocated for
>> an initial set of vectors when enabling MSI-X on the device. When
>> dynamic MSIX allocation is not supported, the vector table, and thus
>> the allocated irq set can only be resized by disabling and re-enabling
>> MSI-X with a different range. In that case the irq allocation is
>> essentially a cache for configuring vectors within the previously
>> allocated vector range. When dynamic MSI-X allocation is supported,
>> the API still requires some initial set of irqs to be allocated, but
>> also supports allocating and freeing specific irq vectors both
>> within and beyond the initially allocated range.
>>
>> For consistency between modes, as well as to reduce latency and improve
>> reliability of allocations, and also simplicity, this implementation
>> only releases irqs via pci_free_irq_vectors() when either the interrupt
>> mode changes or the device is released.
>
> It improves the reliability of allocations from the calling device p.o.v.
>
> But system-wide this is not efficient use of irqs and not releasing them
> timely may affect the reliability of allocations for other devices.
Could you please elaborate how other devices may be impacted?
> Should this behavior be something configurable?
This is not clear to me and I look to you for guidance here. From practical
side it looks like configuration via module parameters is supported but
whether it should be done is not clear to me.
When considering this we need to think about what the user may expect when
turning on/off the configuration. For example, MSI-X continues to allocate a
range of interrupts during enabling. These have always been treated as a
"cache" (interrupts remain allocated, whether they have an associated trigger
or not). If there is new configurable behavior, do you expect that the
driver needs to distinguish between the original "cache" that the user is
used to and the new dynamic allocations? That is, should a dynamic MSI-X
capable device always free interrupts when user space removes an eventfd
or should only interrupts that were allocated dynamically be freed dynamically?
>> +/*
>> + * Return Linux IRQ number of an MSI or MSI-X device interrupt vector.
>> + * If a Linux IRQ number is not available then a new interrupt will be
>> + * allocated if dynamic MSI-X is supported.
>> + */
>> +static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev,
>> + unsigned int vector, bool msix)
>> +{
>> + struct pci_dev *pdev = vdev->pdev;
>> + struct msi_map map;
>> + int irq;
>> + u16 cmd;
>> +
>> + irq = pci_irq_vector(pdev, vector);
>> + if (irq > 0 || !msix || !vdev->has_dyn_msix)
>> + return irq;
>
> if (irq >= 0 || ...)
>
I am not sure about this request because pci_irq_vector() cannot return 0.
The Linux interrupt number will be > 0 on success. 0 means "not found"
(see msi_get_virq()), which is translated to -EINVAL by pci_irq_vector().
>> +
>> +/*
>> + * Where is vfio_msi_free_irq() ?
>> + *
>> + * Allocated interrupts are maintained, essentially forming a cache that
>> + * subsequent allocations can draw from. Interrupts are freed using
>> + * pci_free_irq_vectors() when MSI/MSI-X is disabled.
>> + */
>
> Probably merge it with the comment of vfio_msi_alloc_irq()?
Sure, will do.
>
>> @@ -401,6 +430,12 @@ static int vfio_msi_set_vector_signal(struct
>> vfio_pci_core_device *vdev,
>> if (fd < 0)
>> return 0;
>>
>> + if (irq == -EINVAL) {
>> + irq = vfio_msi_alloc_irq(vdev, vector, msix);
>> + if (irq < 0)
>> + return irq;
>> + }
>> +
>> ctx = vfio_irq_ctx_alloc(vdev, vector);
>> if (!ctx)
>> return -ENOMEM;
>
> This doesn't read clean that an irq is allocated but not released
> in the error unwind.
I can add a comment similar to the location where the trigger is released:
/* Interrupt stays allocated, will be freed at MSI-X disable. */
Reinette
> From: Chatre, Reinette <[email protected]>
> Sent: Saturday, April 29, 2023 2:35 AM
>
> Hi Kevin,
>
> On 4/27/2023 11:50 PM, Tian, Kevin wrote:
> >> From: Chatre, Reinette <[email protected]>
> >> Sent: Friday, April 28, 2023 1:36 AM
> >>
> >> pci_msix_alloc_irq_at() enables an individual MSI-X interrupt to be
> >> allocated after MSI-X enabling.
> >>
> >> Use dynamic MSI-X (if supported by the device) to allocate an interrupt
> >> after MSI-X is enabled. An MSI-X interrupt is dynamically allocated at
> >> the time a valid eventfd is assigned. This is different behavior from
> >> a range provided during MSI-X enabling where interrupts are allocated
> >> for the entire range whether a valid eventfd is provided for each
> >> interrupt or not.
> >>
> >> The PCI-MSIX API requires that some number of irqs are allocated for
> >> an initial set of vectors when enabling MSI-X on the device. When
> >> dynamic MSIX allocation is not supported, the vector table, and thus
> >> the allocated irq set can only be resized by disabling and re-enabling
> >> MSI-X with a different range. In that case the irq allocation is
> >> essentially a cache for configuring vectors within the previously
> >> allocated vector range. When dynamic MSI-X allocation is supported,
> >> the API still requires some initial set of irqs to be allocated, but
> >> also supports allocating and freeing specific irq vectors both
> >> within and beyond the initially allocated range.
> >>
> >> For consistency between modes, as well as to reduce latency and improve
> >> reliability of allocations, and also simplicity, this implementation
> >> only releases irqs via pci_free_irq_vectors() when either the interrupt
> >> mode changes or the device is released.
> >
> > It improves the reliability of allocations from the calling device p.o.v.
> >
> > But system-wide this is not efficient use of irqs and not releasing them
> > timely may affect the reliability of allocations for other devices.
>
> Could you please elaborate how other devices may be impacted?
the more this devices reserves the less remains for others, e.g. irte entries.
>
> > Should this behavior be something configurable?
>
> This is not clear to me and I look to you for guidance here. From practical
> side it looks like configuration via module parameters is supported but
> whether it should be done is not clear to me.
>
> When considering this we need to think about what the user may expect
> when
> turning on/off the configuration. For example, MSI-X continues to allocate a
> range of interrupts during enabling. These have always been treated as a
> "cache" (interrupts remain allocated, whether they have an associated
> trigger
> or not). If there is new configurable behavior, do you expect that the
> driver needs to distinguish between the original "cache" that the user is
> used to and the new dynamic allocations? That is, should a dynamic MSI-X
> capable device always free interrupts when user space removes an eventfd
> or should only interrupts that were allocated dynamically be freed
> dynamically?
That looks tricky. Probably that is why Alex suggested doing this simple
scheme and it is on par with the old logic anyway. So I'll withdraw this
comment.
>
> >> +/*
> >> + * Return Linux IRQ number of an MSI or MSI-X device interrupt vector.
> >> + * If a Linux IRQ number is not available then a new interrupt will be
> >> + * allocated if dynamic MSI-X is supported.
> >> + */
> >> +static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev,
> >> + unsigned int vector, bool msix)
> >> +{
> >> + struct pci_dev *pdev = vdev->pdev;
> >> + struct msi_map map;
> >> + int irq;
> >> + u16 cmd;
> >> +
> >> + irq = pci_irq_vector(pdev, vector);
> >> + if (irq > 0 || !msix || !vdev->has_dyn_msix)
> >> + return irq;
> >
> > if (irq >= 0 || ...)
> >
>
> I am not sure about this request because pci_irq_vector() cannot return 0.
> The Linux interrupt number will be > 0 on success. 0 means "not found"
> (see msi_get_virq()), which is translated to -EINVAL by pci_irq_vector().
>
There is a subtle difference between the description and the code of
pci_irq_vector().
/**
* pci_irq_vector() - Get Linux IRQ number of a device interrupt vector
* @dev: the PCI device to operate on
* @nr: device-relative interrupt vector index (0-based); has different
* meanings, depending on interrupt mode:
*
* * MSI-X the index in the MSI-X vector table
* * MSI the index of the enabled MSI vectors
* * INTx must be 0
*
* Return: the Linux IRQ number, or -EINVAL if @nr is out of range
*/
From above '0' is a valid irq number.
then in following code:
irq = msi_get_virq(&dev->dev, nr);
return irq ? irq : -EINVAL;
'0' is obviously invalid for msi.
I didn't realize the msi part when reading the patch. It left me in
confusion that '0' is unhandled as here we only check ">0" while in
other places "-EINVAL" is checked.
Not big matter but it sounds slightly clearer to me to follow the
description of pci_irq_vector() instead of its internal detail.
On Fri, 5 May 2023 08:10:33 +0000
"Tian, Kevin" <[email protected]> wrote:
> > From: Chatre, Reinette <[email protected]>
> > Sent: Saturday, April 29, 2023 2:35 AM
> >
> > Hi Kevin,
> >
> > On 4/27/2023 11:50 PM, Tian, Kevin wrote:
> > >> From: Chatre, Reinette <[email protected]>
> > >> Sent: Friday, April 28, 2023 1:36 AM
> > >>
> > >> pci_msix_alloc_irq_at() enables an individual MSI-X interrupt to be
> > >> allocated after MSI-X enabling.
> > >>
> > >> Use dynamic MSI-X (if supported by the device) to allocate an interrupt
> > >> after MSI-X is enabled. An MSI-X interrupt is dynamically allocated at
> > >> the time a valid eventfd is assigned. This is different behavior from
> > >> a range provided during MSI-X enabling where interrupts are allocated
> > >> for the entire range whether a valid eventfd is provided for each
> > >> interrupt or not.
> > >>
> > >> The PCI-MSIX API requires that some number of irqs are allocated for
> > >> an initial set of vectors when enabling MSI-X on the device. When
> > >> dynamic MSIX allocation is not supported, the vector table, and thus
> > >> the allocated irq set can only be resized by disabling and re-enabling
> > >> MSI-X with a different range. In that case the irq allocation is
> > >> essentially a cache for configuring vectors within the previously
> > >> allocated vector range. When dynamic MSI-X allocation is supported,
> > >> the API still requires some initial set of irqs to be allocated, but
> > >> also supports allocating and freeing specific irq vectors both
> > >> within and beyond the initially allocated range.
> > >>
> > >> For consistency between modes, as well as to reduce latency and improve
> > >> reliability of allocations, and also simplicity, this implementation
> > >> only releases irqs via pci_free_irq_vectors() when either the interrupt
> > >> mode changes or the device is released.
> > >
> > > It improves the reliability of allocations from the calling device p.o.v.
> > >
> > > But system-wide this is not efficient use of irqs and not releasing them
> > > timely may affect the reliability of allocations for other devices.
> >
> > Could you please elaborate how other devices may be impacted?
>
> the more this devices reserves the less remains for others, e.g. irte entries.
>
> >
> > > Should this behavior be something configurable?
> >
> > This is not clear to me and I look to you for guidance here. From practical
> > side it looks like configuration via module parameters is supported but
> > whether it should be done is not clear to me.
> >
> > When considering this we need to think about what the user may expect
> > when
> > turning on/off the configuration. For example, MSI-X continues to allocate a
> > range of interrupts during enabling. These have always been treated as a
> > "cache" (interrupts remain allocated, whether they have an associated
> > trigger
> > or not). If there is new configurable behavior, do you expect that the
> > driver needs to distinguish between the original "cache" that the user is
> > used to and the new dynamic allocations? That is, should a dynamic MSI-X
> > capable device always free interrupts when user space removes an eventfd
> > or should only interrupts that were allocated dynamically be freed
> > dynamically?
>
> That looks tricky. Probably that is why Alex suggested doing this simple
> scheme and it is on par with the old logic anyway. So I'll withdraw this
> comment.
Don't forget we're also releasing the irq reservations when the guest
changes interrupt mode, ex. reboot, so the "caching" is really only
within a session of the guest/userspace driver where it would be
unusual to have an unused reservation for an extended period. Thanks,
Alex
Hi Kevin,
On 5/5/2023 1:10 AM, Tian, Kevin wrote:
>> From: Chatre, Reinette <[email protected]>
>> Sent: Saturday, April 29, 2023 2:35 AM
>> On 4/27/2023 11:50 PM, Tian, Kevin wrote:
>>>> From: Chatre, Reinette <[email protected]>
>>>> Sent: Friday, April 28, 2023 1:36 AM
...
>>>> +/*
>>>> + * Return Linux IRQ number of an MSI or MSI-X device interrupt vector.
>>>> + * If a Linux IRQ number is not available then a new interrupt will be
>>>> + * allocated if dynamic MSI-X is supported.
>>>> + */
>>>> +static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev,
>>>> + unsigned int vector, bool msix)
>>>> +{
>>>> + struct pci_dev *pdev = vdev->pdev;
>>>> + struct msi_map map;
>>>> + int irq;
>>>> + u16 cmd;
>>>> +
>>>> + irq = pci_irq_vector(pdev, vector);
>>>> + if (irq > 0 || !msix || !vdev->has_dyn_msix)
>>>> + return irq;
>>>
>>> if (irq >= 0 || ...)
>>>
>>
>> I am not sure about this request because pci_irq_vector() cannot return 0.
>> The Linux interrupt number will be > 0 on success. 0 means "not found"
>> (see msi_get_virq()), which is translated to -EINVAL by pci_irq_vector().
>>
>
> There is a subtle difference between the description and the code of
> pci_irq_vector().
>
> /**
> * pci_irq_vector() - Get Linux IRQ number of a device interrupt vector
> * @dev: the PCI device to operate on
> * @nr: device-relative interrupt vector index (0-based); has different
> * meanings, depending on interrupt mode:
> *
> * * MSI-X the index in the MSI-X vector table
> * * MSI the index of the enabled MSI vectors
> * * INTx must be 0
> *
> * Return: the Linux IRQ number, or -EINVAL if @nr is out of range
> */
>
> From above '0' is a valid irq number.
>
> then in following code:
>
> irq = msi_get_virq(&dev->dev, nr);
> return irq ? irq : -EINVAL;
>
> '0' is obviously invalid for msi.
>
> I didn't realize the msi part when reading the patch. It left me in
> confusion that '0' is unhandled as here we only check ">0" while in
> other places "-EINVAL" is checked.
>
> Not big matter but it sounds slightly clearer to me to follow the
> description of pci_irq_vector() instead of its internal detail.
I can add an explicit check for '0' and, as you confirmed, this is
invalid for MSI and thus I think it should be treated as an error.
This is perhaps another candidate for a WARN considering that
pci_irq_vector() returning a '0' for MSI indicates a kernel problem .
I now consider taking guidance from pci_irq_get_affinity(). Note that
pci_irq_get_affinity() contains:
const struct cpumask *pci_irq_get_affinity(struct pci_dev *dev, int nr)
{
int idx, irq = pci_irq_vector(dev, nr);
...
if (WARN_ON_ONCE(irq <= 0))
return NULL;
...
}
Would you be ok with something like below?
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index b549f5c97cb8..a8e96254f953 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -393,6 +393,8 @@ static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev,
u16 cmd;
irq = pci_irq_vector(pdev, vector);
+ if (WARN_ON_ONCE(irq == 0))
+ return -EINVAL;
if (irq > 0 || !msix || !vdev->has_dyn_msix)
return irq;
I would prefer that vfio_msi_alloc_irq() returns negative errors. This enables
callers to in turn just return the error code on failure (note that dynamic
allocation can return different error codes), not needing to translate 0 into
an error.
Reinette
> From: Chatre, Reinette <[email protected]>
> Sent: Saturday, May 6, 2023 1:21 AM
>
> Would you be ok with something like below?
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index b549f5c97cb8..a8e96254f953 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -393,6 +393,8 @@ static int vfio_msi_alloc_irq(struct
> vfio_pci_core_device *vdev,
> u16 cmd;
>
> irq = pci_irq_vector(pdev, vector);
> + if (WARN_ON_ONCE(irq == 0))
> + return -EINVAL;
> if (irq > 0 || !msix || !vdev->has_dyn_msix)
> return irq;
>
> I would prefer that vfio_msi_alloc_irq() returns negative errors. This enables
> callers to in turn just return the error code on failure (note that dynamic
> allocation can return different error codes), not needing to translate 0 into
> an error.
>
This looks good to me. Thanks.
> From: Alex Williamson <[email protected]>
> Sent: Friday, May 5, 2023 11:28 PM
>
> On Fri, 5 May 2023 08:10:33 +0000
> "Tian, Kevin" <[email protected]> wrote:
>
> > > From: Chatre, Reinette <[email protected]>
> > > Sent: Saturday, April 29, 2023 2:35 AM
> > >
> > > Hi Kevin,
> > >
> > > On 4/27/2023 11:50 PM, Tian, Kevin wrote:
> > >
> > > > Should this behavior be something configurable?
> > >
> > > This is not clear to me and I look to you for guidance here. From practical
> > > side it looks like configuration via module parameters is supported but
> > > whether it should be done is not clear to me.
> > >
> > > When considering this we need to think about what the user may expect
> > > when
> > > turning on/off the configuration. For example, MSI-X continues to
> allocate a
> > > range of interrupts during enabling. These have always been treated as a
> > > "cache" (interrupts remain allocated, whether they have an associated
> > > trigger
> > > or not). If there is new configurable behavior, do you expect that the
> > > driver needs to distinguish between the original "cache" that the user is
> > > used to and the new dynamic allocations? That is, should a dynamic MSI-
> X
> > > capable device always free interrupts when user space removes an
> eventfd
> > > or should only interrupts that were allocated dynamically be freed
> > > dynamically?
> >
> > That looks tricky. Probably that is why Alex suggested doing this simple
> > scheme and it is on par with the old logic anyway. So I'll withdraw this
> > comment.
>
> Don't forget we're also releasing the irq reservations when the guest
> changes interrupt mode, ex. reboot, so the "caching" is really only
> within a session of the guest/userspace driver where it would be
> unusual to have an unused reservation for an extended period. Thanks,
>
Yeah, that makes sense.