Re: [Qemu-devel] [PATCH v7 11/12] s390-ccw: set cp_receive mask only when needed and consume pending service irqs

[Christian Borntraeger]

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 <address@hidden>

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)
>