| Message ID | 20200622074235.32528-6-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 1 | expand |
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 { >
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 --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 {