[PATCH-for-8.0 0/4] ppc: Clean up few headers to make them target agnostic

[PATCH-for-8.0 0/4] ppc: Clean up few headers to make them target agnostic

[Philippe Mathieu-Daudé]

Few changes in hw/ & target/ to reduce the target specificity
of some sPAPR headers.

Philippe Mathieu-Daudé (4):
  target/ppc/kvm: Add missing "cpu.h" and "exec/hwaddr.h"
  hw/ppc/vof: Do not include the full "cpu.h"
  hw/ppc/spapr: Reduce "vof.h" inclusion
  hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()

 hw/ppc/spapr.c              | 1 +
 hw/ppc/spapr_ovec.c         | 3 ++-
 include/hw/ppc/spapr.h      | 3 ++-
 include/hw/ppc/spapr_ovec.h | 4 ++--
 include/hw/ppc/vof.h        | 2 +-
 target/ppc/kvm_ppc.h        | 3 +++
 6 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.38.1

[PATCH-for-8.0 1/4] target/ppc/kvm: Add missing "cpu.h" and "exec/hwaddr.h"

[Philippe Mathieu-Daudé]

kvm_ppc.h is missing various declarations from "cpu.h":

  target/ppc/kvm_ppc.h:128:40: error: unknown type name 'CPUPPCState'; did you mean 'CPUState'?
  static inline int kvmppc_get_hypercall(CPUPPCState *env,
                                         ^~~~~~~~~~~
                                         CPUState
  include/qemu/typedefs.h:45:25: note: 'CPUState' declared here
  typedef struct CPUState CPUState;
                          ^
  target/ppc/kvm_ppc.h:134:40: error: unknown type name 'PowerPCCPU'
  static inline int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level)
                                         ^
  target/ppc/kvm_ppc.h:285:38: error: unknown type name 'hwaddr'
                                       hwaddr ptex, int n)
                                       ^
  target/ppc/kvm_ppc.h:220:15: error: unknown type name 'target_ulong'
  static inline target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
                ^
  target/ppc/kvm_ppc.h:286:38: error: unknown type name 'ppc_hash_pte64_t'
  static inline void kvmppc_read_hptes(ppc_hash_pte64_t *hptes,
                                       ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/kvm_ppc.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index ee9325bf9a..5fd9753953 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -9,6 +9,9 @@
 #ifndef KVM_PPC_H
 #define KVM_PPC_H
 
+#include "exec/hwaddr.h"
+#include "cpu.h"
+
 #define TYPE_HOST_POWERPC_CPU POWERPC_CPU_TYPE_NAME("host")
 
 #ifdef CONFIG_KVM
-- 
2.38.1

[PATCH-for-8.0 2/4] hw/ppc/vof: Do not include the full "cpu.h"

[Philippe Mathieu-Daudé]

"vof.h" doesn't need the full "cpu.h" to get the target_ulong
definition, including "exec/cpu-defs.h" is enough.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/ppc/vof.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/ppc/vof.h b/include/hw/ppc/vof.h
index f8c0effcaf..d3f293da8b 100644
--- a/include/hw/ppc/vof.h
+++ b/include/hw/ppc/vof.h
@@ -9,7 +9,7 @@
 #include "qom/object.h"
 #include "exec/address-spaces.h"
 #include "exec/memory.h"
-#include "cpu.h"
+#include "exec/cpu-defs.h"
 
 typedef struct Vof {
     uint64_t top_addr; /* copied from rma_size */
-- 
2.38.1

[PATCH-for-8.0 3/4] hw/ppc/spapr: Reduce "vof.h" inclusion

[Philippe Mathieu-Daudé]

Currently objects including "hw/ppc/spapr.h" are forced to be
target specific due to the inclusion of "vof.h" in "spapr.h".

"spapr.h" only uses a Vof pointer, so doesn't require the structure
declaration. The only place where Vof structure is accessed is in
spapr.c, so include "vof.h" there, and forward declare the structure
in "spapr.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/spapr.c         | 1 +
 include/hw/ppc/spapr.h | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 66b414d2e9..f38a851ee3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -62,6 +62,7 @@
 #include "hw/ppc/fdt.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
+#include "hw/ppc/vof.h"
 #include "hw/qdev-properties.h"
 #include "hw/pci-host/spapr.h"
 #include "hw/pci/msi.h"
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 04a95669ab..5c8aabd444 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -12,7 +12,6 @@
 #include "hw/ppc/spapr_xive.h"  /* For SpaprXive */
 #include "hw/ppc/xics.h"        /* For ICSState */
 #include "hw/ppc/spapr_tpm_proxy.h"
-#include "hw/ppc/vof.h"
 
 struct SpaprVioBus;
 struct SpaprPhbState;
@@ -22,6 +21,8 @@ typedef struct SpaprEventLogEntry SpaprEventLogEntry;
 typedef struct SpaprEventSource SpaprEventSource;
 typedef struct SpaprPendingHpt SpaprPendingHpt;
 
+typedef struct Vof Vof;
+
 #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
 #define SPAPR_ENTRY_POINT       0x100
 
-- 
2.38.1

[PATCH-for-8.0 4/4] hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()

[Philippe Mathieu-Daudé]

spapr_ovec.c is a device, but it uses target_ulong which is
target specific. The hwaddr type (declared in "exec/hwaddr.h")
better fits hardware addresses.

Change spapr_ovec_parse_vector() to take a hwaddr argument,
allowing the removal of "cpu.h" in a device header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/spapr_ovec.c         | 3 ++-
 include/hw/ppc/spapr_ovec.h | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
index b2567caa5c..a18a751b57 100644
--- a/hw/ppc/spapr_ovec.c
+++ b/hw/ppc/spapr_ovec.c
@@ -19,6 +19,7 @@
 #include "qemu/error-report.h"
 #include "trace.h"
 #include <libfdt.h>
+#include "cpu.h"
 
 #define OV_MAXBYTES 256 /* not including length byte */
 #define OV_MAXBITS (OV_MAXBYTES * BITS_PER_BYTE)
@@ -176,7 +177,7 @@ static target_ulong vector_addr(target_ulong table_addr, int vector)
     return table_addr;
 }
 
-SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
+SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector)
 {
     SpaprOptionVector *ov;
     target_ulong addr;
diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
index c3e8b98e7e..d756b916e4 100644
--- a/include/hw/ppc/spapr_ovec.h
+++ b/include/hw/ppc/spapr_ovec.h
@@ -37,7 +37,7 @@
 #ifndef SPAPR_OVEC_H
 #define SPAPR_OVEC_H
 
-#include "cpu.h"
+#include "exec/hwaddr.h"
 
 typedef struct SpaprOptionVector SpaprOptionVector;
 
@@ -73,7 +73,7 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
 void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
 bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
 bool spapr_ovec_empty(SpaprOptionVector *ov);
-SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
+SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector);
 int spapr_dt_ovec(void *fdt, int fdt_offset,
                   SpaprOptionVector *ov, const char *name);
 
