| Message ID | 20221213123550.39302-5-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ppc: Clean up few headers to make them target agnostic | expand |
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); >
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); >
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);
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); >
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 --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);
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(-)