| Message ID | 20190103100806.9039-8-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 390x: Add cross hypervisor and disk boot | expand |
On 2019-01-03 11:08, Janosch Frank wrote: > We need to properly implement interrupt handling for SCLP, because on > z/VM and LPAR SCLP calls are not synchronous! > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > lib/s390x/asm/arch_def.h | 1 + > lib/s390x/asm/interrupt.h | 2 ++ > lib/s390x/interrupt.c | 12 ++++++++++-- > lib/s390x/sclp-console.c | 2 ++ > lib/s390x/sclp.c | 39 +++++++++++++++++++++++++++++++++++++-- > lib/s390x/sclp.h | 3 +++ > 6 files changed, 55 insertions(+), 4 deletions(-) [...] > diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c > index 7f556e5..817c692 100644 > --- a/lib/s390x/sclp.c > +++ b/lib/s390x/sclp.c > @@ -14,6 +14,7 @@ > #include <asm/page.h> > #include <asm/arch_def.h> > #include <asm/interrupt.h> > +#include <asm/barrier.h> > #include "sclp.h" > #include <alloc_phys.h> > > @@ -24,6 +25,7 @@ static uint64_t max_ram_size; > static uint64_t ram_size; > > char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096))); > +static volatile bool sclp_busy; > > static void mem_init(phys_addr_t mem_end) > { > @@ -32,17 +34,48 @@ static void mem_init(phys_addr_t mem_end) > phys_alloc_init(freemem_start, mem_end - freemem_start); > } > > +static void sclp_setup_int(void) > +{ > + uint64_t mask; > + > + ctl_set_bit(0, 9); > + > + mask = extract_psw_mask(); > + mask |= PSW_MASK_EXT; > + load_psw_mask(mask); > +} > + > +void sclp_handle_ext(void) > +{ > + ctl_clear_bit(0, 9); > + sclp_busy = false; > +} > + > +void sclp_wait_busy(void) > +{ > + while (sclp_busy) > + mb(); > +} > + > +void sclp_mark_busy(void) > +{ > + sclp_busy = true; > +} > + > static void sclp_read_scp_info(ReadInfo *ri, int length) > { > unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, > SCLP_CMDW_READ_SCP_INFO }; > - int i; > + int i, cc; > > for (i = 0; i < ARRAY_SIZE(commands); i++) { > memset(&ri->h, 0, sizeof(ri->h)); > ri->h.length = length; > > - if (sclp_service_call(commands[i], ri)) > + sclp_mark_busy(); > + cc = sclp_service_call(commands[i], ri); > + sclp_wait_busy(); You already do the sclp_wait_busy() in sclp_service_call now, so I think you don't need the sclp_wait_busy() here anymore? Also, what about moving the sclp_mark_busy() calls to the beginning of sclp_service_call() instead? > + if (cc) > break; > if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION) > return; > @@ -57,12 +90,14 @@ int sclp_service_call(unsigned int command, void *sccb) > { > int cc; > > + sclp_setup_int(); > asm volatile( > " .insn rre,0xb2200000,%1,%2\n" /* servc %1,%2 */ > " ipm %0\n" > " srl %0,28" > : "=&d" (cc) : "d" (command), "a" (__pa(sccb)) > : "cc", "memory"); > + sclp_wait_busy(); > if (cc == 3) > return -1; > if (cc == 2) Thomas
On 03.01.19 13:58, Thomas Huth wrote: > On 2019-01-03 11:08, Janosch Frank wrote: >> We need to properly implement interrupt handling for SCLP, because on >> z/VM and LPAR SCLP calls are not synchronous! >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> [...] >> + >> static void sclp_read_scp_info(ReadInfo *ri, int length) >> { >> unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, >> SCLP_CMDW_READ_SCP_INFO }; >> - int i; >> + int i, cc; >> >> for (i = 0; i < ARRAY_SIZE(commands); i++) { >> memset(&ri->h, 0, sizeof(ri->h)); >> ri->h.length = length; >> >> - if (sclp_service_call(commands[i], ri)) >> + sclp_mark_busy(); >> + cc = sclp_service_call(commands[i], ri); >> + sclp_wait_busy(); > > You already do the sclp_wait_busy() in sclp_service_call now, so I think > you don't need the sclp_wait_busy() here anymore? Yeah, that has to go. > > Also, what about moving the sclp_mark_busy() calls to the beginning of > sclp_service_call() instead? Wouldn't that create a race on the data of __sccb and we could end with garbled scb commands?
On 2019-01-03 14:13, Janosch Frank wrote: > On 03.01.19 13:58, Thomas Huth wrote: >> On 2019-01-03 11:08, Janosch Frank wrote: >>> We need to properly implement interrupt handling for SCLP, because on >>> z/VM and LPAR SCLP calls are not synchronous! >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > [...] >>> + >>> static void sclp_read_scp_info(ReadInfo *ri, int length) >>> { >>> unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, >>> SCLP_CMDW_READ_SCP_INFO }; >>> - int i; >>> + int i, cc; >>> >>> for (i = 0; i < ARRAY_SIZE(commands); i++) { >>> memset(&ri->h, 0, sizeof(ri->h)); >>> ri->h.length = length; >>> >>> - if (sclp_service_call(commands[i], ri)) >>> + sclp_mark_busy(); >>> + cc = sclp_service_call(commands[i], ri); >>> + sclp_wait_busy(); >> >> You already do the sclp_wait_busy() in sclp_service_call now, so I think >> you don't need the sclp_wait_busy() here anymore? > > Yeah, that has to go. > >> >> Also, what about moving the sclp_mark_busy() calls to the beginning of >> sclp_service_call() instead? > > Wouldn't that create a race on the data of __sccb and we could end with > garbled scb commands? Since there is a sclp_wait_busy in sclp_service_call already, you can be sure that the previous command already finished, can't you? Thomas
On 03.01.19 14:15, Thomas Huth wrote: > On 2019-01-03 14:13, Janosch Frank wrote: >> On 03.01.19 13:58, Thomas Huth wrote: >>> On 2019-01-03 11:08, Janosch Frank wrote: >>>> We need to properly implement interrupt handling for SCLP, because on >>>> z/VM and LPAR SCLP calls are not synchronous! >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> [...] >>>> + >>>> static void sclp_read_scp_info(ReadInfo *ri, int length) >>>> { >>>> unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, >>>> SCLP_CMDW_READ_SCP_INFO }; >>>> - int i; >>>> + int i, cc; >>>> >>>> for (i = 0; i < ARRAY_SIZE(commands); i++) { >>>> memset(&ri->h, 0, sizeof(ri->h)); >>>> ri->h.length = length; >>>> >>>> - if (sclp_service_call(commands[i], ri)) >>>> + sclp_mark_busy(); >>>> + cc = sclp_service_call(commands[i], ri); >>>> + sclp_wait_busy(); >>> >>> You already do the sclp_wait_busy() in sclp_service_call now, so I think >>> you don't need the sclp_wait_busy() here anymore? >> >> Yeah, that has to go. >> >>> >>> Also, what about moving the sclp_mark_busy() calls to the beginning of >>> sclp_service_call() instead? >> >> Wouldn't that create a race on the data of __sccb and we could end with >> garbled scb commands? > > Since there is a sclp_wait_busy in sclp_service_call already, you can be > sure that the previous command already finished, can't you? I mean before calling sclp_service_call. That's only a problem for smp, but before calling sclp, we prepare the data in the shared __sccb page and that is currently protected by the busy mark. If it's not, then two threads could write to __sccb at the same time and the first that succeeds to call sclp will get a mix of data of both threads.
On 2019-01-03 14:23, Janosch Frank wrote: > On 03.01.19 14:15, Thomas Huth wrote: >> On 2019-01-03 14:13, Janosch Frank wrote: >>> On 03.01.19 13:58, Thomas Huth wrote: >>>> On 2019-01-03 11:08, Janosch Frank wrote: >>>>> We need to properly implement interrupt handling for SCLP, because on >>>>> z/VM and LPAR SCLP calls are not synchronous! >>>>> >>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> [...] >>>>> + >>>>> static void sclp_read_scp_info(ReadInfo *ri, int length) >>>>> { >>>>> unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, >>>>> SCLP_CMDW_READ_SCP_INFO }; >>>>> - int i; >>>>> + int i, cc; >>>>> >>>>> for (i = 0; i < ARRAY_SIZE(commands); i++) { >>>>> memset(&ri->h, 0, sizeof(ri->h)); >>>>> ri->h.length = length; >>>>> >>>>> - if (sclp_service_call(commands[i], ri)) >>>>> + sclp_mark_busy(); >>>>> + cc = sclp_service_call(commands[i], ri); >>>>> + sclp_wait_busy(); >>>> >>>> You already do the sclp_wait_busy() in sclp_service_call now, so I think >>>> you don't need the sclp_wait_busy() here anymore? >>> >>> Yeah, that has to go. >>> >>>> >>>> Also, what about moving the sclp_mark_busy() calls to the beginning of >>>> sclp_service_call() instead? >>> >>> Wouldn't that create a race on the data of __sccb and we could end with >>> garbled scb commands? >> >> Since there is a sclp_wait_busy in sclp_service_call already, you can be >> sure that the previous command already finished, can't you? > > I mean before calling sclp_service_call. > That's only a problem for smp, but before calling sclp, we prepare the > data in the shared __sccb page and that is currently protected by the > busy mark. If it's not, then two threads could write to __sccb at the > same time and the first that succeeds to call sclp will get a mix of > data of both threads. But in that case, you also need to do a sclp_wait_busy() before calling sclp_mark_busy() in all locations - otherwise the code is also not thread-safe in its current shape, is it? Thomas
On 2019-01-03 14:33, Thomas Huth wrote: > On 2019-01-03 14:23, Janosch Frank wrote: >> On 03.01.19 14:15, Thomas Huth wrote: >>> On 2019-01-03 14:13, Janosch Frank wrote: >>>> On 03.01.19 13:58, Thomas Huth wrote: >>>>> On 2019-01-03 11:08, Janosch Frank wrote: >>>>>> We need to properly implement interrupt handling for SCLP, because on >>>>>> z/VM and LPAR SCLP calls are not synchronous! >>>>>> >>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> [...] >>>>>> + >>>>>> static void sclp_read_scp_info(ReadInfo *ri, int length) >>>>>> { >>>>>> unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, >>>>>> SCLP_CMDW_READ_SCP_INFO }; >>>>>> - int i; >>>>>> + int i, cc; >>>>>> >>>>>> for (i = 0; i < ARRAY_SIZE(commands); i++) { >>>>>> memset(&ri->h, 0, sizeof(ri->h)); >>>>>> ri->h.length = length; >>>>>> >>>>>> - if (sclp_service_call(commands[i], ri)) >>>>>> + sclp_mark_busy(); >>>>>> + cc = sclp_service_call(commands[i], ri); >>>>>> + sclp_wait_busy(); >>>>> >>>>> You already do the sclp_wait_busy() in sclp_service_call now, so I think >>>>> you don't need the sclp_wait_busy() here anymore? >>>> >>>> Yeah, that has to go. >>>> >>>>> >>>>> Also, what about moving the sclp_mark_busy() calls to the beginning of >>>>> sclp_service_call() instead? >>>> >>>> Wouldn't that create a race on the data of __sccb and we could end with >>>> garbled scb commands? >>> >>> Since there is a sclp_wait_busy in sclp_service_call already, you can be >>> sure that the previous command already finished, can't you? >> >> I mean before calling sclp_service_call. >> That's only a problem for smp, but before calling sclp, we prepare the >> data in the shared __sccb page and that is currently protected by the >> busy mark. If it's not, then two threads could write to __sccb at the >> same time and the first that succeeds to call sclp will get a mix of >> data of both threads. > > But in that case, you also need to do a sclp_wait_busy() before calling > sclp_mark_busy() in all locations - otherwise the code is also not > thread-safe in its current shape, is it? Ah, never mind, now I had a look at patch 11/13, and now it makes sense. So maybe fold patch 11 into this one, to make this clear right from the start? Thomas
On 03.01.19 14:37, Thomas Huth wrote: > On 2019-01-03 14:33, Thomas Huth wrote: >> On 2019-01-03 14:23, Janosch Frank wrote: >>> On 03.01.19 14:15, Thomas Huth wrote: >>>> On 2019-01-03 14:13, Janosch Frank wrote: >>>>> On 03.01.19 13:58, Thomas Huth wrote: >>>>>> On 2019-01-03 11:08, Janosch Frank wrote: >>>>>>> We need to properly implement interrupt handling for SCLP, because on >>>>>>> z/VM and LPAR SCLP calls are not synchronous! >>>>>>> >>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>> [...] >>>>>>> + >>>>>>> static void sclp_read_scp_info(ReadInfo *ri, int length) >>>>>>> { >>>>>>> unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, >>>>>>> SCLP_CMDW_READ_SCP_INFO }; >>>>>>> - int i; >>>>>>> + int i, cc; >>>>>>> >>>>>>> for (i = 0; i < ARRAY_SIZE(commands); i++) { >>>>>>> memset(&ri->h, 0, sizeof(ri->h)); >>>>>>> ri->h.length = length; >>>>>>> >>>>>>> - if (sclp_service_call(commands[i], ri)) >>>>>>> + sclp_mark_busy(); >>>>>>> + cc = sclp_service_call(commands[i], ri); >>>>>>> + sclp_wait_busy(); >>>>>> >>>>>> You already do the sclp_wait_busy() in sclp_service_call now, so I think >>>>>> you don't need the sclp_wait_busy() here anymore? >>>>> >>>>> Yeah, that has to go. >>>>> >>>>>> >>>>>> Also, what about moving the sclp_mark_busy() calls to the beginning of >>>>>> sclp_service_call() instead? >>>>> >>>>> Wouldn't that create a race on the data of __sccb and we could end with >>>>> garbled scb commands? >>>> >>>> Since there is a sclp_wait_busy in sclp_service_call already, you can be >>>> sure that the previous command already finished, can't you? >>> >>> I mean before calling sclp_service_call. >>> That's only a problem for smp, but before calling sclp, we prepare the >>> data in the shared __sccb page and that is currently protected by the >>> busy mark. If it's not, then two threads could write to __sccb at the >>> same time and the first that succeeds to call sclp will get a mix of >>> data of both threads. >> >> But in that case, you also need to do a sclp_wait_busy() before calling >> sclp_mark_busy() in all locations - otherwise the code is also not >> thread-safe in its current shape, is it? > > Ah, never mind, now I had a look at patch 11/13, and now it makes sense. > So maybe fold patch 11 into this one, to make this clear right from the > start? > > Thomas > Already done :)
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h index d2cd727..4bbb428 100644 --- a/lib/s390x/asm/arch_def.h +++ b/lib/s390x/asm/arch_def.h @@ -15,6 +15,7 @@ struct psw { uint64_t addr; }; +#define PSW_MASK_EXT 0x0100000000000000UL #define PSW_MASK_DAT 0x0400000000000000UL #define PSW_MASK_PSTATE 0x0001000000000000UL diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h index 013709f..f485e96 100644 --- a/lib/s390x/asm/interrupt.h +++ b/lib/s390x/asm/interrupt.h @@ -11,6 +11,8 @@ #define _ASMS390X_IRQ_H_ #include <asm/arch_def.h> +#define EXT_IRQ_SERVICE_SIG 0x2401 + void handle_pgm_int(void); void handle_ext_int(void); void handle_mcck_int(void); diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c index cf0a794..7832711 100644 --- a/lib/s390x/interrupt.c +++ b/lib/s390x/interrupt.c @@ -12,6 +12,7 @@ #include <libcflat.h> #include <asm/interrupt.h> #include <asm/barrier.h> +#include <sclp.h> static bool pgm_int_expected; static struct lowcore *lc; @@ -107,8 +108,15 @@ void handle_pgm_int(void) void handle_ext_int(void) { - report_abort("Unexpected external call interrupt: at %#lx", - lc->ext_old_psw.addr); + if (lc->ext_int_code != EXT_IRQ_SERVICE_SIG) { + report_abort("Unexpected external call interrupt: at %#lx", + lc->ext_old_psw.addr); + } else { + lc->ext_old_psw.mask &= ~PSW_MASK_EXT; + lc->sw_int_cr0 &= ~(1UL << 9); + sclp_handle_ext(); + lc->ext_int_code = 0; + } } void handle_mcck_int(void) diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c index bc01f41..a5ef45f 100644 --- a/lib/s390x/sclp-console.c +++ b/lib/s390x/sclp-console.c @@ -17,6 +17,7 @@ static void sclp_set_write_mask(void) { WriteEventMask *sccb = (void *)_sccb; + sclp_mark_busy(); sccb->h.length = sizeof(WriteEventMask); sccb->mask_length = sizeof(unsigned int); sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII; @@ -37,6 +38,7 @@ void sclp_print(const char *str) int len = strlen(str); WriteEventData *sccb = (void *)_sccb; + sclp_mark_busy(); sccb->h.length = sizeof(WriteEventData) + len; sccb->h.function_code = SCLP_FC_NORMAL_WRITE; sccb->ebh.length = sizeof(EventBufferHeader) + len; diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c index 7f556e5..817c692 100644 --- a/lib/s390x/sclp.c +++ b/lib/s390x/sclp.c @@ -14,6 +14,7 @@ #include <asm/page.h> #include <asm/arch_def.h> #include <asm/interrupt.h> +#include <asm/barrier.h> #include "sclp.h" #include <alloc_phys.h> @@ -24,6 +25,7 @@ static uint64_t max_ram_size; static uint64_t ram_size; char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096))); +static volatile bool sclp_busy; static void mem_init(phys_addr_t mem_end) { @@ -32,17 +34,48 @@ static void mem_init(phys_addr_t mem_end) phys_alloc_init(freemem_start, mem_end - freemem_start); } +static void sclp_setup_int(void) +{ + uint64_t mask; + + ctl_set_bit(0, 9); + + mask = extract_psw_mask(); + mask |= PSW_MASK_EXT; + load_psw_mask(mask); +} + +void sclp_handle_ext(void) +{ + ctl_clear_bit(0, 9); + sclp_busy = false; +} + +void sclp_wait_busy(void) +{ + while (sclp_busy) + mb(); +} + +void sclp_mark_busy(void) +{ + sclp_busy = true; +} + static void sclp_read_scp_info(ReadInfo *ri, int length) { unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, SCLP_CMDW_READ_SCP_INFO }; - int i; + int i, cc; for (i = 0; i < ARRAY_SIZE(commands); i++) { memset(&ri->h, 0, sizeof(ri->h)); ri->h.length = length; - if (sclp_service_call(commands[i], ri)) + sclp_mark_busy(); + cc = sclp_service_call(commands[i], ri); + sclp_wait_busy(); + if (cc) break; if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION) return; @@ -57,12 +90,14 @@ int sclp_service_call(unsigned int command, void *sccb) { int cc; + sclp_setup_int(); asm volatile( " .insn rre,0xb2200000,%1,%2\n" /* servc %1,%2 */ " ipm %0\n" " srl %0,28" : "=&d" (cc) : "d" (command), "a" (__pa(sccb)) : "cc", "memory"); + sclp_wait_busy(); if (cc == 3) return -1; if (cc == 2) diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h index 583c4e5..63cf609 100644 --- a/lib/s390x/sclp.h +++ b/lib/s390x/sclp.h @@ -213,6 +213,9 @@ typedef struct ReadEventData { } __attribute__((packed)) ReadEventData; extern char _sccb[]; +void sclp_handle_ext(void); +void sclp_wait_busy(void); +void sclp_mark_busy(void); void sclp_console_setup(void); void sclp_print(const char *str); int sclp_service_call(unsigned int command, void *sccb);
We need to properly implement interrupt handling for SCLP, because on z/VM and LPAR SCLP calls are not synchronous! Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- lib/s390x/asm/arch_def.h | 1 + lib/s390x/asm/interrupt.h | 2 ++ lib/s390x/interrupt.c | 12 ++++++++++-- lib/s390x/sclp-console.c | 2 ++ lib/s390x/sclp.c | 39 +++++++++++++++++++++++++++++++++++++-- lib/s390x/sclp.h | 3 +++ 6 files changed, 55 insertions(+), 4 deletions(-)