Re: [PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addresses

[Janosch Frank]

On 6/25/20 2:46 PM, Thomas Huth wrote:
> On 24/06/2020 09.52, Janosch Frank wrote:
>> The component entries written by zipl contain short PSWs, not
>> addresses. Let's mask them and only pass the address part to
>> jump_to_IPL_code(uint64_t address) because it expects an address as
>> visible by the name of the argument.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 5 +++--
>>   pc-bios/s390-ccw/bootmap.h | 2 +-
>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 97205674e5..8547a140df 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -10,6 +10,7 @@
>>   
>>   #include "libc.h"
>>   #include "s390-ccw.h"
>> +#include "s390-arch.h"
>>   #include "bootmap.h"
>>   #include "virtio.h"
>>   #include "bswap.h"
>> @@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry)
>>       char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) 
>> */
>>   
>>       blockno = entry->data.blockno;
>> -    address = entry->load_address;
>> +    address = entry->psw & PSW_MASK_SHORT_ADDR;
> 
> Are you really sure about this one here? The address does not seem to be 
> used for any of the jump_to_IPL() functions. And in the zipl sources, I 
> can also see spots like this:

This one slipped through and is indeed wrong.

> 
>     entry->compdat.load_address = data.load_address;
> 
> ... without any further short mask bits. So I somehow doubt that this 
> change is really ok?
> 
>>       debug_print_int("loading segment at block", blockno);
>>       debug_print_int("addr", address);
>> @@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>>       IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC 
>> entry");
>>   
>>       /* should not return */
>> -    jump_to_IPL_code(entry->load_address);
>> +    jump_to_IPL_code(entry->psw & PSW_MASK_SHORT_ADDR);
> 
> That one should be fine, I think.

Yes, as it is a execute type entry, this needs to be a PSW and therefore
needs to be masked.

> 
>>   }
>>   
>>   static void ipl_scsi(void)
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index 12a0166aae..e07f87e690 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -68,7 +68,7 @@ typedef struct ComponentEntry {
>>       ScsiBlockPtr data;
>>       uint8_t pad[7];
>>       uint8_t component_type;
>> -    uint64_t load_address;
>> +    uint64_t psw;
> 
> I'd recommend to keep the load_address name. It's the same name as used 
> in the zipl sources, and as far as I can see, the field does not always 
> contain a PSW.

The problem is that this is a union in zipl containing an address, psw
or signature header.

I guess we should also make it a union and use the proper members so it
is clear what we retrieve from the entry. If it is a PSW we need to mask
it if it is a component address masking might be a bad idea.

But I absolutely do not want to have this named PSW and then being used
like a normal address. It took me way too long to figure out why my
guest wasn't booting anymore.

Time for a new series of patches :)

> 
>>   } __attribute((packed)) ComponentEntry;
>>   
>>   typedef struct ComponentHeader {
>>
> 
>   Thomas
>

signature.asc
Description: OpenPGP digital signature