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

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 24 May 2018 17:42:38 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/24/2018 12:29 PM, Halil Pasic wrote:
> > 
> > 
> > On 05/24/2018 09:16 AM, Cornelia Huck wrote:  
> >> On Wed, 23 May 2018 19:28:31 +0200
> >> Halil Pasic <pasic@linux.ibm.com> wrote:
> >>  
> >>> On 05/23/2018 06:59 PM, Cornelia Huck wrote:  
> >>>> On Wed, 23 May 2018 18:23:44 +0200
> >>>> Halil Pasic <pasic@linux.ibm.com> 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?)  
> 
> I'm doing the changes right now. And while doing it occurred to to me that
> a device number is probably preferable over the subchannel id, ie. cssid.ssid.devno
> is probably better that cssid.ssid.schid. What we really want to tell is,
> which device is affected and not over which subchannel is this device talking.
> 
> Agree, disagree?

A good argument can be made for both cases: While the admin may care
about the device, we work on the subchannel level. So choose whatever
you think makes most sense, but label it clearly :)