From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52365) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOjdy-0001f2-PA for qemu-devel@nongnu.org; Tue, 02 Sep 2014 04:40:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOjdj-0001ih-VO for qemu-devel@nongnu.org; Tue, 02 Sep 2014 04:40:50 -0400 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:48264) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOjdj-0001ha-C7 for qemu-devel@nongnu.org; Tue, 02 Sep 2014 04:40:35 -0400 Received: from /spool/local by e23smtp07.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 2 Sep 2014 18:40:24 +1000 Message-ID: <54058274.7070006@au1.ibm.com> Date: Tue, 02 Sep 2014 18:40:20 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <20140825134353.2361.52046.stgit@aravindap> <20140825134516.2361.32987.stgit@aravindap> <5404245C.7000004@au1.ibm.com> <5404571F.7080700@linux.vnet.ibm.com> <540542FC.6020304@au1.ibm.com> <540554D5.5050107@linux.vnet.ibm.com> <54055A4F.10305@au1.ibm.com> <54055C13.9080405@linux.vnet.ibm.com> <540564EE.8050202@ozlabs.ru> <54056CAA.2020802@linux.vnet.ibm.com> In-Reply-To: <54056CAA.2020802@linux.vnet.ibm.com> Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aravinda Prasad , Alexey Kardashevskiy Cc: paulus@samba.org, benh@au1.ibm.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 >>>>>>>>> --- >>>>>>>>> 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. > > 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 >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>> >> >> > -- Alexey Kardashevskiy IBM OzLabs, LTC Team e-mail: aik@au1.ibm.com notes: Alexey Kardashevskiy/Australia/IBM