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

[Qemu-devel] [PATCH v4 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")

v3 -> v4
* removed all pre_load hooks
* split the internal represntation of the receive mask into an array of
  uint32_t and added accessors; the union would not work on little
  endian hosts!
* fixed the pre-existing documentation comment for copy_mask

v2 -> v3
* fixed some typos in the first patch description
* updated an existing comment in the third patch: newer Linux versions
  will support masks larger than 4 bytes.

v1 -> v2 
* improved comments and patch descriptions to better explain why we need
  this, including better description of the old broken behaviour
* rename SCLPEVMSK to SCLP_EVMASK to improve readability
* removed some unneded variable initializations
* fixed a pre-existing typo

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         | 153 ++++++++++++++++++++++++++++++--------
 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, 148 insertions(+), 51 deletions(-)

-- 
2.7.4

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

[Claudio Imbrenda]

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

When setting the sclp event mask, a mask size is also specified. Until
now we only considered the size in order to decide which bits to save
in the internal state. On the other hand, when a guest performs a
selective read, it sends a mask, but it does not specify a size; the
implied size is the size of the last mask that has been set.

Specifying bits in the mask of selective read that are not available in
the internal mask should return an error, and bits past the end of the
mask should obviously be ignored. This can only be achieved by keeping
track of the lenght of the mask.

The mask length is thus now part of the internal state that needs to be
migrated.

This patch fixes the handling of selective reads, whose size will now
match the length of the event mask, as per architecture.

While the default behaviour is to be compliant with the architecture,
when using older machine models the old broken behaviour is selected
(allowing only masks of size exactly 4), 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  | 81 ++++++++++++++++++++++++++++++++++++++--------
 hw/s390x/s390-virtio-ccw.c |  8 ++++-
 2 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 155a694..9233892 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -31,6 +31,15 @@ struct SCLPEventFacility {
     SCLPEventsBus sbus;
     /* guest' receive mask */
     unsigned int receive_mask;
+    /*
+     * when false, we keep the same broken, backwards compatible behaviour as
+     * before, allowing only masks of size exactly 4; when true, we implement
+     * the architecture correctly, allowing all valid mask sizes. 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 +229,17 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
     return rc;
 }
 
+/* copy up to src_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 +260,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 +281,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 +313,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 +369,24 @@ 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 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,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT16(mask_length, SCLPEventFacility),
+        VMSTATE_END_OF_LIST()
+     }
+};
+
 static const VMStateDescription vmstate_event_facility = {
     .name = "vmstate-event-facility",
     .version_id = 0,
@@ -363,15 +394,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

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

[Claudio Imbrenda]

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         | 20 ++++++++++----------
 hw/s390x/sclpcpu.c                |  4 ++--
 hw/s390x/sclpquiesce.c            |  4 ++--
 include/hw/s390x/event-facility.h | 22 +++++++++++++---------
 6 files changed, 31 insertions(+), 27 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 9233892..e04ed9f 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -29,8 +29,8 @@ typedef struct SCLPEventsBus {
 struct SCLPEventFacility {
     SysBusDevice parent_obj;
     SCLPEventsBus sbus;
-    /* guest' receive mask */
-    unsigned int receive_mask;
+    /* guest's receive mask */
+    sccb_mask_t receive_mask;
     /*
      * when false, we keep the same broken, backwards compatible behaviour as
      * before, allowing only masks of size exactly 4; when true, we implement
@@ -61,9 +61,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;
 
@@ -77,9 +77,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;
 
@@ -189,7 +189,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;
@@ -242,8 +242,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;
+    sccb_mask_t sclp_cp_receive_mask;
 
     ReadEventData *red = (ReadEventData *) sccb;
 
@@ -285,7 +285,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;
 
     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..0e2b761 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 SCLP_EVMASK(T)  (1ULL << (sizeof(sccb_mask_t) * 8 - (T)))
+
+#define SCLP_EVENT_MASK_OP_CMD          SCLP_EVMASK(SCLP_EVENT_OPRTNS_COMMAND)
+#define SCLP_EVENT_MASK_MSG             SCLP_EVMASK(SCLP_EVENT_MESSAGE)
+#define SCLP_EVENT_MASK_CONFIG_MGT_DATA SCLP_EVMASK(SCLP_EVENT_CONFIG_MGT_DATA)
+#define SCLP_EVENT_MASK_PMSGCMD         SCLP_EVMASK(SCLP_EVENT_PMSGCMD)
+#define SCLP_EVENT_MASK_MSG_ASCII       SCLP_EVMASK(SCLP_EVENT_ASCII_CONSOLE_DATA)
+#define SCLP_EVENT_MASK_SIGNAL_QUIESCE  SCLP_EVMASK(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

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

[Claudio Imbrenda]

Extend the SCLP event masks to 64 bits.

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         | 56 ++++++++++++++++++++++++++++++---------
 include/hw/s390x/event-facility.h |  2 +-
 2 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index e04ed9f..c3e39ee 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's receive mask */
-    sccb_mask_t receive_mask;
+    uint32_t receive_mask_pieces[2];
     /*
      * when false, we keep the same broken, backwards compatible behaviour as
      * before, allowing only masks of size exactly 4; when true, we implement
@@ -42,6 +42,18 @@ struct SCLPEventFacility {
     uint16_t mask_length;
 };
 
+static inline sccb_mask_t make_receive_mask(SCLPEventFacility *ef)
+{
+    return ((sccb_mask_t)ef->receive_mask_pieces[0]) << 32 |
+                         ef->receive_mask_pieces[1];
+}
+
+static inline void store_receive_mask(SCLPEventFacility *ef, sccb_mask_t val)
+{
+    ef->receive_mask_pieces[1] = val;
+    ef->receive_mask_pieces[0] = val >> 32;
+}
+
 /* return true if any child has event pending set */
 static bool event_pending(SCLPEventFacility *ef)
 {
@@ -54,7 +66,7 @@ static bool event_pending(SCLPEventFacility *ef)
         event = DO_UPCAST(SCLPEvent, qdev, qdev);
         event_class = SCLP_EVENT_GET_CLASS(event);
         if (event->event_pending &&
-            event_class->get_send_mask() & ef->receive_mask) {
+            event_class->get_send_mask() & make_receive_mask(ef)) {
             return true;
         }
     }
@@ -252,7 +264,7 @@ static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
         goto out;
     }
 
-    sclp_cp_receive_mask = ef->receive_mask;
+    sclp_cp_receive_mask = make_receive_mask(ef);
 
     /* get active selection mask */
     switch (sccb->h.function_code) {
@@ -262,7 +274,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 =
@@ -294,21 +306,22 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
     }
 
     /*
-     * Note: We currently only support masks up to 4 byte length;
-     *       the remainder is filled up with zeroes. Linux uses
-     *       a 4 byte mask length.
+     * Note: We currently only support masks up to 8 byte length;
+     *       the remainder is filled up with zeroes. Older Linux
+     *       kernels use a 4 byte mask length, newer ones can use both
+     *       8 or 4 depending on what is available on the host.
      */
 
     /* 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);
+    store_receive_mask(ef, 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));
 
@@ -369,6 +382,13 @@ 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_pieces[1] != 0;
+}
+
 static bool vmstate_event_facility_mask_length_needed(void *opaque)
 {
     SCLPEventFacility *ef = opaque;
@@ -376,6 +396,17 @@ static bool vmstate_event_facility_mask_length_needed(void *opaque)
     return ef->allow_all_mask_sizes;
 }
 
+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,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(receive_mask_pieces[1], SCLPEventFacility),
+        VMSTATE_END_OF_LIST()
+     }
+};
+
 static const VMStateDescription vmstate_event_facility_mask_length = {
     .name = "vmstate-event-facility/mask_length",
     .version_id = 0,
@@ -392,10 +423,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_pieces[0], SCLPEventFacility),
         VMSTATE_END_OF_LIST()
      },
     .subsections = (const VMStateDescription * []) {
+        &vmstate_event_facility_mask64,
         &vmstate_event_facility_mask_length,
         NULL
      }
@@ -447,7 +479,7 @@ static void reset_event_facility(DeviceState *dev)
 {
     SCLPEventFacility *sdev = EVENT_FACILITY(dev);
 
-    sdev->receive_mask = 0;
+    store_receive_mask(sdev, 0);
 }
 
 static void init_event_facility_class(ObjectClass *klass, void *data)
diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
index 0e2b761..06ba4ea 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 v4 0/3] s390x/sclp: 64 bit event masks

[Cornelia Huck]

On Fri, 23 Feb 2018 18:42:55 +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")
> 
> v3 -> v4
> * removed all pre_load hooks
> * split the internal represntation of the receive mask into an array of
>   uint32_t and added accessors; the union would not work on little
>   endian hosts!

Oops. Did you test under a le host? How can I test this (I guess using
the current s390/features branch as guest)?

> * fixed the pre-existing documentation comment for copy_mask
> 
> v2 -> v3
> * fixed some typos in the first patch description
> * updated an existing comment in the third patch: newer Linux versions
>   will support masks larger than 4 bytes.
> 
> v1 -> v2 
> * improved comments and patch descriptions to better explain why we need
>   this, including better description of the old broken behaviour
> * rename SCLPEVMSK to SCLP_EVMASK to improve readability
> * removed some unneded variable initializations
> * fixed a pre-existing typo
> 
> 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         | 153 ++++++++++++++++++++++++++++++--------
>  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, 148 insertions(+), 51 deletions(-)
> 

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

[Cornelia Huck]

On Fri, 23 Feb 2018 18:42:58 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> Extend the SCLP event masks to 64 bits.
> 
> 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         | 56 ++++++++++++++++++++++++++++++---------
>  include/hw/s390x/event-facility.h |  2 +-
>  2 files changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index e04ed9f..c3e39ee 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's receive mask */
> -    sccb_mask_t receive_mask;
> +    uint32_t receive_mask_pieces[2];
>      /*
>       * when false, we keep the same broken, backwards compatible behaviour as
>       * before, allowing only masks of size exactly 4; when true, we implement
> @@ -42,6 +42,18 @@ struct SCLPEventFacility {
>      uint16_t mask_length;
>  };
>  
> +static inline sccb_mask_t make_receive_mask(SCLPEventFacility *ef)
> +{
> +    return ((sccb_mask_t)ef->receive_mask_pieces[0]) << 32 |
> +                         ef->receive_mask_pieces[1];
> +}
> +
> +static inline void store_receive_mask(SCLPEventFacility *ef, sccb_mask_t val)
> +{
> +    ef->receive_mask_pieces[1] = val;
> +    ef->receive_mask_pieces[0] = val >> 32;
> +}
> +

Hm... how are all those values actually defined in the architecture?
You pass around some values internally (which are supposedly in host
endian) and then and/or them with the receive mask here. Are they
compared byte-for-byte? 32-bit-for-32-bit?

I'm also not a fan of the _pieces suffix - reminds me of Dwarf pieces :)

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

[Christian Borntraeger]

h

On 02/26/2018 05:57 PM, Cornelia Huck wrote:
> On Fri, 23 Feb 2018 18:42:58 +0100
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> 
>> Extend the SCLP event masks to 64 bits.
>>
>> 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         | 56 ++++++++++++++++++++++++++++++---------
>>  include/hw/s390x/event-facility.h |  2 +-
>>  2 files changed, 45 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>> index e04ed9f..c3e39ee 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's receive mask */
>> -    sccb_mask_t receive_mask;
>> +    uint32_t receive_mask_pieces[2];
>>      /*
>>       * when false, we keep the same broken, backwards compatible behaviour as
>>       * before, allowing only masks of size exactly 4; when true, we implement
>> @@ -42,6 +42,18 @@ struct SCLPEventFacility {
>>      uint16_t mask_length;
>>  };
>>  
>> +static inline sccb_mask_t make_receive_mask(SCLPEventFacility *ef)
>> +{
>> +    return ((sccb_mask_t)ef->receive_mask_pieces[0]) << 32 |
>> +                         ef->receive_mask_pieces[1];
>> +}
>> +
>> +static inline void store_receive_mask(SCLPEventFacility *ef, sccb_mask_t val)
>> +{
>> +    ef->receive_mask_pieces[1] = val;
>> +    ef->receive_mask_pieces[0] = val >> 32;
>> +}
>> +
> 
> Hm... how are all those values actually defined in the architecture?
> You pass around some values internally (which are supposedly in host
> endian) and then and/or them with the receive mask here. Are they
> compared byte-for-byte? 32-bit-for-32-bit?

Its a bitfield in ibm byte order. the length is a multitude of bytes.


> 
> I'm also not a fan of the _pieces suffix - reminds me of Dwarf pieces :)
> 

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

[Christian Borntraeger]

On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:
> Until 67915de9f0383ccf4a ("s390x/event-facility: variable-length event masks")
> we only supported sclp event masks with a size of exactly 4 bytes, even
> though the architecture allows the guests to set up sclp event masks
> from 1 to 1021 bytes in length.
> After that patch, the behaviour was almost compliant, but some issues
> were still remaining, in particular regarding the handling of selective
> reads and migration.
> 
> When setting the sclp event mask, a mask size is also specified. Until
> now we only considered the size in order to decide which bits to save
> in the internal state. On the other hand, when a guest performs a
> selective read, it sends a mask, but it does not specify a size; the
> implied size is the size of the last mask that has been set.
> 
> Specifying bits in the mask of selective read that are not available in
> the internal mask should return an error, and bits past the end of the
> mask should obviously be ignored. This can only be achieved by keeping
> track of the lenght of the mask.
> 
> The mask length is thus now part of the internal state that needs to be
> migrated.
> 
> This patch fixes the handling of selective reads, whose size will now
> match the length of the event mask, as per architecture.
> 
> While the default behaviour is to be compliant with the architecture,
> when using older machine models the old broken behaviour is selected
> (allowing only masks of size exactly 4), 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>

Looks sane and seems to match the docs.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

One question below:

> ---
>  hw/s390x/event-facility.c  | 81 ++++++++++++++++++++++++++++++++++++++--------
>  hw/s390x/s390-virtio-ccw.c |  8 ++++-
>  2 files changed, 75 insertions(+), 14 deletions(-)
[...9
>  static void init_event_facility(Object *obj)
>  {
>      SCLPEventFacility *event_facility = EVENT_FACILITY(obj);
>      DeviceState *sdev = DEVICE(obj);
>      Object *new;
> 
> +    event_facility->mask_length = 4;

Shouldnt we start with 0 here (as no mask is set)

and
> +    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",\
> +        },

then set the mask_len here

or is this overkill? In the end it should not matter as the active mask is 0 so the
length does not matter.


> 
>  #define CCW_COMPAT_2_10 \
>          HW_COMPAT_2_10
> 

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

[Christian Borntraeger]

On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:
> 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>
Certainly a sane cleanup, especially the use if the SCLP_EVMASK.
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  hw/char/sclpconsole-lm.c          |  4 ++--
>  hw/char/sclpconsole.c             |  4 ++--
>  hw/s390x/event-facility.c         | 20 ++++++++++----------
>  hw/s390x/sclpcpu.c                |  4 ++--
>  hw/s390x/sclpquiesce.c            |  4 ++--
>  include/hw/s390x/event-facility.h | 22 +++++++++++++---------
>  6 files changed, 31 insertions(+), 27 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 9233892..e04ed9f 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -29,8 +29,8 @@ typedef struct SCLPEventsBus {
>  struct SCLPEventFacility {
>      SysBusDevice parent_obj;
>      SCLPEventsBus sbus;
> -    /* guest' receive mask */
> -    unsigned int receive_mask;
> +    /* guest's receive mask */
> +    sccb_mask_t receive_mask;
>      /*
>       * when false, we keep the same broken, backwards compatible behaviour as
>       * before, allowing only masks of size exactly 4; when true, we implement
> @@ -61,9 +61,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;
> 
> @@ -77,9 +77,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;
> 
> @@ -189,7 +189,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;
> @@ -242,8 +242,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;
> +    sccb_mask_t sclp_cp_receive_mask;
> 
>      ReadEventData *red = (ReadEventData *) sccb;
> 
> @@ -285,7 +285,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;
> 
>      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..0e2b761 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 SCLP_EVMASK(T)  (1ULL << (sizeof(sccb_mask_t) * 8 - (T)))
> +
> +#define SCLP_EVENT_MASK_OP_CMD          SCLP_EVMASK(SCLP_EVENT_OPRTNS_COMMAND)
> +#define SCLP_EVENT_MASK_MSG             SCLP_EVMASK(SCLP_EVENT_MESSAGE)
> +#define SCLP_EVENT_MASK_CONFIG_MGT_DATA SCLP_EVMASK(SCLP_EVENT_CONFIG_MGT_DATA)
> +#define SCLP_EVENT_MASK_PMSGCMD         SCLP_EVMASK(SCLP_EVENT_PMSGCMD)
> +#define SCLP_EVENT_MASK_MSG_ASCII       SCLP_EVMASK(SCLP_EVENT_ASCII_CONSOLE_DATA)
> +#define SCLP_EVENT_MASK_SIGNAL_QUIESCE  SCLP_EVMASK(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);
> 

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

[Christian Borntraeger]

On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:
> Extend the SCLP event masks to 64 bits.
> 
> 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         | 56 ++++++++++++++++++++++++++++++---------
>  include/hw/s390x/event-facility.h |  2 +-
>  2 files changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index e04ed9f..c3e39ee 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's receive mask */
> -    sccb_mask_t receive_mask;
> +    uint32_t receive_mask_pieces[2];


Before the change, we basically use be32_to_cpu to transfer the byte field into a cpu
endianess value. In the end it is actually a bitfield, but for compat we need to keep
he reversal. So it will be hard to get this fixed without some kind of ugliness.

Does it make sense to make the sccb_mask_t a union of an uint64_t and the pieces?



>      /*
>       * when false, we keep the same broken, backwards compatible behaviour as
>       * before, allowing only masks of size exactly 4; when true, we implement
> @@ -42,6 +42,18 @@ struct SCLPEventFacility {
>      uint16_t mask_length;
>  };
> 
> +static inline sccb_mask_t make_receive_mask(SCLPEventFacility *ef)
> +{
> +    return ((sccb_mask_t)ef->receive_mask_pieces[0]) << 32 |
> +                         ef->receive_mask_pieces[1];
> +}
> +
> +static inline void store_receive_mask(SCLPEventFacility *ef, sccb_mask_t val)
> +{
> +    ef->receive_mask_pieces[1] = val;
> +    ef->receive_mask_pieces[0] = val >> 32;
> +}
> +
>  /* return true if any child has event pending set */
>  static bool event_pending(SCLPEventFacility *ef)
>  {
> @@ -54,7 +66,7 @@ static bool event_pending(SCLPEventFacility *ef)
>          event = DO_UPCAST(SCLPEvent, qdev, qdev);
>          event_class = SCLP_EVENT_GET_CLASS(event);
>          if (event->event_pending &&
> -            event_class->get_send_mask() & ef->receive_mask) {
> +            event_class->get_send_mask() & make_receive_mask(ef)) {
>              return true;
>          }
>      }
> @@ -252,7 +264,7 @@ static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
>          goto out;
>      }
> 
> -    sclp_cp_receive_mask = ef->receive_mask;
> +    sclp_cp_receive_mask = make_receive_mask(ef);
> 
>      /* get active selection mask */
>      switch (sccb->h.function_code) {
> @@ -262,7 +274,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 =
> @@ -294,21 +306,22 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
>      }
> 
>      /*
> -     * Note: We currently only support masks up to 4 byte length;
> -     *       the remainder is filled up with zeroes. Linux uses
> -     *       a 4 byte mask length.
> +     * Note: We currently only support masks up to 8 byte length;
> +     *       the remainder is filled up with zeroes. Older Linux
> +     *       kernels use a 4 byte mask length, newer ones can use both
> +     *       8 or 4 depending on what is available on the host.
>       */
> 
>      /* 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);
> +    store_receive_mask(ef, 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));
> 
> @@ -369,6 +382,13 @@ 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_pieces[1] != 0;
> +}
> +
>  static bool vmstate_event_facility_mask_length_needed(void *opaque)
>  {
>      SCLPEventFacility *ef = opaque;
> @@ -376,6 +396,17 @@ static bool vmstate_event_facility_mask_length_needed(void *opaque)
>      return ef->allow_all_mask_sizes;
>  }
> 
> +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,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(receive_mask_pieces[1], SCLPEventFacility),
> +        VMSTATE_END_OF_LIST()
> +     }
> +};
> +
>  static const VMStateDescription vmstate_event_facility_mask_length = {
>      .name = "vmstate-event-facility/mask_length",
>      .version_id = 0,
> @@ -392,10 +423,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_pieces[0], SCLPEventFacility),
>          VMSTATE_END_OF_LIST()
>       },
>      .subsections = (const VMStateDescription * []) {
> +        &vmstate_event_facility_mask64,
>          &vmstate_event_facility_mask_length,
>          NULL
>       }
> @@ -447,7 +479,7 @@ static void reset_event_facility(DeviceState *dev)
>  {
>      SCLPEventFacility *sdev = EVENT_FACILITY(dev);
> 
> -    sdev->receive_mask = 0;
> +    store_receive_mask(sdev, 0);
>  }
> 
>  static void init_event_facility_class(ObjectClass *klass, void *data)
> diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
> index 0e2b761..06ba4ea 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;
> 

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

[Cornelia Huck]

On Fri, 2 Mar 2018 10:44:46 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:
> > Extend the SCLP event masks to 64 bits.
> > 
> > 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         | 56 ++++++++++++++++++++++++++++++---------
> >  include/hw/s390x/event-facility.h |  2 +-
> >  2 files changed, 45 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> > index e04ed9f..c3e39ee 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's receive mask */
> > -    sccb_mask_t receive_mask;
> > +    uint32_t receive_mask_pieces[2];  
> 
> 
> Before the change, we basically use be32_to_cpu to transfer the byte field into a cpu
> endianess value. In the end it is actually a bitfield, but for compat we need to keep
> he reversal. So it will be hard to get this fixed without some kind of ugliness.

