Re: [libvirt] [PATCH] Read PCI class from sysfs class file instead of config space.

2014-01-07 Thread Thadeu Lima de Souza Cascardo 
On Tue, Jan 07, 2014 at 09:53:51AM -0700, Eric Blake wrote:
> On 01/07/2014 09:35 AM, Michal Privoznik wrote:
> 
> >> diff --git a/tests/virpcitestdata/0001:00:00.0.config 
> >> b/tests/virpcitestdata/0001:00:00.0.config
> >> new file mode 100644
> >> index 
> >> ..808d48993cfc0f41223fcb5f49deffc594f136b7
> >> GIT binary patch
> >> literal 4096
> >> zcmeH@I}XAy5Jca`5~RZg60MJb!~ys;a0 >> z{ig90wibyn0^=hytn<`78f&|D=zEvd5U8+Sxa1hw*nI*%I6fEDtkd58IUH=(-@Ei&
> >> z`ONZ@#r(MD|GagrnWu5_32tAW*RPg6sv;l)A|L`HAOa#F0wN#+A|L`H@J9q*PZkdc
> >>
> >> literal 0
> >> HcmV?d1
> >>
> > 
> > Some binary garbage ...
> 
> I know this was copied from a working sample file, as opposed to
> something you generated (if it were generated, I'd request that we
> instead version-control the human-readable source that got converted
> into the binary blob).  As long as it doesn't trip up 'make
> syntax-check' or git server side hooks, I think we can live with this.
> Does the resulting file contain any newlines or carriage returns that
> might get botched on Windows platforms, if we don't add a .gitattributes
> listing that mentions it is explicitly a binary file?
> 

This was copied from a system where it was failing before. I also copied
the config files from a working device + bridge.

make syntax-check seems to work, but some of the files have a newline
character. I haven't worked with git on Windows. Is that going to be a
problem?

Regards.
Cascardo.

> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Read PCI class from sysfs class file instead of config space.

2013-12-24 Thread Thadeu Lima de Souza Cascardo 
When determining if a device is behind a PCI bridge, the PCI device
class is checked by reading the config space. However, there are some
devices which have the wrong class on the config space, but the class is
initialized by Linux correctly as a PCI BRIDGE. This class can be read
by the sysfs file '/sys/bus/pci/devices/:xx:xx.x/class'.

One example of such bridge is IBM PCI Bridge 1014:03b9, which is
identified as a Host Bridge when reading the config space.

Signed-off-by: Thadeu Lima de Souza Cascardo 
---

The new test cases succeed with this patch. The second test fails
without the changes to src/util/virpci.c.

---
 src/util/virpci.c|   41 +++--
 tests/virpcimock.c   |   33 
 tests/virpcitest.c   |   34 
 tests/virpcitestdata/0001:00:00.0.config |  Bin 0 -> 4096 bytes
 tests/virpcitestdata/0001:01:00.0.config |  Bin 0 -> 4096 bytes
 tests/virpcitestdata/0001:01:00.1.config |  Bin 0 -> 4096 bytes
 tests/virpcitestdata/0005:80:00.0.config |  Bin 0 -> 4096 bytes
 tests/virpcitestdata/0005:90:01.0.config |  Bin 0 -> 256 bytes
 tests/virpcitestdata/0005:90:01.1.config |  Bin 0 -> 256 bytes
 tests/virpcitestdata/0005:90:01.2.config |  Bin 0 -> 256 bytes
 10 files changed, 105 insertions(+), 3 deletions(-)
 create mode 100644 tests/virpcitestdata/0001:00:00.0.config
 create mode 100644 tests/virpcitestdata/0001:01:00.0.config
 create mode 100644 tests/virpcitestdata/0001:01:00.1.config
 create mode 100644 tests/virpcitestdata/0005:80:00.0.config
 create mode 100644 tests/virpcitestdata/0005:90:01.0.config
 create mode 100644 tests/virpcitestdata/0005:90:01.1.config
 create mode 100644 tests/virpcitestdata/0005:90:01.2.config

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 8ec642f..8c10a93 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -344,6 +344,37 @@ virPCIDeviceRead32(virPCIDevicePtr dev, int cfgfd, 
unsigned int pos)
 }
 
 static int
+virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class)
+{
+char *path = NULL;
+char *id_str;
+int ret = 0;
+unsigned int value;
+
+if (virPCIFile(&path, dev->name, "class") < 0)
+return -1;
+
+/* class string is '0xNN\n' ... i.e. 9 bytes */
+if (virFileReadAll(path, 9, &id_str) < 0) {
+VIR_FREE(path);
+return -1;
+}
+
+VIR_FREE(path);
+
+id_str[8] = '\0';
+ret = virStrToLong_ui(id_str, NULL, 16, &value);
+if (ret == 0)
+*device_class = (value >> 8) & 0x;
+else
+VIR_WARN("Unusual value in " PCI_SYSFS "devices/%s/class: %s",
+ dev->name, id_str);
+
+VIR_FREE(id_str);
+return ret;
+}
+
+static int
 virPCIDeviceWrite(virPCIDevicePtr dev,
   int cfgfd,
   unsigned int pos,
@@ -645,8 +676,8 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr 
check, void *data)
 return 0;
 
 /* Is it a bridge? */
