Re: [kvm-unit-tests PATCH v1 1/4] lib: s390x: add support for SCLP console read

[Nico Boehr <nrb@linux.ibm.com>]

On Tue, 2022-04-12 at 17:32 +0200, Janosch Frank wrote:
> On 4/11/22 12:07, Nico Boehr wrote:
> > Add a basic implementation for reading from the SCLP ACII console.
> > The goal of
> > this is to support migration tests on s390x. To know when the
> > migration has been
> > finished, we need to listen for a newline on our console.
> > 
> > Hence, this implementation is focused on the SCLP ASCII console of
> > QEMU and
> > currently won't work under e.g. LPAR.
> 
> How much pain would it be to add the line mode read?

I am not terribly familiar with the line mode, but I can say it would
make the implementation of the ASCII console more complex. Right now we
can just assume there will just be events from the ASCII console when
we read event data.

Not impossible to do, but I thought we don't need it so I kept things
simple. Is there some benefit we would have from the line mode console?

[...]
> >   
> > +static void sclp_console_enable_read(void)
> > +{
> > +       sclp_write_event_mask(SCLP_EVENT_MASK_MSG_ASCII,
> > SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG);
> > +}
> > +
> > +static void sclp_console_disable_read(void)
> > +{
> > +       sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII |
> > SCLP_EVENT_MASK_MSG);
> > +}
> > +
> >   void sclp_console_setup(void)
> >   {
> > -       sclp_set_write_mask();
> > +       /* We send ASCII and line mode. */
> > +       sclp_write_event_mask(0, SCLP_EVENT_MASK_MSG_ASCII |
> > SCLP_EVENT_MASK_MSG);
> >   }
> >   
> >   void sclp_print(const char *str)
> > @@ -227,3 +240,59 @@ void sclp_print(const char *str)
> >         sclp_print_ascii(str);
> >         sclp_print_lm(str);
> >   }
> > +
> > +#define SCLP_EVENT_ASCII_DATA_STREAM_FOLLOWS 0
> 
> -> sclp.h

Yes, thanks.

> 
> > +
> > +static int console_refill_read_buffer(void)
> > +{
> > +       const int MAX_EVENT_BUFFER_LEN = SCCB_SIZE -
> > offsetof(ReadEventDataAsciiConsole, ebh);
> > +       ReadEventDataAsciiConsole *sccb = (void *)_sccb;
> > +       const int EVENT_BUFFER_ASCII_RECV_HEADER_LEN = sizeof(sccb-
> > >ebh) + sizeof(sccb->type);
> > +       int ret = -1;
> 
> Reverse Christmas tree

Hm, I think it's not possible for EVENT_BUFFER_ASCII_RECV_HEADER_LEN
because it needs sccb first. I would want to leave as-is except if you
have a better idea on how to do this?

> The const int variables are all caps because they are essentially
> constants?

Yes, that was my reasoning. But it is uncommon in kvm-unit-test to have
it uppercase, all const ints in the codebase are lowercase, so I will
lowercase it.

> > +
> > +       sclp_console_enable_read();
> > +
> > +       sclp_mark_busy();
> > +       memset(sccb, 0, 4096);
> 
> sizeof(*sccb)

If you are OK with it, I would prefer to use SCCB_SIZE, s.t. the entire
buffer is cleared.