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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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

-- 
Alexey