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

[Aravinda Prasad]

On Wednesday 27 March 2019 08:16 PM, Greg Kurz wrote:
> On Wed, 27 Mar 2019 12:18:31 +0530
> Aravinda Prasad <address@hidden> wrote:
> 
>> On Tuesday 26 March 2019 03:35 PM, 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.
>>>   
>>>> Also, spapr_get_rtas_addr() is called during physical memory corruption
>>>> which is a fatal error.  
>>>
> 
> BTW... IIUC this can also occur if the guest created duplicate SLB entries,
> which isn't fatal from an hypervisor perspective.

yes.. right..

> 
>>> Not that fatal since we care to report it to the guest.  
>>
>> True, but if guest does not provide rtas_addr then I am not getting the
>> point on why terminating the QEMU instance (which actually terminates
>> the guest) is a problem. Am I missing something?
>>
> 
> This isn't terminating QEMU, this is aborting QEMU, which should only
> ever happen in case of a QEMU bug or g_malloc() failure.
> 
> LoPAPR v1.1 7.3.14 Firmware Assisted Non-Maskable Interrupts Option (FWNMI)
> doesn't describe this specific case of course. However, it gives a hint on
> what to do in case of a fatal condition where it isn't possible to notify
> the guest:
> 
>       "...the firmware has no choice but to declare the condition
>       fatal, log the result and execute the partition’s reboot policy."
> 
> ie, either triggering a system_reset or exiting QEMU.

Now I see the point. I will modify it to trigger system_reset instead of
aborting.

> 
>>>   
>>>> 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.  
>>
>> We can do some checks earlier, may be during fwnmi registration to see
>> if rtas entry is missing. Again, the guest can possibly update DT after
>> fwnmi registration.
>>
> 
> Hmm... you're right. Maybe also add these checks to h_update_dt() if the
> guest registered fwnmi ? Or even forbid DT updates since we really expect
> this to occur only once from SLOF between machine resets ? In which case,
> it would be appropriate to keep the asserts in spapr_get_rtas_addr().

As you pointed out above, we can trigger system_reset if we fail to
fetch rtas_addr. I think this approach is simple and straight forward
and will not forbid guests from updating the DT.

> 
>> But I think we cannot switch to old machine check behavior if we cannot
>> fetch the rtas_addr, because according to PAPR the guest would have
>> relinquished 0x200 vector to the firmware when fwnmi is registered. So
>> we cannot expect the guest to handle 0x200 interrupt.
>>
> 
> I wonder how kexec fits in the picture... I mean what if the guest switches
> from an FWNMI-capable kernel to an FWNMI-not-capable one ? There's no machine
> reset in this case...

Ah.. not sure I need to check. So in this case QEMU will never know that
the guest rebooted into an FWNMI-not-capable kexec kernel.

> 
>>
>>>   
>>>>>     
>>>>  
>>>   
>>
> 

-- 
Regards,
Aravinda