* [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2
@ 2020-08-31 15:09 Janosch Frank
2020-08-31 15:09 ` [PATCH v3 1/5] pc-bios: s390x: Fix bootmap.c zipl component entry data handling Janosch Frank
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Janosch Frank @ 2020-08-31 15:09 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
So, here are a few more cleanup patches mostly cleaning up ipl code
and some of the assembly.
The patches are available here:
https://github.com/frankjaa/qemu/pull/new/cleanup_bios2
v3:
* Split PSW save rework again
* Added noreturn annotation
* Minor cleanup
v2:
* Fixed psw saving in use reset psw patch (thanks Jason)
* Dropped a lot of patches which weren't strictly necessary
* Added disabled wait patch
* Added RFC PSW save patch
Janosch Frank (5):
pc-bios: s390x: Fix bootmap.c zipl component entry data handling
pc-bios: s390x: Save PSW rework
pc-bios: s390x: Use reset PSW if avaliable
pc-bios: s390x: Save io and external new PSWs before overwriting them
pc-bios: s390x: Go into disabled wait when encountering a PGM
exception
pc-bios/s390-ccw/bootmap.c | 6 ++--
pc-bios/s390-ccw/bootmap.h | 7 +++-
pc-bios/s390-ccw/jump2ipl.c | 51 +++++++++++++++++------------
pc-bios/s390-ccw/netmain.c | 3 ++
pc-bios/s390-ccw/s390-ccw.h | 1 +
pc-bios/s390-ccw/start.S | 65 +++++++++++++++++++++++++++----------
6 files changed, 92 insertions(+), 41 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v3 1/5] pc-bios: s390x: Fix bootmap.c zipl component entry data handling 2020-08-31 15:09 [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 Janosch Frank @ 2020-08-31 15:09 ` Janosch Frank 2020-08-31 15:09 ` [PATCH v3 2/5] pc-bios: s390x: Save PSW rework Janosch Frank ` (4 subsequent siblings) 5 siblings, 0 replies; 15+ messages in thread From: Janosch Frank @ 2020-08-31 15:09 UTC (permalink / raw) To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david 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> Reviewed-by: Thomas Huth <thuth@redhat.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 { -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/5] pc-bios: s390x: Save PSW rework 2020-08-31 15:09 [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 Janosch Frank 2020-08-31 15:09 ` [PATCH v3 1/5] pc-bios: s390x: Fix bootmap.c zipl component entry data handling Janosch Frank @ 2020-08-31 15:09 ` Janosch Frank 2020-08-31 15:27 ` Thomas Huth 2020-08-31 15:09 ` [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable Janosch Frank ` (3 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Janosch Frank @ 2020-08-31 15:09 UTC (permalink / raw) To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david We don't need to save the ipl_continue variable in lowcore and have it limited to 32 bits because of the lowcore layout. Let's move it to a new 64 bit variable and get rid of the reset info struct. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- pc-bios/s390-ccw/jump2ipl.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c index 767012bf0c..b6aad32def 100644 --- a/pc-bios/s390-ccw/jump2ipl.c +++ b/pc-bios/s390-ccw/jump2ipl.c @@ -13,20 +13,17 @@ #define KERN_IMAGE_START 0x010000UL #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) -typedef struct ResetInfo { - uint64_t ipl_psw; - uint32_t ipl_continue; -} ResetInfo; +static uint64_t *reset_psw = 0, save_psw, ipl_continue; -static ResetInfo save; - -static void jump_to_IPL_2(void) +static void jump_to_IPL_addr(void) { - ResetInfo *current = 0; + __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; - void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue; - *current = save; - ipl(); /* should not return */ + /* Restore reset PSW */ + *reset_psw = save_psw; + + ipl(); + /* should not return */ } void jump_to_IPL_code(uint64_t address) @@ -46,15 +43,11 @@ void jump_to_IPL_code(uint64_t address) * content of non-BIOS memory after we loaded the guest, so we * save the original content and restore it in jump_to_IPL_2. */ - ResetInfo *current = 0; - - save = *current; - - current->ipl_psw = (uint64_t) &jump_to_IPL_2; - current->ipl_psw |= RESET_PSW_MASK; - current->ipl_continue = address & PSW_MASK_SHORT_ADDR; - - debug_print_int("set IPL addr to", current->ipl_continue); + save_psw = *reset_psw; + *reset_psw = (uint64_t) &jump_to_IPL_addr; + *reset_psw |= RESET_PSW_MASK; + ipl_continue = address; + debug_print_int("set IPL addr to", ipl_continue); /* Ensure the guest output starts fresh */ sclp_print("\n"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/5] pc-bios: s390x: Save PSW rework 2020-08-31 15:09 ` [PATCH v3 2/5] pc-bios: s390x: Save PSW rework Janosch Frank @ 2020-08-31 15:27 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2020-08-31 15:27 UTC (permalink / raw) To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david On 31/08/2020 17.09, Janosch Frank wrote: > We don't need to save the ipl_continue variable in lowcore and have it > limited to 32 bits because of the lowcore layout. Let's move it to a > new 64 bit variable and get rid of the reset info struct. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/jump2ipl.c | 33 +++++++++++++-------------------- > 1 file changed, 13 insertions(+), 20 deletions(-) > > diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c > index 767012bf0c..b6aad32def 100644 > --- a/pc-bios/s390-ccw/jump2ipl.c > +++ b/pc-bios/s390-ccw/jump2ipl.c > @@ -13,20 +13,17 @@ > #define KERN_IMAGE_START 0x010000UL > #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) > > -typedef struct ResetInfo { > - uint64_t ipl_psw; > - uint32_t ipl_continue; > -} ResetInfo; > +static uint64_t *reset_psw = 0, save_psw, ipl_continue; > > -static ResetInfo save; > - > -static void jump_to_IPL_2(void) > +static void jump_to_IPL_addr(void) > { > - ResetInfo *current = 0; > + __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; > > - void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue; > - *current = save; > - ipl(); /* should not return */ > + /* Restore reset PSW */ > + *reset_psw = save_psw; > + > + ipl(); > + /* should not return */ > } > > void jump_to_IPL_code(uint64_t address) > @@ -46,15 +43,11 @@ void jump_to_IPL_code(uint64_t address) > * content of non-BIOS memory after we loaded the guest, so we > * save the original content and restore it in jump_to_IPL_2. > */ > - ResetInfo *current = 0; > - > - save = *current; > - > - current->ipl_psw = (uint64_t) &jump_to_IPL_2; > - current->ipl_psw |= RESET_PSW_MASK; > - current->ipl_continue = address & PSW_MASK_SHORT_ADDR; > - > - debug_print_int("set IPL addr to", current->ipl_continue); > + save_psw = *reset_psw; > + *reset_psw = (uint64_t) &jump_to_IPL_addr; > + *reset_psw |= RESET_PSW_MASK; > + ipl_continue = address; > + debug_print_int("set IPL addr to", ipl_continue); > > /* Ensure the guest output starts fresh */ > sclp_print("\n"); Reviewed-by: Thomas Huth <thuth@redhat.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable 2020-08-31 15:09 [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 Janosch Frank 2020-08-31 15:09 ` [PATCH v3 1/5] pc-bios: s390x: Fix bootmap.c zipl component entry data handling Janosch Frank 2020-08-31 15:09 ` [PATCH v3 2/5] pc-bios: s390x: Save PSW rework Janosch Frank @ 2020-08-31 15:09 ` Janosch Frank 2020-09-01 16:59 ` Thomas Huth 2020-08-31 15:09 ` [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them Janosch Frank ` (2 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Janosch Frank @ 2020-08-31 15:09 UTC (permalink / raw) To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david If a blob provides a reset PSW then we should use it instead of branching to the PSW address and using our own mask. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- pc-bios/s390-ccw/bootmap.c | 3 ++- pc-bios/s390-ccw/jump2ipl.c | 22 +++++++++++++++++----- pc-bios/s390-ccw/s390-ccw.h | 1 + 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c index 8747c4ea26..5a03b1eb8b 100644 --- a/pc-bios/s390-ccw/bootmap.c +++ b/pc-bios/s390-ccw/bootmap.c @@ -515,7 +515,8 @@ 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->compdat.load_psw & PSW_MASK_SHORT_ADDR); + write_reset_psw(entry->compdat.load_psw); + jump_to_IPL_code(0); } static void ipl_scsi(void) diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c index b6aad32def..5b8352d257 100644 --- a/pc-bios/s390-ccw/jump2ipl.c +++ b/pc-bios/s390-ccw/jump2ipl.c @@ -12,15 +12,21 @@ #define KERN_IMAGE_START 0x010000UL #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) +#define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK) static uint64_t *reset_psw = 0, save_psw, ipl_continue; +void write_reset_psw(uint64_t psw) +{ + *reset_psw = psw; +} + static void jump_to_IPL_addr(void) { __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; /* Restore reset PSW */ - *reset_psw = save_psw; + write_reset_psw(save_psw); ipl(); /* should not return */ @@ -43,9 +49,10 @@ void jump_to_IPL_code(uint64_t address) * content of non-BIOS memory after we loaded the guest, so we * save the original content and restore it in jump_to_IPL_2. */ - save_psw = *reset_psw; - *reset_psw = (uint64_t) &jump_to_IPL_addr; - *reset_psw |= RESET_PSW_MASK; + if (address) { + save_psw = *reset_psw; + write_reset_psw(RESET_PSW); + } ipl_continue = address; debug_print_int("set IPL addr to", ipl_continue); @@ -77,7 +84,12 @@ void jump_to_low_kernel(void) /* Trying to get PSW at zero address */ if (*((uint64_t *)0) & RESET_PSW_MASK) { - jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR); + /* + * Surely nobody will try running directly from lowcore, so + * let's use 0 as an indication that we want to load the reset + * psw at 0x0 and not jump to the entry. + */ + jump_to_IPL_code(0); } /* No other option left, so use the Linux kernel start address */ diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h index 36b884cced..7090720422 100644 --- a/pc-bios/s390-ccw/s390-ccw.h +++ b/pc-bios/s390-ccw/s390-ccw.h @@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr); void zipl_load(void); /* jump2ipl.c */ +void write_reset_psw(uint64_t psw); void jump_to_IPL_code(uint64_t address); void jump_to_low_kernel(void); -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable 2020-08-31 15:09 ` [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable Janosch Frank @ 2020-09-01 16:59 ` Thomas Huth 2020-09-02 8:46 ` Janosch Frank 0 siblings, 1 reply; 15+ messages in thread From: Thomas Huth @ 2020-09-01 16:59 UTC (permalink / raw) To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david On 31/08/2020 17.09, Janosch Frank wrote: > If a blob provides a reset PSW then we should use it instead of > branching to the PSW address and using our own mask. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/bootmap.c | 3 ++- > pc-bios/s390-ccw/jump2ipl.c | 22 +++++++++++++++++----- > pc-bios/s390-ccw/s390-ccw.h | 1 + > 3 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > index 8747c4ea26..5a03b1eb8b 100644 > --- a/pc-bios/s390-ccw/bootmap.c > +++ b/pc-bios/s390-ccw/bootmap.c > @@ -515,7 +515,8 @@ 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->compdat.load_psw & PSW_MASK_SHORT_ADDR); > + write_reset_psw(entry->compdat.load_psw); > + jump_to_IPL_code(0); > } > > static void ipl_scsi(void) > diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c > index b6aad32def..5b8352d257 100644 > --- a/pc-bios/s390-ccw/jump2ipl.c > +++ b/pc-bios/s390-ccw/jump2ipl.c > @@ -12,15 +12,21 @@ > > #define KERN_IMAGE_START 0x010000UL > #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) > +#define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK) > > static uint64_t *reset_psw = 0, save_psw, ipl_continue; > > +void write_reset_psw(uint64_t psw) > +{ > + *reset_psw = psw; > +} > + > static void jump_to_IPL_addr(void) > { > __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; > > /* Restore reset PSW */ > - *reset_psw = save_psw; > + write_reset_psw(save_psw); > > ipl(); > /* should not return */ > @@ -43,9 +49,10 @@ void jump_to_IPL_code(uint64_t address) > * content of non-BIOS memory after we loaded the guest, so we > * save the original content and restore it in jump_to_IPL_2. > */ > - save_psw = *reset_psw; > - *reset_psw = (uint64_t) &jump_to_IPL_addr; > - *reset_psw |= RESET_PSW_MASK; > + if (address) { > + save_psw = *reset_psw; > + write_reset_psw(RESET_PSW); > + } > ipl_continue = address; > debug_print_int("set IPL addr to", ipl_continue); In case you respin this series, I think I'd move the "ipl_continue = address" into the if-statement, too, and change the debug_print_int line to use address instead of ipl_continue. > @@ -77,7 +84,12 @@ void jump_to_low_kernel(void) > > /* Trying to get PSW at zero address */ > if (*((uint64_t *)0) & RESET_PSW_MASK) { > - jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR); > + /* > + * Surely nobody will try running directly from lowcore, so > + * let's use 0 as an indication that we want to load the reset > + * psw at 0x0 and not jump to the entry. > + */ > + jump_to_IPL_code(0); > } > > /* No other option left, so use the Linux kernel start address */ > diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h > index 36b884cced..7090720422 100644 > --- a/pc-bios/s390-ccw/s390-ccw.h > +++ b/pc-bios/s390-ccw/s390-ccw.h > @@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr); > void zipl_load(void); > > /* jump2ipl.c */ > +void write_reset_psw(uint64_t psw); > void jump_to_IPL_code(uint64_t address); > void jump_to_low_kernel(void); Looks sane to me: Reviewed-by: Thomas Huth <thuth@redhat.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable 2020-09-01 16:59 ` Thomas Huth @ 2020-09-02 8:46 ` Janosch Frank 2020-09-02 9:50 ` Thomas Huth 0 siblings, 1 reply; 15+ messages in thread From: Janosch Frank @ 2020-09-02 8:46 UTC (permalink / raw) To: Thomas Huth, qemu-devel; +Cc: borntraeger, cohuck, david [-- Attachment #1.1: Type: text/plain, Size: 3912 bytes --] On 9/1/20 6:59 PM, Thomas Huth wrote: > On 31/08/2020 17.09, Janosch Frank wrote: >> If a blob provides a reset PSW then we should use it instead of >> branching to the PSW address and using our own mask. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> pc-bios/s390-ccw/bootmap.c | 3 ++- >> pc-bios/s390-ccw/jump2ipl.c | 22 +++++++++++++++++----- >> pc-bios/s390-ccw/s390-ccw.h | 1 + >> 3 files changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c >> index 8747c4ea26..5a03b1eb8b 100644 >> --- a/pc-bios/s390-ccw/bootmap.c >> +++ b/pc-bios/s390-ccw/bootmap.c >> @@ -515,7 +515,8 @@ 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->compdat.load_psw & PSW_MASK_SHORT_ADDR); >> + write_reset_psw(entry->compdat.load_psw); >> + jump_to_IPL_code(0); >> } >> >> static void ipl_scsi(void) >> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >> index b6aad32def..5b8352d257 100644 >> --- a/pc-bios/s390-ccw/jump2ipl.c >> +++ b/pc-bios/s390-ccw/jump2ipl.c >> @@ -12,15 +12,21 @@ >> >> #define KERN_IMAGE_START 0x010000UL >> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) >> +#define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK) >> >> static uint64_t *reset_psw = 0, save_psw, ipl_continue; >> >> +void write_reset_psw(uint64_t psw) >> +{ >> + *reset_psw = psw; >> +} >> + >> static void jump_to_IPL_addr(void) >> { >> __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; >> >> /* Restore reset PSW */ >> - *reset_psw = save_psw; >> + write_reset_psw(save_psw); >> >> ipl(); >> /* should not return */ >> @@ -43,9 +49,10 @@ void jump_to_IPL_code(uint64_t address) >> * content of non-BIOS memory after we loaded the guest, so we >> * save the original content and restore it in jump_to_IPL_2. >> */ >> - save_psw = *reset_psw; >> - *reset_psw = (uint64_t) &jump_to_IPL_addr; >> - *reset_psw |= RESET_PSW_MASK; >> + if (address) { >> + save_psw = *reset_psw; >> + write_reset_psw(RESET_PSW); >> + } >> ipl_continue = address; >> debug_print_int("set IPL addr to", ipl_continue); > > In case you respin this series, I think I'd move the "ipl_continue = > address" into the if-statement, too, and change the debug_print_int line > to use address instead of ipl_continue. Hmm, my intention was to always have something printed. But I guess it would make more sense to print the reset psw addr in the ~address case. > >> @@ -77,7 +84,12 @@ void jump_to_low_kernel(void) >> >> /* Trying to get PSW at zero address */ >> if (*((uint64_t *)0) & RESET_PSW_MASK) { >> - jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR); >> + /* >> + * Surely nobody will try running directly from lowcore, so >> + * let's use 0 as an indication that we want to load the reset >> + * psw at 0x0 and not jump to the entry. >> + */ >> + jump_to_IPL_code(0); >> } >> >> /* No other option left, so use the Linux kernel start address */ >> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h >> index 36b884cced..7090720422 100644 >> --- a/pc-bios/s390-ccw/s390-ccw.h >> +++ b/pc-bios/s390-ccw/s390-ccw.h >> @@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr); >> void zipl_load(void); >> >> /* jump2ipl.c */ >> +void write_reset_psw(uint64_t psw); >> void jump_to_IPL_code(uint64_t address); >> void jump_to_low_kernel(void); > > Looks sane to me: > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable 2020-09-02 8:46 ` Janosch Frank @ 2020-09-02 9:50 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2020-09-02 9:50 UTC (permalink / raw) To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david On 02/09/2020 10.46, Janosch Frank wrote: > On 9/1/20 6:59 PM, Thomas Huth wrote: >> On 31/08/2020 17.09, Janosch Frank wrote: >>> If a blob provides a reset PSW then we should use it instead of >>> branching to the PSW address and using our own mask. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> pc-bios/s390-ccw/bootmap.c | 3 ++- >>> pc-bios/s390-ccw/jump2ipl.c | 22 +++++++++++++++++----- >>> pc-bios/s390-ccw/s390-ccw.h | 1 + >>> 3 files changed, 20 insertions(+), 6 deletions(-) >>> >>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c >>> index 8747c4ea26..5a03b1eb8b 100644 >>> --- a/pc-bios/s390-ccw/bootmap.c >>> +++ b/pc-bios/s390-ccw/bootmap.c >>> @@ -515,7 +515,8 @@ 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->compdat.load_psw & PSW_MASK_SHORT_ADDR); >>> + write_reset_psw(entry->compdat.load_psw); >>> + jump_to_IPL_code(0); >>> } >>> >>> static void ipl_scsi(void) >>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >>> index b6aad32def..5b8352d257 100644 >>> --- a/pc-bios/s390-ccw/jump2ipl.c >>> +++ b/pc-bios/s390-ccw/jump2ipl.c >>> @@ -12,15 +12,21 @@ >>> >>> #define KERN_IMAGE_START 0x010000UL >>> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) >>> +#define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK) >>> >>> static uint64_t *reset_psw = 0, save_psw, ipl_continue; >>> >>> +void write_reset_psw(uint64_t psw) >>> +{ >>> + *reset_psw = psw; >>> +} >>> + >>> static void jump_to_IPL_addr(void) >>> { >>> __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; >>> >>> /* Restore reset PSW */ >>> - *reset_psw = save_psw; >>> + write_reset_psw(save_psw); >>> >>> ipl(); >>> /* should not return */ >>> @@ -43,9 +49,10 @@ void jump_to_IPL_code(uint64_t address) >>> * content of non-BIOS memory after we loaded the guest, so we >>> * save the original content and restore it in jump_to_IPL_2. >>> */ >>> - save_psw = *reset_psw; >>> - *reset_psw = (uint64_t) &jump_to_IPL_addr; >>> - *reset_psw |= RESET_PSW_MASK; >>> + if (address) { >>> + save_psw = *reset_psw; >>> + write_reset_psw(RESET_PSW); >>> + } >>> ipl_continue = address; >>> debug_print_int("set IPL addr to", ipl_continue); >> >> In case you respin this series, I think I'd move the "ipl_continue = >> address" into the if-statement, too, and change the debug_print_int line >> to use address instead of ipl_continue. > > Hmm, my intention was to always have something printed. > But I guess it would make more sense to print the reset psw addr in the > ~address case. I meant to only move the "ipl_continue = address" line and keep the debug_print_int() at its current place (you just have to replace ipl_continue there). But you're right, it would make more sense to print the PSW at address 0 in that case instead. Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them 2020-08-31 15:09 [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 Janosch Frank ` (2 preceding siblings ...) 2020-08-31 15:09 ` [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable Janosch Frank @ 2020-08-31 15:09 ` Janosch Frank 2020-09-01 17:22 ` Thomas Huth 2020-08-31 15:09 ` [PATCH v3 5/5] pc-bios: s390x: Go into disabled wait when encountering a PGM exception Janosch Frank 2020-08-31 18:31 ` [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 no-reply 5 siblings, 1 reply; 15+ messages in thread From: Janosch Frank @ 2020-08-31 15:09 UTC (permalink / raw) To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david Currently we always overwrite the mentioned exception new PSWs before loading the enabled wait PSW. Let's save the PSW before overwriting and restore it right before starting the loaded kernel. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- pc-bios/s390-ccw/jump2ipl.c | 4 +++ pc-bios/s390-ccw/netmain.c | 3 ++ pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++---------- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c index 5b8352d257..bb94ba7550 100644 --- a/pc-bios/s390-ccw/jump2ipl.c +++ b/pc-bios/s390-ccw/jump2ipl.c @@ -14,6 +14,7 @@ #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK) +extern uint64_t psw_save_io[], psw_save_ext[]; static uint64_t *reset_psw = 0, save_psw, ipl_continue; void write_reset_psw(uint64_t psw) @@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address) /* Ensure the guest output starts fresh */ sclp_print("\n"); + memcpy(&lowcore->io_new_psw, psw_save_io, 16); + memcpy(&lowcore->external_new_psw, psw_save_ext, 16); + /* * HACK ALERT. * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2 diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c index 056e93a818..74ef28fbc6 100644 --- a/pc-bios/s390-ccw/netmain.c +++ b/pc-bios/s390-ccw/netmain.c @@ -32,6 +32,7 @@ #include <time.h> #include <pxelinux.h> +#include "s390-arch.h" #include "s390-ccw.h" #include "cio.h" #include "virtio.h" @@ -43,6 +44,8 @@ extern char _start[]; void write_iplb_location(void) {} +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */ + #define KERNEL_ADDR ((void *)0L) #define KERNEL_MAX_SIZE ((long)_start) #define ARCH_COMMAND_LINE_SIZE 896 /* Taken from Linux kernel */ diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S index ce519300a1..939aac3a7c 100644 --- a/pc-bios/s390-ccw/start.S +++ b/pc-bios/s390-ccw/start.S @@ -34,7 +34,17 @@ remainder: larl %r2,memsetxc ex %r3,0(%r2) done: - j main /* And call C */ + /* prepare i/o call handler */ + larl %r1, io_new_code + larl %r2, io_new_psw + stg %r1, 8(%r2) + mvc 0x1f0(16),0(%r2) + /* prepare external call handler */ + larl %r1, external_new_code + larl %r2, external_new_psw + stg %r1, 8(%r2) + mvc 0x1b0(16),0(%r2) + j main /* And call C */ memsetxc: xc 0(1,%r1),0(%r1) @@ -64,13 +74,16 @@ consume_sclp_int: oi 6(%r15),0x2 lctlg %c0,%c0,0(%r15) /* prepare external call handler */ - larl %r1, external_new_code - stg %r1, 0x1b8 - larl %r1, external_new_mask - mvc 0x1b0(8),0(%r1) - /* load enabled wait PSW */ - larl %r1, enabled_wait_psw - lpswe 0(%r1) + larl %r1, external_new_psw + lghi %r2, 0x1b0 + /* Is the BIOS' external new PSW already set? */ + clc 0(16, %r1), 0(%r2) + je load_ewait + /* No, save old PSW and write BIOS PSW */ + larl %r3, psw_save_ext + mvc 0(16, %r3), 0x1b0 + mvc 0x1b0(16),0(%r1) + j load_ewait /* * void consume_io_int(void) @@ -84,11 +97,20 @@ consume_io_int: oi 4(%r15), 0xff lctlg %c6,%c6,0(%r15) /* prepare i/o call handler */ - larl %r1, io_new_code - stg %r1, 0x1f8 - larl %r1, io_new_mask - mvc 0x1f0(8),0(%r1) - /* load enabled wait PSW */ + larl %r1, io_new_psw + lghi %r2, 0x1f0 + /* Is the BIOS' PSW already set? */ + larl %r3, load_ewait + clc 0(16, %r1), 0(%r2) + bcr 8, %r3 + /* No, save old PSW and write BIOS PSW */ + larl %r3, psw_save_io + mvc 0(16, %r3), 0x1f0 + mvc 0x1f0(16),0(%r1) + j load_ewait + +load_ewait: + /* PSW is the correct one, time to load the enabled wait PSW */ larl %r1, enabled_wait_psw lpswe 0(%r1) @@ -107,11 +129,17 @@ io_new_code: br %r14 .align 8 + .globl psw_save_io + .globl psw_save_ext disabled_wait_psw: .quad 0x0002000180000000,0x0000000000000000 enabled_wait_psw: .quad 0x0302000180000000,0x0000000000000000 -external_new_mask: - .quad 0x0000000180000000 -io_new_mask: - .quad 0x0000000180000000 +external_new_psw: + .quad 0x0000000180000000,0 +io_new_psw: + .quad 0x0000000180000000,0 +psw_save_io: + .quad 0,0 +psw_save_ext: + .quad 0,0 -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them 2020-08-31 15:09 ` [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them Janosch Frank @ 2020-09-01 17:22 ` Thomas Huth 2020-09-02 10:30 ` Janosch Frank 0 siblings, 1 reply; 15+ messages in thread From: Thomas Huth @ 2020-09-01 17:22 UTC (permalink / raw) To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david On 31/08/2020 17.09, Janosch Frank wrote: > Currently we always overwrite the mentioned exception new PSWs before > loading the enabled wait PSW. Let's save the PSW before overwriting > and restore it right before starting the loaded kernel. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/jump2ipl.c | 4 +++ > pc-bios/s390-ccw/netmain.c | 3 ++ > pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++---------- > 3 files changed, 52 insertions(+), 17 deletions(-) Patch looks basically fine to me, I just got some questions for my understanding below... > diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c > index 5b8352d257..bb94ba7550 100644 > --- a/pc-bios/s390-ccw/jump2ipl.c > +++ b/pc-bios/s390-ccw/jump2ipl.c > @@ -14,6 +14,7 @@ > #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) > #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK) > > +extern uint64_t psw_save_io[], psw_save_ext[]; > static uint64_t *reset_psw = 0, save_psw, ipl_continue; > > void write_reset_psw(uint64_t psw) > @@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address) > /* Ensure the guest output starts fresh */ > sclp_print("\n"); > > + memcpy(&lowcore->io_new_psw, psw_save_io, 16); > + memcpy(&lowcore->external_new_psw, psw_save_ext, 16); > + > /* > * HACK ALERT. > * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2 > diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c > index 056e93a818..74ef28fbc6 100644 > --- a/pc-bios/s390-ccw/netmain.c > +++ b/pc-bios/s390-ccw/netmain.c > @@ -32,6 +32,7 @@ > #include <time.h> > #include <pxelinux.h> > > +#include "s390-arch.h" > #include "s390-ccw.h" > #include "cio.h" > #include "virtio.h" > @@ -43,6 +44,8 @@ > extern char _start[]; > void write_iplb_location(void) {} > > +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */ > + > #define KERNEL_ADDR ((void *)0L) > #define KERNEL_MAX_SIZE ((long)_start) > #define ARCH_COMMAND_LINE_SIZE 896 /* Taken from Linux kernel */ > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index ce519300a1..939aac3a7c 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -34,7 +34,17 @@ remainder: > larl %r2,memsetxc > ex %r3,0(%r2) > done: > - j main /* And call C */ > + /* prepare i/o call handler */ > + larl %r1, io_new_code > + larl %r2, io_new_psw > + stg %r1, 8(%r2) > + mvc 0x1f0(16),0(%r2) > + /* prepare external call handler */ > + larl %r1, external_new_code > + larl %r2, external_new_psw > + stg %r1, 8(%r2) Can't you specify the external_new_code and io_new_code in the external_new_psw / io_new_psw directly? Or is our relocation code not good enough for this? > + mvc 0x1b0(16),0(%r2) > + j main /* And call C */ > > memsetxc: > xc 0(1,%r1),0(%r1) > @@ -64,13 +74,16 @@ consume_sclp_int: > oi 6(%r15),0x2 > lctlg %c0,%c0,0(%r15) > /* prepare external call handler */ > - larl %r1, external_new_code > - stg %r1, 0x1b8 > - larl %r1, external_new_mask > - mvc 0x1b0(8),0(%r1) > - /* load enabled wait PSW */ > - larl %r1, enabled_wait_psw > - lpswe 0(%r1) > + larl %r1, external_new_psw > + lghi %r2, 0x1b0 > + /* Is the BIOS' external new PSW already set? */ > + clc 0(16, %r1), 0(%r2) > + je load_ewait > + /* No, save old PSW and write BIOS PSW */ > + larl %r3, psw_save_ext > + mvc 0(16, %r3), 0x1b0 > + mvc 0x1b0(16),0(%r1) > + j load_ewait > > /* > * void consume_io_int(void) > @@ -84,11 +97,20 @@ consume_io_int: > oi 4(%r15), 0xff > lctlg %c6,%c6,0(%r15) > /* prepare i/o call handler */ > - larl %r1, io_new_code > - stg %r1, 0x1f8 > - larl %r1, io_new_mask > - mvc 0x1f0(8),0(%r1) > - /* load enabled wait PSW */ > + larl %r1, io_new_psw > + lghi %r2, 0x1f0 > + /* Is the BIOS' PSW already set? */ > + larl %r3, load_ewait > + clc 0(16, %r1), 0(%r2) > + bcr 8, %r3 Why not a "je load_ewait" again, like in the consume_sclp_int handler? > + /* No, save old PSW and write BIOS PSW */ > + larl %r3, psw_save_io > + mvc 0(16, %r3), 0x1f0 > + mvc 0x1f0(16),0(%r1) > + j load_ewait > + > +load_ewait: > + /* PSW is the correct one, time to load the enabled wait PSW */ > larl %r1, enabled_wait_psw > lpswe 0(%r1) > > @@ -107,11 +129,17 @@ io_new_code: > br %r14 > > .align 8 > + .globl psw_save_io > + .globl psw_save_ext > disabled_wait_psw: > .quad 0x0002000180000000,0x0000000000000000 > enabled_wait_psw: > .quad 0x0302000180000000,0x0000000000000000 > -external_new_mask: > - .quad 0x0000000180000000 > -io_new_mask: > - .quad 0x0000000180000000 > +external_new_psw: > + .quad 0x0000000180000000,0 > +io_new_psw: > + .quad 0x0000000180000000,0 > +psw_save_io: > + .quad 0,0 > +psw_save_ext: > + .quad 0,0 > In case you respin, could you maybe add some local #defines for 0x1f0 and 0x1b0 ? Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them 2020-09-01 17:22 ` Thomas Huth @ 2020-09-02 10:30 ` Janosch Frank 2020-09-02 16:55 ` Thomas Huth 0 siblings, 1 reply; 15+ messages in thread From: Janosch Frank @ 2020-09-02 10:30 UTC (permalink / raw) To: Thomas Huth, qemu-devel; +Cc: borntraeger, cohuck, david [-- Attachment #1.1: Type: text/plain, Size: 6088 bytes --] On 9/1/20 7:22 PM, Thomas Huth wrote: > On 31/08/2020 17.09, Janosch Frank wrote: >> Currently we always overwrite the mentioned exception new PSWs before >> loading the enabled wait PSW. Let's save the PSW before overwriting >> and restore it right before starting the loaded kernel. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> pc-bios/s390-ccw/jump2ipl.c | 4 +++ >> pc-bios/s390-ccw/netmain.c | 3 ++ >> pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++---------- >> 3 files changed, 52 insertions(+), 17 deletions(-) > > Patch looks basically fine to me, I just got some questions for my > understanding below... > >> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >> index 5b8352d257..bb94ba7550 100644 >> --- a/pc-bios/s390-ccw/jump2ipl.c >> +++ b/pc-bios/s390-ccw/jump2ipl.c >> @@ -14,6 +14,7 @@ >> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) >> #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK) >> >> +extern uint64_t psw_save_io[], psw_save_ext[]; >> static uint64_t *reset_psw = 0, save_psw, ipl_continue; >> >> void write_reset_psw(uint64_t psw) >> @@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address) >> /* Ensure the guest output starts fresh */ >> sclp_print("\n"); >> >> + memcpy(&lowcore->io_new_psw, psw_save_io, 16); >> + memcpy(&lowcore->external_new_psw, psw_save_ext, 16); >> + >> /* >> * HACK ALERT. >> * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2 >> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c >> index 056e93a818..74ef28fbc6 100644 >> --- a/pc-bios/s390-ccw/netmain.c >> +++ b/pc-bios/s390-ccw/netmain.c >> @@ -32,6 +32,7 @@ >> #include <time.h> >> #include <pxelinux.h> >> >> +#include "s390-arch.h" >> #include "s390-ccw.h" >> #include "cio.h" >> #include "virtio.h" >> @@ -43,6 +44,8 @@ >> extern char _start[]; >> void write_iplb_location(void) {} >> >> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */ >> + >> #define KERNEL_ADDR ((void *)0L) >> #define KERNEL_MAX_SIZE ((long)_start) >> #define ARCH_COMMAND_LINE_SIZE 896 /* Taken from Linux kernel */ >> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S >> index ce519300a1..939aac3a7c 100644 >> --- a/pc-bios/s390-ccw/start.S >> +++ b/pc-bios/s390-ccw/start.S >> @@ -34,7 +34,17 @@ remainder: >> larl %r2,memsetxc >> ex %r3,0(%r2) >> done: >> - j main /* And call C */ >> + /* prepare i/o call handler */ >> + larl %r1, io_new_code >> + larl %r2, io_new_psw >> + stg %r1, 8(%r2) >> + mvc 0x1f0(16),0(%r2) >> + /* prepare external call handler */ >> + larl %r1, external_new_code >> + larl %r2, external_new_psw >> + stg %r1, 8(%r2) > > Can't you specify the external_new_code and io_new_code in the > external_new_psw / io_new_psw directly? Or is our relocation code not > good enough for this? Initially I had some problems with this. I just had another try and it seems to work well, but as the testing infrastructure doesn't really work, I can't vouch for that. > >> + mvc 0x1b0(16),0(%r2) >> + j main /* And call C */ >> >> memsetxc: >> xc 0(1,%r1),0(%r1) >> @@ -64,13 +74,16 @@ consume_sclp_int: >> oi 6(%r15),0x2 >> lctlg %c0,%c0,0(%r15) >> /* prepare external call handler */ >> - larl %r1, external_new_code >> - stg %r1, 0x1b8 >> - larl %r1, external_new_mask >> - mvc 0x1b0(8),0(%r1) >> - /* load enabled wait PSW */ >> - larl %r1, enabled_wait_psw >> - lpswe 0(%r1) >> + larl %r1, external_new_psw >> + lghi %r2, 0x1b0 >> + /* Is the BIOS' external new PSW already set? */ >> + clc 0(16, %r1), 0(%r2) >> + je load_ewait >> + /* No, save old PSW and write BIOS PSW */ >> + larl %r3, psw_save_ext >> + mvc 0(16, %r3), 0x1b0 >> + mvc 0x1b0(16),0(%r1) >> + j load_ewait >> >> /* >> * void consume_io_int(void) >> @@ -84,11 +97,20 @@ consume_io_int: >> oi 4(%r15), 0xff >> lctlg %c6,%c6,0(%r15) >> /* prepare i/o call handler */ >> - larl %r1, io_new_code >> - stg %r1, 0x1f8 >> - larl %r1, io_new_mask >> - mvc 0x1f0(8),0(%r1) >> - /* load enabled wait PSW */ >> + larl %r1, io_new_psw >> + lghi %r2, 0x1f0 >> + /* Is the BIOS' PSW already set? */ >> + larl %r3, load_ewait >> + clc 0(16, %r1), 0(%r2) >> + bcr 8, %r3 > > Why not a "je load_ewait" again, like in the consume_sclp_int handler? Great question > >> + /* No, save old PSW and write BIOS PSW */ >> + larl %r3, psw_save_io >> + mvc 0(16, %r3), 0x1f0 >> + mvc 0x1f0(16),0(%r1) >> + j load_ewait >> + >> +load_ewait: >> + /* PSW is the correct one, time to load the enabled wait PSW */ >> larl %r1, enabled_wait_psw >> lpswe 0(%r1) >> >> @@ -107,11 +129,17 @@ io_new_code: >> br %r14 >> >> .align 8 >> + .globl psw_save_io >> + .globl psw_save_ext >> disabled_wait_psw: >> .quad 0x0002000180000000,0x0000000000000000 >> enabled_wait_psw: >> .quad 0x0302000180000000,0x0000000000000000 >> -external_new_mask: >> - .quad 0x0000000180000000 >> -io_new_mask: >> - .quad 0x0000000180000000 >> +external_new_psw: >> + .quad 0x0000000180000000,0 >> +io_new_psw: >> + .quad 0x0000000180000000,0 >> +psw_save_io: >> + .quad 0,0 >> +psw_save_ext: >> + .quad 0,0 >> > > In case you respin, could you maybe add some local #defines for 0x1f0 > and 0x1b0 ? At the top of this file? > > Thomas > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them 2020-09-02 10:30 ` Janosch Frank @ 2020-09-02 16:55 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2020-09-02 16:55 UTC (permalink / raw) To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david On 02/09/2020 12.30, Janosch Frank wrote: > On 9/1/20 7:22 PM, Thomas Huth wrote: >> On 31/08/2020 17.09, Janosch Frank wrote: >>> Currently we always overwrite the mentioned exception new PSWs before >>> loading the enabled wait PSW. Let's save the PSW before overwriting >>> and restore it right before starting the loaded kernel. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> pc-bios/s390-ccw/jump2ipl.c | 4 +++ >>> pc-bios/s390-ccw/netmain.c | 3 ++ >>> pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++---------- >>> 3 files changed, 52 insertions(+), 17 deletions(-) >> >> Patch looks basically fine to me, I just got some questions for my >> understanding below... >> >>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >>> index 5b8352d257..bb94ba7550 100644 >>> --- a/pc-bios/s390-ccw/jump2ipl.c >>> +++ b/pc-bios/s390-ccw/jump2ipl.c >>> @@ -14,6 +14,7 @@ >>> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) >>> #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK) >>> >>> +extern uint64_t psw_save_io[], psw_save_ext[]; >>> static uint64_t *reset_psw = 0, save_psw, ipl_continue; >>> >>> void write_reset_psw(uint64_t psw) >>> @@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address) >>> /* Ensure the guest output starts fresh */ >>> sclp_print("\n"); >>> >>> + memcpy(&lowcore->io_new_psw, psw_save_io, 16); >>> + memcpy(&lowcore->external_new_psw, psw_save_ext, 16); >>> + >>> /* >>> * HACK ALERT. >>> * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2 >>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c >>> index 056e93a818..74ef28fbc6 100644 >>> --- a/pc-bios/s390-ccw/netmain.c >>> +++ b/pc-bios/s390-ccw/netmain.c >>> @@ -32,6 +32,7 @@ >>> #include <time.h> >>> #include <pxelinux.h> >>> >>> +#include "s390-arch.h" >>> #include "s390-ccw.h" >>> #include "cio.h" >>> #include "virtio.h" >>> @@ -43,6 +44,8 @@ >>> extern char _start[]; >>> void write_iplb_location(void) {} >>> >>> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */ >>> + >>> #define KERNEL_ADDR ((void *)0L) >>> #define KERNEL_MAX_SIZE ((long)_start) >>> #define ARCH_COMMAND_LINE_SIZE 896 /* Taken from Linux kernel */ >>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S >>> index ce519300a1..939aac3a7c 100644 >>> --- a/pc-bios/s390-ccw/start.S >>> +++ b/pc-bios/s390-ccw/start.S >>> @@ -34,7 +34,17 @@ remainder: >>> larl %r2,memsetxc >>> ex %r3,0(%r2) >>> done: >>> - j main /* And call C */ >>> + /* prepare i/o call handler */ >>> + larl %r1, io_new_code >>> + larl %r2, io_new_psw >>> + stg %r1, 8(%r2) >>> + mvc 0x1f0(16),0(%r2) >>> + /* prepare external call handler */ >>> + larl %r1, external_new_code >>> + larl %r2, external_new_psw >>> + stg %r1, 8(%r2) >> >> Can't you specify the external_new_code and io_new_code in the >> external_new_psw / io_new_psw directly? Or is our relocation code not >> good enough for this? > > Initially I had some problems with this. I just had another try and it > seems to work well, but as the testing infrastructure doesn't really > work, I can't vouch for that. You could maybe dump the memory in both cases to see whether external_new_psw and io_new_psw contain the same values before and after the change? >> In case you respin, could you maybe add some local #defines for 0x1f0 >> and 0x1b0 ? > > At the top of this file? Yes, please. Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 5/5] pc-bios: s390x: Go into disabled wait when encountering a PGM exception 2020-08-31 15:09 [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 Janosch Frank ` (3 preceding siblings ...) 2020-08-31 15:09 ` [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them Janosch Frank @ 2020-08-31 15:09 ` Janosch Frank 2020-09-01 17:24 ` Thomas Huth 2020-08-31 18:31 ` [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 no-reply 5 siblings, 1 reply; 15+ messages in thread From: Janosch Frank @ 2020-08-31 15:09 UTC (permalink / raw) To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david Let's setup a PGM PSW, so we won't load 0s when a program exception happens. Instead we'll load a disabled wait PSW. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> --- pc-bios/s390-ccw/start.S | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S index 939aac3a7c..775b45baeb 100644 --- a/pc-bios/s390-ccw/start.S +++ b/pc-bios/s390-ccw/start.S @@ -44,6 +44,9 @@ done: larl %r2, external_new_psw stg %r1, 8(%r2) mvc 0x1b0(16),0(%r2) + /* set up a pgm exception disabled wait psw */ + larl %r2, disabled_wait_psw + mvc 0x01d0(16), 0(%r2) j main /* And call C */ memsetxc: -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 5/5] pc-bios: s390x: Go into disabled wait when encountering a PGM exception 2020-08-31 15:09 ` [PATCH v3 5/5] pc-bios: s390x: Go into disabled wait when encountering a PGM exception Janosch Frank @ 2020-09-01 17:24 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2020-09-01 17:24 UTC (permalink / raw) To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david On 31/08/2020 17.09, Janosch Frank wrote: > Let's setup a PGM PSW, so we won't load 0s when a program exception > happens. Instead we'll load a disabled wait PSW. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > pc-bios/s390-ccw/start.S | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index 939aac3a7c..775b45baeb 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -44,6 +44,9 @@ done: > larl %r2, external_new_psw > stg %r1, 8(%r2) > mvc 0x1b0(16),0(%r2) > + /* set up a pgm exception disabled wait psw */ > + larl %r2, disabled_wait_psw > + mvc 0x01d0(16), 0(%r2) > j main /* And call C */ > > memsetxc: > Sounds like a good idea. Reviewed-by: Thomas Huth <thuth@redhat.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 2020-08-31 15:09 [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 Janosch Frank ` (4 preceding siblings ...) 2020-08-31 15:09 ` [PATCH v3 5/5] pc-bios: s390x: Go into disabled wait when encountering a PGM exception Janosch Frank @ 2020-08-31 18:31 ` no-reply 5 siblings, 0 replies; 15+ messages in thread From: no-reply @ 2020-08-31 18:31 UTC (permalink / raw) To: frankja; +Cc: borntraeger, thuth, cohuck, qemu-devel, david Patchew URL: https://patchew.org/QEMU/20200831150910.317171-1-frankja@linux.ibm.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20200831150910.317171-1-frankja@linux.ibm.com Subject: [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === From https://github.com/patchew-project/qemu * [new tag] patchew/20200831150910.317171-1-frankja@linux.ibm.com -> patchew/20200831150910.317171-1-frankja@linux.ibm.com Switched to a new branch 'test' 62c6c18 pc-bios: s390x: Go into disabled wait when encountering a PGM exception 28a68e4 pc-bios: s390x: Save io and external new PSWs before overwriting them 58aec9e pc-bios: s390x: Use reset PSW if avaliable b4cf040 pc-bios: s390x: Save PSW rework 30bab1a pc-bios: s390x: Fix bootmap.c zipl component entry data handling === OUTPUT BEGIN === 1/5 Checking commit 30bab1ae7536 (pc-bios: s390x: Fix bootmap.c zipl component entry data handling) 2/5 Checking commit b4cf040a1e53 (pc-bios: s390x: Save PSW rework) 3/5 Checking commit 58aec9efd157 (pc-bios: s390x: Use reset PSW if avaliable) 4/5 Checking commit 28a68e4503b1 (pc-bios: s390x: Save io and external new PSWs before overwriting them) ERROR: externs should be avoided in .c files #22: FILE: pc-bios/s390-ccw/jump2ipl.c:17: +extern uint64_t psw_save_io[], psw_save_ext[]; total: 1 errors, 0 warnings, 118 lines checked Patch 4/5 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 5/5 Checking commit 62c6c187c90f (pc-bios: s390x: Go into disabled wait when encountering a PGM exception) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20200831150910.317171-1-frankja@linux.ibm.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-09-02 16:56 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-31 15:09 [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 Janosch Frank 2020-08-31 15:09 ` [PATCH v3 1/5] pc-bios: s390x: Fix bootmap.c zipl component entry data handling Janosch Frank 2020-08-31 15:09 ` [PATCH v3 2/5] pc-bios: s390x: Save PSW rework Janosch Frank 2020-08-31 15:27 ` Thomas Huth 2020-08-31 15:09 ` [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable Janosch Frank 2020-09-01 16:59 ` Thomas Huth 2020-09-02 8:46 ` Janosch Frank 2020-09-02 9:50 ` Thomas Huth 2020-08-31 15:09 ` [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them Janosch Frank 2020-09-01 17:22 ` Thomas Huth 2020-09-02 10:30 ` Janosch Frank 2020-09-02 16:55 ` Thomas Huth 2020-08-31 15:09 ` [PATCH v3 5/5] pc-bios: s390x: Go into disabled wait when encountering a PGM exception Janosch Frank 2020-09-01 17:24 ` Thomas Huth 2020-08-31 18:31 ` [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2 no-reply
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.