-- 
2.38.1

Re: [PATCH-for-8.0 4/4] hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()

[Harsh Prateek Bora]

On 12/13/22 18:05, Philippe Mathieu-Daudé wrote:
> spapr_ovec.c is a device, but it uses target_ulong which is
> target specific. The hwaddr type (declared in "exec/hwaddr.h")
> better fits hardware addresses.
> 
> Change spapr_ovec_parse_vector() to take a hwaddr argument,
> allowing the removal of "cpu.h" in a device header.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ppc/spapr_ovec.c         | 3 ++-
>   include/hw/ppc/spapr_ovec.h | 4 ++--
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> index b2567caa5c..a18a751b57 100644
> --- a/hw/ppc/spapr_ovec.c
> +++ b/hw/ppc/spapr_ovec.c
> @@ -19,6 +19,7 @@
>   #include "qemu/error-report.h"
>   #include "trace.h"
>   #include <libfdt.h>
> +#include "cpu.h"
>   
>   #define OV_MAXBYTES 256 /* not including length byte */
>   #define OV_MAXBITS (OV_MAXBYTES * BITS_PER_BYTE)
> @@ -176,7 +177,7 @@ static target_ulong vector_addr(target_ulong table_addr, int vector)
>       return table_addr;
>   }
>   
> -SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
> +SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector)

