[Qemu-devel] [PATCH v1 0/3] s390x/sclp: 64 bit event masks

[Qemu-devel] [PATCH v1 0/3] s390x/sclp: 64 bit event masks

[Claudio Imbrenda]

Until 67915de9f0383ccf4a ("s390x/event-facility: variable-length event masks")
we only supported 32bit sclp event masks, even though the archiecture
allows the guests to set up sclp event masks up to 1021 bytes in length.
With that patch the behaviour was almost compliant, but some issues were
still remaining, in particular regarding the handling of selective reads
and migration.

This patchset fixes migration and the handling of selective reads, and
puts in place the support for 64-bit sclp event masks internally.

A new property of the sclp-event device switches between the 32bit masks
and the compliant behaviour. The property is bound to the machine
version, so older machines keep the old broken behaviour, allowing for
migration, but the default is the compliant implementation.

Fixes: 67915de9f0383ccf4a ("s390x/event-facility: variable-length event masks")

Claudio Imbrenda (3):
  s390x/sclp: proper support of larger send and receive masks
  s390x/sclp: clean up sclp masks
  s390x/sclp: extend SCLP event masks to 64 bits

 hw/char/sclpconsole-lm.c          |   4 +-
 hw/char/sclpconsole.c             |   4 +-
 hw/s390x/event-facility.c         | 147 +++++++++++++++++++++++++++++++-------
 hw/s390x/s390-virtio-ccw.c        |   8 ++-
 hw/s390x/sclpcpu.c                |   4 +-
 hw/s390x/sclpquiesce.c            |   4 +-
 include/hw/s390x/event-facility.h |  22 +++---
 7 files changed, 149 insertions(+), 44 deletions(-)

-- 
2.7.4

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

[Claudio Imbrenda]

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

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

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

[Cornelia Huck]

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;
>      }

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

[Claudio Imbrenda]

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;
> >      }  
> 

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

[Cornelia Huck]

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;
> > >      }    
> >   
> 

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

[Claudio Imbrenda]

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;
> > > >      }      
> > >     
> >   
> 

[Qemu-devel] [PATCH v1 2/3] s390x/sclp: clean up sclp masks

[Claudio Imbrenda]

Clean up SCLP masks: introduce an sccb_mask_t to be used for SCLP event
masks instead of just unsigned int or uint32_t. This will allow later
to extend the mask with more ease.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
---
 hw/char/sclpconsole-lm.c          |  4 ++--
 hw/char/sclpconsole.c             |  4 ++--
 hw/s390x/event-facility.c         | 18 +++++++++---------
 hw/s390x/sclpcpu.c                |  4 ++--
 hw/s390x/sclpquiesce.c            |  4 ++--
 include/hw/s390x/event-facility.h | 22 +++++++++++++---------
 6 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
index c500bda..cc4d70a 100644
--- a/hw/char/sclpconsole-lm.c
+++ b/hw/char/sclpconsole-lm.c
@@ -102,12 +102,12 @@ static bool can_handle_event(uint8_t type)
     return type == SCLP_EVENT_MESSAGE || type == SCLP_EVENT_PMSGCMD;
 }
 
-static unsigned int send_mask(void)
+static sccb_mask_t send_mask(void)
 {
     return SCLP_EVENT_MASK_OP_CMD | SCLP_EVENT_MASK_PMSGCMD;
 }
 
-static unsigned int receive_mask(void)
+static sccb_mask_t receive_mask(void)
 {
     return SCLP_EVENT_MASK_MSG | SCLP_EVENT_MASK_PMSGCMD;
 }
diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
index d0265df..ec9db13 100644
--- a/hw/char/sclpconsole.c
+++ b/hw/char/sclpconsole.c
@@ -83,12 +83,12 @@ static bool can_handle_event(uint8_t type)
     return type == SCLP_EVENT_ASCII_CONSOLE_DATA;
 }
 
-static unsigned int send_mask(void)
+static sccb_mask_t send_mask(void)
 {
     return SCLP_EVENT_MASK_MSG_ASCII;
 }
 
