On 09/02/2014 07:30 PM, Aravinda Prasad wrote: > > > 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?
Yes since this branch is closer to upstream and affects what you do. > >> >> >>> >>> 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); >>>>>> [...] -- Alexey