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

[Alexander Graf]

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<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.

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