[Qemu-devel] [PATCH v2 0/2] vfio-ccw: loosen orb flags checks

[Qemu-devel] [PATCH v2 0/2] vfio-ccw: loosen orb flags checks

[Halil Pasic]

See the individual patches (inclusive change log).

Halil Pasic (2):
  vfio-ccw: add force unlimited prefetch property
  vfio-ccw: remove orb.c64 (64 bit data addresses) check

 hw/s390x/css.c | 12 ------------
 hw/vfio/ccw.c  | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 12 deletions(-)

-- 
2.16.3

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

[Halil Pasic]

There is at least one guest (OS) such that although it does not rely on
the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka
P bit) not being set, it fails to tell this to the machine.

Usually this ain't a big deal, as the original purpose of the P bit is to
allow for performance optimizations. vfio-ccw however can not provide the
guarantees required if the bit is not set.

It is impossible to implement support for P bit not set (at impossible
least without transitioning to lower level protocols) for vfio-ccw. So
let's give the user the opportunity to force the P bit to set, if the
user knows this is safe.  For self modifying channel programs forcing the
P bit is not safe. If P bit is forced for a self modifying channel
program things are expected to break in strange ways.

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>

---
v1 -> v2:
* reworded commit message
* re-factored the code (Connie)
* added warning when the P bit is forced the first time (Connie)

---
 hw/s390x/css.c |  3 +--
 hw/vfio/ccw.c  | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 56c3fa8c89..39ae5bbf7e 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1204,8 +1204,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");
         sch_gen_unit_exception(sch);
         css_inject_io_interrupt(sch);
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e67392c5f9..62de4c9710 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -32,8 +32,20 @@ typedef struct VFIOCCWDevice {
     uint64_t io_region_offset;
     struct ccw_io_region *io_region;
     EventNotifier io_notifier;
+    bool force_orb_pfch;
+    bool warned_force_orb_pfch;
 } VFIOCCWDevice;
 
+#define WARN_ONCE(warned, fmt...) \
+({\
+if (!(warned)) {\
+    warn_report((fmt));\
+} \
+warned = true;\
+})
+
+
+
 static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
 {
     vdev->needs_reset = false;
@@ -54,6 +66,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
     struct ccw_io_region *region = vcdev->io_region;
     int ret;
 
+    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
+        if (!(vcdev->force_orb_pfch)) {
+            warn_report("vfio-ccw requires PFCH flag set");
+            sch_gen_unit_exception(sch);
+            css_inject_io_interrupt(sch);
+            return IOINST_CC_EXPECTED;
+        } else {
+            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
+            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");
+        }
+    }
+
     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 +453,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("force-orb-pfch", VFIOCCWDevice, force_orb_pfch, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.16.3

[Qemu-devel] [PATCH v2 2/2] vfio-ccw: remove orb.c64 (64 bit data addresses) check

[Halil Pasic]

The vfio-ccw module does the check too, and there is actually no
technical obstacle for supporting fmt 1 idaws. Let us be ready for the
beautiful day when fmt 1 idaws become supported by the vfio-ccw kernel
module. QEMU does not have to do a thing for that, except not insisting
on this check.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Acked-by: Jason J. Herne <jjherne@linux.ibm.com>
Tested-by: Jason J. Herne <jjherne@linux.ibm.com>
---
v1 -> v2: unchanged

---
 hw/s390x/css.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 39ae5bbf7e..5424ea4562 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1199,17 +1199,6 @@ static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
         assert(orb != NULL);
         p->intparm = orb->intparm;
     }
-
-    /*
-     * Only support prefetch enable mode.
-     * Only support 64bit addressing idal.
-     */
-    if (!(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
-        warn_report("vfio-ccw requires PFCH and C64 flags set");
-        sch_gen_unit_exception(sch);
-        css_inject_io_interrupt(sch);
-        return IOINST_CC_EXPECTED;
-    }
     return s390_ccw_cmd_request(sch);
 }
 
-- 
2.16.3

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

[Cornelia Huck]

On Wed, 23 May 2018 00:16:54 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> There is at least one guest (OS) such that although it does not rely on
> the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka
> P bit) not being set, it fails to tell this to the machine.
> 
> Usually this ain't a big deal, as the original purpose of the P bit is to
> allow for performance optimizations. vfio-ccw however can not provide the
> guarantees required if the bit is not set.
> 
> It is impossible to implement support for P bit not set (at impossible
> least without transitioning to lower level protocols) for vfio-ccw. 