-device_class = virPCIDeviceRead16(check, fd, PCI_CLASS_DEVICE);
-if (device_class != PCI_CLASS_BRIDGE_PCI)
+ret = virPCIDeviceReadClass(check, &device_class);
+if (ret == -1 || device_class != PCI_CLASS_BRIDGE_PCI)
 goto cleanup;
 
 /* Is it a plane? */
@@ -2110,6 +2141,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
 unsigned int pos;
 int fd;
 int ret = 0;
+uint16_t device_class;
 
 if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
 return -1;
@@ -2119,8 +2151,11 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
 goto cleanup;
 }
 
+if (virPCIDeviceReadClass(dev, &device_class))
+goto cleanup;
+
 pos = dev->pcie_cap_pos;
-if (!pos || virPCIDeviceRead16(dev, fd, PCI_CLASS_DEVICE) != 
PCI_CLASS_BRIDGE_PCI)
+if (!pos || device_class != PCI_CLASS_BRIDGE_PCI)
 goto cleanup;
 
 flags = virPCIDeviceRead16(dev, fd, pos + PCI_EXP_FLAGS);
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index a5cef46..49759b0 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -29,6 +29,7 @@
 # include 
 # include 
 # include 
+# include 
 # include "viralloc.h"
 # include "virstring.h"
 # include "virfile.h"
@@ -42,6 +43,7 @@ static int (*real__xstat)(int ver, const char *path, struct 
stat *sb);
 static char *(*realcanonicalize_file_name)(const char *path);
 static int (*realopen)(const char *path, int flags, ...);
 static int (*realclose)(int fd);
+static DIR * (*realopendir)(const char *name);
 
 /* Don't make static, since it causes problems with clang
  * when passed as an arg to virAsprintf()
@@ -112,6 +114,7 @@ struct pciDevice {
 char *id;
 int vendor;
 int device;
+int class;
 struct pciDriver *driver;   /* Driver attached. NULL if attached to no

Re: [libvirt] [PATCH] Read PCI class from sysfs class file instead of config space.

2013-12-11 Thread Thadeu Lima de Souza Cascardo 
On Wed, Dec 11, 2013 at 04:30:48PM +0200, Laine Stump wrote:
> On 12/10/2013 11:57 AM, Thadeu Lima de Souza Cascardo wrote:
> > When determining if a device is behind a PCI bridge, the PCI device
> > class is checked by reading the config space. However, there are some
> > devices which have the wrong class on the config space, but the class is
> > initialized by Linux correctly as a PCI BRIDGE. This class can be read
> > by the sysfs file '/sys/bus/pci/devices/:xx:xx.x/class'.
> 
> Since I'm not an expert on the PCI spec, I'll just have to take your
> word for this :-)
> 

Well, I'm not an expert either, but in this case, it's just a matter of
dealing with bad hardware. After this patch, I managed to get a guest up
with the PCI device and it worked. But if it makes everybody more
comfortable (or less comfortable, realizing there is broken hardware out
there :-), here are two snippets from kernel code:

arch/powerpc/platforms/powernv/pci.c:
/* Fixup wrong class code in p7ioc and p8 root complex */
static void pnv_p7ioc_rc_quirk(struct pci_dev *dev)
{
dev->class = PCI_CLASS_BRIDGE_PCI << 8;
}
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_IBM, 0x3b9, pnv_p7ioc_rc_quirk);

vers/pci/host/pci-tegra.c:
/* Tegra PCIE root complex wrongly reports device class */
static void tegra_pcie_fixup_class(struct pci_dev *dev)
{
dev->class = PCI_CLASS_BRIDGE_PCI << 8;
}
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf0, tegra_pcie_fixup_class);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_fixup_class);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_fixup_class);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_fixup_class);

I just believe that, instead of introducing those same quirks into
libvirt code, it's so much better to leave this at the kernel, and read
the fixed value of class.


