Re: [RFC PATCH v2 7/7] vfio-ccw: Add support for the CRW irq

[Cornelia Huck]

On Mon, 6 Apr 2020 17:37:18 -0400
Eric Farman <address@hidden> wrote:

> On 4/6/20 12:22 PM, Cornelia Huck wrote:
> > On Thu,  6 Feb 2020 22:45:09 +0100
> > Eric Farman <address@hidden> wrote:
> >   
> >> From: Farhan Ali <address@hidden>
> >>
> >> 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 <address@hidden>
> >> Signed-off-by: Eric Farman <address@hidden>
> >> ---
> >>
> >> 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, &region->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;  
> >   
>