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

[Claudio Imbrenda]

On Wed, 7 Mar 2018 16:55:48 +0100
Cornelia Huck <address@hidden> wrote:

> On Wed,  7 Mar 2018 16:10:34 +0100
> Claudio Imbrenda <address@hidden> 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 <address@hidden>
> > ---
> >  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...

yes, this is better, I'll respin

> > +        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?)

well, by then we'll need proper bitfields, so everything will need to
be rewritten anyway :)

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