"It is not possible to implement support for the P bit not set without
transitioning to lower level protocols for vfio-ccw."

?

> So
> let's give the user the opportunity to force the P bit to set, if the

s/to set/to be set/

> user knows this is safe.  For self modifying channel programs forcing the
> P bit is not safe. If P bit is forced for a self modifying channel

s/P bit/the P bit/

> program things are expected to break in strange ways.
> 
> 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>
> 
> ---
> v1 -> v2:
> * reworded commit message
> * re-factored the code (Connie)
> * added warning when the P bit is forced the first time (Connie)
> 
> ---
>  hw/s390x/css.c |  3 +--
>  hw/vfio/ccw.c  | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 56c3fa8c89..39ae5bbf7e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1204,8 +1204,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");
>          sch_gen_unit_exception(sch);
>          css_inject_io_interrupt(sch);
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index e67392c5f9..62de4c9710 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -32,8 +32,20 @@ typedef struct VFIOCCWDevice {
>      uint64_t io_region_offset;
>      struct ccw_io_region *io_region;
>      EventNotifier io_notifier;
> +    bool force_orb_pfch;
> +    bool warned_force_orb_pfch;
>  } VFIOCCWDevice;
>  
> +#define WARN_ONCE(warned, fmt...) \
> +({\
> +if (!(warned)) {\
> +    warn_report((fmt));\
> +} \
> +warned = true;\
> +})

I think introducing a macro for the single user is overkill here.

We might contemplate a generic "print this error once, controlled by
this flag" functionality, if there are more users.

> +
> +
> +
>  static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
>  {
>      vdev->needs_reset = false;
> @@ -54,6 +66,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>      struct ccw_io_region *region = vcdev->io_region;
>      int ret;
>  
> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> +        if (!(vcdev->force_orb_pfch)) {
> +            warn_report("vfio-ccw requires PFCH flag set");
> +            sch_gen_unit_exception(sch);
> +            css_inject_io_interrupt(sch);
> +            return IOINST_CC_EXPECTED;
> +        } else {
> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");

This message should probably mention vfio-ccw as well as the subchannel
id?

> +        }
> +    }
> +
>      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 +453,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("force-orb-pfch", VFIOCCWDevice, force_orb_pfch, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

Otherwise, the changes look good.

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

[Halil Pasic]

On 05/23/2018 11:37 AM, Cornelia Huck wrote:
> On Wed, 23 May 2018 00:16:54 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> There is at least one guest (OS) such that although it does not rely on
>> the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka
>> P bit) not being set, it fails to tell this to the machine.
>>
>> Usually this ain't a big deal, as the original purpose of the P bit is to
>> allow for performance optimizations. vfio-ccw however can not provide the
>> guarantees required if the bit is not set.
>>
>> It is impossible to implement support for P bit not set (at impossible
>> least without transitioning to lower level protocols) for vfio-ccw.
> 
> "It is not possible to implement support for the P bit not set without
> transitioning to lower level protocols for vfio-ccw."
> > ?

Sounds much better. My sentence is ungrammatical.

> 
>> So
>> let's give the user the opportunity to force the P bit to set, if the
> 
> s/to set/to be set/
> 

Why do we need the 'be'?

>> user knows this is safe.  For self modifying channel programs forcing the
>> P bit is not safe. If P bit is forced for a self modifying channel
> 
> s/P bit/the P bit/
> 

Right.

>> program things are expected to break in strange ways.
>>
>> 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>
>>
>> ---
>> v1 -> v2:
>> * reworded commit message
>> * re-factored the code (Connie)
>> * added warning when the P bit is forced the first time (Connie)
>>
>> ---
>>   hw/s390x/css.c |  3 +--
>>   hw/vfio/ccw.c  | 25 +++++++++++++++++++++++++
>>   2 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 56c3fa8c89..39ae5bbf7e 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1204,8 +1204,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");
>>           sch_gen_unit_exception(sch);
>>           css_inject_io_interrupt(sch);
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index e67392c5f9..62de4c9710 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -32,8 +32,20 @@ typedef struct VFIOCCWDevice {
>>       uint64_t io_region_offset;
>>       struct ccw_io_region *io_region;
>>       EventNotifier io_notifier;
>> +    bool force_orb_pfch;
>> +    bool warned_force_orb_pfch;
>>   } VFIOCCWDevice;
>>   
>> +#define WARN_ONCE(warned, fmt...) \
>> +({\
>> +if (!(warned)) {\
>> +    warn_report((fmt));\
>> +} \
>> +warned = true;\
>> +})
> 
> I think introducing a macro for the single user is overkill here.
> 
> We might contemplate a generic "print this error once, controlled by
> this flag" functionality, if there are more users.
>

