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. 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