[Qemu-devel] [PATCH 0/3] Reduce abuse of rtas_st / rtas_ld

[Qemu-devel] [PATCH 0/3] Reduce abuse of rtas_st / rtas_ld

[David Gibson]

The rtas_ld() and rtas_st() helpers were designed for loading RTAS
arguments and storing RTAS returns which are in a simple, common array
format.

However, a number of RTAS routines - and even non-RTAS routines - have
started using these for accessing other memory buffers, where the
normal qemu memory access routines would be more appropriate.

This series removes some of these abuses of the RTAS accessors.

David Gibson (3):
  spapr: Small fixes to rtas_ibm_get_system_parameter, remove
    rtas_st_buffer
  spapr: Remove rtas_st_buffer_direct()
  spapr: Remove abuse of rtas_ld() in h_client_architecture_support

 hw/ppc/spapr_hcall.c   | 14 +++++++-------
 hw/ppc/spapr_rtas.c    | 47 ++++++++++++++++++++++++++++++++---------------
 include/hw/ppc/spapr.h | 19 -------------------
 3 files changed, 39 insertions(+), 41 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/3] spapr: Small fixes to rtas_ibm_get_system_parameter, remove rtas_st_buffer

[David Gibson]

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;
+
+    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, &param_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,
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/3] spapr: Small fixes to rtas_ibm_get_system_parameter, remove rtas_st_buffer

[Alexey Kardashevskiy]

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, &param_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

Re: [Qemu-devel] [PATCH 1/3] spapr: Small fixes to rtas_ibm_get_system_parameter, remove rtas_st_buffer

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6089 bytes --]

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, &param_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,
> >
> 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 2/3] spapr: Remove rtas_st_buffer_direct()

[David Gibson]

rtas_st_buffer_direct() is a not particularly useful wrapper around
cpu_physical_memory_write().  All the callers are in
rtas_ibm_configure_connector, where it's better handled by local helper.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_rtas.c    | 19 ++++++++++++-------
 include/hw/ppc/spapr.h |  8 --------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index f4fb9ba..940509e 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -504,6 +504,13 @@ out:
 #define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
 #define CC_WA_LEN 4096
 
+static void st_cc_buf(target_ulong addr, target_ulong offset,
+                      void *buf, size_t len)
+{
+    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
+                              buf, MIN(len, CC_WA_LEN - offset));
+}
+
 static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
                                          sPAPRMachineState *spapr,
                                          uint32_t token, uint32_t nargs,
@@ -526,6 +533,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
     }
 
     wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
+    wa_addr = ppc64_phys_to_real(wa_addr);
 
     drc_index = rtas_ld(wa_addr, 0);
     drc = spapr_dr_connector_by_index(drc_index);
@@ -569,8 +577,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
             /* provide the name of the next OF node */
             wa_offset = CC_VAL_DATA_OFFSET;
             rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
-            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
-                                  (uint8_t *)name, strlen(name) + 1);
+            st_cc_buf(wa_addr, wa_offset, (uint8_t *)name, strlen(name) + 1);
             resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
             break;
         case FDT_END_NODE:
@@ -595,8 +602,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
             /* provide the name of the next OF property */
             wa_offset = CC_VAL_DATA_OFFSET;
             rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
