From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42797) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xly2Y-0002u2-W1 for qemu-devel@nongnu.org; Wed, 05 Nov 2014 05:42:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xly2P-0002JV-TU for qemu-devel@nongnu.org; Wed, 05 Nov 2014 05:42:14 -0500 Received: from e33.co.us.ibm.com ([32.97.110.151]:46531) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xly2P-0002JM-LQ for qemu-devel@nongnu.org; Wed, 05 Nov 2014 05:42:05 -0500 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 5 Nov 2014 03:42:05 -0700 Message-ID: <5459FEF0.1000403@linux.vnet.ibm.com> Date: Wed, 05 Nov 2014 16:11:52 +0530 From: Aravinda Prasad MIME-Version: 1.0 References: <20141105071019.26196.93729.stgit@aravindap> <20141105071235.26196.28398.stgit@aravindap> <5459DBA6.5090503@suse.de> <5459E3DE.9030309@linux.vnet.ibm.com> <5459E731.5060906@suse.de> <5459E8CB.6030502@suse.de> In-Reply-To: <5459E8CB.6030502@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/4] target-ppc: Extend rtas-blob List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: benh@au1.ibm.com, aik@au1.ibm.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, paulus@samba.org On Wednesday 05 November 2014 02:37 PM, Alexander Graf wrote: > > > On 05.11.14 10:00, Alexander Graf wrote: >> >> >> On 05.11.14 09:46, Aravinda Prasad wrote: >>> >>> >>> On Wednesday 05 November 2014 01:41 PM, Alexander Graf wrote: >>>> >>>> >>>> On 05.11.14 08:12, 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 | 7 +++++++ >>>>> include/hw/ppc/spapr.h | 5 +++++ >>>>> 2 files changed, 12 insertions(+) >>>>> >>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>>>> index 30de25d..38e26af 100644 >>>>> --- a/hw/ppc/spapr.c >>>>> +++ b/hw/ppc/spapr.c >>>>> @@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine) >>>>> >>>>> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin"); >>>>> spapr->rtas_size = get_image_size(filename); >>>>> + >>>>> + /* >>>>> + * Resize blob to accommodate error log. The layout of the rtas >>>>> + * blob is defined in include/hw/ppc/spapr.h >>>>> + */ >>>>> + spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size); >>>> >>>> How big is the error log? You could just extend the RTAS blob to include >>>> space for it if it's not too big. >>> >>> Error log is around 10 bytes and requires additional 24 bytes to store >>> saved sro/srr1. >>> >>> Hmm.. yes it can be included in RTAS blob itself. >>> >>> >>>> >>>>> + >>>>> spapr->rtas_blob = g_malloc(spapr->rtas_size); >>>>> if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) { >>>>> hw_error("qemu: could not load LPAR rtas '%s'\n", filename); >>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >>>>> index 749daf4..d08fcc2 100644 >>>>> --- a/include/hw/ppc/spapr.h >>>>> +++ b/include/hw/ppc/spapr.h >>>>> @@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname, >>>>> int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname, >>>>> sPAPRTCETable *tcet); >>>>> >>>>> +/* RTAS Blob layout in memory */ >>>>> +#define RTAS_ENTRY_OFFSET 0 >>>>> +#define RTAS_TRAMPOLINE_OFFSET 0x200 >>>>> +#define RTAS_ERRLOG_OFFSET 0x800 >>>> >>>> I thought we agreed that these offsets should've been defined by the >>>> blob itself? >>>> >>> >>> I think I got it wrong. >>> >>> I will include these indexes at the entry of RTAS blob. With that we >>> will have something like this: >>> >>> RTAS_ENTRY_OFFSET = *(spapr->rtas_addr) >>> RTAS_TRAMPOLINE_OFFSET = *(spapr->rtas_addr+8) >>> RTAS_ERRLOG_OFFSET = *(spapr->rtas_addr+16) >>> >>> I will fix this. >> >> Cool :). Just store the offsets inside of a helper struct that you for >> example store in the spapr struct, then we don't need to read volatile >> guest memory for the offsets. > > I just reread what I wrote and figured it's not exactly verbose. What I > meant was that you read them on load into a struct. Then when working > with the offsets, you only use the cached ones from the struct. > > That way when the guest for whatever reason modifies the RTAS blob in > memory, we would still use the old offsets and ensure that we don't end > up overwriting memory that we never intended to overwrite ;). sure > > > Alex > -- Regards, Aravinda