Re: [Qemu-devel] [qemu-s390x] [PATCH v3 16/16] s390-bios: dasd-ipl: Use control unit type to customize error data

[Eric Farman <farman@linux.ibm.com>]

On 03/07/2019 09:38 AM, Jason J. Herne wrote:
> On 3/4/19 12:02 PM, Eric Farman wrote:
>>
>>
>> On 03/01/2019 01:59 PM, Jason J. Herne wrote:
>>> Propagate control unit type from main through the dasd ipl call chain.
>>> The control unit type can be used to determine if we are attempting to
>>> boot from a real dasd device. If something goes wrong we'll want to 
>>> print
>>> detailed dasd sense data (for diagnostic use) but only if we're 
>>> attempting
>>> to boot from a real dasd device.
>>>
>>> Note: We also query and print the dasd sense data if we fail while
>>> attempting to determine the control unit type. In this case, we don't 
>>> know
>>> if we're dealing with a real dasd device yet, but if we are, then the 
>>> sense
>>> data may be useful for figuring out what went wrong. Since determining
>>> the control unit type is the very first thing we do with any real 
>>> dasd device,
>>> this is our most likely point of failure.
>>>
>>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>>> ---
>>>   pc-bios/s390-ccw/cio.c      | 16 ++++++++++------
>>>   pc-bios/s390-ccw/cio.h      |  6 ++++--
>>>   pc-bios/s390-ccw/dasd-ipl.c | 25 +++++++++++++------------
>>>   pc-bios/s390-ccw/dasd-ipl.h |  2 +-
>>>   pc-bios/s390-ccw/main.c     |  2 +-
>>>   pc-bios/s390-ccw/virtio.c   |  2 +-
>>>   6 files changed, 30 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
>>> index c528bbf..593fb33 100644
>>> --- a/pc-bios/s390-ccw/cio.c
>>> +++ b/pc-bios/s390-ccw/cio.c
>>> @@ -54,14 +54,15 @@ uint16_t cu_type(SubChannelId schid)
>>>       sense_id_ccw.count = sizeof(sense_data);
>>>       sense_id_ccw.flags |= CCW_FLAG_SLI;
>>> -    if (do_cio(schid, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
>>> +    if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), 
>>> CCW_FMT1)) {
>>>           panic("Failed to run SenseID CCw\n");
>>>       }
>>>       return sense_data.cu_type;
>>>   }
>>> -void basic_sense(SubChannelId schid, void *sense_data, uint16_t 
>>> data_size)
>>> +void basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
>>> +                 uint16_t data_size)
>>>   {
>>>       Ccw1 senseCcw;
>>> @@ -69,7 +70,7 @@ void basic_sense(SubChannelId schid, void 
>>> *sense_data, uint16_t data_size)
>>>       senseCcw.cda = ptr2u32(sense_data);
>>>       senseCcw.count = data_size;
>>> -    if (do_cio(schid, ptr2u32(&senseCcw), CCW_FMT1)) {
>>> +    if (do_cio(schid, cutype, ptr2u32(&senseCcw), CCW_FMT1)) {
>>>           panic("Failed to run Basic Sense CCW\n");
>>>       }
>>>   }
>>> @@ -364,7 +365,7 @@ static int __do_cio(SubChannelId schid, uint32_t 
>>> ccw_addr, int fmt, Irb *irb)
>>>    *
>>>    * Returns non-zero on error.
>>>    */
>>> -int do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt)
>>> +int do_cio(SubChannelId schid, uint16_t cutype, uint32_t ccw_addr, 
>>> int fmt)
>>>   {
>>>       Irb irb = {};
>>>       SenseDataEckdDasd sd;
>>
>> Missed this one?  :)
>>
> This field is used in two places. 1) For an ECKD Dasd only path. 2) In 
> an error recovery path where we need to issue a basic sense to clear 
> status but we never examine the data. So I guess having the struct named 
> "SenseDataEckdDasd" and using it in a potentially generic path *might* 
> be slightly misleading, it seems superior to allocating a new chunk of 
> memory we'll never use. I assume this is the issue you are hinting at?
> 

Nope...  I guess my git hunk got cut off...  I get a compile error for 
the basic_sense() call with only three parameters in this function.  (if 
(unit_check(&irb)) ...

diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
index 593fb33fbd..0bf551d313 100644
--- a/pc-bios/s390-ccw/cio.c
+++ b/pc-bios/s390-ccw/cio.c
@@ -393,7 +393,7 @@ int do_cio(SubChannelId schid, uint16_t cutype, 
uint32_t ccw_addr, int fmt)
           */
          if ((unit_check(&irb) || iface_ctrl_check(&irb)) && retries <= 
2) {
              if (unit_check(&irb)) {
-                basic_sense(schid, &sd, sizeof(sd));
+                basic_sense(schid, cutype, &sd, sizeof(sd));
              }
              retries++;
              continue;