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

[David Gibson]

On Tue, Mar 26, 2019 at 11:05:44AM +0100, Greg Kurz wrote:
> On Tue, 26 Mar 2019 14:45:57 +0530
> Aravinda Prasad <address@hidden> wrote:
> 
> > On Tuesday 26 March 2019 02:00 PM, Greg Kurz wrote:
> > > 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.  
> > 
> > Is it not the QEMU that builds the DT and provides it to the guest?
> > 
> 
> Yes, but then the guest can push a new DT with the KVMPPC_H_UPDATE_DT hcall.
> We only do some minimalist sanity checks in h_update_dt(). I don't think
> we want to abort QEMU because the guest sent a DT where "linux,rtas-base"
> is missing for example.

Yeah, that's a good point.

> > Also, spapr_get_rtas_addr() is called during physical memory corruption
> > which is a fatal error.
> 
> Not that fatal since we care to report it to the guest.
> 
> > So, if we cannot fetch rtas_addr (required to
> > build and pass the error info to the guest) then I think we should abort.
> > 
> 
> Maybe we cannot do anything better at this point, but then we should
> do some earlier checks and switch to the old machine check behaviour
> if what we need is missing from the updated DT for example.
> 
> > >   
> > 
> 

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