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

[David Hildenbrand]

On 21.06.2018 18:21, Christian Borntraeger wrote:
> 
> 
> On 06/21/2018 06:20 PM, Christian Borntraeger wrote:
>>
>>
>> On 06/21/2018 06:15 PM, David Hildenbrand wrote:
>>> On 21.06.2018 17:49, Christian Borntraeger 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.
>>>>
>>>> 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;
>>>
>>> As the value is not use anywhere, you can make this less ugly by not
>>> setting it like this maybe
>>>
>>> if (reason != SHUTDOWN_CAUSE_GUEST_RESET_FORCE && no_reboot)
>>
>> I also change the reason from SHUTDOWN_CAUSE_GUEST_RESET_FORCE back
>> to  SHUTDOWN_CAUSE_GUEST_RESET. I think I have to, so that the handler do 
>> not need
>> to be modified. No?
> 
> Now I see you point. You say nobody uses the value?
> 

Yes, the only relevant place I see is

shutdown_caused_by_guest()

and that seems to work just fine with that change.

-- 

Thanks,

David / dhildenb