include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- target/ppc/cpu.h | 12 ++++++++++++ hw/intc/pnv_xive.c | 20 -------------------- hw/intc/pnv_xive2.c | 20 -------------------- hw/pci-host/pnv_phb4.c | 16 ---------------- 5 files changed, 12 insertions(+), 72 deletions(-)
It keeps repeating, move it to the header. This uses __builtin_ffsl() to
allow using the macros in #define.
This is not using the QEMU's FIELD macros as this would require changing
all such macros found in skiboot (the PPC PowerNV firmware).
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* preserved the comment about skiboot
* copied the actual macros from skiboot:
https://github.com/open-power/skiboot/blob/master/include/bitutils.h#L31
---
include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
target/ppc/cpu.h | 12 ++++++++++++
hw/intc/pnv_xive.c | 20 --------------------
hw/intc/pnv_xive2.c | 20 --------------------
hw/pci-host/pnv_phb4.c | 16 ----------------
5 files changed, 12 insertions(+), 72 deletions(-)
diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
index a174ef1f7045..38f8ce9d7406 100644
--- a/include/hw/pci-host/pnv_phb3_regs.h
+++ b/include/hw/pci-host/pnv_phb3_regs.h
@@ -12,22 +12,6 @@
#include "qemu/host-utils.h"
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * These are common with the PnvXive model.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
- return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
- uint64_t value)
-{
- return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
/*
* PBCQ XSCOM registers
*/
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 6d78078f379d..e45cc7a8c115 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -47,6 +47,18 @@
PPC_BIT32(bs))
#define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
+/*
+ * QEMU version of the GETFIELD/SETFIELD macros
+ *
+ * It might be better to use the existing extract64() and
+ * deposit64() but this means that all the register definitions will
+ * change and become incompatible with the ones found in skiboot.
+ */
+#define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1)
+#define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m))
+#define SETFIELD(m, v, val) \
+ (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
+
/*****************************************************************************/
/* Exception vectors definitions */
enum {
diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index 1ce1d7b07d63..c7b75ed12ee0 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
(xive)->chip->chip_id, ## __VA_ARGS__);
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * TODO: It might be better to use the existing extract64() and
- * deposit64() but this means that all the register definitions will
- * change and become incompatible with the ones found in skiboot.
- *
- * Keep it as it is for now until we find a common ground.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
- return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
- uint64_t value)
-{
- return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
/*
* When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
* field overrides the hardwired chip ID in the Powerbus operations
diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index f31c53c28dd2..f22ce5ca59ae 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \
(xive)->chip->chip_id, ## __VA_ARGS__);
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * TODO: It might be better to use the existing extract64() and
- * deposit64() but this means that all the register definitions will
- * change and become incompatible with the ones found in skiboot.
- *
- * Keep it as it is for now until we find a common ground.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
- return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
- uint64_t value)
-{
- return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
/*
* TODO: Document block id override
*/
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 6594016121a3..5d72c0c432b2 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -31,22 +31,6 @@
qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n", \
(pec)->chip_id, (pec)->index, ## __VA_ARGS__)
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * These are common with the PnvXive model.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
- return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
- uint64_t value)
-{
- return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
{
PCIHostState *pci = PCI_HOST_BRIDGE(phb);
--
2.30.2
Alexey, Gitlab does not like what you're doing here. Several cross compile runners fails with errors like these (this is from cross-win64-system): ../hw/intc/pnv_xive.c: In function 'pnv_xive_block_id': 3328/builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: overflow in conversion from 'long long unsigned int' to 'long int' changes value from '4222124650659840' to '0' [-Werror=overflow] 3329 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs)) 3330 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3331/builds/danielhb/qemu/target/ppc/cpu.h:57:49: note: in definition of macro 'MASK_TO_LSH' 3332 57 | #define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) 3333 | ^ 3334../hw/intc/pnv_xive.c:80:15: note: in expansion of macro 'GETFIELD' 3335 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val); 3336 | ^~~~~~~~ 3337../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro 'PPC_BITMASK' 3338 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15) 3339 | ^~~~~~~~~~~ 3340../hw/intc/pnv_xive.c:80:24: note: in expansion of macro 'PC_TCTXT_CHIPID' 3341 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val); 3342 | ^~~~~~~~~~~~~~~ 3343/builds/danielhb/qemu/target/ppc/cpu.h:58:46: error: right shift count is negative [-Werror=shift-count-negative] 3344 58 | #define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m)) 3345 | ^~ 3346../hw/intc/pnv_xive.c:80:15: note: in expansion of macro 'GETFIELD' 3347 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val); 3348 | ^~~~~~~~ ../hw/intc/pnv_xive.c: In function 'pnv_xive_vst_addr': 3350/builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: overflow in conversion from 'long long unsigned int' to 'long int' changes value from '13835058055282163712' to '0' [-Werror=overflow] 3351 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs)) 3352 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3353/builds/danielhb/qemu/target/ppc/cpu.h:57:49: note: in definition of macro 'MASK_TO_LSH' 3354 57 | #define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) 3355 | ^ 3356../hw/intc/pnv_xive.c:226:9: note: in expansion of macro 'GETFIELD' 3357 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) { 3358 | ^~~~~~~~ 3359../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro 'PPC_BITMASK' 3360 230 | #define VSD_MODE PPC_BITMASK(0, 1) 3361 | ^~~~~~~~~~~ 3362../hw/intc/pnv_xive.c:226:18: note: in expansion of macro 'VSD_MODE' 3363 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) { 3364 | ^~~~~~~~ 3365/builds/danielhb/qemu/target/ppc/cpu.h:58:46: error: right shift count is negative [-Werror=shift-count-negative] 3366 58 | #define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m)) 3367 | ^~ 3368../hw/intc/pnv_xive.c:226:9: note: in expansion of macro 'GETFIELD' 3369 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) { 3370 | ^~~~~~~~ You can see the results here: https://gitlab.com/danielhb/qemu/-/jobs/2636585317 Other failing runners include cross-armel-system and cross-mips-system, so I don't think that the error is related to Windows specifics. I guess we're missing an uint64_t cast somewhere like you did in the v2 of this patch. The skiboot macros as is will not cut it. Thanks, Daniel On 6/22/22 02:08, Alexey Kardashevskiy wrote: > It keeps repeating, move it to the header. This uses __builtin_ffsl() to > allow using the macros in #define. > > This is not using the QEMU's FIELD macros as this would require changing > all such macros found in skiboot (the PPC PowerNV firmware). > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > Changes: > v2: > * preserved the comment about skiboot > * copied the actual macros from skiboot: > https://github.com/open-power/skiboot/blob/master/include/bitutils.h#L31 > --- > include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- > target/ppc/cpu.h | 12 ++++++++++++ > hw/intc/pnv_xive.c | 20 -------------------- > hw/intc/pnv_xive2.c | 20 -------------------- > hw/pci-host/pnv_phb4.c | 16 ---------------- > 5 files changed, 12 insertions(+), 72 deletions(-) > > diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h > index a174ef1f7045..38f8ce9d7406 100644 > --- a/include/hw/pci-host/pnv_phb3_regs.h > +++ b/include/hw/pci-host/pnv_phb3_regs.h > @@ -12,22 +12,6 @@ > > #include "qemu/host-utils.h" > > -/* > - * QEMU version of the GETFIELD/SETFIELD macros > - * > - * These are common with the PnvXive model. > - */ > -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word) > -{ > - return (word & mask) >> ctz64(mask); > -} > - > -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word, > - uint64_t value) > -{ > - return (word & ~mask) | ((value << ctz64(mask)) & mask); > -} > - > /* > * PBCQ XSCOM registers > */ > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 6d78078f379d..e45cc7a8c115 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -47,6 +47,18 @@ > PPC_BIT32(bs)) > #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) > > +/* > + * QEMU version of the GETFIELD/SETFIELD macros > + * > + * It might be better to use the existing extract64() and > + * deposit64() but this means that all the register definitions will > + * change and become incompatible with the ones found in skiboot. > + */ > +#define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) > +#define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m)) > +#define SETFIELD(m, v, val) \ > + (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m))) > + > /*****************************************************************************/ > /* Exception vectors definitions */ > enum { > diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c > index 1ce1d7b07d63..c7b75ed12ee0 100644 > --- a/hw/intc/pnv_xive.c > +++ b/hw/intc/pnv_xive.c > @@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = { > qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \ > (xive)->chip->chip_id, ## __VA_ARGS__); > > -/* > - * QEMU version of the GETFIELD/SETFIELD macros > - * > - * TODO: It might be better to use the existing extract64() and > - * deposit64() but this means that all the register definitions will > - * change and become incompatible with the ones found in skiboot. > - * > - * Keep it as it is for now until we find a common ground. > - */ > -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word) > -{ > - return (word & mask) >> ctz64(mask); > -} > - > -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word, > - uint64_t value) > -{ > - return (word & ~mask) | ((value << ctz64(mask)) & mask); > -} > - > /* > * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID > * field overrides the hardwired chip ID in the Powerbus operations > diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c > index f31c53c28dd2..f22ce5ca59ae 100644 > --- a/hw/intc/pnv_xive2.c > +++ b/hw/intc/pnv_xive2.c > @@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = { > qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \ > (xive)->chip->chip_id, ## __VA_ARGS__); > > -/* > - * QEMU version of the GETFIELD/SETFIELD macros > - * > - * TODO: It might be better to use the existing extract64() and > - * deposit64() but this means that all the register definitions will > - * change and become incompatible with the ones found in skiboot. > - * > - * Keep it as it is for now until we find a common ground. > - */ > -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word) > -{ > - return (word & mask) >> ctz64(mask); > -} > - > -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word, > - uint64_t value) > -{ > - return (word & ~mask) | ((value << ctz64(mask)) & mask); > -} > - > /* > * TODO: Document block id override > */ > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c > index 6594016121a3..5d72c0c432b2 100644 > --- a/hw/pci-host/pnv_phb4.c > +++ b/hw/pci-host/pnv_phb4.c > @@ -31,22 +31,6 @@ > qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n", \ > (pec)->chip_id, (pec)->index, ## __VA_ARGS__) > > -/* > - * QEMU version of the GETFIELD/SETFIELD macros > - * > - * These are common with the PnvXive model. > - */ > -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word) > -{ > - return (word & mask) >> ctz64(mask); > -} > - > -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word, > - uint64_t value) > -{ > - return (word & ~mask) | ((value << ctz64(mask)) & mask); > -} > - > static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb) > { > PCIHostState *pci = PCI_HOST_BRIDGE(phb);
On 6/22/22 02:08, Alexey Kardashevskiy wrote: > It keeps repeating, move it to the header. This uses __builtin_ffsl() to > allow using the macros in #define. > > This is not using the QEMU's FIELD macros as this would require changing > all such macros found in skiboot (the PPC PowerNV firmware). > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > Changes: > v2: > * preserved the comment about skiboot > * copied the actual macros from skiboot: > https://github.com/open-power/skiboot/blob/master/include/bitutils.h#L31 > --- > include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- > target/ppc/cpu.h | 12 ++++++++++++ > hw/intc/pnv_xive.c | 20 -------------------- > hw/intc/pnv_xive2.c | 20 -------------------- > hw/pci-host/pnv_phb4.c | 16 ---------------- > 5 files changed, 12 insertions(+), 72 deletions(-) > > diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h > index a174ef1f7045..38f8ce9d7406 100644 > --- a/include/hw/pci-host/pnv_phb3_regs.h > +++ b/include/hw/pci-host/pnv_phb3_regs.h > @@ -12,22 +12,6 @@ > > #include "qemu/host-utils.h" > > -/* > - * QEMU version of the GETFIELD/SETFIELD macros > - * > - * These are common with the PnvXive model. > - */ > -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word) > -{ > - return (word & mask) >> ctz64(mask); > -} > - > -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word, > - uint64_t value) > -{ > - return (word & ~mask) | ((value << ctz64(mask)) & mask); > -} > - > /* > * PBCQ XSCOM registers > */ > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 6d78078f379d..e45cc7a8c115 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -47,6 +47,18 @@ > PPC_BIT32(bs)) > #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) > > +/* > + * QEMU version of the GETFIELD/SETFIELD macros > + * > + * It might be better to use the existing extract64() and > + * deposit64() but this means that all the register definitions will > + * change and become incompatible with the ones found in skiboot. > + */ > +#define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) > +#define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m)) > +#define SETFIELD(m, v, val) \ > + (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m))) > + > /*****************************************************************************/ > /* Exception vectors definitions */ > enum { > diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c > index 1ce1d7b07d63..c7b75ed12ee0 100644 > --- a/hw/intc/pnv_xive.c > +++ b/hw/intc/pnv_xive.c > @@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = { > qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \ > (xive)->chip->chip_id, ## __VA_ARGS__); > > -/* > - * QEMU version of the GETFIELD/SETFIELD macros > - * > - * TODO: It might be better to use the existing extract64() and > - * deposit64() but this means that all the register definitions will > - * change and become incompatible with the ones found in skiboot. > - * > - * Keep it as it is for now until we find a common ground. > - */ > -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word) > -{ > - return (word & mask) >> ctz64(mask); > -} > - > -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word, > - uint64_t value) > -{ > - return (word & ~mask) | ((value << ctz64(mask)) & mask); > -} > - > /* > * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID > * field overrides the hardwired chip ID in the Powerbus operations > diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c > index f31c53c28dd2..f22ce5ca59ae 100644 > --- a/hw/intc/pnv_xive2.c > +++ b/hw/intc/pnv_xive2.c > @@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = { > qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n", \ > (xive)->chip->chip_id, ## __VA_ARGS__); > > -/* > - * QEMU version of the GETFIELD/SETFIELD macros > - * > - * TODO: It might be better to use the existing extract64() and > - * deposit64() but this means that all the register definitions will > - * change and become incompatible with the ones found in skiboot. > - * > - * Keep it as it is for now until we find a common ground. > - */ > -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word) > -{ > - return (word & mask) >> ctz64(mask); > -} > - > -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word, > - uint64_t value) > -{ > - return (word & ~mask) | ((value << ctz64(mask)) & mask); > -} > - > /* > * TODO: Document block id override > */ > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c > index 6594016121a3..5d72c0c432b2 100644 > --- a/hw/pci-host/pnv_phb4.c > +++ b/hw/pci-host/pnv_phb4.c > @@ -31,22 +31,6 @@ > qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n", \ > (pec)->chip_id, (pec)->index, ## __VA_ARGS__) > > -/* > - * QEMU version of the GETFIELD/SETFIELD macros > - * > - * These are common with the PnvXive model. > - */ > -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word) > -{ > - return (word & mask) >> ctz64(mask); > -} > - > -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word, > - uint64_t value) > -{ > - return (word & ~mask) | ((value << ctz64(mask)) & mask); > -} > - > static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb) > { > PCIHostState *pci = PCI_HOST_BRIDGE(phb);
© 2016 - 2024 Red Hat, Inc.