qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 3/3] s390x/sclp: extend SCLP event masks to 6


From: Claudio Imbrenda
Subject: Re: [Qemu-devel] [PATCH v4 3/3] s390x/sclp: extend SCLP event masks to 64 bits
Date: Tue, 6 Mar 2018 15:26:23 +0100

On Mon, 26 Feb 2018 17:57:51 +0100
Cornelia Huck <address@hidden> wrote:

> On Fri, 23 Feb 2018 18:42:58 +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         | 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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]