IIUC, Option vectors represents a data structure of vectors to advertise 
guest capabilities to the platform (ref b20b7b7adda4) and doesn't really 
represent a hardware device by itself. IMHO, target_ulong appears to be 
more appropriate for this purpose. However, the header file inclusion 
could be changed to cpu-defs.h if target_ulong is the only requirement here.

regards,
Harsh
>   {
>       SpaprOptionVector *ov;
>       target_ulong addr;
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> index c3e8b98e7e..d756b916e4 100644
> --- a/include/hw/ppc/spapr_ovec.h
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -37,7 +37,7 @@
>   #ifndef SPAPR_OVEC_H
>   #define SPAPR_OVEC_H
>   
> -#include "cpu.h"
> +#include "exec/hwaddr.h"
>   
>   typedef struct SpaprOptionVector SpaprOptionVector;
>   
> @@ -73,7 +73,7 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
>   void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
>   bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
>   bool spapr_ovec_empty(SpaprOptionVector *ov);
> -SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
> +SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector);
>   int spapr_dt_ovec(void *fdt, int fdt_offset,
>                     SpaprOptionVector *ov, const char *name);
>   

Re: [PATCH-for-8.0 1/4] target/ppc/kvm: Add missing "cpu.h" and "exec/hwaddr.h"

[Daniel Henrique Barboza]

