On Tuesday 02 September 2014 02:10 PM, Alexey Kardashevskiy wrote:
> 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 <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$
>>>
>>
>> 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.
Hmm.. ok. Let me have a look at it.
So my v2 should be based on git://github.com/agraf/qemu.git branch:
ppc-next right?
>
>
>>
>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
--
Regards,
Aravinda
