Re: [qemu-s390x] [PATCH 12/15] s390-bios: Refactor virtio to run channel programs via cio

[Jason J. Herne]

On 2/4/19 6:44 AM, Cornelia Huck wrote:
> On Tue, 29 Jan 2019 08:29:19 -0500
> "Jason J. Herne" <address@hidden> wrote:
>
> > Now that we have a Channel I/O library let's modify virtio boot code to
> > make use of it for running channel programs.
> >
> > Signed-off-by: Jason J. Herne <address@hidden>
> > ---
> >   pc-bios/s390-ccw/virtio.c | 48 +++++++++++++++++++----------------------------
> >   1 file changed, 19 insertions(+), 29 deletions(-)
> >
> > diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> > index aa9da72..f8d71ed 100644
> > --- a/pc-bios/s390-ccw/virtio.c
> > +++ b/pc-bios/s390-ccw/virtio.c
> > @@ -89,33 +89,20 @@ int drain_irqs(SubChannelId schid)
> >       }
> >   }
> >   
> > -static int run_ccw(VDev *vdev, int cmd, void *ptr, int len)
> > +static int run_ccw(VDev *vdev, int cmd, void *ptr, int len, bool sli)
> >   {
> >       Ccw1 ccw = {};
> > -    CmdOrb orb = {};
> > -    int r;
> > -
> > -    enable_subchannel(vdev->schid);
> > -
> > -    /* start subchannel command */
> > -    orb.fmt = 1;
> > -    orb.cpa = (u32)(long)&ccw;
> > -    orb.lpm = 0x80;
> >   
> >       ccw.cmd_code = cmd;
> >       ccw.cda = (long)ptr;
> >       ccw.count = len;
> >   
> > -    r = ssch(vdev->schid, &orb);
> > -    /*
> > -     * XXX Wait until device is done processing the CCW. For now we can
> > -     *     assume that a simple tsch will have finished the CCW processing,
> > -     *     but the architecture allows for asynchronous operation
> > -     */
> > -    if (!r) {
> > -        r = drain_irqs(vdev->schid);
> > +    if (sli) {
> > +        ccw.flags |= CCW_FLAG_SLI;
> >       }
> > -    return r;
> > +
> > +    enable_subchannel(vdev->schid);
> > +    return do_cio(vdev->schid, ptr2u32(&ccw), CCW_FMT1);
>
> That still has the very odd pattern that you enable the subchannel
> every time you run a channel program...

I originally agreed and removed this. But now that I've gotten around to doing some 
testing I've discovered that  we actually rely on this for one important code path.

Here is the call chain before my patches:
main -> virtio_setup -> find_dev -> virtio_is_supported -> run_ccw

Here is the call chain after my patches:
main -> find_boot_device -> find_subch -> virtio_is_supported -> run_ccw

We end up in the same situation in both scenarios. If we remove the enable_subchannel() 
call from run_ccw() then we end up in run_ccw() for a disabled subchannel.

That said, I can remove the enable_subchannel call from run_ccw and instead add it to 
find_subch() if that seems cleaner to you.

Thoughts?

--
-- Jason J. Herne (address@hidden)