On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
> kvm_ppc.h is missing various declarations from "cpu.h":
> 
>    target/ppc/kvm_ppc.h:128:40: error: unknown type name 'CPUPPCState'; did you mean 'CPUState'?
>    static inline int kvmppc_get_hypercall(CPUPPCState *env,
>                                           ^~~~~~~~~~~
>                                           CPUState
>    include/qemu/typedefs.h:45:25: note: 'CPUState' declared here
>    typedef struct CPUState CPUState;
>                            ^
>    target/ppc/kvm_ppc.h:134:40: error: unknown type name 'PowerPCCPU'
>    static inline int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level)
>                                           ^
>    target/ppc/kvm_ppc.h:285:38: error: unknown type name 'hwaddr'
>                                         hwaddr ptex, int n)
>                                         ^
>    target/ppc/kvm_ppc.h:220:15: error: unknown type name 'target_ulong'
>    static inline target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
>                  ^
>    target/ppc/kvm_ppc.h:286:38: error: unknown type name 'ppc_hash_pte64_t'
>    static inline void kvmppc_read_hptes(ppc_hash_pte64_t *hptes,
>                                         ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   target/ppc/kvm_ppc.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index ee9325bf9a..5fd9753953 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -9,6 +9,9 @@
>   #ifndef KVM_PPC_H
>   #define KVM_PPC_H
>   
> +#include "exec/hwaddr.h"
> +#include "cpu.h"
> +
>   #define TYPE_HOST_POWERPC_CPU POWERPC_CPU_TYPE_NAME("host")
>   
>   #ifdef CONFIG_KVM

Re: [PATCH-for-8.0 2/4] hw/ppc/vof: Do not include the full "cpu.h"

[Daniel Henrique Barboza]

On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
> "vof.h" doesn't need the full "cpu.h" to get the target_ulong
> definition, including "exec/cpu-defs.h" is enough.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/ppc/vof.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/vof.h b/include/hw/ppc/vof.h
> index f8c0effcaf..d3f293da8b 100644
> --- a/include/hw/ppc/vof.h
> +++ b/include/hw/ppc/vof.h
> @@ -9,7 +9,7 @@
>   #include "qom/object.h"
>   #include "exec/address-spaces.h"
>   #include "exec/memory.h"
> -#include "cpu.h"
> +#include "exec/cpu-defs.h"
>   
>   typedef struct Vof {
>       uint64_t top_addr; /* copied from rma_size */

Re: [PATCH-for-8.0 3/4] hw/ppc/spapr: Reduce "vof.h" inclusion

[Daniel Henrique Barboza]

On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
> Currently objects including "hw/ppc/spapr.h" are forced to be
> target specific due to the inclusion of "vof.h" in "spapr.h".
> 
> "spapr.h" only uses a Vof pointer, so doesn't require the structure
> declaration. The only place where Vof structure is accessed is in
> spapr.c, so include "vof.h" there, and forward declare the structure
> in "spapr.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/ppc/spapr.c         | 1 +
>   include/hw/ppc/spapr.h | 3 ++-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 66b414d2e9..f38a851ee3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -62,6 +62,7 @@
>   #include "hw/ppc/fdt.h"
>   #include "hw/ppc/spapr.h"
>   #include "hw/ppc/spapr_vio.h"
> +#include "hw/ppc/vof.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/pci-host/spapr.h"
>   #include "hw/pci/msi.h"
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 04a95669ab..5c8aabd444 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -12,7 +12,6 @@
>   #include "hw/ppc/spapr_xive.h"  /* For SpaprXive */
>   #include "hw/ppc/xics.h"        /* For ICSState */
>   #include "hw/ppc/spapr_tpm_proxy.h"
> -#include "hw/ppc/vof.h"
>   
>   struct SpaprVioBus;
>   struct SpaprPhbState;
> @@ -22,6 +21,8 @@ typedef struct SpaprEventLogEntry SpaprEventLogEntry;
>   typedef struct SpaprEventSource SpaprEventSource;
>   typedef struct SpaprPendingHpt SpaprPendingHpt;
>   
> +typedef struct Vof Vof;
> +
>   #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>   #define SPAPR_ENTRY_POINT       0x100
>   

Re: [PATCH-for-8.0 4/4] hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()

[Daniel Henrique Barboza]

On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
> spapr_ovec.c is a device, but it uses target_ulong which is
> target specific. The hwaddr type (declared in "exec/hwaddr.h")
> better fits hardware addresses.

As said by Harsh, spapr_ovec is in fact a data structure that stores platform
options that are supported by the guest.

That doesn't mean that I oppose the change made here. Aside from semantics - which
I also don't have a strong opinion about it - I don't believe it matters that
much - spapr is 64 bit only, so hwaddr will always be == target_ulong.

Cedric/David/Greg, let me know if you have any restriction/thoughts about this.
I'm inclined to accept it as is.


Daniel

> 
> Change spapr_ovec_parse_vector() to take a hwaddr argument,
> allowing the removal of "cpu.h" in a device header.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ppc/spapr_ovec.c         | 3 ++-
>   include/hw/ppc/spapr_ovec.h | 4 ++--
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> index b2567caa5c..a18a751b57 100644
> --- a/hw/ppc/spapr_ovec.c
> +++ b/hw/ppc/spapr_ovec.c
> @@ -19,6 +19,7 @@
>   #include "qemu/error-report.h"
>   #include "trace.h"
>   #include <libfdt.h>
> +#include "cpu.h"
>   
>   #define OV_MAXBYTES 256 /* not including length byte */
>   #define OV_MAXBITS (OV_MAXBYTES * BITS_PER_BYTE)
> @@ -176,7 +177,7 @@ static target_ulong vector_addr(target_ulong table_addr, int vector)
>       return table_addr;
>   }
>   
> -SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
> +SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector)
>   {
>       SpaprOptionVector *ov;
>       target_ulong addr;
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> index c3e8b98e7e..d756b916e4 100644
> --- a/include/hw/ppc/spapr_ovec.h
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -37,7 +37,7 @@
>   #ifndef SPAPR_OVEC_H
>   #define SPAPR_OVEC_H
>   
> -#include "cpu.h"
> +#include "exec/hwaddr.h"
>   
>   typedef struct SpaprOptionVector SpaprOptionVector;
>   
> @@ -73,7 +73,7 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
>   void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
>   bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
>   bool spapr_ovec_empty(SpaprOptionVector *ov);
> -SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
> +SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector);
>   int spapr_dt_ovec(void *fdt, int fdt_offset,
>                     SpaprOptionVector *ov, const char *name);
>   

