[Qemu-devel] [PATCH 2/2] vfio-ccw: remove orb.c64 (64 bit data addresses) check

[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 <***@linux.ibm.com>
Acked-by: Jason J. Herne <***@linux.ibm.com>
Tested-by: Jason J. Herne <***@linux.ibm.com>
---
hw/s390x/css.c | 11 -----------
1 file changed, 11 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 32f1b2820d..1554b4c2f5 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1191,17 +1191,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);
}

[Halil Pasic]

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 <***@linux.ibm.com>
Suggested-by: Dong Jia Shi <***@linux.ibm.com>
Acked-by: Jason J. Herne <***@linux.ibm.com>
Tested-by: Jason J. Herne <***@linux.ibm.com>
---
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");
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;
} 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;
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;
+ }
+
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),
DEFINE_PROP_END_OF_LIST(),
};

[Cornelia Huck]

On Thu, 10 May 2018 02:07:11 +0200

Post by Halil Pasic
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.

Post by Halil Pasic
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?

Post by Halil Pasic
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"?

Post by Halil Pasic
---
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?

Post by Halil Pasic
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 :)

Post by Halil Pasic
} 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...

Post by Halil Pasic
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 :)

Post by Halil Pasic
+
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?

Post by Halil Pasic
DEFINE_PROP_END_OF_LIST(),
};

[Halil Pasic]

Post by Cornelia Huck
On Thu, 10 May 2018 02:07:11 +0200

Post by Halil Pasic
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).

Post by Cornelia Huck

Post by Halil Pasic
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)?

Post by Cornelia Huck

Post by Halil Pasic
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.

Post by Cornelia Huck

Post by Halil Pasic
---
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?

Post by Halil Pasic
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.

Post by Cornelia Huck

Post by Halil Pasic
} 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...

Post by Halil Pasic
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.

Post by Cornelia Huck

Post by Halil Pasic
+
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

Post by Cornelia Huck

Post by Halil Pasic
DEFINE_PROP_END_OF_LIST(),
};

[Cornelia Huck]

On Mon, 14 May 2018 14:40:13 +0200

Post by Halil Pasic

Post by Cornelia Huck
On Thu, 10 May 2018 02:07:11 +0200

Post by Halil Pasic
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).

Post by Cornelia Huck

Post by Halil Pasic
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)?

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

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
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.

Lack of coffee :) I meant 'handling'.

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
---
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?

Post by Halil Pasic
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.

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

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
} 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...

Post by Halil Pasic
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.

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

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
+
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.

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

[Halil Pasic]

Post by Cornelia Huck
On Mon, 14 May 2018 14:40:13 +0200

Post by Halil Pasic

Post by Cornelia Huck
On Thu, 10 May 2018 02:07:11 +0200

Post by Halil Pasic
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).

Post by Cornelia Huck

Post by Halil Pasic
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)?

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.

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
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.

Lack of coffee :) I meant 'handling'.

:)

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

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
---
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?

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.

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
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.

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.

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
} 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...

Post by Halil Pasic
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.

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

I will do as requested.

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
+
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.

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

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.

Post by Cornelia Huck
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?

Regards,
Halil

[Cornelia Huck]

On Mon, 14 May 2018 16:22:30 +0200

Post by Halil Pasic

Post by Cornelia Huck
On Mon, 14 May 2018 14:40:13 +0200

Post by Halil Pasic

Post by Cornelia Huck
On Thu, 10 May 2018 02:07:11 +0200

Post by Halil Pasic
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).

Post by Cornelia Huck

Post by Halil Pasic
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)?

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.

Yes, I think that's the problem here. The suspend flag is used as a
marker 'processing has not progressed here, so we're free to modify
later ccws' and pushed along over time. So we might never actually
suspend in this case.

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
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.

Lack of coffee :) I meant 'handling'.

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

It probably doesn't hurt.

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
---
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?

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.

If the two patches are merged in one go, it does not make sense to
touch it, I agree.

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
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.

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

Any other suggestions?

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
} VFIOCCWDevice;
@@ -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.

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

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.

Yes, the only one who might be able to distinguish the device types is
the host admin. So it's probably ok.

Post by Halil Pasic

Post by Cornelia Huck
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.

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

Post by Halil Pasic
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?

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.

[Halil Pasic]

Post by Cornelia Huck
On Mon, 14 May 2018 16:22:30 +0200

Post by Halil Pasic

Post by Cornelia Huck
On Mon, 14 May 2018 14:40:13 +0200

Post by Halil Pasic

Post by Cornelia Huck
On Thu, 10 May 2018 02:07:11 +0200

[..]

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
+ 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)?

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?

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.

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck

Post by Halil Pasic
} VFIOCCWDevice;
@@ -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),

[..]

Post by Cornelia Huck

Post by Halil Pasic

Post by Cornelia Huck
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.

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

Post by Cornelia Huck

Post by Halil Pasic
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?

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

Regards,
Halil

[Cornelia Huck]

On Thu, 10 May 2018 02:07:12 +0200

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

Yes, it makes sense to defer failing this to the kernel module.

Post by Halil Pasic
---
hw/s390x/css.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 32f1b2820d..1554b4c2f5 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1191,17 +1191,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");

Ah, here you remove the message I complained about in the last patch :)

Post by Halil Pasic
- sch_gen_unit_exception(sch);
- css_inject_io_interrupt(sch);
- return IOINST_CC_EXPECTED;
- }
return s390_ccw_cmd_request(sch);
}