diff mbox

[v4,1/3] s390x/sclp: proper support of larger send and receive masks

Message ID 1519407778-23095-2-git-send-email-imbrenda@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Claudio Imbrenda Feb. 23, 2018, 5:42 p.m. UTC 
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(-)

Comments

Christian Borntraeger March 2, 2018, 9:09 a.m. UTC | #1 
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
>
Cornelia Huck March 6, 2018, 3:21 p.m. UTC | #2 
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
> >   
>
Claudio Imbrenda March 6, 2018, 3:29 p.m. UTC | #3 
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
> > >     
> >   
>
diff mbox

Patch

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