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 <david@redhat.com> 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 <frankja@linux.ibm.com>
>>>> ---
>>>>  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.