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.
>
>
>>
>>>
>>>
>>>
>>>
>>>> mr 4,3
>>>> lis 3,KVMPPC_H_RTAS@h
>>>> ori 3,3,KVMPPC_H_RTAS@l
>>>>
>>>
>>>
>>
>
>
--
Regards,
Aravinda
