[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Halil Pasic
Subject: Re: [qemu-s390x] [PATCH v2 1/2] vfio-ccw: add force unlimited prefetch property
Date: Wed, 23 May 2018 16:31:53 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 05/23/2018 11:37 AM, Cornelia Huck wrote:
On Wed, 23 May 2018 00:16:54 +0200
Halil Pasic <address@hidden> 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.

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/


program things are expected to break in strange ways.

Signed-off-by: Halil Pasic <address@hidden>
Suggested-by: Dong Jia Shi <address@hidden>
Acked-by: Jason J. Herne <address@hidden>
Tested-by: Jason J. Herne <address@hidden>

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");
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) {
    *warned= true;
    va_start(ap, fmt);
    vreport(REPORT_TYPE_WARNING, fmt, 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

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

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 
static Property vfio_ccw_properties[] = {
      DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev),
+    DEFINE_PROP_BOOL("force-orb-pfch", VFIOCCWDevice, force_orb_pfch, false),

Otherwise, the changes look good.

I guess there will be a v3 then.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]