Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE

[Aravinda Prasad]

On Tuesday 14 May 2019 10:10 AM, David Gibson wrote:
> On Tue, May 14, 2019 at 09:56:41AM +0530, Aravinda Prasad wrote:
>>
>>
>> On Tuesday 14 May 2019 05:38 AM, David Gibson wrote:
>>> On Mon, May 13, 2019 at 01:30:53PM +0200, Greg Kurz wrote:
>>>> On Mon, 22 Apr 2019 12:33:26 +0530
>>>> Aravinda Prasad <address@hidden> wrote:
>>>>
>>>>> Upon a machine check exception (MCE) in a guest address space,
>>>>> KVM causes a guest exit to enable QEMU to build and pass the
>>>>> error to the guest in the PAPR defined rtas error log format.
>>>>>
>>>>> This patch builds the rtas error log, copies it to the rtas_addr
>>>>> and then invokes the guest registered machine check handler. The
>>>>> handler in the guest takes suitable action(s) depending on the type
>>>>> and criticality of the error. For example, if an error is
>>>>> unrecoverable memory corruption in an application inside the
>>>>> guest, then the guest kernel sends a SIGBUS to the application.
>>>>> For recoverable errors, the guest performs recovery actions and
>>>>> logs the error.
>>>>>
>>>>> Signed-off-by: Aravinda Prasad <address@hidden>
>>>>> ---
>>>>>  hw/ppc/spapr.c         |    4 +
>>>>>  hw/ppc/spapr_events.c  |  245 
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/hw/ppc/spapr.h |    4 +
>>>>>  3 files changed, 253 insertions(+)
>>>>>
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index 2779efe..ffd1715 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -2918,6 +2918,10 @@ static void spapr_machine_init(MachineState 
>>>>> *machine)
>>>>>          error_report("Could not get size of LPAR rtas '%s'", filename);
>>>>>          exit(1);
>>>>>      }
>>>>> +
>>>>> +    /* Resize blob to accommodate error log. */
>>>>> +    spapr->rtas_size = spapr_get_rtas_size(spapr->rtas_size);
>>>>> +
>>>>
>>>> This is the only user for spapr_get_rtas_size(), which is trivial.
>>>> I suggest you simply open-code it here.
>>>
>>> I agree.
>>
>> Sure.
>>
>>>
>>>> But also, spapr->rtas_size is a guest visible thing, "rtas-size" prop in 
>>>> the
>>>> DT. Since existing machine types don't do that, I guess we should only use
>>>> the new size if cap-fwnmi-mce=on for the sake of compatibility.
>>>
>>> Yes, that's a good idea.  Changing this is very unlikely to break a
>>> guest, but it's easy to be safe here so let's do it.
>>
>> I did it like that because the rtas_blob is allocated based on rtas_size
>> in spapr_machine_init(). During spapr_machine_init() it is not know if
>> the guest calls "ibm, nmi-register". So if we want to use the new size
>> only when cap_fwnmi=on, then we have to realloc the blob in "ibm,
>> nmi-register".
> 
> What?  Just always allocate the necessary space in
> spapr_machine_init() if cap_fwnmi=on, it'll be wasted if
> ibm,nmi-register is never called, but it's not that much space so we
> don't really care.

Yes, not that much space, and ibm,nmi-register is called when the Linux
kernel boots. I guess, even though other OSes might not call
ibm,nmi-register, they do not constitute significant QEMU on Power users.

So I think, I will keep the code as is.

> 

-- 
Regards,
Aravinda