Patchwork [3/4] spapr: Code more defensively against lack of primary PCI bus

login
register
mail settings
Submitter Alexey Kardashevskiy
Date April 30, 2013, 4:33 a.m.
Message ID <1367296434-15453-4-git-send-email-aik@ozlabs.ru>
Download mbox | patch
Permalink /patch/240548/
State New
Headers show

Comments

Alexey Kardashevskiy - April 30, 2013, 4:33 a.m.
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.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Alexander Graf - April 30, 2013, 9:45 a.m.
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?


Alex

> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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 Kardashevskiy - April 30, 2013, 11:15 a.m.
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.




> 
> 
> Alex
> 
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>> 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
>>
>
Alexander Graf - April 30, 2013, 11:30 a.m.
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.


Alex

> 
> 
> 
> 
>> 
>> 
>> Alex
>> 
>>> 
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> 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 Kardashevskiy - April 30, 2013, 11:52 a.m.
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.



> 
> 
> Alex
> 
>>
>>
>>
>>
>>>
>>>
>>> Alex
>>>
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>> ---
>>>> 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
Alexander Graf - April 30, 2013, 11:56 a.m.
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.

Alex

> 
> 
> 
>> 
>> 
>> Alex
>> 
>>> 
>>> 
>>> 
>>> 
>>>> 
>>>> 
>>>> Alex
>>>> 
>>>>> 
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>> ---
>>>>> 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 Kardashevskiy - April 30, 2013, 12:07 p.m.
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.

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 <aik@ozlabs.ru>
>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>> ---
>>>>>> 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
Alexander Graf - April 30, 2013, 12:29 p.m.
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

Patch

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");