-            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
-                                  (uint8_t *)name, strlen(name) + 1);
+            st_cc_buf(wa_addr, wa_offset, (uint8_t *)name, strlen(name) + 1);
 
             /* provide the length and value of the OF property. data gets
              * placed immediately after NULL terminator of the OF property's
@@ -605,9 +611,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
             wa_offset += strlen(name) + 1,
             rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
             rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
-            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
-                                  (uint8_t *)((struct fdt_property *)prop)->data,
-                                  prop_len);
+            st_cc_buf(wa_addr, wa_offset,
+                      (uint8_t *)((struct fdt_property *)prop)->data, prop_len);
             resp = SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY;
             break;
         case FDT_END:
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ec9e7ea..d6f4eb4 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -505,14 +505,6 @@ static inline void rtas_st(target_ulong phys, int n, uint32_t val)
     stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
 }
 
-static inline void rtas_st_buffer_direct(target_ulong phys,
-                                         target_ulong phys_len,
-                                         uint8_t *buffer, uint16_t buffer_len)
-{
-    cpu_physical_memory_write(ppc64_phys_to_real(phys), buffer,
-                              MIN(buffer_len, phys_len));
-}
-
 typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, sPAPRMachineState *sm,
                               uint32_t token,
                               uint32_t nargs, target_ulong args,
-- 
2.5.0

Re: [Qemu-devel] [PATCH 2/3] spapr: Remove rtas_st_buffer_direct()

[Alexey Kardashevskiy]

On 01/16/2016 12:14 PM, David Gibson wrote:
> rtas_st_buffer_direct() is a not particularly useful wrapper around
> cpu_physical_memory_write().  All the callers are in
> rtas_ibm_configure_connector, where it's better handled by local helper.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   hw/ppc/spapr_rtas.c    | 19 ++++++++++++-------
>   include/hw/ppc/spapr.h |  8 --------
>   2 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index f4fb9ba..940509e 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -504,6 +504,13 @@ out:
>   #define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
>   #define CC_WA_LEN 4096
>
> +static void st_cc_buf(target_ulong addr, target_ulong offset,
> +                      void *buf, size_t len)


Can we please call it cc_st_buf() to make it clear that "cc" is a prefix 
and not about functionality?



> +{
> +    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
> +                              buf, MIN(len, CC_WA_LEN - offset));
> +}
> +
>   static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>                                            sPAPRMachineState *spapr,
>                                            uint32_t token, uint32_t nargs,
> @@ -526,6 +533,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>       }
>
>       wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
> +    wa_addr = ppc64_phys_to_real(wa_addr);


Not needed here, st_cc_buf() also does ppc64_phys_to_real().


>
>       drc_index = rtas_ld(wa_addr, 0);
>       drc = spapr_dr_connector_by_index(drc_index);
> @@ -569,8 +577,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>               /* provide the name of the next OF node */
>               wa_offset = CC_VAL_DATA_OFFSET;
>               rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
> -            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> -                                  (uint8_t *)name, strlen(name) + 1);
> +            st_cc_buf(wa_addr, wa_offset, (uint8_t *)name, strlen(name) + 1);


Since you made @buf a void* in st_cc_buf(), you do not need (uint8_t*) here 
and below.



>               resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
>               break;
>           case FDT_END_NODE:
> @@ -595,8 +602,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>               /* provide the name of the next OF property */
>               wa_offset = CC_VAL_DATA_OFFSET;
>               rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
> -            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> -                                  (uint8_t *)name, strlen(name) + 1);
> +            st_cc_buf(wa_addr, wa_offset, (uint8_t *)name, strlen(name) + 1);
>
>               /* provide the length and value of the OF property. data gets
>                * placed immediately after NULL terminator of the OF property's
> @@ -605,9 +611,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>               wa_offset += strlen(name) + 1,
>               rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
>               rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
> -            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> -                                  (uint8_t *)((struct fdt_property *)prop)->data,
> -                                  prop_len);
> +            st_cc_buf(wa_addr, wa_offset,
> +                      (uint8_t *)((struct fdt_property *)prop)->data, prop_len);
>               resp = SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY;
>               break;
>           case FDT_END:
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ec9e7ea..d6f4eb4 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -505,14 +505,6 @@ static inline void rtas_st(target_ulong phys, int n, uint32_t val)
>       stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
>   }
>
> -static inline void rtas_st_buffer_direct(target_ulong phys,
> -                                         target_ulong phys_len,
> -                                         uint8_t *buffer, uint16_t buffer_len)
> -{
> -    cpu_physical_memory_write(ppc64_phys_to_real(phys), buffer,
> -                              MIN(buffer_len, phys_len));
> -}
> -
>   typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, sPAPRMachineState *sm,
>                                 uint32_t token,
>                                 uint32_t nargs, target_ulong args,
>


-- 
Alexey

Re: [Qemu-devel] [PATCH 2/3] spapr: Remove rtas_st_buffer_direct()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5315 bytes --]

On Tue, Jan 19, 2016 at 01:07:23PM +1100, Alexey Kardashevskiy wrote:
> On 01/16/2016 12:14 PM, David Gibson wrote:
> >rtas_st_buffer_direct() is a not particularly useful wrapper around
> >cpu_physical_memory_write().  All the callers are in
> >rtas_ibm_configure_connector, where it's better handled by local helper.
> >
> >Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >---
> >  hw/ppc/spapr_rtas.c    | 19 ++++++++++++-------
> >  include/hw/ppc/spapr.h |  8 --------
> >  2 files changed, 12 insertions(+), 15 deletions(-)
> >
> >diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >index f4fb9ba..940509e 100644
> >--- a/hw/ppc/spapr_rtas.c
> >+++ b/hw/ppc/spapr_rtas.c
> >@@ -504,6 +504,13 @@ out:
> >  #define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
> >  #define CC_WA_LEN 4096
> >
> >+static void st_cc_buf(target_ulong addr, target_ulong offset,
> >+                      void *buf, size_t len)
> 
> 
> Can we please call it cc_st_buf() to make it clear that "cc" is a prefix and
> not about functionality?

Yeah, st_cc_buf() is not a good name.  I've gone with
"configure_connector_st()" which I hope will be better.

> 
> 
> 
> >+{
> >+    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
> >+                              buf, MIN(len, CC_WA_LEN - offset));
> >+}
> >+
> >  static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> >                                           sPAPRMachineState *spapr,
> >                                           uint32_t token, uint32_t nargs,
> >@@ -526,6 +533,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> >      }
> >
> >      wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
> >+    wa_addr = ppc64_phys_to_real(wa_addr);
> 
> 
> Not needed here, st_cc_buf() also does ppc64_phys_to_real().

Good point, corrected.

> >
> >      drc_index = rtas_ld(wa_addr, 0);
> >      drc = spapr_dr_connector_by_index(drc_index);
> >@@ -569,8 +577,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> >              /* provide the name of the next OF node */
> >              wa_offset = CC_VAL_DATA_OFFSET;
> >              rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
> >-            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> >-                                  (uint8_t *)name, strlen(name) + 1);
> >+            st_cc_buf(wa_addr, wa_offset, (uint8_t *)name, strlen(name) + 1);
> 
> 
> Since you made @buf a void* in st_cc_buf(), you do not need (uint8_t*) here
> and below.

