Message ID | 20190718115420.19919-10-clg@kaod.org |
---|---|
State | New |
Headers | show |
Series | ppc/pnv: add XIVE support for KVM guests | expand |
On Thu, Jul 18, 2019 at 01:54:12PM +0200, Cédric Le Goater wrote: > This is to perform lookups in the NVT table when a vCPU is dispatched > and possibily resend interrupts. > > Future XIVE chip will use a different class for the model of the > interrupt controller and we might need to change the type of > 'XiveRouter *' to 'Object *' > > Signed-off-by: Cédric Le Goater <clg@kaod.org> Hrm. This still bothers me. AIUI there can be multiple XiveRouters in the system, yes? And at least theoretically can present irqs from multiple routers? In which case what's the rule for which one should be associated with a specific. I guess it's the one on the same chip, but that needs to be explained up front, with some justification of why that's the relevant one. > --- > include/hw/ppc/xive.h | 2 ++ > hw/intc/xive.c | 9 +++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > index 4851ff87e795..206b23ecfab3 100644 > --- a/include/hw/ppc/xive.h > +++ b/include/hw/ppc/xive.h > @@ -320,6 +320,8 @@ typedef struct XiveTCTX { > qemu_irq os_output; > > uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE]; > + > + struct XiveRouter *xrtr; > } XiveTCTX; > > /* > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 88f2e560db0f..1b0eccb6df40 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -573,6 +573,14 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) > Object *obj; > Error *local_err = NULL; > > + obj = object_property_get_link(OBJECT(dev), "xrtr", &local_err); > + if (!obj) { > + error_propagate(errp, local_err); > + error_prepend(errp, "required link 'xrtr' not found: "); > + return; > + } > + tctx->xrtr = XIVE_ROUTER(obj); > + > obj = object_property_get_link(OBJECT(dev), "cpu", &local_err); > if (!obj) { > error_propagate(errp, local_err); > @@ -666,6 +674,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp) > object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort); > object_unref(obj); > object_property_add_const_link(obj, "cpu", cpu, &error_abort); > + object_property_add_const_link(obj, "xrtr", OBJECT(xrtr), &error_abort); > object_property_set_bool(obj, true, "realized", &local_err); > if (local_err) { > goto error;
On 28/07/2019 09:46, David Gibson wrote: > On Thu, Jul 18, 2019 at 01:54:12PM +0200, Cédric Le Goater wrote: >> This is to perform lookups in the NVT table when a vCPU is dispatched >> and possibily resend interrupts. >> >> Future XIVE chip will use a different class for the model of the >> interrupt controller and we might need to change the type of >> 'XiveRouter *' to 'Object *' >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > Hrm. This still bothers me. Your feeling is right. There should be a good reason to link two objects together as it can be an issue later on (such P10). It should not be an hidden parameter to function calls. this is more or less the case. See below for more explanation. > AIUI there can be multiple XiveRouters in the system, yes? yes and it works relatively well with 4 chips. I say relatively because the presenter model is taking a shortcut we should fix. > And at least theoretically can present irqs from multiple routers? Yes. the console being the most simple example. We only have one device per system on the LPC bus of chip 0. > In which case what's the rule for which one should be associated with > a specific. > I guess it's the one on the same chip, but that needs to be explained > up front, with some justification of why that's the relevant one. Yes. we try to minimize the traffic on the PowerBUS so generally CPU targets are on the same IC. The EAT on POWER10 should be on the same HW chip. I think we can address the proposed changes from another perspective, from the presenter one. this is cleaner and reflects better the HW design. The thread contexts are owned by the presenter. It can scan its list when doing CAM matching and when the thread context registers are being accessed by a CPU. Adding a XiveRouter parameter to all the TIMA operations seems like a better option and solves the problem. The thread context registers are modeled under the CPU object today because it was practical for sPAPR but on HW, these are SRAM entries, one for each HW thread of the chip. So may be, we should have introduced an other table under the XiveRouter to model the contexts but there was no real need for the XIVE POWER9 IC of the pseries machine. This design might be reaching its limits with PowerNV and POWER10. Looking at : [PATCH v2 15/17] ppc/pnv: Grab the XiveRouter object from XiveTCTX in pnv_xive_get_tctx() we see that the code adds an extra check on the THREAD_ENABLE registers and for that, its needs the IC to which belongs the thread context. This code is wrong. We should not be need to find a XiveRouter object from a XiveRouter handler. This is because the xive_presenter_match() routine does: CPU_FOREACH(cs) { XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs); we should be, instead, looping on the different IC of the system looking for a match. Something else to fix. I think I will use the PIR to match the CPU of a chip. Looking at POWER10, XIVE internal structures have changed and we will need to introduce new IC models, one for PowerNV obviously but surely also one for pseries. A third one ... yes, sorry about that. If we go in that direction, it would be good to have a common XiveTCTX and not link it to a specific XiveRouter (P9 or P10). Another good reason not to use that link. So I will rework the end of that patchset. Thanks for having given me time to think more about it before merging. I did more experiments and the models are now more precise, specially with guest and multichip support. C. > >> --- >> include/hw/ppc/xive.h | 2 ++ >> hw/intc/xive.c | 9 +++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h >> index 4851ff87e795..206b23ecfab3 100644 >> --- a/include/hw/ppc/xive.h >> +++ b/include/hw/ppc/xive.h >> @@ -320,6 +320,8 @@ typedef struct XiveTCTX { >> qemu_irq os_output; >> >> uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE]; >> + >> + struct XiveRouter *xrtr; >> } XiveTCTX; >> >> /* >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c >> index 88f2e560db0f..1b0eccb6df40 100644 >> --- a/hw/intc/xive.c >> +++ b/hw/intc/xive.c >> @@ -573,6 +573,14 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) >> Object *obj; >> Error *local_err = NULL; >> >> + obj = object_property_get_link(OBJECT(dev), "xrtr", &local_err); >> + if (!obj) { >> + error_propagate(errp, local_err); >> + error_prepend(errp, "required link 'xrtr' not found: "); >> + return; >> + } >> + tctx->xrtr = XIVE_ROUTER(obj); >> + >> obj = object_property_get_link(OBJECT(dev), "cpu", &local_err); >> if (!obj) { >> error_propagate(errp, local_err); >> @@ -666,6 +674,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp) >> object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort); >> object_unref(obj); >> object_property_add_const_link(obj, "cpu", cpu, &error_abort); >> + object_property_add_const_link(obj, "xrtr", OBJECT(xrtr), &error_abort); >> object_property_set_bool(obj, true, "realized", &local_err); >> if (local_err) { >> goto error; >
On Sun, Jul 28, 2019 at 11:06:27AM +0200, Cédric Le Goater wrote: > On 28/07/2019 09:46, David Gibson wrote: > > On Thu, Jul 18, 2019 at 01:54:12PM +0200, Cédric Le Goater wrote: > >> This is to perform lookups in the NVT table when a vCPU is dispatched > >> and possibily resend interrupts. > >> > >> Future XIVE chip will use a different class for the model of the > >> interrupt controller and we might need to change the type of > >> 'XiveRouter *' to 'Object *' > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > > > Hrm. This still bothers me. > > Your feeling is right. There should be a good reason to link two objects > together as it can be an issue later on (such P10). It should not be an > hidden parameter to function calls. this is more or less the case. > > See below for more explanation. > > > AIUI there can be multiple XiveRouters in the system, yes? > > yes and it works relatively well with 4 chips. I say relatively because > the presenter model is taking a shortcut we should fix. > > > And at least theoretically can present irqs from multiple routers? > > Yes. the console being the most simple example. We only have one device > per system on the LPC bus of chip 0. > > > In which case what's the rule for which one should be associated with > > a specific. > > I guess it's the one on the same chip, but that needs to be explained > > up front, with some justification of why that's the relevant one. > > Yes. we try to minimize the traffic on the PowerBUS so generally CPU > targets are on the same IC. The EAT on POWER10 should be on the same > HW chip. > > > I think we can address the proposed changes from another perspective, > from the presenter one. this is cleaner and reflects better the HW design. > > The thread contexts are owned by the presenter. It can scan its list > when doing CAM matching and when the thread context registers are being > accessed by a CPU. Adding a XiveRouter parameter to all the TIMA > operations seems like a better option and solves the problem. > > > The thread context registers are modeled under the CPU object today > because it was practical for sPAPR but on HW, these are SRAM entries, > one for each HW thread of the chip. So may be, we should have introduced > an other table under the XiveRouter to model the contexts but there > was no real need for the XIVE POWER9 IC of the pseries machine. This > design might be reaching its limits with PowerNV and POWER10. > > > Looking at : > > [PATCH v2 15/17] ppc/pnv: Grab the XiveRouter object from XiveTCTX in pnv_xive_get_tctx() > > we see that the code adds an extra check on the THREAD_ENABLE registers > and for that, its needs the IC to which belongs the thread context. This > code is wrong. We should not be need to find a XiveRouter object from a > XiveRouter handler. > > This is because the xive_presenter_match() routine does: > > CPU_FOREACH(cs) { > XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs); > > we should be, instead, looping on the different IC of the system > looking for a match. Something else to fix. I think I will use the > PIR to match the CPU of a chip. > > > Looking at POWER10, XIVE internal structures have changed and we will > need to introduce new IC models, one for PowerNV obviously but surely > also one for pseries. A third one ... yes, sorry about that. If we go > in that direction, it would be good to have a common XiveTCTX and not > link it to a specific XiveRouter (P9 or P10). Another good reason not > to use that link. > > > So I will rework the end of that patchset. Thanks for having given me > time to think more about it before merging. I did more experiments and > the models are now more precise, specially with guest and multichip > support. Ok, good to hear. I will wait for the respin.
On 29/07/2019 08:11, David Gibson wrote: > On Sun, Jul 28, 2019 at 11:06:27AM +0200, Cédric Le Goater wrote: >> On 28/07/2019 09:46, David Gibson wrote: >>> On Thu, Jul 18, 2019 at 01:54:12PM +0200, Cédric Le Goater wrote: >>>> This is to perform lookups in the NVT table when a vCPU is dispatched >>>> and possibily resend interrupts. >>>> >>>> Future XIVE chip will use a different class for the model of the >>>> interrupt controller and we might need to change the type of >>>> 'XiveRouter *' to 'Object *' >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> >>> Hrm. This still bothers me. >> >> Your feeling is right. There should be a good reason to link two objects >> together as it can be an issue later on (such P10). It should not be an >> hidden parameter to function calls. this is more or less the case. >> >> See below for more explanation. >> >>> AIUI there can be multiple XiveRouters in the system, yes? >> >> yes and it works relatively well with 4 chips. I say relatively because >> the presenter model is taking a shortcut we should fix. >> >>> And at least theoretically can present irqs from multiple routers? >> >> Yes. the console being the most simple example. We only have one device >> per system on the LPC bus of chip 0. >> >>> In which case what's the rule for which one should be associated with >>> a specific. >>> I guess it's the one on the same chip, but that needs to be explained >>> up front, with some justification of why that's the relevant one. >> >> Yes. we try to minimize the traffic on the PowerBUS so generally CPU >> targets are on the same IC. The EAT on POWER10 should be on the same >> HW chip. >> >> >> I think we can address the proposed changes from another perspective, >> from the presenter one. this is cleaner and reflects better the HW design. >> >> The thread contexts are owned by the presenter. It can scan its list >> when doing CAM matching and when the thread context registers are being >> accessed by a CPU. Adding a XiveRouter parameter to all the TIMA >> operations seems like a better option and solves the problem. >> >> >> The thread context registers are modeled under the CPU object today >> because it was practical for sPAPR but on HW, these are SRAM entries, >> one for each HW thread of the chip. So may be, we should have introduced >> an other table under the XiveRouter to model the contexts but there >> was no real need for the XIVE POWER9 IC of the pseries machine. This >> design might be reaching its limits with PowerNV and POWER10. >> >> >> Looking at : >> >> [PATCH v2 15/17] ppc/pnv: Grab the XiveRouter object from XiveTCTX in pnv_xive_get_tctx() >> >> we see that the code adds an extra check on the THREAD_ENABLE registers >> and for that, its needs the IC to which belongs the thread context. This >> code is wrong. We should not be need to find a XiveRouter object from a >> XiveRouter handler. >> >> This is because the xive_presenter_match() routine does: >> >> CPU_FOREACH(cs) { >> XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs); >> >> we should be, instead, looping on the different IC of the system >> looking for a match. Something else to fix. I think I will use the >> PIR to match the CPU of a chip. >> >> >> Looking at POWER10, XIVE internal structures have changed and we will >> need to introduce new IC models, one for PowerNV obviously but surely >> also one for pseries. A third one ... yes, sorry about that. If we go >> in that direction, it would be good to have a common XiveTCTX and not >> link it to a specific XiveRouter (P9 or P10). Another good reason not >> to use that link. >> >> >> So I will rework the end of that patchset. Thanks for having given me >> time to think more about it before merging. I did more experiments and >> the models are now more precise, specially with guest and multichip >> support. > > Ok, good to hear. I will wait for the respin. > Could you check the patch adding the powernv{8,9} machines ? I am rebasing the respin on top of this patch and introducing a new handler to the machine to perform the CAM line matching. Thanks, C.
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h index 4851ff87e795..206b23ecfab3 100644 --- a/include/hw/ppc/xive.h +++ b/include/hw/ppc/xive.h @@ -320,6 +320,8 @@ typedef struct XiveTCTX { qemu_irq os_output; uint8_t regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE]; + + struct XiveRouter *xrtr; } XiveTCTX; /* diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 88f2e560db0f..1b0eccb6df40 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -573,6 +573,14 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) Object *obj; Error *local_err = NULL; + obj = object_property_get_link(OBJECT(dev), "xrtr", &local_err); + if (!obj) { + error_propagate(errp, local_err); + error_prepend(errp, "required link 'xrtr' not found: "); + return; + } + tctx->xrtr = XIVE_ROUTER(obj); + obj = object_property_get_link(OBJECT(dev), "cpu", &local_err); if (!obj) { error_propagate(errp, local_err); @@ -666,6 +674,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp) object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort); object_unref(obj); object_property_add_const_link(obj, "cpu", cpu, &error_abort); + object_property_add_const_link(obj, "xrtr", OBJECT(xrtr), &error_abort); object_property_set_bool(obj, true, "realized", &local_err); if (local_err) { goto error;
This is to perform lookups in the NVT table when a vCPU is dispatched and possibily resend interrupts. Future XIVE chip will use a different class for the model of the interrupt controller and we might need to change the type of 'XiveRouter *' to 'Object *' Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ppc/xive.h | 2 ++ hw/intc/xive.c | 9 +++++++++ 2 files changed, 11 insertions(+)