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

[Qemu-devel] [PATCH v3 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  | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 12 deletions(-)

-- 
2.16.3

[Qemu-devel] [PATCH v3 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 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 setting the P bit, if the user knows this
is safe.  For self modifying channel programs forcing the P bit is not
safe.  If the P bit is forced for a self modifying channel program things
are expected to break in strange ways.

Let's also avoid warning multiple about P bit not set in the ORB in case
P bit is not told to be forced, and designate the affected vfio-ccw
device.

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>
---

@Jason:
I have kept the tags this time without consulting you because
all that changed is the logging. Scream if that's not OK with you.

v2->v3:
* tweak commit message (Connie)
* designate subsystem (vfio-ccw) and devno in the log messages (Connie)
* turn WARN_ONCE macro into inline function

---
 hw/s390x/css.c |  3 +--
 hw/vfio/ccw.c  | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 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..ebf471a310 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -32,8 +32,30 @@ typedef struct VFIOCCWDevice {
     uint64_t io_region_offset;
     struct ccw_io_region *io_region;
     EventNotifier io_notifier;
+    bool force_orb_pfch;
+    bool warned_orb_pfch;
 } VFIOCCWDevice;
 
+static inline void warn_once(bool *warned, const char *fmt, ...)
+{
+    va_list ap;
+
+    if (!warned || *warned) {
+        return;
+    }
+    *warned = true;
+    va_start(ap, fmt);
+    warn_vreport(fmt, ap);
+    va_end(ap);
+}
+
+static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
+                                  const char *msg)
+{
+    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
+              sch->cssid, sch->ssid, sch->devno, msg);
+}
+
 static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
 {
     vdev->needs_reset = false;
@@ -54,6 +76,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_once_pfch(vcdev, sch, "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_pfch(vcdev, sch, "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 +463,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 v3 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>
---
 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 v3 1/2] vfio-ccw: add force unlimited prefetch property

[Cornelia Huck]

On Thu, 24 May 2018 19:58:27 +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 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 setting the P bit, if the user knows this
> is safe.  For self modifying channel programs forcing the P bit is not
> safe.  If the P bit is forced for a self modifying channel program things
> are expected to break in strange ways.
> 
> Let's also avoid warning multiple about P bit not set in the ORB in case
> P bit is not told to be forced, and designate the affected vfio-ccw
> device.
> 
> 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>

> +static inline void warn_once(bool *warned, const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    if (!warned || *warned) {
> +        return;
> +    }
> +    *warned = true;
> +    va_start(ap, fmt);
> +    warn_vreport(fmt, ap);
> +    va_end(ap);
> +}
> +
> +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
> +                                  const char *msg)
> +{
> +    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
> +              sch->cssid, sch->ssid, sch->devno, msg);
> +}
> +

While I still think we want warn_once() in common error handling code,
this looks reasonable enough. We can still move it later.

>  static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
>  {
>      vdev->needs_reset = false;
> @@ -54,6 +76,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_once_pfch(vcdev, sch, "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_pfch(vcdev, sch, "PFCH flag forced");
> +        }
> +    }
> +

Looks good to me. I plan to queue this (and the other patch) to
s390-next, but (as always) further tags are still welcome :)

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

[Halil Pasic]

On 05/25/2018 11:40 AM, Cornelia Huck wrote:
> On Thu, 24 May 2018 19:58:27 +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.

[..]

>> +static inline void warn_once(bool *warned, const char *fmt, ...)
>> +{
>> +    va_list ap;
>> +
>> +    if (!warned || *warned) {
>> +        return;
>> +    }
>> +    *warned = true;
>> +    va_start(ap, fmt);
>> +    warn_vreport(fmt, ap);
>> +    va_end(ap);
>> +}
>> +
>> +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
>> +                                  const char *msg)
>> +{
>> +    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
>> +              sch->cssid, sch->ssid, sch->devno, msg);
>> +}
>> +
> 
> While I still think we want warn_once() in common error handling code,
> this looks reasonable enough. We can still move it later.
>

I agree. I was a bit surprised to not find any warn_once functionality.
Let see what Markus says.
  
>>   static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
>>   {
>>       vdev->needs_reset = false;
>> @@ -54,6 +76,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_once_pfch(vcdev, sch, "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_pfch(vcdev, sch, "PFCH flag forced");
>> +        }
>> +    }
>> +
> 
> Looks good to me. I plan to queue this (and the other patch) to
> s390-next, but (as always) further tags are still welcome :)
> 

Thanks!

Halil

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

[Eric Blake]

On 05/25/2018 12:46 PM, Halil Pasic wrote:

>>> +static inline void warn_once(bool *warned, const char *fmt, ...)
>>> +{
>>> +    va_list ap;
>>> +
>>> +    if (!warned || *warned) {
>>> +        return;
>>> +    }
>>> +    *warned = true;
>>> +    va_start(ap, fmt);
>>> +    warn_vreport(fmt, ap);
>>> +    va_end(ap);
>>> +}
>>> +
>>> +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
>>> +                                  const char *msg)
>>> +{
>>> +    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): 
>>> %s",
>>> +              sch->cssid, sch->ssid, sch->devno, msg);
>>> +}
>>> +
>>
>> While I still think we want warn_once() in common error handling code,
>> this looks reasonable enough. We can still move it later.
>>
> 
> I agree. I was a bit surprised to not find any warn_once functionality.

That has been proposed:
https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05457.html

> Let see what Markus says.

Yes, it's still pending review.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Halil Pasic]

On 05/25/2018 08:08 PM, Eric Blake wrote:
> On 05/25/2018 12:46 PM, Halil Pasic wrote:
> 
>>>> +static inline void warn_once(bool *warned, const char *fmt, ...)
>>>> +{
>>>> +    va_list ap;
>>>> +
>>>> +    if (!warned || *warned) {
>>>> +        return;
>>>> +    }
>>>> +    *warned = true;
>>>> +    va_start(ap, fmt);
>>>> +    warn_vreport(fmt, ap);
>>>> +    va_end(ap);
>>>> +}
>>>> +
>>>> +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
>>>> +                                  const char *msg)
>>>> +{
>>>> +    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
>>>> +              sch->cssid, sch->ssid, sch->devno, msg);
>>>> +}
>>>> +
>>>
>>> While I still think we want warn_once() in common error handling code,
>>> this looks reasonable enough. We can still move it later.
>>>
>>
>> I agree. I was a bit surprised to not find any warn_once functionality.
> 
> That has been proposed:
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05457.html
> 

Thanks! I was not aware of this. If we were to use that we would
AFAIU have to transition to once-once instead of once per device
as done in my patch based on Connie's request.

Regards,
Halil

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

[Cornelia Huck]

On Thu, 24 May 2018 19:58:27 +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 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 setting the P bit, if the user knows this
> is safe.  For self modifying channel programs forcing the P bit is not
> safe.  If the P bit is forced for a self modifying channel program things
> are expected to break in strange ways.
> 
> Let's also avoid warning multiple about P bit not set in the ORB in case
> P bit is not told to be forced, and designate the affected vfio-ccw
> device.
> 
> 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>
> ---
> 
> @Jason:
> I have kept the tags this time without consulting you because
> all that changed is the logging. Scream if that's not OK with you.
> 
> v2->v3:
> * tweak commit message (Connie)
> * designate subsystem (vfio-ccw) and devno in the log messages (Connie)
> * turn WARN_ONCE macro into inline function
> 
> ---
>  hw/s390x/css.c |  3 +--
>  hw/vfio/ccw.c  | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)

How shall we proceed with this? I currently don't have much
(understatement of the year...) queued, so we could wait until we
hashed out where to go with _once logging (although the discussion
seems to have died down a bit). If I merge a respin of David's tcg
patches, however, I'd be inclined to merge this in the current form as
well and transition to a common _once handling later.

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

[Halil Pasic]

On 06/05/2018 12:14 PM, Cornelia Huck wrote:
> On Thu, 24 May 2018 19:58:27 +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 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 setting the P bit, if the user knows this
>> is safe.  For self modifying channel programs forcing the P bit is not
>> safe.  If the P bit is forced for a self modifying channel program things
>> are expected to break in strange ways.
>>
>> Let's also avoid warning multiple about P bit not set in the ORB in case
>> P bit is not told to be forced, and designate the affected vfio-ccw
>> device.
>>
>> 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>
>> ---
>>
>> @Jason:
>> I have kept the tags this time without consulting you because
>> all that changed is the logging. Scream if that's not OK with you.
>>
>> v2->v3:
>> * tweak commit message (Connie)
>> * designate subsystem (vfio-ccw) and devno in the log messages (Connie)
>> * turn WARN_ONCE macro into inline function
>>
>> ---
>>   hw/s390x/css.c |  3 +--
>>   hw/vfio/ccw.c  | 35 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 36 insertions(+), 2 deletions(-)
> 
> How shall we proceed with this? I currently don't have much
> (understatement of the year...) queued, so we could wait until we
> hashed out where to go with _once logging (although the discussion
> seems to have died down a bit). If I merge a respin of David's tcg
> patches, however, I'd be inclined to merge this in the current form as
> well and transition to a common _once handling later.
> 

I'd prefer going ahead with this as is, and eventually transitioning
to common infrastructure when it's there.

Regards,
Halil

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

[Cornelia Huck]

On Thu, 24 May 2018 19:58:26 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> 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  | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 12 deletions(-)
> 

Thanks, applied.

We can do any changeover to a common _once reporting infrastructure on
top.