vfio-ccw: loosen orb flags checks

There is at least one control program (guest) 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, 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.

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.

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

hw/s390x/css.c +0 -0

index 301bf1772f..32f1b2820d 100644

hw/vfio/ccw.c +0 -0

     DEFINE_PROP_END_OF_LIST(),
 };
 

Quoted 1 line · On Thu, 10 May 2018 02:07:11 +0200 Halil Pasic <[email protected]> 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.

Quoted 6 lines

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?

Quoted 4 lines

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

Quoted 15 lines

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

hw/s390x/css.c +0 -0

index 301bf1772f..32f1b2820d 100644

hw/s390x/css.c static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) · line 1200

Quoted 7 lines

hw/s390x/css.c +1 -2

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

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

Quoted 12 lines

hw/s390x/css.c +0 -0

          sch_gen_unit_exception(sch);
          css_inject_io_interrupt(sch);

hw/vfio/ccw.c +2 -0

index e67392c5f9..32cf606a71 100644
@@ -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 :)

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

Quoted 8 lines

hw/vfio/ccw.c +1 -0

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

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

Quoted 10 lines

hw/vfio/ccw.c +8 -0

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

Quoted 6 lines

hw/vfio/ccw.c +1 -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 +441,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
 

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

Quoted 3 lines

hw/vfio/ccw.c +1 -0

  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?

Quoted 3 lines

hw/vfio/ccw.c +0 -0

      DEFINE_PROP_END_OF_LIST(),
  };
 

Quoted 8 lines · Cornelia Huck

Quoted 1 line · On Thu, 10 May 2018 02:07:11 +0200 Halil Pasic <[email protected]> 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).

Quoted 7 lines

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 6 lines

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 26 lines

Quoted 16 lines

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

hw/s390x/css.c +0 -0

index 301bf1772f..32f1b2820d 100644

hw/s390x/css.c static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) · line 1200

Quoted 7 lines

hw/s390x/css.c +1 -2

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

Quoted 15 lines

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

Quoted 12 lines

hw/s390x/css.c +0 -0

          sch_gen_unit_exception(sch);
          css_inject_io_interrupt(sch);

hw/vfio/ccw.c +2 -0

index e67392c5f9..32cf606a71 100644
@@ -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.

Quoted 33 lines

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

Quoted 9 lines

hw/vfio/ccw.c +1 -0

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

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

Quoted 10 lines

hw/vfio/ccw.c +8 -0

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

Quoted 2 lines

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.

Quoted 7 lines

Quoted 7 lines

hw/vfio/ccw.c +1 -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 +441,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
 

Quoted 7 lines

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

Quoted 3 lines

hw/vfio/ccw.c +1 -0

  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.

Quoted 23 lines · On Mon, 14 May 2018 14:40:13 +0200 Halil Pasic <[email protected]> wrote:

Quoted 8 lines · Cornelia Huck

Quoted 1 line · On Thu, 10 May 2018 02:07:11 +0200 Halil Pasic <[email protected]> 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).

Quoted 10 lines

Quoted 6 lines

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?

Quoted 2 lines

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

If the caller doesn't allow prefetching, it may manipulate parts of the channel program that have not yet been fetched. For example, setting a suspend flag and manipulating ccws that come after that. (I think the ctc and lcs drivers do something like that, or at least did in the past.)

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Lack of coffee :) I meant 'handling'.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

hw/s390x/css.c +0 -0

index 301bf1772f..32f1b2820d 100644
@@ -1196,8 +1196,7 @@ static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
        * Only support prefetch enable mode.
        * Only support 64bit addressing idal.

Quoted 8 lines

Quoted 8 lines

hw/s390x/css.c static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) · line 1200

Quoted 5 lines

hw/s390x/css.c +1 -2

        */
-    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?

Quoted 16 lines

Quoted 15 lines

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

Quoted 12 lines

hw/s390x/css.c +0 -0

           sch_gen_unit_exception(sch);
           css_inject_io_interrupt(sch);

hw/vfio/ccw.c +2 -0

index e67392c5f9..32cf606a71 100644
@@ -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 :)

Quoted 3 lines

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.

What about 'force_pfch' (at least matches the name of the bit in the code)?

Quoted 33 lines

Quoted 33 lines

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

Quoted 9 lines

hw/vfio/ccw.c +1 -0

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

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

Quoted 10 lines

hw/vfio/ccw.c +8 -0

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

Quoted 5 lines

Quoted 2 lines

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.

Yes, I think this makes it easier to see which branch is taken.

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

hw/vfio/ccw.c +1 -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 +441,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
 

Quoted 7 lines

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

Quoted 3 lines

