diff mbox series

[v2,4/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant

Message ID 20200514123729.156283-5-frankja@linux.ibm.com
State New
Headers show
Series pc-bios: s390x: Cleanup part 1 | expand

Commit Message

Janosch Frank May 14, 2020, 12:37 p.m. UTC 
ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
actually 64 bit addressing.

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  | 3 +--
 pc-bios/s390-ccw/s390-arch.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Thomas Huth May 21, 2020, 5:44 a.m. UTC | #1 
On 14/05/2020 14.37, Janosch Frank wrote:
> ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
> actually 64 bit addressing.
> 
> 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  | 3 +--
>  pc-bios/s390-ccw/s390-arch.h | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index 0fc879bb8e..b932531e6f 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -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->mask |= PSW_MASK_64;   /* Force 64 bit addressing */

This is not only a rename (as announced in the subject), but also a
change in behavior since you now do not change pswl->addr anymore. So
this is even a bug fix? Could you please mention this in the patch
description, too?

 Thanks,
  Thomas
Thomas Huth May 21, 2020, 5:47 a.m. UTC | #2 
On 21/05/2020 07.44, Thomas Huth wrote:
> On 14/05/2020 14.37, Janosch Frank wrote:
>> ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
>> actually 64 bit addressing.
>>
>> 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  | 3 +--
>>  pc-bios/s390-ccw/s390-arch.h | 2 +-
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
>> index 0fc879bb8e..b932531e6f 100644
>> --- a/pc-bios/s390-ccw/dasd-ipl.c
>> +++ b/pc-bios/s390-ccw/dasd-ipl.c
>> @@ -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->mask |= PSW_MASK_64;   /* Force 64 bit addressing */
> 
> This is not only a rename (as announced in the subject), but also a
> change in behavior since you now do not change pswl->addr anymore. So
> this is even a bug fix? Could you please mention this in the patch
> description, too?

Ah, wait, pswl is of type PSWLegacy, and ->mask and ->addr are of type
uint32_t here! So it seems wrong to use a 64-bit value for mask here,
doesn't it?

 Thomas
Janosch Frank May 25, 2020, 12:03 p.m. UTC | #3 
On 5/21/20 7:47 AM, Thomas Huth wrote:
> On 21/05/2020 07.44, Thomas Huth wrote:
>> On 14/05/2020 14.37, Janosch Frank wrote:
>>> ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
>>> actually 64 bit addressing.
>>>
>>> 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  | 3 +--
>>>  pc-bios/s390-ccw/s390-arch.h | 2 +-
>>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
>>> index 0fc879bb8e..b932531e6f 100644
>>> --- a/pc-bios/s390-ccw/dasd-ipl.c
>>> +++ b/pc-bios/s390-ccw/dasd-ipl.c
>>> @@ -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->mask |= PSW_MASK_64;   /* Force 64 bit addressing */
>>
>> This is not only a rename (as announced in the subject), but also a
>> change in behavior since you now do not change pswl->addr anymore. So
>> this is even a bug fix? Could you please mention this in the patch
>> description, too?
> 
> Ah, wait, pswl is of type PSWLegacy, and ->mask and ->addr are of type
> uint32_t here! So it seems wrong to use a 64-bit value for mask here,
> doesn't it?

Absolutely, how did that even compile?

I'm tempted to just make it a unsigned long ptr instead. The legacy PSW
struct doesn't seem to be used correctly anyway, the lowcore one is in
fact never used and this is the only other occurrence.
diff mbox series

Patch

diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0fc879bb8e..b932531e6f 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -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->mask |= 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 {