Re: [Qemu-devel] [PATCH v8 09/12] spapr: set the interrupt presenter at reset

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5401 bytes --]

On Mon, Dec 17, 2018 at 11:41:34PM +0100, Cédric Le Goater wrote:
> On 12/17/18 7:01 AM, David Gibson wrote:
> > On Thu, Dec 13, 2018 at 01:52:14PM +0100, Cédric Le Goater wrote:
> >> On 12/11/18 11:38 PM, Cédric Le Goater wrote:
> >>> Currently, the interrupt presenter of the vCPU is set at realize
> >>> time. Setting it at reset will become necessary when the new machine
> >>> supporting both interrupt modes is introduced. In this machine, the
> >>> interrupt mode is chosen at CAS time and activated after a reset.
> >>
> >> I think we need a second '->intc' pointer specific to XIVE instead. 
> >> How about ->intc_xive ?
> > 
> > Ah, interesting.  If we're going to need that, we might as well go
> > to specific ->icp and ->tctx pointers with the right types.
> 
> Hmm, PowerPCCPU is defined under target/ppc and adding the type might 
> add some noise. I will check.

It's a pointer, so you can just declare the type without a
definition.  That should be ok.

> > I don't particularly like having those machine specific pointers
> > within the cpu structure, but we can look at fixing that later by
> > reviving my patches to move various machine specific stuff within the
> > cpu into a separate sub-allocation.
> 
> ok.
> 
> Thanks,
> 
> C. 
> 
> >>
> >> We could just drop this patch and easly get rid of the XiveTCTX object 
> >> leak in spapr_unrealize_vcpu().
> >>
> >> C. 
> >>
> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>> ---
> >>>
> >>>  Changes since v7:
> >>>
> >>>  - introduced spapr_irq_reset_xics(). 
> >>>
> >>>  include/hw/ppc/spapr_cpu_core.h |  2 ++
> >>>  hw/ppc/spapr_cpu_core.c         | 26 ++++++++++++++++++++++++++
> >>>  hw/ppc/spapr_irq.c              | 13 +++++++++++++
> >>>  3 files changed, 41 insertions(+)
> >>>
> >>> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> >>> index 9e2821e4b31f..fc8ea9021656 100644
> >>> --- a/include/hw/ppc/spapr_cpu_core.h
> >>> +++ b/include/hw/ppc/spapr_cpu_core.h
> >>> @@ -53,4 +53,6 @@ static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
> >>>      return (sPAPRCPUState *)cpu->machine_data;
> >>>  }
> >>>  
> >>> +void spapr_cpu_core_set_intc(PowerPCCPU *cpu, const char *intc_type);
> >>> +
> >>>  #endif
> >>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> >>> index 82666436e9b4..afc5d4f4e739 100644
> >>> --- a/hw/ppc/spapr_cpu_core.c
> >>> +++ b/hw/ppc/spapr_cpu_core.c
> >>> @@ -397,3 +397,29 @@ static const TypeInfo spapr_cpu_core_type_infos[] = {
> >>>  };
> >>>  
> >>>  DEFINE_TYPES(spapr_cpu_core_type_infos)
> >>> +
> >>> +typedef struct ForeachFindIntCArgs {
> >>> +    const char *intc_type;
> >>> +    Object *intc;
> >>> +} ForeachFindIntCArgs;
> >>> +
> >>> +static int spapr_cpu_core_find_intc(Object *child, void *opaque)
> >>> +{
> >>> +    ForeachFindIntCArgs *args = opaque;
> >>> +
> >>> +    if (object_dynamic_cast(child, args->intc_type)) {
> >>> +        args->intc = child;
> >>> +    }
> >>> +
> >>> +    return args->intc != NULL;
> >>> +}
> >>> +
> >>> +void spapr_cpu_core_set_intc(PowerPCCPU *cpu, const char *intc_type)
> >>> +{
> >>> +    ForeachFindIntCArgs args = { intc_type, NULL };
> >>> +
> >>> +    object_child_foreach(OBJECT(cpu), spapr_cpu_core_find_intc, &args);
> >>> +    g_assert(args.intc);
> >>> +
> >>> +    cpu->intc = args.intc;
> >>> +}
> >>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> >>> index 0999a2b2d69c..814500f22d34 100644
> >>> --- a/hw/ppc/spapr_irq.c
> >>> +++ b/hw/ppc/spapr_irq.c
> >>> @@ -12,6 +12,7 @@
> >>>  #include "qemu/error-report.h"
> >>>  #include "qapi/error.h"
> >>>  #include "hw/ppc/spapr.h"
> >>> +#include "hw/ppc/spapr_cpu_core.h"
> >>>  #include "hw/ppc/spapr_xive.h"
> >>>  #include "hw/ppc/xics.h"
> >>>  #include "sysemu/kvm.h"
> >>> @@ -208,6 +209,15 @@ static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
> >>>      return 0;
> >>>  }
> >>>  
> >>> +static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
> >>> +{
> >>> +   CPUState *cs;
> >>> +
> >>> +    CPU_FOREACH(cs) {
> >>> +        spapr_cpu_core_set_intc(POWERPC_CPU(cs), spapr->icp_type);
> >>> +    }
> >>> +}
> >>> +
> >>>  #define SPAPR_IRQ_XICS_NR_IRQS     0x1000
> >>>  #define SPAPR_IRQ_XICS_NR_MSIS     \
> >>>      (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> >>> @@ -225,6 +235,7 @@ sPAPRIrq spapr_irq_xics = {
> >>>      .dt_populate = spapr_dt_xics,
> >>>      .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
> >>>      .post_load   = spapr_irq_post_load_xics,
> >>> +    .reset       = spapr_irq_reset_xics,
> >>>  };
> >>>  
> >>>  /*
> >>> @@ -325,6 +336,8 @@ static void spapr_irq_reset_xive(sPAPRMachineState *spapr, Error **errp)
> >>>      CPU_FOREACH(cs) {
> >>>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>>  
> >>> +        spapr_cpu_core_set_intc(cpu, TYPE_XIVE_TCTX);
> >>> +
> >>>          /* (TCG) Set the OS CAM line of the thread interrupt context. */
> >>>          spapr_xive_set_tctx_os_cam(XIVE_TCTX(cpu->intc));
> >>>      }
> >>>
> >>
> > 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]