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

[David Gibson]

On Mon, Mar 16, 2020 at 06:52:54PM +0100, Greg Kurz wrote:
> 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/

In the meantime, this is still correct.  Because we rebuild the device
tree at CAS time, the qemu supplied value will be the one the guest
sees in the end.  We obviously want that qemu update to avoid
confusion, but we don't need it for things to work.

-- 
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