[PATCH v4 0/8] igd passthrough chipset tweaks

[PATCH v4 0/8] igd passthrough chipset tweaks

[Gerd Hoffmann]

  Hi,

Next version of the patches, after a longish break.

Meanwhile it is more clear how we are going to handle the igd
passthrough quirks with kvm:  vfio will get support for device-specific
regions, and we will use that for the opregion and also to provide
unpriviledged read access to host bridge (00:00.0) and isa bridge
(00:1f,0) pci config space.

That implies we wouldn't share the code for pci config space access
and the existing xen code wouldn't be reused for kvm, except maybe for
the struct IGDHostInfo tables.

Separating out the igd support code into its own file and the cleanups +
bugfixes on top of that still make sense though.  So here we go with a
stripped down patch series ...

cheers,
  Gerd

Gerd Hoffmann (8):
  pc: remove has_igd_gfx_passthru global
  pc: move igd support code to igd.c
  igd: switch TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE to realize
  igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize
  igd: use defines for standard pci config space offsets
  igd: revamp host config read
  igd: move igd-passthrough-isa-bridge to igd.c too
  igd: handle igd-passthrough-isa-bridge setup in realize()

 default-configs/x86_64-softmmu.mak |   1 +
 hw/i386/pc_piix.c                  | 115 +--------------------------
 hw/pci-host/Makefile.objs          |   3 +
 hw/pci-host/igd.c                  | 157 +++++++++++++++++++++++++++++++++++++
 hw/pci-host/piix.c                 |  90 ---------------------
 hw/xen/xen_pt.c                    |   4 +-
 hw/xen/xen_pt.h                    |   5 +-
 include/hw/i386/pc.h               |   2 +-
 vl.c                               |  10 ---
 9 files changed, 167 insertions(+), 220 deletions(-)
 create mode 100644 hw/pci-host/igd.c

-- 
1.8.3.1


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

[PATCH v4 1/8] pc: remove has_igd_gfx_passthru global

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_piix.c |  2 +-
 hw/xen/xen_pt.h   |  5 +++--
 vl.c              | 10 ----------
 3 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6f8c2cd..40c58a5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -364,7 +364,7 @@ static void pc_init_isa(MachineState *machine)
 #ifdef CONFIG_XEN
 static void pc_xen_hvm_init_pci(MachineState *machine)
 {
-    const char *pci_type = has_igd_gfx_passthru ?
+    const char *pci_type = machine->igd_gfx_passthru ?
                 TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : TYPE_I440FX_PCI_DEVICE;
 
     pc_init1(machine,
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index c2f8e1f..4048a5a 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -4,6 +4,7 @@
 #include "qemu-common.h"
 #include "hw/xen/xen_common.h"
 #include "hw/pci/pci.h"
+#include "hw/boards.h"
 #include "xen-host-pci-device.h"
 
 void xen_pt_log(const PCIDevice *d, const char *f, ...) GCC_FMT_ATTR(2, 3);
@@ -322,10 +323,10 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice *dev,
                                             unsigned int domain,
                                             unsigned int bus, unsigned int slot,
                                             unsigned int function);
-extern bool has_igd_gfx_passthru;
 static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
 {
-    return (has_igd_gfx_passthru
+    MachineState *machine = MACHINE(qdev_get_machine());
+    return (machine->igd_gfx_passthru
             && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
 }
 int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
diff --git a/vl.c b/vl.c
index adeddd9..bdc2879 100644
--- a/vl.c
+++ b/vl.c
@@ -1374,13 +1374,6 @@ static inline void semihosting_arg_fallback(const char *file, const char *cmd)
     }
 }
 
-/* Now we still need this for compatibility with XEN. */
-bool has_igd_gfx_passthru;
-static void igd_gfx_passthru(void)
-{
-    has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
-}
-
 /***********************************************************/
 /* USB devices */
 
@@ -4524,9 +4517,6 @@ int main(int argc, char **argv, char **envp)
             exit(1);
     }
 
-    /* Check if IGD GFX passthrough. */
-    igd_gfx_passthru();
-
     /* init generic devices */
     if (qemu_opts_foreach(qemu_find_opts("device"),
                           device_init_func, NULL, NULL)) {
-- 
1.8.3.1

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

[Gerd Hoffmann]

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

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);
-- 
1.8.3.1

[PATCH v4 3/8] igd: switch TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE to realize

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/pci-host/igd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 331e9e1..93b86ca 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -56,7 +56,7 @@ out:
     return ret;
 }
 
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 {
     uint32_t val = 0;
     int rc, i, num;
@@ -68,12 +68,11 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
         len = igd_host_bridge_infos[i].len;
         rc = host_pci_config_read(pos, len, &val);
         if (rc) {
-            return -ENODEV;
+            error_setg(errp, "failed to read host config");
+            return;
         }
         pci_default_write_config(pci_dev, pos, val, len);
     }
-
-    return 0;
 }
 
 static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
@@ -81,7 +80,7 @@ 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;
+    k->realize = igd_pt_i440fx_realize;
     dc->desc = "IGD Passthrough Host bridge";
 }
 
-- 
1.8.3.1


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

[PATCH v4 4/8] igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/igd.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 93b86ca..3654298 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -56,12 +56,32 @@ out:
     return ret;
 }
 
