| Message ID | 1487252865-12064-9-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
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. 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. > return 0;
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. > 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; >
>>> @@ -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 ? Could you give me a little more info on what should be done ? I lack context on this problem. So should we call : ICPState *ss = opaque; ICPStateClass *info = ICP_GET_CLASS(ss); if (info->post_load) { return info->post_load(ss, version_id); } and then ICSState *ics = opaque; ICSStateClass *info = ICS_BASE_GET_CLASS(ics); if (info->post_load) { return info->post_load(ics, version_id); } from spapr_post_load() ? Thanks, C.
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; > > >
On Fri, Feb 24, 2017 at 06:34:06PM +0100, Cédric Le Goater wrote: > >>> @@ -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 ? > > Could you give me a little more info on what should be done ? I lack > context on this problem. > > So should we call : > > ICPState *ss = opaque; > ICPStateClass *info = ICP_GET_CLASS(ss); > > if (info->post_load) { > return info->post_load(ss, version_id); > } > > and then > > ICSState *ics = opaque; > ICSStateClass *info = ICS_BASE_GET_CLASS(ics); > > if (info->post_load) { > return info->post_load(ics, version_id); > } > > from spapr_post_load() ? Uh.. now I'm trying to remember what the correct order is. Any part of the post_load which is entirely local to the ics or icp, and doesn't communicate with the other part should remain in a local post-load handler. The part of the resend which involves interaction between ics and icp should move to the mahine. Because the ics and icp are both descendents of the machine, the migration core does guarantee their local post_lodas will run before the machine's post-load. At the moment however, we require either the ics post_loads to run before the icp or the other way around - I forget which. Because these are "cousin" objects, that order is not guaranteed by the migration core.
>>>> 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. I fixed that with the backlink on the XICSFabric. C.
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); } } } @@ -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]); } return 0;
Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/xics.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)