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

[Thomas Huth]

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 {
>