[v4,2/8] pc: move igd support code to igd.c
diff mbox

Message ID 1457447247-4865-3-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann March 8, 2016, 2:27 p.m. UTC
Pure code motion, except for dropping instance_size for
TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE (no need to set,
we can inherit it from TYPE_I440FX_PCI_DEVICE).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 default-configs/x86_64-softmmu.mak |  1 +
 hw/pci-host/Makefile.objs          |  3 ++
 hw/pci-host/igd.c                  | 99 ++++++++++++++++++++++++++++++++++++++
 hw/pci-host/piix.c                 | 90 ----------------------------------
 4 files changed, 103 insertions(+), 90 deletions(-)
 create mode 100644 hw/pci-host/igd.c

Comments

Gerd Hoffmann March 9, 2016, 3:08 p.m. UTC | #1
Hi,

> +/* Here we just expose minimal host bridge offset subset. */
> +static const IGDHostInfo igd_host_bridge_infos[] = {
> +    {0x08, 2},  /* revision id */
> +    {0x2c, 2},  /* sybsystem vendor id */
> +    {0x2e, 2},  /* sybsystem id */

Can anyone clarify where this comes from?

Setting the subsystem id without also setting the pci id looks wrong,
given that each pci id has its own subsystem id namespace.

Testing (with alex vfio patches) shows that dropping this seems to have
no bad effects.  Things are still working fine of we only set these ...

> +    {0x50, 2},  /* SNB: processor graphics control register */
> +    {0x52, 2},  /* processor graphics control register */
> +    {0xa4, 4},  /* SNB: graphics base of stolen memory */
> +    {0xa8, 4},  /* SNB: base of GTT stolen memory */

... gfx registers in host bridge pci config space.

thanks,
  Gerd
Tian, Kevin March 10, 2016, 5:56 a.m. UTC | #2
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]

> Sent: Wednesday, March 09, 2016 11:08 PM

> 

>   Hi,

> 

> > +/* Here we just expose minimal host bridge offset subset. */

> > +static const IGDHostInfo igd_host_bridge_infos[] = {

> > +    {0x08, 2},  /* revision id */

> > +    {0x2c, 2},  /* sybsystem vendor id */

> > +    {0x2e, 2},  /* sybsystem id */

> 

> Can anyone clarify where this comes from?


Add Allen who is the original author.

> 

> Setting the subsystem id without also setting the pci id looks wrong,

> given that each pci id has its own subsystem id namespace.

> 

> Testing (with alex vfio patches) shows that dropping this seems to have

> no bad effects.  Things are still working fine of we only set these ...


It's possible that original list may become more than what current
driver requires...

> 

> > +    {0x50, 2},  /* SNB: processor graphics control register */

> > +    {0x52, 2},  /* processor graphics control register */

> > +    {0xa4, 4},  /* SNB: graphics base of stolen memory */

> > +    {0xa8, 4},  /* SNB: base of GTT stolen memory */

> 

> ... gfx registers in host bridge pci config space.

> 


Yes.

Thanks
Kevin
Kay, Allen M March 10, 2016, 5:51 p.m. UTC | #3
> -----Original Message-----

> From: Tian, Kevin

> Sent: Wednesday, March 09, 2016 9:56 PM

> To: Gerd Hoffmann <kraxel@redhat.com>

> Cc: Alex Williamson <alex.williamson@redhat.com>; Stefano Stabellini

> <stefano.stabellini@eu.citrix.com>; xen-devel@lists.xensource.com; igvt-

> g@ml01.01.org; Michael S. Tsirkin <mst@redhat.com>; open list:All patches

> CC here <qemu-devel@nongnu.org>; Kay, Allen M <allen.m.kay@intel.com>

> Subject: RE: [PATCH v4 2/8] pc: move igd support code to igd.c

> 

> > From: Gerd Hoffmann [mailto:kraxel@redhat.com]

> > Sent: Wednesday, March 09, 2016 11:08 PM

> >

> >   Hi,

> >

> > > +/* Here we just expose minimal host bridge offset subset. */ static

> > > +const IGDHostInfo igd_host_bridge_infos[] = {

> > > +    {0x08, 2},  /* revision id */

> > > +    {0x2c, 2},  /* sybsystem vendor id */

> > > +    {0x2e, 2},  /* sybsystem id */

> >

> > Can anyone clarify where this comes from?

> 

> Add Allen who is the original author.

> 

> >

> > Setting the subsystem id without also setting the pci id looks wrong,

> > given that each pci id has its own subsystem id namespace.

> >


Host bridge register passthrough only serves the purpose of allowing Windows IGD driver accessing info it needs to boot and leave the rest intact so it would not look too different from the chipset it is emulating. 

Allen

Patch
diff mbox

diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 6e3b312..cd3340e 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -58,3 +58,4 @@  CONFIG_IOH3420=y
 CONFIG_I82801B11=y
 CONFIG_SMBIOS=y
 CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
+CONFIG_PCI_IGD=y
diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs
index 45f1f0e..c03210b 100644
--- a/hw/pci-host/Makefile.objs
+++ b/hw/pci-host/Makefile.objs
@@ -16,3 +16,6 @@  common-obj-$(CONFIG_FULONG) += bonito.o
 common-obj-$(CONFIG_PCI_PIIX) += piix.o
 common-obj-$(CONFIG_PCI_Q35) += q35.o
 common-obj-$(CONFIG_PCI_GENERIC) += gpex.o
+
+# igd passthrough support
+common-obj-$(CONFIG_PCI_IGD) += igd.o
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
new file mode 100644
index 0000000..331e9e1
--- /dev/null
+++ b/hw/pci-host/igd.c
@@ -0,0 +1,99 @@ 
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/pci/pci.h"
+#include "hw/i386/pc.h"
+
+/* IGD Passthrough Host Bridge. */
+typedef struct {
+    uint8_t offset;
+    uint8_t len;
+} IGDHostInfo;
+
+/* Here we just expose minimal host bridge offset subset. */
+static const IGDHostInfo igd_host_bridge_infos[] = {
+    {0x08, 2},  /* revision id */
+    {0x2c, 2},  /* sybsystem vendor id */
+    {0x2e, 2},  /* sybsystem id */
+    {0x50, 2},  /* SNB: processor graphics control register */
+    {0x52, 2},  /* processor graphics control register */
+    {0xa4, 4},  /* SNB: graphics base of stolen memory */
+    {0xa8, 4},  /* SNB: base of GTT stolen memory */
+};
+
+static int host_pci_config_read(int pos, int len, uint32_t *val)
+{
+    char path[PATH_MAX];
+    int config_fd;
+    ssize_t size = sizeof(path);
+    /* Access real host bridge. */
+    int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
+                      0, 0, 0, 0, "config");
+    int ret = 0;
+
+    if (rc >= size || rc < 0) {
+        return -ENODEV;
+    }
+
+    config_fd = open(path, O_RDWR);
+    if (config_fd < 0) {
+        return -ENODEV;
+    }
+
+    if (lseek(config_fd, pos, SEEK_SET) != pos) {
+        ret = -errno;
+        goto out;
+    }
+
+    do {
+        rc = read(config_fd, (uint8_t *)val, len);
+    } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
+    if (rc != len) {
+        ret = -errno;
+    }
+
+out:
+    close(config_fd);
+    return ret;
+}
+
+static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+{
+    uint32_t val = 0;
+    int rc, i, num;
+    int pos, len;
+
+    num = ARRAY_SIZE(igd_host_bridge_infos);
+    for (i = 0; i < num; i++) {
+        pos = igd_host_bridge_infos[i].offset;
+        len = igd_host_bridge_infos[i].len;
+        rc = host_pci_config_read(pos, len, &val);
+        if (rc) {
+            return -ENODEV;
+        }
+        pci_default_write_config(pci_dev, pos, val, len);
+    }
+
+    return 0;
+}
+
+static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = igd_pt_i440fx_initfn;
+    dc->desc = "IGD Passthrough Host bridge";
+}
+
+static const TypeInfo igd_passthrough_i440fx_info = {
+    .name          = TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
+    .parent        = TYPE_I440FX_PCI_DEVICE,
+    .class_init    = igd_passthrough_i440fx_class_init,
+};
+
+static void igd_register_types(void)
+{
+    type_register_static(&igd_passthrough_i440fx_info);
+}
+
+type_init(igd_register_types)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 41aa66f..6eb8bff 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -747,95 +747,6 @@  static const TypeInfo i440fx_info = {
     .class_init    = i440fx_class_init,
 };
 
