Re: [Qemu-devel] [PATCH v3 17/19] spapr_irq: Expose the phandle of the interrupt controller

[Greg Kurz]

On Fri, 18 Jan 2019 14:46:42 +0100
Cédric Le Goater <address@hidden> wrote:

> On 1/17/19 6:16 PM, Greg Kurz wrote:
> > This will be used by PHB hotplug in order to create the "interrupt-map"
> > property of the PHB node.
> > 
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  hw/ppc/spapr_irq.c         |   44 
> > +++++++++++++++++++++++++++++++++++++++++++-
> >  include/hw/ppc/spapr_irq.h |    3 +++
> >  2 files changed, 46 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index b3e0713428ed..93243cfd0644 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -243,6 +243,11 @@ static void spapr_irq_reset_xics(sPAPRMachineState 
> > *spapr, Error **errp)
> >      /* TODO: create the KVM XICS device */
> >  }
> >  
> > +static const char *spapr_irq_get_nodename_xics(sPAPRMachineState *spapr)
> > +{
> > +    return xics_spapr_get_nodename();  
> 
> can't we just return XICS_NODENAME ?
> 

Sure.

> > +}
> > +
> >  #define SPAPR_IRQ_XICS_NR_IRQS     0x1000
> >  #define SPAPR_IRQ_XICS_NR_MSIS     \
> >      (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> > @@ -263,6 +268,7 @@ sPAPRIrq spapr_irq_xics = {
> >      .post_load   = spapr_irq_post_load_xics,
> >      .reset       = spapr_irq_reset_xics,
> >      .set_irq     = spapr_irq_set_irq_xics,
> > +    .get_nodename = spapr_irq_get_nodename_xics,
> >  };
> >  
> >  /*
> > @@ -401,6 +407,11 @@ static void spapr_irq_set_irq_xive(void *opaque, int 
> > srcno, int val)
> >      xive_source_set_irq(&spapr->xive->source, srcno, val);
> >  }
> >  
> > +static const char *spapr_irq_get_nodename_xive(sPAPRMachineState *spapr)
> > +{
> > +    return spapr_xive_get_nodename(spapr->xive);
> > +}
> > +
> >  /*
> >   * XIVE uses the full IRQ number space. Set it to 8K to be compatible
> >   * with XICS.
> > @@ -425,6 +436,7 @@ sPAPRIrq spapr_irq_xive = {
> >      .post_load   = spapr_irq_post_load_xive,
> >      .reset       = spapr_irq_reset_xive,
> >      .set_irq     = spapr_irq_set_irq_xive,
> > +    .get_nodename = spapr_irq_get_nodename_xive,
> >  };
> >  
> >  /*
> > @@ -588,6 +600,11 @@ static void spapr_irq_set_irq_dual(void *opaque, int 
> > srcno, int val)
> >      spapr_irq_current(spapr)->set_irq(spapr, srcno, val);
> >  }
> >  
> > +static const char *spapr_irq_get_nodename_dual(sPAPRMachineState *spapr)
> > +{
> > +    return spapr_irq_current(spapr)->get_nodename(spapr);
> > +}
> > +
> >  /*
> >   * Define values in sync with the XIVE and XICS backend
> >   */
> > @@ -609,7 +626,8 @@ sPAPRIrq spapr_irq_dual = {
> >      .cpu_intc_create = spapr_irq_cpu_intc_create_dual,
> >      .post_load   = spapr_irq_post_load_dual,
> >      .reset       = spapr_irq_reset_dual,
> > -    .set_irq     = spapr_irq_set_irq_dual
> > +    .set_irq     = spapr_irq_set_irq_dual,
> > +    .get_nodename = spapr_irq_get_nodename_dual,
> >  };
> >  
> >  /*
> > @@ -680,6 +698,29 @@ void spapr_irq_dt_populate(sPAPRMachineState *spapr, 
> > uint32_t nr_servers,
> >      _FDT(fdt_setprop_cell(fdt, node, "phandle", phandle));
> >  }
> >  
> > +uint32_t spapr_irq_get_phandle(sPAPRMachineState *spapr, void *fdt,
> > +                               Error **errp)
> > +{
> > +    const char *nodename = spapr->irq->get_nodename(spapr);
> > +    int phandle, offset;
> > +
> > +    offset = fdt_subnode_offset(fdt, 0, nodename);  
> 
> if I am correct, you changed dt_populate() to return the same offset.
> 
> > +    if (offset < 0) {
> > +        error_setg(errp, "Can't find node \"%s\": %s", nodename,
> > +                   fdt_strerror(offset));
> > +        return -1;
> > +    }
> > +
> > +    phandle = fdt_get_phandle(fdt, offset);  
> 
> if we stored the phandle when spapr_irq_dt_populate() is called, I think
> things would be easier. May be under the backend ? 
> 

Yes we can do that but SLOF may now update the fdt, so we need
to be able to update the phandle if it got changed. I'll look
how to do that cleanly in v4.

> C.
> 
> 
> > +    if (phandle < 0) {
> > +        error_setg(errp, "Can't get phandle of node \"%s\": %s",
> > +                   nodename, fdt_strerror(phandle));
> > +        return -1;
> > +    }
> > +
> > +    return phandle;
> > +}
> > +
> >  /*
> >   * XICS legacy routines - to deprecate one day
> >   */
> > @@ -752,4 +793,5 @@ sPAPRIrq spapr_irq_xics_legacy = {
> >      .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
> >      .post_load   = spapr_irq_post_load_xics,
> >      .set_irq     = spapr_irq_set_irq_xics,
> > +    .get_nodename = spapr_irq_get_nodename_xics,
> >  };
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index dfed6b7e9976..d44d659231a6 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -48,6 +48,7 @@ typedef struct sPAPRIrq {
> >      int (*post_load)(sPAPRMachineState *spapr, int version_id);
> >      void (*reset)(sPAPRMachineState *spapr, Error **errp);
> >      void (*set_irq)(void *opaque, int srcno, int val);
> > +    const char *(*get_nodename)(sPAPRMachineState *spapr);
> >  } sPAPRIrq;
> >  
> >  extern sPAPRIrq spapr_irq_xics;
> > @@ -65,6 +66,8 @@ int spapr_irq_post_load(sPAPRMachineState *spapr, int 
> > version_id);
> >  void spapr_irq_reset(sPAPRMachineState *spapr, Error **errp);
> >  void spapr_irq_dt_populate(sPAPRMachineState *spapr, uint32_t nr_servers,
> >                             void *fdt, uint32_t phandle);
> > +uint32_t spapr_irq_get_phandle(sPAPRMachineState *spapr, void *fdt,
> > +                               Error **errp);
> >  
> >  /*
> >   * XICS legacy routines
> >   
>