hw/vfio/ccw.c +1 -0

   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?

Quoted 2 lines

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

On the one hand, we want to accommodate certain guests; on the other hand, the guest is free to address different devices in different ways (although I would expect the difference to be more by different device types).

In the end, it seems that a per-device property is the easiest approach after all. (The admin can probably set this globally, if desired.)

Another thought: Should there be a warning logged somewhere if we actually force pfch (i.e., not just set the property)?

Quoted 26 lines · Cornelia Huck

Quoted 23 lines · On Mon, 14 May 2018 14:40:13 +0200 Halil Pasic <[email protected]> wrote:

Quoted 8 lines · Cornelia Huck

Quoted 1 line · On Thu, 10 May 2018 02:07:11 +0200 Halil Pasic <[email protected]> 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).

Quoted 10 lines

Quoted 6 lines

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?

Quoted 9 lines

Quoted 2 lines

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

If the caller doesn't allow prefetching, it may manipulate parts of the channel program that have not yet been fetched. For example, setting a suspend flag and manipulating ccws that come after that. (I think the ctc and lcs drivers do something like that, or at least did in the past.)

Yes. Sorry I did not understand rewriting. I usually refer to the same as self modifying channel programs. Typical example would be the ccw-IPL scheme.

I think a suspend actually would not hurt us. The driver would issue a RSCH and we can happily translate the new stuff. OTOH if the reads modify the channel program we have no chance to do the translation for the parts of the channel program that were not there at the beginning.

Quoted 14 lines

Quoted 13 lines

Quoted 11 lines

Quoted 6 lines

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.

Quoted 2 lines

Lack of coffee :) I meant 'handling'.

:)

Would you like your rephrasing somehow included in the commit message or are we fine as is?

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

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

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

hw/s390x/css.c +0 -0

index 301bf1772f..32f1b2820d 100644
@@ -1196,8 +1196,7 @@ static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
        * Only support prefetch enable mode.
        * Only support 64bit addressing idal.

Quoted 8 lines

Quoted 8 lines

hw/s390x/css.c static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) · line 1200

Quoted 5 lines

hw/s390x/css.c +1 -2

        */
-    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?

Sorry I forgot this one. I would like to keep it as-is because it's going away with #2 anyway. Introducing a new message seems like counter productive.

Quoted 21 lines

Quoted 20 lines

Quoted 16 lines

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

Quoted 13 lines

hw/s390x/css.c +0 -0

           sch_gen_unit_exception(sch);
           css_inject_io_interrupt(sch);

hw/vfio/ccw.c +2 -0

index e67392c5f9..32cf606a71 100644
@@ -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.

Quoted 3 lines

What about 'force_pfch' (at least matches the name of the bit in the code)?

I like upfch more as it really not about forcing any prefetch, but allowing *unlimited* prefetch for the channel program.

Quoted 22 lines

Quoted 14 lines

Quoted 4 lines

Quoted 3 lines

hw/vfio/ccw.c +0 -0

   } VFIOCCWDevice;

Quoted 9 lines

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

Quoted 5 lines

hw/vfio/ccw.c +1 -0

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

Quoted 2 lines

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

On the one hand, we want to accommodate certain guests; on the other hand, the guest is free to address different devices in different ways (although I would expect the difference to be more by different device types).

Quoted 2 lines

In the end, it seems that a per-device property is the easiest approach after all. (The admin can probably set this globally, if desired.)

I'm pretty sure globally is doable (global driver.prop=value). Also this could be a per device driver thing. In vfio-ccw we dont have stuff like device type modeled. So I think this is really the best we can do.

Quoted 5 lines

Another thought: Should there be a warning logged somewhere if we actually force pfch (i.e., not just set the property)?

I don't think so. With libvirt the cmd line gets logged. So we can tell if the machine was running with this forced or not. This knob is really (an opt-in) for expert users only.

Furthermore a warning about this may not be very constructive, as there is not much that can be done to make the warning go away. IMHO getting used to warnings is not a good thing.

Or am I missing a reason for issuing a warning?

Quoted 7 lines · On Mon, 14 May 2018 16:22:30 +0200 Halil Pasic <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

Quoted 3 lines · On Mon, 14 May 2018 14:40:13 +0200 Halil Pasic <[email protected]> wrote:

Quoted 2 lines · Cornelia Huck

On Thu, 10 May 2018 02:07:11 +0200
Halil Pasic <[email protected]> wrote:

Quoted 13 lines

Quoted 12 lines

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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

What about 'force_pfch' (at least matches the name of the bit in the code)?

Quoted 2 lines

I like upfch more as it really not about forcing any prefetch, but allowing *unlimited* prefetch for the channel program.

