Skip to contentSkip to conversation

vfio-ccw: loosen orb flags checks

PATCH Superseded Halil Pasic 2 participants 14 messages May 22, 2018 – May 24, 2018

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Halil Pasic

author
#0 lore raw

v2 cover vfio-ccw: loosen orb flags checks

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(-)
Signature · 1 line
2.16.3
author
#1 lore raw

PATCH v2 1/2 vfio-ccw: add force unlimited prefetch property

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 <[email protected]>
Suggested-by Dong Jia Shi <[email protected]>
Acked-by Jason J. Herne <[email protected]>
Tested-by Jason J. Herne <[email protected]>

---
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(-)
hw/s390x/css.c +1 -2
index 56c3fa8c89..39ae5bbf7e 100644
@@ -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);
hw/vfio/ccw.c +25 -0
index e67392c5f9..62de4c9710 100644
@@ -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(),
 };
Signature · 1 line
2.16.3
#2 lore raw

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

Quoted 10 lines · On Wed, 23 May 2018 00:16:54 +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 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."

?

Quoted 2 lines

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

s/to set/to be set/

Quoted 2 lines

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/

Quoted 41 lines

program things are expected to break in strange ways.

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

---
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(-)
hw/s390x/css.c +1 -2
index 56c3fa8c89..39ae5bbf7e 100644
@@ -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);
hw/vfio/ccw.c +1 -0
index e67392c5f9..62de4c9710 100644
@@ -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;

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

Quoted 10 lines
hw/vfio/ccw.c +8 -0
+    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.

Quoted 9 lines
hw/vfio/ccw.c +3 -0
+
+
+
 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;

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

Quoted 10 lines
hw/vfio/ccw.c +9 -0
 
+    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?

Quoted 7 lines
hw/vfio/ccw.c +3 -0
+        }
+    }
+
     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));

hw/vfio/ccw.c static void vfio_ccw_unrealize(DeviceState *dev, Error **errp) · line 459

Quoted 7 lines
hw/vfio/ccw.c +1 -0
@@ -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.

author
#3 lore raw

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

Quoted 11 lines · Cornelia Huck
Quoted 8 lines · On Wed, 23 May 2018 00:16:54 +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.

Quoted 6 lines
Quoted 2 lines

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

Quoted 1 line

?

Sounds much better. My sentence is ungrammatical.

Quoted 6 lines
Quoted 3 lines

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

Quoted 5 lines
Quoted 2 lines

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.

Quoted 54 lines
Quoted 41 lines

program things are expected to break in strange ways.

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

---
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(-)
hw/s390x/css.c +1 -2
index 56c3fa8c89..39ae5bbf7e 100644
@@ -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);
hw/vfio/ccw.c +1 -0
index e67392c5f9..62de4c9710 100644
@@ -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;

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

Quoted 10 lines
hw/vfio/ccw.c +8 -0
+    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.

Quoted 3 lines

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.

Quoted 10 lines
Quoted 10 lines
hw/vfio/ccw.c +3 -0
+
+
+
  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;
Quoted 13 lines

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

Quoted 9 lines
hw/vfio/ccw.c +9 -0
+    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?

Quoted 8 lines
Quoted 8 lines
hw/vfio/ccw.c +3 -0
+        }
+    }
+
      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)
Quoted 9 lines

hw/vfio/ccw.c static void vfio_ccw_unrealize(DeviceState *dev, Error **errp) · line 459

Quoted 6 lines
hw/vfio/ccw.c +1 -0
  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

#4 lore raw

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

Quoted 22 lines · On Wed, 23 May 2018 16:31:53 +0200 Halil Pasic <[email protected]> wrote:
Quoted 11 lines · Cornelia Huck
Quoted 8 lines · On Wed, 23 May 2018 00:16:54 +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.

Quoted 6 lines
Quoted 2 lines

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

Quoted 1 line

?

Sounds much better. My sentence is ungrammatical.

Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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

Quoted 38 lines
Quoted 6 lines
Quoted 3 lines

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.

Quoted 29 lines
Quoted 16 lines

program things are expected to break in strange ways.

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 +1 -0
index e67392c5f9..62de4c9710 100644
@@ -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;

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

Quoted 10 lines
hw/vfio/ccw.c +8 -0
+    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.

Quoted 7 lines
Quoted 3 lines

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

Quoted 16 lines

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);
}
Quoted 1 line

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.

Quoted 32 lines
Quoted 11 lines
Quoted 11 lines
hw/vfio/ccw.c +3 -0
+
+
+
  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;
Quoted 13 lines

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

Quoted 9 lines
hw/vfio/ccw.c +9 -0
+    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

Quoted 6 lines

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

author
#5 lore raw

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

Quoted 24 lines · Cornelia Huck
Quoted 24 lines
Quoted 9 lines

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

Quoted 9 lines
hw/vfio/ccw.c +9 -0
+    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");
Quoted 3 lines

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

Quoted 7 lines
Quoted 1 line

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

#6 lore raw

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

