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

[Eric Farman]

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.

> 
> 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, &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.

> 
>> +    } while (1);
>> +}
>> +
>>  static void vfio_ccw_io_notifier_handler(void *opaque)
>>  {
>>      VFIOCCWDevice *vcdev = opaque;
>