diff mbox series

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

Message ID 20200622074235.32528-6-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series pc-bios: s390x: Cleanup part 1 | expand

Commit Message

Janosch Frank June 22, 2020, 7:42 a.m. UTC 
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(-)

Comments

Thomas Huth June 22, 2020, 11:40 a.m. UTC | #1 
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


>      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 {
>
Janosch Frank June 22, 2020, 1:01 p.m. UTC | #2 
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 {
>>
>
diff mbox series

Patch

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;
     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 */
-    pswl->addr |= PSW_MASK_BAMODE;   /* ...          */
+    *pswl |= PSW_MASK_64;   /* Force 64 bit addressing */
     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 {