On Fri, 2 Mar 2018 10:09:16 +0100 Christian Borntraeger <borntrae...@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 <imbre...@linux.vnet.ibm.com> > > Looks sane and seems to match the docs. > > Reviewed-by: Christian Borntraeger <borntrae...@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 > > >