ppc/pnv: Introduce common PNV_SETFIELD() and PNV_GETFIELD() macros

Most of QEMU definitions of the register fields of the PowerNV machine
come from skiboot and the models duplicate a set of macros for this
purpose. Make them common under the pnv_utils.h file.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/pci-host/pnv_phb3_regs.h | 16 ------
 include/hw/ppc/pnv_utils.h          | 29 +++++++++++
 hw/intc/pnv_xive.c                  | 76 ++++++++++++-----------------
 hw/pci-host/pnv_phb3.c              | 32 ++++++------
 hw/pci-host/pnv_phb3_msi.c          | 24 ++++-----
 hw/pci-host/pnv_phb4.c              | 51 ++++++++-----------
 6 files changed, 108 insertions(+), 120 deletions(-)
 create mode 100644 include/hw/ppc/pnv_utils.h

On Wed,  1 Apr 2020 17:26:33 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> Most of QEMU definitions of the register fields of the PowerNV machine
> come from skiboot and the models duplicate a set of macros for this
> purpose. Make them common under the pnv_utils.h file.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  include/hw/pci-host/pnv_phb3_regs.h | 16 ------
>  include/hw/ppc/pnv_utils.h          | 29 +++++++++++
>  hw/intc/pnv_xive.c                  | 76 ++++++++++++-----------------
>  hw/pci-host/pnv_phb3.c              | 32 ++++++------
>  hw/pci-host/pnv_phb3_msi.c          | 24 ++++-----
>  hw/pci-host/pnv_phb4.c              | 51 ++++++++-----------
>  6 files changed, 108 insertions(+), 120 deletions(-)
>  create mode 100644 include/hw/ppc/pnv_utils.h
> 
> 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/include/hw/ppc/pnv_utils.h b/include/hw/ppc/pnv_utils.h
> new file mode 100644
> index 000000000000..8521e13b5149
> --- /dev/null
> +++ b/include/hw/ppc/pnv_utils.h
> @@ -0,0 +1,29 @@
> +/*
> + * QEMU PowerPC PowerNV utilities
> + *
> + * Copyright (c) 2020, IBM Corporation.
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#ifndef PPC_PNV_UTILS_H
> +#define PPC_PNV_UTILS_H
> +
> +/*
> + * QEMU version of the GETFIELD/SETFIELD macros used in skiboot to
> + * define the register fields.
> + */
> +
> +static inline uint64_t PNV_GETFIELD(uint64_t mask, uint64_t word)
> +{
> +    return (word & mask) >> ctz64(mask);
> +}
> +
> +static inline uint64_t PNV_SETFIELD(uint64_t mask, uint64_t word,
> +                                    uint64_t value)
> +{
> +    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> +}
> +
> +#endif /* PPC_PNV_UTILS_H */
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index aeda488bd113..77cacdd6c623 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -21,6 +21,7 @@
>  #include "hw/ppc/pnv_core.h"
>  #include "hw/ppc/pnv_xscom.h"
>  #include "hw/ppc/pnv_xive.h"
> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>  #include "hw/ppc/xive_regs.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/ppc/ppc.h"
> @@ -65,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
> @@ -96,7 +77,7 @@ static uint8_t pnv_xive_block_id(PnvXive *xive)
>      uint64_t cfg_val = xive->regs[PC_TCTXT_CFG >> 3];
>  
>      if (cfg_val & PC_TCTXT_CHIPID_OVERRIDE) {
> -        blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
> +        blk = PNV_GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>      }
>  
>      return blk;
> @@ -145,7 +126,7 @@ static uint64_t pnv_xive_vst_addr_direct(PnvXive *xive, uint32_t type,
>  {
>      const XiveVstInfo *info = &vst_infos[type];
>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
> -    uint64_t vst_tsize = 1ull << (GETFIELD(VSD_TSIZE, vsd) + 12);
> +    uint64_t vst_tsize = 1ull << (PNV_GETFIELD(VSD_TSIZE, vsd) + 12);
>      uint32_t idx_max;
>  
>      idx_max = vst_tsize / info->size - 1;
> @@ -180,7 +161,7 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>          return 0;
>      }
>  
> -    page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
> +    page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>  
>      if (!pnv_xive_vst_page_size_allowed(page_shift)) {
>          xive_error(xive, "VST: invalid %s page shift %d", info->name,
> @@ -207,7 +188,7 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>           * Check that the pages have a consistent size across the
>           * indirect table
>           */
> -        if (page_shift != GETFIELD(VSD_TSIZE, vsd) + 12) {
> +        if (page_shift != PNV_GETFIELD(VSD_TSIZE, vsd) + 12) {
>              xive_error(xive, "VST: %s entry %x indirect page size differ !?",
>                         info->name, idx);
>              return 0;
> @@ -232,7 +213,7 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>      vsd = xive->vsds[type][blk];
>  
>      /* Remote VST access */
> -    if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
> +    if (PNV_GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>          xive = pnv_xive_get_remote(blk);
>  
>          return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
> @@ -295,9 +276,9 @@ static int pnv_xive_write_end(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>  
>  static int pnv_xive_end_update(PnvXive *xive)
>  {
> -    uint8_t  blk = GETFIELD(VC_EQC_CWATCH_BLOCKID,
> +    uint8_t  blk = PNV_GETFIELD(VC_EQC_CWATCH_BLOCKID,
>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
> -    uint32_t idx = GETFIELD(VC_EQC_CWATCH_OFFSET,
> +    uint32_t idx = PNV_GETFIELD(VC_EQC_CWATCH_OFFSET,
>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
>      int i;
>      uint64_t eqc_watch[4];
> @@ -312,9 +293,9 @@ static int pnv_xive_end_update(PnvXive *xive)
>  
>  static void pnv_xive_end_cache_load(PnvXive *xive)
>  {
> -    uint8_t  blk = GETFIELD(VC_EQC_CWATCH_BLOCKID,
> +    uint8_t  blk = PNV_GETFIELD(VC_EQC_CWATCH_BLOCKID,
>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
> -    uint32_t idx = GETFIELD(VC_EQC_CWATCH_OFFSET,
> +    uint32_t idx = PNV_GETFIELD(VC_EQC_CWATCH_OFFSET,
>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
>      uint64_t eqc_watch[4] = { 0 };
>      int i;
> @@ -343,9 +324,9 @@ static int pnv_xive_write_nvt(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>  
>  static int pnv_xive_nvt_update(PnvXive *xive)
>  {
> -    uint8_t  blk = GETFIELD(PC_VPC_CWATCH_BLOCKID,
> +    uint8_t  blk = PNV_GETFIELD(PC_VPC_CWATCH_BLOCKID,
>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
> -    uint32_t idx = GETFIELD(PC_VPC_CWATCH_OFFSET,
> +    uint32_t idx = PNV_GETFIELD(PC_VPC_CWATCH_OFFSET,
>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
>      int i;
>      uint64_t vpc_watch[8];
> @@ -360,9 +341,9 @@ static int pnv_xive_nvt_update(PnvXive *xive)
>  
>  static void pnv_xive_nvt_cache_load(PnvXive *xive)
>  {
> -    uint8_t  blk = GETFIELD(PC_VPC_CWATCH_BLOCKID,
> +    uint8_t  blk = PNV_GETFIELD(PC_VPC_CWATCH_BLOCKID,
>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
> -    uint32_t idx = GETFIELD(PC_VPC_CWATCH_OFFSET,
> +    uint32_t idx = PNV_GETFIELD(PC_VPC_CWATCH_OFFSET,
>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
>      uint64_t vpc_watch[8] = { 0 };
>      int i;
> @@ -518,7 +499,7 @@ static uint64_t pnv_xive_pc_size(PnvXive *xive)
>  static uint32_t pnv_xive_nr_ipis(PnvXive *xive, uint8_t blk)
>  {
>      uint64_t vsd = xive->vsds[VST_TSEL_SBE][blk];
> -    uint64_t vst_tsize = 1ull << (GETFIELD(VSD_TSIZE, vsd) + 12);
> +    uint64_t vst_tsize = 1ull << (PNV_GETFIELD(VSD_TSIZE, vsd) + 12);
>  
>      return VSD_INDIRECT & vsd ? 0 : vst_tsize * SBE_PER_BYTE;
>  }
> @@ -550,7 +531,7 @@ static uint64_t pnv_xive_vst_per_subpage(PnvXive *xive, uint32_t type)
>          return 0;
>      }
>  
> -    page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
> +    page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>  
>      if (!pnv_xive_vst_page_size_allowed(page_shift)) {
>          xive_error(xive, "VST: invalid %s page shift %d", info->name,
> @@ -582,7 +563,7 @@ static uint64_t pnv_xive_edt_size(PnvXive *xive, uint64_t type)
>      int i;
>  
>      for (i = 0; i < XIVE_TABLE_EDT_MAX; i++) {
> -        uint64_t edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
> +        uint64_t edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
>  
>          if (edt_type == type) {
>              size += edt_size;
> @@ -604,7 +585,7 @@ static uint64_t pnv_xive_edt_offset(PnvXive *xive, uint64_t vc_offset,
>      uint64_t edt_offset = vc_offset;
>  
>      for (i = 0; i < XIVE_TABLE_EDT_MAX && (i * edt_size) < vc_offset; i++) {
> -        uint64_t edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
> +        uint64_t edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
>  
>          if (edt_type != type) {
>              edt_offset -= edt_size;
> @@ -632,7 +613,8 @@ static void pnv_xive_edt_resize(PnvXive *xive)
>  static int pnv_xive_table_set_data(PnvXive *xive, uint64_t val)
>  {
>      uint64_t tsel = xive->regs[CQ_TAR >> 3] & CQ_TAR_TSEL;
> -    uint8_t tsel_index = GETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3]);
> +    uint8_t tsel_index = PNV_GETFIELD(CQ_TAR_TSEL_INDEX,
> +                                      xive->regs[CQ_TAR >> 3]);
>      uint64_t *xive_table;
>      uint8_t max_index;
>  
> @@ -667,7 +649,8 @@ static int pnv_xive_table_set_data(PnvXive *xive, uint64_t val)
>  
>      if (xive->regs[CQ_TAR >> 3] & CQ_TAR_TBL_AUTOINC) {
>          xive->regs[CQ_TAR >> 3] =
> -            SETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3], ++tsel_index);
> +            PNV_SETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3],
> +                         ++tsel_index);
>      }
>  
>      /*
> @@ -690,7 +673,7 @@ static void pnv_xive_vst_set_exclusive(PnvXive *xive, uint8_t type,
>      XiveENDSource *end_xsrc = &xive->end_source;
>      XiveSource *xsrc = &xive->ipi_source;
>      const XiveVstInfo *info = &vst_infos[type];
> -    uint32_t page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
> +    uint32_t page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>      uint64_t vst_tsize = 1ull << page_shift;
>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
>  
> @@ -777,10 +760,10 @@ static void pnv_xive_vst_set_exclusive(PnvXive *xive, uint8_t type,
>   */
>  static void pnv_xive_vst_set_data(PnvXive *xive, uint64_t vsd, bool pc_engine)
>  {
> -    uint8_t mode = GETFIELD(VSD_MODE, vsd);
> -    uint8_t type = GETFIELD(VST_TABLE_SELECT,
> +    uint8_t mode = PNV_GETFIELD(VSD_MODE, vsd);
> +    uint8_t type = PNV_GETFIELD(VST_TABLE_SELECT,
>                              xive->regs[VC_VSD_TABLE_ADDR >> 3]);
> -    uint8_t blk = GETFIELD(VST_TABLE_BLOCK,
> +    uint8_t blk = PNV_GETFIELD(VST_TABLE_BLOCK,
>                             xive->regs[VC_VSD_TABLE_ADDR >> 3]);
>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
>  
> @@ -1456,7 +1439,8 @@ static XiveTCTX *pnv_xive_get_indirect_tctx(PnvXive *xive)
>          return NULL;
>      }
>  
> -    pir = (chip->chip_id << 8) | GETFIELD(PC_TCTXT_INDIR_THRDID, tctxt_indir);
> +    pir = (chip->chip_id << 8) |
> +        PNV_GETFIELD(PC_TCTXT_INDIR_THRDID, tctxt_indir);
>      cpu = pnv_chip_find_cpu(chip, pir);
>      if (!cpu) {
>          xive_error(xive, "IC: invalid PIR %x for indirect access", pir);
> @@ -1583,7 +1567,7 @@ static uint64_t pnv_xive_vc_read(void *opaque, hwaddr offset,
>      uint64_t ret = -1;
>  
>      if (edt_index < XIVE_TABLE_EDT_MAX) {
> -        edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
> +        edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
>      }
>  
>      switch (edt_type) {
> @@ -1625,7 +1609,7 @@ static void pnv_xive_vc_write(void *opaque, hwaddr offset,
>      AddressSpace *edt_as = NULL;
>  
>      if (edt_index < XIVE_TABLE_EDT_MAX) {
> -        edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
> +        edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
>      }
>  
>      switch (edt_type) {
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 74618fadf085..a2d5815d3485 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -16,6 +16,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pcie_port.h"
>  #include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  
> @@ -171,11 +172,11 @@ static void pnv_phb3_check_m64(PnvPHB3 *phb, uint32_t index)
>      }
>  
>      /* Grab geometry from registers */
> -    base = GETFIELD(IODA2_M64BT_BASE, m64) << 20;
> +    base = PNV_GETFIELD(IODA2_M64BT_BASE, m64) << 20;
>      if (m64 & IODA2_M64BT_SINGLE_PE) {
>          base &= ~0x1ffffffull;
>      }
> -    size = GETFIELD(IODA2_M64BT_MASK, m64) << 20;
> +    size = PNV_GETFIELD(IODA2_M64BT_MASK, m64) << 20;
>      size |= 0xfffc000000000000ull;
>      size = ~size + 1;
>      start = base | (phb->regs[PHB_M64_UPPER_BITS >> 3]);
> @@ -217,8 +218,8 @@ static void pnv_phb3_lxivt_write(PnvPHB3 *phb, unsigned idx, uint64_t val)
>      phb->ioda_LXIVT[idx] = val & (IODA2_LXIVT_SERVER |
>                                    IODA2_LXIVT_PRIORITY |
>                                    IODA2_LXIVT_NODE_ID);
> -    server = GETFIELD(IODA2_LXIVT_SERVER, val);
> -    prio = GETFIELD(IODA2_LXIVT_PRIORITY, val);
> +    server = PNV_GETFIELD(IODA2_LXIVT_SERVER, val);
> +    prio = PNV_GETFIELD(IODA2_LXIVT_PRIORITY, val);
>  
>      /*
>       * The low order 2 bits are the link pointer (Type II interrupts).
> @@ -233,8 +234,8 @@ static uint64_t *pnv_phb3_ioda_access(PnvPHB3 *phb,
>                                        unsigned *out_table, unsigned *out_idx)
>  {
>      uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
> -    unsigned int index = GETFIELD(PHB_IODA_AD_TADR, adreg);
> -    unsigned int table = GETFIELD(PHB_IODA_AD_TSEL, adreg);
> +    unsigned int index = PNV_GETFIELD(PHB_IODA_AD_TADR, adreg);
> +    unsigned int table = PNV_GETFIELD(PHB_IODA_AD_TSEL, adreg);
>      unsigned int mask;
>      uint64_t *tptr = NULL;
>  
> @@ -297,7 +298,7 @@ static uint64_t *pnv_phb3_ioda_access(PnvPHB3 *phb,
>      }
>      if (adreg & PHB_IODA_AD_AUTOINC) {
>          index = (index + 1) & mask;
> -        adreg = SETFIELD(PHB_IODA_AD_TADR, adreg, index);
> +        adreg = PNV_SETFIELD(PHB_IODA_AD_TADR, adreg, index);
>      }
>      phb->regs[PHB_IODA_ADDR >> 3] = adreg;
>      return tptr;
> @@ -363,10 +364,11 @@ void pnv_phb3_remap_irqs(PnvPHB3 *phb)
>      }
>  
>      /* Grab local LSI source ID */
> -    local = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]) << 3;
> +    local = PNV_GETFIELD(PHB_LSI_SRC_ID,
> +                         phb->regs[PHB_LSI_SOURCE_ID >> 3]) << 3;
>  
>      /* Grab global one and compare */
> -    global = GETFIELD(PBCQ_NEST_LSI_SRC,
> +    global = PNV_GETFIELD(PBCQ_NEST_LSI_SRC,
>                        pbcq->nest_regs[PBCQ_NEST_LSI_SRC_ID]) << 3;
>      if (global != local) {
>          /*
> @@ -378,9 +380,9 @@ void pnv_phb3_remap_irqs(PnvPHB3 *phb)
>      }
>  
>      /* Get the base on the powerbus */
> -    comp = GETFIELD(PBCQ_NEST_IRSN_COMP,
> +    comp = PNV_GETFIELD(PBCQ_NEST_IRSN_COMP,
>                      pbcq->nest_regs[PBCQ_NEST_IRSN_COMPARE]);
> -    mask = GETFIELD(PBCQ_NEST_IRSN_COMP,
> +    mask = PNV_GETFIELD(PBCQ_NEST_IRSN_COMP,
>                      pbcq->nest_regs[PBCQ_NEST_IRSN_MASK]);
>      count = ((~mask) + 1) & 0x7ffff;
>      phb->total_irq = count;
> @@ -735,10 +737,10 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr,
>                                     bool is_write, uint64_t tve,
>                                     IOMMUTLBEntry *tlb)
>  {
> -    uint64_t tta = GETFIELD(IODA2_TVT_TABLE_ADDR, tve);
> -    int32_t  lev = GETFIELD(IODA2_TVT_NUM_LEVELS, tve);
> -    uint32_t tts = GETFIELD(IODA2_TVT_TCE_TABLE_SIZE, tve);
> -    uint32_t tps = GETFIELD(IODA2_TVT_IO_PSIZE, tve);
> +    uint64_t tta = PNV_GETFIELD(IODA2_TVT_TABLE_ADDR, tve);
> +    int32_t  lev = PNV_GETFIELD(IODA2_TVT_NUM_LEVELS, tve);
> +    uint32_t tts = PNV_GETFIELD(IODA2_TVT_TCE_TABLE_SIZE, tve);
> +    uint32_t tps = PNV_GETFIELD(IODA2_TVT_IO_PSIZE, tve);
>      PnvPHB3 *phb = ds->phb;
>  
>      /* Invalid levels */
> diff --git a/hw/pci-host/pnv_phb3_msi.c b/hw/pci-host/pnv_phb3_msi.c
> index d645468f4a6f..f283d5cd7dc1 100644
> --- a/hw/pci-host/pnv_phb3_msi.c
> +++ b/hw/pci-host/pnv_phb3_msi.c
> @@ -13,6 +13,7 @@
>  #include "hw/pci-host/pnv_phb3_regs.h"
>  #include "hw/pci-host/pnv_phb3.h"
>  #include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>  #include "hw/pci/msi.h"
>  #include "monitor/monitor.h"
>  #include "hw/irq.h"
> @@ -105,14 +106,15 @@ static void phb3_msi_try_send(Phb3MsiState *msi, int srcno, bool force)
>          return;
>      }
>  
> -    server = GETFIELD(IODA2_IVT_SERVER, ive);
> -    prio = GETFIELD(IODA2_IVT_PRIORITY, ive);
> +    server = PNV_GETFIELD(IODA2_IVT_SERVER, ive);
> +    prio = PNV_GETFIELD(IODA2_IVT_PRIORITY, ive);
>      if (!force) {
> -        pq = GETFIELD(IODA2_IVT_Q, ive) | (GETFIELD(IODA2_IVT_P, ive) << 1);
> +        pq = PNV_GETFIELD(IODA2_IVT_Q, ive) |
> +            (PNV_GETFIELD(IODA2_IVT_P, ive) << 1);
>      } else {
>          pq = 0;
>      }
> -    gen = GETFIELD(IODA2_IVT_GEN, ive);
> +    gen = PNV_GETFIELD(IODA2_IVT_GEN, ive);
>  
>      /*
>       * The low order 2 bits are the link pointer (Type II interrupts).
> @@ -169,7 +171,7 @@ void pnv_phb3_msi_send(Phb3MsiState *msi, uint64_t addr, uint16_t data,
>          if (!phb3_msi_read_ive(msi->phb, src, &ive)) {
>              return;
>          }
> -        pe = GETFIELD(IODA2_IVT_PE, ive);
> +        pe = PNV_GETFIELD(IODA2_IVT_PE, ive);
>          if (pe != dev_pe) {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "MSI %d send by PE#%d but assigned to PE#%d",
> @@ -334,16 +336,16 @@ void pnv_phb3_msi_pic_print_info(Phb3MsiState *msi, Monitor *mon)
>              return;
>          }
>  
> -        if (GETFIELD(IODA2_IVT_PRIORITY, ive) == 0xff) {
> +        if (PNV_GETFIELD(IODA2_IVT_PRIORITY, ive) == 0xff) {
>              continue;
>          }
>  
>          monitor_printf(mon, "  %4x %c%c server=%04x prio=%02x gen=%d\n",
>                         ics->offset + i,
> -                       GETFIELD(IODA2_IVT_P, ive) ? 'P' : '-',
> -                       GETFIELD(IODA2_IVT_Q, ive) ? 'Q' : '-',
> -                       (uint32_t) GETFIELD(IODA2_IVT_SERVER, ive) >> 2,
> -                       (uint32_t) GETFIELD(IODA2_IVT_PRIORITY, ive),
> -                       (uint32_t) GETFIELD(IODA2_IVT_GEN, ive));
> +                       PNV_GETFIELD(IODA2_IVT_P, ive) ? 'P' : '-',
> +                       PNV_GETFIELD(IODA2_IVT_Q, ive) ? 'Q' : '-',
> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_SERVER, ive) >> 2,
> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_PRIORITY, ive),
> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_GEN, ive));
>      }
>  }
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 23cf093928ed..ac824f877cf1 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -19,6 +19,7 @@
>  #include "hw/pci/pcie_port.h"
>  #include "hw/ppc/pnv.h"
>  #include "hw/ppc/pnv_xscom.h"
> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  
> @@ -26,22 +27,6 @@
>      qemu_log_mask(LOG_GUEST_ERROR, "phb4[%d:%d]: " fmt "\n",            \
>                    (phb)->chip_id, (phb)->phb_id, ## __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);
> @@ -196,8 +181,8 @@ static void pnv_phb4_check_mbt(PnvPHB4 *phb, uint32_t index)
>      }
>  
>      /* Grab geometry from registers */
> -    base = GETFIELD(IODA3_MBT0_BASE_ADDR, mbe0) << 12;
> -    size = GETFIELD(IODA3_MBT1_MASK, mbe1) << 12;
> +    base = PNV_GETFIELD(IODA3_MBT0_BASE_ADDR, mbe0) << 12;
> +    size = PNV_GETFIELD(IODA3_MBT1_MASK, mbe1) << 12;
>      size |= 0xff00000000000000ull;
>      size = ~size + 1;
>  
> @@ -252,8 +237,8 @@ static uint64_t *pnv_phb4_ioda_access(PnvPHB4 *phb,
>                                        unsigned *out_table, unsigned *out_idx)
>  {
>      uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
> -    unsigned int index = GETFIELD(PHB_IODA_AD_TADR, adreg);
> -    unsigned int table = GETFIELD(PHB_IODA_AD_TSEL, adreg);
> +    unsigned int index = PNV_GETFIELD(PHB_IODA_AD_TADR, adreg);
> +    unsigned int table = PNV_GETFIELD(PHB_IODA_AD_TSEL, adreg);
>      unsigned int mask;
>      uint64_t *tptr = NULL;
>  
> @@ -318,7 +303,7 @@ static uint64_t *pnv_phb4_ioda_access(PnvPHB4 *phb,
>      }
>      if (adreg & PHB_IODA_AD_AUTOINC) {
>          index = (index + 1) & mask;
> -        adreg = SETFIELD(PHB_IODA_AD_TADR, adreg, index);
> +        adreg = PNV_SETFIELD(PHB_IODA_AD_TADR, adreg, index);
>      }
>  
>      phb->regs[PHB_IODA_ADDR >> 3] = adreg;
> @@ -369,7 +354,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb, uint64_t val)
>      case IODA3_TBL_MIST: {
>          /* Special mask for MIST partial write */
>          uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
> -        uint32_t mmask = GETFIELD(PHB_IODA_AD_MIST_PWV, adreg);
> +        uint32_t mmask = PNV_GETFIELD(PHB_IODA_AD_MIST_PWV, adreg);
>          uint64_t v = *tptr;
>          if (mmask == 0) {
>              mmask = 0xf;
> @@ -476,7 +461,7 @@ static void pnv_phb4_update_xsrc(PnvPHB4 *phb)
>      phb->xsrc.esb_shift = shift;
>      phb->xsrc.esb_flags = flags;
>  
> -    lsi_base = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
> +    lsi_base = PNV_GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
>      lsi_base <<= 3;
>  
>      /* TODO: handle reset values of PHB_LSI_SRC_ID */
> @@ -747,12 +732,13 @@ static uint64_t pnv_phb4_xscom_read(void *opaque, hwaddr addr, unsigned size)
>              return ~0ull;
>          }
>          size = (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_4B) ? 4 : 8;
> -        offset = GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR, phb->scom_hv_ind_addr_reg);
> +        offset = PNV_GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
> +                              phb->scom_hv_ind_addr_reg);
>          val = pnv_phb4_reg_read(phb, offset, size);
>          if (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_AUTOINC) {
>              offset += size;
>              offset &= 0x3fff;
> -            phb->scom_hv_ind_addr_reg = SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
> +            phb->scom_hv_ind_addr_reg = PNV_SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>                                                   phb->scom_hv_ind_addr_reg,
>                                                   offset);
>          }
> @@ -799,12 +785,13 @@ static void pnv_phb4_xscom_write(void *opaque, hwaddr addr,
>              break;
>          }
>          size = (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_4B) ? 4 : 8;
> -        offset = GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR, phb->scom_hv_ind_addr_reg);
> +        offset = PNV_GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
> +                              phb->scom_hv_ind_addr_reg);
>          pnv_phb4_reg_write(phb, offset, val, size);
>          if (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_AUTOINC) {
>              offset += size;
>              offset &= 0x3fff;
> -            phb->scom_hv_ind_addr_reg = SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
> +            phb->scom_hv_ind_addr_reg = PNV_SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>                                                   phb->scom_hv_ind_addr_reg,
>                                                   offset);
>          }
> @@ -860,7 +847,7 @@ static void pnv_phb4_set_irq(void *opaque, int irq_num, int level)
>      if (irq_num > 3) {
>          phb_error(phb, "IRQ %x is not an LSI", irq_num);
>      }
> -    lsi_base = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
> +    lsi_base = PNV_GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
>      lsi_base <<= 3;
>      qemu_set_irq(phb->qirqs[lsi_base + irq_num], level);
>  }
> @@ -910,10 +897,10 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr,
>                                     bool is_write, uint64_t tve,
>                                     IOMMUTLBEntry *tlb)
>  {
> -    uint64_t tta = GETFIELD(IODA3_TVT_TABLE_ADDR, tve);
> -    int32_t  lev = GETFIELD(IODA3_TVT_NUM_LEVELS, tve);
> -    uint32_t tts = GETFIELD(IODA3_TVT_TCE_TABLE_SIZE, tve);
> -    uint32_t tps = GETFIELD(IODA3_TVT_IO_PSIZE, tve);
> +    uint64_t tta = PNV_GETFIELD(IODA3_TVT_TABLE_ADDR, tve);
> +    int32_t  lev = PNV_GETFIELD(IODA3_TVT_NUM_LEVELS, tve);
> +    uint32_t tts = PNV_GETFIELD(IODA3_TVT_TCE_TABLE_SIZE, tve);
> +    uint32_t tps = PNV_GETFIELD(IODA3_TVT_IO_PSIZE, tve);
>  
>      /* Invalid levels */
>      if (lev > 4) {

On Wed, Apr 01, 2020 at 05:26:33PM +0200, Cédric Le Goater wrote:
> Most of QEMU definitions of the register fields of the PowerNV machine
> come from skiboot and the models duplicate a set of macros for this
> purpose. Make them common under the pnv_utils.h file.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Hrm.  If we're touching these, would it make sense to rewrite them in
terms of the cross-qemu generic extract64() and deposit64()?

> ---
>  include/hw/pci-host/pnv_phb3_regs.h | 16 ------
>  include/hw/ppc/pnv_utils.h          | 29 +++++++++++
>  hw/intc/pnv_xive.c                  | 76 ++++++++++++-----------------
>  hw/pci-host/pnv_phb3.c              | 32 ++++++------
>  hw/pci-host/pnv_phb3_msi.c          | 24 ++++-----
>  hw/pci-host/pnv_phb4.c              | 51 ++++++++-----------
>  6 files changed, 108 insertions(+), 120 deletions(-)
>  create mode 100644 include/hw/ppc/pnv_utils.h
> 
> 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/include/hw/ppc/pnv_utils.h b/include/hw/ppc/pnv_utils.h
> new file mode 100644
> index 000000000000..8521e13b5149
> --- /dev/null
> +++ b/include/hw/ppc/pnv_utils.h
> @@ -0,0 +1,29 @@
> +/*
> + * QEMU PowerPC PowerNV utilities
> + *
> + * Copyright (c) 2020, IBM Corporation.
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#ifndef PPC_PNV_UTILS_H
> +#define PPC_PNV_UTILS_H
> +
> +/*
> + * QEMU version of the GETFIELD/SETFIELD macros used in skiboot to
> + * define the register fields.
> + */
> +
> +static inline uint64_t PNV_GETFIELD(uint64_t mask, uint64_t word)
> +{
> +    return (word & mask) >> ctz64(mask);
> +}
> +
> +static inline uint64_t PNV_SETFIELD(uint64_t mask, uint64_t word,
> +                                    uint64_t value)
> +{
> +    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> +}
> +
> +#endif /* PPC_PNV_UTILS_H */
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index aeda488bd113..77cacdd6c623 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -21,6 +21,7 @@
>  #include "hw/ppc/pnv_core.h"
>  #include "hw/ppc/pnv_xscom.h"
>  #include "hw/ppc/pnv_xive.h"
> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>  #include "hw/ppc/xive_regs.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/ppc/ppc.h"
> @@ -65,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
> @@ -96,7 +77,7 @@ static uint8_t pnv_xive_block_id(PnvXive *xive)
>      uint64_t cfg_val = xive->regs[PC_TCTXT_CFG >> 3];
>  
>      if (cfg_val & PC_TCTXT_CHIPID_OVERRIDE) {
> -        blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
> +        blk = PNV_GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>      }
>  
>      return blk;
> @@ -145,7 +126,7 @@ static uint64_t pnv_xive_vst_addr_direct(PnvXive *xive, uint32_t type,
>  {
>      const XiveVstInfo *info = &vst_infos[type];
>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
> -    uint64_t vst_tsize = 1ull << (GETFIELD(VSD_TSIZE, vsd) + 12);
> +    uint64_t vst_tsize = 1ull << (PNV_GETFIELD(VSD_TSIZE, vsd) + 12);
>      uint32_t idx_max;
>  
>      idx_max = vst_tsize / info->size - 1;
> @@ -180,7 +161,7 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>          return 0;
>      }
>  
> -    page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
> +    page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>  
>      if (!pnv_xive_vst_page_size_allowed(page_shift)) {
>          xive_error(xive, "VST: invalid %s page shift %d", info->name,
> @@ -207,7 +188,7 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>           * Check that the pages have a consistent size across the
>           * indirect table
>           */
> -        if (page_shift != GETFIELD(VSD_TSIZE, vsd) + 12) {
> +        if (page_shift != PNV_GETFIELD(VSD_TSIZE, vsd) + 12) {
>              xive_error(xive, "VST: %s entry %x indirect page size differ !?",
>                         info->name, idx);
>              return 0;
> @@ -232,7 +213,7 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>      vsd = xive->vsds[type][blk];
>  
>      /* Remote VST access */
> -    if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
> +    if (PNV_GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>          xive = pnv_xive_get_remote(blk);
>  
>          return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
> @@ -295,9 +276,9 @@ static int pnv_xive_write_end(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>  
>  static int pnv_xive_end_update(PnvXive *xive)
>  {
> -    uint8_t  blk = GETFIELD(VC_EQC_CWATCH_BLOCKID,
> +    uint8_t  blk = PNV_GETFIELD(VC_EQC_CWATCH_BLOCKID,
>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
> -    uint32_t idx = GETFIELD(VC_EQC_CWATCH_OFFSET,
> +    uint32_t idx = PNV_GETFIELD(VC_EQC_CWATCH_OFFSET,
>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
>      int i;
>      uint64_t eqc_watch[4];
> @@ -312,9 +293,9 @@ static int pnv_xive_end_update(PnvXive *xive)
>  
>  static void pnv_xive_end_cache_load(PnvXive *xive)
>  {
> -    uint8_t  blk = GETFIELD(VC_EQC_CWATCH_BLOCKID,
> +    uint8_t  blk = PNV_GETFIELD(VC_EQC_CWATCH_BLOCKID,
>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
> -    uint32_t idx = GETFIELD(VC_EQC_CWATCH_OFFSET,
> +    uint32_t idx = PNV_GETFIELD(VC_EQC_CWATCH_OFFSET,
>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
>      uint64_t eqc_watch[4] = { 0 };
>      int i;
> @@ -343,9 +324,9 @@ static int pnv_xive_write_nvt(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>  
>  static int pnv_xive_nvt_update(PnvXive *xive)
>  {
> -    uint8_t  blk = GETFIELD(PC_VPC_CWATCH_BLOCKID,
> +    uint8_t  blk = PNV_GETFIELD(PC_VPC_CWATCH_BLOCKID,
>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
> -    uint32_t idx = GETFIELD(PC_VPC_CWATCH_OFFSET,
> +    uint32_t idx = PNV_GETFIELD(PC_VPC_CWATCH_OFFSET,
>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
>      int i;
>      uint64_t vpc_watch[8];
> @@ -360,9 +341,9 @@ static int pnv_xive_nvt_update(PnvXive *xive)
>  
>  static void pnv_xive_nvt_cache_load(PnvXive *xive)
>  {
> -    uint8_t  blk = GETFIELD(PC_VPC_CWATCH_BLOCKID,
> +    uint8_t  blk = PNV_GETFIELD(PC_VPC_CWATCH_BLOCKID,
>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
> -    uint32_t idx = GETFIELD(PC_VPC_CWATCH_OFFSET,
> +    uint32_t idx = PNV_GETFIELD(PC_VPC_CWATCH_OFFSET,
>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
>      uint64_t vpc_watch[8] = { 0 };
>      int i;
> @@ -518,7 +499,7 @@ static uint64_t pnv_xive_pc_size(PnvXive *xive)
>  static uint32_t pnv_xive_nr_ipis(PnvXive *xive, uint8_t blk)
>  {
>      uint64_t vsd = xive->vsds[VST_TSEL_SBE][blk];
> -    uint64_t vst_tsize = 1ull << (GETFIELD(VSD_TSIZE, vsd) + 12);
> +    uint64_t vst_tsize = 1ull << (PNV_GETFIELD(VSD_TSIZE, vsd) + 12);
>  
>      return VSD_INDIRECT & vsd ? 0 : vst_tsize * SBE_PER_BYTE;
>  }
> @@ -550,7 +531,7 @@ static uint64_t pnv_xive_vst_per_subpage(PnvXive *xive, uint32_t type)
>          return 0;
>      }
>  
> -    page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
> +    page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>  
>      if (!pnv_xive_vst_page_size_allowed(page_shift)) {
>          xive_error(xive, "VST: invalid %s page shift %d", info->name,
> @@ -582,7 +563,7 @@ static uint64_t pnv_xive_edt_size(PnvXive *xive, uint64_t type)
>      int i;
>  
>      for (i = 0; i < XIVE_TABLE_EDT_MAX; i++) {
> -        uint64_t edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
> +        uint64_t edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
>  
>          if (edt_type == type) {
>              size += edt_size;
> @@ -604,7 +585,7 @@ static uint64_t pnv_xive_edt_offset(PnvXive *xive, uint64_t vc_offset,
>      uint64_t edt_offset = vc_offset;
>  
>      for (i = 0; i < XIVE_TABLE_EDT_MAX && (i * edt_size) < vc_offset; i++) {
> -        uint64_t edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
> +        uint64_t edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
>  
>          if (edt_type != type) {
>              edt_offset -= edt_size;
> @@ -632,7 +613,8 @@ static void pnv_xive_edt_resize(PnvXive *xive)
>  static int pnv_xive_table_set_data(PnvXive *xive, uint64_t val)
>  {
>      uint64_t tsel = xive->regs[CQ_TAR >> 3] & CQ_TAR_TSEL;
> -    uint8_t tsel_index = GETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3]);
> +    uint8_t tsel_index = PNV_GETFIELD(CQ_TAR_TSEL_INDEX,
> +                                      xive->regs[CQ_TAR >> 3]);
>      uint64_t *xive_table;
>      uint8_t max_index;
>  
> @@ -667,7 +649,8 @@ static int pnv_xive_table_set_data(PnvXive *xive, uint64_t val)
>  
>      if (xive->regs[CQ_TAR >> 3] & CQ_TAR_TBL_AUTOINC) {
>          xive->regs[CQ_TAR >> 3] =
> -            SETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3], ++tsel_index);
> +            PNV_SETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3],
> +                         ++tsel_index);
>      }
>  
>      /*
> @@ -690,7 +673,7 @@ static void pnv_xive_vst_set_exclusive(PnvXive *xive, uint8_t type,
>      XiveENDSource *end_xsrc = &xive->end_source;
>      XiveSource *xsrc = &xive->ipi_source;
>      const XiveVstInfo *info = &vst_infos[type];
> -    uint32_t page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
> +    uint32_t page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>      uint64_t vst_tsize = 1ull << page_shift;
>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
>  
> @@ -777,10 +760,10 @@ static void pnv_xive_vst_set_exclusive(PnvXive *xive, uint8_t type,
>   */
>  static void pnv_xive_vst_set_data(PnvXive *xive, uint64_t vsd, bool pc_engine)
>  {
> -    uint8_t mode = GETFIELD(VSD_MODE, vsd);
> -    uint8_t type = GETFIELD(VST_TABLE_SELECT,
> +    uint8_t mode = PNV_GETFIELD(VSD_MODE, vsd);
> +    uint8_t type = PNV_GETFIELD(VST_TABLE_SELECT,
>                              xive->regs[VC_VSD_TABLE_ADDR >> 3]);
> -    uint8_t blk = GETFIELD(VST_TABLE_BLOCK,
> +    uint8_t blk = PNV_GETFIELD(VST_TABLE_BLOCK,
>                             xive->regs[VC_VSD_TABLE_ADDR >> 3]);
>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
>  
> @@ -1456,7 +1439,8 @@ static XiveTCTX *pnv_xive_get_indirect_tctx(PnvXive *xive)
>          return NULL;
>      }
>  
> -    pir = (chip->chip_id << 8) | GETFIELD(PC_TCTXT_INDIR_THRDID, tctxt_indir);
> +    pir = (chip->chip_id << 8) |
> +        PNV_GETFIELD(PC_TCTXT_INDIR_THRDID, tctxt_indir);
>      cpu = pnv_chip_find_cpu(chip, pir);
>      if (!cpu) {
>          xive_error(xive, "IC: invalid PIR %x for indirect access", pir);
> @@ -1583,7 +1567,7 @@ static uint64_t pnv_xive_vc_read(void *opaque, hwaddr offset,
>      uint64_t ret = -1;
>  
>      if (edt_index < XIVE_TABLE_EDT_MAX) {
> -        edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
> +        edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
>      }
>  
>      switch (edt_type) {
> @@ -1625,7 +1609,7 @@ static void pnv_xive_vc_write(void *opaque, hwaddr offset,
>      AddressSpace *edt_as = NULL;
>  
>      if (edt_index < XIVE_TABLE_EDT_MAX) {
> -        edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
> +        edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
>      }
>  
>      switch (edt_type) {
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 74618fadf085..a2d5815d3485 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -16,6 +16,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pcie_port.h"
>  #include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  
> @@ -171,11 +172,11 @@ static void pnv_phb3_check_m64(PnvPHB3 *phb, uint32_t index)
>      }
>  
>      /* Grab geometry from registers */
> -    base = GETFIELD(IODA2_M64BT_BASE, m64) << 20;
> +    base = PNV_GETFIELD(IODA2_M64BT_BASE, m64) << 20;
>      if (m64 & IODA2_M64BT_SINGLE_PE) {
>          base &= ~0x1ffffffull;
>      }
> -    size = GETFIELD(IODA2_M64BT_MASK, m64) << 20;
> +    size = PNV_GETFIELD(IODA2_M64BT_MASK, m64) << 20;
>      size |= 0xfffc000000000000ull;
>      size = ~size + 1;
>      start = base | (phb->regs[PHB_M64_UPPER_BITS >> 3]);
> @@ -217,8 +218,8 @@ static void pnv_phb3_lxivt_write(PnvPHB3 *phb, unsigned idx, uint64_t val)
>      phb->ioda_LXIVT[idx] = val & (IODA2_LXIVT_SERVER |
>                                    IODA2_LXIVT_PRIORITY |
>                                    IODA2_LXIVT_NODE_ID);
> -    server = GETFIELD(IODA2_LXIVT_SERVER, val);
> -    prio = GETFIELD(IODA2_LXIVT_PRIORITY, val);
> +    server = PNV_GETFIELD(IODA2_LXIVT_SERVER, val);
> +    prio = PNV_GETFIELD(IODA2_LXIVT_PRIORITY, val);
>  
>      /*
>       * The low order 2 bits are the link pointer (Type II interrupts).
> @@ -233,8 +234,8 @@ static uint64_t *pnv_phb3_ioda_access(PnvPHB3 *phb,
>                                        unsigned *out_table, unsigned *out_idx)
>  {
>      uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
> -    unsigned int index = GETFIELD(PHB_IODA_AD_TADR, adreg);
> -    unsigned int table = GETFIELD(PHB_IODA_AD_TSEL, adreg);
> +    unsigned int index = PNV_GETFIELD(PHB_IODA_AD_TADR, adreg);
> +    unsigned int table = PNV_GETFIELD(PHB_IODA_AD_TSEL, adreg);
>      unsigned int mask;
>      uint64_t *tptr = NULL;
>  
> @@ -297,7 +298,7 @@ static uint64_t *pnv_phb3_ioda_access(PnvPHB3 *phb,
>      }
>      if (adreg & PHB_IODA_AD_AUTOINC) {
>          index = (index + 1) & mask;
> -        adreg = SETFIELD(PHB_IODA_AD_TADR, adreg, index);
> +        adreg = PNV_SETFIELD(PHB_IODA_AD_TADR, adreg, index);
>      }
>      phb->regs[PHB_IODA_ADDR >> 3] = adreg;
>      return tptr;
> @@ -363,10 +364,11 @@ void pnv_phb3_remap_irqs(PnvPHB3 *phb)
>      }
>  
>      /* Grab local LSI source ID */
> -    local = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]) << 3;
> +    local = PNV_GETFIELD(PHB_LSI_SRC_ID,
> +                         phb->regs[PHB_LSI_SOURCE_ID >> 3]) << 3;
>  
>      /* Grab global one and compare */
> -    global = GETFIELD(PBCQ_NEST_LSI_SRC,
> +    global = PNV_GETFIELD(PBCQ_NEST_LSI_SRC,
>                        pbcq->nest_regs[PBCQ_NEST_LSI_SRC_ID]) << 3;
>      if (global != local) {
>          /*
> @@ -378,9 +380,9 @@ void pnv_phb3_remap_irqs(PnvPHB3 *phb)
>      }
>  
>      /* Get the base on the powerbus */
> -    comp = GETFIELD(PBCQ_NEST_IRSN_COMP,
> +    comp = PNV_GETFIELD(PBCQ_NEST_IRSN_COMP,
>                      pbcq->nest_regs[PBCQ_NEST_IRSN_COMPARE]);
> -    mask = GETFIELD(PBCQ_NEST_IRSN_COMP,
> +    mask = PNV_GETFIELD(PBCQ_NEST_IRSN_COMP,
>                      pbcq->nest_regs[PBCQ_NEST_IRSN_MASK]);
>      count = ((~mask) + 1) & 0x7ffff;
>      phb->total_irq = count;
> @@ -735,10 +737,10 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr,
>                                     bool is_write, uint64_t tve,
>                                     IOMMUTLBEntry *tlb)
>  {
> -    uint64_t tta = GETFIELD(IODA2_TVT_TABLE_ADDR, tve);
> -    int32_t  lev = GETFIELD(IODA2_TVT_NUM_LEVELS, tve);
> -    uint32_t tts = GETFIELD(IODA2_TVT_TCE_TABLE_SIZE, tve);
> -    uint32_t tps = GETFIELD(IODA2_TVT_IO_PSIZE, tve);
> +    uint64_t tta = PNV_GETFIELD(IODA2_TVT_TABLE_ADDR, tve);
> +    int32_t  lev = PNV_GETFIELD(IODA2_TVT_NUM_LEVELS, tve);
> +    uint32_t tts = PNV_GETFIELD(IODA2_TVT_TCE_TABLE_SIZE, tve);
> +    uint32_t tps = PNV_GETFIELD(IODA2_TVT_IO_PSIZE, tve);
>      PnvPHB3 *phb = ds->phb;
>  
>      /* Invalid levels */
> diff --git a/hw/pci-host/pnv_phb3_msi.c b/hw/pci-host/pnv_phb3_msi.c
> index d645468f4a6f..f283d5cd7dc1 100644
> --- a/hw/pci-host/pnv_phb3_msi.c
> +++ b/hw/pci-host/pnv_phb3_msi.c
> @@ -13,6 +13,7 @@
>  #include "hw/pci-host/pnv_phb3_regs.h"
>  #include "hw/pci-host/pnv_phb3.h"
>  #include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>  #include "hw/pci/msi.h"
>  #include "monitor/monitor.h"
>  #include "hw/irq.h"
> @@ -105,14 +106,15 @@ static void phb3_msi_try_send(Phb3MsiState *msi, int srcno, bool force)
>          return;
>      }
>  
> -    server = GETFIELD(IODA2_IVT_SERVER, ive);
> -    prio = GETFIELD(IODA2_IVT_PRIORITY, ive);
> +    server = PNV_GETFIELD(IODA2_IVT_SERVER, ive);
> +    prio = PNV_GETFIELD(IODA2_IVT_PRIORITY, ive);
>      if (!force) {
> -        pq = GETFIELD(IODA2_IVT_Q, ive) | (GETFIELD(IODA2_IVT_P, ive) << 1);
> +        pq = PNV_GETFIELD(IODA2_IVT_Q, ive) |
> +            (PNV_GETFIELD(IODA2_IVT_P, ive) << 1);
>      } else {
>          pq = 0;
>      }
> -    gen = GETFIELD(IODA2_IVT_GEN, ive);
> +    gen = PNV_GETFIELD(IODA2_IVT_GEN, ive);
>  
>      /*
>       * The low order 2 bits are the link pointer (Type II interrupts).
> @@ -169,7 +171,7 @@ void pnv_phb3_msi_send(Phb3MsiState *msi, uint64_t addr, uint16_t data,
>          if (!phb3_msi_read_ive(msi->phb, src, &ive)) {
>              return;
>          }
> -        pe = GETFIELD(IODA2_IVT_PE, ive);
> +        pe = PNV_GETFIELD(IODA2_IVT_PE, ive);
>          if (pe != dev_pe) {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "MSI %d send by PE#%d but assigned to PE#%d",
> @@ -334,16 +336,16 @@ void pnv_phb3_msi_pic_print_info(Phb3MsiState *msi, Monitor *mon)
>              return;
>          }
>  
> -        if (GETFIELD(IODA2_IVT_PRIORITY, ive) == 0xff) {
> +        if (PNV_GETFIELD(IODA2_IVT_PRIORITY, ive) == 0xff) {
>              continue;
>          }
>  
>          monitor_printf(mon, "  %4x %c%c server=%04x prio=%02x gen=%d\n",
>                         ics->offset + i,
> -                       GETFIELD(IODA2_IVT_P, ive) ? 'P' : '-',
> -                       GETFIELD(IODA2_IVT_Q, ive) ? 'Q' : '-',
> -                       (uint32_t) GETFIELD(IODA2_IVT_SERVER, ive) >> 2,
> -                       (uint32_t) GETFIELD(IODA2_IVT_PRIORITY, ive),
> -                       (uint32_t) GETFIELD(IODA2_IVT_GEN, ive));
> +                       PNV_GETFIELD(IODA2_IVT_P, ive) ? 'P' : '-',
> +                       PNV_GETFIELD(IODA2_IVT_Q, ive) ? 'Q' : '-',
> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_SERVER, ive) >> 2,
> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_PRIORITY, ive),
> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_GEN, ive));
>      }
>  }
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 23cf093928ed..ac824f877cf1 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -19,6 +19,7 @@
>  #include "hw/pci/pcie_port.h"
>  #include "hw/ppc/pnv.h"
>  #include "hw/ppc/pnv_xscom.h"
> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  
> @@ -26,22 +27,6 @@
>      qemu_log_mask(LOG_GUEST_ERROR, "phb4[%d:%d]: " fmt "\n",            \
>                    (phb)->chip_id, (phb)->phb_id, ## __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);
> @@ -196,8 +181,8 @@ static void pnv_phb4_check_mbt(PnvPHB4 *phb, uint32_t index)
>      }
>  
>      /* Grab geometry from registers */
> -    base = GETFIELD(IODA3_MBT0_BASE_ADDR, mbe0) << 12;
> -    size = GETFIELD(IODA3_MBT1_MASK, mbe1) << 12;
> +    base = PNV_GETFIELD(IODA3_MBT0_BASE_ADDR, mbe0) << 12;
> +    size = PNV_GETFIELD(IODA3_MBT1_MASK, mbe1) << 12;
>      size |= 0xff00000000000000ull;
>      size = ~size + 1;
>  
> @@ -252,8 +237,8 @@ static uint64_t *pnv_phb4_ioda_access(PnvPHB4 *phb,
>                                        unsigned *out_table, unsigned *out_idx)
>  {
>      uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
> -    unsigned int index = GETFIELD(PHB_IODA_AD_TADR, adreg);
> -    unsigned int table = GETFIELD(PHB_IODA_AD_TSEL, adreg);
> +    unsigned int index = PNV_GETFIELD(PHB_IODA_AD_TADR, adreg);
> +    unsigned int table = PNV_GETFIELD(PHB_IODA_AD_TSEL, adreg);
>      unsigned int mask;
>      uint64_t *tptr = NULL;
>  
> @@ -318,7 +303,7 @@ static uint64_t *pnv_phb4_ioda_access(PnvPHB4 *phb,
>      }
>      if (adreg & PHB_IODA_AD_AUTOINC) {
>          index = (index + 1) & mask;
> -        adreg = SETFIELD(PHB_IODA_AD_TADR, adreg, index);
> +        adreg = PNV_SETFIELD(PHB_IODA_AD_TADR, adreg, index);
>      }
>  
>      phb->regs[PHB_IODA_ADDR >> 3] = adreg;
> @@ -369,7 +354,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb, uint64_t val)
>      case IODA3_TBL_MIST: {
>          /* Special mask for MIST partial write */
>          uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
> -        uint32_t mmask = GETFIELD(PHB_IODA_AD_MIST_PWV, adreg);
> +        uint32_t mmask = PNV_GETFIELD(PHB_IODA_AD_MIST_PWV, adreg);
>          uint64_t v = *tptr;
>          if (mmask == 0) {
>              mmask = 0xf;
> @@ -476,7 +461,7 @@ static void pnv_phb4_update_xsrc(PnvPHB4 *phb)
>      phb->xsrc.esb_shift = shift;
>      phb->xsrc.esb_flags = flags;
>  
> -    lsi_base = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
> +    lsi_base = PNV_GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
>      lsi_base <<= 3;
>  
>      /* TODO: handle reset values of PHB_LSI_SRC_ID */
> @@ -747,12 +732,13 @@ static uint64_t pnv_phb4_xscom_read(void *opaque, hwaddr addr, unsigned size)
>              return ~0ull;
>          }
>          size = (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_4B) ? 4 : 8;
> -        offset = GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR, phb->scom_hv_ind_addr_reg);
> +        offset = PNV_GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
> +                              phb->scom_hv_ind_addr_reg);
>          val = pnv_phb4_reg_read(phb, offset, size);
>          if (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_AUTOINC) {
>              offset += size;
>              offset &= 0x3fff;
> -            phb->scom_hv_ind_addr_reg = SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
> +            phb->scom_hv_ind_addr_reg = PNV_SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>                                                   phb->scom_hv_ind_addr_reg,
>                                                   offset);
>          }
> @@ -799,12 +785,13 @@ static void pnv_phb4_xscom_write(void *opaque, hwaddr addr,
>              break;
>          }
>          size = (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_4B) ? 4 : 8;
> -        offset = GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR, phb->scom_hv_ind_addr_reg);
> +        offset = PNV_GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
> +                              phb->scom_hv_ind_addr_reg);
>          pnv_phb4_reg_write(phb, offset, val, size);
>          if (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_AUTOINC) {
>              offset += size;
>              offset &= 0x3fff;
> -            phb->scom_hv_ind_addr_reg = SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
> +            phb->scom_hv_ind_addr_reg = PNV_SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>                                                   phb->scom_hv_ind_addr_reg,
>                                                   offset);
>          }
> @@ -860,7 +847,7 @@ static void pnv_phb4_set_irq(void *opaque, int irq_num, int level)
>      if (irq_num > 3) {
>          phb_error(phb, "IRQ %x is not an LSI", irq_num);
>      }
> -    lsi_base = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
> +    lsi_base = PNV_GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
>      lsi_base <<= 3;
>      qemu_set_irq(phb->qirqs[lsi_base + irq_num], level);
>  }
> @@ -910,10 +897,10 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr,
>                                     bool is_write, uint64_t tve,
>                                     IOMMUTLBEntry *tlb)
>  {
> -    uint64_t tta = GETFIELD(IODA3_TVT_TABLE_ADDR, tve);
> -    int32_t  lev = GETFIELD(IODA3_TVT_NUM_LEVELS, tve);
> -    uint32_t tts = GETFIELD(IODA3_TVT_TCE_TABLE_SIZE, tve);
> -    uint32_t tps = GETFIELD(IODA3_TVT_IO_PSIZE, tve);
> +    uint64_t tta = PNV_GETFIELD(IODA3_TVT_TABLE_ADDR, tve);
> +    int32_t  lev = PNV_GETFIELD(IODA3_TVT_NUM_LEVELS, tve);
> +    uint32_t tts = PNV_GETFIELD(IODA3_TVT_TCE_TABLE_SIZE, tve);
> +    uint32_t tps = PNV_GETFIELD(IODA3_TVT_IO_PSIZE, tve);
>  
>      /* Invalid levels */
>      if (lev > 4) {

On 4/2/20 2:31 AM, David Gibson wrote:
> On Wed, Apr 01, 2020 at 05:26:33PM +0200, Cédric Le Goater wrote:
>> Most of QEMU definitions of the register fields of the PowerNV machine
>> come from skiboot and the models duplicate a set of macros for this
>> purpose. Make them common under the pnv_utils.h file.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Hrm.  If we're touching these, would it make sense to rewrite them in
> terms of the cross-qemu generic extract64() and deposit64()?

I won't do that because we will loose compatibility with skiboot.

C.

> 
>> ---
>>  include/hw/pci-host/pnv_phb3_regs.h | 16 ------
>>  include/hw/ppc/pnv_utils.h          | 29 +++++++++++
>>  hw/intc/pnv_xive.c                  | 76 ++++++++++++-----------------
>>  hw/pci-host/pnv_phb3.c              | 32 ++++++------
>>  hw/pci-host/pnv_phb3_msi.c          | 24 ++++-----
>>  hw/pci-host/pnv_phb4.c              | 51 ++++++++-----------
>>  6 files changed, 108 insertions(+), 120 deletions(-)
>>  create mode 100644 include/hw/ppc/pnv_utils.h
>>
>> 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/include/hw/ppc/pnv_utils.h b/include/hw/ppc/pnv_utils.h
>> new file mode 100644
>> index 000000000000..8521e13b5149
>> --- /dev/null
>> +++ b/include/hw/ppc/pnv_utils.h
>> @@ -0,0 +1,29 @@
>> +/*
>> + * QEMU PowerPC PowerNV utilities
>> + *
>> + * Copyright (c) 2020, IBM Corporation.
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef PPC_PNV_UTILS_H
>> +#define PPC_PNV_UTILS_H
>> +
>> +/*
>> + * QEMU version of the GETFIELD/SETFIELD macros used in skiboot to
>> + * define the register fields.
>> + */
>> +
>> +static inline uint64_t PNV_GETFIELD(uint64_t mask, uint64_t word)
>> +{
>> +    return (word & mask) >> ctz64(mask);
>> +}
>> +
>> +static inline uint64_t PNV_SETFIELD(uint64_t mask, uint64_t word,
>> +                                    uint64_t value)
>> +{
>> +    return (word & ~mask) | ((value << ctz64(mask)) & mask);
>> +}
>> +
>> +#endif /* PPC_PNV_UTILS_H */
>> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
>> index aeda488bd113..77cacdd6c623 100644
>> --- a/hw/intc/pnv_xive.c
>> +++ b/hw/intc/pnv_xive.c
>> @@ -21,6 +21,7 @@
>>  #include "hw/ppc/pnv_core.h"
>>  #include "hw/ppc/pnv_xscom.h"
>>  #include "hw/ppc/pnv_xive.h"
>> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>>  #include "hw/ppc/xive_regs.h"
>>  #include "hw/qdev-properties.h"
>>  #include "hw/ppc/ppc.h"
>> @@ -65,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
>> @@ -96,7 +77,7 @@ static uint8_t pnv_xive_block_id(PnvXive *xive)
>>      uint64_t cfg_val = xive->regs[PC_TCTXT_CFG >> 3];
>>  
>>      if (cfg_val & PC_TCTXT_CHIPID_OVERRIDE) {
>> -        blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>> +        blk = PNV_GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>>      }
>>  
>>      return blk;
>> @@ -145,7 +126,7 @@ static uint64_t pnv_xive_vst_addr_direct(PnvXive *xive, uint32_t type,
>>  {
>>      const XiveVstInfo *info = &vst_infos[type];
>>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
>> -    uint64_t vst_tsize = 1ull << (GETFIELD(VSD_TSIZE, vsd) + 12);
>> +    uint64_t vst_tsize = 1ull << (PNV_GETFIELD(VSD_TSIZE, vsd) + 12);
>>      uint32_t idx_max;
>>  
>>      idx_max = vst_tsize / info->size - 1;
>> @@ -180,7 +161,7 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>>          return 0;
>>      }
>>  
>> -    page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
>> +    page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>>  
>>      if (!pnv_xive_vst_page_size_allowed(page_shift)) {
>>          xive_error(xive, "VST: invalid %s page shift %d", info->name,
>> @@ -207,7 +188,7 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>>           * Check that the pages have a consistent size across the
>>           * indirect table
>>           */
>> -        if (page_shift != GETFIELD(VSD_TSIZE, vsd) + 12) {
>> +        if (page_shift != PNV_GETFIELD(VSD_TSIZE, vsd) + 12) {
>>              xive_error(xive, "VST: %s entry %x indirect page size differ !?",
>>                         info->name, idx);
>>              return 0;
>> @@ -232,7 +213,7 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>>      vsd = xive->vsds[type][blk];
>>  
>>      /* Remote VST access */
>> -    if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>> +    if (PNV_GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>>          xive = pnv_xive_get_remote(blk);
>>  
>>          return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
>> @@ -295,9 +276,9 @@ static int pnv_xive_write_end(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>>  
>>  static int pnv_xive_end_update(PnvXive *xive)
>>  {
>> -    uint8_t  blk = GETFIELD(VC_EQC_CWATCH_BLOCKID,
>> +    uint8_t  blk = PNV_GETFIELD(VC_EQC_CWATCH_BLOCKID,
>>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
>> -    uint32_t idx = GETFIELD(VC_EQC_CWATCH_OFFSET,
>> +    uint32_t idx = PNV_GETFIELD(VC_EQC_CWATCH_OFFSET,
>>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
>>      int i;
>>      uint64_t eqc_watch[4];
>> @@ -312,9 +293,9 @@ static int pnv_xive_end_update(PnvXive *xive)
>>  
>>  static void pnv_xive_end_cache_load(PnvXive *xive)
>>  {
>> -    uint8_t  blk = GETFIELD(VC_EQC_CWATCH_BLOCKID,
>> +    uint8_t  blk = PNV_GETFIELD(VC_EQC_CWATCH_BLOCKID,
>>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
>> -    uint32_t idx = GETFIELD(VC_EQC_CWATCH_OFFSET,
>> +    uint32_t idx = PNV_GETFIELD(VC_EQC_CWATCH_OFFSET,
>>                             xive->regs[(VC_EQC_CWATCH_SPEC >> 3)]);
>>      uint64_t eqc_watch[4] = { 0 };
>>      int i;
>> @@ -343,9 +324,9 @@ static int pnv_xive_write_nvt(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>>  
>>  static int pnv_xive_nvt_update(PnvXive *xive)
>>  {
>> -    uint8_t  blk = GETFIELD(PC_VPC_CWATCH_BLOCKID,
>> +    uint8_t  blk = PNV_GETFIELD(PC_VPC_CWATCH_BLOCKID,
>>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
>> -    uint32_t idx = GETFIELD(PC_VPC_CWATCH_OFFSET,
>> +    uint32_t idx = PNV_GETFIELD(PC_VPC_CWATCH_OFFSET,
>>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
>>      int i;
>>      uint64_t vpc_watch[8];
>> @@ -360,9 +341,9 @@ static int pnv_xive_nvt_update(PnvXive *xive)
>>  
>>  static void pnv_xive_nvt_cache_load(PnvXive *xive)
>>  {
>> -    uint8_t  blk = GETFIELD(PC_VPC_CWATCH_BLOCKID,
>> +    uint8_t  blk = PNV_GETFIELD(PC_VPC_CWATCH_BLOCKID,
>>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
>> -    uint32_t idx = GETFIELD(PC_VPC_CWATCH_OFFSET,
>> +    uint32_t idx = PNV_GETFIELD(PC_VPC_CWATCH_OFFSET,
>>                             xive->regs[(PC_VPC_CWATCH_SPEC >> 3)]);
>>      uint64_t vpc_watch[8] = { 0 };
>>      int i;
>> @@ -518,7 +499,7 @@ static uint64_t pnv_xive_pc_size(PnvXive *xive)
>>  static uint32_t pnv_xive_nr_ipis(PnvXive *xive, uint8_t blk)
>>  {
>>      uint64_t vsd = xive->vsds[VST_TSEL_SBE][blk];
>> -    uint64_t vst_tsize = 1ull << (GETFIELD(VSD_TSIZE, vsd) + 12);
>> +    uint64_t vst_tsize = 1ull << (PNV_GETFIELD(VSD_TSIZE, vsd) + 12);
>>  
>>      return VSD_INDIRECT & vsd ? 0 : vst_tsize * SBE_PER_BYTE;
>>  }
>> @@ -550,7 +531,7 @@ static uint64_t pnv_xive_vst_per_subpage(PnvXive *xive, uint32_t type)
>>          return 0;
>>      }
>>  
>> -    page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
>> +    page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>>  
>>      if (!pnv_xive_vst_page_size_allowed(page_shift)) {
>>          xive_error(xive, "VST: invalid %s page shift %d", info->name,
>> @@ -582,7 +563,7 @@ static uint64_t pnv_xive_edt_size(PnvXive *xive, uint64_t type)
>>      int i;
>>  
>>      for (i = 0; i < XIVE_TABLE_EDT_MAX; i++) {
>> -        uint64_t edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
>> +        uint64_t edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
>>  
>>          if (edt_type == type) {
>>              size += edt_size;
>> @@ -604,7 +585,7 @@ static uint64_t pnv_xive_edt_offset(PnvXive *xive, uint64_t vc_offset,
>>      uint64_t edt_offset = vc_offset;
>>  
>>      for (i = 0; i < XIVE_TABLE_EDT_MAX && (i * edt_size) < vc_offset; i++) {
>> -        uint64_t edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
>> +        uint64_t edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[i]);
>>  
>>          if (edt_type != type) {
>>              edt_offset -= edt_size;
>> @@ -632,7 +613,8 @@ static void pnv_xive_edt_resize(PnvXive *xive)
>>  static int pnv_xive_table_set_data(PnvXive *xive, uint64_t val)
>>  {
>>      uint64_t tsel = xive->regs[CQ_TAR >> 3] & CQ_TAR_TSEL;
>> -    uint8_t tsel_index = GETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3]);
>> +    uint8_t tsel_index = PNV_GETFIELD(CQ_TAR_TSEL_INDEX,
>> +                                      xive->regs[CQ_TAR >> 3]);
>>      uint64_t *xive_table;
>>      uint8_t max_index;
>>  
>> @@ -667,7 +649,8 @@ static int pnv_xive_table_set_data(PnvXive *xive, uint64_t val)
>>  
>>      if (xive->regs[CQ_TAR >> 3] & CQ_TAR_TBL_AUTOINC) {
>>          xive->regs[CQ_TAR >> 3] =
>> -            SETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3], ++tsel_index);
>> +            PNV_SETFIELD(CQ_TAR_TSEL_INDEX, xive->regs[CQ_TAR >> 3],
>> +                         ++tsel_index);
>>      }
>>  
>>      /*
>> @@ -690,7 +673,7 @@ static void pnv_xive_vst_set_exclusive(PnvXive *xive, uint8_t type,
>>      XiveENDSource *end_xsrc = &xive->end_source;
>>      XiveSource *xsrc = &xive->ipi_source;
>>      const XiveVstInfo *info = &vst_infos[type];
>> -    uint32_t page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
>> +    uint32_t page_shift = PNV_GETFIELD(VSD_TSIZE, vsd) + 12;
>>      uint64_t vst_tsize = 1ull << page_shift;
>>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
>>  
>> @@ -777,10 +760,10 @@ static void pnv_xive_vst_set_exclusive(PnvXive *xive, uint8_t type,
>>   */
>>  static void pnv_xive_vst_set_data(PnvXive *xive, uint64_t vsd, bool pc_engine)
>>  {
>> -    uint8_t mode = GETFIELD(VSD_MODE, vsd);
>> -    uint8_t type = GETFIELD(VST_TABLE_SELECT,
>> +    uint8_t mode = PNV_GETFIELD(VSD_MODE, vsd);
>> +    uint8_t type = PNV_GETFIELD(VST_TABLE_SELECT,
>>                              xive->regs[VC_VSD_TABLE_ADDR >> 3]);
>> -    uint8_t blk = GETFIELD(VST_TABLE_BLOCK,
>> +    uint8_t blk = PNV_GETFIELD(VST_TABLE_BLOCK,
>>                             xive->regs[VC_VSD_TABLE_ADDR >> 3]);
>>      uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
>>  
>> @@ -1456,7 +1439,8 @@ static XiveTCTX *pnv_xive_get_indirect_tctx(PnvXive *xive)
>>          return NULL;
>>      }
>>  
>> -    pir = (chip->chip_id << 8) | GETFIELD(PC_TCTXT_INDIR_THRDID, tctxt_indir);
>> +    pir = (chip->chip_id << 8) |
>> +        PNV_GETFIELD(PC_TCTXT_INDIR_THRDID, tctxt_indir);
>>      cpu = pnv_chip_find_cpu(chip, pir);
>>      if (!cpu) {
>>          xive_error(xive, "IC: invalid PIR %x for indirect access", pir);
>> @@ -1583,7 +1567,7 @@ static uint64_t pnv_xive_vc_read(void *opaque, hwaddr offset,
>>      uint64_t ret = -1;
>>  
>>      if (edt_index < XIVE_TABLE_EDT_MAX) {
>> -        edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
>> +        edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
>>      }
>>  
>>      switch (edt_type) {
>> @@ -1625,7 +1609,7 @@ static void pnv_xive_vc_write(void *opaque, hwaddr offset,
>>      AddressSpace *edt_as = NULL;
>>  
>>      if (edt_index < XIVE_TABLE_EDT_MAX) {
>> -        edt_type = GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
>> +        edt_type = PNV_GETFIELD(CQ_TDR_EDT_TYPE, xive->edt[edt_index]);
>>      }
>>  
>>      switch (edt_type) {
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index 74618fadf085..a2d5815d3485 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -16,6 +16,7 @@
>>  #include "hw/pci/pcie_host.h"
>>  #include "hw/pci/pcie_port.h"
>>  #include "hw/ppc/pnv.h"
>> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>>  #include "hw/irq.h"
>>  #include "hw/qdev-properties.h"
>>  
>> @@ -171,11 +172,11 @@ static void pnv_phb3_check_m64(PnvPHB3 *phb, uint32_t index)
>>      }
>>  
>>      /* Grab geometry from registers */
>> -    base = GETFIELD(IODA2_M64BT_BASE, m64) << 20;
>> +    base = PNV_GETFIELD(IODA2_M64BT_BASE, m64) << 20;
>>      if (m64 & IODA2_M64BT_SINGLE_PE) {
>>          base &= ~0x1ffffffull;
>>      }
>> -    size = GETFIELD(IODA2_M64BT_MASK, m64) << 20;
>> +    size = PNV_GETFIELD(IODA2_M64BT_MASK, m64) << 20;
>>      size |= 0xfffc000000000000ull;
>>      size = ~size + 1;
>>      start = base | (phb->regs[PHB_M64_UPPER_BITS >> 3]);
>> @@ -217,8 +218,8 @@ static void pnv_phb3_lxivt_write(PnvPHB3 *phb, unsigned idx, uint64_t val)
>>      phb->ioda_LXIVT[idx] = val & (IODA2_LXIVT_SERVER |
>>                                    IODA2_LXIVT_PRIORITY |
>>                                    IODA2_LXIVT_NODE_ID);
>> -    server = GETFIELD(IODA2_LXIVT_SERVER, val);
>> -    prio = GETFIELD(IODA2_LXIVT_PRIORITY, val);
>> +    server = PNV_GETFIELD(IODA2_LXIVT_SERVER, val);
>> +    prio = PNV_GETFIELD(IODA2_LXIVT_PRIORITY, val);
>>  
>>      /*
>>       * The low order 2 bits are the link pointer (Type II interrupts).
>> @@ -233,8 +234,8 @@ static uint64_t *pnv_phb3_ioda_access(PnvPHB3 *phb,
>>                                        unsigned *out_table, unsigned *out_idx)
>>  {
>>      uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
>> -    unsigned int index = GETFIELD(PHB_IODA_AD_TADR, adreg);
>> -    unsigned int table = GETFIELD(PHB_IODA_AD_TSEL, adreg);
>> +    unsigned int index = PNV_GETFIELD(PHB_IODA_AD_TADR, adreg);
>> +    unsigned int table = PNV_GETFIELD(PHB_IODA_AD_TSEL, adreg);
>>      unsigned int mask;
>>      uint64_t *tptr = NULL;
>>  
>> @@ -297,7 +298,7 @@ static uint64_t *pnv_phb3_ioda_access(PnvPHB3 *phb,
>>      }
>>      if (adreg & PHB_IODA_AD_AUTOINC) {
>>          index = (index + 1) & mask;
>> -        adreg = SETFIELD(PHB_IODA_AD_TADR, adreg, index);
>> +        adreg = PNV_SETFIELD(PHB_IODA_AD_TADR, adreg, index);
>>      }
>>      phb->regs[PHB_IODA_ADDR >> 3] = adreg;
>>      return tptr;
>> @@ -363,10 +364,11 @@ void pnv_phb3_remap_irqs(PnvPHB3 *phb)
>>      }
>>  
>>      /* Grab local LSI source ID */
>> -    local = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]) << 3;
>> +    local = PNV_GETFIELD(PHB_LSI_SRC_ID,
>> +                         phb->regs[PHB_LSI_SOURCE_ID >> 3]) << 3;
>>  
>>      /* Grab global one and compare */
>> -    global = GETFIELD(PBCQ_NEST_LSI_SRC,
>> +    global = PNV_GETFIELD(PBCQ_NEST_LSI_SRC,
>>                        pbcq->nest_regs[PBCQ_NEST_LSI_SRC_ID]) << 3;
>>      if (global != local) {
>>          /*
>> @@ -378,9 +380,9 @@ void pnv_phb3_remap_irqs(PnvPHB3 *phb)
>>      }
>>  
>>      /* Get the base on the powerbus */
>> -    comp = GETFIELD(PBCQ_NEST_IRSN_COMP,
>> +    comp = PNV_GETFIELD(PBCQ_NEST_IRSN_COMP,
>>                      pbcq->nest_regs[PBCQ_NEST_IRSN_COMPARE]);
>> -    mask = GETFIELD(PBCQ_NEST_IRSN_COMP,
>> +    mask = PNV_GETFIELD(PBCQ_NEST_IRSN_COMP,
>>                      pbcq->nest_regs[PBCQ_NEST_IRSN_MASK]);
>>      count = ((~mask) + 1) & 0x7ffff;
>>      phb->total_irq = count;
>> @@ -735,10 +737,10 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr,
>>                                     bool is_write, uint64_t tve,
>>                                     IOMMUTLBEntry *tlb)
>>  {
>> -    uint64_t tta = GETFIELD(IODA2_TVT_TABLE_ADDR, tve);
>> -    int32_t  lev = GETFIELD(IODA2_TVT_NUM_LEVELS, tve);
>> -    uint32_t tts = GETFIELD(IODA2_TVT_TCE_TABLE_SIZE, tve);
>> -    uint32_t tps = GETFIELD(IODA2_TVT_IO_PSIZE, tve);
>> +    uint64_t tta = PNV_GETFIELD(IODA2_TVT_TABLE_ADDR, tve);
>> +    int32_t  lev = PNV_GETFIELD(IODA2_TVT_NUM_LEVELS, tve);
>> +    uint32_t tts = PNV_GETFIELD(IODA2_TVT_TCE_TABLE_SIZE, tve);
>> +    uint32_t tps = PNV_GETFIELD(IODA2_TVT_IO_PSIZE, tve);
>>      PnvPHB3 *phb = ds->phb;
>>  
>>      /* Invalid levels */
>> diff --git a/hw/pci-host/pnv_phb3_msi.c b/hw/pci-host/pnv_phb3_msi.c
>> index d645468f4a6f..f283d5cd7dc1 100644
>> --- a/hw/pci-host/pnv_phb3_msi.c
>> +++ b/hw/pci-host/pnv_phb3_msi.c
>> @@ -13,6 +13,7 @@
>>  #include "hw/pci-host/pnv_phb3_regs.h"
>>  #include "hw/pci-host/pnv_phb3.h"
>>  #include "hw/ppc/pnv.h"
>> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>>  #include "hw/pci/msi.h"
>>  #include "monitor/monitor.h"
>>  #include "hw/irq.h"
>> @@ -105,14 +106,15 @@ static void phb3_msi_try_send(Phb3MsiState *msi, int srcno, bool force)
>>          return;
>>      }
>>  
>> -    server = GETFIELD(IODA2_IVT_SERVER, ive);
>> -    prio = GETFIELD(IODA2_IVT_PRIORITY, ive);
>> +    server = PNV_GETFIELD(IODA2_IVT_SERVER, ive);
>> +    prio = PNV_GETFIELD(IODA2_IVT_PRIORITY, ive);
>>      if (!force) {
>> -        pq = GETFIELD(IODA2_IVT_Q, ive) | (GETFIELD(IODA2_IVT_P, ive) << 1);
>> +        pq = PNV_GETFIELD(IODA2_IVT_Q, ive) |
>> +            (PNV_GETFIELD(IODA2_IVT_P, ive) << 1);
>>      } else {
>>          pq = 0;
>>      }
>> -    gen = GETFIELD(IODA2_IVT_GEN, ive);
>> +    gen = PNV_GETFIELD(IODA2_IVT_GEN, ive);
>>  
>>      /*
>>       * The low order 2 bits are the link pointer (Type II interrupts).
>> @@ -169,7 +171,7 @@ void pnv_phb3_msi_send(Phb3MsiState *msi, uint64_t addr, uint16_t data,
>>          if (!phb3_msi_read_ive(msi->phb, src, &ive)) {
>>              return;
>>          }
>> -        pe = GETFIELD(IODA2_IVT_PE, ive);
>> +        pe = PNV_GETFIELD(IODA2_IVT_PE, ive);
>>          if (pe != dev_pe) {
>>              qemu_log_mask(LOG_GUEST_ERROR,
>>                            "MSI %d send by PE#%d but assigned to PE#%d",
>> @@ -334,16 +336,16 @@ void pnv_phb3_msi_pic_print_info(Phb3MsiState *msi, Monitor *mon)
>>              return;
>>          }
>>  
>> -        if (GETFIELD(IODA2_IVT_PRIORITY, ive) == 0xff) {
>> +        if (PNV_GETFIELD(IODA2_IVT_PRIORITY, ive) == 0xff) {
>>              continue;
>>          }
>>  
>>          monitor_printf(mon, "  %4x %c%c server=%04x prio=%02x gen=%d\n",
>>                         ics->offset + i,
>> -                       GETFIELD(IODA2_IVT_P, ive) ? 'P' : '-',
>> -                       GETFIELD(IODA2_IVT_Q, ive) ? 'Q' : '-',
>> -                       (uint32_t) GETFIELD(IODA2_IVT_SERVER, ive) >> 2,
>> -                       (uint32_t) GETFIELD(IODA2_IVT_PRIORITY, ive),
>> -                       (uint32_t) GETFIELD(IODA2_IVT_GEN, ive));
>> +                       PNV_GETFIELD(IODA2_IVT_P, ive) ? 'P' : '-',
>> +                       PNV_GETFIELD(IODA2_IVT_Q, ive) ? 'Q' : '-',
>> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_SERVER, ive) >> 2,
>> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_PRIORITY, ive),
>> +                       (uint32_t) PNV_GETFIELD(IODA2_IVT_GEN, ive));
>>      }
>>  }
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 23cf093928ed..ac824f877cf1 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -19,6 +19,7 @@
>>  #include "hw/pci/pcie_port.h"
>>  #include "hw/ppc/pnv.h"
>>  #include "hw/ppc/pnv_xscom.h"
>> +#include "hw/ppc/pnv_utils.h" /* SETFIELD() and GETFIELD() macros */
>>  #include "hw/irq.h"
>>  #include "hw/qdev-properties.h"
>>  
>> @@ -26,22 +27,6 @@
>>      qemu_log_mask(LOG_GUEST_ERROR, "phb4[%d:%d]: " fmt "\n",            \
>>                    (phb)->chip_id, (phb)->phb_id, ## __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);
>> @@ -196,8 +181,8 @@ static void pnv_phb4_check_mbt(PnvPHB4 *phb, uint32_t index)
>>      }
>>  
>>      /* Grab geometry from registers */
>> -    base = GETFIELD(IODA3_MBT0_BASE_ADDR, mbe0) << 12;
>> -    size = GETFIELD(IODA3_MBT1_MASK, mbe1) << 12;
>> +    base = PNV_GETFIELD(IODA3_MBT0_BASE_ADDR, mbe0) << 12;
>> +    size = PNV_GETFIELD(IODA3_MBT1_MASK, mbe1) << 12;
>>      size |= 0xff00000000000000ull;
>>      size = ~size + 1;
>>  
>> @@ -252,8 +237,8 @@ static uint64_t *pnv_phb4_ioda_access(PnvPHB4 *phb,
>>                                        unsigned *out_table, unsigned *out_idx)
>>  {
>>      uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
>> -    unsigned int index = GETFIELD(PHB_IODA_AD_TADR, adreg);
>> -    unsigned int table = GETFIELD(PHB_IODA_AD_TSEL, adreg);
>> +    unsigned int index = PNV_GETFIELD(PHB_IODA_AD_TADR, adreg);
>> +    unsigned int table = PNV_GETFIELD(PHB_IODA_AD_TSEL, adreg);
>>      unsigned int mask;
>>      uint64_t *tptr = NULL;
>>  
>> @@ -318,7 +303,7 @@ static uint64_t *pnv_phb4_ioda_access(PnvPHB4 *phb,
>>      }
>>      if (adreg & PHB_IODA_AD_AUTOINC) {
>>          index = (index + 1) & mask;
>> -        adreg = SETFIELD(PHB_IODA_AD_TADR, adreg, index);
>> +        adreg = PNV_SETFIELD(PHB_IODA_AD_TADR, adreg, index);
>>      }
>>  
>>      phb->regs[PHB_IODA_ADDR >> 3] = adreg;
>> @@ -369,7 +354,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb, uint64_t val)
>>      case IODA3_TBL_MIST: {
>>          /* Special mask for MIST partial write */
>>          uint64_t adreg = phb->regs[PHB_IODA_ADDR >> 3];
>> -        uint32_t mmask = GETFIELD(PHB_IODA_AD_MIST_PWV, adreg);
>> +        uint32_t mmask = PNV_GETFIELD(PHB_IODA_AD_MIST_PWV, adreg);
>>          uint64_t v = *tptr;
>>          if (mmask == 0) {
>>              mmask = 0xf;
>> @@ -476,7 +461,7 @@ static void pnv_phb4_update_xsrc(PnvPHB4 *phb)
>>      phb->xsrc.esb_shift = shift;
>>      phb->xsrc.esb_flags = flags;
>>  
>> -    lsi_base = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
>> +    lsi_base = PNV_GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
>>      lsi_base <<= 3;
>>  
>>      /* TODO: handle reset values of PHB_LSI_SRC_ID */
>> @@ -747,12 +732,13 @@ static uint64_t pnv_phb4_xscom_read(void *opaque, hwaddr addr, unsigned size)
>>              return ~0ull;
>>          }
>>          size = (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_4B) ? 4 : 8;
>> -        offset = GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR, phb->scom_hv_ind_addr_reg);
>> +        offset = PNV_GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>> +                              phb->scom_hv_ind_addr_reg);
>>          val = pnv_phb4_reg_read(phb, offset, size);
>>          if (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_AUTOINC) {
>>              offset += size;
>>              offset &= 0x3fff;
>> -            phb->scom_hv_ind_addr_reg = SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>> +            phb->scom_hv_ind_addr_reg = PNV_SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>>                                                   phb->scom_hv_ind_addr_reg,
>>                                                   offset);
>>          }
>> @@ -799,12 +785,13 @@ static void pnv_phb4_xscom_write(void *opaque, hwaddr addr,
>>              break;
>>          }
>>          size = (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_4B) ? 4 : 8;
>> -        offset = GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR, phb->scom_hv_ind_addr_reg);
>> +        offset = PNV_GETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>> +                              phb->scom_hv_ind_addr_reg);
>>          pnv_phb4_reg_write(phb, offset, val, size);
>>          if (phb->scom_hv_ind_addr_reg & PHB_SCOM_HV_IND_ADDR_AUTOINC) {
>>              offset += size;
>>              offset &= 0x3fff;
>> -            phb->scom_hv_ind_addr_reg = SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>> +            phb->scom_hv_ind_addr_reg = PNV_SETFIELD(PHB_SCOM_HV_IND_ADDR_ADDR,
>>                                                   phb->scom_hv_ind_addr_reg,
>>                                                   offset);
>>          }
>> @@ -860,7 +847,7 @@ static void pnv_phb4_set_irq(void *opaque, int irq_num, int level)
>>      if (irq_num > 3) {
>>          phb_error(phb, "IRQ %x is not an LSI", irq_num);
>>      }
>> -    lsi_base = GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
>> +    lsi_base = PNV_GETFIELD(PHB_LSI_SRC_ID, phb->regs[PHB_LSI_SOURCE_ID >> 3]);
>>      lsi_base <<= 3;
>>      qemu_set_irq(phb->qirqs[lsi_base + irq_num], level);
>>  }
>> @@ -910,10 +897,10 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr,
>>                                     bool is_write, uint64_t tve,
>>                                     IOMMUTLBEntry *tlb)
>>  {
>> -    uint64_t tta = GETFIELD(IODA3_TVT_TABLE_ADDR, tve);
>> -    int32_t  lev = GETFIELD(IODA3_TVT_NUM_LEVELS, tve);
>> -    uint32_t tts = GETFIELD(IODA3_TVT_TCE_TABLE_SIZE, tve);
>> -    uint32_t tps = GETFIELD(IODA3_TVT_IO_PSIZE, tve);
>> +    uint64_t tta = PNV_GETFIELD(IODA3_TVT_TABLE_ADDR, tve);
>> +    int32_t  lev = PNV_GETFIELD(IODA3_TVT_NUM_LEVELS, tve);
>> +    uint32_t tts = PNV_GETFIELD(IODA3_TVT_TCE_TABLE_SIZE, tve);
>> +    uint32_t tps = PNV_GETFIELD(IODA3_TVT_IO_PSIZE, tve);
>>  
>>      /* Invalid levels */
>>      if (lev > 4) {
>

On Thu, Apr 02, 2020 at 08:41:24AM +0200, Cédric Le Goater wrote:
> On 4/2/20 2:31 AM, David Gibson wrote:
> > On Wed, Apr 01, 2020 at 05:26:33PM +0200, Cédric Le Goater wrote:
> >> Most of QEMU definitions of the register fields of the PowerNV machine
> >> come from skiboot and the models duplicate a set of macros for this
> >> purpose. Make them common under the pnv_utils.h file.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > 
> > Hrm.  If we're touching these, would it make sense to rewrite them in
> > terms of the cross-qemu generic extract64() and deposit64()?
> 
> I won't do that because we will loose compatibility with skiboot.

Uh.. how so?

On 4/2/20 8:50 AM, David Gibson wrote:
> On Thu, Apr 02, 2020 at 08:41:24AM +0200, Cédric Le Goater wrote:
>> On 4/2/20 2:31 AM, David Gibson wrote:
>>> On Wed, Apr 01, 2020 at 05:26:33PM +0200, Cédric Le Goater wrote:
>>>> Most of QEMU definitions of the register fields of the PowerNV machine
>>>> come from skiboot and the models duplicate a set of macros for this
>>>> purpose. Make them common under the pnv_utils.h file.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>
>>> Hrm.  If we're touching these, would it make sense to rewrite them in
>>> terms of the cross-qemu generic extract64() and deposit64()?
>>
>> I won't do that because we will loose compatibility with skiboot.
> 
> Uh.. how so?

What would be very nice is to use the QEMU FIELD() from "hw/registerfields.h".

But that's a complete different approach from what skiboot does and it would
mean rewriting all the register definitions we include in QEMU for the powernv 
models and the code using the fields. It is a major change and I would rather 
have the same files on both side (without tabs on the QEMU side). I think
it is safer.

Using extract64() and deposit64() raises the same kind of problem AFAICT. 
If we find a clean way to keep the register definition files the same, I am
OK with any changes.

C.

On Thu, Apr 02, 2020 at 09:18:09AM +0200, Cédric Le Goater wrote:
> On 4/2/20 8:50 AM, David Gibson wrote:
> > On Thu, Apr 02, 2020 at 08:41:24AM +0200, Cédric Le Goater wrote:
> >> On 4/2/20 2:31 AM, David Gibson wrote:
> >>> On Wed, Apr 01, 2020 at 05:26:33PM +0200, Cédric Le Goater wrote:
> >>>> Most of QEMU definitions of the register fields of the PowerNV machine
> >>>> come from skiboot and the models duplicate a set of macros for this
> >>>> purpose. Make them common under the pnv_utils.h file.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>
> >>> Hrm.  If we're touching these, would it make sense to rewrite them in
> >>> terms of the cross-qemu generic extract64() and deposit64()?
> >>
> >> I won't do that because we will loose compatibility with skiboot.
> > 
> > Uh.. how so?
> 
> What would be very nice is to use the QEMU FIELD() from "hw/registerfields.h".
> 
> But that's a complete different approach from what skiboot does and it would
> mean rewriting all the register definitions we include in QEMU for the powernv 
> models and the code using the fields. It is a major change and I would rather 
> have the same files on both side (without tabs on the QEMU side). I think
> it is safer.
> 
> Using extract64() and deposit64() raises the same kind of problem AFAICT. 
> If we find a clean way to keep the register definition files the same, I am
> OK with any changes.

So, I wasn't suggesting changing all the users to use
extract64()/deposit64()/FIELD().  I was suggesting making the skiboot
like versions a wrapper than translates the arguments into the right
form for FIELD() or whatever.