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 <aravi...@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$
>
> 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
