qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PULL 25/26] spapr_pci: factorize the use of SPAPR_MACHIN


From: Cédric Le Goater
Subject: Re: [Qemu-ppc] [PULL 25/26] spapr_pci: factorize the use of SPAPR_MACHINE_GET_CLASS()
Date: Fri, 24 Aug 2018 18:43:41 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

On 08/24/2018 05:38 PM, Greg Kurz wrote:
> 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.

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]