Re: [PATCH for-5.0 v5 17/23] ppc/pnv: Clarify how the TIMA is accessed on a multichip system

[Greg Kurz]

On Fri, 15 Nov 2019 17:24:30 +0100
Cédric Le Goater <address@hidden> wrote:

> The TIMA region gives access to the thread interrupt context registers
> of a CPU. It is mapped at the same address on all chips and can be
> accessed by any CPU of the system. To identify the chip from which the
> access is being done, the PowerBUS uses a 'chip' field in the
> load/store messages. QEMU does not model these messages, instead, we
> extract the chip id from the CPU PIR and do a lookup at the machine
> level to fetch the targeted interrupt controller.
> 
> Introduce pnv_get_chip() and pnv_xive_tm_get_xive() helpers to clarify
> this process in pnv_xive_get_tctx(). The latter will be removed in the
> subsequent patches but the same principle will be kept.
> 
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  include/hw/ppc/pnv.h | 13 +++++++++++++
>  hw/intc/pnv_xive.c   | 46 ++++++++++++++++++++++++++++----------------
>  2 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 58f4dcc0b71d..9b98b6afa31b 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -192,6 +192,19 @@ static inline bool pnv_is_power9(PnvMachineState *pnv)
>      return pnv_chip_is_power9(pnv->chips[0]);
>  }
>  
> +static inline PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)

I understand this has global scope because it may be used by
some other code, eg. PHB4 in your private branch, but I'm not
sure it is small enough to deserve the inline qualifier.

> +{
> +    int i;
> +
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        PnvChip *chip = pnv->chips[i];
> +        if (chip->chip_id == chip_id) {
> +            return chip;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  #define PNV_FDT_ADDR          0x01000000
>  #define PNV_TIMEBASE_FREQ     512000000ULL
>  
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 3ee28f00694a..d75053d0baad 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -378,6 +378,12 @@ static int cpu_pir(PowerPCCPU *cpu)
>      return env->spr_cb[SPR_PIR].default_value;
>  }
>  
> +static int cpu_chip_id(PowerPCCPU *cpu)
> +{
> +    int pir = cpu_pir(cpu);
> +    return (pir >> 8) & 0x7f;
> +}

Just a thought: it would be nice to have all pir<->ids conversion
functions grouped by CPU type in pnv.h (inline makes sense in this
case) along with the layout comment.

Anyway,

Reviewed-by: Greg Kurz <address@hidden>

> +
>  static bool pnv_xive_is_cpu_enabled(PnvXive *xive, PowerPCCPU *cpu)
>  {
>      int pir = cpu_pir(cpu);
> @@ -440,31 +446,37 @@ static int pnv_xive_match_nvt(XivePresenter *xptr, 
> uint8_t format,
>      return count;
>  }
>  
> +/*
> + * The TIMA MMIO space is shared among the chips and to identify the
> + * chip from which the access is being done, we extract the chip id
> + * from the PIR.
> + */
> +static PnvXive *pnv_xive_tm_get_xive(PowerPCCPU *cpu)
> +{
> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> +    PnvChip *chip;
> +    PnvXive *xive;
> +
> +    chip = pnv_get_chip(pnv, cpu_chip_id(cpu));
> +    assert(chip);
> +    xive = &PNV9_CHIP(chip)->xive;
> +
> +    if (!pnv_xive_is_cpu_enabled(xive, cpu)) {
> +        xive_error(xive, "IC: CPU %x is not enabled", cpu_pir(cpu));
> +    }
> +    return xive;
> +}
> +
>  static XiveTCTX *pnv_xive_get_tctx(XiveRouter *xrtr, CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
> -    PnvXive *xive = NULL;
> -    CPUPPCState *env = &cpu->env;
> -    int pir = env->spr_cb[SPR_PIR].default_value;
> +    PnvXive *xive = pnv_xive_tm_get_xive(cpu);
>  
> -    /*
> -     * Perform an extra check on the HW thread enablement.
> -     *
> -     * The TIMA is shared among the chips and to identify the chip
> -     * from which the access is being done, we extract the chip id
> -     * from the PIR.
> -     */
> -    xive = pnv_xive_get_ic((pir >> 8) & 0xf);
>      if (!xive) {
>          return NULL;
>      }
>  
> -    if (!(xive->regs[PC_THREAD_EN_REG0 >> 3] & PPC_BIT(pir & 0x3f))) {
> -        xive_error(PNV_XIVE(xrtr), "IC: CPU %x is not enabled", pir);
> -    }
> -
> -    return tctx;
> +    return XIVE_TCTX(pnv_cpu_state(cpu)->intc);
>  }
>  
>  /*