Message ID | 1402574971-26672-2-git-send-email-nikunj@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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)
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
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 >
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
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
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
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
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
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
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
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
> 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 >
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
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
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)
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(+)