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

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

[Nikunj A Dadhania]

PAPR compliant guest calls this in absence of kdump. After
receiving this call qemu could trigger a guest dump. This guest dump
can be used to analyse using crash tool.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---

v2: indentation fixes

 hw/ppc/spapr_rtas.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index ea4a2b2..f030e73 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -29,6 +29,8 @@
 #include "sysemu/char.h"
 #include "hw/qdev.h"
 #include "sysemu/device_tree.h"
+#include "qapi/qmp/qjson.h"
+#include "monitor/monitor.h"
 
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
@@ -267,6 +269,34 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
     rtas_st(rets, 0, ret);
 }
 
+static void rtas_ibm_os_term(PowerPCCPU *cpu,
+                             sPAPREnvironment *spapr,
+                             uint32_t token, uint32_t nargs,
+                             target_ulong args,
+                             uint32_t nret, target_ulong rets)
+{
+    target_ulong ret = 0;
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'action': %s }", "pause");
+    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+    qobject_decref(data);
+    vm_stop(RUN_STATE_GUEST_PANICKED);
+
+    rtas_st(rets, 0, ret);
+}
+
+static void rtas_ibm_ext_os_term(PowerPCCPU *cpu,
+                                 sPAPREnvironment *spapr,
+                                 uint32_t token, uint32_t nargs,
+                                 target_ulong args,
+                                 uint32_t nret, target_ulong rets)
+{
+    target_ulong ret = 0;
+
+    rtas_st(rets, 0, ret);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;
@@ -392,6 +422,8 @@ static void core_rtas_register_types(void)
                         rtas_ibm_get_system_parameter);
     spapr_rtas_register("ibm,set-system-parameter",
                         rtas_ibm_set_system_parameter);
+    spapr_rtas_register("ibm,os-term", rtas_ibm_os_term);
+    spapr_rtas_register("ibm,extended-os-term", rtas_ibm_ext_os_term);
 }
 
 type_init(core_rtas_register_types)
-- 
1.8.3.1

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

[Alexander Graf]

On 12.06.14 14:09, Nikunj A Dadhania wrote:
> PAPR compliant guest calls this in absence of kdump. After
> receiving this call qemu could trigger a guest dump. This guest dump
> can be used to analyse using crash tool.
>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>
> v2: indentation fixes
>
>   hw/ppc/spapr_rtas.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index ea4a2b2..f030e73 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -29,6 +29,8 @@
>   #include "sysemu/char.h"
>   #include "hw/qdev.h"
>   #include "sysemu/device_tree.h"
> +#include "qapi/qmp/qjson.h"
> +#include "monitor/monitor.h"
>   
>   #include "hw/ppc/spapr.h"
>   #include "hw/ppc/spapr_vio.h"
> @@ -267,6 +269,34 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>       rtas_st(rets, 0, ret);
>   }
>   
> +static void rtas_ibm_os_term(PowerPCCPU *cpu,
> +                             sPAPREnvironment *spapr,
> +                             uint32_t token, uint32_t nargs,
> +                             target_ulong args,
> +                             uint32_t nret, target_ulong rets)
> +{
> +    target_ulong ret = 0;
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'action': %s }", "pause");
> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
> +    qobject_decref(data);
> +    vm_stop(RUN_STATE_GUEST_PANICKED);
> +
> +    rtas_st(rets, 0, ret);
> +}
> +
> +static void rtas_ibm_ext_os_term(PowerPCCPU *cpu,
> +                                 sPAPREnvironment *spapr,
> +                                 uint32_t token, uint32_t nargs,
> +                                 target_ulong args,
> +                                 uint32_t nret, target_ulong rets)
> +{
> +    target_ulong ret = 0;
> +
> +    rtas_st(rets, 0, ret);
> +}
> +
>   static struct rtas_call {
>       const char *name;
>       spapr_rtas_fn fn;
> @@ -392,6 +422,8 @@ static void core_rtas_register_types(void)
>                           rtas_ibm_get_system_parameter);
>       spapr_rtas_register("ibm,set-system-parameter",
>                           rtas_ibm_set_system_parameter);
> +    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?