Ah, yes good point also.

> 
> 
> 
> >              resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
> >              break;
> >          case FDT_END_NODE:
> >@@ -595,8 +602,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> >              /* provide the name of the next OF property */
> >              wa_offset = CC_VAL_DATA_OFFSET;
> >              rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
> >-            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> >-                                  (uint8_t *)name, strlen(name) + 1);
> >+            st_cc_buf(wa_addr, wa_offset, (uint8_t *)name, strlen(name) + 1);
> >
> >              /* provide the length and value of the OF property. data gets
> >               * placed immediately after NULL terminator of the OF property's
> >@@ -605,9 +611,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> >              wa_offset += strlen(name) + 1,
> >              rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
> >              rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
> >-            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> >-                                  (uint8_t *)((struct fdt_property *)prop)->data,
> >-                                  prop_len);
> >+            st_cc_buf(wa_addr, wa_offset,
> >+                      (uint8_t *)((struct fdt_property *)prop)->data, prop_len);
> >              resp = SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY;
> >              break;
> >          case FDT_END:
> >diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >index ec9e7ea..d6f4eb4 100644
> >--- a/include/hw/ppc/spapr.h
> >+++ b/include/hw/ppc/spapr.h
> >@@ -505,14 +505,6 @@ static inline void rtas_st(target_ulong phys, int n, uint32_t val)
> >      stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
> >  }
> >
> >-static inline void rtas_st_buffer_direct(target_ulong phys,
> >-                                         target_ulong phys_len,
> >-                                         uint8_t *buffer, uint16_t buffer_len)
> >-{
> >-    cpu_physical_memory_write(ppc64_phys_to_real(phys), buffer,
> >-                              MIN(buffer_len, phys_len));
> >-}
> >-
> >  typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, sPAPRMachineState *sm,
> >                                uint32_t token,
> >                                uint32_t nargs, target_ulong args,
> >
> 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 3/3] spapr: Remove abuse of rtas_ld() in h_client_architecture_support

[David Gibson]

h_client_architecture_support() uses rtas_ld() for general purpose memory
access, despite the fact that it's not an RTAS routine at all and rtas_ld
makes things more awkward.

Clean this up by replacing rtas_ld() calls with appropriate ldXX_phys()
calls.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index cebceea..9dbdba9 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -861,7 +861,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
                                                   target_ulong opcode,
                                                   target_ulong *args)
 {
-    target_ulong list = args[0], ov_table;
+    target_ulong list = ppc64_phys_to_real(args[0]);
+    target_ulong ov_table, ov5;
     PowerPCCPUClass *pcc_ = POWERPC_CPU_GET_CLASS(cpu_);
     CPUState *cs;
     bool cpu_match = false, cpu_update = true, memory_update = false;
@@ -875,9 +876,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
     for (counter = 0; counter < 512; ++counter) {
         uint32_t pvr, pvr_mask;
 
-        pvr_mask = rtas_ld(list, 0);
+        pvr_mask = ldl_be_phys(&address_space_memory, list);
         list += 4;
-        pvr = rtas_ld(list, 0);
+        pvr = ldl_be_phys(&address_space_memory, list);
         list += 4;
 
         trace_spapr_cas_pvr_try(pvr);
@@ -948,14 +949,13 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
     /* For the future use: here @ov_table points to the first option vector */
     ov_table = list;
 
