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

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

[George Dunlap]

# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1328888874 0
# Node ID e4a4e4f4156dcdffb8e81fb28bb16b6a9d611af7
# Parent  6efeff914609a3870e2d07a8d73a26c4615ac60b
qemu: Don't access /proc/bus/pci unless graphics pass-thru is enabled

A recent changeset introduced a bug whereby an initialization function
that reads /proc/bus/pci is called from graphics set-up functions even
if pass-through graphics are not enabled.  If qemu is run without
permission to this file, this causes qemu to fail during
initialization.

This patch re-works the functions so that the initialization happens
only if we actually need to do the pci host read or write.  It also
makes failures call abort().

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 6efeff914609 -r e4a4e4f4156d hw/pass-through.h
--- a/hw/pass-through.h	Fri Feb 10 11:02:25 2012 +0000
+++ b/hw/pass-through.h	Fri Feb 10 15:47:54 2012 +0000
@@ -29,6 +29,8 @@
 /* Log acesss */
 #define PT_LOGGING_ENABLED
 
+/* Print errors even if logging is disabled */
+#define PT_ERR(_f, _a...)   fprintf(logfile, "%s: " _f, __func__, ##_a)
 #ifdef PT_LOGGING_ENABLED
 #define PT_LOG(_f, _a...)   fprintf(logfile, "%s: " _f, __func__, ##_a)
 #define PT_LOG_DEV(_dev, _f, _a...)   fprintf(logfile, "%s: [%02x:%02x:%01x] " _f, __func__,    \
diff -r 6efeff914609 -r e4a4e4f4156d 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 15:47:54 2012 +0000
@@ -23,11 +23,17 @@ 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 )
         return;
 
+    if ( !(pci_dev_1f=pt_pci_get_dev(0, 0x1f, 0)) )
+    {
+        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
+        abort();
+    }
+
     vid = pt_pci_host_read(pci_dev_1f, PCI_VENDOR_ID, 2);
     did = pt_pci_host_read(pci_dev_1f, PCI_DEVICE_ID, 2);
     rid = pt_pci_host_read(pci_dev_1f, PCI_REVISION, 1);
@@ -39,36 +45,45 @@ 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 ) {
-        pci_default_write_config(pci_dev, config_addr, val, len);
-        return;
-    }
+    if ( !igd_passthru )
+        goto write_default;
 
     switch (config_addr)
     {
         case 0x58:        // PAVPC Offset
-            pt_pci_host_write(pci_dev_host_bridge, config_addr, val, len);
-#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
-            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
-                    config_addr, len, val);
-#endif
             break;
         default:
-            pci_default_write_config(pci_dev, config_addr, val, len);
+            goto write_default;
     }
+
+    /* Host write */
+    if ( !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) )
+    {
+        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
+        abort();
+    }
+
+    pt_pci_host_write(pci_dev_host_bridge, config_addr, val, len);
+#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
+    PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
+               config_addr, len, val);
+#endif
+    return;
+
+write_default:
+    pci_default_write_config(pci_dev, config_addr, val, len);
 }
 
 uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
 {
-    struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
+    struct pci_dev *pci_dev_host_bridge;
     uint32_t val;
 
     assert(pci_dev->devfn == 0x00);
-    if ( !igd_passthru ) {
-        return pci_default_read_config(pci_dev, config_addr, len);
-    }
+    if ( !igd_passthru )
+        goto read_default;
 
     switch (config_addr)
     {
@@ -81,16 +96,28 @@ uint32_t igd_pci_read(PCIDevice *pci_dev
         case 0x58:        /* SNB: PAVPC Offset */
         case 0xa4:        /* SNB: graphics base of stolen memory */
         case 0xa8:        /* SNB: base of GTT stolen memory */
-            val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
-#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
-            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
-                    config_addr, len, val);
-#endif
             break;
         default:
-            val = pci_default_read_config(pci_dev, config_addr, len);
+            goto read_default;
     }
+
+    /* Host read */
+    if ( !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) )
+    {
+        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
+        abort();
+    }
+
+    val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
+#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
+    PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
+               config_addr, len, val);
+#endif
     return val;
+   
+read_default:
+   
+   return pci_default_read_config(pci_dev, config_addr, len);
 }
 
 /*

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

[George Dunlap]

Sorry, didn't get the CC right...
 -George

On Fri, Feb 10, 2012 at 3:56 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@eu.citrix.com>
> # Date 1328888874 0
> # Node ID e4a4e4f4156dcdffb8e81fb28bb16b6a9d611af7
> # Parent  6efeff914609a3870e2d07a8d73a26c4615ac60b
> qemu: Don't access /proc/bus/pci unless graphics pass-thru is enabled
>
> A recent changeset introduced a bug whereby an initialization function
> that reads /proc/bus/pci is called from graphics set-up functions even
> if pass-through graphics are not enabled.  If qemu is run without
> permission to this file, this causes qemu to fail during
> initialization.
>
> This patch re-works the functions so that the initialization happens
> only if we actually need to do the pci host read or write.  It also
> makes failures call abort().
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> diff -r 6efeff914609 -r e4a4e4f4156d hw/pass-through.h
> --- a/hw/pass-through.h Fri Feb 10 11:02:25 2012 +0000
> +++ b/hw/pass-through.h Fri Feb 10 15:47:54 2012 +0000
> @@ -29,6 +29,8 @@
>  /* Log acesss */
>  #define PT_LOGGING_ENABLED
>
> +/* Print errors even if logging is disabled */
> +#define PT_ERR(_f, _a...)   fprintf(logfile, "%s: " _f, __func__, ##_a)
>  #ifdef PT_LOGGING_ENABLED
>  #define PT_LOG(_f, _a...)   fprintf(logfile, "%s: " _f, __func__, ##_a)
>  #define PT_LOG_DEV(_dev, _f, _a...)   fprintf(logfile, "%s: [%02x:%02x:%01x] " _f, __func__,    \
> diff -r 6efeff914609 -r e4a4e4f4156d 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 15:47:54 2012 +0000
> @@ -23,11 +23,17 @@ 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 )
>         return;
>
> +    if ( !(pci_dev_1f=pt_pci_get_dev(0, 0x1f, 0)) )
> +    {
> +        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
> +        abort();
> +    }
> +
>     vid = pt_pci_host_read(pci_dev_1f, PCI_VENDOR_ID, 2);
>     did = pt_pci_host_read(pci_dev_1f, PCI_DEVICE_ID, 2);
>     rid = pt_pci_host_read(pci_dev_1f, PCI_REVISION, 1);
> @@ -39,36 +45,45 @@ 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 ) {
> -        pci_default_write_config(pci_dev, config_addr, val, len);
> -        return;
> -    }
> +    if ( !igd_passthru )
> +        goto write_default;
>
>     switch (config_addr)
>     {
>         case 0x58:        // PAVPC Offset
> -            pt_pci_host_write(pci_dev_host_bridge, config_addr, val, len);
> -#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> -            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> -                    config_addr, len, val);
> -#endif
>             break;
>         default:
> -            pci_default_write_config(pci_dev, config_addr, val, len);
> +            goto write_default;
>     }
> +
> +    /* Host write */
> +    if ( !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) )
> +    {
> +        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
> +        abort();
> +    }
> +
> +    pt_pci_host_write(pci_dev_host_bridge, config_addr, val, len);
> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> +               config_addr, len, val);
> +#endif
> +    return;
> +
> +write_default:
> +    pci_default_write_config(pci_dev, config_addr, val, len);
>  }
>
>  uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
>  {
> -    struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
> +    struct pci_dev *pci_dev_host_bridge;
>     uint32_t val;
>
>     assert(pci_dev->devfn == 0x00);
> -    if ( !igd_passthru ) {
> -        return pci_default_read_config(pci_dev, config_addr, len);
> -    }
> +    if ( !igd_passthru )
> +        goto read_default;
>
>     switch (config_addr)
>     {
> @@ -81,16 +96,28 @@ uint32_t igd_pci_read(PCIDevice *pci_dev
>         case 0x58:        /* SNB: PAVPC Offset */
>         case 0xa4:        /* SNB: graphics base of stolen memory */
>         case 0xa8:        /* SNB: base of GTT stolen memory */
> -            val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
> -#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> -            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> -                    config_addr, len, val);
> -#endif
>             break;
>         default:
> -            val = pci_default_read_config(pci_dev, config_addr, len);
> +            goto read_default;
>     }
> +
> +    /* Host read */
> +    if ( !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) )
> +    {
> +        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
> +        abort();
> +    }
> +
> +    val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> +               config_addr, len, val);
> +#endif
>     return val;
> +
> +read_default:
> +
> +   return pci_default_read_config(pci_dev, config_addr, len);
>  }
>
>  /*
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

[Stefano Stabellini]

On Fri, 10 Feb 2012, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@eu.citrix.com>
> # Date 1328888874 0
> # Node ID e4a4e4f4156dcdffb8e81fb28bb16b6a9d611af7
> # Parent  6efeff914609a3870e2d07a8d73a26c4615ac60b
> qemu: Don't access /proc/bus/pci unless graphics pass-thru is enabled
> 
> A recent changeset introduced a bug whereby an initialization function
> that reads /proc/bus/pci is called from graphics set-up functions even
> if pass-through graphics are not enabled.  If qemu is run without
> permission to this file, this causes qemu to fail during
> initialization.
> 
> This patch re-works the functions so that the initialization happens
> only if we actually need to do the pci host read or write.  It also
> makes failures call abort().
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

the patch is OK


> diff -r 6efeff914609 -r e4a4e4f4156d hw/pass-through.h
> --- a/hw/pass-through.h	Fri Feb 10 11:02:25 2012 +0000
> +++ b/hw/pass-through.h	Fri Feb 10 15:47:54 2012 +0000
> @@ -29,6 +29,8 @@
>  /* Log acesss */
>  #define PT_LOGGING_ENABLED
>  
> +/* Print errors even if logging is disabled */
> +#define PT_ERR(_f, _a...)   fprintf(logfile, "%s: " _f, __func__, ##_a)
>  #ifdef PT_LOGGING_ENABLED
>  #define PT_LOG(_f, _a...)   fprintf(logfile, "%s: " _f, __func__, ##_a)
>  #define PT_LOG_DEV(_dev, _f, _a...)   fprintf(logfile, "%s: [%02x:%02x:%01x] " _f, __func__,    \
> diff -r 6efeff914609 -r e4a4e4f4156d 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 15:47:54 2012 +0000
> @@ -23,11 +23,17 @@ 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 )
>          return;
>  
> +    if ( !(pci_dev_1f=pt_pci_get_dev(0, 0x1f, 0)) )
> +    {
> +        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
> +        abort();
> +    }
> +
>      vid = pt_pci_host_read(pci_dev_1f, PCI_VENDOR_ID, 2);
>      did = pt_pci_host_read(pci_dev_1f, PCI_DEVICE_ID, 2);
>      rid = pt_pci_host_read(pci_dev_1f, PCI_REVISION, 1);
> @@ -39,36 +45,45 @@ 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 ) {
> -        pci_default_write_config(pci_dev, config_addr, val, len);
> -        return;
> -    }
> +    if ( !igd_passthru )
> +        goto write_default;
>  
>      switch (config_addr)
>      {
>          case 0x58:        // PAVPC Offset
> -            pt_pci_host_write(pci_dev_host_bridge, config_addr, val, len);
> -#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> -            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> -                    config_addr, len, val);
> -#endif
>              break;
>          default:
> -            pci_default_write_config(pci_dev, config_addr, val, len);
> +            goto write_default;
>      }
> +
> +    /* Host write */
> +    if ( !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) )
> +    {
> +        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
> +        abort();
> +    }
> +
> +    pt_pci_host_write(pci_dev_host_bridge, config_addr, val, len);
> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> +               config_addr, len, val);
> +#endif
> +    return;
> +
> +write_default:
> +    pci_default_write_config(pci_dev, config_addr, val, len);
>  }
>  
>  uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
>  {
> -    struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
> +    struct pci_dev *pci_dev_host_bridge;
>      uint32_t val;
>  
>      assert(pci_dev->devfn == 0x00);
> -    if ( !igd_passthru ) {
> -        return pci_default_read_config(pci_dev, config_addr, len);
> -    }
> +    if ( !igd_passthru )
> +        goto read_default;
>  
>      switch (config_addr)
>      {
> @@ -81,16 +96,28 @@ uint32_t igd_pci_read(PCIDevice *pci_dev
>          case 0x58:        /* SNB: PAVPC Offset */
>          case 0xa4:        /* SNB: graphics base of stolen memory */
>          case 0xa8:        /* SNB: base of GTT stolen memory */
> -            val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
> -#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> -            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> -                    config_addr, len, val);
> -#endif
>              break;
>          default:
> -            val = pci_default_read_config(pci_dev, config_addr, len);
> +            goto read_default;
>      }
> +
> +    /* Host read */
> +    if ( !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) )
> +    {
> +        PT_ERR("Error: Can't get pci_dev_host_bridge\n");
> +        abort();
> +    }
> +
> +    val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
> +#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> +    PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> +               config_addr, len, val);
> +#endif
>      return val;
> +   
> +read_default:
> +   
> +   return pci_default_read_config(pci_dev, config_addr, len);
>  }
>  
>  /*
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

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

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] qemu: Don't access /proc/bus/pci unless graphics pass-thru is enabled"):
> On Fri, 10 Feb 2012, George Dunlap wrote:
> > qemu: Don't access /proc/bus/pci unless graphics pass-thru is enabled

Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

[Stefano Stabellini]

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.

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

[Stefano Stabellini]

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.

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

[George Dunlap]

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

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

[George Dunlap]

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

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

[Stefano Stabellini]

On Fri, 10 Feb 2012, George Dunlap wrote:
> A recent changeset introduced a bug whereby an initialization function
> that reads /proc/bus/pci is called from graphics set-up functions
> even if pass-through graphics are not enabled.  If qemu is run without
> permission to this file, this causes qemu to fail during initialization.
> 
> This patch changes the initialization functions to only happen if graphics
> pass-through are enabled.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

could you please send patches inline?


> # HG changeset patch
> # Parent 6efeff914609a3870e2d07a8d73a26c4615ac60b
> qemu: Don't access /proc/bus/pci unless graphics pass-thru is enabled
> 
> A recent changeset introduced a bug whereby an initialization function
> that reads /proc/bus/pci is called from graphics set-up functions
> even if pass-through graphics are not enabled.  If qemu is run without
> permission to this file, this causes qemu to fail during initialization.
> 
> This patch changes the initialization functions to only happen if graphics
> pass-through are enabled.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> 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


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


> @@ -62,11 +62,11 @@ void igd_pci_write(PCIDevice *pci_dev, u
>  
>  uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
>  {
> -    struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
> +    struct pci_dev *pci_dev_host_bridge;
>      uint32_t val;
>  
>      assert(pci_dev->devfn == 0x00);
> -    if ( !igd_passthru ) {
> +    if ( !igd_passthru || !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) ) {
>          return pci_default_read_config(pci_dev, config_addr, len);
>      }

same here

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

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

A recent changeset introduced a bug whereby an initialization function
that reads /proc/bus/pci is called from graphics set-up functions
even if pass-through graphics are not enabled.  If qemu is run without
permission to this file, this causes qemu to fail during initialization.

This patch changes the initialization functions to only happen if graphics
pass-through are enabled.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

[-- Attachment #2: fix-intel_pch_init.diff --]
[-- Type: text/x-diff, Size: 2094 bytes --]

# HG changeset patch
# Parent 6efeff914609a3870e2d07a8d73a26c4615ac60b
qemu: Don't access /proc/bus/pci unless graphics pass-thru is enabled

A recent changeset introduced a bug whereby an initialization function
that reads /proc/bus/pci is called from graphics set-up functions
even if pass-through graphics are not enabled.  If qemu is run without
permission to this file, this causes qemu to fail during initialization.

This patch changes the initialization functions to only happen if graphics
pass-through are enabled.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

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;
 
     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;
     }
@@ -62,11 +62,11 @@ void igd_pci_write(PCIDevice *pci_dev, u
 
 uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
 {
-    struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
+    struct pci_dev *pci_dev_host_bridge;
     uint32_t val;
 
     assert(pci_dev->devfn == 0x00);
-    if ( !igd_passthru ) {
+    if ( !igd_passthru || !(pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0)) ) {
         return pci_default_read_config(pci_dev, config_addr, len);
     }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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