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

[Cornelia Huck]

On Thu, 27 Feb 2020 09:09:47 +0100
Janosch Frank <address@hidden> wrote:

> On 2/27/20 8:53 AM, Janosch Frank wrote:
> > On 2/26/20 6:51 PM, Cornelia Huck wrote:  
> >> 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/ :)  
> > 
> > Ack
> >   
> >>  
> >>>> 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  
> > 
> > Sure, why not
> >   
> >>
> >> (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 :)  
> > 
> > Well, you would also need to rename all users of PSW_MASK_ESA_ADDR
> > Let me split that up into two patches, the rename for the ADDR and this
> > one. I'll send it out once I'm more or less awake.  
> 
> Seems like the ADDR constant has never been used anyway...
> Ok, I renounce everything I said before, if you want to fix this up
> yourself that would be wonderful, if not I'd be happy to provide you
> with a patch.

A quick respin of this patch would be easiest for me.

pgpUMkqhg3766.pgp
Description: OpenPGP digital signature