+#define IGD_PT_I440FX_CLASS(class)                              \
+    OBJECT_CLASS_CHECK(IGDPtI440fxClass, (class),               \
+                       TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+#define IGD_PT_I440FX_GET_CLASS(obj)                            \
+    OBJECT_GET_CLASS(IGDPtI440fxClass, (obj),                   \
+                     TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+
+typedef struct IGDPtI440fxClass {
+    PCIDeviceClass parent_class;
+    void (*parent_realize)(PCIDevice *dev, Error **errp);
+} IGDPtI440fxClass;
+
 static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 {
+    IGDPtI440fxClass *k = IGD_PT_I440FX_GET_CLASS(pci_dev);
+    Error *err = NULL;
     uint32_t val = 0;
     int rc, i, num;
     int pos, len;
 
+    k->parent_realize(pci_dev, &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+
     num = ARRAY_SIZE(igd_host_bridge_infos);
     for (i = 0; i < num; i++) {
         pos = igd_host_bridge_infos[i].offset;
@@ -77,17 +97,20 @@ static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 
 static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
 {
+    IGDPtI440fxClass *k = IGD_PT_I440FX_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
 
-    k->realize = igd_pt_i440fx_realize;
-    dc->desc = "IGD Passthrough Host bridge";
+    k->parent_realize = pc->realize;
+    pc->realize = igd_pt_i440fx_realize;
+    dc->desc = "IGD Passthrough Host bridge (i440fx)";
 }
 
 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,
+    .class_size    = sizeof(IGDPtI440fxClass),
 };
 
 static void igd_register_types(void)
-- 
1.8.3.1


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

[PATCH v4 5/8] igd: use defines for standard pci config space offsets

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/pci-host/igd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 3654298..8a8b37b 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -11,9 +11,9 @@ typedef struct {
 
 /* 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 */
+    {PCI_REVISION_ID,         2},
+    {PCI_SUBSYSTEM_VENDOR_ID, 2},
+    {PCI_SUBSYSTEM_ID,        2},
     {0x50, 2},  /* SNB: processor graphics control register */
     {0x52, 2},  /* processor graphics control register */
     {0xa4, 4},  /* SNB: graphics base of stolen memory */
-- 
1.8.3.1


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

[PATCH v4 6/8] igd: revamp host config read

[Gerd Hoffmann]

Move all work to the host_pci_config_copy helper function,
which we can easily reuse when adding q35 support.
Open sysfs file only once for all values.  Use pread.
Proper error handling.

Fix bug:  Update config space directly (writing via
pci_default_write_config only works for registers
whitelisted in wmask).

Hmm, this code can hardly ever worked before,
/me wonders what test coverage it had.

With this patch in place igd-passthru=on actually
works, although it still requires root priviledges
because linux refuses to allow non-root users access
pci config space above offset 0x50.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/igd.c | 65 ++++++++++++++++++++++---------------------------------
 1 file changed, 26 insertions(+), 39 deletions(-)

diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 8a8b37b..5c4a008 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -20,40 +20,33 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
     {0xa8, 4},  /* SNB: base of GTT stolen memory */
 };
 
-static int host_pci_config_read(int pos, int len, uint32_t *val)
+static void host_pci_config_copy(PCIDevice *guest, const char *host,
+                                 const IGDHostInfo *list, int len, Error **errp)
 {
-    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;
+    char *path;
+    int config_fd, rc, i;
 
-    if (rc >= size || rc < 0) {
-        return -ENODEV;
-    }
-
-    config_fd = open(path, O_RDWR);
+    path = g_strdup_printf("/sys/bus/pci/devices/%s/config", host);
+    config_fd = open(path, O_RDONLY);
     if (config_fd < 0) {
-        return -ENODEV;
+        error_setg_file_open(errp, errno, path);
+        goto out_free;
     }
 
-    if (lseek(config_fd, pos, SEEK_SET) != pos) {
-        ret = -errno;
-        goto out;
+    for (i = 0; i < len; i++) {
+        rc = pread(config_fd, guest->config + list[i].offset,
+                   list[i].len, list[i].offset);
+        if (rc != list[i].len) {
+            error_setg_errno(errp, errno, "read %s, offset 0x%x",
+                             path, list[i].offset);
+            goto out_close;
+        }
     }
 
-    do {
-        rc = read(config_fd, (uint8_t *)val, len);
-    } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
-    if (rc != len) {
-        ret = -errno;
-    }
-
-out:
+out_close:
     close(config_fd);
-    return ret;
+out_free:
+    g_free(path);
 }
 
 #define IGD_PT_I440FX_CLASS(class)                              \
@@ -72,9 +65,6 @@ static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 {
     IGDPtI440fxClass *k = IGD_PT_I440FX_GET_CLASS(pci_dev);
     Error *err = NULL;
-    uint32_t val = 0;
-    int rc, i, num;
-    int pos, len;
 
     k->parent_realize(pci_dev, &err);
     if (err != NULL) {
@@ -82,16 +72,13 @@ static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
 
-    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) {
-            error_setg(errp, "failed to read host config");
-            return;
-        }
-        pci_default_write_config(pci_dev, pos, val, len);
+    host_pci_config_copy(pci_dev, "0000:00:00.0",
+                         igd_host_bridge_infos,
+                         ARRAY_SIZE(igd_host_bridge_infos),
+                         &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
     }
 }
 
-- 
1.8.3.1

[PATCH v4 7/8] igd: move igd-passthrough-isa-bridge to igd.c too

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/pc_piix.c | 113 ------------------------------------------------------
 hw/pci-host/igd.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 113 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 40c58a5..43964dc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -910,119 +910,6 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m)
 DEFINE_I440FX_MACHINE(v0_10, "pc-0.10", pc_compat_0_13,
                       pc_i440fx_0_10_machine_options);
 
-typedef struct {
-    uint16_t gpu_device_id;
-    uint16_t pch_device_id;
-    uint8_t pch_revision_id;
-} IGDDeviceIDInfo;
-
-/* In real world different GPU should have different PCH. But actually
- * the different PCH DIDs likely map to different PCH SKUs. We do the
- * same thing for the GPU. For PCH, the different SKUs are going to be
- * all the same silicon design and implementation, just different
- * features turn on and off with fuses. The SW interfaces should be
- * consistent across all SKUs in a given family (eg LPT). But just same
- * features may not be supported.
- *
- * Most of these different PCH features probably don't matter to the
- * Gfx driver, but obviously any difference in display port connections
- * will so it should be fine with any PCH in case of passthrough.
- *
- * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
- * scenarios, 0x9cc3 for BDW(Broadwell).
- */
-static const IGDDeviceIDInfo igd_combo_id_infos[] = {
-    /* HSW Classic */
-    {0x0402, 0x8c4e, 0x04}, /* HSWGT1D, HSWD_w7 */
-    {0x0406, 0x8c4e, 0x04}, /* HSWGT1M, HSWM_w7 */
-    {0x0412, 0x8c4e, 0x04}, /* HSWGT2D, HSWD_w7 */
-    {0x0416, 0x8c4e, 0x04}, /* HSWGT2M, HSWM_w7 */
-    {0x041E, 0x8c4e, 0x04}, /* HSWGT15D, HSWD_w7 */
-    /* HSW ULT */
-    {0x0A06, 0x8c4e, 0x04}, /* HSWGT1UT, HSWM_w7 */
-    {0x0A16, 0x8c4e, 0x04}, /* HSWGT2UT, HSWM_w7 */
-    {0x0A26, 0x8c4e, 0x06}, /* HSWGT3UT, HSWM_w7 */
-    {0x0A2E, 0x8c4e, 0x04}, /* HSWGT3UT28W, HSWM_w7 */
-    {0x0A1E, 0x8c4e, 0x04}, /* HSWGT2UX, HSWM_w7 */
-    {0x0A0E, 0x8c4e, 0x04}, /* HSWGT1ULX, HSWM_w7 */
-    /* HSW CRW */
-    {0x0D26, 0x8c4e, 0x04}, /* HSWGT3CW, HSWM_w7 */
-    {0x0D22, 0x8c4e, 0x04}, /* HSWGT3CWDT, HSWD_w7 */
-    /* HSW Server */
-    {0x041A, 0x8c4e, 0x04}, /* HSWSVGT2, HSWD_w7 */
-    /* HSW SRVR */
-    {0x040A, 0x8c4e, 0x04}, /* HSWSVGT1, HSWD_w7 */
-    /* BSW */
-    {0x1606, 0x9cc3, 0x03}, /* BDWULTGT1, BDWM_w7 */
-    {0x1616, 0x9cc3, 0x03}, /* BDWULTGT2, BDWM_w7 */
-    {0x1626, 0x9cc3, 0x03}, /* BDWULTGT3, BDWM_w7 */
-    {0x160E, 0x9cc3, 0x03}, /* BDWULXGT1, BDWM_w7 */
-    {0x161E, 0x9cc3, 0x03}, /* BDWULXGT2, BDWM_w7 */
-    {0x1602, 0x9cc3, 0x03}, /* BDWHALOGT1, BDWM_w7 */
-    {0x1612, 0x9cc3, 0x03}, /* BDWHALOGT2, BDWM_w7 */
-    {0x1622, 0x9cc3, 0x03}, /* BDWHALOGT3, BDWM_w7 */
-    {0x162B, 0x9cc3, 0x03}, /* BDWHALO28W, BDWM_w7 */
-    {0x162A, 0x9cc3, 0x03}, /* BDWGT3WRKS, BDWM_w7 */
-    {0x162D, 0x9cc3, 0x03}, /* BDWGT3SRVR, BDWM_w7 */
-};
-
-static void isa_bridge_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    dc->desc        = "ISA bridge faked to support IGD PT";
-    k->vendor_id    = PCI_VENDOR_ID_INTEL;
-    k->class_id     = PCI_CLASS_BRIDGE_ISA;
-};
-
-static TypeInfo isa_bridge_info = {
-    .name          = "igd-passthrough-isa-bridge",
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PCIDevice),
-    .class_init = isa_bridge_class_init,
-};
-
-static void pt_graphics_register_types(void)
-{
-    type_register_static(&isa_bridge_info);
-}
-type_init(pt_graphics_register_types)
-
-void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id)
-{
-    struct PCIDevice *bridge_dev;
-    int i, num;
-    uint16_t pch_dev_id = 0xffff;
-    uint8_t pch_rev_id;
-
-    num = ARRAY_SIZE(igd_combo_id_infos);
-    for (i = 0; i < num; i++) {
-        if (gpu_dev_id == igd_combo_id_infos[i].gpu_device_id) {
-            pch_dev_id = igd_combo_id_infos[i].pch_device_id;
-            pch_rev_id = igd_combo_id_infos[i].pch_revision_id;
-        }
-    }
-
-    if (pch_dev_id == 0xffff) {
-        return;
-    }
-
-    /* Currently IGD drivers always need to access PCH by 1f.0. */
-    bridge_dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
-                                   "igd-passthrough-isa-bridge");
-
-    /*
-     * Note that vendor id is always PCI_VENDOR_ID_INTEL.
-     */
-    if (!bridge_dev) {
-        fprintf(stderr, "set igd-passthrough-isa-bridge failed!\n");
-        return;
-    }
-    pci_config_set_device_id(bridge_dev->config, pch_dev_id);
-    pci_config_set_revision(bridge_dev->config, pch_rev_id);
-}
-
 static void isapc_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 5c4a008..e7183a3 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -100,9 +100,117 @@ static const TypeInfo igd_passthrough_i440fx_info = {
     .class_size    = sizeof(IGDPtI440fxClass),
 };
 