I would prefer keeping the macro. If this generic functionality comes
along it will be easier to spot the home-brewn counterpart. Also it's
easier to read IMHO. BTW the macro could be an inline function like:

  static inline void warn_once(bool *warned, const char *fmt, ...)
{
     va_list ap;


     if (!warned || *warned) {
         return;
     }
     *warned= true;
     va_start(ap, fmt);
     vreport(REPORT_TYPE_WARNING, fmt, ap);
     va_end(ap);
}

if that's better.

>> +
>> +
>> +
>>   static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
>>   {
>>       vdev->needs_reset = false;
>> @@ -54,6 +66,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>>       struct ccw_io_region *region = vcdev->io_region;
>>       int ret;
>>   
>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
>> +        if (!(vcdev->force_orb_pfch)) {
>> +            warn_report("vfio-ccw requires PFCH flag set");
>> +            sch_gen_unit_exception(sch);
>> +            css_inject_io_interrupt(sch);
>> +            return IOINST_CC_EXPECTED;
>> +        } else {
>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");
> 
> This message should probably mention vfio-ccw as well as the subchannel
> id?
> 

I was thinking about this. I think all it would make sense to have a common
prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
is a separate patch.

Maybe something like:
vfio-ccw (xx.xx.xxxx): specific message

OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
AFAIR the error_setg captures context (like, src, line, func) but does not
necessarily report it. Another question is if this should be extended to
hw/s390x/s390-ccw.c

What do you think?


>> +        }
>> +    }
>> +
>>       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 +453,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("force-orb-pfch", VFIOCCWDevice, force_orb_pfch, false),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
> 
> Otherwise, the changes look good.
> 

I guess there will be a v3 then.

Thanks!

Halil

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

[Cornelia Huck]

On Wed, 23 May 2018 16:31:53 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/23/2018 11:37 AM, Cornelia Huck wrote:
> > On Wed, 23 May 2018 00:16:54 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> There is at least one guest (OS) such that although it does not rely on
> >> the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka
> >> P bit) not being set, it fails to tell this to the machine.
> >>
> >> Usually this ain't a big deal, as the original purpose of the P bit is to
> >> allow for performance optimizations. vfio-ccw however can not provide the
> >> guarantees required if the bit is not set.
> >>
> >> It is impossible to implement support for P bit not set (at impossible
> >> least without transitioning to lower level protocols) for vfio-ccw.  
> > 
> > "It is not possible to implement support for the P bit not set without
> > transitioning to lower level protocols for vfio-ccw."  
> > > ?  
> 
> Sounds much better. My sentence is ungrammatical.
> 
> >   
> >> So
> >> let's give the user the opportunity to force the P bit to set, if the  
> > 
> > s/to set/to be set/
> >   
> 
> Why do we need the 'be'?

Or "to force setting the P bit". It doesn't set itself :)

> 
> >> user knows this is safe.  For self modifying channel programs forcing the
> >> P bit is not safe. If P bit is forced for a self modifying channel  
> > 
> > s/P bit/the P bit/
> >   
> 
> Right.
> 
> >> program things are expected to break in strange ways.
> >>
> >> 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>

> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> >> index e67392c5f9..62de4c9710 100644
> >> --- a/hw/vfio/ccw.c
> >> +++ b/hw/vfio/ccw.c
> >> @@ -32,8 +32,20 @@ typedef struct VFIOCCWDevice {
> >>       uint64_t io_region_offset;
> >>       struct ccw_io_region *io_region;
> >>       EventNotifier io_notifier;
> >> +    bool force_orb_pfch;
> >> +    bool warned_force_orb_pfch;
> >>   } VFIOCCWDevice;
> >>   
> >> +#define WARN_ONCE(warned, fmt...) \
> >> +({\
> >> +if (!(warned)) {\
> >> +    warn_report((fmt));\
> >> +} \
> >> +warned = true;\
> >> +})  
> > 
> > I think introducing a macro for the single user is overkill here.
> > 
> > We might contemplate a generic "print this error once, controlled by
> > this flag" functionality, if there are more users.
> >  
> 
> I would prefer keeping the macro. If this generic functionality comes
> along it will be easier to spot the home-brewn counterpart. Also it's
> easier to read IMHO.