> >
> > One example of such bridge is IBM PCI Bridge 1014:03b9, which is
> > identified as a Host Bridge when reading the config space.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo 
> > ---
> >  src/util/virpci.c |   38 +++---
> >  1 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/util/virpci.c b/src/util/virpci.c
> > index 8ec642f..8353411 100644
> > --- a/src/util/virpci.c
> > +++ b/src/util/virpci.c
> > @@ -344,6 +344,34 @@ virPCIDeviceRead32(virPCIDevicePtr dev, int cfgfd, 
> > unsigned int pos)
> >  }
> >  
> >  static int
> > +virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class)
> > +{
> > +char *path = NULL;
> > +char *id_str;
> > +int ret = 0;
> > +unsigned int value;
> > +
> > +if (virPCIFile(&path, dev->name, "class") < 0)
> > +return -1;
> > +
> > +/* class string is '0xNN\n' ... i.e. 9 bytes */
> > +if (virFileReadAll(path, 9, &id_str) < 0) {
> > +VIR_FREE(path);
> > +return -1;
> > +}
> > +
> > +VIR_FREE(path);
> > +
> > +id_str[8] = '\0';
> > +ret = virStrToLong_ui(id_str, NULL, 16, &value);
> > +if (ret == 0)
> > +*device_class = (value >> 8) & 0x;
> 
> If the class file for some reason doesn't contain a hexadecimal number,
> this will return an error (-1) without having logged an error. This
> results in one of those "An error occurred, but the cause is unknown"
> messages, which is always very frustrating to debug. Even though it's
> *highly* unlikely that such an error would occur, we should still have
> the code to log it just in case.

I will work it out and repost.

> 
> 
> > +
> > +VIR_FREE(id_str);
> > +return ret;
> > +}
> > +
> > +static int
> >  virPCIDeviceWrite(virPCIDevicePtr dev,
> >int cfgfd,
> >unsigned int pos,
> > @@ -645,8 +673,8 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, 
> > virPCIDevicePtr check, void *data)
> >  return 0;
> >  
> >  /* Is it a bridge? */
> > -device_class = virPCIDeviceRead16(check, fd, PCI_CLASS_DEVICE);
> > -if (device_class != PCI_CLASS_BRIDGE_PCI)
> > +ret = virPCIDeviceReadClass(check, &device_class);
> > +if (ret == -1 || device_class != PCI_CLASS_BRIDGE_PCI)
> >  goto cleanup;
> >  
> >  /* Is it a plane? */
> > @@ -2110,6 +2138,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
> >  unsigned int pos;
> >  int fd;
> >  int ret = 0;
>

Re: [libvirt] [PATCH] Read PCI class from sysfs class file instead of config space.

2013-12-11 Thread Thadeu Lima de Souza Cascardo 
On Wed, Dec 11, 2013 at 12:23:59PM +, Daniel P. Berrange wrote:
> On Tue, Dec 10, 2013 at 07:57:01AM -0200, Thadeu Lima de Souza Cascardo wrote:
> > When determining if a device is behind a PCI bridge, the PCI device
> > class is checked by reading the config space. However, there are some
> > devices which have the wrong class on the config space, but the class is
> > initialized by Linux correctly as a PCI BRIDGE. This class can be read
> > by the sysfs file '/sys/bus/pci/devices/:xx:xx.x/class'.
> 
> Do you have any idea how long this file has been present in sysfs ? Hopefully
> it is a fairly ancient feature, so we can rely on it existing ? Otherwise
> we'll need to fallback to reading the config space when the sysfs file does
> not exist.
> 

At least 10 years, and before the config file was available (also more
than 10 years).  :-)

> > One example of such bridge is IBM PCI Bridge 1014:03b9, which is
> > identified as a Host Bridge when reading the config space.
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo 
> > ---
> >  src/util/virpci.c |   38 +++---
> >  1 files changed, 35 insertions(+), 3 deletions(-)
> 
> Looks reasonable to me, though prefer Laine to ACK it since he knows
> the PCI code best of anyone
> 

Thanks for your review.
Cascardo.

> Daniel
> -- 
> |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org  -o- http://virt-manager.org :|
> |: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Read PCI class from sysfs class file instead of config space.

2013-12-10 Thread Thadeu Lima de Souza Cascardo 
When determining if a device is behind a PCI bridge, the PCI device
class is checked by reading the config space. However, there are some
devices which have the wrong class on the config space, but the class is
initialized by Linux correctly as a PCI BRIDGE. This class can be read
by the sysfs file '/sys/bus/pci/devices/:xx:xx.x/class'.

One example of such bridge is IBM PCI Bridge 1014:03b9, which is
identified as a Host Bridge when reading the config space.

Signed-off-by: Thadeu Lima de Souza Cascardo 
---
 src/util/virpci.c |   38 +++---
 1 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 8ec642f..8353411 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -344,6 +344,34 @@ virPCIDeviceRead32(virPCIDevicePtr dev, int cfgfd, 
unsigned int pos)
 }
 
 static int
+virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class)
+{
+char *path = NULL;
+char *id_str;
+int ret = 0;
+unsigned int value;
+
+if (virPCIFile(&path, dev->name, "class") < 0)
+return -1;
+
+/* class string is '0xNN\n' ... i.e. 9 bytes */
+if (virFileReadAll(path, 9, &id_str) < 0) {
+VIR_FREE(path);
+return -1;
+}
+
+VIR_FREE(path);
+
+id_str[8] = '\0';
+ret = virStrToLong_ui(id_str, NULL, 16, &value);
+if (ret == 0)
+*device_class = (value >> 8) & 0x;
+
+VIR_FREE(id_str);
+return ret;
+}
+
+static int
 virPCIDeviceWrite(virPCIDevicePtr dev,
   int cfgfd,
   unsigned int pos,
@@ -645,8 +673,8 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr 
check, void *data)
 return 0;
 
 /* Is it a bridge? */
-device_class = virPCIDeviceRead16(check, fd, PCI_CLASS_DEVICE);
-if (device_class != PCI_CLASS_BRIDGE_PCI)
+ret = virPCIDeviceReadClass(check, &device_class);
+if (ret == -1 || device_class != PCI_CLASS_BRIDGE_PCI)
 goto cleanup;
 
 /* Is it a plane? */
@@ -2110,6 +2138,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
 unsigned int pos;
 int fd;
 int ret = 0;
+uint16_t device_class;
 
 if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
 return -1;
@@ -2119,8 +2148,11 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
 goto cleanup;
 }
 
+if (virPCIDeviceReadClass(dev, &device_class))
+goto cleanup;
+
 pos = dev->pcie_cap_pos;
-if (!pos || virPCIDeviceRead16(dev, fd, PCI_CLASS_DEVICE) != 
PCI_CLASS_BRIDGE_PCI)
+if (!pos || device_class != PCI_CLASS_BRIDGE_PCI)
 goto cleanup;
 
 flags = virPCIDeviceRead16(dev, fd, pos + PCI_EXP_FLAGS);
-- 
1.7.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Read PCI class from sysfs class file instead of config space.

2013-12-10 Thread Thadeu Lima de Souza Cascardo 
When determining if a device is behind a PCI bridge, the PCI device
class is checked by reading the config space. However, there are some
devices which have the wrong class on the config space, but the class is
initialized by Linux correctly as a PCI BRIDGE. This class can be read
by the sysfs file '/sys/bus/pci/devices/:xx:xx.x/class'.

One example of such bridge is IBM PCI Bridge 1014:03b9, which is
identified as a Host Bridge when reading the config space.

Signed-off-by: Thadeu Lima de Souza Cascardo 
---
 src/util/virpci.c |   38 +++---
 1 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 8ec642f..8353411 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -344,6 +344,34 @@ virPCIDeviceRead32(virPCIDevicePtr dev, int cfgfd, 
unsigned int pos)
 }
 
 static int
+virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class)
+{
+char *path = NULL;
+char *id_str;
+int ret = 0;
+unsigned int value;
+
+if (virPCIFile(&path, dev->name, "class") < 0)
+return -1;
+
+/* class string is '0xNN\n' ... i.e. 9 bytes */
+if (virFileReadAll(path, 9, &id_str) < 0) {
+VIR_FREE(path);
+return -1;
+}
+
+VIR_FREE(path);
+
+id_str[8] = '\0';
+ret = virStrToLong_ui(id_str, NULL, 16, &value);
+if (ret == 0)
+*device_class = (value >> 8) & 0x;
+
+VIR_FREE(id_str);
+return ret;
+}
+
+static int
 virPCIDeviceWrite(virPCIDevicePtr dev,
   int cfgfd,
   unsigned int pos,
@@ -645,8 +673,8 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr 
check, void *data)
 return 0;
 
 /* Is it a bridge? */
-device_class = virPCIDeviceRead16(check, fd, PCI_CLASS_DEVICE);
-if (device_class != PCI_CLASS_BRIDGE_PCI)
+ret = virPCIDeviceReadClass(check, &device_class);
+if (ret == -1 || device_class != PCI_CLASS_BRIDGE_PCI)
 goto cleanup;
 
 /* Is it a plane? */
@@ -2110,6 +2138,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
 unsigned int pos;
 int fd;
 int ret = 0;
+uint16_t device_class;
 
 if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
 return -1;
@@ -2119,8 +2148,11 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev)
 goto cleanup;
 }
 
+if (virPCIDeviceReadClass(dev, &device_class))
+goto cleanup;
+
 pos = dev->pcie_cap_pos;
-if (!pos || virPCIDeviceRead16(dev, fd, PCI_CLASS_DEVICE) != 
PCI_CLASS_BRIDGE_PCI)
+if (!pos || device_class != PCI_CLASS_BRIDGE_PCI)
 goto cleanup;
 
 flags = virPCIDeviceRead16(dev, fd, pos + PCI_EXP_FLAGS);
-- 
1.7.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list