| Message ID | 20180524175828.3143-2-pasic@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio-ccw: loosen orb flags checks | expand |
On Thu, 24 May 2018 19:58:27 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > There is at least one guest (OS) such that although it does not rely on > the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka > P bit) not being set, it fails to tell this to the machine. > > Usually this ain't a big deal, as the original purpose of the P bit is to > allow for performance optimizations. vfio-ccw however can not provide the > guarantees required if the bit is not set. > > It is not possible to implement support for the P bit not set without > transitioning to lower level protocols for vfio-ccw. So let's give the > user the opportunity to force setting the P bit, if the user knows this > is safe. For self modifying channel programs forcing the P bit is not > safe. If the P bit is forced for a self modifying channel program things > are expected to break in strange ways. > > Let's also avoid warning multiple about P bit not set in the ORB in case > P bit is not told to be forced, and designate the affected vfio-ccw > device. > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > Suggested-by: Dong Jia Shi <bjsdjshi@linux.ibm.com> > Acked-by: Jason J. Herne <jjherne@linux.ibm.com> > Tested-by: Jason J. Herne <jjherne@linux.ibm.com> > +static inline void warn_once(bool *warned, const char *fmt, ...) > +{ > + va_list ap; > + > + if (!warned || *warned) { > + return; > + } > + *warned = true; > + va_start(ap, fmt); > + warn_vreport(fmt, ap); > + va_end(ap); > +} > + > +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, > + const char *msg) > +{ > + warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s", > + sch->cssid, sch->ssid, sch->devno, msg); > +} > + While I still think we want warn_once() in common error handling code, this looks reasonable enough. We can still move it later. > static void vfio_ccw_compute_needs_reset(VFIODevice *vdev) > { > vdev->needs_reset = false; > @@ -54,6 +76,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) > struct ccw_io_region *region = vcdev->io_region; > int ret; > > + if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) { > + if (!(vcdev->force_orb_pfch)) { > + warn_once_pfch(vcdev, sch, "requires PFCH flag set"); > + sch_gen_unit_exception(sch); > + css_inject_io_interrupt(sch); > + return IOINST_CC_EXPECTED; > + } else { > + sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; > + warn_once_pfch(vcdev, sch, "PFCH flag forced"); > + } > + } > + Looks good to me. I plan to queue this (and the other patch) to s390-next, but (as always) further tags are still welcome :)
On 05/25/2018 11:40 AM, Cornelia Huck wrote: > On Thu, 24 May 2018 19:58:27 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > >> There is at least one guest (OS) such that although it does not rely on >> the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka >> P bit) not being set, it fails to tell this to the machine. [..] >> +static inline void warn_once(bool *warned, const char *fmt, ...) >> +{ >> + va_list ap; >> + >> + if (!warned || *warned) { >> + return; >> + } >> + *warned = true; >> + va_start(ap, fmt); >> + warn_vreport(fmt, ap); >> + va_end(ap); >> +} >> + >> +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, >> + const char *msg) >> +{ >> + warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s", >> + sch->cssid, sch->ssid, sch->devno, msg); >> +} >> + > > While I still think we want warn_once() in common error handling code, > this looks reasonable enough. We can still move it later. > I agree. I was a bit surprised to not find any warn_once functionality. Let see what Markus says. >> static void vfio_ccw_compute_needs_reset(VFIODevice *vdev) >> { >> vdev->needs_reset = false; >> @@ -54,6 +76,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) >> struct ccw_io_region *region = vcdev->io_region; >> int ret; >> >> + if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) { >> + if (!(vcdev->force_orb_pfch)) { >> + warn_once_pfch(vcdev, sch, "requires PFCH flag set"); >> + sch_gen_unit_exception(sch); >> + css_inject_io_interrupt(sch); >> + return IOINST_CC_EXPECTED; >> + } else { >> + sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; >> + warn_once_pfch(vcdev, sch, "PFCH flag forced"); >> + } >> + } >> + > > Looks good to me. I plan to queue this (and the other patch) to > s390-next, but (as always) further tags are still welcome :) > Thanks! Halil
On 05/25/2018 12:46 PM, Halil Pasic wrote: >>> +static inline void warn_once(bool *warned, const char *fmt, ...) >>> +{ >>> + va_list ap; >>> + >>> + if (!warned || *warned) { >>> + return; >>> + } >>> + *warned = true; >>> + va_start(ap, fmt); >>> + warn_vreport(fmt, ap); >>> + va_end(ap); >>> +} >>> + >>> +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, >>> + const char *msg) >>> +{ >>> + warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): >>> %s", >>> + sch->cssid, sch->ssid, sch->devno, msg); >>> +} >>> + >> >> While I still think we want warn_once() in common error handling code, >> this looks reasonable enough. We can still move it later. >> > > I agree. I was a bit surprised to not find any warn_once functionality. That has been proposed: https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05457.html > Let see what Markus says. Yes, it's still pending review.
On 05/25/2018 08:08 PM, Eric Blake wrote: > On 05/25/2018 12:46 PM, Halil Pasic wrote: > >>>> +static inline void warn_once(bool *warned, const char *fmt, ...) >>>> +{ >>>> + va_list ap; >>>> + >>>> + if (!warned || *warned) { >>>> + return; >>>> + } >>>> + *warned = true; >>>> + va_start(ap, fmt); >>>> + warn_vreport(fmt, ap); >>>> + va_end(ap); >>>> +} >>>> + >>>> +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, >>>> + const char *msg) >>>> +{ >>>> + warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s", >>>> + sch->cssid, sch->ssid, sch->devno, msg); >>>> +} >>>> + >>> >>> While I still think we want warn_once() in common error handling code, >>> this looks reasonable enough. We can still move it later. >>> >> >> I agree. I was a bit surprised to not find any warn_once functionality. > > That has been proposed: > https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05457.html > Thanks! I was not aware of this. If we were to use that we would AFAIU have to transition to once-once instead of once per device as done in my patch based on Connie's request. Regards, Halil
On Thu, 24 May 2018 19:58:27 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > There is at least one guest (OS) such that although it does not rely on > the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka > P bit) not being set, it fails to tell this to the machine. > > Usually this ain't a big deal, as the original purpose of the P bit is to > allow for performance optimizations. vfio-ccw however can not provide the > guarantees required if the bit is not set. > > It is not possible to implement support for the P bit not set without > transitioning to lower level protocols for vfio-ccw. So let's give the > user the opportunity to force setting the P bit, if the user knows this > is safe. For self modifying channel programs forcing the P bit is not > safe. If the P bit is forced for a self modifying channel program things > are expected to break in strange ways. > > Let's also avoid warning multiple about P bit not set in the ORB in case > P bit is not told to be forced, and designate the affected vfio-ccw > device. > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > Suggested-by: Dong Jia Shi <bjsdjshi@linux.ibm.com> > Acked-by: Jason J. Herne <jjherne@linux.ibm.com> > Tested-by: Jason J. Herne <jjherne@linux.ibm.com> > --- > > @Jason: > I have kept the tags this time without consulting you because > all that changed is the logging. Scream if that's not OK with you. > > v2->v3: > * tweak commit message (Connie) > * designate subsystem (vfio-ccw) and devno in the log messages (Connie) > * turn WARN_ONCE macro into inline function > > --- > hw/s390x/css.c | 3 +-- > hw/vfio/ccw.c | 35 +++++++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+), 2 deletions(-) How shall we proceed with this? I currently don't have much (understatement of the year...) queued, so we could wait until we hashed out where to go with _once logging (although the discussion seems to have died down a bit). If I merge a respin of David's tcg patches, however, I'd be inclined to merge this in the current form as well and transition to a common _once handling later.
On 06/05/2018 12:14 PM, Cornelia Huck wrote: > On Thu, 24 May 2018 19:58:27 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > >> There is at least one guest (OS) such that although it does not rely on >> the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka >> P bit) not being set, it fails to tell this to the machine. >> >> Usually this ain't a big deal, as the original purpose of the P bit is to >> allow for performance optimizations. vfio-ccw however can not provide the >> guarantees required if the bit is not set. >> >> It is not possible to implement support for the P bit not set without >> transitioning to lower level protocols for vfio-ccw. So let's give the >> user the opportunity to force setting the P bit, if the user knows this >> is safe. For self modifying channel programs forcing the P bit is not >> safe. If the P bit is forced for a self modifying channel program things >> are expected to break in strange ways. >> >> Let's also avoid warning multiple about P bit not set in the ORB in case >> P bit is not told to be forced, and designate the affected vfio-ccw >> device. >> >> Signed-off-by: Halil Pasic <pasic@linux.ibm.com> >> Suggested-by: Dong Jia Shi <bjsdjshi@linux.ibm.com> >> Acked-by: Jason J. Herne <jjherne@linux.ibm.com> >> Tested-by: Jason J. Herne <jjherne@linux.ibm.com> >> --- >> >> @Jason: >> I have kept the tags this time without consulting you because >> all that changed is the logging. Scream if that's not OK with you. >> >> v2->v3: >> * tweak commit message (Connie) >> * designate subsystem (vfio-ccw) and devno in the log messages (Connie) >> * turn WARN_ONCE macro into inline function >> >> --- >> hw/s390x/css.c | 3 +-- >> hw/vfio/ccw.c | 35 +++++++++++++++++++++++++++++++++++ >> 2 files changed, 36 insertions(+), 2 deletions(-) > > How shall we proceed with this? I currently don't have much > (understatement of the year...) queued, so we could wait until we > hashed out where to go with _once logging (although the discussion > seems to have died down a bit). If I merge a respin of David's tcg > patches, however, I'd be inclined to merge this in the current form as > well and transition to a common _once handling later. > I'd prefer going ahead with this as is, and eventually transitioning to common infrastructure when it's there. Regards, Halil
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 56c3fa8c89..39ae5bbf7e 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -1204,8 +1204,7 @@ static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) * Only support prefetch enable mode. * Only support 64bit addressing idal. */ - if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) || - !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) { + if (!(orb->ctrl0 & ORB_CTRL0_MASK_C64)) { warn_report("vfio-ccw requires PFCH and C64 flags set"); sch_gen_unit_exception(sch); css_inject_io_interrupt(sch); diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index e67392c5f9..ebf471a310 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -32,8 +32,30 @@ typedef struct VFIOCCWDevice { uint64_t io_region_offset; struct ccw_io_region *io_region; EventNotifier io_notifier; + bool force_orb_pfch; + bool warned_orb_pfch; } VFIOCCWDevice; +static inline void warn_once(bool *warned, const char *fmt, ...) +{ + va_list ap; + + if (!warned || *warned) { + return; + } + *warned = true; + va_start(ap, fmt); + warn_vreport(fmt, ap); + va_end(ap); +} + +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch, + const char *msg) +{ + warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s", + sch->cssid, sch->ssid, sch->devno, msg); +} + static void vfio_ccw_compute_needs_reset(VFIODevice *vdev) { vdev->needs_reset = false; @@ -54,6 +76,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) struct ccw_io_region *region = vcdev->io_region; int ret; + if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) { + if (!(vcdev->force_orb_pfch)) { + warn_once_pfch(vcdev, sch, "requires PFCH flag set"); + sch_gen_unit_exception(sch); + css_inject_io_interrupt(sch); + return IOINST_CC_EXPECTED; + } else { + sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; + warn_once_pfch(vcdev, sch, "PFCH flag forced"); + } + } + 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 +463,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("force-orb-pfch", VFIOCCWDevice, force_orb_pfch, false), DEFINE_PROP_END_OF_LIST(), };