-/* IGD Passthrough Host Bridge. */
-typedef struct {
-    uint8_t offset;
-    uint8_t len;
-} IGDHostInfo;
-
-/* Here we just expose minimal host bridge offset subset. */
-static const IGDHostInfo igd_host_bridge_infos[] = {
-    {0x08, 2},  /* revision id */
-    {0x2c, 2},  /* sybsystem vendor id */
-    {0x2e, 2},  /* sybsystem id */
-    {0x50, 2},  /* SNB: processor graphics control register */
-    {0x52, 2},  /* processor graphics control register */
-    {0xa4, 4},  /* SNB: graphics base of stolen memory */
-    {0xa8, 4},  /* SNB: base of GTT stolen memory */
-};
-
-static int host_pci_config_read(int pos, int len, uint32_t *val)
-{
-    char path[PATH_MAX];
-    int config_fd;
-    ssize_t size = sizeof(path);
-    /* Access real host bridge. */
-    int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
-                      0, 0, 0, 0, "config");
-    int ret = 0;
-
-    if (rc >= size || rc < 0) {
-        return -ENODEV;
-    }
-
-    config_fd = open(path, O_RDWR);
-    if (config_fd < 0) {
-        return -ENODEV;
-    }
-
-    if (lseek(config_fd, pos, SEEK_SET) != pos) {
-        ret = -errno;
-        goto out;
-    }
-
-    do {
-        rc = read(config_fd, (uint8_t *)val, len);
-    } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
-    if (rc != len) {
-        ret = -errno;
-    }
-
-out:
-    close(config_fd);
-    return ret;
-}
-
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
-{
-    uint32_t val = 0;
-    int rc, i, num;
-    int pos, len;
-
-    num = ARRAY_SIZE(igd_host_bridge_infos);
-    for (i = 0; i < num; i++) {
-        pos = igd_host_bridge_infos[i].offset;
-        len = igd_host_bridge_infos[i].len;
-        rc = host_pci_config_read(pos, len, &val);
-        if (rc) {
-            return -ENODEV;
-        }
-        pci_default_write_config(pci_dev, pos, val, len);
-    }
-
-    return 0;
-}
-
-static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->init = igd_pt_i440fx_initfn;
-    dc->desc = "IGD Passthrough Host bridge";
-}
-
-static const TypeInfo igd_passthrough_i440fx_info = {
-    .name          = TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
-    .parent        = TYPE_I440FX_PCI_DEVICE,
-    .instance_size = sizeof(PCII440FXState),
-    .class_init    = igd_passthrough_i440fx_class_init,
-};
-
 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
                                                 PCIBus *rootbus)
 {
@@ -877,7 +788,6 @@  static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
     type_register_static(&i440fx_info);
-    type_register_static(&igd_passthrough_i440fx_info);
     type_register_static(&piix3_pci_type_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);