Re: [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery

[Greg Kurz]

On Mon, 16 Mar 2020 23:05:00 +0530
Mahesh J Salgaonkar <address@hidden> wrote:

> On 2020-03-17 00:26:12 Tue, Nicholas Piggin wrote:
> > PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor
> > delivers all system reset and machine check exceptions to the registered
> > addresses.
> > 
> > System Resets are delivered with registers set to the architected state,
> > and with no interlock.
> > 
> > Signed-off-by: Nicholas Piggin <address@hidden>
> > ---
> >  hw/ppc/spapr.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 44 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 25221d843c..78e649f47d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, 
> > void *fdt)
> >      _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
> >                       maxdomains, sizeof(maxdomains)));
> > 
> > -    _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE));
> > +    /*
> > +     * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log,
> > +     * and 16 bytes per CPU for system reset error log plus an extra 8 
> > bytes.
> > +     *
> > +     * The system reset requirements are driven by existing Linux and 
> > PowerVM
> > +     * implementation which (contrary to PAPR) saves r3 in the error log
> > +     * structure like machine check, so Linux expects to find the saved r3
> > +     * value at the address in r3 upon FWNMI-enabled sreset interrupt (and
> > +     * does not look at the error value).
> > +     *
> > +     * System reset interrupts are not subject to interlock like machine
> > +     * check, so this memory area could be corrupted if the sreset is
> > +     * interrupted by a machine check (or vice versa) if it was shared. To
> > +     * prevent this, system reset uses per-CPU areas for the sreset save
> > +     * area. A system reset that interrupts a system reset handler could
> > +     * still overwrite this area, but Linux doesn't try to recover in that
> > +     * case anyway.
> > +     *
> > +     * The extra 8 bytes is required because Linux's FWNMI error log check
> > +     * is off-by-one.
> > +     */
> > +    _FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX +
> > +                     ms->smp.max_cpus * sizeof(uint64_t)*2 + 
> > sizeof(uint64_t)));
> 
> Currently the rtas region is only of size 2048 (i.e RTAS_ERROR_LOG_MAX).
> Do we need SLOF change to increase rtas area as well ? Otherwise QEMU
> may corrupt guest memory area OR Am I wrong ?
> 

A change is pending for SLOF to use the "rtas-size" property
provided by QEMU:

https://patchwork.ozlabs.org/patch/1255264/

> Thanks,
> -Mahesh/
> 
> >      _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
> >                            RTAS_ERROR_LOG_MAX));
> >      _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",
> > @@ -3399,8 +3421,28 @@ static void spapr_machine_finalizefn(Object *obj)
> > 
> >  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
> >  {
> > +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +
> >      cpu_synchronize_state(cs);
> > -    ppc_cpu_do_system_reset(cs, -1);
> > +    /* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 
> > */
> > +    if (spapr->fwnmi_system_reset_addr != -1) {
> > +        uint64_t rtas_addr, addr;
> > +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +        CPUPPCState *env = &cpu->env;
> > +
> > +        /* get rtas addr from fdt */
> > +        rtas_addr = spapr_get_rtas_addr();
> > +        if (!rtas_addr) {
> > +            qemu_system_guest_panicked(NULL);
> > +            return;
> > +        }
> > +
> > +        addr = rtas_addr + RTAS_ERROR_LOG_MAX + cs->cpu_index * 
> > sizeof(uint64_t)*2;
> > +        stq_be_phys(&address_space_memory, addr, env->gpr[3]);
> > +        stq_be_phys(&address_space_memory, addr + sizeof(uint64_t), 0);
> > +        env->gpr[3] = addr;
> > +    }
> > +    ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr);
> >  }
> > 
> >  static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> > -- 
> > 2.23.0
> > 
> > 
>