From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37902) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLLbQ-0007ve-RF for qemu-devel@nongnu.org; Mon, 18 Jan 2016 21:01:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLLbN-0003WR-Kt for qemu-devel@nongnu.org; Mon, 18 Jan 2016 21:01:00 -0500 Received: from mail-pf0-x241.google.com ([2607:f8b0:400e:c00::241]:36772) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLLbM-0003TV-VV for qemu-devel@nongnu.org; Mon, 18 Jan 2016 21:00:57 -0500 Received: by mail-pf0-x241.google.com with SMTP id n128so12411182pfn.3 for ; Mon, 18 Jan 2016 18:00:56 -0800 (PST) References: <1452906886-806-1-git-send-email-david@gibson.dropbear.id.au> <1452906886-806-2-git-send-email-david@gibson.dropbear.id.au> From: Alexey Kardashevskiy Message-ID: <569D98D2.1060901@ozlabs.ru> Date: Tue, 19 Jan 2016 13:00:50 +1100 MIME-Version: 1.0 In-Reply-To: <1452906886-806-2-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] spapr: Small fixes to rtas_ibm_get_system_parameter, remove rtas_st_buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, mdroth@linux.vnet.ibm.com, agraf@suse.de On 01/16/2016 12:14 PM, David Gibson wrote: > rtas_st_buffer() appears in spapr.h as though it were a widely used helper, > but in fact it is only used for saving data in a format used by > rtas_ibm_get_system_parameter(). We can fold it into that caller just as > simply. > > While we're there fix a couple of small defects in > rtas_ibm_get_system_parameter: > - For the string value SPLPAR_CHARACTERISTICS, it wasn't including the > terminating \0 in the length which it should according to LoPAPR > 7.3.16.1 > - It now checks that the supplied buffer has at least enough space for > the length of the returned data, and returns an error if it does not. > > Signed-off-by: David Gibson > --- > hw/ppc/spapr_rtas.c | 28 ++++++++++++++++++++-------- > include/hw/ppc/spapr.h | 11 ----------- > 2 files changed, 20 insertions(+), 19 deletions(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 34b12a3..f4fb9ba 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -235,9 +235,15 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > uint32_t nret, target_ulong rets) > { > target_ulong parameter = rtas_ld(args, 0); > - target_ulong buffer = rtas_ld(args, 1); > + target_ulong buffer = ppc64_phys_to_real(rtas_ld(args, 1)); > target_ulong length = rtas_ld(args, 2); > - target_ulong ret = RTAS_OUT_SUCCESS; > + void *val; > + size_t vallen; Having such temporary variables (val/vallen) tells me that a helper is a better solution as this assumes that one word and one buffer will be ever enough for all cases but there is already "7.3.16.17 Processor Module Information" coming (even though I have not heard from Sukadev for quite some time) which is more complicated that this. May be rtas_st_buffer() is not the best name and it does not have to be global so I'd propose making it static and calling it syspar_st(). > + > + if (length < 2) { > + rtas_st(rets, 0, -9999); /* Parameter error */ #define RTAS_OUT_SYSPAR_PARAM_ERROR -9999 and then rtas_st(rets, 0, RTAS_OUT_SYSPAR_PARAM_ERROR); ? In all other places ("rtas_st(\S\+, 0") a macro is used (except just one case in spapr_pci.c which needs a fix). > + return; > + } > > switch (parameter) { > case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: { > @@ -249,24 +255,30 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > current_machine->ram_size / M_BYTE, > smp_cpus, > max_cpus); > - rtas_st_buffer(buffer, length, (uint8_t *)param_val, strlen(param_val)); > + val = param_val; > + vallen = strlen(param_val) + 1; > g_free(param_val); > break; > } > case RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE: { > - uint8_t param_val = DIAGNOSTICS_RUN_MODE_DISABLED; > + uint8_t diagnostics_run_mode = DIAGNOSTICS_RUN_MODE_DISABLED; > > - rtas_st_buffer(buffer, length, ¶m_val, sizeof(param_val)); > + val = &diagnostics_run_mode; I know that any reasonable compiler will keep @diagnostics_run_mode on stack till return but it is still not clean... > + vallen = sizeof(diagnostics_run_mode); > break; > } > case RTAS_SYSPARM_UUID: > - rtas_st_buffer(buffer, length, qemu_uuid, (qemu_uuid_set ? 16 : 0)); > + val = qemu_uuid; > + vallen = qemu_uuid_set ? 16 : 0; > break; > default: > - ret = RTAS_OUT_NOT_SUPPORTED; > + rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); > + return; > } > > - rtas_st(rets, 0, ret); > + stw_be_phys(&address_space_memory, buffer, vallen); > + cpu_physical_memory_write(buffer + 2, val, MIN(vallen, length - 2)); > + rtas_st(rets, 0, 0); /* Success */ rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > > static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 53af76a..ec9e7ea 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -513,17 +513,6 @@ static inline void rtas_st_buffer_direct(target_ulong phys, > MIN(buffer_len, phys_len)); > } > > -static inline void rtas_st_buffer(target_ulong phys, target_ulong phys_len, > - uint8_t *buffer, uint16_t buffer_len) > -{ > - if (phys_len < 2) { > - return; > - } > - stw_be_phys(&address_space_memory, > - ppc64_phys_to_real(phys), buffer_len); > - rtas_st_buffer_direct(phys + 2, phys_len - 2, buffer, buffer_len); > -} > - > typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, sPAPRMachineState *sm, > uint32_t token, > uint32_t nargs, target_ulong args, > -- Alexey