-static unsigned int receive_mask(void)
+static sccb_mask_t receive_mask(void)
 {
     return SCLP_EVENT_MASK_MSG_ASCII;
 }
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 2414614..f6f28fd 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -30,7 +30,7 @@ struct SCLPEventFacility {
     SysBusDevice parent_obj;
     SCLPEventsBus sbus;
     /* guest' receive mask */
-    unsigned int receive_mask;
+    sccb_mask_t receive_mask;
     /*
      * when false, we keep the same broken, backwards compatible behaviour as
      * before; when true, we implement the architecture correctly. Needed for
@@ -60,9 +60,9 @@ static bool event_pending(SCLPEventFacility *ef)
     return false;
 }
 
-static unsigned int get_host_send_mask(SCLPEventFacility *ef)
+static sccb_mask_t get_host_send_mask(SCLPEventFacility *ef)
 {
-    unsigned int mask;
+    sccb_mask_t mask;
     BusChild *kid;
     SCLPEventClass *child;
 
@@ -76,9 +76,9 @@ static unsigned int get_host_send_mask(SCLPEventFacility *ef)
     return mask;
 }
 
-static unsigned int get_host_receive_mask(SCLPEventFacility *ef)
+static sccb_mask_t get_host_receive_mask(SCLPEventFacility *ef)
 {
-    unsigned int mask;
+    sccb_mask_t mask;
     BusChild *kid;
     SCLPEventClass *child;
 
@@ -188,7 +188,7 @@ out:
 }
 
 static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
-                                        unsigned int mask)
+                                        sccb_mask_t mask)
 {
     uint16_t rc;
     int slen;
@@ -241,8 +241,8 @@ static void copy_mask(uint8_t *dst, uint8_t *src, uint16_t dst_len,
 
 static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
 {
-    unsigned int sclp_active_selection_mask;
-    unsigned int sclp_cp_receive_mask;
+    sccb_mask_t sclp_active_selection_mask = 0;
+    sccb_mask_t sclp_cp_receive_mask;
 
     ReadEventData *red = (ReadEventData *) sccb;
 
@@ -284,7 +284,7 @@ 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;
+    sccb_mask_t tmp_mask = 0;
 
     if (!mask_length || (mask_length > SCLP_EVENT_MASK_LEN_MAX) ||
         ((mask_length != 4) && !ef->allow_all_mask_sizes)) {
diff --git a/hw/s390x/sclpcpu.c b/hw/s390x/sclpcpu.c
index 3ee890b..50c021b 100644
--- a/hw/s390x/sclpcpu.c
+++ b/hw/s390x/sclpcpu.c
@@ -37,12 +37,12 @@ void raise_irq_cpu_hotplug(void)
     sclp_service_interrupt(0);
 }
 
-static unsigned int send_mask(void)
+static sccb_mask_t send_mask(void)
 {
     return SCLP_EVENT_MASK_CONFIG_MGT_DATA;
 }
 
-static unsigned int receive_mask(void)
+static sccb_mask_t receive_mask(void)
 {
     return 0;
 }
diff --git a/hw/s390x/sclpquiesce.c b/hw/s390x/sclpquiesce.c
index 0241643..1c8f5c9 100644
--- a/hw/s390x/sclpquiesce.c
+++ b/hw/s390x/sclpquiesce.c
@@ -28,12 +28,12 @@ static bool can_handle_event(uint8_t type)
     return type == SCLP_EVENT_SIGNAL_QUIESCE;
 }
 
-static unsigned int send_mask(void)
+static sccb_mask_t send_mask(void)
 {
     return SCLP_EVENT_MASK_SIGNAL_QUIESCE;
 }
 
-static unsigned int receive_mask(void)
+static sccb_mask_t receive_mask(void)
 {
     return 0;
 }
diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
index 5119b9b..0a8b47a 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -28,12 +28,14 @@
 #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
 
 /* SCLP event masks */
-#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
-#define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
-#define SCLP_EVENT_MASK_CONFIG_MGT_DATA         0x10000000
-#define SCLP_EVENT_MASK_OP_CMD                  0x80000000
-#define SCLP_EVENT_MASK_MSG                     0x40000000
-#define SCLP_EVENT_MASK_PMSGCMD                 0x00800000
+#define SCLPEVMSK(T)  (1ULL << (sizeof(sccb_mask_t) * 8 - (T)))
+
+#define SCLP_EVENT_MASK_OP_CMD          SCLPEVMSK(SCLP_EVENT_OPRTNS_COMMAND)
+#define SCLP_EVENT_MASK_MSG             SCLPEVMSK(SCLP_EVENT_MESSAGE)
+#define SCLP_EVENT_MASK_CONFIG_MGT_DATA SCLPEVMSK(SCLP_EVENT_CONFIG_MGT_DATA)
+#define SCLP_EVENT_MASK_PMSGCMD         SCLPEVMSK(SCLP_EVENT_PMSGCMD)
+#define SCLP_EVENT_MASK_MSG_ASCII       SCLPEVMSK(SCLP_EVENT_ASCII_CONSOLE_DATA)
+#define SCLP_EVENT_MASK_SIGNAL_QUIESCE  SCLPEVMSK(SCLP_EVENT_SIGNAL_QUIESCE)
 
 #define SCLP_UNCONDITIONAL_READ                 0x00
 #define SCLP_SELECTIVE_READ                     0x01
@@ -71,6 +73,8 @@ typedef struct WriteEventMask {
 #define WEM_RECEIVE_MASK(wem, mask_len) ((wem)->masks + 2 * (mask_len))
 #define WEM_SEND_MASK(wem, mask_len) ((wem)->masks + 3 * (mask_len))
 
+typedef uint32_t sccb_mask_t;
+
 typedef struct EventBufferHeader {
     uint16_t length;
     uint8_t  type;
@@ -160,7 +164,7 @@ typedef struct WriteEventData {
 typedef struct ReadEventData {
     SCCBHeader h;
     union {
-        uint32_t mask;
+        sccb_mask_t mask;
         EventBufferHeader ebh;
     };
 } QEMU_PACKED ReadEventData;
@@ -177,10 +181,10 @@ typedef struct SCLPEventClass {
     int (*exit)(SCLPEvent *event);
 
     /* get SCLP's send mask */
-    unsigned int (*get_send_mask)(void);
+    sccb_mask_t (*get_send_mask)(void);
 
     /* get SCLP's receive mask */
-    unsigned int (*get_receive_mask)(void);
+    sccb_mask_t (*get_receive_mask)(void);
 
     int (*read_event_data)(SCLPEvent *event, EventBufferHeader *evt_buf_hdr,
                            int *slen);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v1 2/3] s390x/sclp: clean up sclp masks

[Cornelia Huck]

On Tue, 20 Feb 2018 19:45:01 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> Clean up SCLP masks: introduce an sccb_mask_t to be used for SCLP event
> masks instead of just unsigned int or uint32_t. This will allow later
> to extend the mask with more ease.

Looks mostly sane.

> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> ---
>  hw/char/sclpconsole-lm.c          |  4 ++--
>  hw/char/sclpconsole.c             |  4 ++--
>  hw/s390x/event-facility.c         | 18 +++++++++---------
>  hw/s390x/sclpcpu.c                |  4 ++--
>  hw/s390x/sclpquiesce.c            |  4 ++--
>  include/hw/s390x/event-facility.h | 22 +++++++++++++---------
>  6 files changed, 30 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index c500bda..cc4d70a 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c

> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 2414614..f6f28fd 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -30,7 +30,7 @@ struct SCLPEventFacility {
>      SysBusDevice parent_obj;
>      SCLPEventsBus sbus;
>      /* guest' receive mask */

Let's make this "guest's", as you're touching the line right below.

> -    unsigned int receive_mask;
> +    sccb_mask_t receive_mask;
>      /*
>       * when false, we keep the same broken, backwards compatible behaviour as
>       * before; when true, we implement the architecture correctly. Needed for

(...)

> @@ -241,8 +241,8 @@ static void copy_mask(uint8_t *dst, uint8_t *src, uint16_t dst_len,
>  
>  static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
>  {
> -    unsigned int sclp_active_selection_mask;
> -    unsigned int sclp_cp_receive_mask;
> +    sccb_mask_t sclp_active_selection_mask = 0;

Why do you need to initialize this now?

> +    sccb_mask_t sclp_cp_receive_mask;
>  
>      ReadEventData *red = (ReadEventData *) sccb;
>  
> @@ -284,7 +284,7 @@ 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;
> +    sccb_mask_t tmp_mask = 0;

Same here.

>  
>      if (!mask_length || (mask_length > SCLP_EVENT_MASK_LEN_MAX) ||
>          ((mask_length != 4) && !ef->allow_all_mask_sizes)) {

(...)

> diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
> index 5119b9b..0a8b47a 100644
> --- a/include/hw/s390x/event-facility.h
> +++ b/include/hw/s390x/event-facility.h
> @@ -28,12 +28,14 @@
>  #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
>  
>  /* SCLP event masks */
> -#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
> -#define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
> -#define SCLP_EVENT_MASK_CONFIG_MGT_DATA         0x10000000
> -#define SCLP_EVENT_MASK_OP_CMD                  0x80000000
> -#define SCLP_EVENT_MASK_MSG                     0x40000000
> -#define SCLP_EVENT_MASK_PMSGCMD                 0x00800000
> +#define SCLPEVMSK(T)  (1ULL << (sizeof(sccb_mask_t) * 8 - (T)))

SCLP_EVMASK() would be a bit more readable, I think.

> +
> +#define SCLP_EVENT_MASK_OP_CMD          SCLPEVMSK(SCLP_EVENT_OPRTNS_COMMAND)
> +#define SCLP_EVENT_MASK_MSG             SCLPEVMSK(SCLP_EVENT_MESSAGE)
> +#define SCLP_EVENT_MASK_CONFIG_MGT_DATA SCLPEVMSK(SCLP_EVENT_CONFIG_MGT_DATA)
> +#define SCLP_EVENT_MASK_PMSGCMD         SCLPEVMSK(SCLP_EVENT_PMSGCMD)
> +#define SCLP_EVENT_MASK_MSG_ASCII       SCLPEVMSK(SCLP_EVENT_ASCII_CONSOLE_DATA)
> +#define SCLP_EVENT_MASK_SIGNAL_QUIESCE  SCLPEVMSK(SCLP_EVENT_SIGNAL_QUIESCE)
>  
>  #define SCLP_UNCONDITIONAL_READ                 0x00
>  #define SCLP_SELECTIVE_READ                     0x01

Re: [Qemu-devel] [PATCH v1 2/3] s390x/sclp: clean up sclp masks

[Claudio Imbrenda]

On Wed, 21 Feb 2018 16:20:05 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 20 Feb 2018 19:45:01 +0100
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> 
> > Clean up SCLP masks: introduce an sccb_mask_t to be used for SCLP
> > event masks instead of just unsigned int or uint32_t. This will
> > allow later to extend the mask with more ease.  
> 
> Looks mostly sane.
> 
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> > ---
> >  hw/char/sclpconsole-lm.c          |  4 ++--
> >  hw/char/sclpconsole.c             |  4 ++--
> >  hw/s390x/event-facility.c         | 18 +++++++++---------
> >  hw/s390x/sclpcpu.c                |  4 ++--
> >  hw/s390x/sclpquiesce.c            |  4 ++--
> >  include/hw/s390x/event-facility.h | 22 +++++++++++++---------
> >  6 files changed, 30 insertions(+), 26 deletions(-)
> > 
> > diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> > index c500bda..cc4d70a 100644
> > --- a/hw/char/sclpconsole-lm.c
> > +++ b/hw/char/sclpconsole-lm.c  
> 
> > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> > index 2414614..f6f28fd 100644
> > --- a/hw/s390x/event-facility.c
> > +++ b/hw/s390x/event-facility.c
> > @@ -30,7 +30,7 @@ struct SCLPEventFacility {
> >      SysBusDevice parent_obj;
> >      SCLPEventsBus sbus;
> >      /* guest' receive mask */  
> 
> Let's make this "guest's", as you're touching the line right below.

will do

> > -    unsigned int receive_mask;
> > +    sccb_mask_t receive_mask;
> >      /*
> >       * when false, we keep the same broken, backwards compatible
> > behaviour as
> >       * before; when true, we implement the architecture correctly.
> > Needed for  
> 
> (...)
> 
> > @@ -241,8 +241,8 @@ static void copy_mask(uint8_t *dst, uint8_t
> > *src, uint16_t dst_len, 
> >  static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
> >  {
> > -    unsigned int sclp_active_selection_mask;
> > -    unsigned int sclp_cp_receive_mask;
> > +    sccb_mask_t sclp_active_selection_mask = 0;  
> 
> Why do you need to initialize this now?

right, copy_mask zeroes the unused space at the end of the destination,
it can be removed

> > +    sccb_mask_t sclp_cp_receive_mask;
> >  
> >      ReadEventData *red = (ReadEventData *) sccb;
> >  
> > @@ -284,7 +284,7 @@ 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;
> > +    sccb_mask_t tmp_mask = 0;  
> 
> Same here.

this is different, but it can still be avoided; I'll fix both.

> >  
> >      if (!mask_length || (mask_length > SCLP_EVENT_MASK_LEN_MAX) ||
> >          ((mask_length != 4) && !ef->allow_all_mask_sizes)) {  
> 
> (...)
> 
> > diff --git a/include/hw/s390x/event-facility.h
> > b/include/hw/s390x/event-facility.h index 5119b9b..0a8b47a 100644
> > --- a/include/hw/s390x/event-facility.h
> > +++ b/include/hw/s390x/event-facility.h
> > @@ -28,12 +28,14 @@
> >  #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
> >  
> >  /* SCLP event masks */
> > -#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
> > -#define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
> > -#define SCLP_EVENT_MASK_CONFIG_MGT_DATA         0x10000000
> > -#define SCLP_EVENT_MASK_OP_CMD                  0x80000000
> > -#define SCLP_EVENT_MASK_MSG                     0x40000000
> > -#define SCLP_EVENT_MASK_PMSGCMD                 0x00800000
> > +#define SCLPEVMSK(T)  (1ULL << (sizeof(sccb_mask_t) * 8 - (T)))  
> 
> SCLP_EVMASK() would be a bit more readable, I think.

I know, but then it looks ugly when trying to fit everything in 80
columns. 

> > +
> > +#define SCLP_EVENT_MASK_OP_CMD
> > SCLPEVMSK(SCLP_EVENT_OPRTNS_COMMAND) +#define
> > SCLP_EVENT_MASK_MSG             SCLPEVMSK(SCLP_EVENT_MESSAGE)
> > +#define SCLP_EVENT_MASK_CONFIG_MGT_DATA
> > SCLPEVMSK(SCLP_EVENT_CONFIG_MGT_DATA) +#define
> > SCLP_EVENT_MASK_PMSGCMD         SCLPEVMSK(SCLP_EVENT_PMSGCMD)
> > +#define SCLP_EVENT_MASK_MSG_ASCII
> > SCLPEVMSK(SCLP_EVENT_ASCII_CONSOLE_DATA) +#define
> > SCLP_EVENT_MASK_SIGNAL_QUIESCE
> > SCLPEVMSK(SCLP_EVENT_SIGNAL_QUIESCE) #define
> > SCLP_UNCONDITIONAL_READ                 0x00 #define
> > SCLP_SELECTIVE_READ                     0x01  
> 

Re: [Qemu-devel] [PATCH v1 2/3] s390x/sclp: clean up sclp masks

[Cornelia Huck]

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

> On Wed, 21 Feb 2018 16:20:05 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 20 Feb 2018 19:45:01 +0100
> > Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> > > diff --git a/include/hw/s390x/event-facility.h
> > > b/include/hw/s390x/event-facility.h index 5119b9b..0a8b47a 100644
> > > --- a/include/hw/s390x/event-facility.h
> > > +++ b/include/hw/s390x/event-facility.h
> > > @@ -28,12 +28,14 @@
> > >  #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
> > >  
> > >  /* SCLP event masks */
> > > -#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
> > > -#define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
> > > -#define SCLP_EVENT_MASK_CONFIG_MGT_DATA         0x10000000
> > > -#define SCLP_EVENT_MASK_OP_CMD                  0x80000000
> > > -#define SCLP_EVENT_MASK_MSG                     0x40000000
> > > -#define SCLP_EVENT_MASK_PMSGCMD                 0x00800000
> > > +#define SCLPEVMSK(T)  (1ULL << (sizeof(sccb_mask_t) * 8 - (T)))    
> > 
> > SCLP_EVMASK() would be a bit more readable, I think.  
> 
> I know, but then it looks ugly when trying to fit everything in 80
> columns. 

I'd rather go slightly over 80 in that case (as long as you don't cross
90).

> 
> > > +
> > > +#define SCLP_EVENT_MASK_OP_CMD
> > > SCLPEVMSK(SCLP_EVENT_OPRTNS_COMMAND) +#define
> > > SCLP_EVENT_MASK_MSG             SCLPEVMSK(SCLP_EVENT_MESSAGE)
> > > +#define SCLP_EVENT_MASK_CONFIG_MGT_DATA
> > > SCLPEVMSK(SCLP_EVENT_CONFIG_MGT_DATA) +#define
> > > SCLP_EVENT_MASK_PMSGCMD         SCLPEVMSK(SCLP_EVENT_PMSGCMD)
> > > +#define SCLP_EVENT_MASK_MSG_ASCII
> > > SCLPEVMSK(SCLP_EVENT_ASCII_CONSOLE_DATA) +#define
> > > SCLP_EVENT_MASK_SIGNAL_QUIESCE
> > > SCLPEVMSK(SCLP_EVENT_SIGNAL_QUIESCE) #define
> > > SCLP_UNCONDITIONAL_READ                 0x00 #define
> > > SCLP_SELECTIVE_READ                     0x01    
> >   
> 

Re: [Qemu-devel] [PATCH v1 2/3] s390x/sclp: clean up sclp masks

[Claudio Imbrenda]

On Wed, 21 Feb 2018 18:30:12 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 21 Feb 2018 17:42:57 +0100
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> 
> > On Wed, 21 Feb 2018 16:20:05 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Tue, 20 Feb 2018 19:45:01 +0100
> > > Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:  
> 
> > > > diff --git a/include/hw/s390x/event-facility.h
> > > > b/include/hw/s390x/event-facility.h index 5119b9b..0a8b47a
> > > > 100644 --- a/include/hw/s390x/event-facility.h
> > > > +++ b/include/hw/s390x/event-facility.h
> > > > @@ -28,12 +28,14 @@
> > > >  #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
> > > >  
> > > >  /* SCLP event masks */
> > > > -#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
> > > > -#define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
> > > > -#define SCLP_EVENT_MASK_CONFIG_MGT_DATA         0x10000000
> > > > -#define SCLP_EVENT_MASK_OP_CMD                  0x80000000
> > > > -#define SCLP_EVENT_MASK_MSG                     0x40000000
> > > > -#define SCLP_EVENT_MASK_PMSGCMD                 0x00800000
> > > > +#define SCLPEVMSK(T)  (1ULL << (sizeof(sccb_mask_t) * 8 -
> > > > (T)))      
> > > 
> > > SCLP_EVMASK() would be a bit more readable, I think.    
> > 
> > I know, but then it looks ugly when trying to fit everything in 80
> > columns.   
> 
> I'd rather go slightly over 80 in that case (as long as you don't
> cross 90).

will do

> >   
> > > > +
> > > > +#define SCLP_EVENT_MASK_OP_CMD
> > > > SCLPEVMSK(SCLP_EVENT_OPRTNS_COMMAND) +#define
> > > > SCLP_EVENT_MASK_MSG             SCLPEVMSK(SCLP_EVENT_MESSAGE)
> > > > +#define SCLP_EVENT_MASK_CONFIG_MGT_DATA
> > > > SCLPEVMSK(SCLP_EVENT_CONFIG_MGT_DATA) +#define
> > > > SCLP_EVENT_MASK_PMSGCMD         SCLPEVMSK(SCLP_EVENT_PMSGCMD)
> > > > +#define SCLP_EVENT_MASK_MSG_ASCII
> > > > SCLPEVMSK(SCLP_EVENT_ASCII_CONSOLE_DATA) +#define
> > > > SCLP_EVENT_MASK_SIGNAL_QUIESCE
> > > > SCLPEVMSK(SCLP_EVENT_SIGNAL_QUIESCE) #define
> > > > SCLP_UNCONDITIONAL_READ                 0x00 #define
> > > > SCLP_SELECTIVE_READ                     0x01      
> > >     
> >   
> 

[Qemu-devel] [PATCH v1 3/3] s390x/sclp: extend SCLP event masks to 64 bits

[Claudio Imbrenda]

Extend the SCLP event masks to 64 bits. This will make us future proof
against future extensions of the architecture.

Notice that using any of the new bits results in a state that cannot be
migrated to an older version.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
---
 hw/s390x/event-facility.c         | 43 +++++++++++++++++++++++++++++++++------
 include/hw/s390x/event-facility.h |  2 +-
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index f6f28fd..e71302a 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -30,7 +30,10 @@ struct SCLPEventFacility {
     SysBusDevice parent_obj;
     SCLPEventsBus sbus;
     /* guest' receive mask */
-    sccb_mask_t receive_mask;
+    union {
+        uint32_t receive_mask_compat32;
+        sccb_mask_t receive_mask;
+    };
     /*
      * when false, we keep the same broken, backwards compatible behaviour as
      * before; when true, we implement the architecture correctly. Needed for
@@ -261,7 +264,7 @@ static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
     case SCLP_SELECTIVE_READ:
         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);
+        sclp_active_selection_mask = be64_to_cpu(sclp_active_selection_mask);
         if (!sclp_cp_receive_mask ||
             (sclp_active_selection_mask & ~sclp_cp_receive_mask)) {
             sccb->h.response_code =
@@ -301,13 +304,13 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
     /* keep track of the guest's capability masks */
     copy_mask((uint8_t *)&tmp_mask, WEM_CP_RECEIVE_MASK(we_mask, mask_length),
               sizeof(tmp_mask), mask_length);
-    ef->receive_mask = be32_to_cpu(tmp_mask);
+    ef->receive_mask = be64_to_cpu(tmp_mask);
 
     /* return the SCLP's capability masks to the guest */
-    tmp_mask = cpu_to_be32(get_host_receive_mask(ef));
+    tmp_mask = cpu_to_be64(get_host_receive_mask(ef));
     copy_mask(WEM_RECEIVE_MASK(we_mask, mask_length), (uint8_t *)&tmp_mask,
               mask_length, sizeof(tmp_mask));
-    tmp_mask = cpu_to_be32(get_host_send_mask(ef));
+    tmp_mask = cpu_to_be64(get_host_send_mask(ef));
     copy_mask(WEM_SEND_MASK(we_mask, mask_length), (uint8_t *)&tmp_mask,
               mask_length, sizeof(tmp_mask));
 
@@ -368,6 +371,21 @@ static void command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code)
     }
 }
 
+static bool vmstate_event_facility_mask64_needed(void *opaque)
+{
+    SCLPEventFacility *ef = opaque;
+
+    return (ef->receive_mask & 0xFFFFFFFF) != 0;
+}
+
+static int vmstate_event_facility_mask64_pre_load(void *opaque)
+{
+    SCLPEventFacility *ef = opaque;
+
+    ef->receive_mask &= ~0xFFFFFFFFULL;
+    return 0;
+}
+
 static bool vmstate_event_facility_mask_length_needed(void *opaque)
 {
     SCLPEventFacility *ef = opaque;
@@ -383,6 +401,18 @@ static int vmstate_event_facility_mask_length_pre_load(void *opaque)
     return 0;
 }
 
+static const VMStateDescription vmstate_event_facility_mask64 = {
+    .name = "vmstate-event-facility/mask64",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .needed = vmstate_event_facility_mask64_needed,
+    .pre_load = vmstate_event_facility_mask64_pre_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(receive_mask, SCLPEventFacility),
+        VMSTATE_END_OF_LIST()
+     }
+};
+
 static const VMStateDescription vmstate_event_facility_mask_length = {
     .name = "vmstate-event-facility/mask_length",
     .version_id = 0,
@@ -401,10 +431,11 @@ static const VMStateDescription vmstate_event_facility = {
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(receive_mask, SCLPEventFacility),
+        VMSTATE_UINT32(receive_mask_compat32, SCLPEventFacility),
         VMSTATE_END_OF_LIST()
      },
     .subsections = (const VMStateDescription * []) {
+        &vmstate_event_facility_mask64,
         &vmstate_event_facility_mask_length,
         NULL
      }
diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
index 0a8b47a..e40c85f 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -73,7 +73,7 @@ typedef struct WriteEventMask {
 #define WEM_RECEIVE_MASK(wem, mask_len) ((wem)->masks + 2 * (mask_len))
 #define WEM_SEND_MASK(wem, mask_len) ((wem)->masks + 3 * (mask_len))
 
-typedef uint32_t sccb_mask_t;
+typedef uint64_t sccb_mask_t;
 
 typedef struct EventBufferHeader {
     uint16_t length;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v1 3/3] s390x/sclp: extend SCLP event masks to 64 bits

[Cornelia Huck]

On Tue, 20 Feb 2018 19:45:02 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> Extend the SCLP event masks to 64 bits. This will make us future proof
> against future extensions of the architecture.
> 
> Notice that using any of the new bits results in a state that cannot be
> migrated to an older version.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> ---
>  hw/s390x/event-facility.c         | 43 +++++++++++++++++++++++++++++++++------
>  include/hw/s390x/event-facility.h |  2 +-
>  2 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index f6f28fd..e71302a 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -30,7 +30,10 @@ struct SCLPEventFacility {
>      SysBusDevice parent_obj;
>      SCLPEventsBus sbus;
>      /* guest' receive mask */
> -    sccb_mask_t receive_mask;
> +    union {
> +        uint32_t receive_mask_compat32;
> +        sccb_mask_t receive_mask;
> +    };
>      /*
>       * when false, we keep the same broken, backwards compatible behaviour as
>       * before; when true, we implement the architecture correctly. Needed for
> @@ -261,7 +264,7 @@ static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
>      case SCLP_SELECTIVE_READ:
>          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);
> +        sclp_active_selection_mask = be64_to_cpu(sclp_active_selection_mask);

Would it make sense to introduce a wrapper that combines copy_mask()
and the endianness conversion (just in case you want to extend this
again in the future?

>          if (!sclp_cp_receive_mask ||
>              (sclp_active_selection_mask & ~sclp_cp_receive_mask)) {
>              sccb->h.response_code =
> @@ -301,13 +304,13 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
>      /* keep track of the guest's capability masks */
>      copy_mask((uint8_t *)&tmp_mask, WEM_CP_RECEIVE_MASK(we_mask, mask_length),
>                sizeof(tmp_mask), mask_length);
> -    ef->receive_mask = be32_to_cpu(tmp_mask);
> +    ef->receive_mask = be64_to_cpu(tmp_mask);
>  
>      /* return the SCLP's capability masks to the guest */
> -    tmp_mask = cpu_to_be32(get_host_receive_mask(ef));
> +    tmp_mask = cpu_to_be64(get_host_receive_mask(ef));
>      copy_mask(WEM_RECEIVE_MASK(we_mask, mask_length), (uint8_t *)&tmp_mask,
>                mask_length, sizeof(tmp_mask));
> -    tmp_mask = cpu_to_be32(get_host_send_mask(ef));
> +    tmp_mask = cpu_to_be64(get_host_send_mask(ef));
>      copy_mask(WEM_SEND_MASK(we_mask, mask_length), (uint8_t *)&tmp_mask,
>                mask_length, sizeof(tmp_mask));
>  
> @@ -368,6 +371,21 @@ static void command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code)
>      }
>  }
>  
> +static bool vmstate_event_facility_mask64_needed(void *opaque)
> +{
> +    SCLPEventFacility *ef = opaque;
> +
> +    return (ef->receive_mask & 0xFFFFFFFF) != 0;
> +}
> +
> +static int vmstate_event_facility_mask64_pre_load(void *opaque)
> +{
> +    SCLPEventFacility *ef = opaque;
> +
> +    ef->receive_mask &= ~0xFFFFFFFFULL;
> +    return 0;
> +}
> +
>  static bool vmstate_event_facility_mask_length_needed(void *opaque)
>  {
>      SCLPEventFacility *ef = opaque;
> @@ -383,6 +401,18 @@ static int vmstate_event_facility_mask_length_pre_load(void *opaque)
>      return 0;
>  }
>  
> +static const VMStateDescription vmstate_event_facility_mask64 = {
> +    .name = "vmstate-event-facility/mask64",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .needed = vmstate_event_facility_mask64_needed,
> +    .pre_load = vmstate_event_facility_mask64_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(receive_mask, SCLPEventFacility),
> +        VMSTATE_END_OF_LIST()
> +     }
> +};
> +

