| Message ID | 1548768562-20007-13-git-send-email-jjherne@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390: vfio-ccw dasd ipl support | expand |
On Tue, 29 Jan 2019 08:29:19 -0500 "Jason J. Herne" <jjherne@linux.ibm.com> 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 <jjherne@linux.ibm.com> > --- > 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... > } > > static void vring_init(VRing *vr, VqInfo *info)
On 2/4/19 6:44 AM, Cornelia Huck wrote: > On Tue, 29 Jan 2019 08:29:19 -0500 > "Jason J. Herne" <jjherne@linux.ibm.com> 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 <jjherne@linux.ibm.com> >> --- >> 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?
On Mon, 25 Feb 2019 08:20:04 -0500 "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > On 2/4/19 6:44 AM, Cornelia Huck wrote: > > On Tue, 29 Jan 2019 08:29:19 -0500 > > "Jason J. Herne" <jjherne@linux.ibm.com> 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 <jjherne@linux.ibm.com> > >> --- > >> 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? > If I recall the flow correctly, we have two categories of wanting to do I/O (and needing an enabled subchannel for that): - initial detection, when we do sense id to find out the type (done for every device, until we located the wanted one) - actually setting up etc. that specific device I think two ways make sense: - enable/sense id/disable for every device, enable/do specific stuff/disable for the actual boot device - same as above, but don't disable when you found the device That said, if reordering this is too involved, just keep it as in your patch. It's just a bit odd, not really wrong :)
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); } static void vring_init(VRing *vr, VqInfo *info) @@ -257,7 +244,7 @@ void virtio_setup_ccw(VDev *vdev) vdev->config.blk.blk_size = 0; /* mark "illegal" - setup started... */ vdev->guessed_disk_nature = VIRTIO_GDN_NONE; - run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0); + run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false); switch (vdev->senseid.cu_model) { case VIRTIO_ID_NET: @@ -278,18 +265,19 @@ void virtio_setup_ccw(VDev *vdev) default: panic("Unsupported virtio device\n"); } - IPL_assert(run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size) == 0, - "Could not get block device configuration"); + IPL_assert( + run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size, false) == 0, + "Could not get block device configuration"); /* Feature negotiation */ for (i = 0; i < ARRAY_SIZE(vdev->guest_features); i++) { feats.features = 0; feats.index = i; - rc = run_ccw(vdev, CCW_CMD_READ_FEAT, &feats, sizeof(feats)); + rc = run_ccw(vdev, CCW_CMD_READ_FEAT, &feats, sizeof(feats), false); IPL_assert(rc == 0, "Could not get features bits"); vdev->guest_features[i] &= bswap32(feats.features); feats.features = bswap32(vdev->guest_features[i]); - rc = run_ccw(vdev, CCW_CMD_WRITE_FEAT, &feats, sizeof(feats)); + rc = run_ccw(vdev, CCW_CMD_WRITE_FEAT, &feats, sizeof(feats), false); IPL_assert(rc == 0, "Could not set features bits"); } @@ -306,16 +294,17 @@ void virtio_setup_ccw(VDev *vdev) }; IPL_assert( - run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config, sizeof(config)) == 0, + run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config, sizeof(config), false) == 0, "Could not get block device VQ configuration"); info.num = config.num; vring_init(&vdev->vrings[i], &info); vdev->vrings[i].schid = vdev->schid; - IPL_assert(run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info)) == 0, - "Cannot set VQ info"); + IPL_assert( + run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info), false) == 0, + "Cannot set VQ info"); } IPL_assert( - run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status)) == 0, + run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false) == 0, "Could not write status to host"); } @@ -324,7 +313,8 @@ bool virtio_is_supported(SubChannelId schid) vdev.schid = schid; memset(&vdev.senseid, 0, sizeof(vdev.senseid)); /* run sense id command */ - if (run_ccw(&vdev, CCW_CMD_SENSE_ID, &vdev.senseid, sizeof(vdev.senseid))) { + if (run_ccw(&vdev, CCW_CMD_SENSE_ID, &vdev.senseid, sizeof(vdev.senseid), + true)) { return false; } if (vdev.senseid.cu_type == 0x3832) {
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 <jjherne@linux.ibm.com> --- pc-bios/s390-ccw/virtio.c | 48 +++++++++++++++++++---------------------------- 1 file changed, 19 insertions(+), 29 deletions(-)