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

[Alexey Kardashevskiy <aik@au1.ibm.com>]

On 09/02/2014 05:07 PM, Aravinda Prasad wrote:
> 
> 
> On Tuesday 02 September 2014 12:04 PM, Alexey Kardashevskiy wrote:
>> On 09/02/2014 03:56 PM, Aravinda Prasad wrote:
>>>
>>>
>>> On Tuesday 02 September 2014 11:19 AM, Alexey Kardashevskiy wrote:
>>>> On 09/02/2014 03:25 PM, Aravinda Prasad wrote:
>>>>>
>>>>>
>>>>> On Tuesday 02 September 2014 09:39 AM, Alexey Kardashevskiy wrote:
>>>>>> On 09/01/2014 09:23 PM, Aravinda Prasad wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Monday 01 September 2014 01:16 PM, Alexey Kardashevskiy wrote:
>>>>>>>> On 08/25/2014 11:45 PM, 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                  |   13 ++++++++++---
>>>>>>>>>  hw/ppc/spapr_rtas.c             |    4 ++--
>>>>>>>>>  include/hw/ppc/spapr.h          |    2 +-
>>>>>>>>>  pc-bios/spapr-rtas/spapr-rtas.S |   12 ++++++++++++
>>>>>>>>>  4 files changed, 25 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>>>> index d01978f..1120988 100644
>>>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>>>> @@ -85,6 +85,12 @@
>>>>>>>>>  
>>>>>>>>>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
>>>>>>>>>  
>>>>>>>>> +/*
>>>>>>>>> + * The rtas-entry-offset should match the value specified in
>>>>>>>>> + * spapr-rtas.S
>>>>>>>>> + */
>>>>>>>>> +#define RTAS_ENTRY_OFFSET       0x1000
>>>>>>>>> +
>>>>>>>>>  typedef struct sPAPRMachineState sPAPRMachineState;
>>>>>>>>>  
>>>>>>>>>  #define TYPE_SPAPR_MACHINE      "spapr-machine"
>>>>>>>>> @@ -670,7 +676,8 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
>>>>>>>>>  static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>>>>>>>>>                                 hwaddr fdt_addr,
>>>>>>>>>                                 hwaddr rtas_addr,
>>>>>>>>> -                               hwaddr rtas_size)
>>>>>>>>> +                               hwaddr rtas_size,
>>>>>>>>> +                               hwaddr rtas_entry)
>>>>>>>>>  {
>>>>>>>>>      int ret, i;
>>>>>>>>>      size_t cb = 0;
>>>>>>>>> @@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>>>>>>>>>      }
>>>>>>>>>  
>>>>>>>>>      /* RTAS */
>>>>>>>>> -    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
>>>>>>>>> +    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size, rtas_entry);
>>>>>>>>>      if (ret < 0) {
>>>>>>>>>          fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
>>>>>>>>>      }
>>>>>>>>> @@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
>>>>>>>>>  
>>>>>>>>>      /* Load the fdt */
>>>>>>>>>      spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
>>>>>>>>> -                       spapr->rtas_size);
>>>>>>>>> +                       spapr->rtas_size, spapr->rtas_addr + RTAS_ENTRY_OFFSET);
>>>>>>>>>  
>>>>>>>>>      /* Set up the entry state */
>>>>>>>>>      first_ppc_cpu = POWERPC_CPU(first_cpu);
>>>>>>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>>>>>> index 9ba1ba6..02ddbf9 100644
>>>>>>>>> --- a/hw/ppc/spapr_rtas.c
>>>>>>>>> +++ b/hw/ppc/spapr_rtas.c
>>>>>>>>> @@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>>  int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>>>>>>>> -                                 hwaddr rtas_size)
>>>>>>>>> +                                 hwaddr rtas_size, hwaddr rtas_entry)
>>>>>>>>>  {
>>>>>>>>>      int ret;
>>>>>>>>>      int i;
>>>>>>>>> @@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>>>>>>>>      }
>>>>>>>>>  
>>>>>>>>>      ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
>>>>>>>>> -                                rtas_addr);
>>>>>>>>> +                                rtas_entry);
>>>>>>>>>      if (ret < 0) {
>>>>>>>>>          fprintf(stderr, "Couldn't add linux,rtas-entry property: %s\n",
>>>>>>>>>                  fdt_strerror(ret));
>>>>>>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>>>>>>> index bbba51a..dedfa67 100644
>>>>>>>>> --- a/include/hw/ppc/spapr.h
>>>>>>>>> +++ b/include/hw/ppc/spapr.h
>>>>>>>>> @@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>>>>>>>                               uint32_t token, uint32_t nargs, target_ulong args,
>>>>>>>>>                               uint32_t nret, target_ulong rets);
>>>>>>>>>  int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>>>>>>>> -                                 hwaddr rtas_size);
>>>>>>>>> +                                 hwaddr rtas_size, hwaddr rtas_entry);
>>>>>>>>>  
>>>>>>>>>  #define SPAPR_TCE_PAGE_SHIFT   12
>>>>>>>>>  #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)
>>>>>>>>> diff --git a/pc-bios/spapr-rtas/spapr-rtas.S b/pc-bios/spapr-rtas/spapr-rtas.S
>>>>>>>>> index 903bec2..8c9b17e 100644
>>>>>>>>> --- a/pc-bios/spapr-rtas/spapr-rtas.S
>>>>>>>>> +++ b/pc-bios/spapr-rtas/spapr-rtas.S
>>>>>>>>> @@ -30,6 +30,18 @@
>>>>>>>>>  
>>>>>>>>>  .globl	_start
>>>>>>>>>  _start:
>>>>>>>>> +	/*
>>>>>>>>> +	 * Reserve space for error log in RTAS blob.
>>>>>>>>> +	 *
>>>>>>>>> +	 * Either we can reserve initial bytes for error log followed by
>>>>>>>>> +	 * rtas-entry or space can be reserved after rtas-entry. I prefer
>>>>>>>>> +	 * former, as we already have rtas-base and rtas-entry (currently
>>>>>>>>> +	 * both pointing to rtas-base) defined in qemu and we can update
>>>>>>>>> +	 * rtas-entry to point to an offset from rtas-base. This avoids
>>>>>>>>> +	 * unnecessary definition of rtas-error-offset while keeping
>>>>>>>>> +	 * rtas-entry redundant.
>>>>>>>>> +	 */
>>>>>>>>> +	. = 0x1000
>>>>>>>>
>>>>>>>>
>>>>>>>> Why not this (and not changing spapr-rtas.S)?
>>>>>>>>
>>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>>> @@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
>>>>>>>>                         spapr->rtas_size);
>>>>>>>>
>>>>>>>>      /* Copy RTAS over */
>>>>>>>> -    cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
>>>>>>>> +    cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
>>>>>>>> +                              spapr->rtas_blob,
>>>>>>>>                                spapr->rtas_size);
>>>>>>>
>>>>>>> This is possible, however requires suitable adjustment to make sure
>>>>>>> spapr->rtas_addr has enough space allocated.
>>>>>>
>>>>>>
>>>>>> How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has enough
>>>>>> space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry
>>>>>> properties.
>>>>>
>>>>> QEMU adds spapr-rtas.bin as a rom, with rom->addr set to
>>>>> spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current
>>>>> spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data
>>>>> (malloc-ed region).
>>>>>
>>>>> I think, access to spapr->rtas_addr is mapped to this rom. Hence it is
>>>>> necessary to have rtas_addr and rtas_size consistent with the Rom
>>>>> struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are
>>>>> trying to access an invalid offset in rom region.
>>>>
>>>>
>>>> What is that "rom" struct you are referring to? In upstream QEMU, I can
>>>> only see:
>>>>
>>>> ppc_spapr_init():
>>>> [...]
>>>> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")
>>>
>>> In ppc_spapr_init() just after qemu_find_file() we have:
>>>
>>> spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr, ...);
>>
>>
>> What tree has this? Mine does not.
>>
>> ka1:~/p/qemu$ grep load_image_targphys hw/ppc/spapr.c
>>             initrd_size = load_image_targphys(initrd_filename, initrd_base,
>>     fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
>> ka1:~/p/qemu$
>>
> 
> I am using git://git.qemu.org/qemu.git, master branch.

Ah. I see. My bad, I recommended the wrong tree then, sorry about it...

This one is the current queue of ppc patches and I believe this patchset
will go through it:
git://github.com/agraf/qemu.git  branch: ppc-next

And it does not have load_image_targphys() for rtas.


> 
> I have 88e89a57f9 as the latest commit and I see:
> 
> $ grep load_image_targphys hw/ppc/spapr.c
>     spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr,
>          initrd_size = load_image_targphys(initrd_filename, initrd_base,
>     fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
> 
> 
> This was introduced in commit a3467baa.
> 
> Regards,
> Aravinda
> 
>>
>>
>>>
>>> load_image_targphys() -> rom_add_file_fixed() -> rom_add_file(), where
>>> Rom is initialized.
>>>
>>>> spapr->rtas_size = get_image_size(filename);
>>>> spapr->rtas_blob = g_malloc(spapr->rtas_size);
>>>> if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
>>>> [...]
>>>>
>>>> and then
>>>>
>>>> ppc_spapr_reset():
>>>> [...]
>>>> spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE
>>>> [...]
>>>> cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
>>>>                           spapr->rtas_size);
>>>> [...]
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>  	mr	4,3
>>>>>>>>>  	lis	3,KVMPPC_H_RTAS@h
>>>>>>>>>  	ori	3,3,KVMPPC_H_RTAS@l
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
> 


-- 
Alexey Kardashevskiy
IBM OzLabs, LTC Team

e-mail: aik@au1.ibm.com
notes: Alexey Kardashevskiy/Australia/IBM