Re: [Qemu-devel] [PATCH v2] ppc: spapr-rtas - implement os-term rtas call

[Nikunj A Dadhania]

Alexander Graf <address@hidden> writes:

> On 25.06.14 06:36, Nikunj A Dadhania wrote:
>> Nikunj A Dadhania <address@hidden> writes:
>>
>>> Alexander Graf <address@hidden> writes:
>>>
>>>> On 17.06.14 11:59, Nikunj A Dadhania wrote:
>>>>> Alexander Graf <address@hidden> writes:
>>>>>> On 17.06.14 11:30, Nikunj A Dadhania wrote:
>>>>>>> Alexander Graf <address@hidden> writes:
>>>>>>>>> +    spapr_rtas_register("ibm,os-term", rtas_ibm_os_term);
>>>>>>>>> +    spapr_rtas_register("ibm,extended-os-term", 
>>>>>>>>> rtas_ibm_ext_os_term);
>>>>>>>> Why do we need the extended-os-term if we don't do anything with it?
>>>>>>> Linux kernel checks for both of them because of legacy:
>>>>>>>
>>>>>>> arch/powerpc/kernel/rtas.c:
>>>>>>>
>>>>>>> void rtas_os_term(char *str)
>>>>>>> {
>>>>>>> [...]
>>>>>>>            /*
>>>>>>>             * Firmware with the ibm,extended-os-term property is 
>>>>>>> guaranteed
>>>>>>>             * to always return from an ibm,os-term call. Earlier 
>>>>>>> versions without
>>>>>>>             * this property may terminate the partition which we want 
>>>>>>> to avoid
>>>>>>>             * since it interferes with panic_timeout.
>>>>>> But we do not return from the RTAS call, so we don't adhere to the
>>>>>> extended semantics?
>>>>> But you would return without calling os-term call if
>>>>> ibm,extended-os-term isnt registered. For that reason I h       ave 
>>>>> defined a
>>>>> stub.
>>>> I appreciate the hacker mentality, but Linux explicitly checks on
>>>> ibm,extended-os-term to ensure that the hypervisor does not stop the VM
>>>> when it calls ibm,os-term. However, the implementation above does stop
>>>> the VM when the guest calls ibm,os-term.
>>> Seems to be added to do just that:
>>>
>>> commit e9bbc8cde0e3c33b42ddbe1b02108cb5c97275eb
>>> Author: Anton Blanchard <address@hidden>
>>> Date:   Thu Feb 18 12:11:51 2010 +0000
>>>
>>>      powerpc/pseries: Call ibm,os-term if the ibm,extended-os-term is 
>>> present
>>>      
>>>      We have had issues in the past with ibm,os-term initiating shutdown of 
>>> a
>>>      partition. This is confusing to the user, especially if panic_timeout 
>>> is
>>>      non zero.
>>>      
>>>      The temporary fix was to avoid calling ibm,os-term if a panic_timeout 
>>> was set
>>>      and since we set it on every boot we basically never call ibm,os-term.
>>>      
>>>      An extended version of ibm,os-term has since been implemented which 
>>> gives us
>>>      the behaviour we want:
>>>      
>>>        "When the platform supports extended ibm,os-term behavior, the 
>>> return to the
>>>        RTAS will always occur unless there is a kernel assisted dump active 
>>> as
>>>        initiated by an ibm,configure-kernel-dump call."
>>>      
>>>      This patch checks for the ibm,extended-os-term property and calls 
>>> ibm,os-term
>>>      if it exists.
>>>      
>>>      Signed-off-by: Anton Blanchard <address@hidden>
>>>      Signed-off-by: Benjamin Herrenschmidt <address@hidden>
>>
>> I was thinking of the following:
>>
>> 1) Return the RTAS unsupported for extended-os-term
>> 2) A comment in the beginning of the function to suggest that this is a
>>     stub need for legacy of PowerVM
>>
>> Please let me know your thoughts.
>
> I think we need to clarify what bug Anton was trying to fix. The
> implementation you're proposing for os-term may "initiate a shutdown
> of a partition", albeit a hard stop usually. Is this what Linux is
> trying to avoid?

Let me put down my understanding:

There are two possible way to handle kernel panic:
1) Kdump service running in guest - already working
2) Pass the kernel panic information to hypervisor - not there in Qemu
   pseries

So without kdump service running, if linux kernel hits a panic, its going
to check os-term and extended-os-term, only then its going to call
os-term.

Now for what to do in os-term is the question, at present my code is
putting that to guest paniced state(paused)

> Did PowerVM try to inject a soft shutdown signal into the VM and this 
> check is to make sure we don't get that?

In this case, isnt that implementation dependent on what we do in qemu?

> I think we should make 100% sure we understand what situation the fix 
> above actually tried to avoid and make sure QEMU doesn't do it.

Regards
Nikunj