Re: [PATCH-for-8.0 0/4] ppc: Clean up few headers to make them target agnostic

[Daniel Henrique Barboza]

On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
> Few changes in hw/ & target/ to reduce the target specificity
> of some sPAPR headers.
> 
> Philippe Mathieu-Daudé (4):
>    target/ppc/kvm: Add missing "cpu.h" and "exec/hwaddr.h"
>    hw/ppc/vof: Do not include the full "cpu.h"
>    hw/ppc/spapr: Reduce "vof.h" inclusion
>    hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()

Patches 1-3 queued in https://gitlab.com/danielhb/qemu/tree/ppc-next. Patch
4 can use a few more comments.


Thanks,


Daniel

> 
>   hw/ppc/spapr.c              | 1 +
>   hw/ppc/spapr_ovec.c         | 3 ++-
>   include/hw/ppc/spapr.h      | 3 ++-
>   include/hw/ppc/spapr_ovec.h | 4 ++--
>   include/hw/ppc/vof.h        | 2 +-
>   target/ppc/kvm_ppc.h        | 3 +++
>   6 files changed, 11 insertions(+), 5 deletions(-)
> 

Re: [PATCH-for-8.0 0/4] ppc: Clean up few headers to make them target agnostic

[Philippe Mathieu-Daudé]

On 16/12/22 17:54, Daniel Henrique Barboza wrote:
> 
> 
> On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
>> Few changes in hw/ & target/ to reduce the target specificity
>> of some sPAPR headers.
>>
>> Philippe Mathieu-Daudé (4):
>>    target/ppc/kvm: Add missing "cpu.h" and "exec/hwaddr.h"
>>    hw/ppc/vof: Do not include the full "cpu.h"
>>    hw/ppc/spapr: Reduce "vof.h" inclusion
>>    hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()
> 
> Patches 1-3 queued in https://gitlab.com/danielhb/qemu/tree/ppc-next.

Thanks!

> Patch 4 can use a few more comments.

Yes, I'm not sure yet how to improve it, but I'll work on it.

Regards,

Phil.

Re: [PATCH-for-8.0 4/4] hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()

[Cédric Le Goater]

On 12/16/22 17:47, Daniel Henrique Barboza wrote:
> 
> 
> On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
>> spapr_ovec.c is a device, but it uses target_ulong which is
>> target specific. The hwaddr type (declared in "exec/hwaddr.h")
>> better fits hardware addresses.
> 
> As said by Harsh, spapr_ovec is in fact a data structure that stores platform
> options that are supported by the guest.
> 
> That doesn't mean that I oppose the change made here. Aside from semantics - which
> I also don't have a strong opinion about it - I don't believe it matters that
> much - spapr is 64 bit only, so hwaddr will always be == target_ulong.
> 
> Cedric/David/Greg, let me know if you have any restriction/thoughts about this.
> I'm inclined to accept it as is.

Well, I am not sure.

The vector table variable is the result of a ppc64_phys_to_real() conversion
of the CAS hcall parameter, which is a target_ulong, but ppc64_phys_to_real()
returns a uint64_t.

The code is not consistent in another places :

   hw/ppc/spapr_tpm_proxy.c uses a uint64_t
   hw/ppc/spapr_hcall.c, a target_ulong
   hw/ppc/spapr_rtas.c, a hwaddr
   hw/ppc/spapr_drc.c, a hwaddr indirectly

