| Message ID | 1367296434-15453-4-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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 >
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 >> >
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
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
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
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
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
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");