| Message ID | 20220617060703.951747-2-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ppc/spapr: Implement H_WATCHDOG | expand |
On 6/17/22 03:07, Alexey Kardashevskiy wrote: > It keeps repeating, move it to the header. This uses __builtin_ctzl() to > allow using the macros in #define. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- > target/ppc/cpu.h | 5 +++++ > hw/intc/pnv_xive.c | 20 -------------------- > hw/intc/pnv_xive2.c | 20 -------------------- > hw/pci-host/pnv_phb4.c | 16 ---------------- > 5 files changed, 5 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..9a1f1e9999a3 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -47,6 +47,11 @@ > PPC_BIT32(bs)) > #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) > > +#define GETFIELD(mask, word) \ > + (((word) & (mask)) >> __builtin_ctzl(mask)) > +#define SETFIELD(mask, word, val) \ > + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask))) > + > /*****************************************************************************/ > /* 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 a39e070e82d2..3fe349749384 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 13ba9e45d8b6..0913e7c8f015 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/17/22 08:07, Alexey Kardashevskiy wrote: > It keeps repeating, move it to the header. This uses __builtin_ctzl() to > allow using the macros in #define. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks Alexey, C. > --- > include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- > target/ppc/cpu.h | 5 +++++ > hw/intc/pnv_xive.c | 20 -------------------- > hw/intc/pnv_xive2.c | 20 -------------------- > hw/pci-host/pnv_phb4.c | 16 ---------------- > 5 files changed, 5 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..9a1f1e9999a3 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -47,6 +47,11 @@ > PPC_BIT32(bs)) > #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) > > +#define GETFIELD(mask, word) \ > + (((word) & (mask)) >> __builtin_ctzl(mask)) > +#define SETFIELD(mask, word, val) \ > + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask))) > + > /*****************************************************************************/ > /* 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 a39e070e82d2..3fe349749384 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 13ba9e45d8b6..0913e7c8f015 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/18/22 02:50, Daniel Henrique Barboza wrote: > > > On 6/17/22 03:07, Alexey Kardashevskiy wrote: >> It keeps repeating, move it to the header. This uses __builtin_ctzl() to >> allow using the macros in #define. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- > > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> soooo I do not need to repost it, right?
On 6/20/22 05:37, Alexey Kardashevskiy wrote: > > > On 6/18/22 02:50, Daniel Henrique Barboza wrote: >> >> >> On 6/17/22 03:07, Alexey Kardashevskiy wrote: >>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to >>> allow using the macros in #define. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >> >> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > > > > soooo I do not need to repost it, right? > > The watchdog patch depends on the availability of these macros. Doesn't it ? C.
On 6/20/22 16:17, Cédric Le Goater wrote: > On 6/20/22 05:37, Alexey Kardashevskiy wrote: >> >> >> On 6/18/22 02:50, Daniel Henrique Barboza wrote: >>> >>> >>> On 6/17/22 03:07, Alexey Kardashevskiy wrote: >>>> It keeps repeating, move it to the header. This uses >>>> __builtin_ctzl() to >>>> allow using the macros in #define. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> --- >>> >>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> >> >> >> soooo I do not need to repost it, right? >> >> > > The watchdog patch depends on the availability of these macros. > Doesn't it ? yes but this one could go in now and I will repost 2/2 a little later on top.
On 6/20/22 05:10, Alexey Kardashevskiy wrote: > > > On 6/20/22 16:17, Cédric Le Goater wrote: >> On 6/20/22 05:37, Alexey Kardashevskiy wrote: >>> >>> >>> On 6/18/22 02:50, Daniel Henrique Barboza wrote: >>>> >>>> >>>> On 6/17/22 03:07, Alexey Kardashevskiy wrote: >>>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to >>>>> allow using the macros in #define. >>>>> >>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>>> --- >>>> >>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> >>> >>> >>> soooo I do not need to repost it, right? >>> >>> >> >> The watchdog patch depends on the availability of these macros. >> Doesn't it ? > > yes but this one could go in now and I will repost 2/2 a little later on top. As soon as the current ppc PR is merged I can take this patch. If the 2/2 repost happens earlier than that you can just re-submit it with 1/2 again. Thanks, Daniel > >
On Fri, 17 Jun 2022 at 07:20, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > It keeps repeating, move it to the header. This uses __builtin_ctzl() to > allow using the macros in #define. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- > target/ppc/cpu.h | 5 +++++ > hw/intc/pnv_xive.c | 20 -------------------- > hw/intc/pnv_xive2.c | 20 -------------------- > hw/pci-host/pnv_phb4.c | 16 ---------------- > 5 files changed, 5 insertions(+), 72 deletions(-) > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 6d78078f379d..9a1f1e9999a3 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -47,6 +47,11 @@ > PPC_BIT32(bs)) > #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) > > +#define GETFIELD(mask, word) \ > + (((word) & (mask)) >> __builtin_ctzl(mask)) > +#define SETFIELD(mask, word, val) \ > + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask))) Can we retain the explanatory comment that says why we don't use the standard QEMU mechanism for field extraction (ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64() functions) ? > -/* > - * 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. > - */ thanks -- PMM
Alexey,
The newer version of this patch is having trouble with Gitlab runners, as
you can read in my feedback there.
I've tested this one just in case. The same problems happen. E.g. for the
cross-armel-system runner:
In file included from ../hw/intc/pnv_xive.c:14:
../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
/builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow]
45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
51 | (((word) & (mask)) >> __builtin_ctzl(mask))
| ^~~~
../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15)
| ^~~~~~~~~~~
../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
| ^~~~~~~~~~~~~~~
../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
/builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
51 | (((word) & (mask)) >> __builtin_ctzl(mask))
| ^~~~
../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’
230 | #define VSD_MODE PPC_BITMASK(0, 1)
| ^~~~~~~~~~~
../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
| ^~~~~~~~
../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
Link:
https://gitlab.com/danielhb/qemu/-/jobs/2637716673
I don´t know how to deal with that.
For the record: if this is too troublesome to fix, I am ok with just consolidating
the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly
as they are today (functions, not macros).
Thanks,
Daniel
On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
> target/ppc/cpu.h | 5 +++++
> hw/intc/pnv_xive.c | 20 --------------------
> hw/intc/pnv_xive2.c | 20 --------------------
> hw/pci-host/pnv_phb4.c | 16 ----------------
> 5 files changed, 5 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..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
> PPC_BIT32(bs))
> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>
> +#define GETFIELD(mask, word) \
> + (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val) \
> + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
> +
> /*****************************************************************************/
> /* 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 a39e070e82d2..3fe349749384 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 13ba9e45d8b6..0913e7c8f015 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/25/22 06:12, Daniel Henrique Barboza wrote: > Alexey, > > The newer version of this patch is having trouble with Gitlab runners, as > you can read in my feedback there. > > I've tested this one just in case. The same problems happen. E.g. for the > cross-armel-system runner: > > > In file included from ../hw/intc/pnv_xive.c:14: > ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’: > /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from > ‘long long unsigned int’ to ‘long unsigned int’ changes value from > ‘4222124650659840’ to ‘0’ [-Werror=overflow] > 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | > PPC_BIT(bs)) > | > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of > macro ‘GETFIELD’ > 51 | (((word) & (mask)) >> __builtin_ctzl(mask)) > | ^~~~ > ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’ > 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15) > | ^~~~~~~~~~~ > ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’ > 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val); > | ^~~~~~~~~~~~~~~ > ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’: > /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from > ‘long long unsigned int’ to ‘long unsigned int’ changes value from > ‘13835058055282163712’ to ‘0’ [-Werror=overflow] > 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | > PPC_BIT(bs)) > | > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of > macro ‘GETFIELD’ > 51 | (((word) & (mask)) >> __builtin_ctzl(mask)) > | ^~~~ > ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro > ‘PPC_BITMASK’ > 230 | #define VSD_MODE PPC_BITMASK(0, 1) > | ^~~~~~~~~~~ > ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’ > 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) { > | ^~~~~~~~ > ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’: > > > Link: > > https://gitlab.com/danielhb/qemu/-/jobs/2637716673 > > > I don´t know how to deal with that. > > > For the record: if this is too troublesome to fix, I am ok with just > consolidating > the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping > them exactly > as they are today (functions, not macros). > > > Thanks, > > > Daniel > > > > On 6/17/22 03:07, Alexey Kardashevskiy wrote: >> It keeps repeating, move it to the header. This uses __builtin_ctzl() to >> allow using the macros in #define. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- >> target/ppc/cpu.h | 5 +++++ >> hw/intc/pnv_xive.c | 20 -------------------- >> hw/intc/pnv_xive2.c | 20 -------------------- >> hw/pci-host/pnv_phb4.c | 16 ---------------- >> 5 files changed, 5 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..9a1f1e9999a3 100644 >> --- a/target/ppc/cpu.h >> +++ b/target/ppc/cpu.h >> @@ -47,6 +47,11 @@ >> PPC_BIT32(bs)) >> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | >> PPC_BIT8(bs)) >> +#define GETFIELD(mask, word) \ >> + (((word) & (mask)) >> __builtin_ctzl(mask)) Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do you have a quick way to test this? Gitlab's CI takes time :) https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run. Thanks,
On 6/27/22 01:54, Alexey Kardashevskiy wrote: > > > On 6/25/22 06:12, Daniel Henrique Barboza wrote: >> Alexey, >> >> The newer version of this patch is having trouble with Gitlab runners, as >> you can read in my feedback there. >> >> I've tested this one just in case. The same problems happen. E.g. for the >> cross-armel-system runner: >> >> >> In file included from ../hw/intc/pnv_xive.c:14: >> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’: >> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow] >> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs)) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’ >> 51 | (((word) & (mask)) >> __builtin_ctzl(mask)) >> | ^~~~ >> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’ >> 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15) >> | ^~~~~~~~~~~ >> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’ >> 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val); >> | ^~~~~~~~~~~~~~~ >> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’: >> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow] >> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs)) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’ >> 51 | (((word) & (mask)) >> __builtin_ctzl(mask)) >> | ^~~~ >> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’ >> 230 | #define VSD_MODE PPC_BITMASK(0, 1) >> | ^~~~~~~~~~~ >> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’ >> 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) { >> | ^~~~~~~~ >> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’: >> >> >> Link: >> >> https://gitlab.com/danielhb/qemu/-/jobs/2637716673 >> >> >> I don´t know how to deal with that. >> >> >> For the record: if this is too troublesome to fix, I am ok with just consolidating >> the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly >> as they are today (functions, not macros). >> >> >> Thanks, >> >> >> Daniel >> >> >> >> On 6/17/22 03:07, Alexey Kardashevskiy wrote: >>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to >>> allow using the macros in #define. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- >>> target/ppc/cpu.h | 5 +++++ >>> hw/intc/pnv_xive.c | 20 -------------------- >>> hw/intc/pnv_xive2.c | 20 -------------------- >>> hw/pci-host/pnv_phb4.c | 16 ---------------- >>> 5 files changed, 5 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..9a1f1e9999a3 100644 >>> --- a/target/ppc/cpu.h >>> +++ b/target/ppc/cpu.h >>> @@ -47,6 +47,11 @@ >>> PPC_BIT32(bs)) >>> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) >>> +#define GETFIELD(mask, word) \ >>> + (((word) & (mask)) >> __builtin_ctzl(mask)) > > > Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do you have a quick way to test this? Gitlab's CI takes time :) > https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run. I'll take a look, but apparently it fixed the problem I reported up above. Also, there's an error in the msys2-64bit runner that I keep seeing from time to time. It goes away eventually if you keep retrying it: [301/1664] Generating input-keymap-qcode-to-qnum.c.inc with a custom command (wrapped by meson to capture output) [302/1664] Generating input-keymap-qcode-to-sun.c.inc with a custom command (wrapped by meson to capture output) [303/1664] Generating input-keymap-qnum-to-qcode.c.inc with a custom command (wrapped by meson to capture output) [304/1664] Generating input-keymap-usb-to-qcode.c.inc with a custom command (wrapped by meson to capture output) FAILED: ui/input-keymap-usb-to-qcode.c.inc "C:/GitLab-Runner/builds/aik1/qemu/msys64/mingw64/bin/python3.exe" "C:/GitLab-Runner/builds/aik1/qemu/meson/meson.py" "--internal" "exe" "--capture" "ui/input-keymap-usb-to-qcode.c.inc" "--" "C:/GitLab-Runner/builds/aik1/qemu/msys64/mingw64/bin/python3.exe" "../ui/keycodemapdb/tools/keymap-gen" "code-map" "--lang" "glib2" "--varname" "qemu_input_map_usb_to_qcode" "../ui/keycodemapdb/data/keymaps.csv" "usb" "qcode" [305/1664] Generating input-keymap-win32-to-qcode.c.inc with a custom command (wrapped by meson to capture output) ninja: build stopped: subcommand failed. make[1]: Leaving directory '/c/GitLab-Runner/builds/aik1/qemu/build' make[1]: *** [Makefile:162: run-ninja] Error 1 make: *** [GNUmakefile:11: all] Error 2 It's a strange one because it's an error triggered by an ui/keymap file which you're not changing. Richard already created a thread about it in the QEMU ML, so I'll assume that this has nothing to do with powerpc code. Thanks, Daniel > Thanks, > >
On 6/27/22 01:54, Alexey Kardashevskiy wrote: > > > On 6/25/22 06:12, Daniel Henrique Barboza wrote: >> Alexey, >> >> The newer version of this patch is having trouble with Gitlab runners, as >> you can read in my feedback there. >> >> I've tested this one just in case. The same problems happen. E.g. for the >> cross-armel-system runner: >> >> >> In file included from ../hw/intc/pnv_xive.c:14: >> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’: >> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow] >> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs)) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’ >> 51 | (((word) & (mask)) >> __builtin_ctzl(mask)) >> | ^~~~ >> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’ >> 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15) >> | ^~~~~~~~~~~ >> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’ >> 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val); >> | ^~~~~~~~~~~~~~~ >> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’: >> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow] >> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs)) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’ >> 51 | (((word) & (mask)) >> __builtin_ctzl(mask)) >> | ^~~~ >> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’ >> 230 | #define VSD_MODE PPC_BITMASK(0, 1) >> | ^~~~~~~~~~~ >> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’ >> 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) { >> | ^~~~~~~~ >> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’: >> >> >> Link: >> >> https://gitlab.com/danielhb/qemu/-/jobs/2637716673 >> >> >> I don´t know how to deal with that. >> >> >> For the record: if this is too troublesome to fix, I am ok with just consolidating >> the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly >> as they are today (functions, not macros). >> >> >> Thanks, >> >> >> Daniel >> >> >> >> On 6/17/22 03:07, Alexey Kardashevskiy wrote: >>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to >>> allow using the macros in #define. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- >>> target/ppc/cpu.h | 5 +++++ >>> hw/intc/pnv_xive.c | 20 -------------------- >>> hw/intc/pnv_xive2.c | 20 -------------------- >>> hw/pci-host/pnv_phb4.c | 16 ---------------- >>> 5 files changed, 5 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..9a1f1e9999a3 100644 >>> --- a/target/ppc/cpu.h >>> +++ b/target/ppc/cpu.h >>> @@ -47,6 +47,11 @@ >>> PPC_BIT32(bs)) >>> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) >>> +#define GETFIELD(mask, word) \ >>> + (((word) & (mask)) >> __builtin_ctzl(mask)) > > > Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do you have a quick way to test this? Gitlab's CI takes time :) > https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run. > Thanks, This worked for me as well. Can you re-send this patch with this fix plus the extra comment Peter mentioned in his review? ---- Can we retain the explanatory comment that says why we don't use the standard QEMU mechanism for field extraction (ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64() functions) ? ------ You can re-send this as v4 (I'm assuming that we're giving up trying to copy the Skiboot macros) and then I'll take the patch together with the watchdog v3 implementation. Thanks, Daniel > >
On 6/28/22 04:04, Daniel Henrique Barboza wrote: > > > On 6/27/22 01:54, Alexey Kardashevskiy wrote: >> >> >> On 6/25/22 06:12, Daniel Henrique Barboza wrote: >>> Alexey, >>> >>> The newer version of this patch is having trouble with Gitlab >>> runners, as >>> you can read in my feedback there. >>> >>> I've tested this one just in case. The same problems happen. E.g. for >>> the >>> cross-armel-system runner: >>> >>> >>> In file included from ../hw/intc/pnv_xive.c:14: >>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’: >>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from >>> ‘long long unsigned int’ to ‘long unsigned int’ changes value from >>> ‘4222124650659840’ to ‘0’ [-Werror=overflow] >>> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) >>> | PPC_BIT(bs)) >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of >>> macro ‘GETFIELD’ >>> 51 | (((word) & (mask)) >> __builtin_ctzl(mask)) >>> | ^~~~ >>> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro >>> ‘PPC_BITMASK’ >>> 77 | #define PC_TCTXT_CHIPID PPC_BITMASK(12, 15) >>> | ^~~~~~~~~~~ >>> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro >>> ‘PC_TCTXT_CHIPID’ >>> 80 | blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val); >>> | ^~~~~~~~~~~~~~~ >>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’: >>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from >>> ‘long long unsigned int’ to ‘long unsigned int’ changes value from >>> ‘13835058055282163712’ to ‘0’ [-Werror=overflow] >>> 45 | #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) >>> | PPC_BIT(bs)) >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of >>> macro ‘GETFIELD’ >>> 51 | (((word) & (mask)) >> __builtin_ctzl(mask)) >>> | ^~~~ >>> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro >>> ‘PPC_BITMASK’ >>> 230 | #define VSD_MODE PPC_BITMASK(0, 1) >>> | ^~~~~~~~~~~ >>> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’ >>> 226 | if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) { >>> | ^~~~~~~~ >>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’: >>> >>> >>> Link: >>> >>> https://gitlab.com/danielhb/qemu/-/jobs/2637716673 >>> >>> >>> I don´t know how to deal with that. >>> >>> >>> For the record: if this is too troublesome to fix, I am ok with just >>> consolidating >>> the GETFIELD and SETFIELD inlines we already have, under cpu.h, >>> keeping them exactly >>> as they are today (functions, not macros). >>> >>> >>> Thanks, >>> >>> >>> Daniel >>> >>> >>> >>> On 6/17/22 03:07, Alexey Kardashevskiy wrote: >>>> It keeps repeating, move it to the header. This uses >>>> __builtin_ctzl() to >>>> allow using the macros in #define. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> --- >>>> include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- >>>> target/ppc/cpu.h | 5 +++++ >>>> hw/intc/pnv_xive.c | 20 -------------------- >>>> hw/intc/pnv_xive2.c | 20 -------------------- >>>> hw/pci-host/pnv_phb4.c | 16 ---------------- >>>> 5 files changed, 5 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..9a1f1e9999a3 100644 >>>> --- a/target/ppc/cpu.h >>>> +++ b/target/ppc/cpu.h >>>> @@ -47,6 +47,11 @@ >>>> PPC_BIT32(bs)) >>>> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | >>>> PPC_BIT8(bs)) >>>> +#define GETFIELD(mask, word) \ >>>> + (((word) & (mask)) >> __builtin_ctzl(mask)) >> >> >> Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, >> do you have a quick way to test this? Gitlab's CI takes time :) >> https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run. >> Thanks, > > This worked for me as well. > > Can you re-send this patch with this fix plus the extra comment Peter > mentioned > in his review? > > ---- > Can we retain the explanatory comment that says why we don't > use the standard QEMU mechanism for field extraction > (ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64() > functions) ? > ------ > > > You can re-send this as v4 (I'm assuming that we're giving up trying to > copy > the Skiboot macros) and Sure, will do. > then I'll take the patch together with the watchdog > v3 implementation. The watchdog patch is not using these macros anymore, I moved to QEMU's macros, this is why separate patches. Thanks, > > > Thanks, > > Daniel > >> >>
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..9a1f1e9999a3 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -47,6 +47,11 @@ PPC_BIT32(bs)) #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) +#define GETFIELD(mask, word) \ + (((word) & (mask)) >> __builtin_ctzl(mask)) +#define SETFIELD(mask, word, val) \ + (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask))) + /*****************************************************************************/ /* 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 a39e070e82d2..3fe349749384 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 13ba9e45d8b6..0913e7c8f015 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);
It keeps repeating, move it to the header. This uses __builtin_ctzl() to allow using the macros in #define. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- include/hw/pci-host/pnv_phb3_regs.h | 16 ---------------- target/ppc/cpu.h | 5 +++++ hw/intc/pnv_xive.c | 20 -------------------- hw/intc/pnv_xive2.c | 20 -------------------- hw/pci-host/pnv_phb4.c | 16 ---------------- 5 files changed, 5 insertions(+), 72 deletions(-)