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 <[email protected]> > >> --- > >> 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
signature.asc
Description: PGP signature
