Re: [PATCH for-5.0 v5 10/23] ppc/pnv: Loop on the threads of the chip to find a matching NVT

[Greg Kurz <groug@kaod.org>]

On Fri, 15 Nov 2019 17:24:23 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> CPU_FOREACH() loops on all the CPUs of the machine which is incorrect.
> Each XIVE Presenter should scan only the HW threads of the chip it
> belongs to.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/pnv.h |  2 ++
>  hw/intc/pnv_xive.c   | 63 ++++++++++++++++++++++++++------------------
>  hw/ppc/pnv.c         |  2 +-
>  3 files changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 07c56c05ad30..58f4dcc0b71d 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -150,6 +150,8 @@ typedef struct PnvChipClass {
>   */
>  #define PNV_CHIP_HWID(i) ((((i) & 0x3e) << 3) | ((i) & 0x1))
>  
> +const char *pnv_chip_core_typename(const PnvChip *chip);
> +
>  /*
>   * Converts back a HW chip id to an index. This is useful to calculate
>   * the MMIO addresses of some controllers which depend on the chip id.
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 087cbfbaad48..71ca4961b6b1 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -377,34 +377,45 @@ static int pnv_xive_match_nvt(XivePresenter *xptr, uint8_t format,
>                                bool cam_ignore, uint8_t priority,
>                                uint32_t logic_serv, XiveTCTXMatch *match)
>  {
> -    CPUState *cs;
> +    PnvXive *xive = PNV_XIVE(xptr);
> +    PnvChip *chip = xive->chip;
> +    const char *typename = pnv_chip_core_typename(chip);
> +    size_t typesize = object_type_get_instance_size(typename);

Ahh the same boiler plate is needed again because the cores are
stored in an void *cores array in PnvChip... which is an ugly and
fragile pattern we got rid of in spapr (commit 94ad93bd976841). We
probably don't have the same risk here because the powernv machine
doesn't support hot-unplug, but this is still ugly anyway.

I certainly don't want to hold this series any longer for the sake
of Cedric's health :) but I'll be glad to turn the void *cores array
into a PnvCore **cores as a follow-up cleanup.

Rest LGTM.

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

>      int count = 0;
> -
> -    CPU_FOREACH(cs) {
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
> -        int ring;
> -
> -        /*
> -         * Check the thread context CAM lines and record matches.
> -         */
> -        ring = xive_presenter_tctx_match(xptr, tctx, format, nvt_blk, nvt_idx,
> -                                         cam_ignore, logic_serv);
> -        /*
> -         * Save the context and follow on to catch duplicates, that we
> -         * don't support yet.
> -         */
> -        if (ring != -1) {
> -            if (match->tctx) {
> -                qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a "
> -                              "thread context NVT %x/%x\n",
> -                              nvt_blk, nvt_idx);
> -                return -1;
> +    int i, j;
> +
> +    for (i = 0; i < chip->nr_cores; i++) {
> +        PnvCore *pc = PNV_CORE(chip->cores + i * typesize);
> +        CPUCore *cc = CPU_CORE(pc);
> +
> +        for (j = 0; j < cc->nr_threads; j++) {
> +            PowerPCCPU *cpu = pc->threads[j];
> +            XiveTCTX *tctx;
> +            int ring;
> +
> +            tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
> +
> +            /*
> +             * Check the thread context CAM lines and record matches.
> +             */
> +            ring = xive_presenter_tctx_match(xptr, tctx, format, nvt_blk,
> +                                             nvt_idx, cam_ignore, logic_serv);
> +            /*
> +             * Save the context and follow on to catch duplicates, that we
> +             * don't support yet.
> +             */
> +            if (ring != -1) {
> +                if (match->tctx) {
> +                    qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a "
> +                                  "thread context NVT %x/%x\n",
> +                                  nvt_blk, nvt_idx);
> +                    return -1;
> +                }
> +
> +                match->ring = ring;
> +                match->tctx = tctx;
> +                count++;
>              }
> -
> -            match->ring = ring;
> -            match->tctx = tctx;
> -            count++;
>          }
>      }
>  
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 57f924ba0466..94c9f536413f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -64,7 +64,7 @@
>  #define INITRD_LOAD_ADDR        0x60000000
>  #define INITRD_MAX_SIZE         (256 * MiB)
>  
> -static const char *pnv_chip_core_typename(const PnvChip *o)
> +const char *pnv_chip_core_typename(const PnvChip *o)
>  {
>      const char *chip_type = object_class_get_name(object_get_class(OBJECT(o)));
>      int len = strlen(chip_type) - strlen(PNV_CHIP_TYPE_SUFFIX);