Re: [Qemu-devel] [PATCH 3/4] spapr: Code more defensively against lack of primary PCI bus

[Alexander Graf <agraf@suse.de>]

On 04/30/2013 02:07 PM, Alexey Kardashevskiy wrote:
> On 04/30/2013 09:56 PM, Alexander Graf wrote:
>>
>> Am 30.04.2013 um 13:52 schrieb Alexey Kardashevskiy<aik@ozlabs.ru>:
>>
>>> On 04/30/2013 09:30 PM, Alexander Graf wrote:
>>>>
>>>> Am 30.04.2013 um 13:15 schrieb Alexey Kardashevskiy<aik@ozlabs.ru>:
>>>>
>>>>> On 04/30/2013 07:45 PM, Alexander Graf wrote:
>>>>>> On 30.04.2013, at 06:33, Alexey Kardashevskiy wrote:
>>>>>>
>>>>>>> Currently sPAPR always creates a primary PCI host bridge for emulated PCI
>>>>>>> devices.  However, because the platform supports native virtual IO, and
>>>>>>> can also support multiple independent PCI host bridges, it's quite often
>>>>>>> useful to disable the primary bridge for debugging purposes.
>>>>>>>
>>>>>>> This patch, therefore, makes the code cope more gracefully with a missing
>>>>>>> primary host bridge.
>>>>>> Does this handle the -net case too? What about disks?
>>>>>>
>>>>>> If those use a different mechanism to find their bus, maybe it'd be better to instead do
>>>>>>
>>>>>> if (phb) {
>>>>>>    pcibus = phb->bus;
>>>>>> } else {
>>>>>>    pcibus = find_pci_bus();
>>>>>> }
>>>>>>
>>>>>> In fact, maybe the code already deals with bus==NULL?
>>>>>
>>>>> I needed it to deal with phb==NULL.
>>>>>
>>>>> The story behind this patch is that while doing VFIO stuff, I have to deal
>>>>> with multiple PHBs. And sometime I want to be 100% sure that it is just
>>>>> VFIO PHB and no emulated PCI stuff (especially when I debug USB PCI card in
>>>>> VFIO) so I simply commented out the call which creates default emulated PHB
>>>>> and that would be it if the existing code did not try to use phb->bus.
>>>> Then this code does nor belong in upstream. If you want to have full control over instantiated devices, implement -nodefaults.
>>>
>>> -nodefaults means no default devices but it is still expected to create PCI
>>> bus and libvirt heavily uses this feature.
>> Your patch only addresses default device creation.
>
> It makes it easier to disable emulated PHB by commenting out one single
> line in the code. Profit.

It adds untested code paths. Nack.

> If there was a way to tell QEMU via command line not to create default
> emulated PHB, sure I would have implemented it but it is not there. And
> this is for debug, not for a normal use so there is no point in inventing
> such a parameter for qemu.

Take a look at what your patch does, then at your comment and then start 
thinking please.


Alex