Could we also use a compat mask callback/handler for older machines and
switch to 64 bit handlers for the default case? Probably would be even
more ugly, though.

> 
> Does it make sense to make the sccb_mask_t a union of an uint64_t and the pieces?
> 
> 
> 
> >      /*
> >       * when false, we keep the same broken, backwards compatible behaviour as
> >       * before, allowing only masks of size exactly 4; when true, we implement
> > @@ -42,6 +42,18 @@ struct SCLPEventFacility {
> >      uint16_t mask_length;
> >  };
> > 
> > +static inline sccb_mask_t make_receive_mask(SCLPEventFacility *ef)
> > +{
> > +    return ((sccb_mask_t)ef->receive_mask_pieces[0]) << 32 |
> > +                         ef->receive_mask_pieces[1];
> > +}
> > +
> > +static inline void store_receive_mask(SCLPEventFacility *ef, sccb_mask_t val)
> > +{
> > +    ef->receive_mask_pieces[1] = val;
> > +    ef->receive_mask_pieces[0] = val >> 32;
> > +}
> > +
> >  /* return true if any child has event pending set */
> >  static bool event_pending(SCLPEventFacility *ef)
> >  {
> > @@ -54,7 +66,7 @@ static bool event_pending(SCLPEventFacility *ef)
> >          event = DO_UPCAST(SCLPEvent, qdev, qdev);
> >          event_class = SCLP_EVENT_GET_CLASS(event);
> >          if (event->event_pending &&
> > -            event_class->get_send_mask() & ef->receive_mask) {
> > +            event_class->get_send_mask() & make_receive_mask(ef)) {
> >              return true;
> >          }
> >      }
> > @@ -252,7 +264,7 @@ static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
> >          goto out;
> >      }
> > 
> > -    sclp_cp_receive_mask = ef->receive_mask;
> > +    sclp_cp_receive_mask = make_receive_mask(ef);
> > 
> >      /* get active selection mask */
> >      switch (sccb->h.function_code) {
> > @@ -262,7 +274,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 =
> > @@ -294,21 +306,22 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
> >      }
> > 
> >      /*
> > -     * Note: We currently only support masks up to 4 byte length;
> > -     *       the remainder is filled up with zeroes. Linux uses
> > -     *       a 4 byte mask length.
> > +     * Note: We currently only support masks up to 8 byte length;
> > +     *       the remainder is filled up with zeroes. Older Linux
> > +     *       kernels use a 4 byte mask length, newer ones can use both
> > +     *       8 or 4 depending on what is available on the host.
> >       */

