diff mbox series

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

Message ID 1548768562-20007-13-git-send-email-jjherne@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390: vfio-ccw dasd ipl support | expand

Commit Message

Jason J. Herne Jan. 29, 2019, 1:29 p.m. UTC 
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(-)

Comments

Cornelia Huck Feb. 4, 2019, 11:44 a.m. UTC | #1 
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)
Jason J. Herne Feb. 25, 2019, 1:20 p.m. UTC | #2 
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?
Cornelia Huck Feb. 25, 2019, 5:07 p.m. UTC | #3 
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 mbox series

Patch

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) {