Lets make it a bit more clear that we're extracting the 31 bit address
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
#undef PSW_ASC_PRIMARY
#undef PSW_ASC_ACCREG
--
2.20.1
On 26.02.20 13:20, Janosch Frank wrote:
> Lets make it a bit more clear that we're extracting the 31 bit address
> 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
...
>
> #undef PSW_ASC_PRIMARY
> #undef PSW_ASC_ACCREG
>
--
Thanks,
David / dhildenb
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/ :)
> > 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
(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 :)
On 26.02.20 18:51, 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/ :)
>
>>> 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
+1
--
Thanks,
David / dhildenb
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.
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.
On Thu, 27 Feb 2020 09:09:47 +0100
Janosch Frank <frankja@linux.ibm.com> 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 <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.
A quick respin of this patch would be easiest for me.
Let's rename PSW_MASK_ESA_ADDR to PSW_MASK_SHORT_ADDR because we're
not working with a ESA PSW which would not support the extended
addressing bit. Also let's actually use it.
Additionally we introduce PSW_MASK_SHORT_CTRL and use it throughout
the codebase.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
hw/s390x/ipl.c | 2 +-
target/s390x/cpu.c | 4 ++--
target/s390x/cpu.h | 3 ++-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 7773499d7f..e36f770a72 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_SHORT_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..3dd396e870 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_SHORT_CTRL;
/*
* 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_SHORT_ADDR;
s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 8a557fd8d1..1d17709d6e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -276,7 +276,8 @@ extern const VMStateDescription vmstate_s390_cpu;
#define PSW_MASK_RI 0x0000008000000000ULL
#define PSW_MASK_64 0x0000000100000000ULL
#define PSW_MASK_32 0x0000000080000000ULL
-#define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
+#define PSW_MASK_SHORT_ADDR 0x000000007fffffffULL
+#define PSW_MASK_SHORT_CTRL 0xffffffff80000000ULL
#undef PSW_ASC_PRIMARY
#undef PSW_ASC_ACCREG
--
2.20.1
On Thu, 27 Feb 2020 04:23:41 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > Let's rename PSW_MASK_ESA_ADDR to PSW_MASK_SHORT_ADDR because we're > not working with a ESA PSW which would not support the extended > addressing bit. Also let's actually use it. > > Additionally we introduce PSW_MASK_SHORT_CTRL and use it throughout > the codebase. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/s390x/ipl.c | 2 +- > target/s390x/cpu.c | 4 ++-- > target/s390x/cpu.h | 3 ++- > 3 files changed, 5 insertions(+), 4 deletions(-) Thanks, applied.
On 27.02.20 10:23, Janosch Frank wrote:
> Let's rename PSW_MASK_ESA_ADDR to PSW_MASK_SHORT_ADDR because we're
> not working with a ESA PSW which would not support the extended
> addressing bit. Also let's actually use it.
>
> Additionally we introduce PSW_MASK_SHORT_CTRL and use it throughout
> the codebase.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390x/ipl.c | 2 +-
> target/s390x/cpu.c | 4 ++--
> target/s390x/cpu.h | 3 ++-
> 3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 7773499d7f..e36f770a72 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_SHORT_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..3dd396e870 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_SHORT_CTRL;
> /*
> * 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_SHORT_ADDR;
>
> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> }
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 8a557fd8d1..1d17709d6e 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -276,7 +276,8 @@ extern const VMStateDescription vmstate_s390_cpu;
> #define PSW_MASK_RI 0x0000008000000000ULL
> #define PSW_MASK_64 0x0000000100000000ULL
> #define PSW_MASK_32 0x0000000080000000ULL
> -#define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
> +#define PSW_MASK_SHORT_ADDR 0x000000007fffffffULL
> +#define PSW_MASK_SHORT_CTRL 0xffffffff80000000ULL
>
> #undef PSW_ASC_PRIMARY
> #undef PSW_ASC_ACCREG
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
On 26.02.20 13:20, Janosch Frank wrote:
> Lets make it a bit more clear that we're extracting the 31 bit address
> from the short psw.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
PSW_MASK_ESA_MASK and PSW_MASK_ESA_ADDR look pretty similar, but I cant find
a good name. We could use ~PSW_MASK_ESA_ADDR as an alternative.
Either way, this makes sense.
Reviewed-by: Christian Borntraeger <borntraeger@de.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
>
> #undef PSW_ASC_PRIMARY
> #undef PSW_ASC_ACCREG
>
On 2/26/20 2:46 PM, Christian Borntraeger 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
>> from the short psw.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>
>
>
> PSW_MASK_ESA_MASK and PSW_MASK_ESA_ADDR look pretty similar, but I cant find
> a good name. We could use ~PSW_MASK_ESA_ADDR as an alternative.
I'm also not too happy about it, if there's a better suggestion I'd take
it any day.
>
> Either way, this makes sense.
>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Thanks!
>
>> ---
>> 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
>>
>> #undef PSW_ASC_PRIMARY
>> #undef PSW_ASC_ACCREG
>>
© 2016 - 2025 Red Hat, Inc.