| Message ID | 157010406203.246126.13381271918474281392.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Use less XIVE HW resources in KVM | expand |
On 03/10/2019 14:01, Greg Kurz wrote: > The sPAPR XIVE object has an nr_ends field which happens to be a > multiple of spapr_max_server_number(). It is currently set with > the help of "nr-ends" property. This is a bit unfortunate since > it exposes to the sPAPR irq frontend what should remain an > implemantation detail within the XIVE backend. implementation > It will be possible soon to inform the XIVE KVM device about the > range of VCPU ids that may be used in the VM, as returned by the > spapr_max_server_number() function. This will allow the device > to substantially reduce the consumption of scarce resources > in the XIVE HW. > > For both reasons, replace the "nr-ends" property with an "nr-servers" > one. The existing nr_ends field must be kept though since it tells how > many ENDs are migrated, it is derived from "nr-servers" at realize time > for simplicity. Convert spapr_dt_xive() to use it as well. Looks good. one question below. > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/intc/spapr_xive.c | 21 ++++++++++++++++----- > hw/ppc/spapr_irq.c | 2 +- > include/hw/ppc/spapr_xive.h | 1 + > 3 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index 04879abf2e7a..62888ddc68db 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -99,6 +99,15 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, > return 0; > } > > +static uint32_t spapr_xive_vcpu_id_to_end_idx(uint32_t vcpu_id) may be use a simpler macro : #define spapr_xive_cpu_end_idx(vcpu, prio) (((vcpu) << 3) + prio) > +{ > + /* > + * 8 XIVE END structures per CPU. One for each available > + * priority > + */ > + return vcpu_id << 3; > +} > + > static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > uint8_t *out_end_blk, uint32_t *out_end_idx) > { > @@ -109,7 +118,7 @@ static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > } > > if (out_end_idx) { > - *out_end_idx = (cpu->vcpu_id << 3) + prio; > + *out_end_idx = spapr_xive_vcpu_id_to_end_idx(cpu->vcpu_id) + prio; > } > } > > @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > return; > } > > - if (!xive->nr_ends) { > - error_setg(errp, "Number of interrupt needs to be greater 0"); > + if (!xive->nr_servers) { > + error_setg(errp, "Number of interrupt servers must be greater than 0"); > return; > } > > + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); > + > /* > * Initialize the internal sources, for IPIs and virtual devices. > */ > @@ -489,7 +500,7 @@ static const VMStateDescription vmstate_spapr_xive = { > > static Property spapr_xive_properties[] = { > DEFINE_PROP_UINT32("nr-irqs", SpaprXive, nr_irqs, 0), > - DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0), > + DEFINE_PROP_UINT32("nr-servers", SpaprXive, nr_servers, 0), > DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE), > DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE), > DEFINE_PROP_END_OF_LIST(), > @@ -1550,7 +1561,7 @@ void spapr_dt_xive(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, we should remove the 'uint32_t nr_servers' parameter from spapr_dt_xive() then ? > /* Interrupt number ranges for the IPIs */ > uint32_t lisn_ranges[] = { > cpu_to_be32(0), > - cpu_to_be32(nr_servers), > + cpu_to_be32(xive->nr_servers), > }; > /* > * EQ size - the sizes of pages supported by the system 4K, 64K, > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 457eabe24cda..025fd00143a2 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -591,7 +591,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > * 8 XIVE END structures per CPU. One for each available > * priority > */ > - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > + qdev_prop_set_uint32(dev, "nr-servers", nr_servers); > qdev_init_nofail(dev); > > spapr->xive = SPAPR_XIVE(dev); > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > index 0df20a6590a5..4a4a6fc6be7f 100644 > --- a/include/hw/ppc/spapr_xive.h > +++ b/include/hw/ppc/spapr_xive.h > @@ -22,6 +22,7 @@ typedef struct SpaprXive { > /* Internal interrupt source for IPIs and virtual devices */ > XiveSource source; > hwaddr vc_base; > + uint32_t nr_servers; > > /* END ESB MMIOs */ > XiveENDSource end_source; >
On Thu, 3 Oct 2019 14:21:59 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 03/10/2019 14:01, Greg Kurz wrote: > > The sPAPR XIVE object has an nr_ends field which happens to be a > > multiple of spapr_max_server_number(). It is currently set with > > the help of "nr-ends" property. This is a bit unfortunate since > > it exposes to the sPAPR irq frontend what should remain an > > implemantation detail within the XIVE backend. > > implementation > oops > > It will be possible soon to inform the XIVE KVM device about the > > range of VCPU ids that may be used in the VM, as returned by the > > spapr_max_server_number() function. This will allow the device > > to substantially reduce the consumption of scarce resources > > in the XIVE HW. > > > > For both reasons, replace the "nr-ends" property with an "nr-servers" > > one. The existing nr_ends field must be kept though since it tells how > > many ENDs are migrated, it is derived from "nr-servers" at realize time > > for simplicity. Convert spapr_dt_xive() to use it as well. > > Looks good. one question below. > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/intc/spapr_xive.c | 21 ++++++++++++++++----- > > hw/ppc/spapr_irq.c | 2 +- > > include/hw/ppc/spapr_xive.h | 1 + > > 3 files changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > index 04879abf2e7a..62888ddc68db 100644 > > --- a/hw/intc/spapr_xive.c > > +++ b/hw/intc/spapr_xive.c > > @@ -99,6 +99,15 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, > > return 0; > > } > > > > +static uint32_t spapr_xive_vcpu_id_to_end_idx(uint32_t vcpu_id) > > > may be use a simpler macro : > > #define spapr_xive_cpu_end_idx(vcpu, prio) (((vcpu) << 3) + prio) > Will do but I'll keep the comment. > > +{ > > + /* > > + * 8 XIVE END structures per CPU. One for each available > > + * priority > > + */ > > + return vcpu_id << 3; > > +} > > + > > static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > > uint8_t *out_end_blk, uint32_t *out_end_idx) > > { > > @@ -109,7 +118,7 @@ static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > > } > > > > if (out_end_idx) { > > - *out_end_idx = (cpu->vcpu_id << 3) + prio; > > + *out_end_idx = spapr_xive_vcpu_id_to_end_idx(cpu->vcpu_id) + prio; > > } > > } > > > > @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > > return; > > } > > > > - if (!xive->nr_ends) { > > - error_setg(errp, "Number of interrupt needs to be greater 0"); > > + if (!xive->nr_servers) { > > + error_setg(errp, "Number of interrupt servers must be greater than 0"); > > return; > > } > > > > + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); > > + > > /* > > * Initialize the internal sources, for IPIs and virtual devices. > > */ > > @@ -489,7 +500,7 @@ static const VMStateDescription vmstate_spapr_xive = { > > > > static Property spapr_xive_properties[] = { > > DEFINE_PROP_UINT32("nr-irqs", SpaprXive, nr_irqs, 0), > > - DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0), > > + DEFINE_PROP_UINT32("nr-servers", SpaprXive, nr_servers, 0), > > DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE), > > DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE), > > DEFINE_PROP_END_OF_LIST(), > > @@ -1550,7 +1561,7 @@ void spapr_dt_xive(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > > we should remove the 'uint32_t nr_servers' parameter from spapr_dt_xive() > then ? > Yes but this would also affect XICS since this function is a backend method. It is hence done in the next patch. > > /* Interrupt number ranges for the IPIs */ > > uint32_t lisn_ranges[] = { > > cpu_to_be32(0), > > - cpu_to_be32(nr_servers), > > + cpu_to_be32(xive->nr_servers), > > }; > > /* > > * EQ size - the sizes of pages supported by the system 4K, 64K, > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > index 457eabe24cda..025fd00143a2 100644 > > --- a/hw/ppc/spapr_irq.c > > +++ b/hw/ppc/spapr_irq.c > > @@ -591,7 +591,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > * 8 XIVE END structures per CPU. One for each available > > * priority > > */ > > - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > > + qdev_prop_set_uint32(dev, "nr-servers", nr_servers); > > qdev_init_nofail(dev); > > > > spapr->xive = SPAPR_XIVE(dev); > > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > > index 0df20a6590a5..4a4a6fc6be7f 100644 > > --- a/include/hw/ppc/spapr_xive.h > > +++ b/include/hw/ppc/spapr_xive.h > > @@ -22,6 +22,7 @@ typedef struct SpaprXive { > > /* Internal interrupt source for IPIs and virtual devices */ > > XiveSource source; > > hwaddr vc_base; > > + uint32_t nr_servers; > > > > /* END ESB MMIOs */ > > XiveENDSource end_source; > > >
On Thu, Oct 03, 2019 at 02:01:13PM +0200, Greg Kurz wrote: > The sPAPR XIVE object has an nr_ends field which happens to be a > multiple of spapr_max_server_number(). It is currently set with > the help of "nr-ends" property. This is a bit unfortunate since > it exposes to the sPAPR irq frontend what should remain an > implemantation detail within the XIVE backend. > > It will be possible soon to inform the XIVE KVM device about the > range of VCPU ids that may be used in the VM, as returned by the > spapr_max_server_number() function. This will allow the device > to substantially reduce the consumption of scarce resources > in the XIVE HW. > > For both reasons, replace the "nr-ends" property with an "nr-servers" > one. The existing nr_ends field must be kept though since it tells how > many ENDs are migrated, it is derived from "nr-servers" at realize time > for simplicity. Convert spapr_dt_xive() to use it as well. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/intc/spapr_xive.c | 21 ++++++++++++++++----- > hw/ppc/spapr_irq.c | 2 +- > include/hw/ppc/spapr_xive.h | 1 + > 3 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index 04879abf2e7a..62888ddc68db 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -99,6 +99,15 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, > return 0; > } > > +static uint32_t spapr_xive_vcpu_id_to_end_idx(uint32_t vcpu_id) > +{ > + /* > + * 8 XIVE END structures per CPU. One for each available > + * priority > + */ > + return vcpu_id << 3; > +} > + > static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > uint8_t *out_end_blk, uint32_t *out_end_idx) > { > @@ -109,7 +118,7 @@ static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > } > > if (out_end_idx) { > - *out_end_idx = (cpu->vcpu_id << 3) + prio; > + *out_end_idx = spapr_xive_vcpu_id_to_end_idx(cpu->vcpu_id) + prio; > } > } > > @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > return; > } > > - if (!xive->nr_ends) { > - error_setg(errp, "Number of interrupt needs to be greater 0"); > + if (!xive->nr_servers) { > + error_setg(errp, "Number of interrupt servers must be greater than 0"); > return; > } > > + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); I'd prefer not to store both nr_servers and nr_servers * 8 in the structure. I think you just want xive->nr_servers, then derive it any any places that current look at xive->nr_ends. > /* > * Initialize the internal sources, for IPIs and virtual devices. > */ > @@ -489,7 +500,7 @@ static const VMStateDescription vmstate_spapr_xive = { > > static Property spapr_xive_properties[] = { > DEFINE_PROP_UINT32("nr-irqs", SpaprXive, nr_irqs, 0), > - DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0), > + DEFINE_PROP_UINT32("nr-servers", SpaprXive, nr_servers, 0), Technically speaking the user can reach in using -global and modify QOM properties like this, so this is arguably an interface breakage. That said, I've always thought it was kind of a problem that the way QOM is used internally thereby exposes as interface a bunch of things that are really intended to be internal. So... I'm inclined to go ahead with this anyway. I won't tell if you don't. > DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE), > DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE), > DEFINE_PROP_END_OF_LIST(), > @@ -1550,7 +1561,7 @@ void spapr_dt_xive(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > /* Interrupt number ranges for the IPIs */ > uint32_t lisn_ranges[] = { > cpu_to_be32(0), > - cpu_to_be32(nr_servers), > + cpu_to_be32(xive->nr_servers), > }; > /* > * EQ size - the sizes of pages supported by the system 4K, 64K, > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 457eabe24cda..025fd00143a2 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -591,7 +591,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > * 8 XIVE END structures per CPU. One for each available > * priority > */ > - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > + qdev_prop_set_uint32(dev, "nr-servers", nr_servers); > qdev_init_nofail(dev); > > spapr->xive = SPAPR_XIVE(dev); > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > index 0df20a6590a5..4a4a6fc6be7f 100644 > --- a/include/hw/ppc/spapr_xive.h > +++ b/include/hw/ppc/spapr_xive.h > @@ -22,6 +22,7 @@ typedef struct SpaprXive { > /* Internal interrupt source for IPIs and virtual devices */ > XiveSource source; > hwaddr vc_base; > + uint32_t nr_servers; This is a basic paraneter, not really related to the internal source structure, so I'd move it up above that comment there. > > /* END ESB MMIOs */ > XiveENDSource end_source; >
>> @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> - if (!xive->nr_ends) { >> - error_setg(errp, "Number of interrupt needs to be greater 0"); >> + if (!xive->nr_servers) { >> + error_setg(errp, "Number of interrupt servers must be greater than 0"); >> return; >> } >> >> + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); > > I'd prefer not to store both nr_servers and nr_servers * 8 in the > structure. I think you just want xive->nr_servers, then derive it any > any places that current look at xive->nr_ends. Yes I agree. This is a small change. C.
On Fri, 4 Oct 2019 14:07:25 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Oct 03, 2019 at 02:01:13PM +0200, Greg Kurz wrote: > > The sPAPR XIVE object has an nr_ends field which happens to be a > > multiple of spapr_max_server_number(). It is currently set with > > the help of "nr-ends" property. This is a bit unfortunate since > > it exposes to the sPAPR irq frontend what should remain an > > implemantation detail within the XIVE backend. > > > > It will be possible soon to inform the XIVE KVM device about the > > range of VCPU ids that may be used in the VM, as returned by the > > spapr_max_server_number() function. This will allow the device > > to substantially reduce the consumption of scarce resources > > in the XIVE HW. > > > > For both reasons, replace the "nr-ends" property with an "nr-servers" > > one. The existing nr_ends field must be kept though since it tells how > > many ENDs are migrated, it is derived from "nr-servers" at realize time > > for simplicity. Convert spapr_dt_xive() to use it as well. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/intc/spapr_xive.c | 21 ++++++++++++++++----- > > hw/ppc/spapr_irq.c | 2 +- > > include/hw/ppc/spapr_xive.h | 1 + > > 3 files changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > index 04879abf2e7a..62888ddc68db 100644 > > --- a/hw/intc/spapr_xive.c > > +++ b/hw/intc/spapr_xive.c > > @@ -99,6 +99,15 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, > > return 0; > > } > > > > +static uint32_t spapr_xive_vcpu_id_to_end_idx(uint32_t vcpu_id) > > +{ > > + /* > > + * 8 XIVE END structures per CPU. One for each available > > + * priority > > + */ > > + return vcpu_id << 3; > > +} > > + > > static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > > uint8_t *out_end_blk, uint32_t *out_end_idx) > > { > > @@ -109,7 +118,7 @@ static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > > } > > > > if (out_end_idx) { > > - *out_end_idx = (cpu->vcpu_id << 3) + prio; > > + *out_end_idx = spapr_xive_vcpu_id_to_end_idx(cpu->vcpu_id) + prio; > > } > > } > > > > @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > > return; > > } > > > > - if (!xive->nr_ends) { > > - error_setg(errp, "Number of interrupt needs to be greater 0"); > > + if (!xive->nr_servers) { > > + error_setg(errp, "Number of interrupt servers must be greater than 0"); > > return; > > } > > > > + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); > > I'd prefer not to store both nr_servers and nr_servers * 8 in the > structure. I think you just want xive->nr_servers, then derive it any > any places that current look at xive->nr_ends. > Of course but unfortunately nr_ends is involved in migration: static const VMStateDescription vmstate_spapr_xive = { .name = TYPE_SPAPR_XIVE, .version_id = 1, .minimum_version_id = 1, .pre_save = vmstate_spapr_xive_pre_save, .post_load = NULL, /* handled at the machine level */ .fields = (VMStateField[]) { VMSTATE_UINT32_EQUAL(nr_irqs, SpaprXive, NULL), VMSTATE_STRUCT_VARRAY_POINTER_UINT32(eat, SpaprXive, nr_irqs, vmstate_spapr_xive_eas, XiveEAS), VMSTATE_STRUCT_VARRAY_POINTER_UINT32(endt, SpaprXive, nr_ends, ^^^^^^^^^^ vmstate_spapr_xive_end, XiveEND), VMSTATE_END_OF_LIST() }, }; and we certainly cannot put nr_servers << 3 here. I suppose I should emphasize that even more in the changelog. We could possibly rename nr_ends to mig_nr_ends, and possibly only compute at migration time, but it doesn't buy much IMHO. > > /* > > * Initialize the internal sources, for IPIs and virtual devices. > > */ > > @@ -489,7 +500,7 @@ static const VMStateDescription vmstate_spapr_xive = { > > > > static Property spapr_xive_properties[] = { > > DEFINE_PROP_UINT32("nr-irqs", SpaprXive, nr_irqs, 0), > > - DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0), > > + DEFINE_PROP_UINT32("nr-servers", SpaprXive, nr_servers, 0), > > Technically speaking the user can reach in using -global and modify > QOM properties like this, so this is arguably an interface breakage. Drat, I didn't think about this one... :-\ but it seems that "spapr-xive" isn't user creatable and doesn't appear in the output of '-device help'. Not sure how -global behaves in this case... > That said, I've always thought it was kind of a problem that the way > QOM is used internally thereby exposes as interface a bunch of things > that are really intended to be internal. > I tend to agree, nr-ends is really an internal detail that a typical sPAPR user should probably never see. > So... I'm inclined to go ahead with this anyway. I won't tell if you > don't. > > > DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE), > > DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE), > > DEFINE_PROP_END_OF_LIST(), > > @@ -1550,7 +1561,7 @@ void spapr_dt_xive(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > > /* Interrupt number ranges for the IPIs */ > > uint32_t lisn_ranges[] = { > > cpu_to_be32(0), > > - cpu_to_be32(nr_servers), > > + cpu_to_be32(xive->nr_servers), > > }; > > /* > > * EQ size - the sizes of pages supported by the system 4K, 64K, > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > index 457eabe24cda..025fd00143a2 100644 > > --- a/hw/ppc/spapr_irq.c > > +++ b/hw/ppc/spapr_irq.c > > @@ -591,7 +591,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > * 8 XIVE END structures per CPU. One for each available > > * priority > > */ > > - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > > + qdev_prop_set_uint32(dev, "nr-servers", nr_servers); > > qdev_init_nofail(dev); > > > > spapr->xive = SPAPR_XIVE(dev); > > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > > index 0df20a6590a5..4a4a6fc6be7f 100644 > > --- a/include/hw/ppc/spapr_xive.h > > +++ b/include/hw/ppc/spapr_xive.h > > @@ -22,6 +22,7 @@ typedef struct SpaprXive { > > /* Internal interrupt source for IPIs and virtual devices */ > > XiveSource source; > > hwaddr vc_base; > > + uint32_t nr_servers; > > This is a basic paraneter, not really related to the internal source > structure, so I'd move it up above that comment there. > Sure. > > > > /* END ESB MMIOs */ > > XiveENDSource end_source; > > >
On Fri, 4 Oct 2019 07:53:13 +0200 Cédric Le Goater <clg@kaod.org> wrote: > >> @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > >> return; > >> } > >> > >> - if (!xive->nr_ends) { > >> - error_setg(errp, "Number of interrupt needs to be greater 0"); > >> + if (!xive->nr_servers) { > >> + error_setg(errp, "Number of interrupt servers must be greater than 0"); > >> return; > >> } > >> > >> + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); > > > > I'd prefer not to store both nr_servers and nr_servers * 8 in the > > structure. I think you just want xive->nr_servers, then derive it any > > any places that current look at xive->nr_ends. > > Yes I agree. This is a small change. > I'm afraid it isn't that simple (see my other mail). > C.
On 04/10/2019 08:52, Greg Kurz wrote: > On Fri, 4 Oct 2019 07:53:13 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >>>> @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) >>>> return; >>>> } >>>> >>>> - if (!xive->nr_ends) { >>>> - error_setg(errp, "Number of interrupt needs to be greater 0"); >>>> + if (!xive->nr_servers) { >>>> + error_setg(errp, "Number of interrupt servers must be greater than 0"); >>>> return; >>>> } >>>> >>>> + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); >>> >>> I'd prefer not to store both nr_servers and nr_servers * 8 in the >>> structure. I think you just want xive->nr_servers, then derive it any >>> any places that current look at xive->nr_ends. >> >> Yes I agree. This is a small change. >> > > I'm afraid it isn't that simple (see my other mail). yes. I had forgotten about the vmstate ...
On Fri, Oct 04, 2019 at 08:51:19AM +0200, Greg Kurz wrote: > On Fri, 4 Oct 2019 14:07:25 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Thu, Oct 03, 2019 at 02:01:13PM +0200, Greg Kurz wrote: > > > The sPAPR XIVE object has an nr_ends field which happens to be a > > > multiple of spapr_max_server_number(). It is currently set with > > > the help of "nr-ends" property. This is a bit unfortunate since > > > it exposes to the sPAPR irq frontend what should remain an > > > implemantation detail within the XIVE backend. > > > > > > It will be possible soon to inform the XIVE KVM device about the > > > range of VCPU ids that may be used in the VM, as returned by the > > > spapr_max_server_number() function. This will allow the device > > > to substantially reduce the consumption of scarce resources > > > in the XIVE HW. > > > > > > For both reasons, replace the "nr-ends" property with an "nr-servers" > > > one. The existing nr_ends field must be kept though since it tells how > > > many ENDs are migrated, it is derived from "nr-servers" at realize time > > > for simplicity. Convert spapr_dt_xive() to use it as well. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/intc/spapr_xive.c | 21 ++++++++++++++++----- > > > hw/ppc/spapr_irq.c | 2 +- > > > include/hw/ppc/spapr_xive.h | 1 + > > > 3 files changed, 18 insertions(+), 6 deletions(-) > > > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > > index 04879abf2e7a..62888ddc68db 100644 > > > --- a/hw/intc/spapr_xive.c > > > +++ b/hw/intc/spapr_xive.c > > > @@ -99,6 +99,15 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, > > > return 0; > > > } > > > > > > +static uint32_t spapr_xive_vcpu_id_to_end_idx(uint32_t vcpu_id) > > > +{ > > > + /* > > > + * 8 XIVE END structures per CPU. One for each available > > > + * priority > > > + */ > > > + return vcpu_id << 3; > > > +} > > > + > > > static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > > > uint8_t *out_end_blk, uint32_t *out_end_idx) > > > { > > > @@ -109,7 +118,7 @@ static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, > > > } > > > > > > if (out_end_idx) { > > > - *out_end_idx = (cpu->vcpu_id << 3) + prio; > > > + *out_end_idx = spapr_xive_vcpu_id_to_end_idx(cpu->vcpu_id) + prio; > > > } > > > } > > > > > > @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > > > return; > > > } > > > > > > - if (!xive->nr_ends) { > > > - error_setg(errp, "Number of interrupt needs to be greater 0"); > > > + if (!xive->nr_servers) { > > > + error_setg(errp, "Number of interrupt servers must be greater than 0"); > > > return; > > > } > > > > > > + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); > > > > I'd prefer not to store both nr_servers and nr_servers * 8 in the > > structure. I think you just want xive->nr_servers, then derive it any > > any places that current look at xive->nr_ends. > > > > Of course but unfortunately nr_ends is involved in migration: > > static const VMStateDescription vmstate_spapr_xive = { > .name = TYPE_SPAPR_XIVE, > .version_id = 1, > .minimum_version_id = 1, > .pre_save = vmstate_spapr_xive_pre_save, > .post_load = NULL, /* handled at the machine level */ > .fields = (VMStateField[]) { > VMSTATE_UINT32_EQUAL(nr_irqs, SpaprXive, NULL), > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(eat, SpaprXive, nr_irqs, > vmstate_spapr_xive_eas, XiveEAS), > VMSTATE_STRUCT_VARRAY_POINTER_UINT32(endt, SpaprXive, nr_ends, > ^^^^^^^^^^ > vmstate_spapr_xive_end, XiveEND), > VMSTATE_END_OF_LIST() > }, > }; > > and we certainly cannot put nr_servers << 3 here. I suppose I should > emphasize that even more in the changelog. Ah, dangit, I forgot about that, though I've hit the same problem before. I'm actually less concerned about a comment in the changelog, than in on the definition in the structure itself. > We could possibly rename nr_ends to mig_nr_ends, and possibly only > compute at migration time, but it doesn't buy much IMHO. No, indeed not. > > > /* > > > * Initialize the internal sources, for IPIs and virtual devices. > > > */ > > > @@ -489,7 +500,7 @@ static const VMStateDescription vmstate_spapr_xive = { > > > > > > static Property spapr_xive_properties[] = { > > > DEFINE_PROP_UINT32("nr-irqs", SpaprXive, nr_irqs, 0), > > > - DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0), > > > + DEFINE_PROP_UINT32("nr-servers", SpaprXive, nr_servers, 0), > > > > Technically speaking the user can reach in using -global and modify > > QOM properties like this, so this is arguably an interface breakage. > > Drat, I didn't think about this one... :-\ but it seems that "spapr-xive" > isn't user creatable and doesn't appear in the output of '-device help'. > Not sure how -global behaves in this case... > > > That said, I've always thought it was kind of a problem that the way > > QOM is used internally thereby exposes as interface a bunch of things > > that are really intended to be internal. > > > > I tend to agree, nr-ends is really an internal detail that a typical > sPAPR user should probably never see. > > > So... I'm inclined to go ahead with this anyway. I won't tell if you > > don't. > > > > > DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE), > > > DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE), > > > DEFINE_PROP_END_OF_LIST(), > > > @@ -1550,7 +1561,7 @@ void spapr_dt_xive(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, > > > /* Interrupt number ranges for the IPIs */ > > > uint32_t lisn_ranges[] = { > > > cpu_to_be32(0), > > > - cpu_to_be32(nr_servers), > > > + cpu_to_be32(xive->nr_servers), > > > }; > > > /* > > > * EQ size - the sizes of pages supported by the system 4K, 64K, > > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > > index 457eabe24cda..025fd00143a2 100644 > > > --- a/hw/ppc/spapr_irq.c > > > +++ b/hw/ppc/spapr_irq.c > > > @@ -591,7 +591,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > > * 8 XIVE END structures per CPU. One for each available > > > * priority > > > */ > > > - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); > > > + qdev_prop_set_uint32(dev, "nr-servers", nr_servers); > > > qdev_init_nofail(dev); > > > > > > spapr->xive = SPAPR_XIVE(dev); > > > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > > > index 0df20a6590a5..4a4a6fc6be7f 100644 > > > --- a/include/hw/ppc/spapr_xive.h > > > +++ b/include/hw/ppc/spapr_xive.h > > > @@ -22,6 +22,7 @@ typedef struct SpaprXive { > > > /* Internal interrupt source for IPIs and virtual devices */ > > > XiveSource source; > > > hwaddr vc_base; > > > + uint32_t nr_servers; > > > > This is a basic paraneter, not really related to the internal source > > structure, so I'd move it up above that comment there. > > > > Sure. > > > > > > > /* END ESB MMIOs */ > > > XiveENDSource end_source; > > > > > >
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index 04879abf2e7a..62888ddc68db 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -99,6 +99,15 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, return 0; } +static uint32_t spapr_xive_vcpu_id_to_end_idx(uint32_t vcpu_id) +{ + /* + * 8 XIVE END structures per CPU. One for each available + * priority + */ + return vcpu_id << 3; +} + static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, uint8_t *out_end_blk, uint32_t *out_end_idx) { @@ -109,7 +118,7 @@ static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio, } if (out_end_idx) { - *out_end_idx = (cpu->vcpu_id << 3) + prio; + *out_end_idx = spapr_xive_vcpu_id_to_end_idx(cpu->vcpu_id) + prio; } } @@ -283,11 +292,13 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) return; } - if (!xive->nr_ends) { - error_setg(errp, "Number of interrupt needs to be greater 0"); + if (!xive->nr_servers) { + error_setg(errp, "Number of interrupt servers must be greater than 0"); return; } + xive->nr_ends = spapr_xive_vcpu_id_to_end_idx(xive->nr_servers); + /* * Initialize the internal sources, for IPIs and virtual devices. */ @@ -489,7 +500,7 @@ static const VMStateDescription vmstate_spapr_xive = { static Property spapr_xive_properties[] = { DEFINE_PROP_UINT32("nr-irqs", SpaprXive, nr_irqs, 0), - DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0), + DEFINE_PROP_UINT32("nr-servers", SpaprXive, nr_servers, 0), DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE), DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE), DEFINE_PROP_END_OF_LIST(), @@ -1550,7 +1561,7 @@ void spapr_dt_xive(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt, /* Interrupt number ranges for the IPIs */ uint32_t lisn_ranges[] = { cpu_to_be32(0), - cpu_to_be32(nr_servers), + cpu_to_be32(xive->nr_servers), }; /* * EQ size - the sizes of pages supported by the system 4K, 64K, diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index 457eabe24cda..025fd00143a2 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -591,7 +591,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) * 8 XIVE END structures per CPU. One for each available * priority */ - qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); + qdev_prop_set_uint32(dev, "nr-servers", nr_servers); qdev_init_nofail(dev); spapr->xive = SPAPR_XIVE(dev); diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h index 0df20a6590a5..4a4a6fc6be7f 100644 --- a/include/hw/ppc/spapr_xive.h +++ b/include/hw/ppc/spapr_xive.h @@ -22,6 +22,7 @@ typedef struct SpaprXive { /* Internal interrupt source for IPIs and virtual devices */ XiveSource source; hwaddr vc_base; + uint32_t nr_servers; /* END ESB MMIOs */ XiveENDSource end_source;
The sPAPR XIVE object has an nr_ends field which happens to be a multiple of spapr_max_server_number(). It is currently set with the help of "nr-ends" property. This is a bit unfortunate since it exposes to the sPAPR irq frontend what should remain an implemantation detail within the XIVE backend. It will be possible soon to inform the XIVE KVM device about the range of VCPU ids that may be used in the VM, as returned by the spapr_max_server_number() function. This will allow the device to substantially reduce the consumption of scarce resources in the XIVE HW. For both reasons, replace the "nr-ends" property with an "nr-servers" one. The existing nr_ends field must be kept though since it tells how many ENDs are migrated, it is derived from "nr-servers" at realize time for simplicity. Convert spapr_dt_xive() to use it as well. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/intc/spapr_xive.c | 21 ++++++++++++++++----- hw/ppc/spapr_irq.c | 2 +- include/hw/ppc/spapr_xive.h | 1 + 3 files changed, 18 insertions(+), 6 deletions(-)