Re: [PATCH] qemu: Don't access /proc/bus/pci unless graphics pass-thru is enabled

[George Dunlap <george.dunlap@citrix.com>]

On Fri, 2012-02-10 at 14:41 +0000, Stefano Stabellini wrote:
> On Fri, 10 Feb 2012, George Dunlap wrote:
> > > >      vid = pt_pci_host_read(pci_dev_1f, PCI_VENDOR_ID, 2);
> > > > @@ -39,9 +39,9 @@ void intel_pch_init(PCIBus *bus)
> > > >  
> > > >  void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t val, int len)
> > > >  {
> > > > -    struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
> > > > +    struct pci_dev *pci_dev_host_bridge;
> > > >      assert(pci_dev->devfn == 0x00);
> > > > -    if ( !igd_passthru ) {
> > > > +    if ( !igd_passthru || !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0))) {
> > > >          pci_default_write_config(pci_dev, config_addr, val, len);
> > > >          return;
> > > >      }
> > > 
> > > Why are you adding this test (!(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) ?
> > > 
> > > If you are worried that pci_dev_host_bridge could be NULL, shouldn't you
> > > also remove the assert?
> > 
> > The assert is about pci_dev, but the check is about the return value of
> > pci_host_bridge() (to which pci_dev_host_bridge is set).  If there's an
> > expected relationship between them, it wasn't immediately clear from the
> > code.  
> 
> I thought you were worried about the BDF being wrong (00:00.0), BDF that
> is the same as pci_dev->devfn and already checked by assert.
> However now I realize that pt_pci_get_dev could also fail because pcilib
> cannot read the host bridge properly so I think that the check makes
> sense.

OK -- in that case, since the fail path involves calling
pci_default_write_config(), would it make sense to keep both checks in
the same if()?

Or, I could just send the patch which I already have ready, which
retains the current behavior of assuming that pt_pci_get_dev()
succeeds. :-)

 -George