I.e., handle the 4 or 8 byte values here. But probably would be too
ugly to live.

> > 
> >      /* 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);
> > +    store_receive_mask(ef, 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));
> > 
> > @@ -369,6 +382,13 @@ 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_pieces[1] != 0;
> > +}
> > +
> >  static bool vmstate_event_facility_mask_length_needed(void *opaque)
> >  {
> >      SCLPEventFacility *ef = opaque;
> > @@ -376,6 +396,17 @@ static bool vmstate_event_facility_mask_length_needed(void *opaque)
> >      return ef->allow_all_mask_sizes;
> >  }
> > 
> > +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,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32(receive_mask_pieces[1], SCLPEventFacility),
> > +        VMSTATE_END_OF_LIST()
> > +     }
> > +};
> > +
> >  static const VMStateDescription vmstate_event_facility_mask_length = {
> >      .name = "vmstate-event-facility/mask_length",
> >      .version_id = 0,
> > @@ -392,10 +423,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_pieces[0], SCLPEventFacility),
> >          VMSTATE_END_OF_LIST()
> >       },
> >      .subsections = (const VMStateDescription * []) {
> > +        &vmstate_event_facility_mask64,
> >          &vmstate_event_facility_mask_length,
> >          NULL
> >       }
> > @@ -447,7 +479,7 @@ static void reset_event_facility(DeviceState *dev)
> >  {
> >      SCLPEventFacility *sdev = EVENT_FACILITY(dev);
> > 
> > -    sdev->receive_mask = 0;
> > +    store_receive_mask(sdev, 0);
> >  }
> > 
> >  static void init_event_facility_class(ObjectClass *klass, void *data)
> > diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
> > index 0e2b761..06ba4ea 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;
> >   
> 

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

[Christian Borntraeger]

On 03/05/2018 04:27 PM, Cornelia Huck wrote:
> On Fri, 2 Mar 2018 10:44:46 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:
>>> Extend the SCLP event masks to 64 bits.
>>>
>>> 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         | 56 ++++++++++++++++++++++++++++++---------
>>>  include/hw/s390x/event-facility.h |  2 +-
>>>  2 files changed, 45 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>>> index e04ed9f..c3e39ee 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's receive mask */
>>> -    sccb_mask_t receive_mask;
>>> +    uint32_t receive_mask_pieces[2];  
>>
>>
>> Before the change, we basically use be32_to_cpu to transfer the byte field into a cpu
>> endianess value. In the end it is actually a bitfield, but for compat we need to keep
>> he reversal. So it will be hard to get this fixed without some kind of ugliness.
> 
> Could we also use a compat mask callback/handler for older machines and
> switch to 64 bit handlers for the default case? Probably would be even
> more ugly, though.

