| Message ID | 20181217223445.28594-3-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | ppc: support for the XIVE interrupt controller (POWER9) | expand |
On Mon, Dec 17, 2018 at 11:34:40PM +0100, Cédric Le Goater wrote: > And remove the intermediate MASK_TO_LSH macro which does not add any value. > > This fixes a compile breakage on windows. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> It's an improvement over what's there, but it still leaves macros whose primary use would be for guest registers, but are typed according to host values, which doesn't make much sense. I think instead we should redefine your BE64 / BE32 variants in terms of the existing extract*() and deposit*() primitives, and get rid of the GETFIELD / SETFIELD macros. > --- > target/ppc/cpu.h | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 527181c0f09f..f4ef4f214564 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -78,18 +78,21 @@ > PPC_BIT32(bs)) > #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) > > +/* > + * OPAL PPC bitmask field manipulation, used in XIVE, PHB3 and PHB4 > + */ > #if HOST_LONG_BITS == 32 > -# define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1) > +# define GETFIELD(m, v) (((v) & (m)) >> ctz32(m)) > +# define SETFIELD(m, v, val) \ > + (((v) & ~(m)) | ((((typeof(v))(val)) << ctz32(m)) & (m))) > #elif HOST_LONG_BITS == 64 > -# define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) > +# define GETFIELD(m, v) (((v) & (m)) >> ctz64(m)) > +# define SETFIELD(m, v, val) \ > + (((v) & ~(m)) | ((((typeof(v))(val)) << ctz64(m)) & (m))) > #else > # error Unknown sizeof long > #endif > > -#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 {
On 12/18/18 3:23 AM, David Gibson wrote: > On Mon, Dec 17, 2018 at 11:34:40PM +0100, Cédric Le Goater wrote: >> And remove the intermediate MASK_TO_LSH macro which does not add any value. >> >> This fixes a compile breakage on windows. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > It's an improvement over what's there, but it still leaves macros > whose primary use would be for guest registers, but are typed > according to host values, which doesn't make much sense. > > I think instead we should redefine your BE64 / BE32 variants in terms > of the existing extract*() and deposit*() primitives, and get rid of > the GETFIELD / SETFIELD macros. I will get rid of the GETFIELD/SETFIELD macros and rewrite the BE64/BE32 variants but I won't use the extract*() and deposit*(). I prefer to keep the same pattern, which is similar to shpc_get/set_status(). I will make the code clearer with static inlines. I don't really like the names also. what about xive_(get/set)_field(32/64) ? C. >> --- >> target/ppc/cpu.h | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h >> index 527181c0f09f..f4ef4f214564 100644 >> --- a/target/ppc/cpu.h >> +++ b/target/ppc/cpu.h >> @@ -78,18 +78,21 @@ >> PPC_BIT32(bs)) >> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) >> >> +/* >> + * OPAL PPC bitmask field manipulation, used in XIVE, PHB3 and PHB4 >> + */ >> #if HOST_LONG_BITS == 32 >> -# define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1) >> +# define GETFIELD(m, v) (((v) & (m)) >> ctz32(m)) >> +# define SETFIELD(m, v, val) \ >> + (((v) & ~(m)) | ((((typeof(v))(val)) << ctz32(m)) & (m))) >> #elif HOST_LONG_BITS == 64 >> -# define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) >> +# define GETFIELD(m, v) (((v) & (m)) >> ctz64(m)) >> +# define SETFIELD(m, v, val) \ >> + (((v) & ~(m)) | ((((typeof(v))(val)) << ctz64(m)) & (m))) >> #else >> # error Unknown sizeof long >> #endif >> >> -#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 { >
On Tue, Dec 18, 2018 at 09:07:47AM +0100, Cédric Le Goater wrote: > On 12/18/18 3:23 AM, David Gibson wrote: > > On Mon, Dec 17, 2018 at 11:34:40PM +0100, Cédric Le Goater wrote: > >> And remove the intermediate MASK_TO_LSH macro which does not add any value. > >> > >> This fixes a compile breakage on windows. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > > > It's an improvement over what's there, but it still leaves macros > > whose primary use would be for guest registers, but are typed > > according to host values, which doesn't make much sense. > > > > I think instead we should redefine your BE64 / BE32 variants in terms > > of the existing extract*() and deposit*() primitives, and get rid of > > the GETFIELD / SETFIELD macros. > > I will get rid of the GETFIELD/SETFIELD macros and rewrite the BE64/BE32 > variants but I won't use the extract*() and deposit*(). I prefer to keep > the same pattern, which is similar to shpc_get/set_status(). I will make > the code clearer with static inlines. That's fine. > I don't really like the names also. what about > xive_(get/set)_field(32/64) ? Sure, works for me. Since these will now be strictly fixed width, you can probably make them inlines instead of macros too, so we get type checking.
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 527181c0f09f..f4ef4f214564 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -78,18 +78,21 @@ PPC_BIT32(bs)) #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) +/* + * OPAL PPC bitmask field manipulation, used in XIVE, PHB3 and PHB4 + */ #if HOST_LONG_BITS == 32 -# define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1) +# define GETFIELD(m, v) (((v) & (m)) >> ctz32(m)) +# define SETFIELD(m, v, val) \ + (((v) & ~(m)) | ((((typeof(v))(val)) << ctz32(m)) & (m))) #elif HOST_LONG_BITS == 64 -# define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) +# define GETFIELD(m, v) (((v) & (m)) >> ctz64(m)) +# define SETFIELD(m, v, val) \ + (((v) & ~(m)) | ((((typeof(v))(val)) << ctz64(m)) & (m))) #else # error Unknown sizeof long #endif -#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 {
And remove the intermediate MASK_TO_LSH macro which does not add any value. This fixes a compile breakage on windows. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- target/ppc/cpu.h | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)