Re: [PATCH qemu v2] ppc: Define SETFIELD for the ppc target

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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