Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address

[Cornelia Huck]

On Wed, 26 Feb 2020 15:27:52 +0100
David Hildenbrand <address@hidden> wrote:

> On 26.02.20 13:20, Janosch Frank wrote:
> > Lets make it a bit more clear that we're extracting the 31 bit address

s/Lets/Let's/ :)

> > from the short psw.
> > 
> > Signed-off-by: Janosch Frank <address@hidden>
> > ---
> >  hw/s390x/ipl.c     | 2 +-
> >  target/s390x/cpu.c | 4 ++--
> >  target/s390x/cpu.h | 1 +
> >  3 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index 7773499d7f..42e21e7a6a 100644
> > --- a/hw/s390x/ipl.c
> > +++ b/hw/s390x/ipl.c
> > @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error 
> > **errp)
> >                  /* if not Linux load the address of the (short) IPL PSW */
> >                  ipl_psw = rom_ptr(4, 4);
> >                  if (ipl_psw) {
> > -                    pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
> > +                    pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
> >                  } else {
> >                      error_setg(&err, "Could not get IPL PSW");
> >                      goto error;
> > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> > index 8da1905485..43360912a0 100644
> > --- a/target/s390x/cpu.c
> > +++ b/target/s390x/cpu.c
> > @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
> >      S390CPU *cpu = S390_CPU(s);
> >      uint64_t spsw = ldq_phys(s->as, 0);
> >  
> > -    cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
> > +    cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
> >      /*
> >       * Invert short psw indication, so SIE will report a specification
> >       * exception if it was not set.
> >       */
> >      cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
> > -    cpu->env.psw.addr = spsw & 0x7fffffffULL;
> > +    cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
> >  
> >      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> >  }
> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index 8a557fd8d1..74e66fe0c2 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
> > @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
> >  #define PSW_MASK_64             0x0000000100000000ULL
> >  #define PSW_MASK_32             0x0000000080000000ULL
> >  #define PSW_MASK_ESA_ADDR       0x000000007fffffffULL
> > +#define PSW_MASK_ESA_MASK       0xffffffff80000000ULL  
> 
> ..._MASK_..._MASK
> 
> Isn't there a better name for all the bits in the PSW that are not an
> address?
> 
> PSW_MASK_ESA_BITS
> PSW_MASK_ESA_FLAGS
> ...

Hm, the PoP says that the PSW "includes the instruction address,
condition code, and other control fields"; it also talks about the
'short' PSW as being distinct from the 'ESA' PSW (bit 31 may be 0 or 1
in the short PSW). Maybe

PSW_MASK_SHORT_ADDR
PSW_MASK_SHORT_CTRL

(Or keep _ESA_ if renaming creates too much churn.)

> 
> >  
> >  #undef PSW_ASC_PRIMARY
> >  #undef PSW_ASC_ACCREG
> >   
> 
> 

This patch is also independent of the protected virtualization
support... I plan to send a pull request tomorrow, so I can include
this patch, if we agree on a name for the constant :)