vfio-ccw: loosen orb flags checks
cover vfio-ccw: loosen orb flags checks
See the individual patches. The second one makes even more sense with the corresponding kernel patch.
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 | 13 +++++++++++++ 2 files changed, 13 insertions(+), 12 deletions(-)
Signature · 1 line
2.16.3
PATCH 1/2 vfio-ccw: add force unlimited prefetch property
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.
--- hw/s390x/css.c | 3 +-- hw/vfio/ccw.c | 13 +++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-)
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" );sch_gen_unit_exception (sch);css_inject_io_interrupt (sch);
hw/vfio/ccw.c +13 -0
@@ -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 (), };
Signature · 1 line
2.16.3
Re: [Qemu-devel] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property
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
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
-0sch_gen_unit_exception (sch);css_inject_io_interrupt (sch);hw/vfio/ccw.c +2
-0@@ -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
-0int 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 :)
hw/vfio/ccw.c static void vfio_ccw_unrealize(DeviceState *dev, Error **errp) · line 444
Quoted 9 lines
hw/vfio/ccw.c +2
-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)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
-0DEFINE_PROP_END_OF_LIST(), };
Re: [Qemu-devel] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property
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?
I don't understand the question. Can you rephrase it (maybe with more details)?
Quoted 9 lines
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"?
That is the idea, although I'm not sure what 'the setting' is.
Quoted 25 lines
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
-0hw/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
-0sch_gen_unit_exception (sch);css_inject_io_interrupt (sch);hw/vfio/ccw.c +2
-0@@ -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 32 lines
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
-0int 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 6 lines
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)
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
-0static 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
Quoted 5 lines
Quoted 3 lines
hw/vfio/ccw.c +0
-0DEFINE_PROP_END_OF_LIST(), };
Re: [Qemu-devel] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property
Quoted 19 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 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 6 lines
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)?
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 42 lines
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
-0hw/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
-0sch_gen_unit_exception (sch);css_inject_io_interrupt (sch);hw/vfio/ccw.c +2
-0@@ -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
-0int 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
-0static 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)?
Re: [Qemu-devel] [PATCH 1/2] vfio-ccw: add force unlimited prefetch 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 6 lines
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 9 lines
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'.
:)
Would you like your rephrasing somehow included in the commit message or are we fine as is?
Quoted 11 lines
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 15 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
hw/s390x/css.c +0
-0@@ -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 16 lines
Quoted 16 lines
Quoted 15 lines
hw/vfio/ccw.c typedef struct VFIOCCWDevice { · line 36
Quoted 12 lines
hw/s390x/css.c +0
-0sch_gen_unit_exception (sch);css_inject_io_interrupt (sch);hw/vfio/ccw.c +2
-0@@ -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 7 lines
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)?
I like upfch more as it really not about forcing any prefetch, but allowing *unlimited* prefetch for the channel program.
Quoted 33 lines
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
-0int 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 8 lines
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.
I will do as requested.
Quoted 18 lines
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
-0static 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.
Quoted 7 lines
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.
Quoted 4 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?
Regards,
Halil
Re: [Qemu-devel] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property
Quoted 37 lines · On Mon, 14 May 2018 16:22:30 +0200 Halil Pasic <[email protected]> wrote:
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.)
Quoted 8 lines
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.
Quoted 14 lines
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 7 lines
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?
It probably doesn't hurt.
Quoted 28 lines
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@@ -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 2 lines
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.
Quoted 21 lines
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
-0sch_gen_unit_exception (sch);css_inject_io_interrupt (sch);hw/vfio/ccw.c +2
-0@@ -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 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 :(
Any other suggestions?
Quoted 22 lines
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 7 lines
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.
Yes, the only one who might be able to distinguish the device types is the host admin. So it's probably ok.
Quoted 9 lines
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.
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 6 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.
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.
Re: [Qemu-devel] [qemu-s390x] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property
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 13 lines
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 9 lines
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?
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 10 lines
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 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).
Regards,
Halil
Re: [Qemu-devel] [qemu-s390x] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property
Quoted 34 lines · On Wed, 16 May 2018 18:42:01 +0200 Halil Pasic <[email protected]> wrote:
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 6 lines
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.
force-orb-pfch looks reasonable to me.
Quoted 3 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.
Quoted 49 lines
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.
Quoted 5 lines
So I do see the benefit now. But then I wonder if it should be a
WARN_ONCEtype 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).
A WARN_ONCE (maybe per device) would be the sanest option, I think.
Re: [Qemu-devel] [qemu-s390x] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property
Quoted 11 lines · Cornelia Huck
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.
I'm afraid you misunderstood me. I was thinking manual type documentation. AFAIK the property would provide what was called online help if my memory does not fail me (I mean in-application help like -h or context-sensitive help) only. That is, it ain't used to generate a portion of the manual.
Anyway, I agree, for this particular property we can live with only people that are not afraid of examining the code having enough info to use it.
I will prepare a v2 tomorrow based the proceedings of the discussion here.
Thank you very much for your patience.
Regards,
Halil
PATCH 2/2 vfio-ccw: remove orb.c64 (64 bit data addresses) check
In reply to Halil PasicThe 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.
--- hw/s390x/css.c | 11 ----------- 1 file changed, 11 deletions(-)
hw/s390x/css.c +0 -11
@@ -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); }
Signature · 1 line
2.16.3
Re: [Qemu-devel] [PATCH 2/2] vfio-ccw: remove orb.c64 (64 bit data addresses) check
Quoted 5 lines · On Thu, 10 May 2018 02:07:12 +0200 Halil Pasic <[email protected]> wrote:
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.
Quoted 13 lines
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
hw/s390x/css.c static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) · line 1200
Quoted 10 lines
hw/s390x/css.c +0
-7@@ -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 :)
Quoted 7 lines
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); }