Re: [Qemu-ppc] [PATCH v7 4/6] target/ppc: Build rtas error log

[Greg Kurz]

On Tue, 26 Mar 2019 10:32:35 +1100
David Gibson <address@hidden> wrote:

> On Mon, Mar 25, 2019 at 01:56:50PM +0530, Aravinda Prasad wrote:
> > 
> > 
> > On Monday 25 March 2019 12:00 PM, David Gibson wrote:  
> > > On Fri, Mar 22, 2019 at 12:04:07PM +0530, Aravinda Prasad wrote:  
> > >> This patch builds the rtas error log, copies it to the
> > >> rtas_addr and then invokes the guest registered machine
> > >> check handler.  
> > > 
> > > This commit message needs more context.  When is this occurring, why
> > > do we need this?
> > > 
> > > [I can answer those questions now, but whether I - or anyone else -
> > >  will be able to looking back at this commit from years in the future
> > >  is a different question]  
> > 
> > will add more info.  
> 
> Thanks.
> 
> [snip]
> > >> +static uint64_t spapr_get_rtas_addr(void)
> > >> +{
> > >> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > >> +    int rtas_node;
> > >> +    const struct fdt_property *rtas_addr_prop;
> > >> +    void *fdt = spapr->fdt_blob;
> > >> +    uint32_t rtas_addr;
> > >> +
> > >> +    /* fetch rtas addr from fdt */
> > >> +    rtas_node = fdt_path_offset(fdt, "/rtas");
> > >> +    g_assert(rtas_node >= 0);
> > >> +
> > >> +    rtas_addr_prop = fdt_get_property(fdt, rtas_node, 
> > >> "linux,rtas-base", NULL);
> > >> +    g_assert(rtas_addr_prop);
> > >> +
> > >> +    rtas_addr = fdt32_to_cpu(*(uint32_t *)rtas_addr_prop->data);
> > >> +    return (uint64_t)rtas_addr;  
> > > 
> > > It seems a bit roundabout to pull the rtas address out of the device
> > > tree, since it was us that put it in there in the first place.  
> > 
> > Slof can change the rtas address. So we need to get the updated rtas
> > address.  
> 
> Ah, ok.
> 

Yeah, and knowing that the DT is guest originated makes me a bit
nervous when I see the g_assert()... a misbehaving guest could
possibly abort QEMU. Either there should be some sanity checks
performed earlier or an non-fatal error path should be added in
this function IMHO.

pgpUxfJ6LWcgc.pgp
Description: OpenPGP digital signature