| Message ID | 1452906886-806-2-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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 <david@gibson.dropbear.id.au> > --- > 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, >
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 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 <david@gibson.dropbear.id.au> > >--- > > 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. 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. > > > >+ > >+ 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); > > ? 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. > > 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... 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. > > > > >+ 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, > > > >
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; + + if (length < 2) { + rtas_st(rets, 0, -9999); /* Parameter error */ + 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; + 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 */ } 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,
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 <david@gibson.dropbear.id.au> --- hw/ppc/spapr_rtas.c | 28 ++++++++++++++++++++-------- include/hw/ppc/spapr.h | 11 ----------- 2 files changed, 20 insertions(+), 19 deletions(-)