qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alexey Kardashevskiy
Subject: Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target
Date: Tue, 28 Jun 2022 12:57:06 +1000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.0



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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]