Re: [qemu-s390x] [PATCH v1] s390x: refactor reset/reipl handling

[Christian Borntraeger]

On 06/21/2018 06:04 PM, Cornelia Huck wrote:
> On Thu, 21 Jun 2018 17:49:56 +0200
> Christian Borntraeger <address@hidden> wrote:
> 
>> On 04/24/2018 12:18 PM, David Hildenbrand wrote:
>>> Calling pause_all_vcpus()/resume_all_vcpus() from a VCPU thread might
>>> not be the best idea. As pause_all_vcpus() temporarily drops the qemu
>>> mutex, two parallel calls to pause_all_vcpus() can be active at a time,
>>> resulting in a deadlock. (either by two VCPUs or by the main thread and a
>>> VCPU)
>>>
>>> Let's handle it via the main loop instead, as suggested by Paolo. If we
>>> would have two parallel reset requests by two different VCPUs at the
>>> same time, the last one would win.
>>>
>>> We use the existing ipl device to handle it. The nice side effect is
>>> that we can get rid of reipl_requested.
>>>
>>> This change implies that all reset handling now goes via the common
>>> path, so "no-reboot" handling is now active for all kinds of reboots.  
>>
>> Ok, this breaks the s390 IPL process when -no-reboot  is specified.
>> The bios does a diagnose 308 subcode 1 to jump to the final image while
>> at the same time resetting all devices. This is now blocked with -no-reboot
>> (although it is actually the boot)
>>
>>
>> I have noticed that with virt-install on iso images since virt-install
>> specifies -no-reboot.
>>
>> Something like this seems to help but it is not a nice solution.
> 
> It's a bit ugly, but allows us to accommodate the different semantics
> for the different diag subcodes... the -no-reboot parameter is only
> supposed to suppress normal guest reboot requests, right?

At least thats how it worked until 2.12. Without this we have no kdump/kexec/ipl
as soon as somebody uses -no-reboot.


Shall I respin my patch as proper patch?
> 
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d67349004..7b32698eaa 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -534,8 +534,14 @@ void s390_ipl_reset_request(CPUState *cs, enum 
>> s390_reset reset_type)
>>               */
>>              ipl->iplb_valid = s390_gen_initial_iplb(ipl);
>>          }
>> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> +    } else  if (reset_type == S390_RESET_MODIFIED_CLEAR ||
>> +                reset_type == S390_RESET_LOAD_NORMAL) {
>> +    /* ignore -no-reboot */
>> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET_FORCE);
>> +    } else {
>> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>      }
>> -    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>      /* as this is triggered by a CPU, make sure to exit the loop */
>>      if (tcg_enabled()) {
>>          cpu_loop_exit(cs);
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index e893f72f3b..e9b11fd6cb 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -44,6 +44,9 @@ typedef enum ShutdownCause {
>>                                       turns that into a shutdown */
>>      SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
>>                                       that into a shutdown */
>> +    SHUTDOWN_CAUSE_GUEST_RESET_FORCE,/* Guest reset that should ignore 
>> --no-reboot
>> +                                     useful for the subsystem resets on 
>> s390 done
>> +                                     for kdump and kexec */
>>      SHUTDOWN_CAUSE__MAX,
>>  } ShutdownCause;
>>  
>> diff --git a/vl.c b/vl.c
>> index b3426e03d0..18f379e833 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1674,7 +1674,9 @@ void qemu_system_guest_panicked(GuestPanicInformation 
>> *info)
>>  
>>  void qemu_system_reset_request(ShutdownCause reason)
>>  {
>> -    if (no_reboot) {
>> +    if (reason == SHUTDOWN_CAUSE_GUEST_RESET_FORCE) {
>> +        reset_requested = SHUTDOWN_CAUSE_GUEST_RESET;
>> +    } else if (no_reboot) {
>>          shutdown_requested = reason;
>>      } else {
>>          reset_requested = reason;
>