From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41065) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLNvC-0006qQ-Fz for qemu-devel@nongnu.org; Mon, 18 Jan 2016 23:29:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLNvA-0002K2-VB for qemu-devel@nongnu.org; Mon, 18 Jan 2016 23:29:34 -0500 Date: Tue, 19 Jan 2016 14:56:29 +1100 From: David Gibson Message-ID: <20160119035629.GY9301@voom.fritz.box> References: <1452906886-806-1-git-send-email-david@gibson.dropbear.id.au> <1452906886-806-2-git-send-email-david@gibson.dropbear.id.au> <569D98D2.1060901@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="bIBSTgLWjO5xMb20" Content-Disposition: inline In-Reply-To: <569D98D2.1060901@ozlabs.ru> 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: Alexey Kardashevskiy Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, mdroth@linux.vnet.ibm.com, agraf@suse.de --bIBSTgLWjO5xMb20 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jan 19, 2016 at 01:00:50PM +1100, Alexey Kardashevskiy wrote: > On 01/16/2016 12:14 PM, David Gibson wrote: > >rtas_st_buffer() appears in spapr.h as though it were a widely used help= er, > >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 no= t. > > > >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(PowerPCCP= U *cpu, > > uint32_t nret, target_ulong = rets) > > { > > target_ulong parameter =3D rtas_ld(args, 0); > >- target_ulong buffer =3D rtas_ld(args, 1); > >+ target_ulong buffer =3D ppc64_phys_to_real(rtas_ld(args, 1)); > > target_ulong length =3D rtas_ld(args, 2); > >- target_ulong ret =3D RTAS_OUT_SUCCESS; > >+ void *val; > >+ size_t vallen; >=20 > 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. Actually latest discussions with Sukadev and the RH licensing people suggest we won't need that after all. But still, point taken. > 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(). Ok, that works for me. >=20 >=20 > >+ > >+ if (length < 2) { > >+ rtas_st(rets, 0, -9999); /* Parameter error */ >=20 >=20 > #define RTAS_OUT_SYSPAR_PARAM_ERROR -9999 > and then > rtas_st(rets, 0, RTAS_OUT_SYSPAR_PARAM_ERROR); >=20 > ? Hm, ok. I'm not entirely sold on the need, since most RTAS return values are specific to individual RTAS calls, and they're not given symbolic names by PAPR. Still, why not. >=20 > In all other places ("rtas_st(\S\+, 0") a macro is used (except just one > case in spapr_pci.c which needs a fix). >=20 >=20 > >+ return; > >+ } > > > > switch (parameter) { > > case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: { > >@@ -249,24 +255,30 @@ static void rtas_ibm_get_system_parameter(PowerPCC= PU *cpu, > > current_machine->ram_size / = M_BYTE, > > smp_cpus, > > max_cpus); > >- rtas_st_buffer(buffer, length, (uint8_t *)param_val, strlen(par= am_val)); > >+ val =3D param_val; > >+ vallen =3D strlen(param_val) + 1; > > g_free(param_val); > > break; > > } > > case RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE: { > >- uint8_t param_val =3D DIAGNOSTICS_RUN_MODE_DISABLED; > >+ uint8_t diagnostics_run_mode =3D DIAGNOSTICS_RUN_MODE_DISABLED; > > > >- rtas_st_buffer(buffer, length, ¶m_val, sizeof(param_val)); > >+ val =3D &diagnostics_run_mode; >=20 >=20 > I know that any reasonable compiler will keep @diagnostics_run_mode on st= ack > till return but it is still not clean... For a minute I didn't see what you were getting at, then realized that diagnostics_run_mode is in a different scope from the final store. Yeah, that's not ok. >=20 >=20 >=20 > >+ vallen =3D sizeof(diagnostics_run_mode); > > break; > > } > > case RTAS_SYSPARM_UUID: > >- rtas_st_buffer(buffer, length, qemu_uuid, (qemu_uuid_set ? 16 := 0)); > >+ val =3D qemu_uuid; > >+ vallen =3D qemu_uuid_set ? 16 : 0; > > break; > > default: > >- ret =3D 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 */ >=20 >=20 > rtas_st(rets, 0, RTAS_OUT_SUCCESS); >=20 >=20 >=20 > > } > > > > 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_ulo= ng 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, > > >=20 >=20 --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --bIBSTgLWjO5xMb20 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWnbPsAAoJEGw4ysog2bOS53wQANtTB6gni2IHEfha9eIwIuVy b1BbcqjdxuMUsm8u93nOkQ/2ZqwUBg2WRSeJlzUy7kMMIHOlcCQVZm2JWO/g+oyp 4tDpiQKMvfgjl14jk4I+X2loH/9ZC9NS/k9T1E0TK8YjMhqAB2aLmW+0SBVFLIFk zf07j/DrN05H74CN1bosmPyasQhh7nqLqtSh9AK0+anTq0d09vlbq9+XevCGsdp3 OniJWCFT3RGN/MGWyGf1u/mUGV5XyRTgBpZJTJUGgwJdaPkBXAm3lQmpyyKG1Uyl 3K+3VXQ+6vzcYH+lPyUPlJvwyQkCUiRiwiTLeqUrruq+W7FySEVez1/xrFg8ylkZ x4kwuHwa3TA6eUzuU6TMWV7QWt+B3hEorYI8IiKcisaYyMLVurXjfWrxVDpd6Duh P3hKxoq32+7pbhzgAFTT2y5s8RFqCKlY7A8+o4YGkO4JbnOZ0VjSW9z0Pec8mFyP /bOLCb5Sf1euwgssBShCK3D66tOj1lWg/oyzfV7WyIWJ2ddfoUO1+/8Vl8yfbxno pyws7RQQ5WLSRiIdDL4gk6kkybjgXrQ1MV4h6rZ5NZY9YLguabyWpAXsH42fD3Sp MMpSh8jCcJ0FOjtac9MU+Y068RCFZTCuLNuzbtkSaZsg1AkjuGuK9aWKtgaLD88s XSULFE1GPSallZehD0a/ =19Cg -----END PGP SIGNATURE----- --bIBSTgLWjO5xMb20--