Re: [Qemu-devel] [Qemu-ppc] [PATCH v10 6/6] ppc: spapr: Handle "ibm, nmi-register" and "ibm, nmi-interlock" RTAS calls

[David Gibson]

On Wed, Jun 26, 2019 at 10:43:33AM +0530, Aravinda Prasad wrote:
> 
> 
> On Tuesday 25 June 2019 12:30 PM, Greg Kurz wrote:
> > On Tue, 25 Jun 2019 11:46:06 +0530
> > Aravinda Prasad <address@hidden> wrote:
> > 
> >> On Monday 24 June 2019 07:59 PM, Greg Kurz wrote:
> >>> On Wed, 12 Jun 2019 14:51:38 +0530
> >>> Aravinda Prasad <address@hidden> wrote:
> >>>   
> >>>> This patch adds support in QEMU to handle "ibm,nmi-register"
> >>>> and "ibm,nmi-interlock" RTAS calls and sets the default
> >>>> value of SPAPR_CAP_FWNMI_MCE to SPAPR_CAP_ON for machine
> >>>> type 4.0.
> >>>>  
> >>>
> >>> Next machine type is 4.1.  
> >>
> >> ok.
> >>
> >>>   
> >>>> The machine check notification address is saved when the
> >>>> OS issues "ibm,nmi-register" RTAS call.
> >>>>
> >>>> This patch also handles the case when multiple processors
> >>>> experience machine check at or about the same time by
> >>>> handling "ibm,nmi-interlock" call. In such cases, as per
> >>>> PAPR, subsequent processors serialize waiting for the first
> >>>> processor to issue the "ibm,nmi-interlock" call. The second
> >>>> processor that also received a machine check error waits
> >>>> till the first processor is done reading the error log.
> >>>> The first processor issues "ibm,nmi-interlock" call
> >>>> when the error log is consumed.
> >>>>
> >>>> Signed-off-by: Aravinda Prasad <address@hidden>
> >>>> ---
> >>>>  hw/ppc/spapr.c         |    6 ++++-
> >>>>  hw/ppc/spapr_rtas.c    |   63 
> >>>> ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/hw/ppc/spapr.h |    5 +++-
> >>>>  3 files changed, 72 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index 3d6d139..213d493 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -2946,6 +2946,9 @@ static void spapr_machine_init(MachineState 
> >>>> *machine)
> >>>>          /* Create the error string for live migration blocker */
> >>>>          error_setg(&spapr->fwnmi_migration_blocker,
> >>>>                  "Live migration not supported during machine check 
> >>>> handling");
> >>>> +
> >>>> +        /* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */
> >>>> +        spapr_fwnmi_register();  
> >>>
> >>> IIRC this was supposed to depend on SPAPR_CAP_FWNMI_MCE being ON.  
> >>
> >> Yes this is inside SPAPR_CAP_FWNMI_MCE check:
> >>
> >> if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_ON) {
> >>     /*
> >>      * Ensure that the rtas image size is less than RTAS_ERROR_LOG_OFFSET
> >>      * or else the rtas image will be overwritten with the rtas error log
> >>      * when a machine check exception is encountered.
> >>      */
> >>     g_assert(spapr->rtas_size < RTAS_ERROR_LOG_OFFSET);
> >>
> >>     /* Resize rtas blob to accommodate error log */
> >>     spapr->rtas_size = RTAS_ERROR_LOG_MAX;
> >>
> >>     /* Create the error string for live migration blocker */
> >>     error_setg(&spapr->fwnmi_migration_blocker,
> >>             "Live migration not supported during machine check handling");
> >>
> >>     /* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */
> >>     spapr_fwnmi_register();
> >> }
> >>
> > 
> > Oops my bad... sorry for the noise.
> > 
> >>
> >>>   
> >>>>      }
> >>>>  
> >>>>      spapr->rtas_blob = g_malloc(spapr->rtas_size);
> >>>> @@ -4408,7 +4411,7 @@ static void spapr_machine_class_init(ObjectClass 
> >>>> *oc, void *data)
> >>>>      smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> >>>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> >>>>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> >>>> -    smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
> >>>> +    smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_ON;
> >>>>      spapr_caps_add_properties(smc, &error_abort);
> >>>>      smc->irq = &spapr_irq_dual;
> >>>>      smc->dr_phb_enabled = true;
> >>>> @@ -4512,6 +4515,7 @@ static void 
> >>>> spapr_machine_3_1_class_options(MachineClass *mc)
> >>>>      smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> >>>>      smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
> >>>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_OFF;
> >>>> +    smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;  
> >>>
> >>> This should have been put into spapr_machine_4_0_class_options().  
> >>
> >> ok. I will change it.
> >>
> >>>
> >>> But unless you manage to get this merged before soft-freeze (2019-07-02),
> >>> I'm afraid this will be a 4.2 feature.  
> >>
> >> If there are no other comments, can this be merged to 4.1? I will send a
> >> revised version with the above changes.
> >>
> > 
> > This is David's call.
> 
> David, can you let me know if this can be merged to 4.1 with the above
> minor changes?

Nope, sorry.  I've been away, but also this still needs more polish.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature