On Mon, 14 May 2018 16:22:30 +0200
Halil Pasic <pa...@linux.ibm.com> wrote:
> On 05/14/2018 03:45 PM, Cornelia Huck wrote:
> > On Mon, 14 May 2018 14:40:13 +0200
> > Halil Pasic <pa...@linux.ibm.com> wrote:
> >
> >> On 05/14/2018 02:18 PM, Cornelia Huck wrote:
> >>> On Thu, 10 May 2018 02:07:11 +0200
> >>> Halil Pasic <pa...@linux.ibm.com> wrote:
> >>>
> >>>> There is at least one control program (guest) that although it does not
> >>>
> >>> I'd drop 'control program' here as well, as it probably confuses more
> >>> than helps.
> >>>
> >>
> >> Will do (everywhere).
> >>
> >>>> rely on the guarantees provided by ORB 1 word 9 bit (aka unlimited
> >>>> prefetch, aka P bit) not being set, fails to tell this to the machine.
> >>>>
> >>>> Usually this ain't a big deal, as the story is usually about performance
> >>>> optimizations only. But vfio-ccw can not provide the guarantees required
> >>>> if the bit is not set.
> >>>
> >>> Isn't that also about channel program rewriting? Or am I mixing things
> >>> up?
> >>>
> >>
> >> I don't understand the question. Can you rephrase it (maybe with more
> >> details)?
> >
> > 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.
>
> >>
> >>>>
> >>>> 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.
>
> >>
> >>>>
> >>>> Signed-off-by: Halil Pasic <pa...@linux.ibm.com>
> >>>> Suggested-by: Dong Jia Shi <bjsdj...@linux.ibm.com>
> >>>> Acked-by: Jason J. Herne <jjhe...@linux.ibm.com>
> >>>> Tested-by: Jason J. Herne <jjhe...@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");
> >>>
> >>> 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.
>
> >>>> 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?
>
> >>
> >>>> } 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.
>
> >
> > 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'.
>
> 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.
