Re: [PATCH v4 5/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant

[Janosch Frank]

On 6/22/20 1:40 PM, Thomas Huth wrote:
> On 22/06/2020 09.42, Janosch Frank wrote:
>> ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
>> actually 64 bit addressing.
>>
>> As PSW_MASK_64 is now effectively 33 bit long and the PSWLegacy struct
>> has 2 32 bit members, let's also use a unsigned long pointer in
>> dasd-ipl.c instead when oring the constant into a 8 byte PSW.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>>  pc-bios/s390-ccw/dasd-ipl.c  | 5 ++---
>>  pc-bios/s390-ccw/s390-arch.h | 2 +-
>>  2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
>> index 0fc879bb8e..0dbad051a2 100644
>> --- a/pc-bios/s390-ccw/dasd-ipl.c
>> +++ b/pc-bios/s390-ccw/dasd-ipl.c
>> @@ -206,7 +206,7 @@ static void run_ipl2(SubChannelId schid, uint16_t 
>> cutype, uint32_t addr)
>>   */
>>  void dasd_ipl(SubChannelId schid, uint16_t cutype)
>>  {
>> -    PSWLegacy *pswl = (PSWLegacy *) 0x00;
>> +    unsigned long *pswl = 0x0;
> 
> ... or we could use the "lowcore" pointer from s390-arch.h ... though
> that's PSWLegacy again...
> 
>>      uint32_t ipl2_addr;
>>  
>>      /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
>> @@ -229,7 +229,6 @@ void dasd_ipl(SubChannelId schid, uint16_t cutype)
>>      run_ipl2(schid, cutype, ipl2_addr);
>>  
>>      /* Transfer control to the guest operating system */
>> -    pswl->mask |= PSW_MASK_EAMODE;   /* Force z-mode */
> 
> Wait, PSW_MASK_EAMODE was 0x0000000100000000 and ->mask was only a
> 32-bit value ... how was that ever supposed to work correctly?
> 
>> -    pswl->addr |= PSW_MASK_BAMODE;   /* ...          */
>> +    *pswl |= PSW_MASK_64;   /* Force 64 bit addressing */
> 
> So is this even a bug fix and not only a cosmetic change?
> 
> ... the whole logic here looks fishy to me ... do we need this PSW
> modification at all? Shouldn't the guest decide which mode it wants to
> use in its startup code?
> 
>  Thomas

Looking at jump_to_low_kernel(), jump_to_IPL_code and jump_to_IPL_2 I
think we can remove this line completely.

jump_to_IPL_code() prepares its own PSW and jumps to jump_to_IPL_2() via
diag308 subcode 1.

jump_to_IPL_2() fetches the saved PSW and does a br to the PSW address
so the mask completely goes to waste anyway.


DASD passthrough is rarely used so nobody looks at that code anymore. I
need to ask Jason about the intention behind those lines.


> 
> 
>>      jump_to_low_kernel();
>>  }
>> diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
>> index 5f36361c02..73852029d4 100644
>> --- a/pc-bios/s390-ccw/s390-arch.h
>> +++ b/pc-bios/s390-ccw/s390-arch.h
>> @@ -29,7 +29,7 @@ _Static_assert(sizeof(struct PSWLegacy) == 8, "PSWLegacy 
>> size incorrect");
>>  #define PSW_MASK_WAIT       0x0002000000000000ULL
>>  #define PSW_MASK_EAMODE     0x0000000100000000ULL
>>  #define PSW_MASK_BAMODE     0x0000000080000000ULL
>> -#define PSW_MASK_ZMODE      (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
>> +#define PSW_MASK_64         (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
>>  
>>  /* Low core mapping */
>>  typedef struct LowCore {
>>
>

signature.asc
Description: OpenPGP digital signature