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

[Alexander Graf <agraf@suse.de>]

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