On Fri, Feb 24, 2017 at 12:12:54PM +0100, Cédric Le Goater wrote:
> On 02/23/2017 03:29 AM, David Gibson wrote:
> > On Thu, Feb 16, 2017 at 02:47:31PM +0100, Cédric Le Goater wrote:
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/intc/xics.c | 26 ++++++++++++++------------
> >>  1 file changed, 14 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >> index 0ffdf09c5304..2decb921e4e3 100644
> >> --- a/hw/intc/xics.c
> >> +++ b/hw/intc/xics.c
> >> @@ -229,16 +229,15 @@ static void icp_check_ipi(ICPState *ss)
> >>      qemu_irq_raise(ss->output);
> >>  }
> >>  
> >> -static void icp_resend(ICPState *ss)
> >> +static void icp_resend(XICSInterface *xi, ICPState *ss)
> >>  {
> >> -    ICSState *ics;
> >> +    XICSInterfaceClass *xic = XICS_INTERFACE_GET_CLASS(xi);
> >>  
> >>      if (ss->mfrr < CPPR(ss)) {
> >>          icp_check_ipi(ss);
> >>      }
> >> -    QLIST_FOREACH(ics, &ss->xics->ics, list) {
> >> -        ics_resend(ics);
> >> -    }
> >> +
> >> +    xic->ics_resend(xi);
> >>  }
> >>  
> >>  void icp_set_cppr(ICPState *ss, uint8_t cppr)
> >> @@ -262,7 +261,7 @@ void icp_set_cppr(ICPState *ss, uint8_t cppr)
> >>          }
> >>      } else {
> >>          if (!XISR(ss)) {
> >> -            icp_resend(ss);
> >> +            icp_resend(XICS_INTERFACE(qdev_get_machine()), ss);
> > 
> > Here you're assuming that the machine is the implementor of the xics
> > interface, which is kinda ugly.  The ICP should have a pointer to the
> > xics interface, which will eventually replace the pointer to the
> > overall xics object it has now.
> 
> yes. I will try improve that. I don't like those calls to 
> qdev_get_machine()either. 
> 
> There are done in a couple of places though, under spapr_cpu_core
> to get XICS for instance.

Right, but I'm happier with it there, in code that's definitely
associated with a particular machine, rather than in the xics code
which is at least somewhat reusable.

> > But I haven't read the rest of the series yet, maybe this is just
> > transitional.
> > 
> >>          }
> >>      }
> >>  }
> >> @@ -299,6 +298,8 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr)
> >>  
> >>  void icp_eoi(ICPState *ss, uint32_t xirr)
> >>  {
> >> +    XICSInterface *xi = XICS_INTERFACE(qdev_get_machine());
> >> +    XICSInterfaceClass *xic = XICS_INTERFACE_GET_CLASS(xi);
> >>      ICSState *ics;
> >>      uint32_t irq;
> >>  
> >> @@ -306,13 +307,13 @@ void icp_eoi(ICPState *ss, uint32_t xirr)
> >>      ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK);
> >>      trace_xics_icp_eoi(ss->cs->cpu_index, xirr, ss->xirr);
> >>      irq = xirr & XISR_MASK;
> >> -    QLIST_FOREACH(ics, &ss->xics->ics, list) {
> >> -        if (ics_valid_irq(ics, irq)) {
> >> -            ics_eoi(ics, irq);
> >> -        }
> >> +
> >> +    ics = xic->ics_get(xi, irq);
> >> +    if (ics) {
> >> +        ics_eoi(ics, irq);
> >>      }
> >>      if (!XISR(ss)) {
> >> -        icp_resend(ss);
> >> +        icp_resend(xi, ss);
> >>      }
> >>  }
> >>  
> >> @@ -592,10 +593,11 @@ static void ics_simple_reset(DeviceState *dev)
> >>  
> >>  static int ics_simple_post_load(ICSState *ics, int version_id)
> >>  {
> >> +    XICSInterface *xi = XICS_INTERFACE(qdev_get_machine());
> >>      int i;
> >>  
> >>      for (i = 0; i < ics->xics->nr_servers; i++) {
> >> -        icp_resend(&ics->xics->ss[i]);
> >> +        icp_resend(xi, &ics->xics->ss[i]);
> >>      }
> > 
> > This resend triggering needs to get moved to the xics interface
> > implementor - i.e. the machine.  It's actually already broken right
> > now, since it incorrectly relies on the ordering of the ics and icp
> > restore during migration.
> 
> I'm adding a icp_resend() handler in patch 12 and using it patch 14.
> Maybe we can move the post_load() handler out of ICS simple now ? 
> 
> Thanks,
> 
> C. 
> 
>  
> >>      return 0;
> > 
> 

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