| Message ID | 1519152302-19079-2-git-send-email-imbrenda@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/sclp: 64 bit event masks | expand |
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? > 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 ;) So, if this is about extending the mask size, call this "allow_large_masks" or so? > + /* 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? (Not sure what the consequences are here, other than unwanted bits at the end; can't say without doc.) > 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? > sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH); > goto out; > }
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; } 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) > > + /* 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) 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. > (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 :) > > sccb->h.response_code = > > cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH); goto out; > > } >
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; > > > } > > >
On Wed, 21 Feb 2018 18:06:36 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > 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... yes > > > > > > + /* 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 :/ I'll put some more comments, maybe add some details in the patch description too > > > > 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. will do > > > > > > sccb->h.response_code = > > > > cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH); goto out; > > > > } > > > > > >
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; + /* 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); 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)) { sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH); goto out; } @@ -301,6 +312,7 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb) mask_length, sizeof(tmp_mask)); sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION); + ef->mask_length = mask_length; out: return; @@ -356,6 +368,34 @@ static void command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code) } } +static bool vmstate_event_facility_mask_length_needed(void *opaque) +{ + SCLPEventFacility *ef = opaque; + + return ef->allow_all_mask_sizes; +} + +static int vmstate_event_facility_mask_length_pre_load(void *opaque) +{ + SCLPEventFacility *ef = opaque; + + ef->allow_all_mask_sizes = false; + return 0; +} + +static const VMStateDescription vmstate_event_facility_mask_length = { + .name = "vmstate-event-facility/mask_length", + .version_id = 0, + .minimum_version_id = 0, + .needed = vmstate_event_facility_mask_length_needed, + .pre_load = vmstate_event_facility_mask_length_pre_load, + .fields = (VMStateField[]) { + VMSTATE_BOOL(allow_all_mask_sizes, SCLPEventFacility), + VMSTATE_UINT16(mask_length, SCLPEventFacility), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_event_facility = { .name = "vmstate-event-facility", .version_id = 0, @@ -363,15 +403,39 @@ static const VMStateDescription vmstate_event_facility = { .fields = (VMStateField[]) { VMSTATE_UINT32(receive_mask, SCLPEventFacility), VMSTATE_END_OF_LIST() + }, + .subsections = (const VMStateDescription * []) { + &vmstate_event_facility_mask_length, + NULL } }; +static void sclp_event_set_allow_all_mask_sizes(Object *obj, bool value, + Error **errp) +{ + SCLPEventFacility *ef = (SCLPEventFacility *)obj; + + ef->allow_all_mask_sizes = value; +} + +static bool sclp_event_get_allow_all_mask_sizes(Object *obj, Error **e) +{ + SCLPEventFacility *ef = (SCLPEventFacility *)obj; + + return ef->allow_all_mask_sizes; +} + static void init_event_facility(Object *obj) { SCLPEventFacility *event_facility = EVENT_FACILITY(obj); DeviceState *sdev = DEVICE(obj); Object *new; + event_facility->mask_length = 4; + event_facility->allow_all_mask_sizes = true; + object_property_add_bool(obj, "allow_all_mask_sizes", + sclp_event_get_allow_all_mask_sizes, + sclp_event_set_allow_all_mask_sizes, NULL); /* Spawn a new bus for SCLP events */ qbus_create_inplace(&event_facility->sbus, sizeof(event_facility->sbus), TYPE_SCLP_EVENTS_BUS, sdev, NULL); diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 8b3053f..e9309fd 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -29,6 +29,7 @@ #include "s390-pci-bus.h" #include "hw/s390x/storage-keys.h" #include "hw/s390x/storage-attributes.h" +#include "hw/s390x/event-facility.h" #include "hw/compat.h" #include "ipl.h" #include "hw/s390x/s390-virtio-ccw.h" @@ -671,7 +672,12 @@ bool css_migration_enabled(void) type_init(ccw_machine_register_##suffix) #define CCW_COMPAT_2_11 \ - HW_COMPAT_2_11 + HW_COMPAT_2_11 \ + {\ + .driver = TYPE_SCLP_EVENT_FACILITY,\ + .property = "allow_all_mask_sizes",\ + .value = "off",\ + }, #define CCW_COMPAT_2_10 \ HW_COMPAT_2_10
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. Fixes: 67915de9f0383ccf4a ("s390x/event-facility: variable-length event masks") 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(-)