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

[Alexander Graf]

> Am 01.07.2014 um 05:20 schrieb Nikunj A Dadhania <address@hidden>:
> 
> Alexander Graf <address@hidden> writes:
> 
>>> On 30.06.2014, at 17:49, Tyrel Datwyler <address@hidden> wrote:
>>> 
>>>> On 06/30/2014 01:35 AM, Nikunj A Dadhania wrote:
>>>> PAPR compliant guest calls this in absence of kdump. This finally
>>>> reaches the guest and can be handled according to the policies set by
>>>> higher level tools(like taking dump) for further analysis by tools like
>>>> crash.
>>>> 
>>>> Linux kernel calls ibm,os-term when extended property of os-term is set.
>>>> This makes sure that a return to the linux kernel is gauranteed.
>>>> 
>>>> CC: Benjamin Herrenschmidt <address@hidden>
>>>> CC: Anton Blanchard <address@hidden>
>>>> CC: Alexander Graf <address@hidden>
>>>> CC: Tyrel Datwyler <address@hidden>
>>>> Signed-off-by: Nikunj A Dadhania <address@hidden>
>>>> 
>>>> ---
>>>> 
>>>> v2: rebase to ppcnext
>>>> v3: Do not stop the VM, and update comments
>>>> v4: update spapr_register_rtas and qapi_event changes
>>>> v5: set ibm,extended-os-term as null encoded property
>>>> ---
>>>> hw/ppc/spapr.c         |  9 +++++++++
>>>> hw/ppc/spapr_rtas.c    | 15 +++++++++++++++
>>>> include/hw/ppc/spapr.h |  1 -
>>>> 3 files changed, 24 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 307c58d..e6c9014 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -520,6 +520,15 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>> 
>>>>    _FDT((fdt_property_cell(fdt, "rtas-error-log-max", 
>>>> RTAS_ERROR_LOG_MAX)));
>>>> 
>>>> +    /*
>>>> +     * According to PAPR, rtas ibm,os-term, does not gaurantee a return
>>>> +     * back to the guest cpu.
>>>> +     *
>>>> +     * While an additional ibm,extended-os-term property indicates that
>>>> +     * rtas call return will always occur. Set this property.
>>>> +     */
>>>> +    _FDT((fdt_property(fdt, "ibm,extended-os-term", NULL, 0)));
>>>> +
>>>>    _FDT((fdt_end_node(fdt)));
>>>> 
>>>>    /* interrupt controller */
>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>> index 9ba1ba6..2ec2a8e 100644
>>>> --- a/hw/ppc/spapr_rtas.c
>>>> +++ b/hw/ppc/spapr_rtas.c
>>>> @@ -277,6 +277,19 @@ 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;
>>>> +
>>>> +    qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, 
>>>> &error_abort);
>>>> +
>>>> +    rtas_st(rets, 0, ret);
>>>> +}
>>>> +
>>>> static struct rtas_call {
>>>>    const char *name;
>>>>    spapr_rtas_fn fn;
>>>> @@ -404,6 +417,8 @@ static void core_rtas_register_types(void)
>>>>    spapr_rtas_register(RTAS_IBM_SET_SYSTEM_PARAMETER,
>>>>                        "ibm,set-system-parameter",
>>>>                        rtas_ibm_set_system_parameter);
>>>> +    spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
>>>> +                        rtas_ibm_os_term);
>>>> }
>>>> 
>>>> type_init(core_rtas_register_types)
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index bbba51a..4e96381 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -382,7 +382,6 @@ int spapr_allocate_irq_block(int num, bool lsi, bool 
>>>> msi);
>>>> #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
>>>> #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
>>>> #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
>>>> -#define RTAS_IBM_EXTENDED_OS_TERM               (RTAS_TOKEN_BASE + 0x20)
>>>> 
>>>> #define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x21)
>>> 
>>> With the removal of RTAS_IBM_EXTENDED_OS_TERM doesn't RTAS_TOKEN_MAX
>>> need to be decremented to (RTAS_TOKEN_BASE + 0x20)?
> 
> You are right.
> 
>> Good catch. Fixed in my queue.
> 
> Thanks, Any reason we dont have that as enum?

We need to maintain number stability for live migration compatibility with 
older machine types. So we have to make 100% sure that removing (or adding) one 
entry does not have any guest visible impact.

This is a lot easier to guarantee with a lot of defines, as you'd have to touch 
multiple lines rather than only a single one to change unrelated entries. Hence 
it makes review easier.


Alex