Claudio had a version with a pre/post/load/save handler. Claudio can you repost this
version so that we can have a look what is "less ugly"?

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

[Cornelia Huck]

On Tue, 6 Mar 2018 09:23:23 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 03/05/2018 04:27 PM, Cornelia Huck wrote:
> > On Fri, 2 Mar 2018 10:44:46 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:  
> >>> Extend the SCLP event masks to 64 bits.
> >>>
> >>> 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         | 56 ++++++++++++++++++++++++++++++---------
> >>>  include/hw/s390x/event-facility.h |  2 +-
> >>>  2 files changed, 45 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> >>> index e04ed9f..c3e39ee 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's receive mask */
> >>> -    sccb_mask_t receive_mask;
> >>> +    uint32_t receive_mask_pieces[2];    
> >>
> >>
> >> Before the change, we basically use be32_to_cpu to transfer the byte field into a cpu
> >> endianess value. In the end it is actually a bitfield, but for compat we need to keep
> >> he reversal. So it will be hard to get this fixed without some kind of ugliness.  
> > 
> > Could we also use a compat mask callback/handler for older machines and
> > switch to 64 bit handlers for the default case? Probably would be even
> > more ugly, though.  
> 
> Claudio had a version with a pre/post/load/save handler. Claudio can you repost this
> version so that we can have a look what is "less ugly"?
> 

Would that other version be independent of the first two patches (i.e.,
only replace this patch)?

I would like to apply patch 1 as it is a fix, and patch 2 seems
uncontroversial as a cleanup.

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

[Claudio Imbrenda]

On Mon, 26 Feb 2018 17:57:51 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 23 Feb 2018 18:42:58 +0100
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> 
> > Extend the SCLP event masks to 64 bits.
> > 
> > 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         | 56
> > ++++++++++++++++++++++++++++++---------
> > include/hw/s390x/event-facility.h |  2 +- 2 files changed, 45
> > insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> > index e04ed9f..c3e39ee 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's receive mask */
> > -    sccb_mask_t receive_mask;
> > +    uint32_t receive_mask_pieces[2];
> >      /*
> >       * when false, we keep the same broken, backwards compatible
> > behaviour as
> >       * before, allowing only masks of size exactly 4; when true,
> > we implement @@ -42,6 +42,18 @@ struct SCLPEventFacility {
> >      uint16_t mask_length;
> >  };
> >  
> > +static inline sccb_mask_t make_receive_mask(SCLPEventFacility *ef)
> > +{
> > +    return ((sccb_mask_t)ef->receive_mask_pieces[0]) << 32 |
> > +                         ef->receive_mask_pieces[1];
> > +}
> > +
> > +static inline void store_receive_mask(SCLPEventFacility *ef,
> > sccb_mask_t val) +{
> > +    ef->receive_mask_pieces[1] = val;
> > +    ef->receive_mask_pieces[0] = val >> 32;
> > +}
> > +  
> 
> Hm... how are all those values actually defined in the architecture?
> You pass around some values internally (which are supposedly in host
> endian) and then and/or them with the receive mask here. Are they
> compared byte-for-byte? 32-bit-for-32-bit?

We always convert anything from/to the guest into/from host endian, so
things Just Work™. comparison is always with == and so on, on the full
reconstructed values. receive_mask_pieces[0] always needs to contain
the leftmost 32 bits of the mask, which does not correspond to the
leftmost 32 bits in little endian, which is why these functions need to
exist.

These utility functions help recompose the 64-bit value from the two
32-bit pieces or the other way around. On a big endian system this will
result in a nop. 

The accessors are needed because we can't store the 64 bit value
internally as a 64bit value -- we need to save the high 32 bits for
compat, and the lower (rightmost in the mask) 32 bits only when used,
and this is not possible otherwise. But all operations are always only
performed on the correct 64-bit values.

I hope it makes sense

> I'm also not a fan of the _pieces suffix - reminds me of Dwarf
> pieces :)

I also think this is very ugly, but there isn't much that can be done,
unless we start using all the pre_ and post_ hooks in the savestate,
but that would really look ugly.

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

[Claudio Imbrenda]

On Mon, 5 Mar 2018 16:27:10 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 2 Mar 2018 10:44:46 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:  
> > > Extend the SCLP event masks to 64 bits.
> > > 
> > > 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         | 56
> > > ++++++++++++++++++++++++++++++---------
> > > include/hw/s390x/event-facility.h |  2 +- 2 files changed, 45
> > > insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> > > index e04ed9f..c3e39ee 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's receive mask */
> > > -    sccb_mask_t receive_mask;
> > > +    uint32_t receive_mask_pieces[2];    
> > 
> > 
> > Before the change, we basically use be32_to_cpu to transfer the
> > byte field into a cpu endianess value. In the end it is actually a
> > bitfield, but for compat we need to keep he reversal. So it will be
> > hard to get this fixed without some kind of ugliness.  
> 
> Could we also use a compat mask callback/handler for older machines
> and switch to 64 bit handlers for the default case? Probably would be
> even more ugly, though.

it would make the code here slightly more straightforward (no pieces, no
accessor functions) at the expense of a lot of ugliness in the VMState
(we'd need all of pre_save, pre_load, post_load).

basically we'd need both the 32-bit and 64-bit internal variables, and
we'd need to keep them in sync when loading/storing the VMState. 

> > 
> > Does it make sense to make the sccb_mask_t a union of an uint64_t
> > and the pieces?
> > 
> > 
> >   
> > >      /*
> > >       * when false, we keep the same broken, backwards compatible
> > > behaviour as
> > >       * before, allowing only masks of size exactly 4; when true,
> > > we implement @@ -42,6 +42,18 @@ struct SCLPEventFacility {
> > >      uint16_t mask_length;
> > >  };
> > > 
> > > +static inline sccb_mask_t make_receive_mask(SCLPEventFacility
> > > *ef) +{
> > > +    return ((sccb_mask_t)ef->receive_mask_pieces[0]) << 32 |
> > > +                         ef->receive_mask_pieces[1];
> > > +}
> > > +
> > > +static inline void store_receive_mask(SCLPEventFacility *ef,
> > > sccb_mask_t val) +{
> > > +    ef->receive_mask_pieces[1] = val;
> > > +    ef->receive_mask_pieces[0] = val >> 32;
> > > +}
> > > +
> > >  /* return true if any child has event pending set */
> > >  static bool event_pending(SCLPEventFacility *ef)
> > >  {
> > > @@ -54,7 +66,7 @@ static bool event_pending(SCLPEventFacility *ef)
> > >          event = DO_UPCAST(SCLPEvent, qdev, qdev);
> > >          event_class = SCLP_EVENT_GET_CLASS(event);
> > >          if (event->event_pending &&
> > > -            event_class->get_send_mask() & ef->receive_mask) {
> > > +            event_class->get_send_mask() &
> > > make_receive_mask(ef)) { return true;
> > >          }
> > >      }
> > > @@ -252,7 +264,7 @@ static void read_event_data(SCLPEventFacility
> > > *ef, SCCB *sccb) goto out;
> > >      }
> > > 
> > > -    sclp_cp_receive_mask = ef->receive_mask;
> > > +    sclp_cp_receive_mask = make_receive_mask(ef);
> > > 
> > >      /* get active selection mask */
> > >      switch (sccb->h.function_code) {
> > > @@ -262,7 +274,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 =
> > > @@ -294,21 +306,22 @@ static void
> > > write_event_mask(SCLPEventFacility *ef, SCCB *sccb) }
> > > 
> > >      /*
> > > -     * Note: We currently only support masks up to 4 byte length;
> > > -     *       the remainder is filled up with zeroes. Linux uses
> > > -     *       a 4 byte mask length.
> > > +     * Note: We currently only support masks up to 8 byte length;
> > > +     *       the remainder is filled up with zeroes. Older Linux
> > > +     *       kernels use a 4 byte mask length, newer ones can
> > > use both
> > > +     *       8 or 4 depending on what is available on the host.
> > >       */  
> 
> I.e., handle the 4 or 8 byte values here. But probably would be too
> ugly to live.

I agree, let's avoid

> > > 
> > >      /* 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);
> > > +    store_receive_mask(ef, 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));
> > > 
> > > @@ -369,6 +382,13 @@ 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_pieces[1] != 0;
> > > +}
> > > +
> > >  static bool vmstate_event_facility_mask_length_needed(void
> > > *opaque) {
> > >      SCLPEventFacility *ef = opaque;
> > > @@ -376,6 +396,17 @@ static bool
> > > vmstate_event_facility_mask_length_needed(void *opaque) return
> > > ef->allow_all_mask_sizes; }
> > > 
> > > +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,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT32(receive_mask_pieces[1],
> > > SCLPEventFacility),
> > > +        VMSTATE_END_OF_LIST()
> > > +     }
> > > +};
> > > +
> > >  static const VMStateDescription
> > > vmstate_event_facility_mask_length = { .name =
> > > "vmstate-event-facility/mask_length", .version_id = 0,
> > > @@ -392,10 +423,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_pieces[0],
> > > SCLPEventFacility), VMSTATE_END_OF_LIST()
> > >       },
> > >      .subsections = (const VMStateDescription * []) {
> > > +        &vmstate_event_facility_mask64,
> > >          &vmstate_event_facility_mask_length,
> > >          NULL
> > >       }
> > > @@ -447,7 +479,7 @@ static void reset_event_facility(DeviceState
> > > *dev) {
> > >      SCLPEventFacility *sdev = EVENT_FACILITY(dev);
> > > 
> > > -    sdev->receive_mask = 0;
> > > +    store_receive_mask(sdev, 0);
> > >  }
> > > 
> > >  static void init_event_facility_class(ObjectClass *klass, void
> > > *data) diff --git a/include/hw/s390x/event-facility.h
> > > b/include/hw/s390x/event-facility.h index 0e2b761..06ba4ea 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;
> > >     
> >   
> 

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

[Claudio Imbrenda]

On Tue, 6 Mar 2018 09:23:23 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 03/05/2018 04:27 PM, Cornelia Huck wrote:
> > On Fri, 2 Mar 2018 10:44:46 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:  
> >>> Extend the SCLP event masks to 64 bits.
> >>>
> >>> 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         | 56
> >>> ++++++++++++++++++++++++++++++---------
> >>> include/hw/s390x/event-facility.h |  2 +- 2 files changed, 45
> >>> insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> >>> index e04ed9f..c3e39ee 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's receive mask */
> >>> -    sccb_mask_t receive_mask;
> >>> +    uint32_t receive_mask_pieces[2];    
> >>
> >>
> >> Before the change, we basically use be32_to_cpu to transfer the
> >> byte field into a cpu endianess value. In the end it is actually a
> >> bitfield, but for compat we need to keep he reversal. So it will
> >> be hard to get this fixed without some kind of ugliness.  
> > 
> > Could we also use a compat mask callback/handler for older machines
> > and switch to 64 bit handlers for the default case? Probably would
> > be even more ugly, though.  
> 
> Claudio had a version with a pre/post/load/save handler. Claudio can
> you repost this version so that we can have a look what is "less
> ugly"?

