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

[Greg Kurz]

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.

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

> >   
>