Should we change ppc64_phys_to_real() to return an hwaddr (also) ?

C.


> 
> 
> Daniel
> 
>>
>> Change spapr_ovec_parse_vector() to take a hwaddr argument,
>> allowing the removal of "cpu.h" in a device header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/ppc/spapr_ovec.c         | 3 ++-
>>   include/hw/ppc/spapr_ovec.h | 4 ++--
>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
>> index b2567caa5c..a18a751b57 100644
>> --- a/hw/ppc/spapr_ovec.c
>> +++ b/hw/ppc/spapr_ovec.c
>> @@ -19,6 +19,7 @@
>>   #include "qemu/error-report.h"
>>   #include "trace.h"
>>   #include <libfdt.h>
>> +#include "cpu.h"
>>   #define OV_MAXBYTES 256 /* not including length byte */
>>   #define OV_MAXBITS (OV_MAXBYTES * BITS_PER_BYTE)
>> @@ -176,7 +177,7 @@ static target_ulong vector_addr(target_ulong table_addr, int vector)
>>       return table_addr;
>>   }
>> -SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
>> +SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector)
>>   {
>>       SpaprOptionVector *ov;
>>       target_ulong addr;
>> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
>> index c3e8b98e7e..d756b916e4 100644
>> --- a/include/hw/ppc/spapr_ovec.h
>> +++ b/include/hw/ppc/spapr_ovec.h
>> @@ -37,7 +37,7 @@
>>   #ifndef SPAPR_OVEC_H
>>   #define SPAPR_OVEC_H
>> -#include "cpu.h"
>> +#include "exec/hwaddr.h"
>>   typedef struct SpaprOptionVector SpaprOptionVector;
>> @@ -73,7 +73,7 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
>>   void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
>>   bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
>>   bool spapr_ovec_empty(SpaprOptionVector *ov);
>> -SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
>> +SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector);
>>   int spapr_dt_ovec(void *fdt, int fdt_offset,
>>                     SpaprOptionVector *ov, const char *name);

Re: [PATCH-for-8.0 4/4] hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()

[Daniel Henrique Barboza]

On 12/21/22 06:46, Cédric Le Goater wrote:
> On 12/16/22 17:47, Daniel Henrique Barboza wrote:
>>
>>
>> On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
>>> spapr_ovec.c is a device, but it uses target_ulong which is
>>> target specific. The hwaddr type (declared in "exec/hwaddr.h")
>>> better fits hardware addresses.
>>
>> As said by Harsh, spapr_ovec is in fact a data structure that stores platform
>> options that are supported by the guest.
>>
>> That doesn't mean that I oppose the change made here. Aside from semantics - which
>> I also don't have a strong opinion about it - I don't believe it matters that
>> much - spapr is 64 bit only, so hwaddr will always be == target_ulong.
>>
>> Cedric/David/Greg, let me know if you have any restriction/thoughts about this.
>> I'm inclined to accept it as is.
> 
> Well, I am not sure.
> 
> The vector table variable is the result of a ppc64_phys_to_real() conversion
> of the CAS hcall parameter, which is a target_ulong, but ppc64_phys_to_real()
> returns a uint64_t.
> 
> The code is not consistent in another places :
> 
>    hw/ppc/spapr_tpm_proxy.c uses a uint64_t
>    hw/ppc/spapr_hcall.c, a target_ulong
>    hw/ppc/spapr_rtas.c, a hwaddr
>    hw/ppc/spapr_drc.c, a hwaddr indirectly
> 
> Should we change ppc64_phys_to_real() to return an hwaddr (also) ?

It makes sense to me a function called ppc64_phys_to_real() returning
a hwaddr type.

Daniel


