Re: [Qemu-devel] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property

[Halil Pasic <pasic@linux.ibm.com>]

On 05/14/2018 02:18 PM, Cornelia Huck wrote:
> On Thu, 10 May 2018 02:07:11 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> There is at least one control program (guest) that although it does not
> 
> I'd drop 'control program' here as well, as it probably confuses more
> than helps.
> 

Will do (everywhere).

>> rely on the guarantees provided by ORB 1 word 9 bit (aka unlimited
>> prefetch, aka P bit) not being set, fails to tell this to the machine.
>>
>> Usually this ain't a big deal, as the story is usually about performance
>> optimizations only. But vfio-ccw can not provide the guarantees required
>> if the bit is not set.
> 
> Isn't that also about channel program rewriting? Or am I mixing things
> up?
> 

I don't understand the question. Can you rephrase it (maybe with more
details)?

>>
>> Since it is impossible to implement support for P bit not set (at
>> impossible least without transitioning to lower level protocols) in
>> vfio-ccw let's provide a manual override.
> 
> Hm... so the basic idea seems to be "we don't support !PFCH, but we
> know that the guest will not rely on the guarantees, so we provide the
> host admin with a way to override the setting"?
> 

That is the idea, although I'm not sure what 'the setting' is.

>>
>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> Suggested-by: Dong Jia Shi <bjsdjshi@linux.ibm.com>
>> Acked-by: Jason J. Herne <jjherne@linux.ibm.com>
>> Tested-by: Jason J. Herne <jjherne@linux.ibm.com>
>> ---
>>   hw/s390x/css.c |  3 +--
>>   hw/vfio/ccw.c  | 13 +++++++++++++
>>   2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 301bf1772f..32f1b2820d 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1196,8 +1196,7 @@ static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>>        * Only support prefetch enable mode.
>>        * Only support 64bit addressing idal.
>>        */
>> -    if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>> -        !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>> +    if (!(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>>           warn_report("vfio-ccw requires PFCH and C64 flags set");
> 
> Adapt this warning?
> 
>>           sch_gen_unit_exception(sch);
>>           css_inject_io_interrupt(sch);
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index e67392c5f9..32cf606a71 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -32,6 +32,8 @@ typedef struct VFIOCCWDevice {
>>       uint64_t io_region_offset;
>>       struct ccw_io_region *io_region;
>>       EventNotifier io_notifier;
>> +    /* force unlimited prefetch */
>> +    bool f_upfch;
> 
> force_unlimited_prefetch? You only use it that often :)
> 

I would have expected complaints for the property name in the
first place. I think we should first find a good name for the
property and then consider the rest.

>>   } VFIOCCWDevice;
>>   
>>   static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
>> @@ -52,8 +54,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>>       S390CCWDevice *cdev = sch->driver_data;
>>       VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
>>       struct ccw_io_region *region = vcdev->io_region;
>> +    bool upfch = sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH;
> 
> Frankly, I'd drop that variable...
> 
>>       int ret;
>>   
>> +    if (!upfch && !vcdev->f_upfch) {
>> +        warn_report("vfio-ccw requires PFCH flag set");
>> +        sch_gen_unit_exception(sch);
>> +        css_inject_io_interrupt(sch);
>> +        return IOINST_CC_EXPECTED;
>> +    } else if (!upfch) {
>> +        sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>> +    }
> 
> and do
> 
> if (!(sch->orb.ctrl0 & ORB_CTR0_MASK_PFCH)) {
>    if (!vcdev->f_upfch) {
>      ...error...
>    } else {
>      ...set bit...
>    }
> }
> 
> Avoids discussions around variable naming, as well :)
> 

Seems like more indentation and more lies of code to me, but
no strong feelings. It may be easier to read.

>> +
>>       QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
>>       QEMU_BUILD_BUG_ON(sizeof(region->scsw_area) != sizeof(SCSW));
>>       QEMU_BUILD_BUG_ON(sizeof(region->irb_area) != sizeof(IRB));
>> @@ -429,6 +441,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
>>   
>>   static Property vfio_ccw_properties[] = {
>>       DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev),
>> +    DEFINE_PROP_BOOL("f-upfch", VFIOCCWDevice, f_upfch, false),
> 
> Any particular reason you want to control this on a device-by-device
> level?
> 

It seemed natural for me. What are our options here? I don't like
machine property, as it is not a machine thing.

Regards,
Halil

>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
> 
>