-    list = cas_get_option_vector(5, ov_table);
-    if (!list) {
+    ov5 = cas_get_option_vector(5, ov_table);
+    if (!ov5) {
         return H_SUCCESS;
     }
 
     /* @list now points to OV 5 */
-    list += 2;
-    ov5_byte2 = rtas_ld(list, 0) >> 24;
+    ov5_byte2 = ldub_phys(&address_space_memory, ov5 + 2);
     if (ov5_byte2 & OV5_DRCONF_MEMORY) {
         memory_update = true;
     }
-- 
2.5.0

Re: [Qemu-devel] [PATCH 3/3] spapr: Remove abuse of rtas_ld() in h_client_architecture_support

[Alexey Kardashevskiy]

On 01/16/2016 12:14 PM, David Gibson wrote:
> h_client_architecture_support() uses rtas_ld() for general purpose memory
> access, despite the fact that it's not an RTAS routine at all and rtas_ld
> makes things more awkward.
>
> Clean this up by replacing rtas_ld() calls with appropriate ldXX_phys()
> calls.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>   hw/ppc/spapr_hcall.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index cebceea..9dbdba9 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -861,7 +861,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>                                                     target_ulong opcode,
>                                                     target_ulong *args)
>   {
> -    target_ulong list = args[0], ov_table;
> +    target_ulong list = ppc64_phys_to_real(args[0]);
> +    target_ulong ov_table, ov5;
>       PowerPCCPUClass *pcc_ = POWERPC_CPU_GET_CLASS(cpu_);
>       CPUState *cs;
>       bool cpu_match = false, cpu_update = true, memory_update = false;
> @@ -875,9 +876,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>       for (counter = 0; counter < 512; ++counter) {
>           uint32_t pvr, pvr_mask;
>
> -        pvr_mask = rtas_ld(list, 0);
> +        pvr_mask = ldl_be_phys(&address_space_memory, list);
>           list += 4;
> -        pvr = rtas_ld(list, 0);
> +        pvr = ldl_be_phys(&address_space_memory, list);
>           list += 4;
>
>           trace_spapr_cas_pvr_try(pvr);
> @@ -948,14 +949,13 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>       /* For the future use: here @ov_table points to the first option vector */
>       ov_table = list;
>
> -    list = cas_get_option_vector(5, ov_table);
> -    if (!list) {
> +    ov5 = cas_get_option_vector(5, ov_table);
> +    if (!ov5) {
>           return H_SUCCESS;
>       }
>
>       /* @list now points to OV 5 */
> -    list += 2;
> -    ov5_byte2 = rtas_ld(list, 0) >> 24;
> +    ov5_byte2 = ldub_phys(&address_space_memory, ov5 + 2);
>       if (ov5_byte2 & OV5_DRCONF_MEMORY) {
>           memory_update = true;
>       }
>


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/3] Reduce abuse of rtas_st / rtas_ld

[Alexey Kardashevskiy]

On 01/16/2016 12:14 PM, David Gibson wrote:
> The rtas_ld() and rtas_st() helpers were designed for loading RTAS
> arguments and storing RTAS returns which are in a simple, common array
> format.
>
> However, a number of RTAS routines - and even non-RTAS routines - have
> started using these for accessing other memory buffers, where the
> normal qemu memory access routines would be more appropriate.
>
> This series removes some of these abuses of the RTAS accessors.

imho simple renaming rtas_st to stl_be_phys_real (and so on for other 
rtas_xx) would make more sense as rtas_st&co do not have to do anything 
with RTAS itself, it is all about realmode guest memory accessб RTAS just 
happened to be the first client of it.

btw in st_cc_buf(), what does "cc" stand for?



-- 
Alexey

Re: [Qemu-devel] [PATCH 0/3] Reduce abuse of rtas_st / rtas_ld

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

On Mon, Jan 18, 2016 at 10:51:51AM +1100, Alexey Kardashevskiy wrote:
> On 01/16/2016 12:14 PM, David Gibson wrote:
> >The rtas_ld() and rtas_st() helpers were designed for loading RTAS
> >arguments and storing RTAS returns which are in a simple, common array
> >format.
> >
> >However, a number of RTAS routines - and even non-RTAS routines - have
> >started using these for accessing other memory buffers, where the
> >normal qemu memory access routines would be more appropriate.
> >
> >This series removes some of these abuses of the RTAS accessors.
> 
> imho simple renaming rtas_st to stl_be_phys_real (and so on for other
> rtas_xx) would make more sense as rtas_st&co do not have to do anything with
> RTAS itself, it is all about realmode guest memory accessб RTAS just
> happened to be the first client of it.

Well, no.  They were designed, specifically, to be a concise way to
load RTAS arguments, and store RTAS returns.  Nothing more.

To match other ldl* routines they would need to change to not take the
'n' argument, which would make using them more awkward for the exact
use case they're intended for.

I did consider adding ldXX_real() routines for all the memory access
cases these have been abused for, but they're few enough that it seems
simpler just to open code them in terms of the base memory access
routines.
 
> btw in st_cc_buf(), what does "cc" stand for?

"configure connector" - it's a helper just for that routine.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]