Re: [Qemu-devel] [PULL 25/26] spapr_pci: factorize the use of SPAPR_MACHINE_GET_CLASS()

[Greg Kurz]

On Fri, 24 Aug 2018 17:30:12 +0200
Cédric Le Goater <address@hidden> wrote:

> On 08/24/2018 05:09 PM, Peter Maydell wrote:
> > On 21 August 2018 at 05:33, David Gibson <address@hidden> wrote:  
> >> From: Cédric Le Goater <address@hidden>
> >>
> >> It should save us some CPU cycles as these routines perform a lot of
> >> checks.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> Signed-off-by: David Gibson <address@hidden>
> >> ---
> >>  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.