From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46970) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOgbW-0005Bl-CB for qemu-devel@nongnu.org; Tue, 02 Sep 2014 01:26:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOgbH-0001kd-Ft for qemu-devel@nongnu.org; Tue, 02 Sep 2014 01:26:06 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:46583) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOgbH-0001kR-57 for qemu-devel@nongnu.org; Tue, 02 Sep 2014 01:25:51 -0400 Received: from /spool/local by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 1 Sep 2014 23:25:49 -0600 Message-ID: <540554D5.5050107@linux.vnet.ibm.com> Date: Tue, 02 Sep 2014 10:55:41 +0530 From: Aravinda Prasad 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> In-Reply-To: <540542FC.6020304@au1.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: Alexey Kardashevskiy Cc: benh@au1.ibm.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, paulus@samba.org 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. > > >> >>> >>> >>> >>> >>>> mr 4,3 >>>> lis 3,KVMPPC_H_RTAS@h >>>> ori 3,3,KVMPPC_H_RTAS@l >>>> >>> >>> >> > > -- Regards, Aravinda