On Wed, 21 Feb 2018 17:28:49 +0100 Claudio Imbrenda <imbre...@linux.vnet.ibm.com> wrote:
> On Wed, 21 Feb 2018 16:12:59 +0100 > Cornelia Huck <coh...@redhat.com> wrote: > > > On Tue, 20 Feb 2018 19:45:00 +0100 > > Claudio Imbrenda <imbre...@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 <imbre...@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; > > > } > > >