Re: [Qemu-devel] [PATCH] s390x/cpu: expose the guest crash information

[Christian Borntraeger]

On 09/18/2017 06:51 PM, David Hildenbrand wrote:
>>  # An enumeration of the guest panic information types
>>  #
>> +# @kvm-s390: s390 guest panic information type (Since: 2.11)
>> +#
>>  # Since: 2.9
>>  ##
>>  { 'enum': 'GuestPanicInformationType',
>> -  'data': [ 'hyper-v'] }
>> +  'data': [ 'hyper-v', 'kvm-s390' ] }
>>  
>>  ##
>>  # @GuestPanicInformation:
>> @@ -335,7 +337,8 @@
>>  {'union': 'GuestPanicInformation',
>>   'base': {'type': 'GuestPanicInformationType'},
>>   'discriminator': 'type',
>> - 'data': { 'hyper-v': 'GuestPanicInformationHyperV' } }
>> + 'data': { 'hyper-v': 'GuestPanicInformationHyperV',
>> +           'kvm-s390': 'GuestPanicInformationKVMS390' } }
> 
> kvm-s390 is wrong. This is general s390x and should be named s390/s390x
> and GuestPanicInformationS390(x). One can argue about s390 v s390x. See
> last comment in this mail.


Yes, will remove the kvm part and only use s390. Not sure about s390 vs. s390x.
I slightly prefer the s390 name as in the kernel but I really do not care if
we follow the s390x as in QEMU. Conny, any opinion?

[..]
>>  
>> +static GuestPanicInformation *s390x_cpu_get_crash_info(CPUState *cs)
>> +{
>> +    GuestPanicInformation *panic_info;
>> +    S390CPU *cpu = S390_CPU(cs);
>> +
>> +    cpu_synchronize_state(cs);
> 
> Is this the correct place here? Or rephrasing it: in order to detect an
> crash this should already have been done. E.g. unmanageable_intercept()
> uses cpu->env.psa, which has to be synchronized (especially for older
> kernels).
> 
> So we should rather make sure that all crash paths have synchronized the
> state (e.g. in unmanageable_intercept()).
> [...]

This code will also be called from common code (e.g. if KVM_RUN returns with 
EFAULT)
so I would like to keep it here. After all cpu_synchronize_state called be 
called
multiple times without any problems.

> 
>> index d07763f..950ea42 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1941,15 +1941,31 @@ static bool is_special_wait_psw(CPUState *cs)
>>      return cs->kvm_run->psw_addr == 0xfffUL;
>>  }
>>  
>> -static void unmanageable_intercept(S390CPU *cpu, const char *str, int 
>> pswoffset)
>> +static void unmanageable_intercept(S390CPU *cpu, int32_t reason, int 
>> pswoffset)
>>  {
>>      CPUState *cs = CPU(cpu);
>> +    const char *str;
>>  
>> +    switch (reason) {
>> +    case EXCP_CRASH_PGM:
>> +        str = "program interrupt";
> 
> program interrupt loop

Yes.

> 
>> +        break;
>> +    case EXCP_CRASH_EXT:
>> +        str = "external interrupt";
> 
> ... loop
> 


yes

> [...]