+typedef struct {
+    uint16_t gpu_device_id;
+    uint16_t pch_device_id;
+    uint8_t pch_revision_id;
+} IGDDeviceIDInfo;
+
+/* In real world different GPU should have different PCH. But actually
+ * the different PCH DIDs likely map to different PCH SKUs. We do the
+ * same thing for the GPU. For PCH, the different SKUs are going to be
+ * all the same silicon design and implementation, just different
+ * features turn on and off with fuses. The SW interfaces should be
+ * consistent across all SKUs in a given family (eg LPT). But just same
+ * features may not be supported.
+ *
+ * Most of these different PCH features probably don't matter to the
+ * Gfx driver, but obviously any difference in display port connections
+ * will so it should be fine with any PCH in case of passthrough.
+ *
+ * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
+ * scenarios, 0x9cc3 for BDW(Broadwell).
+ */
+static const IGDDeviceIDInfo igd_combo_id_infos[] = {
+    /* HSW Classic */
+    {0x0402, 0x8c4e, 0x04}, /* HSWGT1D, HSWD_w7 */
+    {0x0406, 0x8c4e, 0x04}, /* HSWGT1M, HSWM_w7 */
+    {0x0412, 0x8c4e, 0x04}, /* HSWGT2D, HSWD_w7 */
+    {0x0416, 0x8c4e, 0x04}, /* HSWGT2M, HSWM_w7 */
+    {0x041E, 0x8c4e, 0x04}, /* HSWGT15D, HSWD_w7 */
+    /* HSW ULT */
+    {0x0A06, 0x8c4e, 0x04}, /* HSWGT1UT, HSWM_w7 */
+    {0x0A16, 0x8c4e, 0x04}, /* HSWGT2UT, HSWM_w7 */
+    {0x0A26, 0x8c4e, 0x06}, /* HSWGT3UT, HSWM_w7 */
+    {0x0A2E, 0x8c4e, 0x04}, /* HSWGT3UT28W, HSWM_w7 */
+    {0x0A1E, 0x8c4e, 0x04}, /* HSWGT2UX, HSWM_w7 */
+    {0x0A0E, 0x8c4e, 0x04}, /* HSWGT1ULX, HSWM_w7 */
+    /* HSW CRW */
+    {0x0D26, 0x8c4e, 0x04}, /* HSWGT3CW, HSWM_w7 */
+    {0x0D22, 0x8c4e, 0x04}, /* HSWGT3CWDT, HSWD_w7 */
+    /* HSW Server */
+    {0x041A, 0x8c4e, 0x04}, /* HSWSVGT2, HSWD_w7 */
+    /* HSW SRVR */
+    {0x040A, 0x8c4e, 0x04}, /* HSWSVGT1, HSWD_w7 */
+    /* BSW */
+    {0x1606, 0x9cc3, 0x03}, /* BDWULTGT1, BDWM_w7 */
+    {0x1616, 0x9cc3, 0x03}, /* BDWULTGT2, BDWM_w7 */
+    {0x1626, 0x9cc3, 0x03}, /* BDWULTGT3, BDWM_w7 */
+    {0x160E, 0x9cc3, 0x03}, /* BDWULXGT1, BDWM_w7 */
+    {0x161E, 0x9cc3, 0x03}, /* BDWULXGT2, BDWM_w7 */
+    {0x1602, 0x9cc3, 0x03}, /* BDWHALOGT1, BDWM_w7 */
+    {0x1612, 0x9cc3, 0x03}, /* BDWHALOGT2, BDWM_w7 */
+    {0x1622, 0x9cc3, 0x03}, /* BDWHALOGT3, BDWM_w7 */
+    {0x162B, 0x9cc3, 0x03}, /* BDWHALO28W, BDWM_w7 */
+    {0x162A, 0x9cc3, 0x03}, /* BDWGT3WRKS, BDWM_w7 */
+    {0x162D, 0x9cc3, 0x03}, /* BDWGT3SRVR, BDWM_w7 */
+};
+
+static void isa_bridge_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    dc->desc        = "ISA bridge faked to support IGD PT";
+    k->vendor_id    = PCI_VENDOR_ID_INTEL;
+    k->class_id     = PCI_CLASS_BRIDGE_ISA;
+};
+
+static TypeInfo igd_passthrough_isa_bridge_info = {
+    .name          = "igd-passthrough-isa-bridge",
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PCIDevice),
+    .class_init = isa_bridge_class_init,
+};
+
+void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id)
+{
+    struct PCIDevice *bridge_dev;
+    int i, num;
+    uint16_t pch_dev_id = 0xffff;
+    uint8_t pch_rev_id;
+
+    num = ARRAY_SIZE(igd_combo_id_infos);
+    for (i = 0; i < num; i++) {
+        if (gpu_dev_id == igd_combo_id_infos[i].gpu_device_id) {
+            pch_dev_id = igd_combo_id_infos[i].pch_device_id;
+            pch_rev_id = igd_combo_id_infos[i].pch_revision_id;
+        }
+    }
+
+    if (pch_dev_id == 0xffff) {
+        return;
+    }
+
+    /* Currently IGD drivers always need to access PCH by 1f.0. */
+    bridge_dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
+                                   "igd-passthrough-isa-bridge");
+
+    /*
+     * Note that vendor id is always PCI_VENDOR_ID_INTEL.
+     */
+    if (!bridge_dev) {
+        fprintf(stderr, "set igd-passthrough-isa-bridge failed!\n");
+        return;
+    }
+    pci_config_set_device_id(bridge_dev->config, pch_dev_id);
+    pci_config_set_revision(bridge_dev->config, pch_rev_id);
+}
+
 static void igd_register_types(void)
 {
     type_register_static(&igd_passthrough_i440fx_info);
+    type_register_static(&igd_passthrough_isa_bridge_info);
 }
 
 type_init(igd_register_types)