Quoted 33 lines · On Wed, 23 May 2018 18:23:44 +0200 Halil Pasic <[email protected]> wrote:
Quoted 24 lines · Cornelia Huck
Quoted 24 lines
Quoted 9 lines

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

Quoted 9 lines
hw/vfio/ccw.c +9 -0
+    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");
Quoted 3 lines

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

Quoted 7 lines
Quoted 1 line

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

Quoted 8 lines

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

author
#7 lore raw

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

Quoted 45 lines · Cornelia Huck
Quoted 33 lines · On Wed, 23 May 2018 18:23:44 +0200 Halil Pasic <[email protected]> wrote:
Quoted 24 lines · Cornelia Huck
Quoted 24 lines
Quoted 9 lines

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

Quoted 9 lines
hw/vfio/ccw.c +9 -0
+    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");
Quoted 3 lines

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

Quoted 7 lines
Quoted 1 line

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

Quoted 8 lines

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?

Quoted 6 lines

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

#8 lore raw

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

Quoted 53 lines · On Wed, 23 May 2018 19:28:31 +0200 Halil Pasic <[email protected]> wrote:
Quoted 45 lines · Cornelia Huck
Quoted 33 lines · On Wed, 23 May 2018 18:23:44 +0200 Halil Pasic <[email protected]> wrote:
Quoted 24 lines · Cornelia Huck
Quoted 24 lines
Quoted 9 lines

Review

Quoted 9 lines
(quoted diff) +9 -0
+    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");
Quoted 3 lines

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

Quoted 7 lines
Quoted 1 line

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

Quoted 8 lines

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?

Quoted 6 lines

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

Quoted 4 lines

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?

author
#9 lore raw

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

Quoted 56 lines · Cornelia Huck
Quoted 53 lines · On Wed, 23 May 2018 19:28:31 +0200 Halil Pasic <[email protected]> wrote:
Quoted 51 lines · Cornelia Huck
Quoted 33 lines · On Wed, 23 May 2018 18:23:44 +0200 Halil Pasic <[email protected]> wrote:
Quoted 24 lines · Cornelia Huck
Quoted 24 lines
Quoted 9 lines

Review

Quoted 9 lines
(quoted diff) +9 -0
+    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");
Quoted 3 lines

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

Quoted 7 lines
Quoted 1 line

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

Quoted 8 lines

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

Quoted 7 lines
Quoted 4 lines

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

#10 lore raw

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

Quoted 62 lines · On Thu, 24 May 2018 12:29:01 +0200 Halil Pasic <[email protected]> wrote:
Quoted 61 lines · Cornelia Huck
Quoted 57 lines · On Wed, 23 May 2018 19:28:31 +0200 Halil Pasic <[email protected]> wrote:
Quoted 45 lines · Cornelia Huck
Quoted 33 lines · On Wed, 23 May 2018 18:23:44 +0200 Halil Pasic <[email protected]> wrote:
Quoted 24 lines · Cornelia Huck
Quoted 24 lines
Quoted 9 lines

Review

Quoted 9 lines
(quoted diff) +9 -0
+    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");
Quoted 3 lines

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

Quoted 7 lines
Quoted 1 line

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

Quoted 8 lines

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?

Quoted 6 lines

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.

Quoted 5 lines
Quoted 2 lines

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!

author
#11 lore raw

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

In reply to Halil Pasic
Quoted 33 lines · Halil Pasic
Quoted 30 lines · Cornelia Huck
Quoted 27 lines · On Wed, 23 May 2018 19:28:31 +0200 Halil Pasic <[email protected]> wrote:
Quoted 26 lines · Cornelia Huck
Quoted 24 lines · On Wed, 23 May 2018 18:23:44 +0200 Halil Pasic <[email protected]> wrote:
Quoted 23 lines · Cornelia Huck
Quoted 23 lines
Quoted 11 lines
Quoted 9 lines

+    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

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line

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?

#12 lore raw

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

Quoted 41 lines · On Thu, 24 May 2018 17:42:38 +0200 Halil Pasic <[email protected]> wrote:
Quoted 33 lines · Halil Pasic
Quoted 30 lines · Cornelia Huck
Quoted 27 lines · On Wed, 23 May 2018 19:28:31 +0200 Halil Pasic <[email protected]> wrote:
Quoted 26 lines · Cornelia Huck
Quoted 24 lines · On Wed, 23 May 2018 18:23:44 +0200 Halil Pasic <[email protected]> wrote:
Quoted 23 lines · Cornelia Huck
Quoted 23 lines
Quoted 11 lines
Quoted 9 lines

+    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

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line

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

Quoted 6 lines

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

author
#13 lore raw

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

In reply to 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 <[email protected]>
Acked-by Jason J. Herne <[email protected]>
Tested-by Jason J. Herne <[email protected]>

---
v1 -> v2: unchanged

---
 hw/s390x/css.c | 11 -----------
 1 file changed, 11 deletions(-)
hw/s390x/css.c +0 -11
index 39ae5bbf7e..5424ea4562 100644
@@ -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);
 }
Signature · 1 line
2.16.3