| Message ID | 1519407778-23095-4-git-send-email-imbrenda@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/sclp: 64 bit event masks | expand |
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 :)
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 :) >
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; >
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; > > >
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"?
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.
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.
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; > > > > > >
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 :(
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
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;
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(-)