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(-)
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);
--
2.17.1
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
>
>
© 2016 - 2025 Red Hat, Inc.