| Message ID | 20200311132151.172389-10-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
On 11.03.20 14:21, Janosch Frank wrote: > Handling of CPU reset and setting of the IPL psw from guest storage at > offset 0 is done by a Ultravisor call. Let's only fetch it if > necessary. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: David Hildenbrand <david@redhat.com> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > target/s390x/cpu.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index 84029f14814b4980..a48d39f139cdc1c4 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -78,16 +78,20 @@ static bool s390_cpu_has_work(CPUState *cs) > 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 & 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 & PSW_MASK_SHORT_ADDR; > + uint64_t spsw; > > + if (!s390_is_pv()) { > + spsw = ldq_phys(s->as, 0); > + 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 & PSW_MASK_SHORT_ADDR; > + } else { > + s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu); > + } > s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > } > #endif >
On Wed, 11 Mar 2020 09:21:45 -0400 Janosch Frank <frankja@linux.ibm.com> wrote: > Handling of CPU reset and setting of the IPL psw from guest storage at > offset 0 is done by a Ultravisor call. Let's only fetch it if > necessary. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/cpu.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index 84029f14814b4980..a48d39f139cdc1c4 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -78,16 +78,20 @@ static bool s390_cpu_has_work(CPUState *cs) > 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 & 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 & PSW_MASK_SHORT_ADDR; > + uint64_t spsw; > > + if (!s390_is_pv()) { > + spsw = ldq_phys(s->as, 0); > + 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 & PSW_MASK_SHORT_ADDR; > + } else { > + s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu); > + } > s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > } > #endif I don't understand why you set the state to S390_CPU_STATE_LOAD and then immediately afterwards to S390_CPU_STATE_OPERATING, especially considering that both do the same
On 3/13/20 1:57 PM, Claudio Imbrenda wrote: > On Wed, 11 Mar 2020 09:21:45 -0400 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> Handling of CPU reset and setting of the IPL psw from guest storage at >> offset 0 is done by a Ultravisor call. Let's only fetch it if >> necessary. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> Reviewed-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/cpu.c | 22 +++++++++++++--------- >> 1 file changed, 13 insertions(+), 9 deletions(-) >> >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c >> index 84029f14814b4980..a48d39f139cdc1c4 100644 >> --- a/target/s390x/cpu.c >> +++ b/target/s390x/cpu.c >> @@ -78,16 +78,20 @@ static bool s390_cpu_has_work(CPUState *cs) >> 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 & 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 & PSW_MASK_SHORT_ADDR; >> + uint64_t spsw; >> >> + if (!s390_is_pv()) { >> + spsw = ldq_phys(s->as, 0); >> + 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 & PSW_MASK_SHORT_ADDR; >> + } else { >> + s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu); >> + } >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >> } >> #endif > > I don't understand why you set the state to S390_CPU_STATE_LOAD and > then immediately afterwards to S390_CPU_STATE_OPERATING, especially > considering that both do the same > Have a look at the specs, wee need to set the load state before setting the cpu to operating. I can add a comment to make it clearer if you want.
On 13.03.20 15:21, Janosch Frank wrote: [..] >>> + } else { >>> + s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu); >>> + } >>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >>> } >>> #endif >> >> I don't understand why you set the state to S390_CPU_STATE_LOAD and >> then immediately afterwards to S390_CPU_STATE_OPERATING, especially >> considering that both do the same >> > > Have a look at the specs, wee need to set the load state before setting > the cpu to operating. > > I can add a comment to make it clearer if you want. Yes please. I stumbled over this as well before I read the specs.
On Fri, 13 Mar 2020 15:21:07 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 3/13/20 1:57 PM, Claudio Imbrenda wrote: > > On Wed, 11 Mar 2020 09:21:45 -0400 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > >> Handling of CPU reset and setting of the IPL psw from guest > >> storage at offset 0 is done by a Ultravisor call. Let's only fetch > >> it if necessary. > >> > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> Reviewed-by: Thomas Huth <thuth@redhat.com> > >> Reviewed-by: David Hildenbrand <david@redhat.com> > >> --- > >> target/s390x/cpu.c | 22 +++++++++++++--------- > >> 1 file changed, 13 insertions(+), 9 deletions(-) > >> > >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > >> index 84029f14814b4980..a48d39f139cdc1c4 100644 > >> --- a/target/s390x/cpu.c > >> +++ b/target/s390x/cpu.c > >> @@ -78,16 +78,20 @@ static bool s390_cpu_has_work(CPUState *cs) > >> 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 & 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 & PSW_MASK_SHORT_ADDR; > >> + uint64_t spsw; > >> > >> + if (!s390_is_pv()) { > >> + spsw = ldq_phys(s->as, 0); > >> + 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 & PSW_MASK_SHORT_ADDR; > >> + } else { > >> + s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu); > >> + } > >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > >> } > >> #endif > > > > I don't understand why you set the state to S390_CPU_STATE_LOAD and > > then immediately afterwards to S390_CPU_STATE_OPERATING, especially > > considering that both do the same > > > > Have a look at the specs, wee need to set the load state before > setting the cpu to operating. > > I can add a comment to make it clearer if you want. yes please.
On Fri, 13 Mar 2020 15:21:07 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 3/13/20 1:57 PM, Claudio Imbrenda wrote: > > On Wed, 11 Mar 2020 09:21:45 -0400 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > >> Handling of CPU reset and setting of the IPL psw from guest > >> storage at offset 0 is done by a Ultravisor call. Let's only fetch > >> it if necessary. > >> > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> Reviewed-by: Thomas Huth <thuth@redhat.com> > >> Reviewed-by: David Hildenbrand <david@redhat.com> > >> --- > >> target/s390x/cpu.c | 22 +++++++++++++--------- > >> 1 file changed, 13 insertions(+), 9 deletions(-) > >> > >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > >> index 84029f14814b4980..a48d39f139cdc1c4 100644 > >> --- a/target/s390x/cpu.c > >> +++ b/target/s390x/cpu.c > >> @@ -78,16 +78,20 @@ static bool s390_cpu_has_work(CPUState *cs) > >> 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 & 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 & PSW_MASK_SHORT_ADDR; > >> + uint64_t spsw; > >> > >> + if (!s390_is_pv()) { > >> + spsw = ldq_phys(s->as, 0); > >> + 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 & PSW_MASK_SHORT_ADDR; > >> + } else { > >> + s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu); > >> + } > >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > >> } > >> #endif > > > > I don't understand why you set the state to S390_CPU_STATE_LOAD and > > then immediately afterwards to S390_CPU_STATE_OPERATING, especially > > considering that both do the same > > > > Have a look at the specs, wee need to set the load state before > setting the cpu to operating. > > I can add a comment to make it clearer if you want. once you have added the comment, you can also add: Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 84029f14814b4980..a48d39f139cdc1c4 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -78,16 +78,20 @@ static bool s390_cpu_has_work(CPUState *cs) 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 & 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 & PSW_MASK_SHORT_ADDR; + uint64_t spsw; + if (!s390_is_pv()) { + spsw = ldq_phys(s->as, 0); + 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 & PSW_MASK_SHORT_ADDR; + } else { + s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu); + } s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); } #endif