Re: [qemu-s390x] [Qemu-devel] [PATCH v2 1/2] vfio-ccw: add force unlimited prefetch property

[Halil Pasic]

On 05/24/2018 09:16 AM, Cornelia Huck wrote:
> On Wed, 23 May 2018 19:28:31 +0200
> Halil Pasic <address@hidden> wrote:
>
> > On 05/23/2018 06:59 PM, Cornelia Huck wrote:
> > > On Wed, 23 May 2018 18:23:44 +0200
> > > Halil Pasic <address@hidden> wrote:
> > >    
> > > > On 05/23/2018 04:46 PM, Cornelia Huck wrote:
> > > > > > > > +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> > > > > > > > +        if (!(vcdev->force_orb_pfch)) {
> > > > > > > > +            warn_report("vfio-ccw 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(vcdev->warned_force_orb_pfch, "PFCH flag forced");
> > > > > > > This message should probably mention vfio-ccw as well as the subchannel
> > > > > > > id?
> > > > > > >          
> > > > > > I was thinking about this. I think all it would make sense to have a common
> > > > > > prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
> > > > > > is a separate patch.
> > > > > >
> > > > > > Maybe something like:
> > > > > > vfio-ccw (xx.xx.xxxx): specific message
> > > > > >
> > > > > > OTOH we don't seem to do that elsewhere (git grep -e 
> > > > > > 'warn\|error_report\|error_setg' -- hw/s390x/).
> > > > > > AFAIR the error_setg captures context (like, src, line, func) but does not
> > > > > > necessarily report it. Another question is if this should be extended to
> > > > > > hw/s390x/s390-ccw.c
> > > > > >
> > > > > > What do you think?
> > > > > I'm not sure that makes sense, especially as not everything might
> > > > > explicitly refer to a certain subchannel.
> > > > >
> > > > > Let's just add the subchannel id here? In this case, this is really a
> > > > > useful piece of information (which device is showing this behaviour?)
> > > > >       
> > > >
> > > > The same applies to  warn_report("vfio-ccw requires PFCH flag set") (that is,
> > > > on which device (that has no force-orb-pfch=on specified)  is the guest issuing
> > > > ORBs with the PFCH unset), or?
> > > > Should I go for
> > > > "vfio-ccw (xx.xx.xxxx): vfio-ccw requires PFCH flag set"
> > > > and
> > > > "vfio-ccw (xx.xx.xxxx): PFCH flag forced"
> > > > or just for the second one, or some third option?
> > >
> > > Yes, it makes sense for both.
> > >
> > > Related: Do we expect the guest driver to learn from its experience and
> > > not try without pfch again? It is probably not very helpful if the logs
> > > get filled with a lot of "vfio-ccw requires pfch" messages...
> > >    
> >
> > Don't really know. Dong Jia is probably more qualified to answer that question.
> > I don't expect the guest driver to do so. There are probably more intelligent
> > strategies to deal with this, but the question is what do we gain in the end
> > (linux guests are not affected). We should probably not overthink this.
>
> So, print both messages just once per device?

We can do that. I will morph warned_force_orb_pfch to warned_orb_pfch. That
way we can get away with one boolean, as the both cases are mutually exclusive.

Regards,
Halil