Re: [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2994 bytes --]

On 7/22/20 8:50 AM, Christian Borntraeger wrote:
> 
> 
> On 15.07.20 11:40, Janosch Frank wrote:
>> The two main types of zipl component entries are execute and
>> load/data. The last member of the component entry struct therefore
>> denotes either a PSW or an address. Let's make this a bit more clear
>> by introducing a union and cleaning up the code that uses that struct
>> member.
>>
>> The execute type 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.
> 
> If zipl actually specifies a PSW, shouldnt we actually USE that PSW including
> the zipl specified mask?

I expected the current approach to have some kind of meaning behind it,
if there isn't I'll be the first one to make it more sensible.

>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/bootmap.c | 5 +++--
>>  pc-bios/s390-ccw/bootmap.h | 7 ++++++-
>>  2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 97205674e5..8747c4ea26 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->compdat.load_addr;
>>  
>>      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->compdat.load_psw & PSW_MASK_SHORT_ADDR);
>>  }
>>  
>>  static void ipl_scsi(void)
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index 12a0166aae..3946aa3f8d 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -64,11 +64,16 @@ typedef struct BootMapTable {
>>      BootMapPointer entry[];
>>  } __attribute__ ((packed)) BootMapTable;
>>  
>> +typedef union ComponentEntryData {
>> +    uint64_t load_psw;
>> +    uint64_t load_addr;
>> +} ComponentEntryData;
>> +
>>  typedef struct ComponentEntry {
>>      ScsiBlockPtr data;
>>      uint8_t pad[7];
>>      uint8_t component_type;
>> -    uint64_t load_address;
>> +    ComponentEntryData compdat;
>>  } __attribute((packed)) ComponentEntry;
>>  
>>  typedef struct ComponentHeader {
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]