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

[Cornelia Huck <cohuck@redhat.com>]

On Wed,  7 Mar 2018 16:10:34 +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         | 49 ++++++++++++++++++++++++++++++++-------
>  include/hw/s390x/event-facility.h |  2 +-
>  2 files changed, 41 insertions(+), 10 deletions(-)
> 

> @@ -376,6 +387,21 @@ 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",

Should that really be called 'mask64' - you're just transferring the
missing part of the mask here, aren't you?

> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .needed = vmstate_event_facility_mask64_needed,
> +    .fields = (VMStateField[]) {
> +#ifdef HOST_WORDS_BIGENDIAN
> +        VMSTATE_UINT32(receive_mask_pieces[1], SCLPEventFacility),
> +#else
> +        VMSTATE_UINT32(receive_mask_pieces[0], SCLPEventFacility),
> +#endif

That #ifdef is kind of ugly, and a bit confusing, I think.

What about doing something like

/* we need to save 32 bit chunks for compatibility */
#ifdef HOST_WORDS_BIGENDIAN
#define RECV_MASK_LOWER 0
#define RECV_MASK_UPPER 1
#else /* little endian host */
#define RECV_MASK_LOWER 1
#define RECV_MASK_UPPER 0
#endif

and transfer receive_mask_pieces[RECV_MASK_UPPER] here...

> +        VMSTATE_END_OF_LIST()
> +     }
> +};
> +
>  static const VMStateDescription vmstate_event_facility_mask_length = {
>      .name = "vmstate-event-facility/mask_length",
>      .version_id = 0,
> @@ -392,10 +418,15 @@ static const VMStateDescription vmstate_event_facility = {
>      .version_id = 0,
>      .minimum_version_id = 0,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(receive_mask, SCLPEventFacility),
> +#ifdef HOST_WORDS_BIGENDIAN
> +        VMSTATE_UINT32(receive_mask_pieces[0], SCLPEventFacility),
> +#else
> +        VMSTATE_UINT32(receive_mask_pieces[1], SCLPEventFacility),
> +#endif

...and receive_mask_pieces[REV_MASK_LOWER] here?

(And I guess 64bit receive masks should be enough for everybody? IOW, we
don't expect a similar dance in the near future?)

>          VMSTATE_END_OF_LIST()
>       },
>      .subsections = (const VMStateDescription * []) {
> +        &vmstate_event_facility_mask64,
>          &vmstate_event_facility_mask_length,
>          NULL
>       }