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

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

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