'always_allow_prefetch', then? The problem is that we force a flag to be set, which does not force but allow something. Hard to express in a short property name :(

Any other suggestions?

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

  
    } VFIOCCWDevice;

hw/vfio/ccw.c +1 -0

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

Quoted 5 lines

Quoted 4 lines

Another thought: Should there be a warning logged somewhere if we actually force pfch (i.e., not just set the property)?

Quoted 3 lines

I don't think so. With libvirt the cmd line gets logged. So we can tell if the machine was running with this forced or not. This knob is really (an opt-in) for expert users only.

But there's a difference between 'we set this one preemptively' and 'we set it, and the guest actually did a request with pfch off'.

Quoted 5 lines

Furthermore a warning about this may not be very constructive, as there is not much that can be done to make the warning go away. IMHO getting used to warnings is not a good thing.

Quoted 1 line

Or am I missing a reason for issuing a warning?

Just log this once so that the admin sees 'yes, the guest actually did a request with pfch off, so if funny things happened, that might be the reason'. Of course, if this is only an edge use case, that would be overkill.

Quoted 10 lines · Cornelia Huck

Quoted 7 lines · On Mon, 14 May 2018 16:22:30 +0200 Halil Pasic <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

Quoted 3 lines · On Mon, 14 May 2018 14:40:13 +0200 Halil Pasic <[email protected]> wrote:

Quoted 2 lines · Cornelia Huck

On Thu, 10 May 2018 02:07:11 +0200
Halil Pasic <[email protected]> wrote:

[..]

Quoted 20 lines

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 6 lines

Quoted 3 lines

What about 'force_pfch' (at least matches the name of the bit in the code)?

I like upfch more as it really not about forcing any prefetch, but allowing *unlimited* prefetch for the channel program.

'always_allow_prefetch', then? The problem is that we force a flag to be set, which does not force but allow something. Hard to express in a short property name :(

Quoted 2 lines

Any other suggestions?

How about force-orb-pfch or force-orb-pbit (PoP name) for the property and with underscores for the variable. My idea was (starting from your force_pfch) to spell out that we are fiddling with that orb bit.

Can I/do I have to document the property somewhere? Telling the whole story with the name is going to be difficult.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

 
    
    } VFIOCCWDevice;  

(quoted diff) +1 -0

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

[..]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Another thought: Should there be a warning logged somewhere if we actually force pfch (i.e., not just set the property)?

Quoted 7 lines

Quoted 3 lines

I don't think so. With libvirt the cmd line gets logged. So we can tell if the machine was running with this forced or not. This knob is really (an opt-in) for expert users only.

But there's a difference between 'we set this one preemptively' and 'we set it, and the guest actually did a request with pfch off'.

I expect the admin to set it *only* after seeing SSCHs fail with the 'vfio-ccw requires PFCH flag set' message. So no preemptive usage is expected, but...

Quoted 5 lines

Quoted 5 lines

Furthermore a warning about this may not be very constructive, as there is not much that can be done to make the warning go away. IMHO getting used to warnings is not a good thing.

Quoted 7 lines

Quoted 1 line

Or am I missing a reason for issuing a warning?

Just log this once so that the admin sees 'yes, the guest actually did a request with pfch off, so if funny things happened, that might be the reason'. Of course, if this is only an edge use case, that would be overkill.

... if the admin (actually more likely developer/tester) is mistaken about the guest, and it does require the guarantees, but things don't blow up right away, this message, together with the timestamp could help determine why things turned funny.

So I do see the benefit now. But then I wonder if it should be a WARN_ONCE type thing, or if we shall issue a warning each time the override happens. (Considering the laid out expected benefit, if the first request is OK but some subsequent one is not OK (needs the conservative prefetch behavior) we don't gain much).

Quoted 3 lines

How about force-orb-pfch or force-orb-pbit (PoP name) for the property and with underscores for the variable. My idea was (starting from your force_pfch) to spell out that we are fiddling with that orb bit.

force-orb-pfch looks reasonable to me.

Quoted 2 lines

Can I/do I have to document the property somewhere? Telling the whole story with the name is going to be difficult.

The description member would require that you define your own property type IIUC, which I think would be overkill. So I'd add a comment in the source code.

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

---
 hw/s390x/css.c | 11 -----------
 1 file changed, 11 deletions(-)

hw/s390x/css.c +0 -0

index 32f1b2820d..1554b4c2f5 100644

hw/s390x/css.c +0 -4

-        sch_gen_unit_exception(sch);
-        css_inject_io_interrupt(sch);
-        return IOINST_CC_EXPECTED;
-    }
     return s390_ccw_cmd_request(sch);
 }