diff mbox series

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

Message ID 20221213123550.39302-5-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series ppc: Clean up few headers to make them target agnostic | expand

Commit Message

Philippe Mathieu-Daudé Dec. 13, 2022, 12:35 p.m. UTC
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(-)

Comments

Harsh Prateek Bora Dec. 13, 2022, 4:40 p.m. UTC | #1
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);
>
Daniel Henrique Barboza Dec. 16, 2022, 4:47 p.m. UTC | #2
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);
>
Cédric Le Goater Dec. 21, 2022, 9:46 a.m. UTC | #3
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);
Daniel Henrique Barboza Dec. 21, 2022, 1:26 p.m. UTC | #4
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);
>
David Gibson Dec. 22, 2022, 1:57 a.m. UTC | #5
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.
diff mbox series

Patch

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