I actually never sent that around, and reworked it after you said it
was ugly (and it was ugly), so I don't actually have it around
anymore :(

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

[Claudio Imbrenda]

On Tue, 6 Mar 2018 10:27:52 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 6 Mar 2018 09:23:23 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 03/05/2018 04:27 PM, Cornelia Huck wrote:  
> > > On Fri, 2 Mar 2018 10:44:46 +0100
> > > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> > >     
> > >> On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:    
> > >>> Extend the SCLP event masks to 64 bits.
> > >>>
> > >>> 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         | 56
> > >>> ++++++++++++++++++++++++++++++---------
> > >>> include/hw/s390x/event-facility.h |  2 +- 2 files changed, 45
> > >>> insertions(+), 13 deletions(-)
> > >>>
> > >>> diff --git a/hw/s390x/event-facility.c
> > >>> b/hw/s390x/event-facility.c index e04ed9f..c3e39ee 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's receive mask */
> > >>> -    sccb_mask_t receive_mask;
> > >>> +    uint32_t receive_mask_pieces[2];      
> > >>
> > >>
> > >> Before the change, we basically use be32_to_cpu to transfer the
> > >> byte field into a cpu endianess value. In the end it is actually
> > >> a bitfield, but for compat we need to keep he reversal. So it
> > >> will be hard to get this fixed without some kind of ugliness.    
> > > 
> > > Could we also use a compat mask callback/handler for older
> > > machines and switch to 64 bit handlers for the default case?
> > > Probably would be even more ugly, though.    
> > 
> > Claudio had a version with a pre/post/load/save handler. Claudio
> > can you repost this version so that we can have a look what is
> > "less ugly"? 
> 
> Would that other version be independent of the first two patches
> (i.e., only replace this patch)?

yes

> I would like to apply patch 1 as it is a fix, and patch 2 seems
> uncontroversial as a cleanup.

yes please

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

[Cornelia Huck]

On Fri, 2 Mar 2018 10:09:16 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:
> > Until 67915de9f0383ccf4a ("s390x/event-facility: variable-length event masks")
> > we only supported sclp event masks with a size of exactly 4 bytes, even
> > though the architecture allows the guests to set up sclp event masks
> > from 1 to 1021 bytes in length.
> > After that patch, the behaviour was almost compliant, but some issues
> > were still remaining, in particular regarding the handling of selective
> > reads and migration.
> > 
> > When setting the sclp event mask, a mask size is also specified. Until
> > now we only considered the size in order to decide which bits to save
> > in the internal state. On the other hand, when a guest performs a
> > selective read, it sends a mask, but it does not specify a size; the
> > implied size is the size of the last mask that has been set.
> > 
> > Specifying bits in the mask of selective read that are not available in
> > the internal mask should return an error, and bits past the end of the
> > mask should obviously be ignored. This can only be achieved by keeping
> > track of the lenght of the mask.
> > 
> > The mask length is thus now part of the internal state that needs to be
> > migrated.
> > 
> > This patch fixes the handling of selective reads, whose size will now
> > match the length of the event mask, as per architecture.
> > 
> > While the default behaviour is to be compliant with the architecture,
> > when using older machine models the old broken behaviour is selected
> > (allowing only masks of size exactly 4), 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>  
> 
> Looks sane and seems to match the docs.
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> One question below:
> 
> > ---
> >  hw/s390x/event-facility.c  | 81 ++++++++++++++++++++++++++++++++++++++--------
> >  hw/s390x/s390-virtio-ccw.c |  8 ++++-
> >  2 files changed, 75 insertions(+), 14 deletions(-)  
> [...9
> >  static void init_event_facility(Object *obj)
> >  {
> >      SCLPEventFacility *event_facility = EVENT_FACILITY(obj);
> >      DeviceState *sdev = DEVICE(obj);
> >      Object *new;
> > 
> > +    event_facility->mask_length = 4;  
> 
> Shouldnt we start with 0 here (as no mask is set)
> 
> and
> > +    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",\
> > +        },  
> 
> then set the mask_len here
> 
> or is this overkill? In the end it should not matter as the active mask is 0 so the
> length does not matter.

I think so (does not matter). I'll apply the patch as-is, unless
someone has complaints.

> 
> 
> > 
> >  #define CCW_COMPAT_2_10 \
> >          HW_COMPAT_2_10
> >   
> 

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

[Cornelia Huck]

On Fri, 23 Feb 2018 18:42:57 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> 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         | 20 ++++++++++----------
>  hw/s390x/sclpcpu.c                |  4 ++--
>  hw/s390x/sclpquiesce.c            |  4 ++--
>  include/hw/s390x/event-facility.h | 22 +++++++++++++---------
>  6 files changed, 31 insertions(+), 27 deletions(-)
> 

> @@ -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);

This gained a context conflict (trivial to fix, no need to resend.)

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

[Claudio Imbrenda]

On Tue, 6 Mar 2018 16:21:18 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 2 Mar 2018 10:09:16 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 02/23/2018 06:42 PM, Claudio Imbrenda wrote:  
> > > Until 67915de9f0383ccf4a ("s390x/event-facility: variable-length
> > > event masks") we only supported sclp event masks with a size of
> > > exactly 4 bytes, even though the architecture allows the guests
> > > to set up sclp event masks from 1 to 1021 bytes in length.
> > > After that patch, the behaviour was almost compliant, but some
> > > issues were still remaining, in particular regarding the handling
> > > of selective reads and migration.
> > > 
> > > When setting the sclp event mask, a mask size is also specified.
> > > Until now we only considered the size in order to decide which
> > > bits to save in the internal state. On the other hand, when a
> > > guest performs a selective read, it sends a mask, but it does not
> > > specify a size; the implied size is the size of the last mask
> > > that has been set.
> > > 
> > > Specifying bits in the mask of selective read that are not
> > > available in the internal mask should return an error, and bits
> > > past the end of the mask should obviously be ignored. This can
> > > only be achieved by keeping track of the lenght of the mask.
> > > 
> > > The mask length is thus now part of the internal state that needs
> > > to be migrated.
> > > 
> > > This patch fixes the handling of selective reads, whose size will
> > > now match the length of the event mask, as per architecture.
> > > 
> > > While the default behaviour is to be compliant with the
> > > architecture, when using older machine models the old broken
> > > behaviour is selected (allowing only masks of size exactly 4), 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>    
> > 
> > Looks sane and seems to match the docs.
> > 
> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > 
> > One question below:
> >   
> > > ---
> > >  hw/s390x/event-facility.c  | 81
> > > ++++++++++++++++++++++++++++++++++++++--------
> > > hw/s390x/s390-virtio-ccw.c |  8 ++++- 2 files changed, 75
> > > insertions(+), 14 deletions(-)    
> > [...9  
> > >  static void init_event_facility(Object *obj)
> > >  {
> > >      SCLPEventFacility *event_facility = EVENT_FACILITY(obj);
> > >      DeviceState *sdev = DEVICE(obj);
> > >      Object *new;
> > > 
> > > +    event_facility->mask_length = 4;    
> > 
> > Shouldnt we start with 0 here (as no mask is set)
> > 
> > and  
> > > +    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",\
> > > +        },    
> > 
> > then set the mask_len here
> > 
> > or is this overkill? In the end it should not matter as the active
> > mask is 0 so the length does not matter.  
> 
> I think so (does not matter). I'll apply the patch as-is, unless
> someone has complaints.

the size is 4 to keep backwards compatibility. The guest will set a
mask very early anyway, and that will be the new size.
The only time this could be a problem is if the guest performs a
selective read, but in that case it's also not an issue, as all the
bits in the internal mask are 0, so the selective read will fail no
matter what.
Also selective reads are not supposed to be issued before the mask is
set anyway.

so yes, as you said, it does not matter

> > 
> >   
> > > 
> > >  #define CCW_COMPAT_2_10 \
> > >          HW_COMPAT_2_10
> > >     
> >   
> 

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

[Cornelia Huck]

On Fri, 23 Feb 2018 18:42:55 +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")

Thanks, applied 1+2 (only patch 1 is needed for the fix.)