| Message ID | 20170607070732.23312-1-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
----- Original Message ----- > From: "David Gibson" <david@gibson.dropbear.id.au> > To: mdroth@linux.vnet.ibm.com, thuth@redhat.com, lvivier@redhat.com > Cc: pbonzini@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, "David Gibson" <david@gibson.dropbear.id.au> > Sent: Wednesday, June 7, 2017 9:07:32 AM > Subject: [PATCH] pseries: Correct panic behaviour for pseries machine type > > The pseries machine type doesn't usually use the 'pvpanic' device as such, > because it has a firmware/hypervisor facility with roughly the same > purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the > guest has crashed. > > Our implementation of this call was sending a GUEST_PANICKED qmp event; > however, it was not doing the other usual panic actions, making its > behaviour different from pvpanic for no good reason. > > To correct this, we should call qemu_system_guest_panicked() rather than > directly sending the panic event. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_rtas.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 707c4d4..94a2799 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, > target_ulong args, > uint32_t nret, target_ulong rets) > { > - target_ulong ret = 0; > + qemu_system_guest_panicked(NULL); > > - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, > - &error_abort); > - > - rtas_st(rets, 0, ret); > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } It's possible to "cont" a panicked guest, so I think you should keep the rtas_st. Paolo
On 07.06.2017 09:07, David Gibson wrote: > The pseries machine type doesn't usually use the 'pvpanic' device as such, > because it has a firmware/hypervisor facility with roughly the same > purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the > guest has crashed. > > Our implementation of this call was sending a GUEST_PANICKED qmp event; > however, it was not doing the other usual panic actions, making its > behaviour different from pvpanic for no good reason. > > To correct this, we should call qemu_system_guest_panicked() rather than > directly sending the panic event. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_rtas.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 707c4d4..94a2799 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, > target_ulong args, > uint32_t nret, target_ulong rets) > { > - target_ulong ret = 0; > + qemu_system_guest_panicked(NULL); > > - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, > - &error_abort); > - > - rtas_st(rets, 0, ret); > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > > static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr, > If I get that qemu_system_guest_panicked() function right, it will stop the VM, won't it? That contradicts the LoPAPR spec that says that the RTAS call returns if the "ibm,extended-os-term" property is available in the device tree. And we currently present this property in the device tree. So either the guest should not be stopped here, or we've got to remove the property from the device tree again. Thomas
On Wed, Jun 07, 2017 at 03:24:56AM -0400, Paolo Bonzini wrote: > > > ----- Original Message ----- > > From: "David Gibson" <david@gibson.dropbear.id.au> > > To: mdroth@linux.vnet.ibm.com, thuth@redhat.com, lvivier@redhat.com > > Cc: pbonzini@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, "David Gibson" <david@gibson.dropbear.id.au> > > Sent: Wednesday, June 7, 2017 9:07:32 AM > > Subject: [PATCH] pseries: Correct panic behaviour for pseries machine type > > > > The pseries machine type doesn't usually use the 'pvpanic' device as such, > > because it has a firmware/hypervisor facility with roughly the same > > purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the > > guest has crashed. > > > > Our implementation of this call was sending a GUEST_PANICKED qmp event; > > however, it was not doing the other usual panic actions, making its > > behaviour different from pvpanic for no good reason. > > > > To correct this, we should call qemu_system_guest_panicked() rather than > > directly sending the panic event. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr_rtas.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > > index 707c4d4..94a2799 100644 > > --- a/hw/ppc/spapr_rtas.c > > +++ b/hw/ppc/spapr_rtas.c > > @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, > > target_ulong args, > > uint32_t nret, target_ulong rets) > > { > > - target_ulong ret = 0; > > + qemu_system_guest_panicked(NULL); > > > > - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, > > - &error_abort); > > - > > - rtas_st(rets, 0, ret); > > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > > } > > It's possible to "cont" a panicked guest, so I think you should keep > the rtas_st. I did keep the rtas_st(), I just changed it to using a constant instead of an always-0 variable.
On Wed, Jun 07, 2017 at 09:33:21AM +0200, Thomas Huth wrote: > On 07.06.2017 09:07, David Gibson wrote: > > The pseries machine type doesn't usually use the 'pvpanic' device as such, > > because it has a firmware/hypervisor facility with roughly the same > > purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the > > guest has crashed. > > > > Our implementation of this call was sending a GUEST_PANICKED qmp event; > > however, it was not doing the other usual panic actions, making its > > behaviour different from pvpanic for no good reason. > > > > To correct this, we should call qemu_system_guest_panicked() rather than > > directly sending the panic event. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr_rtas.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > > index 707c4d4..94a2799 100644 > > --- a/hw/ppc/spapr_rtas.c > > +++ b/hw/ppc/spapr_rtas.c > > @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, > > target_ulong args, > > uint32_t nret, target_ulong rets) > > { > > - target_ulong ret = 0; > > + qemu_system_guest_panicked(NULL); > > > > - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, > > - &error_abort); > > - > > - rtas_st(rets, 0, ret); > > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > > } > > > > static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr, > > > > If I get that qemu_system_guest_panicked() function right, it will stop > the VM, won't it? That contradicts the LoPAPR spec that says that the > RTAS call returns if the "ibm,extended-os-term" property is available in > the device tree. And we currently present this property in the device > tree. So either the guest should not be stopped here, or we've got to > remove the property from the device tree again. Hrm. However in the case of this "extended behaviour" it's not clear what we should do next. I'm include to think that the preference set by the qemu user / management layer should take precedence over PAPR.
On 07/06/2017 09:33, Thomas Huth wrote: > On 07.06.2017 09:07, David Gibson wrote: >> The pseries machine type doesn't usually use the 'pvpanic' device as such, >> because it has a firmware/hypervisor facility with roughly the same >> purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the >> guest has crashed. >> >> Our implementation of this call was sending a GUEST_PANICKED qmp event; >> however, it was not doing the other usual panic actions, making its >> behaviour different from pvpanic for no good reason. >> >> To correct this, we should call qemu_system_guest_panicked() rather than >> directly sending the panic event. >> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> hw/ppc/spapr_rtas.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >> index 707c4d4..94a2799 100644 >> --- a/hw/ppc/spapr_rtas.c >> +++ b/hw/ppc/spapr_rtas.c >> @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, >> target_ulong args, >> uint32_t nret, target_ulong rets) >> { >> - target_ulong ret = 0; >> + qemu_system_guest_panicked(NULL); >> >> - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, >> - &error_abort); >> - >> - rtas_st(rets, 0, ret); >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> } >> >> static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr, >> > > If I get that qemu_system_guest_panicked() function right, it will stop > the VM, won't it? That contradicts the LoPAPR spec that says that the > RTAS call returns if the "ibm,extended-os-term" property is available in > the device tree. It does return... but only after the user starts the guest again with "cont". Paolo
On 07.06.2017 16:34, Paolo Bonzini wrote: > > > On 07/06/2017 09:33, Thomas Huth wrote: >> On 07.06.2017 09:07, David Gibson wrote: >>> The pseries machine type doesn't usually use the 'pvpanic' device as such, >>> because it has a firmware/hypervisor facility with roughly the same >>> purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the >>> guest has crashed. >>> >>> Our implementation of this call was sending a GUEST_PANICKED qmp event; >>> however, it was not doing the other usual panic actions, making its >>> behaviour different from pvpanic for no good reason. >>> >>> To correct this, we should call qemu_system_guest_panicked() rather than >>> directly sending the panic event. >>> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >>> --- >>> hw/ppc/spapr_rtas.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >>> index 707c4d4..94a2799 100644 >>> --- a/hw/ppc/spapr_rtas.c >>> +++ b/hw/ppc/spapr_rtas.c >>> @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, >>> target_ulong args, >>> uint32_t nret, target_ulong rets) >>> { >>> - target_ulong ret = 0; >>> + qemu_system_guest_panicked(NULL); >>> >>> - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, >>> - &error_abort); >>> - >>> - rtas_st(rets, 0, ret); >>> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >>> } >>> >>> static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr, >>> >> >> If I get that qemu_system_guest_panicked() function right, it will stop >> the VM, won't it? That contradicts the LoPAPR spec that says that the >> RTAS call returns if the "ibm,extended-os-term" property is available in >> the device tree. > > It does return... but only after the user starts the guest again with > "cont". OK, I guess that's enough to say that the "ibm,extended-os-term" property can stay ... so I think the patch is fine as it is right now. Thomas
On Wed, Jun 07, 2017 at 07:10:55PM +0200, Thomas Huth wrote: > On 07.06.2017 16:34, Paolo Bonzini wrote: > > > > > > On 07/06/2017 09:33, Thomas Huth wrote: > >> On 07.06.2017 09:07, David Gibson wrote: > >>> The pseries machine type doesn't usually use the 'pvpanic' device as such, > >>> because it has a firmware/hypervisor facility with roughly the same > >>> purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the > >>> guest has crashed. > >>> > >>> Our implementation of this call was sending a GUEST_PANICKED qmp event; > >>> however, it was not doing the other usual panic actions, making its > >>> behaviour different from pvpanic for no good reason. > >>> > >>> To correct this, we should call qemu_system_guest_panicked() rather than > >>> directly sending the panic event. > >>> > >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >>> --- > >>> hw/ppc/spapr_rtas.c | 7 ++----- > >>> 1 file changed, 2 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > >>> index 707c4d4..94a2799 100644 > >>> --- a/hw/ppc/spapr_rtas.c > >>> +++ b/hw/ppc/spapr_rtas.c > >>> @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, > >>> target_ulong args, > >>> uint32_t nret, target_ulong rets) > >>> { > >>> - target_ulong ret = 0; > >>> + qemu_system_guest_panicked(NULL); > >>> > >>> - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, > >>> - &error_abort); > >>> - > >>> - rtas_st(rets, 0, ret); > >>> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > >>> } > >>> > >>> static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr, > >>> > >> > >> If I get that qemu_system_guest_panicked() function right, it will stop > >> the VM, won't it? That contradicts the LoPAPR spec that says that the > >> RTAS call returns if the "ibm,extended-os-term" property is available in > >> the device tree. > > > > It does return... but only after the user starts the guest again with > > "cont". > > OK, I guess that's enough to say that the "ibm,extended-os-term" > property can stay ... so I think the patch is fine as it is right now. So.. can I have an R-b?
On 08.06.2017 02:18, David Gibson wrote: > On Wed, Jun 07, 2017 at 07:10:55PM +0200, Thomas Huth wrote: >> On 07.06.2017 16:34, Paolo Bonzini wrote: >>> >>> >>> On 07/06/2017 09:33, Thomas Huth wrote: >>>> On 07.06.2017 09:07, David Gibson wrote: >>>>> The pseries machine type doesn't usually use the 'pvpanic' device as such, >>>>> because it has a firmware/hypervisor facility with roughly the same >>>>> purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the >>>>> guest has crashed. >>>>> >>>>> Our implementation of this call was sending a GUEST_PANICKED qmp event; >>>>> however, it was not doing the other usual panic actions, making its >>>>> behaviour different from pvpanic for no good reason. >>>>> >>>>> To correct this, we should call qemu_system_guest_panicked() rather than >>>>> directly sending the panic event. >>>>> >>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >>>>> --- >>>>> hw/ppc/spapr_rtas.c | 7 ++----- >>>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >>>>> index 707c4d4..94a2799 100644 >>>>> --- a/hw/ppc/spapr_rtas.c >>>>> +++ b/hw/ppc/spapr_rtas.c >>>>> @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, >>>>> target_ulong args, >>>>> uint32_t nret, target_ulong rets) >>>>> { >>>>> - target_ulong ret = 0; >>>>> + qemu_system_guest_panicked(NULL); >>>>> >>>>> - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, >>>>> - &error_abort); >>>>> - >>>>> - rtas_st(rets, 0, ret); >>>>> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >>>>> } >>>>> >>>>> static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr, >>>>> >>>> >>>> If I get that qemu_system_guest_panicked() function right, it will stop >>>> the VM, won't it? That contradicts the LoPAPR spec that says that the >>>> RTAS call returns if the "ibm,extended-os-term" property is available in >>>> the device tree. >>> >>> It does return... but only after the user starts the guest again with >>> "cont". >> >> OK, I guess that's enough to say that the "ibm,extended-os-term" >> property can stay ... so I think the patch is fine as it is right now. > > So.. can I have an R-b? Reviewed-by: Thomas Huth <thuth@redhat.com>
On Thu, Jun 08, 2017 at 06:33:57AM +0200, Thomas Huth wrote: > On 08.06.2017 02:18, David Gibson wrote: > > On Wed, Jun 07, 2017 at 07:10:55PM +0200, Thomas Huth wrote: > >> On 07.06.2017 16:34, Paolo Bonzini wrote: > >>> > >>> > >>> On 07/06/2017 09:33, Thomas Huth wrote: > >>>> On 07.06.2017 09:07, David Gibson wrote: > >>>>> The pseries machine type doesn't usually use the 'pvpanic' device as such, > >>>>> because it has a firmware/hypervisor facility with roughly the same > >>>>> purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the > >>>>> guest has crashed. > >>>>> > >>>>> Our implementation of this call was sending a GUEST_PANICKED qmp event; > >>>>> however, it was not doing the other usual panic actions, making its > >>>>> behaviour different from pvpanic for no good reason. > >>>>> > >>>>> To correct this, we should call qemu_system_guest_panicked() rather than > >>>>> directly sending the panic event. > >>>>> > >>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >>>>> --- > >>>>> hw/ppc/spapr_rtas.c | 7 ++----- > >>>>> 1 file changed, 2 insertions(+), 5 deletions(-) > >>>>> > >>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > >>>>> index 707c4d4..94a2799 100644 > >>>>> --- a/hw/ppc/spapr_rtas.c > >>>>> +++ b/hw/ppc/spapr_rtas.c > >>>>> @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, > >>>>> target_ulong args, > >>>>> uint32_t nret, target_ulong rets) > >>>>> { > >>>>> - target_ulong ret = 0; > >>>>> + qemu_system_guest_panicked(NULL); > >>>>> > >>>>> - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, > >>>>> - &error_abort); > >>>>> - > >>>>> - rtas_st(rets, 0, ret); > >>>>> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > >>>>> } > >>>>> > >>>>> static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr, > >>>>> > >>>> > >>>> If I get that qemu_system_guest_panicked() function right, it will stop > >>>> the VM, won't it? That contradicts the LoPAPR spec that says that the > >>>> RTAS call returns if the "ibm,extended-os-term" property is available in > >>>> the device tree. > >>> > >>> It does return... but only after the user starts the guest again with > >>> "cont". > >> > >> OK, I guess that's enough to say that the "ibm,extended-os-term" > >> property can stay ... so I think the patch is fine as it is right now. > > > > So.. can I have an R-b? > > Reviewed-by: Thomas Huth <thuth@redhat.com> Thanks.
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 707c4d4..94a2799 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu, target_ulong args, uint32_t nret, target_ulong rets) { - target_ulong ret = 0; + qemu_system_guest_panicked(NULL); - qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL, - &error_abort); - - rtas_st(rets, 0, ret); + rtas_st(rets, 0, RTAS_OUT_SUCCESS); } static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr,
The pseries machine type doesn't usually use the 'pvpanic' device as such, because it has a firmware/hypervisor facility with roughly the same purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the guest has crashed. Our implementation of this call was sending a GUEST_PANICKED qmp event; however, it was not doing the other usual panic actions, making its behaviour different from pvpanic for no good reason. To correct this, we should call qemu_system_guest_panicked() rather than directly sending the panic event. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_rtas.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)