Alex


>   }
>   
>   type_init(core_rtas_register_types)

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:

> On 12.06.14 14:09, Nikunj A Dadhania wrote:
>> PAPR compliant guest calls this in absence of kdump. After
>> receiving this call qemu could trigger a guest dump. This guest dump
>> can be used to analyse using crash tool.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>
>> v2: indentation fixes
>>
>>   hw/ppc/spapr_rtas.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index ea4a2b2..f030e73 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -29,6 +29,8 @@
>>   #include "sysemu/char.h"
>>   #include "hw/qdev.h"
>>   #include "sysemu/device_tree.h"
>> +#include "qapi/qmp/qjson.h"
>> +#include "monitor/monitor.h"
>>   
>>   #include "hw/ppc/spapr.h"
>>   #include "hw/ppc/spapr_vio.h"
>> @@ -267,6 +269,34 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>>       rtas_st(rets, 0, ret);
>>   }
>>   
>> +static void rtas_ibm_os_term(PowerPCCPU *cpu,
>> +                             sPAPREnvironment *spapr,
>> +                             uint32_t token, uint32_t nargs,
>> +                             target_ulong args,
>> +                             uint32_t nret, target_ulong rets)
>> +{
>> +    target_ulong ret = 0;
>> +    QObject *data;
>> +
>> +    data = qobject_from_jsonf("{ 'action': %s }", "pause");
>> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
>> +    qobject_decref(data);
>> +    vm_stop(RUN_STATE_GUEST_PANICKED);
>> +
>> +    rtas_st(rets, 0, ret);
>> +}
>> +
>> +static void rtas_ibm_ext_os_term(PowerPCCPU *cpu,
>> +                                 sPAPREnvironment *spapr,
>> +                                 uint32_t token, uint32_t nargs,
>> +                                 target_ulong args,
>> +                                 uint32_t nret, target_ulong rets)
>> +{
>> +    target_ulong ret = 0;
>> +
>> +    rtas_st(rets, 0, ret);
>> +}
>> +
>>   static struct rtas_call {
>>       const char *name;
>>       spapr_rtas_fn fn;
>> @@ -392,6 +422,8 @@ static void core_rtas_register_types(void)
>>                           rtas_ibm_get_system_parameter);
>>       spapr_rtas_register("ibm,set-system-parameter",
>>                           rtas_ibm_set_system_parameter);
>> +    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.
         */
        if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") ||
            RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-term"))
                return;

[...]
}

Regards,
Nikunj

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

[Alexander Graf]