-- 
1.8.3.1


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

[PATCH v4 8/8] igd: handle igd-passthrough-isa-bridge setup in realize()

[Gerd Hoffmann]

That way a simple '-device igd-passthrough-isa-bridge,addr=1f' will
do the setup.

Also instead of looking up reasonable PCI IDs based on the graphic
device id simply copy over the ids from the host, thereby reusing the
infrastructure we have in place for the igd host bridges.  Less code,
and should be more robust as we don't have to maintain the id table
to keep things going.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/igd.c    | 115 +++++++++++++--------------------------------------
 hw/xen/xen_pt.c      |   4 +-
 include/hw/i386/pc.h |   2 +-
 3 files changed, 30 insertions(+), 91 deletions(-)

diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index e7183a3..0513c55 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -100,111 +100,52 @@ static const TypeInfo igd_passthrough_i440fx_info = {
     .class_size    = sizeof(IGDPtI440fxClass),
 };
 
-typedef struct {
-    uint16_t gpu_device_id;
-    uint16_t pch_device_id;
-    uint8_t pch_revision_id;
-} IGDDeviceIDInfo;
-
-/* In real world different GPU should have different PCH. But actually
- * the different PCH DIDs likely map to different PCH SKUs. We do the
- * same thing for the GPU. For PCH, the different SKUs are going to be
- * all the same silicon design and implementation, just different
- * features turn on and off with fuses. The SW interfaces should be
- * consistent across all SKUs in a given family (eg LPT). But just same
- * features may not be supported.
- *
- * Most of these different PCH features probably don't matter to the
- * Gfx driver, but obviously any difference in display port connections
- * will so it should be fine with any PCH in case of passthrough.
- *
- * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
- * scenarios, 0x9cc3 for BDW(Broadwell).
- */
-static const IGDDeviceIDInfo igd_combo_id_infos[] = {
-    /* HSW Classic */
-    {0x0402, 0x8c4e, 0x04}, /* HSWGT1D, HSWD_w7 */
-    {0x0406, 0x8c4e, 0x04}, /* HSWGT1M, HSWM_w7 */
-    {0x0412, 0x8c4e, 0x04}, /* HSWGT2D, HSWD_w7 */
-    {0x0416, 0x8c4e, 0x04}, /* HSWGT2M, HSWM_w7 */
-    {0x041E, 0x8c4e, 0x04}, /* HSWGT15D, HSWD_w7 */
-    /* HSW ULT */
-    {0x0A06, 0x8c4e, 0x04}, /* HSWGT1UT, HSWM_w7 */
-    {0x0A16, 0x8c4e, 0x04}, /* HSWGT2UT, HSWM_w7 */
-    {0x0A26, 0x8c4e, 0x06}, /* HSWGT3UT, HSWM_w7 */
-    {0x0A2E, 0x8c4e, 0x04}, /* HSWGT3UT28W, HSWM_w7 */
-    {0x0A1E, 0x8c4e, 0x04}, /* HSWGT2UX, HSWM_w7 */
-    {0x0A0E, 0x8c4e, 0x04}, /* HSWGT1ULX, HSWM_w7 */
-    /* HSW CRW */
-    {0x0D26, 0x8c4e, 0x04}, /* HSWGT3CW, HSWM_w7 */
-    {0x0D22, 0x8c4e, 0x04}, /* HSWGT3CWDT, HSWD_w7 */
-    /* HSW Server */
-    {0x041A, 0x8c4e, 0x04}, /* HSWSVGT2, HSWD_w7 */
-    /* HSW SRVR */
-    {0x040A, 0x8c4e, 0x04}, /* HSWSVGT1, HSWD_w7 */
-    /* BSW */
-    {0x1606, 0x9cc3, 0x03}, /* BDWULTGT1, BDWM_w7 */
-    {0x1616, 0x9cc3, 0x03}, /* BDWULTGT2, BDWM_w7 */
-    {0x1626, 0x9cc3, 0x03}, /* BDWULTGT3, BDWM_w7 */
-    {0x160E, 0x9cc3, 0x03}, /* BDWULXGT1, BDWM_w7 */
-    {0x161E, 0x9cc3, 0x03}, /* BDWULXGT2, BDWM_w7 */
-    {0x1602, 0x9cc3, 0x03}, /* BDWHALOGT1, BDWM_w7 */
-    {0x1612, 0x9cc3, 0x03}, /* BDWHALOGT2, BDWM_w7 */
-    {0x1622, 0x9cc3, 0x03}, /* BDWHALOGT3, BDWM_w7 */
-    {0x162B, 0x9cc3, 0x03}, /* BDWHALO28W, BDWM_w7 */
-    {0x162A, 0x9cc3, 0x03}, /* BDWGT3WRKS, BDWM_w7 */
-    {0x162D, 0x9cc3, 0x03}, /* BDWGT3SRVR, BDWM_w7 */
+static const IGDHostInfo igd_isa_bridge_infos[] = {
+    {PCI_VENDOR_ID,           2},
+    {PCI_DEVICE_ID,           2},
+    {PCI_REVISION_ID,         2},
+    {PCI_SUBSYSTEM_VENDOR_ID, 2},
+    {PCI_SUBSYSTEM_ID,        2},
 };
 
+static void igd_pt_isa_bridge_realize(PCIDevice *pci_dev, Error **errp)
+{
+    Error *err = NULL;
+
+    if (pci_dev->devfn != PCI_DEVFN(0x1f, 0)) {
+        error_setg(errp, "igd isa bridge must have address 1f.0");
+        return;
+    }
+
+    host_pci_config_copy(pci_dev, "0000:00:1f.0",
+                         igd_isa_bridge_infos,
+                         ARRAY_SIZE(igd_isa_bridge_infos),
+                         &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+}
+
 static void isa_bridge_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     dc->desc        = "ISA bridge faked to support IGD PT";
-    k->vendor_id    = PCI_VENDOR_ID_INTEL;
+    k->realize      = igd_pt_isa_bridge_realize;
     k->class_id     = PCI_CLASS_BRIDGE_ISA;
 };
 
 static TypeInfo igd_passthrough_isa_bridge_info = {
     .name          = "igd-passthrough-isa-bridge",
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PCIDevice),
     .class_init = isa_bridge_class_init,
 };
 
