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

["Jason J. Herne" <jjherne@linux.ibm.com>]

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?

-- 
-- Jason J. Herne (jjherne@linux.ibm.com)