On 17.06.14 11:30, Nikunj A Dadhania wrote:
> Alexander Graf <agraf@suse.de> writes:
>
>> On 12.06.14 14:09, Nikunj A Dadhania wrote:
>>> PAPR compliant guest calls this in absence of kdump. After
>>> receiving this call qemu could trigger a guest dump. This guest dump
>>> can be used to analyse using crash tool.
>>>
>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>>> ---
>>>
>>> v2: indentation fixes
>>>
>>>    hw/ppc/spapr_rtas.c | 32 ++++++++++++++++++++++++++++++++
>>>    1 file changed, 32 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index ea4a2b2..f030e73 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -29,6 +29,8 @@
>>>    #include "sysemu/char.h"
>>>    #include "hw/qdev.h"
>>>    #include "sysemu/device_tree.h"
>>> +#include "qapi/qmp/qjson.h"
>>> +#include "monitor/monitor.h"
>>>    
>>>    #include "hw/ppc/spapr.h"
>>>    #include "hw/ppc/spapr_vio.h"
>>> @@ -267,6 +269,34 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>>>        rtas_st(rets, 0, ret);
>>>    }
>>>    
>>> +static void rtas_ibm_os_term(PowerPCCPU *cpu,
>>> +                             sPAPREnvironment *spapr,
>>> +                             uint32_t token, uint32_t nargs,
>>> +                             target_ulong args,
>>> +                             uint32_t nret, target_ulong rets)
>>> +{
>>> +    target_ulong ret = 0;
>>> +    QObject *data;
>>> +
>>> +    data = qobject_from_jsonf("{ 'action': %s }", "pause");
>>> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
>>> +    qobject_decref(data);
>>> +    vm_stop(RUN_STATE_GUEST_PANICKED);
>>> +
>>> +    rtas_st(rets, 0, ret);
>>> +}
>>> +
>>> +static void rtas_ibm_ext_os_term(PowerPCCPU *cpu,
>>> +                                 sPAPREnvironment *spapr,
>>> +                                 uint32_t token, uint32_t nargs,
>>> +                                 target_ulong args,
>>> +                                 uint32_t nret, target_ulong rets)
>>> +{
>>> +    target_ulong ret = 0;
>>> +
>>> +    rtas_st(rets, 0, ret);
>>> +}
>>> +
>>>    static struct rtas_call {
>>>        const char *name;
>>>        spapr_rtas_fn fn;
>>> @@ -392,6 +422,8 @@ static void core_rtas_register_types(void)
>>>                            rtas_ibm_get_system_parameter);
>>>        spapr_rtas_register("ibm,set-system-parameter",
>>>                            rtas_ibm_set_system_parameter);
>>> +    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?


Alex

>           */
>          if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") ||
>              RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-term"))
>                  return;
>
> [...]
> }
>
> Regards,
> Nikunj
>

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:

> On 17.06.14 11:30, Nikunj A Dadhania wrote:
>> Alexander Graf <agraf@suse.de> writes:
>>
>>> On 12.06.14 14:09, Nikunj A Dadhania wrote:
>>>> PAPR compliant guest calls this in absence of kdump. After
>>>> receiving this call qemu could trigger a guest dump. This guest dump
>>>> can be used to analyse using crash tool.
>>>>
>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>>>> ---
>>>>
>>>> v2: indentation fixes
>>>>
>>>>    hw/ppc/spapr_rtas.c | 32 ++++++++++++++++++++++++++++++++
>>>>    1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>> index ea4a2b2..f030e73 100644
>>>> --- a/hw/ppc/spapr_rtas.c
>>>> +++ b/hw/ppc/spapr_rtas.c
>>>> @@ -29,6 +29,8 @@
>>>>    #include "sysemu/char.h"
>>>>    #include "hw/qdev.h"
>>>>    #include "sysemu/device_tree.h"
>>>> +#include "qapi/qmp/qjson.h"
>>>> +#include "monitor/monitor.h"
>>>>    
>>>>    #include "hw/ppc/spapr.h"
>>>>    #include "hw/ppc/spapr_vio.h"
>>>> @@ -267,6 +269,34 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>>>>        rtas_st(rets, 0, ret);
>>>>    }
>>>>    
>>>> +static void rtas_ibm_os_term(PowerPCCPU *cpu,
>>>> +                             sPAPREnvironment *spapr,
>>>> +                             uint32_t token, uint32_t nargs,
>>>> +                             target_ulong args,
>>>> +                             uint32_t nret, target_ulong rets)
>>>> +{
>>>> +    target_ulong ret = 0;
>>>> +    QObject *data;
>>>> +
>>>> +    data = qobject_from_jsonf("{ 'action': %s }", "pause");
>>>> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
>>>> +    qobject_decref(data);
>>>> +    vm_stop(RUN_STATE_GUEST_PANICKED);
>>>> +
>>>> +    rtas_st(rets, 0, ret);
>>>> +}
>>>> +
>>>> +static void rtas_ibm_ext_os_term(PowerPCCPU *cpu,
>>>> +                                 sPAPREnvironment *spapr,
>>>> +                                 uint32_t token, uint32_t nargs,
>>>> +                                 target_ulong args,
>>>> +                                 uint32_t nret, target_ulong rets)
>>>> +{
>>>> +    target_ulong ret = 0;
>>>> +
>>>> +    rtas_st(rets, 0, ret);
>>>> +}
>>>> +
>>>>    static struct rtas_call {
>>>>        const char *name;
>>>>        spapr_rtas_fn fn;
>>>> @@ -392,6 +422,8 @@ static void core_rtas_register_types(void)
>>>>                            rtas_ibm_get_system_parameter);
>>>>        spapr_rtas_register("ibm,set-system-parameter",
>>>>                            rtas_ibm_set_system_parameter);
>>>> +    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.

