| Message ID | 20200226122038.61481-2-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
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 >>
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 >
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
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.
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
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(-)