Are there plans for extending this beyond 64 bits? Would it make sense
to use the maximum possible size for the mask here, so you don't need
to introduce yet another vmstate in the future? (If it's unlikely that
the mask will ever move beyond 64 bit, that might be overkill, of
course.)

>  static const VMStateDescription vmstate_event_facility_mask_length = {
>      .name = "vmstate-event-facility/mask_length",
>      .version_id = 0,
> @@ -401,10 +431,11 @@ static const VMStateDescription vmstate_event_facility = {
>      .version_id = 0,
>      .minimum_version_id = 0,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(receive_mask, SCLPEventFacility),
> +        VMSTATE_UINT32(receive_mask_compat32, SCLPEventFacility),
>          VMSTATE_END_OF_LIST()
>       },
>      .subsections = (const VMStateDescription * []) {
> +        &vmstate_event_facility_mask64,
>          &vmstate_event_facility_mask_length,
>          NULL
>       }

So what happens for compat machines? They can't ever set bits beyond 31
IIUC?

Re: [Qemu-devel] [PATCH v1 3/3] s390x/sclp: extend SCLP event masks to 64 bits

[Claudio Imbrenda]

On Wed, 21 Feb 2018 16:34:59 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 20 Feb 2018 19:45:02 +0100
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> 
> > Extend the SCLP event masks to 64 bits. This will make us future
> > proof against future extensions of the architecture.
> > 
> > Notice that using any of the new bits results in a state that
> > cannot be migrated to an older version.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> > ---
> >  hw/s390x/event-facility.c         | 43
> > +++++++++++++++++++++++++++++++++------
> > include/hw/s390x/event-facility.h |  2 +- 2 files changed, 38
> > insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> > index f6f28fd..e71302a 100644
> > --- a/hw/s390x/event-facility.c
> > +++ b/hw/s390x/event-facility.c
> > @@ -30,7 +30,10 @@ struct SCLPEventFacility {
> >      SysBusDevice parent_obj;
> >      SCLPEventsBus sbus;
> >      /* guest' receive mask */
> > -    sccb_mask_t receive_mask;
> > +    union {
> > +        uint32_t receive_mask_compat32;
> > +        sccb_mask_t receive_mask;
> > +    };
> >      /*
> >       * when false, we keep the same broken, backwards compatible
> > behaviour as
> >       * before; when true, we implement the architecture correctly.
> > Needed for @@ -261,7 +264,7 @@ static void
> > read_event_data(SCLPEventFacility *ef, SCCB *sccb) case
> > SCLP_SELECTIVE_READ: 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);
> > +        sclp_active_selection_mask =
> > be64_to_cpu(sclp_active_selection_mask);  
> 
> Would it make sense to introduce a wrapper that combines copy_mask()
> and the endianness conversion (just in case you want to extend this
> again in the future?

maybe? but then we'd need to change the scalars into bitmasks, and the
whole thing would have to be rewritten anyway.

> >          if (!sclp_cp_receive_mask ||
> >              (sclp_active_selection_mask & ~sclp_cp_receive_mask)) {
> >              sccb->h.response_code =
> > @@ -301,13 +304,13 @@ static void
> > write_event_mask(SCLPEventFacility *ef, SCCB *sccb) /* keep track
> > of the guest's capability masks */ copy_mask((uint8_t *)&tmp_mask,
> > WEM_CP_RECEIVE_MASK(we_mask, mask_length), sizeof(tmp_mask),
> > mask_length);
> > -    ef->receive_mask = be32_to_cpu(tmp_mask);
> > +    ef->receive_mask = be64_to_cpu(tmp_mask);
> >  
> >      /* return the SCLP's capability masks to the guest */
> > -    tmp_mask = cpu_to_be32(get_host_receive_mask(ef));
> > +    tmp_mask = cpu_to_be64(get_host_receive_mask(ef));
> >      copy_mask(WEM_RECEIVE_MASK(we_mask, mask_length), (uint8_t
> > *)&tmp_mask, mask_length, sizeof(tmp_mask));
> > -    tmp_mask = cpu_to_be32(get_host_send_mask(ef));
> > +    tmp_mask = cpu_to_be64(get_host_send_mask(ef));
> >      copy_mask(WEM_SEND_MASK(we_mask, mask_length), (uint8_t
> > *)&tmp_mask, mask_length, sizeof(tmp_mask));
> >  
> > @@ -368,6 +371,21 @@ static void command_handler(SCLPEventFacility
> > *ef, SCCB *sccb, uint64_t code) }
> >  }
> >  
> > +static bool vmstate_event_facility_mask64_needed(void *opaque)
> > +{
> > +    SCLPEventFacility *ef = opaque;
> > +
> > +    return (ef->receive_mask & 0xFFFFFFFF) != 0;
> > +}
> > +
> > +static int vmstate_event_facility_mask64_pre_load(void *opaque)
> > +{
> > +    SCLPEventFacility *ef = opaque;
> > +
> > +    ef->receive_mask &= ~0xFFFFFFFFULL;
> > +    return 0;
> > +}
> > +
> >  static bool vmstate_event_facility_mask_length_needed(void *opaque)
> >  {
> >      SCLPEventFacility *ef = opaque;
> > @@ -383,6 +401,18 @@ static int
> > vmstate_event_facility_mask_length_pre_load(void *opaque) return 0;
> >  }
> >  
> > +static const VMStateDescription vmstate_event_facility_mask64 = {
> > +    .name = "vmstate-event-facility/mask64",
> > +    .version_id = 0,
> > +    .minimum_version_id = 0,
> > +    .needed = vmstate_event_facility_mask64_needed,
> > +    .pre_load = vmstate_event_facility_mask64_pre_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT64(receive_mask, SCLPEventFacility),
> > +        VMSTATE_END_OF_LIST()
> > +     }
> > +};
> > +  
> 
> Are there plans for extending this beyond 64 bits? Would it make sense

