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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 10 Feb 2012, George Dunlap wrote:
> 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. :-)

Considering that pci_dev_host_bridge is only used in a very specific set
of cases, we could call pt_pci_get_dev and check for the validity of
pci_dev_host_bridge only in case we need to use it.
So it is probably a good idea to move the check below in case
config_addr == 0x58, print a message and return an error.
Same thing for igd_pci_read.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel