George Dunlap
2012-Feb-10 11:06 UTC
[PATCH] 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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2012-Feb-10 11:28 UTC
Re: [PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
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
George Dunlap
2012-Feb-10 14:08 UTC
Re: [PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
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 belowSure.> > > > 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
George Dunlap
2012-Feb-10 14:41 UTC
Re: [PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
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
Stefano Stabellini
2012-Feb-10 14:41 UTC
Re: [PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
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.
Stefano Stabellini
2012-Feb-10 15:02 UTC
Re: [PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
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.
George Dunlap
2012-Feb-10 15:56 UTC
[PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
# 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);
}
/*
George Dunlap
2012-Feb-10 16:05 UTC
Re: [PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
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
Stefano Stabellini
2012-Feb-10 17:14 UTC
Re: [PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
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 >
Ian Jackson
2012-Feb-13 17:00 UTC
Re: [PATCH] qemu: Don''t access /proc/bus/pci unless graphics pass-thru is enabled
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>