| Message ID | 1518818879-18608-12-git-send-email-walling@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/16/2018 11:07 PM, Collin L. Walling wrote: > It is possible while waiting for multiple types of external > interrupts that we might have pending irqs remaining between > irq consumption and irq-type disabling. Those interrupts > could potentially propagate to the guest after IPL completes > and cause unwanted behavior. > > As it is today, the SCLP will only recognize write events that > are enabled by the control program's send and receive masks. To > limit the window for, and prevent further irqs from, ASCII > console events (specifically keystrokes), we should only enable > the control program's receive mask when we need it. > > As an additional measure, once we've disabled/cleared the control > program's receive mask, we will print an empty string in order > to consume any pending service interrupt. > > Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> This should work some comments below: > --- > pc-bios/s390-ccw/menu.c | 5 +++++ > pc-bios/s390-ccw/s390-ccw.h | 1 + > pc-bios/s390-ccw/sclp.c | 10 ++++------ > 3 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c > index 9601043..14410a8 100644 > --- a/pc-bios/s390-ccw/menu.c > +++ b/pc-bios/s390-ccw/menu.c > @@ -11,6 +11,7 @@ > > #include "menu.h" > #include "s390-ccw.h" > +#include "sclp.h" > > #define KEYCODE_NO_INP '\0' > #define KEYCODE_ESCAPE '\033' > @@ -117,8 +118,12 @@ static int get_index(void) > > memset(buf, 0, sizeof(buf)); > > + sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII); > len = read_prompt(buf, sizeof(buf)); > > + sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII); > + sclp_print(""); /* Clear any pending service int */ > + Why cant you use consume_sclp_int from start.S and not do any printing? Shouldnt sclp_set_write_mask always make an interrupt pending? > /* If no input, boot default */ > if (len == 0) { > return 0; > diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h > index a7e6253..ebdcf86 100644 > --- a/pc-bios/s390-ccw/s390-ccw.h > +++ b/pc-bios/s390-ccw/s390-ccw.h > @@ -69,6 +69,7 @@ unsigned int get_loadparm_index(void); > > /* sclp.c */ > void sclp_print(const char *string); > +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask); > void sclp_setup(void); > void sclp_get_loadparm_ascii(char *loadparm); > int sclp_read(char *str, size_t count); > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index a2f25eb..3836cb4 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -46,23 +46,21 @@ static int sclp_service_call(unsigned int command, void *sccb) > return 0; > } > > -static void sclp_set_write_mask(void) > +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask) > { > WriteEventMask *sccb = (void *)_sccb; > > sccb->h.length = sizeof(WriteEventMask); > sccb->mask_length = sizeof(unsigned int); > - sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII; > - sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII; > - sccb->send_mask = SCLP_EVENT_MASK_MSG_ASCII; > - sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII; > + sccb->cp_receive_mask = receive_mask; > + sccb->cp_send_mask = send_mask; Isnt this also fixing another issue. write event mask only takes the control program (the guest) values and the sclp mask are being returned. > > sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb); > } > > void sclp_setup(void) > { > - sclp_set_write_mask(); > + sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII); > } > > long write(int fd, const void *str, size_t len) >
On 02/19/2018 03:15 PM, Christian Borntraeger wrote: > > > On 02/16/2018 11:07 PM, Collin L. Walling wrote: >> It is possible while waiting for multiple types of external >> interrupts that we might have pending irqs remaining between >> irq consumption and irq-type disabling. Those interrupts >> could potentially propagate to the guest after IPL completes >> and cause unwanted behavior. >> >> As it is today, the SCLP will only recognize write events that >> are enabled by the control program's send and receive masks. To >> limit the window for, and prevent further irqs from, ASCII >> console events (specifically keystrokes), we should only enable >> the control program's receive mask when we need it. >> >> As an additional measure, once we've disabled/cleared the control >> program's receive mask, we will print an empty string in order >> to consume any pending service interrupt. >> >> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> > > This should work some comments below: > >> --- >> pc-bios/s390-ccw/menu.c | 5 +++++ >> pc-bios/s390-ccw/s390-ccw.h | 1 + >> pc-bios/s390-ccw/sclp.c | 10 ++++------ >> 3 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c >> index 9601043..14410a8 100644 >> --- a/pc-bios/s390-ccw/menu.c >> +++ b/pc-bios/s390-ccw/menu.c >> @@ -11,6 +11,7 @@ >> >> #include "menu.h" >> #include "s390-ccw.h" >> +#include "sclp.h" >> >> #define KEYCODE_NO_INP '\0' >> #define KEYCODE_ESCAPE '\033' >> @@ -117,8 +118,12 @@ static int get_index(void) >> >> memset(buf, 0, sizeof(buf)); >> >> + sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII); >> len = read_prompt(buf, sizeof(buf)); >> >> + sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII); >> + sclp_print(""); /* Clear any pending service int */ >> + > > Why cant you use consume_sclp_int from start.S and not do any printing? > Shouldnt sclp_set_write_mask always make an interrupt pending? In fact sclp_set_write_mask should already call consume_sclp_int. Have you seen spurious interrupts?
On 02/19/2018 09:15 AM, Christian Borntraeger wrote: > > On 02/16/2018 11:07 PM, Collin L. Walling wrote: >> It is possible while waiting for multiple types of external >> interrupts that we might have pending irqs remaining between >> irq consumption and irq-type disabling. Those interrupts >> could potentially propagate to the guest after IPL completes >> and cause unwanted behavior. >> >> As it is today, the SCLP will only recognize write events that >> are enabled by the control program's send and receive masks. To >> limit the window for, and prevent further irqs from, ASCII >> console events (specifically keystrokes), we should only enable >> the control program's receive mask when we need it. >> >> As an additional measure, once we've disabled/cleared the control >> program's receive mask, we will print an empty string in order >> to consume any pending service interrupt. >> >> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> > This should work some comments below: > >> --- >> pc-bios/s390-ccw/menu.c | 5 +++++ >> pc-bios/s390-ccw/s390-ccw.h | 1 + >> pc-bios/s390-ccw/sclp.c | 10 ++++------ >> 3 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c >> index 9601043..14410a8 100644 >> --- a/pc-bios/s390-ccw/menu.c >> +++ b/pc-bios/s390-ccw/menu.c >> @@ -11,6 +11,7 @@ >> >> #include "menu.h" >> #include "s390-ccw.h" >> +#include "sclp.h" >> >> #define KEYCODE_NO_INP '\0' >> #define KEYCODE_ESCAPE '\033' >> @@ -117,8 +118,12 @@ static int get_index(void) >> >> memset(buf, 0, sizeof(buf)); >> >> + sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII); >> len = read_prompt(buf, sizeof(buf)); >> >> + sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII); >> + sclp_print(""); /* Clear any pending service int */ >> + > Why cant you use consume_sclp_int from start.S and not do any printing? > Shouldnt sclp_set_write_mask always make an interrupt pending? You're correct. Setting the mask should solve the issue without the need for a print. > > >> /* If no input, boot default */ >> if (len == 0) { >> return 0; >> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h >> index a7e6253..ebdcf86 100644 >> --- a/pc-bios/s390-ccw/s390-ccw.h >> +++ b/pc-bios/s390-ccw/s390-ccw.h >> @@ -69,6 +69,7 @@ unsigned int get_loadparm_index(void); >> >> /* sclp.c */ >> void sclp_print(const char *string); >> +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask); >> void sclp_setup(void); >> void sclp_get_loadparm_ascii(char *loadparm); >> int sclp_read(char *str, size_t count); >> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >> index a2f25eb..3836cb4 100644 >> --- a/pc-bios/s390-ccw/sclp.c >> +++ b/pc-bios/s390-ccw/sclp.c >> @@ -46,23 +46,21 @@ static int sclp_service_call(unsigned int command, void *sccb) >> return 0; >> } >> >> -static void sclp_set_write_mask(void) >> +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask) >> { >> WriteEventMask *sccb = (void *)_sccb; >> >> sccb->h.length = sizeof(WriteEventMask); >> sccb->mask_length = sizeof(unsigned int); >> - sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII; >> - sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII; >> - sccb->send_mask = SCLP_EVENT_MASK_MSG_ASCII; >> - sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII; >> + sccb->cp_receive_mask = receive_mask; >> + sccb->cp_send_mask = send_mask; > Isnt this also fixing another issue. write event mask only takes the > control program (the guest) values and the sclp mask are being returned. Correct. I'll briefly describe this change in the commit message as well. > >> sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb); >> } >> >> void sclp_setup(void) >> { >> - sclp_set_write_mask(); >> + sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII); >> } >> >> long write(int fd, const void *str, size_t len) >> >
On 02/19/2018 09:17 AM, Christian Borntraeger wrote: > > On 02/19/2018 03:15 PM, Christian Borntraeger wrote: >> >> On 02/16/2018 11:07 PM, Collin L. Walling wrote: >>> It is possible while waiting for multiple types of external >>> interrupts that we might have pending irqs remaining between >>> irq consumption and irq-type disabling. Those interrupts >>> could potentially propagate to the guest after IPL completes >>> and cause unwanted behavior. >>> >>> As it is today, the SCLP will only recognize write events that >>> are enabled by the control program's send and receive masks. To >>> limit the window for, and prevent further irqs from, ASCII >>> console events (specifically keystrokes), we should only enable >>> the control program's receive mask when we need it. >>> >>> As an additional measure, once we've disabled/cleared the control >>> program's receive mask, we will print an empty string in order >>> to consume any pending service interrupt. >>> >>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> >> This should work some comments below: >> >>> --- >>> pc-bios/s390-ccw/menu.c | 5 +++++ >>> pc-bios/s390-ccw/s390-ccw.h | 1 + >>> pc-bios/s390-ccw/sclp.c | 10 ++++------ >>> 3 files changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c >>> index 9601043..14410a8 100644 >>> --- a/pc-bios/s390-ccw/menu.c >>> +++ b/pc-bios/s390-ccw/menu.c >>> @@ -11,6 +11,7 @@ >>> >>> #include "menu.h" >>> #include "s390-ccw.h" >>> +#include "sclp.h" >>> >>> #define KEYCODE_NO_INP '\0' >>> #define KEYCODE_ESCAPE '\033' >>> @@ -117,8 +118,12 @@ static int get_index(void) >>> >>> memset(buf, 0, sizeof(buf)); >>> >>> + sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII); >>> len = read_prompt(buf, sizeof(buf)); >>> >>> + sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII); >>> + sclp_print(""); /* Clear any pending service int */ >>> + >> Why cant you use consume_sclp_int from start.S and not do any printing? >> Shouldnt sclp_set_write_mask always make an interrupt pending? > In fact sclp_set_write_mask should already call consume_sclp_int. Have you seen spurious > interrupts? > > Correct. We don't need that print there. Setting the mask kills two birds with one stone.
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c index 9601043..14410a8 100644 --- a/pc-bios/s390-ccw/menu.c +++ b/pc-bios/s390-ccw/menu.c @@ -11,6 +11,7 @@ #include "menu.h" #include "s390-ccw.h" +#include "sclp.h" #define KEYCODE_NO_INP '\0' #define KEYCODE_ESCAPE '\033' @@ -117,8 +118,12 @@ static int get_index(void) memset(buf, 0, sizeof(buf)); + sclp_set_write_mask(SCLP_EVENT_MASK_MSG_ASCII, SCLP_EVENT_MASK_MSG_ASCII); len = read_prompt(buf, sizeof(buf)); + sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII); + sclp_print(""); /* Clear any pending service int */ + /* If no input, boot default */ if (len == 0) { return 0; diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h index a7e6253..ebdcf86 100644 --- a/pc-bios/s390-ccw/s390-ccw.h +++ b/pc-bios/s390-ccw/s390-ccw.h @@ -69,6 +69,7 @@ unsigned int get_loadparm_index(void); /* sclp.c */ void sclp_print(const char *string); +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask); void sclp_setup(void); void sclp_get_loadparm_ascii(char *loadparm); int sclp_read(char *str, size_t count); diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c index a2f25eb..3836cb4 100644 --- a/pc-bios/s390-ccw/sclp.c +++ b/pc-bios/s390-ccw/sclp.c @@ -46,23 +46,21 @@ static int sclp_service_call(unsigned int command, void *sccb) return 0; } -static void sclp_set_write_mask(void) +void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask) { WriteEventMask *sccb = (void *)_sccb; sccb->h.length = sizeof(WriteEventMask); sccb->mask_length = sizeof(unsigned int); - sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII; - sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII; - sccb->send_mask = SCLP_EVENT_MASK_MSG_ASCII; - sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII; + sccb->cp_receive_mask = receive_mask; + sccb->cp_send_mask = send_mask; sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb); } void sclp_setup(void) { - sclp_set_write_mask(); + sclp_set_write_mask(0, SCLP_EVENT_MASK_MSG_ASCII); } long write(int fd, const void *str, size_t len)
It is possible while waiting for multiple types of external interrupts that we might have pending irqs remaining between irq consumption and irq-type disabling. Those interrupts could potentially propagate to the guest after IPL completes and cause unwanted behavior. As it is today, the SCLP will only recognize write events that are enabled by the control program's send and receive masks. To limit the window for, and prevent further irqs from, ASCII console events (specifically keystrokes), we should only enable the control program's receive mask when we need it. As an additional measure, once we've disabled/cleared the control program's receive mask, we will print an empty string in order to consume any pending service interrupt. Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> --- pc-bios/s390-ccw/menu.c | 5 +++++ pc-bios/s390-ccw/s390-ccw.h | 1 + pc-bios/s390-ccw/sclp.c | 10 ++++------ 3 files changed, 10 insertions(+), 6 deletions(-)