| Message ID | 20200624075226.92728-12-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 1 | expand |
On 24/06/2020 09.52, Janosch Frank wrote: > The 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 | 2 +- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > index 97205674e5..8547a140df 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->psw & PSW_MASK_SHORT_ADDR; Are you really sure about this one here? The address does not seem to be used for any of the jump_to_IPL() functions. And in the zipl sources, I can also see spots like this: entry->compdat.load_address = data.load_address; ... without any further short mask bits. So I somehow doubt that this change is really ok? > 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->psw & PSW_MASK_SHORT_ADDR); That one should be fine, I think. > } > > static void ipl_scsi(void) > diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h > index 12a0166aae..e07f87e690 100644 > --- a/pc-bios/s390-ccw/bootmap.h > +++ b/pc-bios/s390-ccw/bootmap.h > @@ -68,7 +68,7 @@ typedef struct ComponentEntry { > ScsiBlockPtr data; > uint8_t pad[7]; > uint8_t component_type; > - uint64_t load_address; > + uint64_t psw; I'd recommend to keep the load_address name. It's the same name as used in the zipl sources, and as far as I can see, the field does not always contain a PSW. > } __attribute((packed)) ComponentEntry; > > typedef struct ComponentHeader { > Thomas
On 6/25/20 2:46 PM, Thomas Huth wrote: > On 24/06/2020 09.52, Janosch Frank wrote: >> The 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 | 2 +- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c >> index 97205674e5..8547a140df 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->psw & PSW_MASK_SHORT_ADDR; > > Are you really sure about this one here? The address does not seem to be > used for any of the jump_to_IPL() functions. And in the zipl sources, I > can also see spots like this: This one slipped through and is indeed wrong. > > entry->compdat.load_address = data.load_address; > > ... without any further short mask bits. So I somehow doubt that this > change is really ok? > >> 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->psw & PSW_MASK_SHORT_ADDR); > > That one should be fine, I think. Yes, as it is a execute type entry, this needs to be a PSW and therefore needs to be masked. > >> } >> >> static void ipl_scsi(void) >> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h >> index 12a0166aae..e07f87e690 100644 >> --- a/pc-bios/s390-ccw/bootmap.h >> +++ b/pc-bios/s390-ccw/bootmap.h >> @@ -68,7 +68,7 @@ typedef struct ComponentEntry { >> ScsiBlockPtr data; >> uint8_t pad[7]; >> uint8_t component_type; >> - uint64_t load_address; >> + uint64_t psw; > > I'd recommend to keep the load_address name. It's the same name as used > in the zipl sources, and as far as I can see, the field does not always > contain a PSW. The problem is that this is a union in zipl containing an address, psw or signature header. I guess we should also make it a union and use the proper members so it is clear what we retrieve from the entry. If it is a PSW we need to mask it if it is a component address masking might be a bad idea. But I absolutely do not want to have this named PSW and then being used like a normal address. It took me way too long to figure out why my guest wasn't booting anymore. Time for a new series of patches :) > >> } __attribute((packed)) ComponentEntry; >> >> typedef struct ComponentHeader { >> > > Thomas >
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c index 97205674e5..8547a140df 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->psw & PSW_MASK_SHORT_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->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..e07f87e690 100644 --- a/pc-bios/s390-ccw/bootmap.h +++ b/pc-bios/s390-ccw/bootmap.h @@ -68,7 +68,7 @@ typedef struct ComponentEntry { ScsiBlockPtr data; uint8_t pad[7]; uint8_t component_type; - uint64_t load_address; + uint64_t psw; } __attribute((packed)) ComponentEntry; typedef struct ComponentHeader {
The 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 | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-)