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

[Alexander Graf <agraf@suse.de>]

On 25.06.14 13:27, Nikunj A Dadhania wrote:
> Alexander Graf <agraf@suse.de> writes:
>
>> On 25.06.14 06:36, Nikunj A Dadhania wrote:
>>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>>>
>>>> Alexander Graf <agraf@suse.de> writes:
>>>>
>>>>> On 17.06.14 11:59, Nikunj A Dadhania wrote:
>>>>>> Alexander Graf <agraf@suse.de> writes:
>>>>>>> On 17.06.14 11:30, Nikunj A Dadhania wrote:
>>>>>>>> Alexander Graf <agraf@suse.de> 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 <anton@samba.org>
>>>> 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 <anton@samba.org>
>>>>       Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> 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.

It's checking both for a reason. Find that reason.


Alex