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

[Aravinda Prasad]

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?

Also, spapr_get_rtas_addr() is called during physical memory corruption
which is a fatal error. So, if we cannot fetch rtas_addr (required to
build and pass the error info to the guest) then I think we should abort.

> 

-- 
Regards,
Aravinda