| Message ID | 20200715094045.381984-2-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 2 | expand |
On 15/07/2020 11.40, Janosch Frank wrote: > The two main types of zipl component entries are execute and > load/data. The last member of the component entry struct therefore > denotes either a PSW or an address. Let's make this a bit more clear > by introducing a union and cleaning up the code that uses that struct > member. > > The execute type component entries written by zipl contain short PSWs, > not addresses. Let's mask them and only pass the address part to > jump_to_IPL_code(uint64_t address) because it expects an address as > visible by the name of the argument. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/bootmap.c | 5 +++-- > pc-bios/s390-ccw/bootmap.h | 7 ++++++- > 2 files changed, 9 insertions(+), 3 deletions(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
On 15.07.20 11:40, Janosch Frank wrote: > The two main types of zipl component entries are execute and > load/data. The last member of the component entry struct therefore > denotes either a PSW or an address. Let's make this a bit more clear > by introducing a union and cleaning up the code that uses that struct > member. > > The execute type component entries written by zipl contain short PSWs, > not addresses. Let's mask them and only pass the address part to > jump_to_IPL_code(uint64_t address) because it expects an address as > visible by the name of the argument. If zipl actually specifies a PSW, shouldnt we actually USE that PSW including the zipl specified mask? > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/bootmap.c | 5 +++-- > pc-bios/s390-ccw/bootmap.h | 7 ++++++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > index 97205674e5..8747c4ea26 100644 > --- a/pc-bios/s390-ccw/bootmap.c > +++ b/pc-bios/s390-ccw/bootmap.c > @@ -10,6 +10,7 @@ > > #include "libc.h" > #include "s390-ccw.h" > +#include "s390-arch.h" > #include "bootmap.h" > #include "virtio.h" > #include "bswap.h" > @@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry) > char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */ > > blockno = entry->data.blockno; > - address = entry->load_address; > + address = entry->compdat.load_addr; > > debug_print_int("loading segment at block", blockno); > debug_print_int("addr", address); > @@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte) > IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry"); > > /* should not return */ > - jump_to_IPL_code(entry->load_address); > + jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR); > } > > static void ipl_scsi(void) > diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h > index 12a0166aae..3946aa3f8d 100644 > --- a/pc-bios/s390-ccw/bootmap.h > +++ b/pc-bios/s390-ccw/bootmap.h > @@ -64,11 +64,16 @@ typedef struct BootMapTable { > BootMapPointer entry[]; > } __attribute__ ((packed)) BootMapTable; > > +typedef union ComponentEntryData { > + uint64_t load_psw; > + uint64_t load_addr; > +} ComponentEntryData; > + > typedef struct ComponentEntry { > ScsiBlockPtr data; > uint8_t pad[7]; > uint8_t component_type; > - uint64_t load_address; > + ComponentEntryData compdat; > } __attribute((packed)) ComponentEntry; > > typedef struct ComponentHeader { >
On 7/22/20 8:50 AM, Christian Borntraeger wrote: > > > On 15.07.20 11:40, Janosch Frank wrote: >> The two main types of zipl component entries are execute and >> load/data. The last member of the component entry struct therefore >> denotes either a PSW or an address. Let's make this a bit more clear >> by introducing a union and cleaning up the code that uses that struct >> member. >> >> The execute type component entries written by zipl contain short PSWs, >> not addresses. Let's mask them and only pass the address part to >> jump_to_IPL_code(uint64_t address) because it expects an address as >> visible by the name of the argument. > > If zipl actually specifies a PSW, shouldnt we actually USE that PSW including > the zipl specified mask? I expected the current approach to have some kind of meaning behind it, if there isn't I'll be the first one to make it more sensible. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> pc-bios/s390-ccw/bootmap.c | 5 +++-- >> pc-bios/s390-ccw/bootmap.h | 7 ++++++- >> 2 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c >> index 97205674e5..8747c4ea26 100644 >> --- a/pc-bios/s390-ccw/bootmap.c >> +++ b/pc-bios/s390-ccw/bootmap.c >> @@ -10,6 +10,7 @@ >> >> #include "libc.h" >> #include "s390-ccw.h" >> +#include "s390-arch.h" >> #include "bootmap.h" >> #include "virtio.h" >> #include "bswap.h" >> @@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry) >> char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */ >> >> blockno = entry->data.blockno; >> - address = entry->load_address; >> + address = entry->compdat.load_addr; >> >> debug_print_int("loading segment at block", blockno); >> debug_print_int("addr", address); >> @@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte) >> IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry"); >> >> /* should not return */ >> - jump_to_IPL_code(entry->load_address); >> + jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR); >> } >> >> static void ipl_scsi(void) >> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h >> index 12a0166aae..3946aa3f8d 100644 >> --- a/pc-bios/s390-ccw/bootmap.h >> +++ b/pc-bios/s390-ccw/bootmap.h >> @@ -64,11 +64,16 @@ typedef struct BootMapTable { >> BootMapPointer entry[]; >> } __attribute__ ((packed)) BootMapTable; >> >> +typedef union ComponentEntryData { >> + uint64_t load_psw; >> + uint64_t load_addr; >> +} ComponentEntryData; >> + >> typedef struct ComponentEntry { >> ScsiBlockPtr data; >> uint8_t pad[7]; >> uint8_t component_type; >> - uint64_t load_address; >> + ComponentEntryData compdat; >> } __attribute((packed)) ComponentEntry; >> >> typedef struct ComponentHeader { >>
On 22.07.20 09:30, Janosch Frank wrote: > On 7/22/20 8:50 AM, Christian Borntraeger wrote: >> >> >> On 15.07.20 11:40, Janosch Frank wrote: >>> The two main types of zipl component entries are execute and >>> load/data. The last member of the component entry struct therefore >>> denotes either a PSW or an address. Let's make this a bit more clear >>> by introducing a union and cleaning up the code that uses that struct >>> member. >>> >>> The execute type component entries written by zipl contain short PSWs, >>> not addresses. Let's mask them and only pass the address part to >>> jump_to_IPL_code(uint64_t address) because it expects an address as >>> visible by the name of the argument. >> >> If zipl actually specifies a PSW, shouldnt we actually USE that PSW including >> the zipl specified mask? > > I expected the current approach to have some kind of meaning behind it, > if there isn't I'll be the first one to make it more sensible. I think this was just something to make it work with Linux, especially when Linux still started in ESA mode.(So we faked a mask that is good enough to boot Linux and then used the address). But I think the proper solution is to really use the full PSW and go with that according to the CZAM rules.
On 7/22/20 9:33 AM, Christian Borntraeger wrote: > > > On 22.07.20 09:30, Janosch Frank wrote: >> On 7/22/20 8:50 AM, Christian Borntraeger wrote: >>> >>> >>> On 15.07.20 11:40, Janosch Frank wrote: >>>> The two main types of zipl component entries are execute and >>>> load/data. The last member of the component entry struct therefore >>>> denotes either a PSW or an address. Let's make this a bit more clear >>>> by introducing a union and cleaning up the code that uses that struct >>>> member. >>>> >>>> The execute type component entries written by zipl contain short PSWs, >>>> not addresses. Let's mask them and only pass the address part to >>>> jump_to_IPL_code(uint64_t address) because it expects an address as >>>> visible by the name of the argument. >>> >>> If zipl actually specifies a PSW, shouldnt we actually USE that PSW including >>> the zipl specified mask? >> >> I expected the current approach to have some kind of meaning behind it, >> if there isn't I'll be the first one to make it more sensible. > I think this was just something to make it work with Linux, especially when Linux > still started in ESA mode.(So we faked a mask that is good enough to boot Linux and > then used the address). But I think the proper solution is to really use the > full PSW and go with that according to the CZAM rules. > Okay, I'll come up with a proper fix.
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c index 97205674e5..8747c4ea26 100644 --- a/pc-bios/s390-ccw/bootmap.c +++ b/pc-bios/s390-ccw/bootmap.c @@ -10,6 +10,7 @@ #include "libc.h" #include "s390-ccw.h" +#include "s390-arch.h" #include "bootmap.h" #include "virtio.h" #include "bswap.h" @@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry) char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */ blockno = entry->data.blockno; - address = entry->load_address; + address = entry->compdat.load_addr; debug_print_int("loading segment at block", blockno); debug_print_int("addr", address); @@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte) IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry"); /* should not return */ - jump_to_IPL_code(entry->load_address); + jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR); } static void ipl_scsi(void) diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h index 12a0166aae..3946aa3f8d 100644 --- a/pc-bios/s390-ccw/bootmap.h +++ b/pc-bios/s390-ccw/bootmap.h @@ -64,11 +64,16 @@ typedef struct BootMapTable { BootMapPointer entry[]; } __attribute__ ((packed)) BootMapTable; +typedef union ComponentEntryData { + uint64_t load_psw; + uint64_t load_addr; +} ComponentEntryData; + typedef struct ComponentEntry { ScsiBlockPtr data; uint8_t pad[7]; uint8_t component_type; - uint64_t load_address; + ComponentEntryData compdat; } __attribute((packed)) ComponentEntry; typedef struct ComponentHeader {
The two main types of zipl component entries are execute and load/data. The last member of the component entry struct therefore denotes either a PSW or an address. Let's make this a bit more clear by introducing a union and cleaning up the code that uses that struct member. The execute type component entries written by zipl contain short PSWs, not addresses. Let's mask them and only pass the address part to jump_to_IPL_code(uint64_t address) because it expects an address as visible by the name of the argument. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- pc-bios/s390-ccw/bootmap.c | 5 +++-- pc-bios/s390-ccw/bootmap.h | 7 ++++++- 2 files changed, 9 insertions(+), 3 deletions(-)