qemu-s390x
[Top][All Lists]
Advanced

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

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


From: Halil Pasic
Subject: Re: [qemu-s390x] [Qemu-devel] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property
Date: Mon, 14 May 2018 14:40:13 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0



On 05/14/2018 02:18 PM, Cornelia Huck wrote:
On Thu, 10 May 2018 02:07:11 +0200
Halil Pasic <address@hidden> wrote:

There is at least one control program (guest) that although it does not

I'd drop 'control program' here as well, as it probably confuses more
than helps.


Will do (everywhere).

rely on the guarantees provided by ORB 1 word 9 bit (aka unlimited
prefetch, aka P bit) not being set, fails to tell this to the machine.

Usually this ain't a big deal, as the story is usually about performance
optimizations only. But vfio-ccw can not provide the guarantees required
if the bit is not set.

Isn't that also about channel program rewriting? Or am I mixing things
up?


I don't understand the question. Can you rephrase it (maybe with more
details)?


Since it is impossible to implement support for P bit not set (at
impossible least without transitioning to lower level protocols) in
vfio-ccw let's provide a manual override.

Hm... so the basic idea seems to be "we don't support !PFCH, but we
know that the guest will not rely on the guarantees, so we provide the
host admin with a way to override the setting"?


That is the idea, although I'm not sure what 'the setting' is.


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>
---
  hw/s390x/css.c |  3 +--
  hw/vfio/ccw.c  | 13 +++++++++++++
  2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 301bf1772f..32f1b2820d 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1196,8 +1196,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");

Adapt this warning?

          sch_gen_unit_exception(sch);
          css_inject_io_interrupt(sch);
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e67392c5f9..32cf606a71 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -32,6 +32,8 @@ typedef struct VFIOCCWDevice {
      uint64_t io_region_offset;
      struct ccw_io_region *io_region;
      EventNotifier io_notifier;
+    /* force unlimited prefetch */
+    bool f_upfch;

force_unlimited_prefetch? You only use it that often :)


I would have expected complaints for the property name in the
first place. I think we should first find a good name for the
property and then consider the rest.

  } VFIOCCWDevice;
static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
@@ -52,8 +54,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
      S390CCWDevice *cdev = sch->driver_data;
      VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
      struct ccw_io_region *region = vcdev->io_region;
+    bool upfch = sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH;

Frankly, I'd drop that variable...

      int ret;
+ if (!upfch && !vcdev->f_upfch) {
+        warn_report("vfio-ccw requires PFCH flag set");
+        sch_gen_unit_exception(sch);
+        css_inject_io_interrupt(sch);
+        return IOINST_CC_EXPECTED;
+    } else if (!upfch) {
+        sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
+    }

and do

if (!(sch->orb.ctrl0 & ORB_CTR0_MASK_PFCH)) {
   if (!vcdev->f_upfch) {
     ...error...
   } else {
     ...set bit...
   }
}

Avoids discussions around variable naming, as well :)


Seems like more indentation and more lies of code to me, but
no strong feelings. It may be easier to read.

+
      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 +441,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("f-upfch", VFIOCCWDevice, f_upfch, false),

Any particular reason you want to control this on a device-by-device
level?


It seemed natural for me. What are our options here? I don't like
machine property, as it is not a machine thing.

Regards,
Halil

      DEFINE_PROP_END_OF_LIST(),
  };






reply via email to

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