-void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id)
+void igd_passthrough_isa_bridge_create(PCIBus *bus)
 {
-    struct PCIDevice *bridge_dev;
-    int i, num;
-    uint16_t pch_dev_id = 0xffff;
-    uint8_t pch_rev_id;
-
-    num = ARRAY_SIZE(igd_combo_id_infos);
-    for (i = 0; i < num; i++) {
-        if (gpu_dev_id == igd_combo_id_infos[i].gpu_device_id) {
-            pch_dev_id = igd_combo_id_infos[i].pch_device_id;
-            pch_rev_id = igd_combo_id_infos[i].pch_revision_id;
-        }
-    }
-
-    if (pch_dev_id == 0xffff) {
-        return;
-    }
-
-    /* Currently IGD drivers always need to access PCH by 1f.0. */
-    bridge_dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
-                                   "igd-passthrough-isa-bridge");
-
-    /*
-     * Note that vendor id is always PCI_VENDOR_ID_INTEL.
-     */
-    if (!bridge_dev) {
-        fprintf(stderr, "set igd-passthrough-isa-bridge failed!\n");
-        return;
-    }
-    pci_config_set_device_id(bridge_dev->config, pch_dev_id);
-    pci_config_set_revision(bridge_dev->config, pch_rev_id);
+    pci_create_simple(bus, PCI_DEVFN(0x1f, 0), "igd-passthrough-isa-bridge");
 }
 
 static void igd_register_types(void)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 657bf6c..cada168 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -690,11 +690,9 @@ static void
 xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
                                       XenHostPCIDevice *dev)
 {
-    uint16_t gpu_dev_id;
     PCIDevice *d = &s->dev;
 
-    gpu_dev_id = dev->device_id;
-    igd_passthrough_isa_bridge_create(d->bus, gpu_dev_id);
+    igd_passthrough_isa_bridge_create(d->bus);
 }
 
 /* destroy. */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8b3546e..9d427fd 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -854,5 +854,5 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
     } \
     machine_init(pc_machine_init_##suffix)
 
-extern void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id);
+extern void igd_passthrough_isa_bridge_create(PCIBus *bus);
 #endif
-- 
1.8.3.1

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

[Gerd Hoffmann]

  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

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

[Tian, Kevin]

> 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

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

[Kay, Allen M]

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