Re: [Qemu-devel] [PATCH v1 1/3] s390x/sclp: proper support of larger send and receive masks

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 21 Feb 2018 17:28:49 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> On Wed, 21 Feb 2018 16:12:59 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 20 Feb 2018 19:45:00 +0100
> > Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> >   
> > > The architecture allows the guests to ask for masks up to 1021
> > > bytes in length. Part was fixed in
> > > 67915de9f0383ccf4ab8c42dd02aa18dcd79b411 ("s390x/event-facility:
> > > variable-length event masks"), but some issues were still
> > > remaining, in particular regarding the handling of selective reads.
> > > 
> > > This patch fixes the handling of selective reads, whose size will
> > > now match the length of the event mask, as per architecture.
> > > 
> > > The default behaviour is to be compliant with the architecture, but
> > > when using older machine models the old behaviour is selected, in
> > > order to be able to migrate toward older versions.    
> > 
> > I remember trying to do this back when I still had access to the
> > architecture, but somehow never finished this (don't remember why).
> >   
> > > 
> > > Fixes: 67915de9f0383ccf4a ("s390x/event-facility: variable-length
> > > event masks")    
> > 
> > Doesn't that rather fix the initial implementation? What am I missing?  
> 
> well that too. but I think this fixes the fix, since the fix was
> incomplete?
> 
> > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> > > ---
> > >  hw/s390x/event-facility.c  | 90
> > > +++++++++++++++++++++++++++++++++++++++-------
> > > hw/s390x/s390-virtio-ccw.c |  8 ++++- 2 files changed, 84
> > > insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> > > index 155a694..2414614 100644
> > > --- a/hw/s390x/event-facility.c
> > > +++ b/hw/s390x/event-facility.c
> > > @@ -31,6 +31,14 @@ struct SCLPEventFacility {
> > >      SCLPEventsBus sbus;
> > >      /* guest' receive mask */
> > >      unsigned int receive_mask;
> > > +    /*
> > > +     * when false, we keep the same broken, backwards compatible
> > > behaviour as
> > > +     * before; when true, we implement the architecture correctly.
> > > Needed for
> > > +     * migration toward older versions.
> > > +     */
> > > +    bool allow_all_mask_sizes;    
> > 
> > The comment does not really tell us what the old behaviour is ;)  
> 
> hmm, I'll fix that
> 
> > So, if this is about extending the mask size, call this
> > "allow_large_masks" or so?  
> 
> no, the old broken behaviour allowed only _exactly_ 4 bytes:
> 
> if (be16_to_cpu(we_mask->mask_length) != 4) {
>      sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH);
>      goto out;
> }

Hm, I can't seem to find this in the code?

> 
> With the 67915de9f0383ccf4a patch mentioned above, we allow any size,
> but we don't keep track of the size itself, only the bits. This is a
> problem for selective reads (see below)

Oh, you meant before that patch...

> 
> > > +    /* length of the receive mask */
> > > +    uint16_t mask_length;
> > >  };
> > >  
> > >  /* return true if any child has event pending set */
> > > @@ -220,6 +228,17 @@ static uint16_t
> > > handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb, return
> > > rc; }
> > >  
> > > +/* copy up to dst_len bytes and fill the rest of dst with zeroes */
> > > +static void copy_mask(uint8_t *dst, uint8_t *src, uint16_t dst_len,
> > > +                      uint16_t src_len)
> > > +{
> > > +    int i;
> > > +
> > > +    for (i = 0; i < dst_len; i++) {
> > > +        dst[i] = i < src_len ? src[i] : 0;
> > > +    }
> > > +}
> > > +
> > >  static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
> > >  {
> > >      unsigned int sclp_active_selection_mask;
> > > @@ -240,7 +259,9 @@ static void read_event_data(SCLPEventFacility
> > > *ef, SCCB *sccb) sclp_active_selection_mask = sclp_cp_receive_mask;
> > >          break;
> > >      case SCLP_SELECTIVE_READ:
> > > -        sclp_active_selection_mask = be32_to_cpu(red->mask);
> > > +        copy_mask((uint8_t *)&sclp_active_selection_mask, (uint8_t
> > > *)&red->mask,
> > > +                  sizeof(sclp_active_selection_mask),
> > > ef->mask_length);
> > > +        sclp_active_selection_mask =
> > > be32_to_cpu(sclp_active_selection_mask);    
> > 
> > Hm, this looks like a real bug fix for the commit referenced above.
> > Split this out? We don't need compat support for that; maybe even
> > cc:stable?  
> 
> I think we do. We can avoid keeping track of the mask size when setting
> the mask size, because we can simply take the bits we need and ignore
> the rest. But for selective reads we need the mask size, so we have to
> keep track of it. (selective reads specify a mask, but not a mask
> length, the mask length is the length of the last mask set)

OK, that's non-obvious without documentation :/

> 
> And now we have additional state that we need to copy around when
> migrating. And we don't want to break older machines! Moreover a
> "new" guest started on a new qemu but with older machine version should
> still be limited to 4 bytes, so we can migrate it to an actual older
> version of qemu.
> 
> If a "new" guest uses a larger (or shorter!) mask then we can't migrate
> it back to an older version of qemu. New guests that support masks of
> size != 4 also still need to support the case where only size == 4 is
> allowed, otherwise they will not work at all on actual old versions of
> qemu.
> 
> So fixing selective reads needs compat support. 

Agreed.

> 
> > (Not sure what the consequences are here, other than unwanted bits at
> > the end; can't say without doc.)  
> 
> yes: if the mask is longer, we ignore bits, and we should give back an
> error if selective read asks for bits that are not in the receive mask.
> 
> If the mask is shorter, we risk considering bits that we are instead
> supposed to ignore. 
> 
> > >          if (!sclp_cp_receive_mask ||
> > >              (sclp_active_selection_mask & ~sclp_cp_receive_mask)) {
> > >              sccb->h.response_code =
> > > @@ -259,24 +280,14 @@ out:
> > >      return;
> > >  }
> > >  
> > > -/* copy up to dst_len bytes and fill the rest of dst with zeroes */
> > > -static void copy_mask(uint8_t *dst, uint8_t *src, uint16_t dst_len,
> > > -                      uint16_t src_len)
> > > -{
> > > -    int i;
> > > -
> > > -    for (i = 0; i < dst_len; i++) {
> > > -        dst[i] = i < src_len ? src[i] : 0;
> > > -    }
> > > -}
> > > -
> > >  static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
> > >  {
> > >      WriteEventMask *we_mask = (WriteEventMask *) sccb;
> > >      uint16_t mask_length = be16_to_cpu(we_mask->mask_length);
> > >      uint32_t tmp_mask;
> > >  
> > > -    if (!mask_length || (mask_length > SCLP_EVENT_MASK_LEN_MAX)) {
> > > +    if (!mask_length || (mask_length > SCLP_EVENT_MASK_LEN_MAX) ||
> > > +        ((mask_length != 4) && !ef->allow_all_mask_sizes)) {    
> > 
> > This is a behaviour change, isn't it? Previously, we allowed up to 4
> > bytes, now we allow exactly 4 bytes?  
> 
> no, we allowed only exactly 4 bytes, see above :)

This really needs more explanation in the patch description. Much of
this is really hard to understand without either that or access to the
documentation.

> 
> > >          sccb->h.response_code =
> > > cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH); goto out;
> > >      }    
> >   
>