Re: [Qemu-devel] [PATCH 02/10] ppc/xive: introduce a XiveTCTX pointer under PowerPCCPU

[David Gibson]

On Wed, Jan 02, 2019 at 06:57:35AM +0100, Cédric Le Goater wrote:
> which will be used by the machine only when the XIVE interrupt mode is
> in use.

I don't love the idea of putting a hook this specific into the
PowerPCCPU structure, though it might be the easiest path in the short
term.

A couple of approaches: 1) revisit my changes to allow for a pointer
to machine-defined per-cpu data.  or 2) do we actually need a cpu to
tctx pointer.

Expanding on (2) - here you use the pointer to find the right TIMA
state to access, but that could also be handled by having different
TIMA IO instances and mapping those individually to cpu_as.  On the
interrupt delivery side I think a tctx to cpu link will suffice.  For
sPAPR there might be complications with translating cpu numbers in
hcalls to the right tctx.

> 
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  target/ppc/cpu.h        | 2 ++
>  hw/intc/xive.c          | 6 +++---
>  hw/ppc/spapr_cpu_core.c | 7 ++++++-
>  hw/ppc/spapr_irq.c      | 8 ++++----
>  4 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index d5f99f1fc7b9..c76036985623 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1177,6 +1177,7 @@ do {                                            \
>  
>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> +typedef struct XiveTCTX XiveTCTX;
>  
>  /**
>   * PowerPCCPU:
> @@ -1196,6 +1197,7 @@ struct PowerPCCPU {
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
>      Object *intc;
> +    XiveTCTX *tctx;
>      void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index ea33494338dc..410c53278a11 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -321,7 +321,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>                            uint64_t value, unsigned size)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
> -    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
> +    XiveTCTX *tctx = cpu->tctx;
>      const XiveTmOp *xto;
>  
>      /*
> @@ -360,7 +360,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>  static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
> -    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
> +    XiveTCTX *tctx = cpu->tctx;
>      const XiveTmOp *xto;
>  
>      /*
> @@ -1186,7 +1186,7 @@ static bool xive_presenter_match(XiveRouter *xrtr, 
> uint8_t format,
>  
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
> +        XiveTCTX *tctx = cpu->tctx;
>          int ring;
>  
>          /*
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2739b2a4b818..1473ef853336 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -194,7 +194,12 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, 
> sPAPRCPUCore *sc)
>          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, 
> cpu->machine_data);
>      }
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
> -    object_unparent(cpu->intc);
> +    if (cpu->intc) {
> +        object_unparent(cpu->intc);
> +    }
> +    if (cpu->tctx) {
> +        object_unparent(OBJECT(cpu->tctx));
> +    }
>      cpu_remove_sync(CPU(cpu));
>      object_unparent(OBJECT(cpu));
>  }
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 2d2e17b66533..8d028db44ff4 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -305,7 +305,7 @@ static void spapr_irq_print_info_xive(sPAPRMachineState 
> *spapr,
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        xive_tctx_pic_print_info(XIVE_TCTX(cpu->intc), mon);
> +        xive_tctx_pic_print_info(cpu->tctx, mon);
>      }
>  
>      spapr_xive_pic_print_info(spapr->xive, mon);
> @@ -323,13 +323,13 @@ static void 
> spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    cpu->intc = obj;
> +    cpu->tctx = XIVE_TCTX(obj);
>  
>      /*
>       * (TCG) Early setting the OS CAM line for hotplugged CPUs as they
>       * don't beneficiate from the reset of the XIVE IRQ backend
>       */
> -    spapr_xive_set_tctx_os_cam(XIVE_TCTX(obj));
> +    spapr_xive_set_tctx_os_cam(cpu->tctx);
>  }
>  
>  static int spapr_irq_post_load_xive(sPAPRMachineState *spapr, int version_id)
> @@ -345,7 +345,7 @@ static void spapr_irq_reset_xive(sPAPRMachineState 
> *spapr, Error **errp)
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
>          /* (TCG) Set the OS CAM line of the thread interrupt context. */
> -        spapr_xive_set_tctx_os_cam(XIVE_TCTX(cpu->intc));
> +        spapr_xive_set_tctx_os_cam(cpu->tctx);
>      }
>  }
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature