Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/4] target-ppc: Extend rtas-blob

[Alexander Graf <agraf@suse.de>]

On 05.11.14 10:00, Alexander Graf wrote:
> 
> 
> On 05.11.14 09:46, Aravinda Prasad wrote:
>>
>>
>> On Wednesday 05 November 2014 01:41 PM, Alexander Graf wrote:
>>>
>>>
>>> On 05.11.14 08:12, Aravinda Prasad wrote:
>>>> Extend rtas-blob to accommodate error log. Error log
>>>> structure is saved in rtas space upon a machine check
>>>> exception.
>>>>
>>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/ppc/spapr.c         |    7 +++++++
>>>>  include/hw/ppc/spapr.h |    5 +++++
>>>>  2 files changed, 12 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 30de25d..38e26af 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
>>>>  
>>>>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
>>>>      spapr->rtas_size = get_image_size(filename);
>>>> +
>>>> +    /*
>>>> +     * Resize blob to accommodate error log. The layout of the rtas
>>>> +     * blob is defined in include/hw/ppc/spapr.h
>>>> +     */
>>>> +    spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size);
>>>
>>> How big is the error log? You could just extend the RTAS blob to include
>>> space for it if it's not too big.
>>
>> Error log is around 10 bytes and requires additional 24 bytes to store
>> saved sro/srr1.
>>
>> Hmm.. yes it can be included in RTAS blob itself.
>>
>>
>>>
>>>> +
>>>>      spapr->rtas_blob = g_malloc(spapr->rtas_size);
>>>>      if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
>>>>          hw_error("qemu: could not load LPAR rtas '%s'\n", filename);
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index 749daf4..d08fcc2 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
>>>>  int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
>>>>                        sPAPRTCETable *tcet);
>>>>  
>>>> +/* RTAS Blob layout in memory */
>>>> +#define RTAS_ENTRY_OFFSET        0
>>>> +#define RTAS_TRAMPOLINE_OFFSET   0x200
>>>> +#define RTAS_ERRLOG_OFFSET       0x800
>>>
>>> I thought we agreed that these offsets should've been defined by the
>>> blob itself?
>>>
>>
>> I think I got it wrong.
>>
>> I will include these indexes at the entry of RTAS blob. With that we
>> will have something like this:
>>
>> RTAS_ENTRY_OFFSET  =      *(spapr->rtas_addr)
>> RTAS_TRAMPOLINE_OFFSET =  *(spapr->rtas_addr+8)
>> RTAS_ERRLOG_OFFSET =      *(spapr->rtas_addr+16)
>>
>> I will fix this.
> 
> Cool :). Just store the offsets inside of a helper struct that you for
> example store in the spapr struct, then we don't need to read volatile
> guest memory for the offsets.

I just reread what I wrote and figured it's not exactly verbose. What I
meant was that you read them on load into a struct. Then when working
with the offsets, you only use the cached ones from the struct.

That way when the guest for whatever reason modifies the RTAS blob in
memory, we would still use the old offsets and ensure that we don't end
up overwriting memory that we never intended to overwrite ;).


Alex