Regards
Nikunj

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

[Alexander Graf]

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:
>>>
>>>> On 12.06.14 14:09, Nikunj A Dadhania wrote:
>>>>> PAPR compliant guest calls this in absence of kdump. After
>>>>> receiving this call qemu could trigger a guest dump. This guest dump
>>>>> can be used to analyse using crash tool.
>>>>>
>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>>>>> ---
>>>>>
>>>>> v2: indentation fixes
>>>>>
>>>>>     hw/ppc/spapr_rtas.c | 32 ++++++++++++++++++++++++++++++++
>>>>>     1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>> index ea4a2b2..f030e73 100644
>>>>> --- a/hw/ppc/spapr_rtas.c
>>>>> +++ b/hw/ppc/spapr_rtas.c
>>>>> @@ -29,6 +29,8 @@
>>>>>     #include "sysemu/char.h"
>>>>>     #include "hw/qdev.h"
>>>>>     #include "sysemu/device_tree.h"
>>>>> +#include "qapi/qmp/qjson.h"
>>>>> +#include "monitor/monitor.h"
>>>>>     
>>>>>     #include "hw/ppc/spapr.h"
>>>>>     #include "hw/ppc/spapr_vio.h"
>>>>> @@ -267,6 +269,34 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>>>>>         rtas_st(rets, 0, ret);
>>>>>     }
>>>>>     
>>>>> +static void rtas_ibm_os_term(PowerPCCPU *cpu,
>>>>> +                             sPAPREnvironment *spapr,
>>>>> +                             uint32_t token, uint32_t nargs,
>>>>> +                             target_ulong args,
>>>>> +                             uint32_t nret, target_ulong rets)
>>>>> +{
>>>>> +    target_ulong ret = 0;
>>>>> +    QObject *data;
>>>>> +
>>>>> +    data = qobject_from_jsonf("{ 'action': %s }", "pause");
>>>>> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
>>>>> +    qobject_decref(data);
>>>>> +    vm_stop(RUN_STATE_GUEST_PANICKED);
>>>>> +
>>>>> +    rtas_st(rets, 0, ret);
>>>>> +}
>>>>> +
>>>>> +static void rtas_ibm_ext_os_term(PowerPCCPU *cpu,
>>>>> +                                 sPAPREnvironment *spapr,
>>>>> +                                 uint32_t token, uint32_t nargs,
>>>>> +                                 target_ulong args,
>>>>> +                                 uint32_t nret, target_ulong rets)
>>>>> +{
>>>>> +    target_ulong ret = 0;
>>>>> +
>>>>> +    rtas_st(rets, 0, ret);
>>>>> +}
>>>>> +
>>>>>     static struct rtas_call {
>>>>>         const char *name;
>>>>>         spapr_rtas_fn fn;
>>>>> @@ -392,6 +422,8 @@ static void core_rtas_register_types(void)
>>>>>                             rtas_ibm_get_system_parameter);
>>>>>         spapr_rtas_register("ibm,set-system-parameter",
>>>>>                             rtas_ibm_set_system_parameter);
>>>>> +    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.

