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

[Alexey Kardashevskiy]

On 04/30/2013 09:56 PM, Alexander Graf wrote:
> 
> 
> Am 30.04.2013 um 13:52 schrieb Alexey Kardashevskiy <address@hidden>:
> 
>> On 04/30/2013 09:30 PM, Alexander Graf wrote:
>>>
>>>
>>> Am 30.04.2013 um 13:15 schrieb Alexey Kardashevskiy <address@hidden>:
>>>
>>>> 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.

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.


> 
> Alex
> 
>>
>>
>>
>>>
>>>
>>> Alex
>>>
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>> Alex
>>>>>
>>>>>>
>>>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>>>> Signed-off-by: David Gibson <address@hidden>
>>>>>> ---
>>>>>> hw/ppc/spapr.c |    6 +++---
>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>> index c96ac81..d07c74a 100644
>>>>>> --- a/hw/ppc/spapr.c
>>>>>> +++ b/hw/ppc/spapr.c
>>>>>> @@ -732,7 +732,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>>>>>   const char *boot_device = args->boot_device;
>>>>>>   PowerPCCPU *cpu;
>>>>>>   CPUPPCState *env;
>>>>>> -    PCIHostState *phb;
>>>>>> +    PCIHostState *phb = NULL;
>>>>>>   int i;
>>>>>>   MemoryRegion *sysmem = get_system_memory();
>>>>>>   MemoryRegion *ram = g_new(MemoryRegion, 1);
>>>>>> @@ -898,11 +898,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs 
>>>>>> *args)
>>>>>>   }
>>>>>>
>>>>>>   /* Graphics */
>>>>>> -    if (spapr_vga_init(phb->bus)) {
>>>>>> +    if (phb && spapr_vga_init(phb->bus)) {
>>>>>>       spapr->has_graphics = true;
>>>>>>   }
>>>>>>
>>>>>> -    if (usb_enabled(spapr->has_graphics)) {
>>>>>> +    if (phb && usb_enabled(spapr->has_graphics)) {
>>>>>>       pci_create_simple(phb->bus, -1, "pci-ohci");
>>>>>>       if (spapr->has_graphics) {
>>>>>>           usbdevice_create("keyboard");
>>>>>> -- 
>>>>>> 1.7.10.4
>>>>
>>>>
>>>> -- 
>>>> Alexey
>>
>>
>> -- 
>> Alexey


-- 
Alexey