> 
> C.
> 
> 
>>
>>
>> Daniel
>>
>>>
>>> Change spapr_ovec_parse_vector() to take a hwaddr argument,
>>> allowing the removal of "cpu.h" in a device header.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/ppc/spapr_ovec.c         | 3 ++-
>>>   include/hw/ppc/spapr_ovec.h | 4 ++--
>>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
>>> index b2567caa5c..a18a751b57 100644
>>> --- a/hw/ppc/spapr_ovec.c
>>> +++ b/hw/ppc/spapr_ovec.c
>>> @@ -19,6 +19,7 @@
>>>   #include "qemu/error-report.h"
>>>   #include "trace.h"
>>>   #include <libfdt.h>
>>> +#include "cpu.h"
>>>   #define OV_MAXBYTES 256 /* not including length byte */
>>>   #define OV_MAXBITS (OV_MAXBYTES * BITS_PER_BYTE)
>>> @@ -176,7 +177,7 @@ static target_ulong vector_addr(target_ulong table_addr, int vector)
>>>       return table_addr;
>>>   }
>>> -SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
>>> +SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector)
>>>   {
>>>       SpaprOptionVector *ov;
>>>       target_ulong addr;
>>> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
>>> index c3e8b98e7e..d756b916e4 100644
>>> --- a/include/hw/ppc/spapr_ovec.h
>>> +++ b/include/hw/ppc/spapr_ovec.h
>>> @@ -37,7 +37,7 @@
>>>   #ifndef SPAPR_OVEC_H
>>>   #define SPAPR_OVEC_H
>>> -#include "cpu.h"
>>> +#include "exec/hwaddr.h"
>>>   typedef struct SpaprOptionVector SpaprOptionVector;
>>> @@ -73,7 +73,7 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
>>>   void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
>>>   bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
>>>   bool spapr_ovec_empty(SpaprOptionVector *ov);
>>> -SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
>>> +SpaprOptionVector *spapr_ovec_parse_vector(hwaddr table_addr, int vector);
>>>   int spapr_dt_ovec(void *fdt, int fdt_offset,
>>>                     SpaprOptionVector *ov, const char *name);
> 

Re: [PATCH-for-8.0 4/4] hw/ppc/spapr_ovec: Avoid target_ulong spapr_ovec_parse_vector()

[David Gibson]

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

On Wed, Dec 21, 2022 at 10:26:51AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 12/21/22 06:46, Cédric Le Goater wrote:
> > On 12/16/22 17:47, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 12/13/22 09:35, Philippe Mathieu-Daudé wrote:
> > > > spapr_ovec.c is a device, but it uses target_ulong which is
> > > > target specific. The hwaddr type (declared in "exec/hwaddr.h")
> > > > better fits hardware addresses.
> > > 
> > > As said by Harsh, spapr_ovec is in fact a data structure that stores platform
> > > options that are supported by the guest.
> > > 
> > > That doesn't mean that I oppose the change made here. Aside from semantics - which
> > > I also don't have a strong opinion about it - I don't believe it matters that
> > > much - spapr is 64 bit only, so hwaddr will always be == target_ulong.
> > > 
> > > Cedric/David/Greg, let me know if you have any restriction/thoughts about this.
> > > I'm inclined to accept it as is.
> > 
> > Well, I am not sure.
> > 
> > The vector table variable is the result of a ppc64_phys_to_real() conversion
> > of the CAS hcall parameter, which is a target_ulong, but ppc64_phys_to_real()
> > returns a uint64_t.
> > 
> > The code is not consistent in another places :
> > 
> >    hw/ppc/spapr_tpm_proxy.c uses a uint64_t
> >    hw/ppc/spapr_hcall.c, a target_ulong
> >    hw/ppc/spapr_rtas.c, a hwaddr
> >    hw/ppc/spapr_drc.c, a hwaddr indirectly
> > 
> > Should we change ppc64_phys_to_real() to return an hwaddr (also) ?
> 
> It makes sense to me a function called ppc64_phys_to_real() returning
> a hwaddr type.

Yes, I also think that makes sense.

-- 
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: 833 bytes --]