Why was that check put into place?


Alex

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

[Nikunj A Dadhania]

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>

> Why was that check put into place?


Regards
Nikunj

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

[Nikunj A Dadhania]

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.

Regards,
Nikunj

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

[Alexander Graf]

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

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

[Nikunj A Dadhania]

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.

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

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

[Alexander Graf]

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

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:

> On 25.06.14 13:27, Nikunj A Dadhania wrote:
>> Alexander Graf <agraf@suse.de> writes:
>>
>> 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.

Here is what I figured out from PAPR:

rtas ibm,os-term, does not gaurantee a return back to the guest cpu.

If ibm,extended-os-term property is set rtas call return will always
occur.

Regards
Nikunj

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

[Alexander Graf]

> Am 26.06.2014 um 09:55 schrieb Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>:
> 
> Alexander Graf <agraf@suse.de> writes:
> 
>>> On 25.06.14 13:27, Nikunj A Dadhania wrote:
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>> 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.
> 
> Here is what I figured out from PAPR:
> 
> rtas ibm,os-term, does not gaurantee a return back to the guest cpu.
> 
> If ibm,extended-os-term property is set rtas call return will always
> occur.

With your patch it doesn't occur.

Alex

> 
> Regards
> Nikunj
> 

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:

>> Am 26.06.2014 um 09:55 schrieb Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>:
>> 
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>>> On 25.06.14 13:27, Nikunj A Dadhania wrote:
>>>> Alexander Graf <agraf@suse.de> writes:
>>>> 
>>>> 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.
>> 
>> Here is what I figured out from PAPR:
>> 
>> rtas ibm,os-term, does not gaurantee a return back to the guest cpu.
>> 
>> If ibm,extended-os-term property is set rtas call return will always
>> occur.
>
> With your patch it doesn't occur.

Hmm, you are right.

+    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+    qobject_decref(data);
+    vm_stop(RUN_STATE_GUEST_PANICKED);

So the issue here is using vm_stop. So if I do not do this I will be
returning back to the guest.

I was trying to enable:

<on_crash> coredump-restart </on_crash>
<on_crash> coredump-destroy </on_crash>

in libvirt. But anyways, when libvirt receives these event, it will
either destroy or restart the guest. So if I remove the vm_stop, there
will be a return to the guest. And depending on the libvirt domain
config a restart/destroy will take effect. Does this look ok?

Regards
Nikunj

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

[Alexander Graf]

On 26.06.14 11:05, Nikunj A Dadhania wrote:
> Alexander Graf <agraf@suse.de> writes:
>
>>> Am 26.06.2014 um 09:55 schrieb Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>:
>>>
>>> Alexander Graf <agraf@suse.de> writes:
>>>
>>>>> On 25.06.14 13:27, Nikunj A Dadhania wrote:
>>>>> Alexander Graf <agraf@suse.de> writes:
>>>>>
>>>>> 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.
>>> Here is what I figured out from PAPR:
>>>
>>> rtas ibm,os-term, does not gaurantee a return back to the guest cpu.
>>>
>>> If ibm,extended-os-term property is set rtas call return will always
>>> occur.
>> With your patch it doesn't occur.
> Hmm, you are right.
>
> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
> +    qobject_decref(data);
> +    vm_stop(RUN_STATE_GUEST_PANICKED);
>
> So the issue here is using vm_stop. So if I do not do this I will be
> returning back to the guest.
>
> I was trying to enable:
>
> <on_crash> coredump-restart </on_crash>
> <on_crash> coredump-destroy </on_crash>
>
> in libvirt. But anyways, when libvirt receives these event, it will
> either destroy or restart the guest. So if I remove the vm_stop, there
> will be a return to the guest. And depending on the libvirt domain
> config a restart/destroy will take effect. Does this look ok?

I think that's closer to what Linux expects, yes. But I'd like to have 
an ack on that approach from Ben or Anton.


Alex