| Message ID | 20200514123729.156283-5-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 1 | expand |
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
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
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 --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 {