Re: [PATCH v4] vfio/pci: Propagate ACPI notifications to user-space via eventfd

On Wed, 7 Jun 2023 22:22:12 +0200 Grzegorz Jaszczyk <jaz(a)semihalf.com&gt; wrote: > > > > > > Can we drop the NTFY and just use VFIO_PCI_ACPI_IRQ_INDEX? > > > > ACPI_IRQ at first glance could be confused with SCI, which is e.g. > > registered as "acpi" irq seen in /proc/interrupts, maybe it is worth > > keeping NTFY here to emphasise the "Notify" part? > > Please let me know if you prefer VFIO_PCI_ACPI_IRQ_INDEX or > VFIO_PCI_ACPI_NTFY_IRQ_INDEX taking into account the above. This is a device level ACPI interrupt, so it doesn't seem like it would be confused with SCI. What other ACPI related interrupts would a device have? I'm still partial to dropping the NTFY but if you're attached to it, let's not abbreviate it, make it NOTIFY and do the same for function names.

... > > > > + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) { > > > > + u32 notification_val; > > > > + > > > > + if (!count) > > > > + return -EINVAL; > > > > + > > > > + notification_val = *(u32 *)data; > > > > > > DATA_BOOL is defined as a u8, and of course also as a bool, so we > > > expect only zero/non-zero. I think a valid interpretation would be any > > > non-zero value generates a device check notification value. > > > > Maybe it would be helpful and ease testing if we could use u8 as a > > notification value placeholder so it would be more flexible? > > Notification values from 0x80 to 0xBF are device-specific, 0xC0 and > > above are reserved for definition by hardware vendors for hardware > > specific notifications and BTW in practice I didn't see notification > > values that do not fit in u8 but even if exist we can limit to u8 and > > gain some flexibility anyway. Please let me know what you think. > > Does the above seem ok for you? The data type is only a u8 for practicality, it's still labeled as a bool which suggests it's interpreted as either zero or non-zero. We also need to reconcile DATA_NONE, which should trigger the interrupt, but with an implicit notification value. I see the utility in what you're proposing, but it logically implies an extension of the SET_IRQS ioctl for a new data type which has hardly any practical value. Thanks,