Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
Hi Alex, On 5/16/19 12:52 AM, Alex Williamson wrote: > On Tue, 9 Apr 2019 17:58:31 +0200 > Eric Auger wrote: > >> The code used to assign an interrupt index/subindex to an >> eventfd is duplicated many times. Let's introduce an helper that >> allows to set/unset the signaling for an ACTION_TRIGGER or >> ACTION_UNMASK action. >> >> Signed-off-by: Eric Auger >> >> --- >> >> This is a follow-up to >> [PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler(). >> It looks to me that introducing vfio_set_irq_signaling() has more >> benefits in term of code reduction and the helper abstraction >> looks cleaner. >> --- >> hw/vfio/common.c | 61 + >> hw/vfio/pci.c | 224 -- >> hw/vfio/platform.c| 55 +++-- >> include/hw/vfio/vfio-common.h | 2 + >> 4 files changed, 134 insertions(+), 208 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 4374cc6176..f88fd10ca3 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -95,6 +95,67 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev, int >> index) >> ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set); >> } >> >> +static inline const char *action_to_str(int action) >> +{ >> +switch (action) { >> +case VFIO_IRQ_SET_ACTION_MASK: >> +return "MASK"; >> +case VFIO_IRQ_SET_ACTION_UNMASK: >> +return "UNMASK"; >> +case VFIO_IRQ_SET_ACTION_TRIGGER: >> +return "TRIGGER"; >> +default: >> +return "UNKNOWN ACTION"; >> +} >> +} >> + >> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, >> + int action, int fd, Error **errp) >> +{ >> +struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info), >> + .index = index }; >> +struct vfio_irq_set *irq_set; >> +int argsz, ret = 0; >> +int32_t *pfd; >> + >> +ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info); >> +if (ret < 0) { >> +error_setg_errno(errp, errno, "index %d does not exist", index); >> +goto error; >> +} >> +if (irq_info.count < subindex + 1) { >> +error_setg_errno(errp, errno, "subindex %d does not exist", >> subindex); >> +goto error; >> +} >> + >> +argsz = sizeof(*irq_set) + sizeof(*pfd); >> + >> +irq_set = g_malloc0(argsz); >> +irq_set->argsz = argsz; >> +irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | action; >> +irq_set->index = index; >> +irq_set->start = subindex; >> +irq_set->count = 1; >> +pfd = (int32_t *)&irq_set->data; >> +*pfd = fd; >> + >> +ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set); > > Hi Eric, > > Sorry for the long delay. While I like the code reduction and > simplification, is it really acceptable that every SET_IRQS ioctl is > now a GET_IRQ_INFO + SET_IRQS? Are we trying to protect against > devices dynamically changing their interrupt support? Do we not trust > the callers? I agree this is generally not needed. I will remove the check. > >> + >> +g_free(irq_set); >> + >> +if (ret) { >> +error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure"); >> +goto error; >> +} >> +return 0; >> +error: >> +error_prepend(errp, >> + "Failed to %s %s eventfd signaling for interrupt [%d,%d]: >> ", >> + fd < 0 ? "tear down" : "set up", action_to_str(action), >> + index, subindex); > > > Maybe icing on the cake, but this leaves me wishing it printed "MSIX-3" > rather than "[2,3]" for a PCI device ;) OK > > >> +return ret; >> +} >> + >> /* >> * IO Port/MMIO - Beware of the endians, VFIO is always little endian >> */ >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 504019c458..cd93ff6fa3 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c > [snip] >> @@ -2718,77 +2630,43 @@ static void vfio_req_notifier_handler(void *opaque) >> >> static void vfio_register_req_notifier(VFIOPCIDevice *vdev) >> { >> -struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info), >> - .index = VFIO_PCI_REQ_IRQ_INDEX }; >> -int argsz; >> -struct vfio_irq_set *irq_set; >> -int32_t *pfd; >> +Error *err = NULL; >> +int32_t fd; >> >> if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) { >> return; >> } >> >> -if (ioctl(vdev->vbasedev.fd, >> - VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < >> 1) { >> -return; >> -} > > Here we used GET_IRQ_INFO to quietly only enable the request notifier > when it's available, now it looks like this is no longer quiet if that > support is unavailable. Is that intentional? Thanks, not really I acknowledge ;-) I will restore that quiet check here. Thanks Eric > > Alex > >> - >> if (event_notifier_init(&vdev->req_notifier, 0)) { >> error_report("vfio:
Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
On Tue, 9 Apr 2019 17:58:31 +0200 Eric Auger wrote: > The code used to assign an interrupt index/subindex to an > eventfd is duplicated many times. Let's introduce an helper that > allows to set/unset the signaling for an ACTION_TRIGGER or > ACTION_UNMASK action. > > Signed-off-by: Eric Auger > > --- > > This is a follow-up to > [PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler(). > It looks to me that introducing vfio_set_irq_signaling() has more > benefits in term of code reduction and the helper abstraction > looks cleaner. > --- > hw/vfio/common.c | 61 + > hw/vfio/pci.c | 224 -- > hw/vfio/platform.c| 55 +++-- > include/hw/vfio/vfio-common.h | 2 + > 4 files changed, 134 insertions(+), 208 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 4374cc6176..f88fd10ca3 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -95,6 +95,67 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev, int > index) > ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set); > } > > +static inline const char *action_to_str(int action) > +{ > +switch (action) { > +case VFIO_IRQ_SET_ACTION_MASK: > +return "MASK"; > +case VFIO_IRQ_SET_ACTION_UNMASK: > +return "UNMASK"; > +case VFIO_IRQ_SET_ACTION_TRIGGER: > +return "TRIGGER"; > +default: > +return "UNKNOWN ACTION"; > +} > +} > + > +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex, > + int action, int fd, Error **errp) > +{ > +struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info), > + .index = index }; > +struct vfio_irq_set *irq_set; > +int argsz, ret = 0; > +int32_t *pfd; > + > +ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info); > +if (ret < 0) { > +error_setg_errno(errp, errno, "index %d does not exist", index); > +goto error; > +} > +if (irq_info.count < subindex + 1) { > +error_setg_errno(errp, errno, "subindex %d does not exist", > subindex); > +goto error; > +} > + > +argsz = sizeof(*irq_set) + sizeof(*pfd); > + > +irq_set = g_malloc0(argsz); > +irq_set->argsz = argsz; > +irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | action; > +irq_set->index = index; > +irq_set->start = subindex; > +irq_set->count = 1; > +pfd = (int32_t *)&irq_set->data; > +*pfd = fd; > + > +ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set); Hi Eric, Sorry for the long delay. While I like the code reduction and simplification, is it really acceptable that every SET_IRQS ioctl is now a GET_IRQ_INFO + SET_IRQS? Are we trying to protect against devices dynamically changing their interrupt support? Do we not trust the callers? > + > +g_free(irq_set); > + > +if (ret) { > +error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure"); > +goto error; > +} > +return 0; > +error: > +error_prepend(errp, > + "Failed to %s %s eventfd signaling for interrupt [%d,%d]: > ", > + fd < 0 ? "tear down" : "set up", action_to_str(action), > + index, subindex); Maybe icing on the cake, but this leaves me wishing it printed "MSIX-3" rather than "[2,3]" for a PCI device ;) > +return ret; > +} > + > /* > * IO Port/MMIO - Beware of the endians, VFIO is always little endian > */ > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 504019c458..cd93ff6fa3 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c [snip] > @@ -2718,77 +2630,43 @@ static void vfio_req_notifier_handler(void *opaque) > > static void vfio_register_req_notifier(VFIOPCIDevice *vdev) > { > -struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info), > - .index = VFIO_PCI_REQ_IRQ_INDEX }; > -int argsz; > -struct vfio_irq_set *irq_set; > -int32_t *pfd; > +Error *err = NULL; > +int32_t fd; > > if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) { > return; > } > > -if (ioctl(vdev->vbasedev.fd, > - VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < > 1) { > -return; > -} Here we used GET_IRQ_INFO to quietly only enable the request notifier when it's available, now it looks like this is no longer quiet if that support is unavailable. Is that intentional? Thanks, Alex > - > if (event_notifier_init(&vdev->req_notifier, 0)) { > error_report("vfio: Unable to init event notifier for device > request"); > return; > } > > -argsz = sizeof(*irq_set) + sizeof(*pfd); > +fd = event_notifier_get_fd(&vdev->req_notifier); > +qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev); > > -irq_set = g_malloc0(argsz); > -irq_set->argsz = argsz; > -irq_set->flags = VFIO_IRQ_SET_DAT
Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
Hi Cornelia On 4/12/19 1:31 PM, Cornelia Huck wrote: > On Tue, 9 Apr 2019 17:58:31 +0200 > Eric Auger wrote: > >> The code used to assign an interrupt index/subindex to an >> eventfd is duplicated many times. Let's introduce an helper that >> allows to set/unset the signaling for an ACTION_TRIGGER or >> ACTION_UNMASK action. > > I like that, and it looks like ccw can use the new function as well. (I > can do a patch on top.) Thanks! Yes, feel free to proceed with the ccw patch once this gets merged. > >> >> Signed-off-by: Eric Auger >> >> --- >> >> This is a follow-up to >> [PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler(). >> It looks to me that introducing vfio_set_irq_signaling() has more >> benefits in term of code reduction and the helper abstraction >> looks cleaner. >> --- >> hw/vfio/common.c | 61 + >> hw/vfio/pci.c | 224 -- >> hw/vfio/platform.c| 55 +++-- >> include/hw/vfio/vfio-common.h | 2 + >> 4 files changed, 134 insertions(+), 208 deletions(-) > > Reviewed-by: Cornelia Huck Thanks Eric >
Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
On Tue, 9 Apr 2019 17:58:31 +0200 Eric Auger wrote: > The code used to assign an interrupt index/subindex to an > eventfd is duplicated many times. Let's introduce an helper that > allows to set/unset the signaling for an ACTION_TRIGGER or > ACTION_UNMASK action. I like that, and it looks like ccw can use the new function as well. (I can do a patch on top.) > > Signed-off-by: Eric Auger > > --- > > This is a follow-up to > [PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler(). > It looks to me that introducing vfio_set_irq_signaling() has more > benefits in term of code reduction and the helper abstraction > looks cleaner. > --- > hw/vfio/common.c | 61 + > hw/vfio/pci.c | 224 -- > hw/vfio/platform.c| 55 +++-- > include/hw/vfio/vfio-common.h | 2 + > 4 files changed, 134 insertions(+), 208 deletions(-) Reviewed-by: Cornelia Huck