Re: [PATCH v9 09/15] s390x: protvirt: Set guest IPL PSW

[Janosch Frank]

On 3/13/20 1:57 PM, Claudio Imbrenda wrote:
> On Wed, 11 Mar 2020 09:21:45 -0400
> Janosch Frank <address@hidden> 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 <address@hidden>
>> Reviewed-by: Thomas Huth <address@hidden>
>> Reviewed-by: David Hildenbrand <address@hidden>
>> ---
>>  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.

signature.asc
Description: OpenPGP digital signature