I don't know. I'm not even aware of anything above 32 bits, but since we
are already using all of the first 32 bits, it's only matter of time I
guess :)

> to use the maximum possible size for the mask here, so you don't need
> to introduce yet another vmstate in the future? (If it's unlikely that

That's true, but it requires changing simple scalars into bitmasks.
Surely doable, but I wanted to touch as little as possible.

> the mask will ever move beyond 64 bit, that might be overkill, of
> course.)
> 
> >  static const VMStateDescription vmstate_event_facility_mask_length
> > = { .name = "vmstate-event-facility/mask_length",
> >      .version_id = 0,
> > @@ -401,10 +431,11 @@ static const VMStateDescription
> > vmstate_event_facility = { .version_id = 0,
> >      .minimum_version_id = 0,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_UINT32(receive_mask, SCLPEventFacility),
> > +        VMSTATE_UINT32(receive_mask_compat32, SCLPEventFacility),
> >          VMSTATE_END_OF_LIST()
> >       },
> >      .subsections = (const VMStateDescription * []) {
> > +        &vmstate_event_facility_mask64,
> >          &vmstate_event_facility_mask_length,
> >          NULL
> >       }  
> 
> So what happens for compat machines? They can't ever set bits beyond
> 31 IIUC?

correct. compat machines are limited to exactly 32 bits anyway, as per
old broken behaviour. (this is ensured in the first patch of the series)

Re: [Qemu-devel] [PATCH v1 3/3] s390x/sclp: extend SCLP event masks to 64 bits

[Cornelia Huck]

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

> On Wed, 21 Feb 2018 16:34:59 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 20 Feb 2018 19:45:02 +0100
> > Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> > > +static const VMStateDescription vmstate_event_facility_mask64 = {
> > > +    .name = "vmstate-event-facility/mask64",
> > > +    .version_id = 0,
> > > +    .minimum_version_id = 0,
> > > +    .needed = vmstate_event_facility_mask64_needed,
> > > +    .pre_load = vmstate_event_facility_mask64_pre_load,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT64(receive_mask, SCLPEventFacility),
> > > +        VMSTATE_END_OF_LIST()
> > > +     }
> > > +};
> > > +    
> > 
> > Are there plans for extending this beyond 64 bits? Would it make sense  
> 
> I don't know. I'm not even aware of anything above 32 bits, but since we
> are already using all of the first 32 bits, it's only matter of time I
> guess :)
> 
> > to use the maximum possible size for the mask here, so you don't need
> > to introduce yet another vmstate in the future? (If it's unlikely that  
> 
> That's true, but it requires changing simple scalars into bitmasks.
> Surely doable, but I wanted to touch as little as possible.

OK, that pushes this firmly into the 'overkill' area. Let's just go
with your current approach.

> 
> > the mask will ever move beyond 64 bit, that might be overkill, of
> > course.)