I'm really not too fond of that macro...

> BTW the macro could be an inline function like:
> 
>   static inline void warn_once(bool *warned, const char *fmt, ...)
> {
>      va_list ap;
> 
> 
>      if (!warned || *warned) {
>          return;
>      }
>      *warned= true;
>      va_start(ap, fmt);
>      vreport(REPORT_TYPE_WARNING, fmt, ap);
>      va_end(ap);
> }
> 
> if that's better.

I think an inline function is a better choice.

Also, if this is something that might be generally useful it should go
into util/error.c. Let's cc: Markus.

> 
> >> +
> >> +
> >> +
> >>   static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
> >>   {
> >>       vdev->needs_reset = false;
> >> @@ -54,6 +66,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
> >>       struct ccw_io_region *region = vcdev->io_region;
> >>       int ret;
> >>   
> >> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> >> +        if (!(vcdev->force_orb_pfch)) {
> >> +            warn_report("vfio-ccw requires PFCH flag set");
> >> +            sch_gen_unit_exception(sch);
> >> +            css_inject_io_interrupt(sch);
> >> +            return IOINST_CC_EXPECTED;
> >> +        } else {
> >> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> >> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");  
> > 
> > This message should probably mention vfio-ccw as well as the subchannel
> > id?
> >   
> 
> I was thinking about this. I think all it would make sense to have a common
> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
> is a separate patch.
> 
> Maybe something like:
> vfio-ccw (xx.xx.xxxx): specific message
> 
> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
> AFAIR the error_setg captures context (like, src, line, func) but does not
> necessarily report it. Another question is if this should be extended to
> hw/s390x/s390-ccw.c
> 
> What do you think?

I'm not sure that makes sense, especially as not everything might
explicitly refer to a certain subchannel.

Let's just add the subchannel id here? In this case, this is really a
useful piece of information (which device is showing this behaviour?)

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

[Halil Pasic]

On 05/23/2018 04:46 PM, Cornelia Huck wrote:
>>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
>>>> +        if (!(vcdev->force_orb_pfch)) {
>>>> +            warn_report("vfio-ccw requires PFCH flag set");
>>>> +            sch_gen_unit_exception(sch);
>>>> +            css_inject_io_interrupt(sch);
>>>> +            return IOINST_CC_EXPECTED;
>>>> +        } else {
>>>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>>>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");
>>> This message should probably mention vfio-ccw as well as the subchannel
>>> id?
>>>    
>> I was thinking about this. I think all it would make sense to have a common
>> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
>> is a separate patch.
>>
>> Maybe something like:
>> vfio-ccw (xx.xx.xxxx): specific message
>>
>> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
>> AFAIR the error_setg captures context (like, src, line, func) but does not
>> necessarily report it. Another question is if this should be extended to
>> hw/s390x/s390-ccw.c
>>
>> What do you think?
> I'm not sure that makes sense, especially as not everything might
> explicitly refer to a certain subchannel.
> 
> Let's just add the subchannel id here? In this case, this is really a
> useful piece of information (which device is showing this behaviour?)
> 

The same applies to  warn_report("vfio-ccw requires PFCH flag set") (that is,
on which device (that has no force-orb-pfch=on specified)  is the guest issuing
ORBs with the PFCH unset), or?
Should I go for
"vfio-ccw (xx.xx.xxxx): vfio-ccw requires PFCH flag set"
and
"vfio-ccw (xx.xx.xxxx): PFCH flag forced"
or just for the second one, or some third option?

Regards,
Halil

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

[Cornelia Huck]

On Wed, 23 May 2018 18:23:44 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/23/2018 04:46 PM, Cornelia Huck wrote:
> >>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> >>>> +        if (!(vcdev->force_orb_pfch)) {
> >>>> +            warn_report("vfio-ccw requires PFCH flag set");
> >>>> +            sch_gen_unit_exception(sch);
> >>>> +            css_inject_io_interrupt(sch);
> >>>> +            return IOINST_CC_EXPECTED;
> >>>> +        } else {
> >>>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> >>>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");  
> >>> This message should probably mention vfio-ccw as well as the subchannel
> >>> id?
> >>>      
> >> I was thinking about this. I think all it would make sense to have a common
> >> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
> >> is a separate patch.
> >>
> >> Maybe something like:
> >> vfio-ccw (xx.xx.xxxx): specific message
> >>
> >> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
> >> AFAIR the error_setg captures context (like, src, line, func) but does not
> >> necessarily report it. Another question is if this should be extended to
> >> hw/s390x/s390-ccw.c
> >>
> >> What do you think?  
> > I'm not sure that makes sense, especially as not everything might
> > explicitly refer to a certain subchannel.
> > 
> > Let's just add the subchannel id here? In this case, this is really a
> > useful piece of information (which device is showing this behaviour?)
> >   
> 
> The same applies to  warn_report("vfio-ccw requires PFCH flag set") (that is,
> on which device (that has no force-orb-pfch=on specified)  is the guest issuing
> ORBs with the PFCH unset), or?
> Should I go for
> "vfio-ccw (xx.xx.xxxx): vfio-ccw requires PFCH flag set"
> and
> "vfio-ccw (xx.xx.xxxx): PFCH flag forced"
> or just for the second one, or some third option?

Yes, it makes sense for both.

Related: Do we expect the guest driver to learn from its experience and
not try without pfch again? It is probably not very helpful if the logs
get filled with a lot of "vfio-ccw requires pfch" messages...

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

[Halil Pasic]

On 05/23/2018 06:59 PM, Cornelia Huck wrote:
> On Wed, 23 May 2018 18:23:44 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On 05/23/2018 04:46 PM, Cornelia Huck wrote:
>>>>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
>>>>>> +        if (!(vcdev->force_orb_pfch)) {
>>>>>> +            warn_report("vfio-ccw requires PFCH flag set");
>>>>>> +            sch_gen_unit_exception(sch);
>>>>>> +            css_inject_io_interrupt(sch);
>>>>>> +            return IOINST_CC_EXPECTED;
>>>>>> +        } else {
>>>>>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>>>>>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");
>>>>> This message should probably mention vfio-ccw as well as the subchannel
>>>>> id?
>>>>>       
>>>> I was thinking about this. I think all it would make sense to have a common
>>>> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
>>>> is a separate patch.
>>>>
>>>> Maybe something like:
>>>> vfio-ccw (xx.xx.xxxx): specific message
>>>>
>>>> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
>>>> AFAIR the error_setg captures context (like, src, line, func) but does not
>>>> necessarily report it. Another question is if this should be extended to
>>>> hw/s390x/s390-ccw.c
>>>>
>>>> What do you think?
>>> I'm not sure that makes sense, especially as not everything might
>>> explicitly refer to a certain subchannel.
>>>
>>> Let's just add the subchannel id here? In this case, this is really a
>>> useful piece of information (which device is showing this behaviour?)
>>>    
>>
>> The same applies to  warn_report("vfio-ccw requires PFCH flag set") (that is,
>> on which device (that has no force-orb-pfch=on specified)  is the guest issuing
>> ORBs with the PFCH unset), or?
>> Should I go for
>> "vfio-ccw (xx.xx.xxxx): vfio-ccw requires PFCH flag set"
>> and
>> "vfio-ccw (xx.xx.xxxx): PFCH flag forced"
>> or just for the second one, or some third option?
> 
> Yes, it makes sense for both.
> 
> Related: Do we expect the guest driver to learn from its experience and
> not try without pfch again? It is probably not very helpful if the logs
> get filled with a lot of "vfio-ccw requires pfch" messages...
> 

Don't really know. Dong Jia is probably more qualified to answer that question.
I don't expect the guest driver to do so. There are probably more intelligent
strategies to deal with this, but the question is what do we gain in the end
(linux guests are not affected). We should probably not overthink this.

Regards,
Halil

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

[Cornelia Huck]

On Wed, 23 May 2018 19:28:31 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/23/2018 06:59 PM, Cornelia Huck wrote:
> > On Wed, 23 May 2018 18:23:44 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> On 05/23/2018 04:46 PM, Cornelia Huck wrote:  
> >>>>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> >>>>>> +        if (!(vcdev->force_orb_pfch)) {
> >>>>>> +            warn_report("vfio-ccw requires PFCH flag set");
> >>>>>> +            sch_gen_unit_exception(sch);
> >>>>>> +            css_inject_io_interrupt(sch);
> >>>>>> +            return IOINST_CC_EXPECTED;
> >>>>>> +        } else {
> >>>>>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> >>>>>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");  
> >>>>> This message should probably mention vfio-ccw as well as the subchannel
> >>>>> id?
> >>>>>         
> >>>> I was thinking about this. I think all it would make sense to have a common
> >>>> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
> >>>> is a separate patch.
> >>>>
> >>>> Maybe something like:
> >>>> vfio-ccw (xx.xx.xxxx): specific message
> >>>>
> >>>> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
> >>>> AFAIR the error_setg captures context (like, src, line, func) but does not
> >>>> necessarily report it. Another question is if this should be extended to
> >>>> hw/s390x/s390-ccw.c
> >>>>
> >>>> What do you think?  
> >>> I'm not sure that makes sense, especially as not everything might
> >>> explicitly refer to a certain subchannel.
> >>>
> >>> Let's just add the subchannel id here? In this case, this is really a
> >>> useful piece of information (which device is showing this behaviour?)
> >>>      
> >>
> >> The same applies to  warn_report("vfio-ccw requires PFCH flag set") (that is,
> >> on which device (that has no force-orb-pfch=on specified)  is the guest issuing
> >> ORBs with the PFCH unset), or?
> >> Should I go for
> >> "vfio-ccw (xx.xx.xxxx): vfio-ccw requires PFCH flag set"
> >> and
> >> "vfio-ccw (xx.xx.xxxx): PFCH flag forced"
> >> or just for the second one, or some third option?  
> > 
> > Yes, it makes sense for both.
> > 
> > Related: Do we expect the guest driver to learn from its experience and
> > not try without pfch again? It is probably not very helpful if the logs
> > get filled with a lot of "vfio-ccw requires pfch" messages...
> >   
> 
> Don't really know. Dong Jia is probably more qualified to answer that question.
> I don't expect the guest driver to do so. There are probably more intelligent
> strategies to deal with this, but the question is what do we gain in the end
> (linux guests are not affected). We should probably not overthink this.

So, print both messages just once per device?

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

[Halil Pasic]

On 05/24/2018 09:16 AM, Cornelia Huck wrote:
> On Wed, 23 May 2018 19:28:31 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On 05/23/2018 06:59 PM, Cornelia Huck wrote:
>>> On Wed, 23 May 2018 18:23:44 +0200
>>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>>    
>>>> On 05/23/2018 04:46 PM, Cornelia Huck wrote:
>>>>>>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
>>>>>>>> +        if (!(vcdev->force_orb_pfch)) {
>>>>>>>> +            warn_report("vfio-ccw requires PFCH flag set");
>>>>>>>> +            sch_gen_unit_exception(sch);
>>>>>>>> +            css_inject_io_interrupt(sch);
>>>>>>>> +            return IOINST_CC_EXPECTED;
>>>>>>>> +        } else {
>>>>>>>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>>>>>>>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");
>>>>>>> This message should probably mention vfio-ccw as well as the subchannel
>>>>>>> id?
>>>>>>>          
>>>>>> I was thinking about this. I think all it would make sense to have a common
>>>>>> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
>>>>>> is a separate patch.
>>>>>>
>>>>>> Maybe something like:
>>>>>> vfio-ccw (xx.xx.xxxx): specific message
>>>>>>
>>>>>> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
>>>>>> AFAIR the error_setg captures context (like, src, line, func) but does not
>>>>>> necessarily report it. Another question is if this should be extended to
>>>>>> hw/s390x/s390-ccw.c
>>>>>>
>>>>>> What do you think?
>>>>> I'm not sure that makes sense, especially as not everything might
>>>>> explicitly refer to a certain subchannel.
>>>>>
>>>>> Let's just add the subchannel id here? In this case, this is really a
>>>>> useful piece of information (which device is showing this behaviour?)
>>>>>       
>>>>
>>>> The same applies to  warn_report("vfio-ccw requires PFCH flag set") (that is,
>>>> on which device (that has no force-orb-pfch=on specified)  is the guest issuing
>>>> ORBs with the PFCH unset), or?
>>>> Should I go for
>>>> "vfio-ccw (xx.xx.xxxx): vfio-ccw requires PFCH flag set"
>>>> and
>>>> "vfio-ccw (xx.xx.xxxx): PFCH flag forced"
>>>> or just for the second one, or some third option?
>>>
>>> Yes, it makes sense for both.
>>>
>>> Related: Do we expect the guest driver to learn from its experience and
>>> not try without pfch again? It is probably not very helpful if the logs
>>> get filled with a lot of "vfio-ccw requires pfch" messages...
>>>    
>>
>> Don't really know. Dong Jia is probably more qualified to answer that question.
>> I don't expect the guest driver to do so. There are probably more intelligent
>> strategies to deal with this, but the question is what do we gain in the end
>> (linux guests are not affected). We should probably not overthink this.
> 
> So, print both messages just once per device?
> 

We can do that. I will morph warned_force_orb_pfch to warned_orb_pfch. That
way we can get away with one boolean, as the both cases are mutually exclusive.

Regards,
Halil

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

[Cornelia Huck]

On Thu, 24 May 2018 12:29:01 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/24/2018 09:16 AM, Cornelia Huck wrote:
> > On Wed, 23 May 2018 19:28:31 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> On 05/23/2018 06:59 PM, Cornelia Huck wrote:  
> >>> On Wed, 23 May 2018 18:23:44 +0200
> >>> Halil Pasic <pasic@linux.ibm.com> wrote:
> >>>      
> >>>> On 05/23/2018 04:46 PM, Cornelia Huck wrote:  
> >>>>>>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> >>>>>>>> +        if (!(vcdev->force_orb_pfch)) {
> >>>>>>>> +            warn_report("vfio-ccw requires PFCH flag set");
> >>>>>>>> +            sch_gen_unit_exception(sch);
> >>>>>>>> +            css_inject_io_interrupt(sch);
> >>>>>>>> +            return IOINST_CC_EXPECTED;
> >>>>>>>> +        } else {
> >>>>>>>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> >>>>>>>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");  
> >>>>>>> This message should probably mention vfio-ccw as well as the subchannel
> >>>>>>> id?
> >>>>>>>            
> >>>>>> I was thinking about this. I think all it would make sense to have a common
> >>>>>> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
> >>>>>> is a separate patch.
> >>>>>>
> >>>>>> Maybe something like:
> >>>>>> vfio-ccw (xx.xx.xxxx): specific message
> >>>>>>
> >>>>>> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
> >>>>>> AFAIR the error_setg captures context (like, src, line, func) but does not
> >>>>>> necessarily report it. Another question is if this should be extended to
> >>>>>> hw/s390x/s390-ccw.c
> >>>>>>
> >>>>>> What do you think?  
> >>>>> I'm not sure that makes sense, especially as not everything might
> >>>>> explicitly refer to a certain subchannel.
> >>>>>
> >>>>> Let's just add the subchannel id here? In this case, this is really a
> >>>>> useful piece of information (which device is showing this behaviour?)
> >>>>>         
> >>>>
> >>>> The same applies to  warn_report("vfio-ccw requires PFCH flag set") (that is,
> >>>> on which device (that has no force-orb-pfch=on specified)  is the guest issuing
> >>>> ORBs with the PFCH unset), or?
> >>>> Should I go for
> >>>> "vfio-ccw (xx.xx.xxxx): vfio-ccw requires PFCH flag set"
> >>>> and
> >>>> "vfio-ccw (xx.xx.xxxx): PFCH flag forced"
> >>>> or just for the second one, or some third option?  
> >>>
> >>> Yes, it makes sense for both.
> >>>
> >>> Related: Do we expect the guest driver to learn from its experience and
> >>> not try without pfch again? It is probably not very helpful if the logs
> >>> get filled with a lot of "vfio-ccw requires pfch" messages...
> >>>      
> >>
> >> Don't really know. Dong Jia is probably more qualified to answer that question.
> >> I don't expect the guest driver to do so. There are probably more intelligent
> >> strategies to deal with this, but the question is what do we gain in the end
> >> (linux guests are not affected). We should probably not overthink this.  
> > 
> > So, print both messages just once per device?
> >   
> 
> We can do that. I will morph warned_force_orb_pfch to warned_orb_pfch. That
> way we can get away with one boolean, as the both cases are mutually exclusive.

Sounds good!

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

[Halil Pasic]

On 05/24/2018 12:29 PM, Halil Pasic wrote:
> 
> 
> On 05/24/2018 09:16 AM, Cornelia Huck wrote:
>> On Wed, 23 May 2018 19:28:31 +0200
>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>
>>> On 05/23/2018 06:59 PM, Cornelia Huck wrote:
>>>> On Wed, 23 May 2018 18:23:44 +0200
>>>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>>>> On 05/23/2018 04:46 PM, Cornelia Huck wrote:
>>>>>>>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
>>>>>>>>> +        if (!(vcdev->force_orb_pfch)) {
>>>>>>>>> +            warn_report("vfio-ccw requires PFCH flag set");
>>>>>>>>> +            sch_gen_unit_exception(sch);
>>>>>>>>> +            css_inject_io_interrupt(sch);
>>>>>>>>> +            return IOINST_CC_EXPECTED;
>>>>>>>>> +        } else {
>>>>>>>>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>>>>>>>>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");
>>>>>>>> This message should probably mention vfio-ccw as well as the subchannel
>>>>>>>> id?
>>>>>>> I was thinking about this. I think all it would make sense to have a common
>>>>>>> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
>>>>>>> is a separate patch.
>>>>>>>
>>>>>>> Maybe something like:
>>>>>>> vfio-ccw (xx.xx.xxxx): specific message
>>>>>>>
>>>>>>> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
>>>>>>> AFAIR the error_setg captures context (like, src, line, func) but does not
>>>>>>> necessarily report it. Another question is if this should be extended to
>>>>>>> hw/s390x/s390-ccw.c
>>>>>>>
>>>>>>> What do you think?
>>>>>> I'm not sure that makes sense, especially as not everything might
>>>>>> explicitly refer to a certain subchannel.
>>>>>>
>>>>>> Let's just add the subchannel id here? In this case, this is really a
>>>>>> useful piece of information (which device is showing this behaviour?)

I'm doing the changes right now. And while doing it occurred to to me that
a device number is probably preferable over the subchannel id, ie. cssid.ssid.devno
is probably better that cssid.ssid.schid. What we really want to tell is,
which device is affected and not over which subchannel is this device talking.

Agree, disagree?

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

[Cornelia Huck]

On Thu, 24 May 2018 17:42:38 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/24/2018 12:29 PM, Halil Pasic wrote:
> > 
> > 
> > On 05/24/2018 09:16 AM, Cornelia Huck wrote:  
> >> On Wed, 23 May 2018 19:28:31 +0200
> >> Halil Pasic <pasic@linux.ibm.com> wrote:
> >>  
> >>> On 05/23/2018 06:59 PM, Cornelia Huck wrote:  
> >>>> On Wed, 23 May 2018 18:23:44 +0200
> >>>> Halil Pasic <pasic@linux.ibm.com> wrote:  
> >>>>> On 05/23/2018 04:46 PM, Cornelia Huck wrote:  
> >>>>>>>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> >>>>>>>>> +        if (!(vcdev->force_orb_pfch)) {
> >>>>>>>>> +            warn_report("vfio-ccw requires PFCH flag set");
> >>>>>>>>> +            sch_gen_unit_exception(sch);
> >>>>>>>>> +            css_inject_io_interrupt(sch);
> >>>>>>>>> +            return IOINST_CC_EXPECTED;
> >>>>>>>>> +        } else {
> >>>>>>>>> +            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> >>>>>>>>> +            WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag forced");  
> >>>>>>>> This message should probably mention vfio-ccw as well as the subchannel
> >>>>>>>> id?  
> >>>>>>> I was thinking about this. I think all it would make sense to have a common
> >>>>>>> prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
> >>>>>>> is a separate patch.
> >>>>>>>
> >>>>>>> Maybe something like:
> >>>>>>> vfio-ccw (xx.xx.xxxx): specific message
> >>>>>>>
> >>>>>>> OTOH we don't seem to do that elsewhere (git grep -e 'warn\|error_report\|error_setg' -- hw/s390x/).
> >>>>>>> AFAIR the error_setg captures context (like, src, line, func) but does not
> >>>>>>> necessarily report it. Another question is if this should be extended to
> >>>>>>> hw/s390x/s390-ccw.c
> >>>>>>>
> >>>>>>> What do you think?  
> >>>>>> I'm not sure that makes sense, especially as not everything might
> >>>>>> explicitly refer to a certain subchannel.
> >>>>>>
> >>>>>> Let's just add the subchannel id here? In this case, this is really a
> >>>>>> useful piece of information (which device is showing this behaviour?)  
> 
> I'm doing the changes right now. And while doing it occurred to to me that
> a device number is probably preferable over the subchannel id, ie. cssid.ssid.devno
> is probably better that cssid.ssid.schid. What we really want to tell is,
> which device is affected and not over which subchannel is this device talking.
> 
> Agree, disagree?

A good argument can be made for both cases: While the admin may care
about the device, we work on the subchannel level. So choose whatever
you think makes most sense, but label it clearly :)