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

[Alexander Graf <agraf@suse.de>]

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?

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

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.


Alex