Re: [Qemu-devel] [PATCH v1 0/3] s390x/sclp: 64 bit event masks

[Cornelia Huck]

On Tue, 20 Feb 2018 19:44:59 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> Until 67915de9f0383ccf4a ("s390x/event-facility: variable-length event masks")
> we only supported 32bit sclp event masks, even though the archiecture
> allows the guests to set up sclp event masks up to 1021 bytes in length.
> With that patch the behaviour was almost compliant, but some issues were
> still remaining, in particular regarding the handling of selective reads
> and migration.
> 
> This patchset fixes migration and the handling of selective reads, and
> puts in place the support for 64-bit sclp event masks internally.
> 
> A new property of the sclp-event device switches between the 32bit masks
> and the compliant behaviour. The property is bound to the machine
> version, so older machines keep the old broken behaviour, allowing for
> migration, but the default is the compliant implementation.
> 
> Fixes: 67915de9f0383ccf4a ("s390x/event-facility: variable-length event masks")
> 
> Claudio Imbrenda (3):
>   s390x/sclp: proper support of larger send and receive masks
>   s390x/sclp: clean up sclp masks
>   s390x/sclp: extend SCLP event masks to 64 bits
> 
>  hw/char/sclpconsole-lm.c          |   4 +-
>  hw/char/sclpconsole.c             |   4 +-
>  hw/s390x/event-facility.c         | 147 +++++++++++++++++++++++++++++++-------
>  hw/s390x/s390-virtio-ccw.c        |   8 ++-
>  hw/s390x/sclpcpu.c                |   4 +-
>  hw/s390x/sclpquiesce.c            |   4 +-
>  include/hw/s390x/event-facility.h |  22 +++---
>  7 files changed, 149 insertions(+), 44 deletions(-)
> 

I can't review the architecture details without documentation
obviously, so another question: Do you have a test you can share? :)