From mboxrd@z Thu Jan  1 00:00:00 1970
From: George Dunlap <george.dunlap@citrix.com>
Subject: Re: [PATCH] qemu: Don't access /proc/bus/pci unless
 graphics pass-thru is enabled
Date: Fri, 10 Feb 2012 14:08:02 +0000
Message-ID: <1328882882.3480.16.camel@elijah>
References: <CAFLBxZa=NKgZd+RNTND124LHrYkO1Ej=gYmm3Dxa=cux++h0FA@mail.gmail.com>
	<alpine.DEB.2.00.1202101123160.7456@kaball-desktop>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <alpine.DEB.2.00.1202101123160.7456@kaball-desktop>
List-Unsubscribe: <http://lists.xensource.com/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>, "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Ian Jackson <ian.jackson@xen.org>
List-Id: xen-devel@lists.xenproject.org

On Fri, 2012-02-10 at 11:28 +0000, Stefano Stabellini wrote:
> could you please send patches inline?

I'll see what I can do. :-)

> > diff -r 6efeff914609 hw/pt-graphics.c
> > --- a/hw/pt-graphics.c	Fri Feb 10 11:02:25 2012 +0000
> > +++ b/hw/pt-graphics.c	Fri Feb 10 11:04:01 2012 +0000
> > @@ -23,9 +23,9 @@ void intel_pch_init(PCIBus *bus)
> >  {
> >      uint16_t vid, did;
> >      uint8_t  rid;
> > -    struct pci_dev *pci_dev_1f = pt_pci_get_dev(0, 0x1f, 0);
> > +    struct pci_dev *pci_dev_1f;
> >  
> > -    if ( !gfx_passthru || !pci_dev_1f )
> > +    if ( !gfx_passthru || !(pci_dev_1f=pt_pci_get_dev(0, 0x1f, 0)) )
> >          return;
> 
> I would rather have it as a seprate test after if ( !gfx_passthru ),
> same for the others below

Sure.

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

Are you saying that if the assert passes, that the function will never
return NULL?

 -George