vfio-ccw: loosen orb flags checks

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 <[email protected]>

Suggested-by Dong Jia Shi <[email protected]>

Acked-by Jason J. Herne <[email protected]>

Tested-by Jason J. Herne <[email protected]>

hw/vfio/ccw.c +10 -0

+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);

static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)

 {
     vdev->needs_reset = false;

hw/vfio/ccw.c +2 -0

@@ -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)) {

Quoted 3 lines · On Thu, 24 May 2018 19:58:27 +0200 Halil Pasic <[email protected]> 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.

Quoted 15 lines

hw/vfio/ccw.c +15 -0

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

hw/vfio/ccw.c typedef struct VFIOCCWDevice { · line 58

Quoted 5 lines

hw/vfio/ccw.c +5 -0

+{
+    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.

Quoted 7 lines

  static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
  {
      vdev->needs_reset = false;

hw/vfio/ccw.c +0 -0

@@ -54,6 +76,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
      struct ccw_io_region *region = vcdev->io_region;
      int ret;
 

hw/vfio/ccw.c static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) · line 90

Quoted 12 lines

hw/vfio/ccw.c +12 -0

+    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 :)

Quoted 16 lines

Quoted 16 lines

hw/vfio/ccw.c +16 -0

+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)
+{

Quoted 9 lines

Review

Quoted 5 lines

+    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x):
%s",
+              sch->cssid, sch->ssid, sch->devno, msg);

(quoted diff) +2 -0

+}
+

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.

Quoted 25 lines · Halil Pasic

Quoted 24 lines

Quoted 10 lines

hw/vfio/ccw.c +10 -0

+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);

hw/vfio/ccw.c typedef struct VFIOCCWDevice { · line 58

Quoted 10 lines

hw/vfio/ccw.c +10 -0

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

Quoted 1 line

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

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 <[email protected]>

Suggested-by Dong Jia Shi <[email protected]>

Acked-by Jason J. Herne <[email protected]>

Tested-by Jason J. Herne <[email protected]>

---

@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(-)

Quoted 34 lines · On Thu, 24 May 2018 19:58:27 +0200 Halil Pasic <[email protected]> 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 <[email protected]>

Suggested-by Dong Jia Shi <[email protected]>

Acked-by Jason J. Herne <[email protected]>

Tested-by Jason J. Herne <[email protected]>

---

@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

Quoted 4 lines

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

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