| Message ID | 20200206214509.16434-8-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/vfio_ccw: Channel Path Handling [QEMU] | expand |
On Thu, 6 Feb 2020 22:45:09 +0100 Eric Farman <farman@linux.ibm.com> wrote: > From: Farhan Ali <alifm@linux.ibm.com> > > The CRW irq will be used by vfio-ccw to notify the userspace > about any CRWs the userspace needs to handle. Let's add support > for it. > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com> > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > > Notes: > v1->v2: > - Add a loop to continually read region while data is > present, queueing CRWs as found [CH] > v0->v1: [EF] > - Check vcdev->crw_region before registering the irq, > in case host kernel does not have matching support > - Split the refactoring changes to an earlier (new) patch > (and don't remove the "num_irqs" check in the register > routine, but adjust it to the check the input variable) > - Don't revert the cool vfio_set_irq_signaling() stuff > - Unregister CRW IRQ before IO IRQ in unrealize > - s/crw1/crw0/ > > hw/vfio/ccw.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > @@ -265,6 +266,40 @@ static void vfio_ccw_reset(DeviceState *dev) > ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET); > } > > +static void vfio_ccw_crw_notifier_handler(void *opaque) > +{ > + VFIOCCWDevice *vcdev = opaque; > + struct ccw_crw_region *region = vcdev->crw_region; > + CRW crw; > + int size; > + uint8_t rsc, erc; > + > + if (!event_notifier_test_and_clear(&vcdev->crw_notifier)) { > + return; > + } > + > + do { > + memset(region, 0, sizeof(*region)); > + size = pread(vcdev->vdev.fd, region, vcdev->crw_region_size, > + vcdev->crw_region_offset); > + > + if (size == -1) { > + error_report("vfio-ccw: Read crw region failed with errno=%d", errno); > + break; > + } > + > + if (size == 0 || region->crw0 == 0) { Does it make any sense to expect both of them as an indication that there are no more crws at the moment? Grabbing a zeroed crw makes the most sense as a stop condition, I think. Also, I'm not sure anymore whether having space for two crws makes too much sense. If we have a case in the future where we get two chained crws, the code will retry anyway and just fetch the chained crw and queue it, wouldn't it? > + /* No more CRWs to queue */ > + break; > + } > + > + memcpy(&crw, ®ion->crw0, sizeof(CRW)); > + rsc = (crw.flags & 0x0f00) >> 8; > + erc = crw.flags & 0x003f; I think we already have something for that... ah yes, CRW_FLAGS_MASK_RSC and CRW_FLAGS_MASK_ERC. > + css_queue_crw(rsc, erc, 0, 0, crw.rsid); ...or maybe an alternative interface that allows us to queue a ready-made crw? > + } while (1); > +} > + > static void vfio_ccw_io_notifier_handler(void *opaque) > { > VFIOCCWDevice *vcdev = opaque;
On 4/6/20 12:22 PM, Cornelia Huck wrote: > On Thu, 6 Feb 2020 22:45:09 +0100 > Eric Farman <farman@linux.ibm.com> wrote: > >> From: Farhan Ali <alifm@linux.ibm.com> >> >> The CRW irq will be used by vfio-ccw to notify the userspace >> about any CRWs the userspace needs to handle. Let's add support >> for it. >> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> >> Signed-off-by: Eric Farman <farman@linux.ibm.com> >> --- >> >> Notes: >> v1->v2: >> - Add a loop to continually read region while data is >> present, queueing CRWs as found [CH] >> v0->v1: [EF] >> - Check vcdev->crw_region before registering the irq, >> in case host kernel does not have matching support >> - Split the refactoring changes to an earlier (new) patch >> (and don't remove the "num_irqs" check in the register >> routine, but adjust it to the check the input variable) >> - Don't revert the cool vfio_set_irq_signaling() stuff >> - Unregister CRW IRQ before IO IRQ in unrealize >> - s/crw1/crw0/ >> >> hw/vfio/ccw.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> > >> @@ -265,6 +266,40 @@ static void vfio_ccw_reset(DeviceState *dev) >> ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET); >> } >> >> +static void vfio_ccw_crw_notifier_handler(void *opaque) >> +{ >> + VFIOCCWDevice *vcdev = opaque; >> + struct ccw_crw_region *region = vcdev->crw_region; >> + CRW crw; >> + int size; >> + uint8_t rsc, erc; >> + >> + if (!event_notifier_test_and_clear(&vcdev->crw_notifier)) { >> + return; >> + } >> + >> + do { >> + memset(region, 0, sizeof(*region)); >> + size = pread(vcdev->vdev.fd, region, vcdev->crw_region_size, >> + vcdev->crw_region_offset); >> + >> + if (size == -1) { >> + error_report("vfio-ccw: Read crw region failed with errno=%d", errno); >> + break; >> + } >> + >> + if (size == 0 || region->crw0 == 0) { > > Does it make any sense to expect both of them as an indication that > there are no more crws at the moment? Grabbing a zeroed crw makes the > most sense as a stop condition, I think. I think it was overkill on my part. Though it appears I am missing the "zeroing" of the region once it got read. Whoops. Okay, those are easy fixups. > > Also, I'm not sure anymore whether having space for two crws makes too > much sense. If we have a case in the future where we get two chained > crws, the code will retry anyway and just fetch the chained crw and > queue it, wouldn't it? I suppose. I thought the reason for including them now was to avoid "if region size == 4 vs 8 vs XX" logic at some mysterious time in the future. But certainly ripping it out so we only pass a single CRW at a time would simplify this quite a bit. > >> + /* No more CRWs to queue */ >> + break; >> + } >> + >> + memcpy(&crw, ®ion->crw0, sizeof(CRW)); >> + rsc = (crw.flags & 0x0f00) >> 8; >> + erc = crw.flags & 0x003f; > > I think we already have something for that... ah yes, > CRW_FLAGS_MASK_RSC and CRW_FLAGS_MASK_ERC. Huh, look at that. :) > >> + css_queue_crw(rsc, erc, 0, 0, crw.rsid); > > ...or maybe an alternative interface that allows us to queue a > ready-made crw? Sure, that would be nice. I'll add that as an additional patch to this series, prior to this one. > >> + } while (1); >> +} >> + >> static void vfio_ccw_io_notifier_handler(void *opaque) >> { >> VFIOCCWDevice *vcdev = opaque; >
On Mon, 6 Apr 2020 17:37:18 -0400 Eric Farman <farman@linux.ibm.com> wrote: > On 4/6/20 12:22 PM, Cornelia Huck wrote: > > On Thu, 6 Feb 2020 22:45:09 +0100 > > Eric Farman <farman@linux.ibm.com> wrote: > > > >> From: Farhan Ali <alifm@linux.ibm.com> > >> > >> The CRW irq will be used by vfio-ccw to notify the userspace > >> about any CRWs the userspace needs to handle. Let's add support > >> for it. > >> > >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> > >> Signed-off-by: Eric Farman <farman@linux.ibm.com> > >> --- > >> > >> Notes: > >> v1->v2: > >> - Add a loop to continually read region while data is > >> present, queueing CRWs as found [CH] > >> v0->v1: [EF] > >> - Check vcdev->crw_region before registering the irq, > >> in case host kernel does not have matching support > >> - Split the refactoring changes to an earlier (new) patch > >> (and don't remove the "num_irqs" check in the register > >> routine, but adjust it to the check the input variable) > >> - Don't revert the cool vfio_set_irq_signaling() stuff > >> - Unregister CRW IRQ before IO IRQ in unrealize > >> - s/crw1/crw0/ > >> > >> hw/vfio/ccw.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 51 insertions(+) > >> > > > >> @@ -265,6 +266,40 @@ static void vfio_ccw_reset(DeviceState *dev) > >> ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET); > >> } > >> > >> +static void vfio_ccw_crw_notifier_handler(void *opaque) > >> +{ > >> + VFIOCCWDevice *vcdev = opaque; > >> + struct ccw_crw_region *region = vcdev->crw_region; > >> + CRW crw; > >> + int size; > >> + uint8_t rsc, erc; > >> + > >> + if (!event_notifier_test_and_clear(&vcdev->crw_notifier)) { > >> + return; > >> + } > >> + > >> + do { > >> + memset(region, 0, sizeof(*region)); > >> + size = pread(vcdev->vdev.fd, region, vcdev->crw_region_size, > >> + vcdev->crw_region_offset); > >> + > >> + if (size == -1) { > >> + error_report("vfio-ccw: Read crw region failed with errno=%d", errno); > >> + break; > >> + } > >> + > >> + if (size == 0 || region->crw0 == 0) { > > > > Does it make any sense to expect both of them as an indication that > > there are no more crws at the moment? Grabbing a zeroed crw makes the > > most sense as a stop condition, I think. > > I think it was overkill on my part. Though it appears I am missing the > "zeroing" of the region once it got read. Whoops. Okay, those are easy > fixups. Yes, just looking at the zeroed region (after changing the kernel part) seems like the right thing here. > > > > > Also, I'm not sure anymore whether having space for two crws makes too > > much sense. If we have a case in the future where we get two chained > > crws, the code will retry anyway and just fetch the chained crw and > > queue it, wouldn't it? > > I suppose. > > I thought the reason for including them now was to avoid "if region size > == 4 vs 8 vs XX" logic at some mysterious time in the future. But > certainly ripping it out so we only pass a single CRW at a time would > simplify this quite a bit. Yes, injecting in a loop is easy anyway. > > > > >> + /* No more CRWs to queue */ > >> + break; > >> + } > >> + > >> + memcpy(&crw, ®ion->crw0, sizeof(CRW)); > >> + rsc = (crw.flags & 0x0f00) >> 8; > >> + erc = crw.flags & 0x003f; > > > > I think we already have something for that... ah yes, > > CRW_FLAGS_MASK_RSC and CRW_FLAGS_MASK_ERC. > > Huh, look at that. :) > > > > >> + css_queue_crw(rsc, erc, 0, 0, crw.rsid); > > > > ...or maybe an alternative interface that allows us to queue a > > ready-made crw? > > Sure, that would be nice. I'll add that as an additional patch to this > series, prior to this one. Agreed, makes sense. > > > > >> + } while (1); > >> +} > >> + > >> static void vfio_ccw_io_notifier_handler(void *opaque) > >> { > >> VFIOCCWDevice *vcdev = opaque; > > >
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 044441a277..5e3d446213 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -48,6 +48,7 @@ struct VFIOCCWDevice { uint64_t crw_region_offset; struct ccw_crw_region *crw_region; EventNotifier io_notifier; + EventNotifier crw_notifier; bool force_orb_pfch; bool warned_orb_pfch; }; @@ -265,6 +266,40 @@ static void vfio_ccw_reset(DeviceState *dev) ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET); } +static void vfio_ccw_crw_notifier_handler(void *opaque) +{ + VFIOCCWDevice *vcdev = opaque; + struct ccw_crw_region *region = vcdev->crw_region; + CRW crw; + int size; + uint8_t rsc, erc; + + if (!event_notifier_test_and_clear(&vcdev->crw_notifier)) { + return; + } + + do { + memset(region, 0, sizeof(*region)); + size = pread(vcdev->vdev.fd, region, vcdev->crw_region_size, + vcdev->crw_region_offset); + + if (size == -1) { + error_report("vfio-ccw: Read crw region failed with errno=%d", errno); + break; + } + + if (size == 0 || region->crw0 == 0) { + /* No more CRWs to queue */ + break; + } + + memcpy(&crw, ®ion->crw0, sizeof(CRW)); + rsc = (crw.flags & 0x0f00) >> 8; + erc = crw.flags & 0x003f; + css_queue_crw(rsc, erc, 0, 0, crw.rsid); + } while (1); +} + static void vfio_ccw_io_notifier_handler(void *opaque) { VFIOCCWDevice *vcdev = opaque; @@ -351,6 +386,10 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, notifier = &vcdev->io_notifier; fd_read = vfio_ccw_io_notifier_handler; break; + case VFIO_CCW_CRW_IRQ_INDEX: + notifier = &vcdev->crw_notifier; + fd_read = vfio_ccw_crw_notifier_handler; + break; default: error_setg(errp, "vfio: Unsupported device irq(%d)", irq); return; @@ -401,6 +440,9 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, case VFIO_CCW_IO_IRQ_INDEX: notifier = &vcdev->io_notifier; break; + case VFIO_CCW_CRW_IRQ_INDEX: + notifier = &vcdev->crw_notifier; + break; default: error_report("vfio: Unsupported device irq(%d)", irq); return; @@ -621,6 +663,14 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) goto out_notifier_err; } + if (vcdev->crw_region) { + vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, &err); + if (err) { + vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX); + goto out_notifier_err; + } + } + return; out_notifier_err: @@ -645,6 +695,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp) S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); VFIOGroup *group = vcdev->vdev.group; + vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX); vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX); vfio_ccw_put_region(vcdev); vfio_ccw_put_device(vcdev);