| Message ID | 20180821043343.7514-26-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/26] spapr_cpu_core: vmstate_[un]register per-CPU data from (un)realizefn | expand |
On 21 August 2018 at 05:33, David Gibson <david@gibson.dropbear.id.au> wrote: > From: Cédric Le Goater <clg@kaod.org> > > It should save us some CPU cycles as these routines perform a lot of > checks. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_pci.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) Hi; Coverity points out in CID 1395183 that there's a bug in this part of this patch: > @@ -1558,6 +1559,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > sPAPRMachineState *spapr = > (sPAPRMachineState *) object_dynamic_cast(qdev_get_machine(), > TYPE_SPAPR_MACHINE); > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); This has moved the call to SPAPR_MACHINE_GET_CLASS() above the check for "is spapr NULL", which is wrong, because it will unconditionally dereference the pointer you pass to it. thanks -- PMM
On 08/24/2018 05:09 PM, Peter Maydell wrote: > On 21 August 2018 at 05:33, David Gibson <david@gibson.dropbear.id.au> wrote: >> From: Cédric Le Goater <clg@kaod.org> >> >> It should save us some CPU cycles as these routines perform a lot of >> checks. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> hw/ppc/spapr_pci.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) > > Hi; Coverity points out in CID 1395183 that there's a bug in > this part of this patch: > >> @@ -1558,6 +1559,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> sPAPRMachineState *spapr = >> (sPAPRMachineState *) object_dynamic_cast(qdev_get_machine(), >> TYPE_SPAPR_MACHINE); >> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); > > This has moved the call to SPAPR_MACHINE_GET_CLASS() above > the check for "is spapr NULL", which is wrong, because it > will unconditionally dereference the pointer you pass to it. I see. This is a simple fix but the root reason for this check is commit f7d6bfcdc0fe ("spapr_pci: fail gracefully with non-pseries machine types"). Is there a way to specify which device type can or can not be plugged on a machine ? I suppose we cannot use : machine_class_allow_dynamic_sysbus_dev() for cold plugged devices. Or can we ? That would be better. Thanks, C.
On Fri, 24 Aug 2018 17:30:12 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 08/24/2018 05:09 PM, Peter Maydell wrote: > > On 21 August 2018 at 05:33, David Gibson <david@gibson.dropbear.id.au> wrote: > >> From: Cédric Le Goater <clg@kaod.org> > >> > >> It should save us some CPU cycles as these routines perform a lot of > >> checks. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >> --- > >> hw/ppc/spapr_pci.c | 11 ++++++----- > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > > > Hi; Coverity points out in CID 1395183 that there's a bug in > > this part of this patch: > > > >> @@ -1558,6 +1559,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > >> sPAPRMachineState *spapr = > >> (sPAPRMachineState *) object_dynamic_cast(qdev_get_machine(), > >> TYPE_SPAPR_MACHINE); > >> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); > > > > This has moved the call to SPAPR_MACHINE_GET_CLASS() above > > the check for "is spapr NULL", which is wrong, because it > > will unconditionally dereference the pointer you pass to it. > I've sent a trivial patch to fix this. > I see. This is a simple fix but the root reason for this check is > commit f7d6bfcdc0fe ("spapr_pci: fail gracefully with non-pseries > machine types"). > Yeah, we also have one in spapr_cpu_core_realize() for the very same reason. > Is there a way to specify which device type can or can not be > plugged on a machine ? > > I suppose we cannot use : > > machine_class_allow_dynamic_sysbus_dev() > > for cold plugged devices. Or can we ? That would be better. > Hmm... not sure this would help. The root problem is that many places in spapr_pci and spapr_cpu_core assume the machine is sPAPR. > Thanks, > > C.
On 08/24/2018 05:38 PM, Greg Kurz wrote: > On Fri, 24 Aug 2018 17:30:12 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >> On 08/24/2018 05:09 PM, Peter Maydell wrote: >>> On 21 August 2018 at 05:33, David Gibson <david@gibson.dropbear.id.au> wrote: >>>> From: Cédric Le Goater <clg@kaod.org> >>>> >>>> It should save us some CPU cycles as these routines perform a lot of >>>> checks. >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >>>> --- >>>> hw/ppc/spapr_pci.c | 11 ++++++----- >>>> 1 file changed, 6 insertions(+), 5 deletions(-) >>> >>> Hi; Coverity points out in CID 1395183 that there's a bug in >>> this part of this patch: >>> >>>> @@ -1558,6 +1559,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >>>> sPAPRMachineState *spapr = >>>> (sPAPRMachineState *) object_dynamic_cast(qdev_get_machine(), >>>> TYPE_SPAPR_MACHINE); >>>> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); >>> >>> This has moved the call to SPAPR_MACHINE_GET_CLASS() above >>> the check for "is spapr NULL", which is wrong, because it >>> will unconditionally dereference the pointer you pass to it. >> > > I've sent a trivial patch to fix this. yes. Looks good. But, > >> I see. This is a simple fix but the root reason for this check is >> commit f7d6bfcdc0fe ("spapr_pci: fail gracefully with non-pseries >> machine types"). >> > > Yeah, we also have one in spapr_cpu_core_realize() for the very > same reason. > >> Is there a way to specify which device type can or can not be >> plugged on a machine ? >> >> I suppose we cannot use : >> >> machine_class_allow_dynamic_sysbus_dev() >> >> for cold plugged devices. Or can we ? That would be better. >> > > Hmm... not sure this would help. The root problem is that many > places in spapr_pci and spapr_cpu_core assume the machine is > sPAPR. which is a perfectly legitimate assumption for a sPAPR only device, same for spapr_cpu_core. I would think. Shouldn't we enforce the restriction at the machine level instead and not at the device level ? I thought that was the purpose of commit 0bd1909da606 ("machine: Replace has_dynamic_sysbus with list of allowed devices"), to make sure machines had a predefined list of user-creatable devices. I might be missing something. Thanks, C.
On 2018-08-24 18:43, Cédric Le Goater wrote: > On 08/24/2018 05:38 PM, Greg Kurz wrote: >> On Fri, 24 Aug 2018 17:30:12 +0200 >> Cédric Le Goater <clg@kaod.org> wrote: >> >>> On 08/24/2018 05:09 PM, Peter Maydell wrote: >>>> On 21 August 2018 at 05:33, David Gibson <david@gibson.dropbear.id.au> wrote: >>>>> From: Cédric Le Goater <clg@kaod.org> [...] >>> Is there a way to specify which device type can or can not be >>> plugged on a machine ? >>> >>> I suppose we cannot use : >>> >>> machine_class_allow_dynamic_sysbus_dev() >>> >>> for cold plugged devices. Or can we ? That would be better. >>> >> >> Hmm... not sure this would help. The root problem is that many >> places in spapr_pci and spapr_cpu_core assume the machine is >> sPAPR. > > which is a perfectly legitimate assumption for a sPAPR only device, > same for spapr_cpu_core. I would think. Shouldn't we enforce > the restriction at the machine level instead and not at the device > level ? > > I thought that was the purpose of commit 0bd1909da606 ("machine: > Replace has_dynamic_sysbus with list of allowed devices"), to > make sure machines had a predefined list of user-creatable devices. The "spapr-pci-host-bridge" is explicitly marked with "dc->user_creatable = true" - so it is creatable everywhere. You could try whether it is possible to make it only creatable via the white list instead ... not sure whether that works though, since there is a class hierarchy (TYPE_PCI_HOST_BRIDGE) in between? Thomas
On Mon, 27 Aug 2018 08:21:48 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 2018-08-24 18:43, Cédric Le Goater wrote: > > On 08/24/2018 05:38 PM, Greg Kurz wrote: > >> On Fri, 24 Aug 2018 17:30:12 +0200 > >> Cédric Le Goater <clg@kaod.org> wrote: > >> > >>> On 08/24/2018 05:09 PM, Peter Maydell wrote: > >>>> On 21 August 2018 at 05:33, David Gibson <david@gibson.dropbear.id.au> wrote: > >>>>> From: Cédric Le Goater <clg@kaod.org> > [...] > >>> Is there a way to specify which device type can or can not be > >>> plugged on a machine ? > >>> > >>> I suppose we cannot use : > >>> > >>> machine_class_allow_dynamic_sysbus_dev() > >>> > >>> for cold plugged devices. Or can we ? That would be better. > >>> > >> > >> Hmm... not sure this would help. The root problem is that many > >> places in spapr_pci and spapr_cpu_core assume the machine is > >> sPAPR. > > > > which is a perfectly legitimate assumption for a sPAPR only device, > > same for spapr_cpu_core. I would think. Shouldn't we enforce > > the restriction at the machine level instead and not at the device > > level ? > > > > I thought that was the purpose of commit 0bd1909da606 ("machine: > > Replace has_dynamic_sysbus with list of allowed devices"), to > > make sure machines had a predefined list of user-creatable devices. > > The "spapr-pci-host-bridge" is explicitly marked with > "dc->user_creatable = true" - so it is creatable everywhere. You could > try whether it is possible to make it only creatable via the white list > instead Hmm... how would you do that ? > ... not sure whether that works though, since there is a class > hierarchy (TYPE_PCI_HOST_BRIDGE) in between? > Also, as said above, we have the very same problem with spapr_cpu_core, which is definitely not a sysbus device... Cheers, -- Greg > Thomas
On Mon, 27 Aug 2018 11:03:39 +0200 Greg Kurz <groug@kaod.org> wrote: > On Mon, 27 Aug 2018 08:21:48 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > On 2018-08-24 18:43, Cédric Le Goater wrote: > > > On 08/24/2018 05:38 PM, Greg Kurz wrote: > > >> On Fri, 24 Aug 2018 17:30:12 +0200 > > >> Cédric Le Goater <clg@kaod.org> wrote: > > >> > > >>> On 08/24/2018 05:09 PM, Peter Maydell wrote: > > >>>> On 21 August 2018 at 05:33, David Gibson <david@gibson.dropbear.id.au> wrote: > > >>>>> From: Cédric Le Goater <clg@kaod.org> > > [...] > > >>> Is there a way to specify which device type can or can not be > > >>> plugged on a machine ? > > >>> > > >>> I suppose we cannot use : > > >>> > > >>> machine_class_allow_dynamic_sysbus_dev() > > >>> > > >>> for cold plugged devices. Or can we ? That would be better. > > >>> > > >> > > >> Hmm... not sure this would help. The root problem is that many > > >> places in spapr_pci and spapr_cpu_core assume the machine is > > >> sPAPR. > > > > > > which is a perfectly legitimate assumption for a sPAPR only device, > > > same for spapr_cpu_core. I would think. Shouldn't we enforce > > > the restriction at the machine level instead and not at the device > > > level ? > > > > > > I thought that was the purpose of commit 0bd1909da606 ("machine: > > > Replace has_dynamic_sysbus with list of allowed devices"), to > > > make sure machines had a predefined list of user-creatable devices. > > > > The "spapr-pci-host-bridge" is explicitly marked with > > "dc->user_creatable = true" - so it is creatable everywhere. You could > > try whether it is possible to make it only creatable via the white list > > instead > > Hmm... how would you do that ? > The white list is checked in machine_init_notify() which gets called way after spapr_phb_realize()... we can't rely on this to check the machine and the PHB are compatible. Maybe add a dedicated bus for the PHBs in the spapr machine ? > > ... not sure whether that works though, since there is a class > > hierarchy (TYPE_PCI_HOST_BRIDGE) in between? > > > > Also, as said above, we have the very same problem with spapr_cpu_core, > which is definitely not a sysbus device... > > Cheers, > > -- > Greg > > > Thomas > >
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 3791ced6c5..5cd676e443 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -267,6 +267,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr, target_ulong args, uint32_t nret, target_ulong rets) { + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); uint32_t config_addr = rtas_ld(args, 0); uint64_t buid = rtas_ldq(args, 1); unsigned int func = rtas_ld(args, 3); @@ -334,7 +335,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr, return; } - if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) { + if (!smc->legacy_irq_allocation) { spapr_irq_msi_free(spapr, msi->first_irq, msi->num); } spapr_irq_free(spapr, msi->first_irq, msi->num); @@ -375,7 +376,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr, } /* Allocate MSIs */ - if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) { + if (smc->legacy_irq_allocation) { irq = spapr_irq_find(spapr, req_num, ret_intr_type == RTAS_TYPE_MSI, &err); } else { @@ -401,7 +402,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr, /* Release previous MSIs */ if (msi) { - if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) { + if (!smc->legacy_irq_allocation) { spapr_irq_msi_free(spapr, msi->first_irq, msi->num); } spapr_irq_free(spapr, msi->first_irq, msi->num); @@ -1558,6 +1559,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) sPAPRMachineState *spapr = (sPAPRMachineState *) object_dynamic_cast(qdev_get_machine(), TYPE_SPAPR_MACHINE); + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); SysBusDevice *s = SYS_BUS_DEVICE(dev); sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s); PCIHostState *phb = PCI_HOST_BRIDGE(s); @@ -1575,7 +1577,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) } if (sphb->index != (uint32_t)-1) { - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); Error *local_err = NULL; smc->phb_placement(spapr, sphb->index, @@ -1720,7 +1721,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i; Error *local_err = NULL; - if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) { + if (smc->legacy_irq_allocation) { irq = spapr_irq_findone(spapr, &local_err); if (local_err) { error_propagate(errp, local_err);