qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough
@ 2015-07-15  5:37 Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 01/10] i440fx: make types configurable at run-time Tiejun Chen
                   ` (10 more replies)
  0 siblings, 11 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

v10:

* Don't extern igd_passthrough_isa_bridge_create() in the
  include/hw/xen/xen.h file. Instead, move inside the
  include/hw/i386/pc.h file in patch #7

v9:

* Rebase on the latest
* Inside patch #8, move is_igd_vga_passthrough(dev)) from
  xen_igd_passthrough_isa_bridge_create() into xen_pt_initfn().
* Inside patch #9, simplify pc_xen_hvm_init_pci()
* Michael acked them on pc side
* Stefano ackes then on xen side

v8:

* Rebase on the latest qemu tree
* Cleanup one xen leftover in patch #3

v7:

* Instead of "-gfx_passthru" we'd like to make that a machine
  option, "-machine xxx,igd-passthru=on"" 
* try to make something as common shared by others like KvmGT in
  the future
* Just read those real value from host bridge pci
  configuration space when create host bridge then put in dev->config.

v6:

* Drop introducing a new machine specific to IGD passthrough
* Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
* Currently IGD drivers always need to access PCH by 1f.0. But we
  don't want to poke that directly to get ID, and although 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).
* Drop igd write ops since its fine to emulate that, and we also shrink
  those igd read ops as necessary.
* Rebase and cleanup all patches.

v5:

* Simplify to make sure its really inherited from the standard one in patch #3
* Then drop the original patch #3

v4:

* Rebase on latest tree
* Drop patch #2
* Regenerate patches after Michael introduce patch #1
* We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
* Test: boot with a preinstalled winxp
  ./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c -machine pc

v3:

* Drop patch #4
* Add one patch #1 from Michael
* Rebase
* In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

v2:

* Fix some coding style
* New patch to separate i440fx_init
* Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
* Based on patch #2 to regenerate
* Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
* Test: boot with a preinstalled ubuntu 14.04
  ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc

As we discussed we need to create a separate machine to support current
IGD passthrough.

----------------------------------------------------------------
Michael S. Tsirkin (1):
      i440fx: make types configurable at run-time

Tiejun Chen (9):
      pc_init1: pass parameters just with types
      piix: create host bridge to passthrough
      hw/pci-assign: split pci-assign.c
      xen, gfx passthrough: basic graphics passthrough support
      xen, gfx passthrough: retrieve VGA BIOS to work
      igd gfx passthrough: create a isa bridge
      xen, gfx passthrough: register a isa bridge
      xen, gfx passthrough: register host bridge specific to passthrough
      xen, gfx passthrough: add opregion mapping

 hw/core/machine.c             |  20 +++
 hw/i386/Makefile.objs         |   1 +
 hw/i386/kvm/pci-assign.c      |  82 +---------
 hw/i386/pc_piix.c             | 139 ++++++++++++++++-
 hw/i386/pci-assign-load-rom.c |  93 ++++++++++++
 hw/pci-host/piix.c            |  91 +++++++++++-
 hw/xen/Makefile.objs          |   1 +
 hw/xen/xen-host-pci-device.c  |   5 +
 hw/xen/xen-host-pci-device.h  |   1 +
 hw/xen/xen_pt.c               |  36 +++++
 hw/xen/xen_pt.h               |  21 ++-
 hw/xen/xen_pt_config_init.c   |  51 ++++++-
 hw/xen/xen_pt_graphics.c      | 272 ++++++++++++++++++++++++++++++++++
 include/hw/boards.h           |   1 +
 include/hw/i386/pc.h          |   9 +-
 include/hw/pci/pci-assign.h   |  27 ++++
 qemu-options.hx               |   3 +
 vl.c                          |  10 ++
 19 files changed, 773 insertions(+), 92 deletions(-)
 create mode 100644 hw/i386/pci-assign-load-rom.c
 create mode 100644 hw/xen/xen_pt_graphics.c
 create mode 100644 include/hw/pci/pci-assign.h

Thanks
Tiejun

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 01/10] i440fx: make types configurable at run-time
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 02/10] pc_init1: pass parameters just with types Tiejun Chen
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

From: "Michael S. Tsirkin" <mst@redhat.com>

IGD passthrough wants to supply a different pci and
host devices, inheriting i440fx devices. Make types
configurable.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
v10:

* Nothing is changed.

v9:

* Just rebase on the latest.

 hw/i386/pc_piix.c    | 4 +++-
 hw/pci-host/piix.c   | 9 ++++-----
 include/hw/i386/pc.h | 6 +++++-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a896624..a36fad2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -200,7 +200,9 @@ static void pc_init1(MachineState *machine)
     }
 
     if (pci_enabled) {
-        pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
+        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
+                              TYPE_I440FX_PCI_DEVICE,
+                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, machine->ram_size,
                               below_4g_mem_size,
                               above_4g_mem_size,
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index ad55f99..a203d93 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -40,7 +40,6 @@
  * http://download.intel.com/design/chipsets/datashts/29054901.pdf
  */
 
-#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
 #define I440FX_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE)
 
@@ -95,7 +94,6 @@ typedef struct PIIX3State {
 #define PIIX3_PCI_DEVICE(obj) \
     OBJECT_CHECK(PIIX3State, (obj), TYPE_PIIX3_PCI_DEVICE)
 
-#define TYPE_I440FX_PCI_DEVICE "i440FX"
 #define I440FX_PCI_DEVICE(obj) \
     OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
 
@@ -300,7 +298,8 @@ static void i440fx_realize(PCIDevice *dev, Error **errp)
     dev->config[I440FX_SMRAM] = 0x02;
 }
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
+                    PCII440FXState **pi440fx_state,
                     int *piix3_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
@@ -320,7 +319,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     unsigned i;
     I440FXState *i440fx;
 
-    dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
+    dev = qdev_create(NULL, host_type);
     s = PCI_HOST_BRIDGE(dev);
     b = pci_bus_new(dev, NULL, pci_address_space,
                     address_space_io, 0, TYPE_PCI_BUS);
@@ -328,7 +327,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
     qdev_init_nofail(dev);
 
-    d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
+    d = pci_create_simple(b, 0, pci_type);
     *pi440fx_state = I440FX_PCI_DEVICE(d);
     f = *pi440fx_state;
     f->system_memory = address_space_mem;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 15e3352..449bf4a 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -227,7 +227,11 @@ extern int no_hpet;
 struct PCII440FXState;
 typedef struct PCII440FXState PCII440FXState;
 
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
+#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
+#define TYPE_I440FX_PCI_DEVICE "i440FX"
+
+PCIBus *i440fx_init(const char *host_type, const char *pci_type,
+                    PCII440FXState **pi440fx_state, int *piix_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 02/10] pc_init1: pass parameters just with types
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 01/10] i440fx: make types configurable at run-time Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough Tiejun Chen
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

Pass types to configure pc_init1().

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
v10:
 
* Nothing is changed.

v9:
 
* Just rebase on the latest.

 hw/i386/pc_piix.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a36fad2..8b1f1cd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -76,7 +76,8 @@ static bool has_reserved_memory = true;
 static bool kvmclock_enabled = true;
 
 /* PC hardware initialisation */
-static void pc_init1(MachineState *machine)
+static void pc_init1(MachineState *machine,
+                     const char *host_type, const char *pci_type)
 {
     PCMachineState *pc_machine = PC_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
@@ -200,8 +201,8 @@ static void pc_init1(MachineState *machine)
     }
 
     if (pci_enabled) {
-        pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE,
-                              TYPE_I440FX_PCI_DEVICE,
+        pci_bus = i440fx_init(host_type,
+                              pci_type,
                               &i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, machine->ram_size,
                               below_4g_mem_size,
@@ -443,7 +444,7 @@ static void pc_init_isa(MachineState *machine)
     }
     x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
     enable_compat_apic_id_mode();
-    pc_init1(machine);
+    pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, TYPE_I440FX_PCI_DEVICE);
 }
 
 #ifdef CONFIG_XEN
@@ -451,7 +452,7 @@ static void pc_xen_hvm_init(MachineState *machine)
 {
     PCIBus *bus;
 
-    pc_init1(machine);
+    pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, TYPE_I440FX_PCI_DEVICE);
 
     bus = pci_find_primary_bus();
     if (bus != NULL) {
@@ -467,7 +468,8 @@ static void pc_xen_hvm_init(MachineState *machine)
         if (compat) { \
             compat(machine); \
         } \
-        pc_init1(machine); \
+        pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, \
+                 TYPE_I440FX_PCI_DEVICE); \
     } \
     DEFINE_PC_MACHINE(suffix, name, pc_init_##suffix, optionfn)
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 01/10] i440fx: make types configurable at run-time Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 02/10] pc_init1: pass parameters just with types Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2016-02-09  3:32   ` Alex Williamson
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 04/10] hw/pci-assign: split pci-assign.c Tiejun Chen
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

Implement a pci host bridge specific to passthrough. Actually
this just inherits the standard one. And we also just expose
a minimal real host bridge pci configuration subset.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
v10:
 
* Nothing is changed.

v9:
 
* Just rebase on the latest.

 hw/pci-host/piix.c   | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h |  2 ++
 2 files changed, 84 insertions(+)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index a203d93..7adf645 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -734,6 +734,87 @@ 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");
+
+    if (rc >= size || rc < 0) {
+        return -ENODEV;
+    }
+
+    config_fd = open(path, O_RDWR);
+    if (config_fd < 0) {
+        return -ENODEV;
+    }
+
+    do {
+        rc = pread(config_fd, (uint8_t *)&val, len, pos);
+    } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
+    if (rc != len) {
+        return -errno;
+    }
+
+    return 0;
+}
+
+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)
 {
@@ -775,6 +856,7 @@ 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);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 449bf4a..2db3f6d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -230,6 +230,8 @@ typedef struct PCII440FXState PCII440FXState;
 #define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
 #define TYPE_I440FX_PCI_DEVICE "i440FX"
 
+#define TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE "igd-passthrough-i440FX"
+
 PCIBus *i440fx_init(const char *host_type, const char *pci_type,
                     PCII440FXState **pi440fx_state, int *piix_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 04/10] hw/pci-assign: split pci-assign.c
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
                   ` (2 preceding siblings ...)
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 05/10] xen, gfx passthrough: basic graphics passthrough support Tiejun Chen
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

We will try to reuse assign_dev_load_option_rom in xen side, and
especially its a good beginning to unify pci assign codes both on
kvm and xen in the future.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
v10:
 
* Nothing is changed.

v9:
 
* Just rebase on the latest.

 hw/i386/Makefile.objs         |  1 +
 hw/i386/kvm/pci-assign.c      | 82 ++++----------------------------------
 hw/i386/pci-assign-load-rom.c | 93 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci-assign.h   | 27 +++++++++++++
 4 files changed, 128 insertions(+), 75 deletions(-)
 create mode 100644 hw/i386/pci-assign-load-rom.c
 create mode 100644 include/hw/pci/pci-assign.h

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index bd4f147..cebad90 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -7,6 +7,7 @@ obj-$(CONFIG_XEN) += ../xenpv/ xen/
 
 obj-y += kvmvapic.o
 obj-y += acpi-build.o
+obj-y += pci-assign-load-rom.o
 hw/i386/acpi-build.o: hw/i386/acpi-build.c \
 	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex
 
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 74d22f4..b1beaa6 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -37,6 +37,7 @@
 #include "hw/pci/pci.h"
 #include "hw/pci/msi.h"
 #include "kvm_i386.h"
+#include "hw/pci/pci-assign.h"
 
 #define MSIX_PAGE_SIZE 0x1000
 
@@ -48,17 +49,6 @@
 #define IORESOURCE_PREFETCH 0x00002000  /* No side effects */
 #define IORESOURCE_MEM_64   0x00100000
 
-//#define DEVICE_ASSIGNMENT_DEBUG
-
-#ifdef DEVICE_ASSIGNMENT_DEBUG
-#define DEBUG(fmt, ...)                                       \
-    do {                                                      \
-        fprintf(stderr, "%s: " fmt, __func__ , __VA_ARGS__);  \
-    } while (0)
-#else
-#define DEBUG(fmt, ...)
-#endif
-
 typedef struct PCIRegion {
     int type;           /* Memory or port I/O */
     int valid;
@@ -1896,73 +1886,15 @@ static void assign_register_types(void)
 
 type_init(assign_register_types)
 
-/*
- * Scan the assigned devices for the devices that have an option ROM, and then
- * load the corresponding ROM data to RAM. If an error occurs while loading an
- * option ROM, we just ignore that option ROM and continue with the next one.
- */
 static void assigned_dev_load_option_rom(AssignedDevice *dev)
 {
-    char name[32], rom_file[64];
-    FILE *fp;
-    uint8_t val;
-    struct stat st;
-    void *ptr;
-
-    /* If loading ROM from file, pci handles it */
-    if (dev->dev.romfile || !dev->dev.rom_bar) {
-        return;
-    }
+    int size = 0;
 
-    snprintf(rom_file, sizeof(rom_file),
-             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",
-             dev->host.domain, dev->host.bus, dev->host.slot,
-             dev->host.function);
+    pci_assign_dev_load_option_rom(&dev->dev, OBJECT(dev), &size,
+                                   dev->host.domain, dev->host.bus,
+                                   dev->host.slot, dev->host.function);
 
-    if (stat(rom_file, &st)) {
-        return;
-    }
-
-    if (access(rom_file, F_OK)) {
-        error_report("pci-assign: Insufficient privileges for %s", rom_file);
-        return;
-    }
-
-    /* Write "1" to the ROM file to enable it */
-    fp = fopen(rom_file, "r+");
-    if (fp == NULL) {
-        return;
+    if (!size) {
+        error_report("pci-assign: Invalid ROM.");
     }
-    val = 1;
-    if (fwrite(&val, 1, 1, fp) != 1) {
-        goto close_rom;
-    }
-    fseek(fp, 0, SEEK_SET);
-
-    snprintf(name, sizeof(name), "%s.rom",
-            object_get_typename(OBJECT(dev)));
-    memory_region_init_ram(&dev->dev.rom, OBJECT(dev), name, st.st_size,
-                           &error_abort);
-    vmstate_register_ram(&dev->dev.rom, &dev->dev.qdev);
-    ptr = memory_region_get_ram_ptr(&dev->dev.rom);
-    memset(ptr, 0xff, st.st_size);
-
-    if (!fread(ptr, 1, st.st_size, fp)) {
-        error_report("pci-assign: Cannot read from host %s", rom_file);
-        error_printf("Device option ROM contents are probably invalid "
-                     "(check dmesg).\nSkip option ROM probe with rombar=0, "
-                     "or load from file with romfile=\n");
-        goto close_rom;
-    }
-
-    pci_register_bar(&dev->dev, PCI_ROM_SLOT, 0, &dev->dev.rom);
-    dev->dev.has_rom = true;
-close_rom:
-    /* Write "0" to disable ROM */
-    fseek(fp, 0, SEEK_SET);
-    val = 0;
-    if (!fwrite(&val, 1, 1, fp)) {
-        DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
-    }
-    fclose(fp);
 }
diff --git a/hw/i386/pci-assign-load-rom.c b/hw/i386/pci-assign-load-rom.c
new file mode 100644
index 0000000..bad53b7
--- /dev/null
+++ b/hw/i386/pci-assign-load-rom.c
@@ -0,0 +1,93 @@
+/*
+ * This is splited from hw/i386/kvm/pci-assign.c
+ */
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/io.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include "hw/hw.h"
+#include "hw/i386/pc.h"
+#include "qemu/error-report.h"
+#include "ui/console.h"
+#include "hw/loader.h"
+#include "monitor/monitor.h"
+#include "qemu/range.h"
+#include "sysemu/sysemu.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci-assign.h"
+
+/*
+ * Scan the assigned devices for the devices that have an option ROM, and then
+ * load the corresponding ROM data to RAM. If an error occurs while loading an
+ * option ROM, we just ignore that option ROM and continue with the next one.
+ */
+void *pci_assign_dev_load_option_rom(PCIDevice *dev, struct Object *owner,
+                                     int *size, unsigned int domain,
+                                     unsigned int bus, unsigned int slot,
+                                     unsigned int function)
+{
+    char name[32], rom_file[64];
+    FILE *fp;
+    uint8_t val;
+    struct stat st;
+    void *ptr = NULL;
+
+    /* If loading ROM from file, pci handles it */
+    if (dev->romfile || !dev->rom_bar) {
+        return NULL;
+    }
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",
+             domain, bus, slot, function);
+
+    if (stat(rom_file, &st)) {
+        return NULL;
+    }
+
+    if (access(rom_file, F_OK)) {
+        error_report("pci-assign: Insufficient privileges for %s", rom_file);
+        return NULL;
+    }
+
+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {
+        return NULL;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    snprintf(name, sizeof(name), "%s.rom", object_get_typename(owner));
+    memory_region_init_ram(&dev->rom, owner, name, st.st_size, &error_abort);
+    vmstate_register_ram(&dev->rom, &dev->qdev);
+    ptr = memory_region_get_ram_ptr(&dev->rom);
+    memset(ptr, 0xff, st.st_size);
+
+    if (!fread(ptr, 1, st.st_size, fp)) {
+        error_report("pci-assign: Cannot read from host %s", rom_file);
+        error_printf("Device option ROM contents are probably invalid "
+                     "(check dmesg).\nSkip option ROM probe with rombar=0, "
+                     "or load from file with romfile=\n");
+        goto close_rom;
+    }
+
+    pci_register_bar(dev, PCI_ROM_SLOT, 0, &dev->rom);
+    dev->has_rom = true;
+    *size = st.st_size;
+close_rom:
+    /* Write "0" to disable ROM */
+    fseek(fp, 0, SEEK_SET);
+    val = 0;
+    if (!fwrite(&val, 1, 1, fp)) {
+        DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
+    }
+    fclose(fp);
+
+    return ptr;
+}
diff --git a/include/hw/pci/pci-assign.h b/include/hw/pci/pci-assign.h
new file mode 100644
index 0000000..55f42c5
--- /dev/null
+++ b/include/hw/pci/pci-assign.h
@@ -0,0 +1,27 @@
+/*
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Just split from hw/i386/kvm/pci-assign.c.
+ */
+#ifndef PCI_ASSIGN_H
+#define PCI_ASSIGN_H
+
+#include "hw/pci/pci.h"
+
+//#define DEVICE_ASSIGNMENT_DEBUG
+
+#ifdef DEVICE_ASSIGNMENT_DEBUG
+#define DEBUG(fmt, ...)                                       \
+    do {                                                      \
+        fprintf(stderr, "%s: " fmt, __func__ , __VA_ARGS__);  \
+    } while (0)
+#else
+#define DEBUG(fmt, ...)
+#endif
+
+void *pci_assign_dev_load_option_rom(PCIDevice *dev, struct Object *owner,
+                                     int *size, unsigned int domain,
+                                     unsigned int bus, unsigned int slot,
+                                     unsigned int function);
+#endif /* PCI_ASSIGN_H */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 05/10] xen, gfx passthrough: basic graphics passthrough support
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
                   ` (3 preceding siblings ...)
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 04/10] hw/pci-assign: split pci-assign.c Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 06/10] xen, gfx passthrough: retrieve VGA BIOS to work Tiejun Chen
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

basic gfx passthrough support:
- add a vga type for gfx passthrough
- register/unregister legacy VGA I/O ports and MMIOs for passthrough GFX

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v10:
 
* Nothing is changed.

v9:
 
* Just rebase on the latest.

 hw/core/machine.c            |  20 ++++++++
 hw/xen/Makefile.objs         |   1 +
 hw/xen/xen-host-pci-device.c |   5 ++
 hw/xen/xen-host-pci-device.h |   1 +
 hw/xen/xen_pt.c              |   4 ++
 hw/xen/xen_pt.h              |  10 +++-
 hw/xen/xen_pt_graphics.c     | 111 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/boards.h          |   1 +
 qemu-options.hx              |   3 ++
 vl.c                         |  10 ++++
 10 files changed, 165 insertions(+), 1 deletion(-)
 create mode 100644 hw/xen/xen_pt_graphics.c

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ac4654e..51ed6b2 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -226,6 +226,20 @@ static void machine_set_usb(Object *obj, bool value, Error **errp)
     ms->usb_disabled = !value;
 }
 
+static bool machine_get_igd_gfx_passthru(Object *obj, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    return ms->igd_gfx_passthru;
+}
+
+static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    ms->igd_gfx_passthru = value;
+}
+
 static char *machine_get_firmware(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -388,6 +402,12 @@ static void machine_initfn(Object *obj)
     object_property_set_description(obj, "usb",
                                     "Set on/off to enable/disable usb",
                                     NULL);
+    object_property_add_bool(obj, "igd-passthru",
+                             machine_get_igd_gfx_passthru,
+                             machine_set_igd_gfx_passthru, NULL);
+    object_property_set_description(obj, "igd-passthru",
+                                    "Set on/off to enable/disable igd passthrou",
+                                    NULL);
     object_property_add_str(obj, "firmware",
                             machine_get_firmware,
                             machine_set_firmware, NULL);
diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
index a0ca0aa..a9ad7e7 100644
--- a/hw/xen/Makefile.objs
+++ b/hw/xen/Makefile.objs
@@ -3,3 +3,4 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
 
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
+obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o xen_pt_graphics.o
diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 743b37b..a54b7de 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -376,6 +376,11 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
         goto error;
     }
     d->irq = v;
+    rc = xen_host_pci_get_hex_value(d, "class", &v);
+    if (rc) {
+        goto error;
+    }
+    d->class_code = v;
     d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
 
     return 0;
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index c2486f0..f1e1c30 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -25,6 +25,7 @@ typedef struct XenHostPCIDevice {
 
     uint16_t vendor_id;
     uint16_t device_id;
+    uint32_t class_code;
     int irq;
 
     XenHostPCIIORegion io_regions[PCI_NUM_REGIONS - 1];
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index ed5fcae..42380c3 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -502,6 +502,7 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
                    d->rom.size, d->rom.base_addr);
     }
 
+    xen_pt_register_vga_regions(d);
     return 0;
 }
 
@@ -801,6 +802,7 @@ out:
 static void xen_pt_unregister_device(PCIDevice *d)
 {
     XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
+    XenHostPCIDevice *host_dev = &s->real_device;
     uint8_t machine_irq = s->machine_irq;
     uint8_t intx = xen_pt_pci_intx(s);
     int rc;
@@ -844,6 +846,8 @@ static void xen_pt_unregister_device(PCIDevice *d)
     /* delete all emulated config registers */
     xen_pt_config_delete(s);
 
+    xen_pt_unregister_vga_regions(host_dev);
+
     memory_listener_unregister(&s->memory_listener);
     memory_listener_unregister(&s->io_listener);
 
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 393f36c..5eb3c52 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -305,5 +305,13 @@ static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
     return s->msix && s->msix->bar_index == bar;
 }
 
-
+extern bool has_igd_gfx_passthru;
+static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
+{
+    return (has_igd_gfx_passthru
+            && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
+}
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);
+int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev);
 #endif /* !XEN_PT_H */
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
new file mode 100644
index 0000000..9b3df81
--- /dev/null
+++ b/hw/xen/xen_pt_graphics.c
@@ -0,0 +1,111 @@
+/*
+ * graphics passthrough
+ */
+#include "xen_pt.h"
+#include "xen-host-pci-device.h"
+#include "hw/xen/xen_backend.h"
+
+typedef struct VGARegion {
+    int type;           /* Memory or port I/O */
+    uint64_t guest_base_addr;
+    uint64_t machine_base_addr;
+    uint64_t size;    /* size of the region */
+    int rc;
+} VGARegion;
+
+#define IORESOURCE_IO           0x00000100
+#define IORESOURCE_MEM          0x00000200
+
+static struct VGARegion vga_args[] = {
+    {
+        .type = IORESOURCE_IO,
+        .guest_base_addr = 0x3B0,
+        .machine_base_addr = 0x3B0,
+        .size = 0xC,
+        .rc = -1,
+    },
+    {
+        .type = IORESOURCE_IO,
+        .guest_base_addr = 0x3C0,
+        .machine_base_addr = 0x3C0,
+        .size = 0x20,
+        .rc = -1,
+    },
+    {
+        .type = IORESOURCE_MEM,
+        .guest_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+        .machine_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+        .size = 0x20,
+        .rc = -1,
+    },
+};
+
+/*
+ * register VGA resources for the domain with assigned gfx
+ */
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
+{
+    int i = 0;
+
+    if (!is_igd_vga_passthrough(dev)) {
+        return 0;
+    }
+
+    for (i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+        if (vga_args[i].type == IORESOURCE_IO) {
+            vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_ADD_MAPPING);
+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_ADD_MAPPING);
+        }
+
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s mapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+                    vga_args[i].rc);
+            return vga_args[i].rc;
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx
+ */
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
+{
+    int i = 0;
+
+    if (!is_igd_vga_passthrough(dev)) {
+        return 0;
+    }
+
+    for (i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+        if (vga_args[i].type == IORESOURCE_IO) {
+            vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_REMOVE_MAPPING);
+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_REMOVE_MAPPING);
+        }
+
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s unmapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+                    vga_args[i].rc);
+            return vga_args[i].rc;
+        }
+    }
+
+    return 0;
+}
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2aec9cb..4a0ee05 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -136,6 +136,7 @@ struct MachineState {
     bool mem_merge;
     bool usb;
     bool usb_disabled;
+    bool igd_gfx_passthru;
     char *firmware;
     bool iommu;
     bool suppress_vmdesc;
diff --git a/qemu-options.hx b/qemu-options.hx
index 7b8efbf..e4cfa90 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -38,6 +38,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
     "                mem-merge=on|off controls memory merge support (default: on)\n"
     "                iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n"
+    "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
     "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
     "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
     "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n",
@@ -55,6 +56,8 @@ than one accelerator specified, the next one is used if the previous one fails
 to initialize.
 @item kernel_irqchip=on|off
 Enables in-kernel irqchip support for the chosen accelerator when available.
+@item gfx_passthru=on|off
+Enables IGD GFX passthrough support for the chosen machine when available.
 @item vmport=on|off|auto
 Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
 value based on accel. For accel=xen the default is off otherwise the default
diff --git a/vl.c b/vl.c
index 3f269dc..9dc5b9b 100644
--- a/vl.c
+++ b/vl.c
@@ -1338,6 +1338,13 @@ 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 */
 
@@ -4528,6 +4535,9 @@ 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.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 06/10] xen, gfx passthrough: retrieve VGA BIOS to work
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
                   ` (4 preceding siblings ...)
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 05/10] xen, gfx passthrough: basic graphics passthrough support Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 07/10] igd gfx passthrough: create a isa bridge Tiejun Chen
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

Now we retrieve VGA bios like kvm stuff in qemu but we need to
fix Device Identification in case if its not matched with the
real IGD device since Seabios is always trying to compare this
ID to work out VGA BIOS.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v10:
 
* Nothing is changed.

v9:
 
* Just rebase on the latest.

 hw/xen/xen_pt.c          | 10 ++++++
 hw/xen/xen_pt.h          |  5 +++
 hw/xen/xen_pt_graphics.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 42380c3..15b02cb 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -725,6 +725,16 @@ static int xen_pt_initfn(PCIDevice *d)
     s->memory_listener = xen_pt_memory_listener;
     s->io_listener = xen_pt_io_listener;
 
+    /* Setup VGA bios for passthrough GFX */
+    if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
+        (s->real_device.dev == 2) && (s->real_device.func == 0)) {
+        if (xen_pt_setup_vga(s, &s->real_device) < 0) {
+            XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+            xen_host_pci_device_put(&s->real_device);
+            return -1;
+        }
+    }
+
     /* Handle real device's MMIO/PIO BARs */
     xen_pt_register_regions(s, &cmd);
 
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 5eb3c52..a33e95c 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -305,6 +305,11 @@ static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
     return s->msix && s->msix->bar_index == bar;
 }
 
+extern void *pci_assign_dev_load_option_rom(PCIDevice *dev,
+                                            struct Object *owner, int *size,
+                                            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)
 {
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index 9b3df81..3232296 100644
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -109,3 +109,82 @@ int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
 
     return 0;
 }
+
+static void *get_vgabios(XenPCIPassthroughState *s, int *size,
+                       XenHostPCIDevice *dev)
+{
+    return pci_assign_dev_load_option_rom(&s->dev, OBJECT(&s->dev), size,
+                                          dev->domain, dev->bus,
+                                          dev->dev, dev->func);
+}
+
+/* Refer to Seabios. */
+struct rom_header {
+    uint16_t signature;
+    uint8_t size;
+    uint8_t initVector[4];
+    uint8_t reserved[17];
+    uint16_t pcioffset;
+    uint16_t pnpoffset;
+} __attribute__((packed));
+
+struct pci_data {
+    uint32_t signature;
+    uint16_t vendor;
+    uint16_t device;
+    uint16_t vitaldata;
+    uint16_t dlen;
+    uint8_t drevision;
+    uint8_t class_lo;
+    uint16_t class_hi;
+    uint16_t ilen;
+    uint16_t irevision;
+    uint8_t type;
+    uint8_t indicator;
+    uint16_t reserved;
+} __attribute__((packed));
+
+int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
+{
+    unsigned char *bios = NULL;
+    struct rom_header *rom;
+    int bios_size;
+    char *c = NULL;
+    char checksum = 0;
+    uint32_t len = 0;
+    struct pci_data *pd = NULL;
+
+    if (!is_igd_vga_passthrough(dev)) {
+        return -1;
+    }
+
+    bios = get_vgabios(s, &bios_size, dev);
+    if (!bios) {
+        XEN_PT_ERR(&s->dev, "VGA: Can't getting VBIOS!\n");
+        return -1;
+    }
+
+    /* Currently we fixed this address as a primary. */
+    rom = (struct rom_header *)bios;
+    pd = (void *)(bios + (unsigned char)rom->pcioffset);
+
+    /* We may need to fixup Device Identification. */
+    if (pd->device != s->real_device.device_id) {
+        pd->device = s->real_device.device_id;
+
+        len = rom->size * 512;
+        /* Then adjust the bios checksum */
+        for (c = (char *)bios; c < ((char *)bios + len); c++) {
+            checksum += *c;
+        }
+        if (checksum) {
+            bios[len - 1] -= checksum;
+            XEN_PT_LOG(&s->dev, "vga bios checksum is adjusted %x!\n",
+                       checksum);
+        }
+    }
+
+    /* Currently we fixed this address as a primary for legacy BIOS. */
+    cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
+    return 0;
+}
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 07/10] igd gfx passthrough: create a isa bridge
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
                   ` (5 preceding siblings ...)
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 06/10] xen, gfx passthrough: retrieve VGA BIOS to work Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 08/10] xen, gfx passthrough: register " Tiejun Chen
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

Currently IGD drivers always need to access PCH by 1f.0. But we
don't want to poke that directly to get ID, and although 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).

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
v10:
 
* extern igd_passthrough_isa_bridge_create() in the
  include/hw/i386/pc.h file

v9:
 
* Just rebase on the latest.

 hw/i386/pc_piix.c    | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h |   1 +
 2 files changed, 113 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8b1f1cd..8d6f629 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -910,6 +910,118 @@ 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)
 {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 2db3f6d..a842aed 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -564,4 +564,5 @@ static inline void pc_default_machine_options(MachineClass *m)
     (m)->compat_props = props; \
 } while (0)
 
+extern void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id);
 #endif
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 08/10] xen, gfx passthrough: register a isa bridge
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
                   ` (6 preceding siblings ...)
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 07/10] igd gfx passthrough: create a isa bridge Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 09/10] xen, gfx passthrough: register host bridge specific to passthrough Tiejun Chen
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

Currently we just register this isa bridge when we use IGD
passthrough in Xen side.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v10:

* Don't extern igd_passthrough_isa_bridge_create() in the
  include/hw/xen/xen.h file. Instead, move inside the
  include/hw/i386/pc.h file in patch #7

v9:

* Move is_igd_vga_passthrough(dev)) from xen_igd_passthrough_isa_bridge_create()
  into xen_pt_initfn().

 hw/xen/xen_pt.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 15b02cb..d67bccf 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -56,6 +56,7 @@
 
 #include "hw/pci/pci.h"
 #include "hw/xen/xen.h"
+#include "hw/i386/pc.h"
 #include "hw/xen/xen_backend.h"
 #include "xen_pt.h"
 #include "qemu/range.h"
@@ -684,6 +685,17 @@ static const MemoryListener xen_pt_io_listener = {
     .priority = 10,
 };
 
+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);
+}
+
 /* init */
 
 static int xen_pt_initfn(PCIDevice *d)
@@ -728,11 +740,21 @@ static int xen_pt_initfn(PCIDevice *d)
     /* Setup VGA bios for passthrough GFX */
     if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
         (s->real_device.dev == 2) && (s->real_device.func == 0)) {
+        if (!is_igd_vga_passthrough(&s->real_device)) {
+            XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
+                       " to passthrough IGD GFX.\n");
+            xen_host_pci_device_put(&s->real_device);
+            return -1;
+        }
+
         if (xen_pt_setup_vga(s, &s->real_device) < 0) {
             XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
             xen_host_pci_device_put(&s->real_device);
             return -1;
         }
+
+        /* Register ISA bridge for passthrough GFX. */
+        xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
     }
 
     /* Handle real device's MMIO/PIO BARs */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 09/10] xen, gfx passthrough: register host bridge specific to passthrough
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
                   ` (7 preceding siblings ...)
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 08/10] xen, gfx passthrough: register " Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 10/10] xen, gfx passthrough: add opregion mapping Tiejun Chen
  2015-07-15 11:46 ` [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Stefano Stabellini
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

Just register that pci host bridge specific to passthrough.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v10:
 
* Nothing is changed.

v9:

* Simplify pc_xen_hvm_init_pci()

 hw/i386/pc_piix.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8d6f629..631bd8d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -50,7 +50,8 @@
 #include "cpu.h"
 #include "qemu/error-report.h"
 #ifdef CONFIG_XEN
-#  include <xen/hvm/hvm_info_table.h>
+#include <xen/hvm/hvm_info_table.h>
+#include "hw/xen/xen_pt.h"
 #endif
 #include "migration/migration.h"
 
@@ -448,11 +449,21 @@ 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 ?
+                TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : TYPE_I440FX_PCI_DEVICE;
+
+    pc_init1(machine,
+             TYPE_I440FX_PCI_HOST_BRIDGE,
+             pci_type);
+}
+
 static void pc_xen_hvm_init(MachineState *machine)
 {
     PCIBus *bus;
 
-    pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, TYPE_I440FX_PCI_DEVICE);
+    pc_xen_hvm_init_pci(machine);
 
     bus = pci_find_primary_bus();
     if (bus != NULL) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [v10][PATCH 10/10] xen, gfx passthrough: add opregion mapping
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
                   ` (8 preceding siblings ...)
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 09/10] xen, gfx passthrough: register host bridge specific to passthrough Tiejun Chen
@ 2015-07-15  5:37 ` Tiejun Chen
  2015-07-15 11:46 ` [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Stefano Stabellini
  10 siblings, 0 replies; 25+ messages in thread
From: Tiejun Chen @ 2015-07-15  5:37 UTC (permalink / raw)
  To: stefano.stabellini, mst, pbonzini, ehabkost, rth; +Cc: qemu-devel

The OpRegion shouldn't be mapped 1:1 because the address in the host
can't be used in the guest directly.

This patch traps read and write access to the opregion of the Intel
GPU config space (offset 0xfc).

The original patch is from Jean Guyader <jean.guyader@eu.citrix.com>

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v10:
 
* Nothing is changed.

v9:
 
* Just rebase on the latest.

 hw/xen/xen_pt.h             |  6 +++-
 hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++++--
 hw/xen/xen_pt_graphics.c    | 82 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+), 3 deletions(-)

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index a33e95c..e89d231 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -40,6 +40,9 @@ typedef struct XenPCIPassthroughState XenPCIPassthroughState;
 #define XEN_PT_DEVICE(obj) \
     OBJECT_CHECK(XenPCIPassthroughState, (obj), TYPE_XEN_PT_DEVICE)
 
+uint32_t igd_read_opregion(XenPCIPassthroughState *s);
+void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val);
+
 /* function type for config reg */
 typedef int (*xen_pt_conf_reg_init)
     (XenPCIPassthroughState *, XenPTRegInfo *, uint32_t real_offset,
@@ -66,8 +69,9 @@ typedef int (*xen_pt_conf_byte_read)
 #define XEN_PT_BAR_ALLF 0xFFFFFFFF
 #define XEN_PT_BAR_UNMAPPED (-1)
 
-#define PCI_CAP_MAX 48
+#define XEN_PCI_CAP_MAX 48
 
+#define XEN_PCI_INTEL_OPREGION 0xfc
 
 typedef enum {
     XEN_PT_GRP_TYPE_HARDWIRED = 0,  /* 0 Hardwired reg group */
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index dd37be3..9fb3670 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -552,6 +552,22 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
     return 0;
 }
 
+static int xen_pt_intel_opregion_read(XenPCIPassthroughState *s,
+                                      XenPTReg *cfg_entry,
+                                      uint32_t *value, uint32_t valid_mask)
+{
+    *value = igd_read_opregion(s);
+    return 0;
+}
+
+static int xen_pt_intel_opregion_write(XenPCIPassthroughState *s,
+                                       XenPTReg *cfg_entry, uint32_t *value,
+                                       uint32_t dev_value, uint32_t valid_mask)
+{
+    igd_write_opregion(s, *value);
+    return 0;
+}
+
 /* Header Type0 reg static information table */
 static XenPTRegInfo xen_pt_emu_reg_header0[] = {
     /* Vendor ID reg */
@@ -1492,6 +1508,19 @@ static XenPTRegInfo xen_pt_emu_reg_msix[] = {
     },
 };
 
+static XenPTRegInfo xen_pt_emu_reg_igd_opregion[] = {
+    /* Intel IGFX OpRegion reg */
+    {
+        .offset     = 0x0,
+        .size       = 4,
+        .init_val   = 0,
+        .u.dw.read   = xen_pt_intel_opregion_read,
+        .u.dw.write  = xen_pt_intel_opregion_write,
+    },
+    {
+        .size = 0,
+    },
+};
 
 /****************************
  * Capabilities
@@ -1729,6 +1758,14 @@ static const XenPTRegGroupInfo xen_pt_emu_reg_grps[] = {
         .size_init   = xen_pt_msix_size_init,
         .emu_regs = xen_pt_emu_reg_msix,
     },
+    /* Intel IGD Opregion group */
+    {
+        .grp_id      = XEN_PCI_INTEL_OPREGION,
+        .grp_type    = XEN_PT_GRP_TYPE_EMU,
+        .grp_size    = 0x4,
+        .size_init   = xen_pt_reg_grp_size_init,
+        .emu_regs    = xen_pt_emu_reg_igd_opregion,
+    },
     {
         .grp_size = 0,
     },
@@ -1779,7 +1816,7 @@ out:
 static uint8_t find_cap_offset(XenPCIPassthroughState *s, uint8_t cap)
 {
     uint8_t id;
-    unsigned max_cap = PCI_CAP_MAX;
+    unsigned max_cap = XEN_PCI_CAP_MAX;
     uint8_t pos = PCI_CAPABILITY_LIST;
     uint8_t status = 0;
 
@@ -1858,7 +1895,8 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
         uint32_t reg_grp_offset = 0;
         XenPTRegGroup *reg_grp_entry = NULL;
 
-        if (xen_pt_emu_reg_grps[i].grp_id != 0xFF) {
+        if (xen_pt_emu_reg_grps[i].grp_id != 0xFF
+            && xen_pt_emu_reg_grps[i].grp_id != XEN_PCI_INTEL_OPREGION) {
             if (xen_pt_hide_dev_cap(&s->real_device,
                                     xen_pt_emu_reg_grps[i].grp_id)) {
                 continue;
@@ -1871,6 +1909,15 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
             }
         }
 
+        /*
+         * By default we will trap up to 0x40 in the cfg space.
+         * If an intel device is pass through we need to trap 0xfc,
+         * therefore the size should be 0xff.
+         */
+        if (xen_pt_emu_reg_grps[i].grp_id == XEN_PCI_INTEL_OPREGION) {
+            reg_grp_offset = XEN_PCI_INTEL_OPREGION;
+        }
+
         reg_grp_entry = g_new0(XenPTRegGroup, 1);
         QLIST_INIT(&reg_grp_entry->reg_tbl_list);
         QLIST_INSERT_HEAD(&s->reg_grps, reg_grp_entry, entries);
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index 3232296..df6069b 100644
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -5,6 +5,11 @@
 #include "xen-host-pci-device.h"
 #include "hw/xen/xen_backend.h"
 
+static unsigned long igd_guest_opregion;
+static unsigned long igd_host_opregion;
+
+#define XEN_PCI_INTEL_OPREGION_MASK 0xfff
+
 typedef struct VGARegion {
     int type;           /* Memory or port I/O */
     uint64_t guest_base_addr;
@@ -81,6 +86,7 @@ int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
 int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
 {
     int i = 0;
+    int ret = 0;
 
     if (!is_igd_vga_passthrough(dev)) {
         return 0;
@@ -107,6 +113,17 @@ int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
         }
     }
 
+    if (igd_guest_opregion) {
+        ret = xc_domain_memory_mapping(xen_xc, xen_domid,
+                (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT),
+                (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
+                3,
+                DPCI_REMOVE_MAPPING);
+        if (ret) {
+            return ret;
+        }
+    }
+
     return 0;
 }
 
@@ -188,3 +205,68 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
     cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
     return 0;
 }
+
+uint32_t igd_read_opregion(XenPCIPassthroughState *s)
+{
+    uint32_t val = 0;
+
+    if (!igd_guest_opregion) {
+        return val;
+    }
+
+    val = igd_guest_opregion;
+
+    XEN_PT_LOG(&s->dev, "Read opregion val=%x\n", val);
+    return val;
+}
+
+#define XEN_PCI_INTEL_OPREGION_PAGES 0x3
+#define XEN_PCI_INTEL_OPREGION_ENABLE_ACCESSED 0x1
+void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val)
+{
+    int ret;
+
+    if (igd_guest_opregion) {
+        XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring %x\n",
+                   val);
+        return;
+    }
+
+    /* We just work with LE. */
+    xen_host_pci_get_block(&s->real_device, XEN_PCI_INTEL_OPREGION,
+            (uint8_t *)&igd_host_opregion, 4);
+    igd_guest_opregion = (unsigned long)(val & ~XEN_PCI_INTEL_OPREGION_MASK)
+                            | (igd_host_opregion & XEN_PCI_INTEL_OPREGION_MASK);
+
+    ret = xc_domain_iomem_permission(xen_xc, xen_domid,
+            (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
+            XEN_PCI_INTEL_OPREGION_PAGES,
+            XEN_PCI_INTEL_OPREGION_ENABLE_ACCESSED);
+
+    if (ret) {
+        XEN_PT_ERR(&s->dev, "[%d]:Can't enable to access IGD host opregion:"
+                    " 0x%lx.\n", ret,
+                    (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT)),
+        igd_guest_opregion = 0;
+        return;
+    }
+
+    ret = xc_domain_memory_mapping(xen_xc, xen_domid,
+            (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT),
+            (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
+            XEN_PCI_INTEL_OPREGION_PAGES,
+            DPCI_ADD_MAPPING);
+
+    if (ret) {
+        XEN_PT_ERR(&s->dev, "[%d]:Can't map IGD host opregion:0x%lx to"
+                    " guest opregion:0x%lx.\n", ret,
+                    (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
+                    (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT));
+        igd_guest_opregion = 0;
+        return;
+    }
+
+    XEN_PT_LOG(&s->dev, "Map OpRegion: 0x%lx -> 0x%lx\n",
+                    (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
+                    (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT));
+}
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough
  2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
                   ` (9 preceding siblings ...)
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 10/10] xen, gfx passthrough: add opregion mapping Tiejun Chen
@ 2015-07-15 11:46 ` Stefano Stabellini
  2015-07-15 12:01   ` Michael S. Tsirkin
  10 siblings, 1 reply; 25+ messages in thread
From: Stefano Stabellini @ 2015-07-15 11:46 UTC (permalink / raw)
  To: Tiejun Chen; +Cc: ehabkost, mst, stefano.stabellini, qemu-devel, pbonzini, rth

Thanks Tiejun, the patch series looks OK to me now.

It looks like it has all the required acks.
Michael, are you OK with it? If so, should I add it to my queue, or do
you want to add it to yours?

On Wed, 15 Jul 2015, Tiejun Chen wrote:
> v10:
> 
> * Don't extern igd_passthrough_isa_bridge_create() in the
>   include/hw/xen/xen.h file. Instead, move inside the
>   include/hw/i386/pc.h file in patch #7
> 
> v9:
> 
> * Rebase on the latest
> * Inside patch #8, move is_igd_vga_passthrough(dev)) from
>   xen_igd_passthrough_isa_bridge_create() into xen_pt_initfn().
> * Inside patch #9, simplify pc_xen_hvm_init_pci()
> * Michael acked them on pc side
> * Stefano ackes then on xen side
> 
> v8:
> 
> * Rebase on the latest qemu tree
> * Cleanup one xen leftover in patch #3
> 
> v7:
> 
> * Instead of "-gfx_passthru" we'd like to make that a machine
>   option, "-machine xxx,igd-passthru=on"" 
> * try to make something as common shared by others like KvmGT in
>   the future
> * Just read those real value from host bridge pci
>   configuration space when create host bridge then put in dev->config.
> 
> v6:
> 
> * Drop introducing a new machine specific to IGD passthrough
> * Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
> * Currently IGD drivers always need to access PCH by 1f.0. But we
>   don't want to poke that directly to get ID, and although 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).
> * Drop igd write ops since its fine to emulate that, and we also shrink
>   those igd read ops as necessary.
> * Rebase and cleanup all patches.
> 
> v5:
> 
> * Simplify to make sure its really inherited from the standard one in patch #3
> * Then drop the original patch #3
> 
> v4:
> 
> * Rebase on latest tree
> * Drop patch #2
> * Regenerate patches after Michael introduce patch #1
> * We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
> * Test: boot with a preinstalled winxp
>   ./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c -machine pc
> 
> v3:
> 
> * Drop patch #4
> * Add one patch #1 from Michael
> * Rebase
> * In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc
> 
> v2:
> 
> * Fix some coding style
> * New patch to separate i440fx_init
> * Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
> * Based on patch #2 to regenerate
> * Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
> * Test: boot with a preinstalled ubuntu 14.04
>   ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc
> 
> As we discussed we need to create a separate machine to support current
> IGD passthrough.
> 
> ----------------------------------------------------------------
> Michael S. Tsirkin (1):
>       i440fx: make types configurable at run-time
> 
> Tiejun Chen (9):
>       pc_init1: pass parameters just with types
>       piix: create host bridge to passthrough
>       hw/pci-assign: split pci-assign.c
>       xen, gfx passthrough: basic graphics passthrough support
>       xen, gfx passthrough: retrieve VGA BIOS to work
>       igd gfx passthrough: create a isa bridge
>       xen, gfx passthrough: register a isa bridge
>       xen, gfx passthrough: register host bridge specific to passthrough
>       xen, gfx passthrough: add opregion mapping
> 
>  hw/core/machine.c             |  20 +++
>  hw/i386/Makefile.objs         |   1 +
>  hw/i386/kvm/pci-assign.c      |  82 +---------
>  hw/i386/pc_piix.c             | 139 ++++++++++++++++-
>  hw/i386/pci-assign-load-rom.c |  93 ++++++++++++
>  hw/pci-host/piix.c            |  91 +++++++++++-
>  hw/xen/Makefile.objs          |   1 +
>  hw/xen/xen-host-pci-device.c  |   5 +
>  hw/xen/xen-host-pci-device.h  |   1 +
>  hw/xen/xen_pt.c               |  36 +++++
>  hw/xen/xen_pt.h               |  21 ++-
>  hw/xen/xen_pt_config_init.c   |  51 ++++++-
>  hw/xen/xen_pt_graphics.c      | 272 ++++++++++++++++++++++++++++++++++
>  include/hw/boards.h           |   1 +
>  include/hw/i386/pc.h          |   9 +-
>  include/hw/pci/pci-assign.h   |  27 ++++
>  qemu-options.hx               |   3 +
>  vl.c                          |  10 ++
>  19 files changed, 773 insertions(+), 92 deletions(-)
>  create mode 100644 hw/i386/pci-assign-load-rom.c
>  create mode 100644 hw/xen/xen_pt_graphics.c
>  create mode 100644 include/hw/pci/pci-assign.h
> 
> Thanks
> Tiejun
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough
  2015-07-15 11:46 ` [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Stefano Stabellini
@ 2015-07-15 12:01   ` Michael S. Tsirkin
  2015-07-15 12:38     ` Stefano Stabellini
  0 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2015-07-15 12:01 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: qemu-devel, Tiejun Chen, rth, ehabkost, pbonzini

On Wed, Jul 15, 2015 at 12:46:29PM +0100, Stefano Stabellini wrote:
> Thanks Tiejun, the patch series looks OK to me now.
> 
> It looks like it has all the required acks.
> Michael, are you OK with it? If so, should I add it to my queue, or do
> you want to add it to yours?

I'm OK with the PC bits. Please merge it through your tree.

> On Wed, 15 Jul 2015, Tiejun Chen wrote:
> > v10:
> > 
> > * Don't extern igd_passthrough_isa_bridge_create() in the
> >   include/hw/xen/xen.h file. Instead, move inside the
> >   include/hw/i386/pc.h file in patch #7
> > 
> > v9:
> > 
> > * Rebase on the latest
> > * Inside patch #8, move is_igd_vga_passthrough(dev)) from
> >   xen_igd_passthrough_isa_bridge_create() into xen_pt_initfn().
> > * Inside patch #9, simplify pc_xen_hvm_init_pci()
> > * Michael acked them on pc side
> > * Stefano ackes then on xen side
> > 
> > v8:
> > 
> > * Rebase on the latest qemu tree
> > * Cleanup one xen leftover in patch #3
> > 
> > v7:
> > 
> > * Instead of "-gfx_passthru" we'd like to make that a machine
> >   option, "-machine xxx,igd-passthru=on"" 
> > * try to make something as common shared by others like KvmGT in
> >   the future
> > * Just read those real value from host bridge pci
> >   configuration space when create host bridge then put in dev->config.
> > 
> > v6:
> > 
> > * Drop introducing a new machine specific to IGD passthrough
> > * Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
> > * Currently IGD drivers always need to access PCH by 1f.0. But we
> >   don't want to poke that directly to get ID, and although 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).
> > * Drop igd write ops since its fine to emulate that, and we also shrink
> >   those igd read ops as necessary.
> > * Rebase and cleanup all patches.
> > 
> > v5:
> > 
> > * Simplify to make sure its really inherited from the standard one in patch #3
> > * Then drop the original patch #3
> > 
> > v4:
> > 
> > * Rebase on latest tree
> > * Drop patch #2
> > * Regenerate patches after Michael introduce patch #1
> > * We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
> > * Test: boot with a preinstalled winxp
> >   ./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c -machine pc
> > 
> > v3:
> > 
> > * Drop patch #4
> > * Add one patch #1 from Michael
> > * Rebase
> > * In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc
> > 
> > v2:
> > 
> > * Fix some coding style
> > * New patch to separate i440fx_init
> > * Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
> > * Based on patch #2 to regenerate
> > * Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
> > * Test: boot with a preinstalled ubuntu 14.04
> >   ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc
> > 
> > As we discussed we need to create a separate machine to support current
> > IGD passthrough.
> > 
> > ----------------------------------------------------------------
> > Michael S. Tsirkin (1):
> >       i440fx: make types configurable at run-time
> > 
> > Tiejun Chen (9):
> >       pc_init1: pass parameters just with types
> >       piix: create host bridge to passthrough
> >       hw/pci-assign: split pci-assign.c
> >       xen, gfx passthrough: basic graphics passthrough support
> >       xen, gfx passthrough: retrieve VGA BIOS to work
> >       igd gfx passthrough: create a isa bridge
> >       xen, gfx passthrough: register a isa bridge
> >       xen, gfx passthrough: register host bridge specific to passthrough
> >       xen, gfx passthrough: add opregion mapping
> > 
> >  hw/core/machine.c             |  20 +++
> >  hw/i386/Makefile.objs         |   1 +
> >  hw/i386/kvm/pci-assign.c      |  82 +---------
> >  hw/i386/pc_piix.c             | 139 ++++++++++++++++-
> >  hw/i386/pci-assign-load-rom.c |  93 ++++++++++++
> >  hw/pci-host/piix.c            |  91 +++++++++++-
> >  hw/xen/Makefile.objs          |   1 +
> >  hw/xen/xen-host-pci-device.c  |   5 +
> >  hw/xen/xen-host-pci-device.h  |   1 +
> >  hw/xen/xen_pt.c               |  36 +++++
> >  hw/xen/xen_pt.h               |  21 ++-
> >  hw/xen/xen_pt_config_init.c   |  51 ++++++-
> >  hw/xen/xen_pt_graphics.c      | 272 ++++++++++++++++++++++++++++++++++
> >  include/hw/boards.h           |   1 +
> >  include/hw/i386/pc.h          |   9 +-
> >  include/hw/pci/pci-assign.h   |  27 ++++
> >  qemu-options.hx               |   3 +
> >  vl.c                          |  10 ++
> >  19 files changed, 773 insertions(+), 92 deletions(-)
> >  create mode 100644 hw/i386/pci-assign-load-rom.c
> >  create mode 100644 hw/xen/xen_pt_graphics.c
> >  create mode 100644 include/hw/pci/pci-assign.h
> > 
> > Thanks
> > Tiejun
> > 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough
  2015-07-15 12:01   ` Michael S. Tsirkin
@ 2015-07-15 12:38     ` Stefano Stabellini
  2015-07-15 13:27       ` Chen, Tiejun
  0 siblings, 1 reply; 25+ messages in thread
From: Stefano Stabellini @ 2015-07-15 12:38 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: ehabkost, Stefano Stabellini, qemu-devel, pbonzini, Tiejun Chen, rth

On Wed, 15 Jul 2015, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2015 at 12:46:29PM +0100, Stefano Stabellini wrote:
> > Thanks Tiejun, the patch series looks OK to me now.
> > 
> > It looks like it has all the required acks.
> > Michael, are you OK with it? If so, should I add it to my queue, or do
> > you want to add it to yours?
> 
> I'm OK with the PC bits. Please merge it through your tree.

OK. Added to my queue. I'll send it out after 2.4.


> > On Wed, 15 Jul 2015, Tiejun Chen wrote:
> > > v10:
> > > 
> > > * Don't extern igd_passthrough_isa_bridge_create() in the
> > >   include/hw/xen/xen.h file. Instead, move inside the
> > >   include/hw/i386/pc.h file in patch #7
> > > 
> > > v9:
> > > 
> > > * Rebase on the latest
> > > * Inside patch #8, move is_igd_vga_passthrough(dev)) from
> > >   xen_igd_passthrough_isa_bridge_create() into xen_pt_initfn().
> > > * Inside patch #9, simplify pc_xen_hvm_init_pci()
> > > * Michael acked them on pc side
> > > * Stefano ackes then on xen side
> > > 
> > > v8:
> > > 
> > > * Rebase on the latest qemu tree
> > > * Cleanup one xen leftover in patch #3
> > > 
> > > v7:
> > > 
> > > * Instead of "-gfx_passthru" we'd like to make that a machine
> > >   option, "-machine xxx,igd-passthru=on"" 
> > > * try to make something as common shared by others like KvmGT in
> > >   the future
> > > * Just read those real value from host bridge pci
> > >   configuration space when create host bridge then put in dev->config.
> > > 
> > > v6:
> > > 
> > > * Drop introducing a new machine specific to IGD passthrough
> > > * Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
> > > * Currently IGD drivers always need to access PCH by 1f.0. But we
> > >   don't want to poke that directly to get ID, and although 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).
> > > * Drop igd write ops since its fine to emulate that, and we also shrink
> > >   those igd read ops as necessary.
> > > * Rebase and cleanup all patches.
> > > 
> > > v5:
> > > 
> > > * Simplify to make sure its really inherited from the standard one in patch #3
> > > * Then drop the original patch #3
> > > 
> > > v4:
> > > 
> > > * Rebase on latest tree
> > > * Drop patch #2
> > > * Regenerate patches after Michael introduce patch #1
> > > * We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
> > > * Test: boot with a preinstalled winxp
> > >   ./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c -machine pc
> > > 
> > > v3:
> > > 
> > > * Drop patch #4
> > > * Add one patch #1 from Michael
> > > * Rebase
> > > * In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc
> > > 
> > > v2:
> > > 
> > > * Fix some coding style
> > > * New patch to separate i440fx_init
> > > * Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
> > > * Based on patch #2 to regenerate
> > > * Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
> > > * Test: boot with a preinstalled ubuntu 14.04
> > >   ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc
> > > 
> > > As we discussed we need to create a separate machine to support current
> > > IGD passthrough.
> > > 
> > > ----------------------------------------------------------------
> > > Michael S. Tsirkin (1):
> > >       i440fx: make types configurable at run-time
> > > 
> > > Tiejun Chen (9):
> > >       pc_init1: pass parameters just with types
> > >       piix: create host bridge to passthrough
> > >       hw/pci-assign: split pci-assign.c
> > >       xen, gfx passthrough: basic graphics passthrough support
> > >       xen, gfx passthrough: retrieve VGA BIOS to work
> > >       igd gfx passthrough: create a isa bridge
> > >       xen, gfx passthrough: register a isa bridge
> > >       xen, gfx passthrough: register host bridge specific to passthrough
> > >       xen, gfx passthrough: add opregion mapping
> > > 
> > >  hw/core/machine.c             |  20 +++
> > >  hw/i386/Makefile.objs         |   1 +
> > >  hw/i386/kvm/pci-assign.c      |  82 +---------
> > >  hw/i386/pc_piix.c             | 139 ++++++++++++++++-
> > >  hw/i386/pci-assign-load-rom.c |  93 ++++++++++++
> > >  hw/pci-host/piix.c            |  91 +++++++++++-
> > >  hw/xen/Makefile.objs          |   1 +
> > >  hw/xen/xen-host-pci-device.c  |   5 +
> > >  hw/xen/xen-host-pci-device.h  |   1 +
> > >  hw/xen/xen_pt.c               |  36 +++++
> > >  hw/xen/xen_pt.h               |  21 ++-
> > >  hw/xen/xen_pt_config_init.c   |  51 ++++++-
> > >  hw/xen/xen_pt_graphics.c      | 272 ++++++++++++++++++++++++++++++++++
> > >  include/hw/boards.h           |   1 +
> > >  include/hw/i386/pc.h          |   9 +-
> > >  include/hw/pci/pci-assign.h   |  27 ++++
> > >  qemu-options.hx               |   3 +
> > >  vl.c                          |  10 ++
> > >  19 files changed, 773 insertions(+), 92 deletions(-)
> > >  create mode 100644 hw/i386/pci-assign-load-rom.c
> > >  create mode 100644 hw/xen/xen_pt_graphics.c
> > >  create mode 100644 include/hw/pci/pci-assign.h
> > > 
> > > Thanks
> > > Tiejun
> > > 
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough
  2015-07-15 12:38     ` Stefano Stabellini
@ 2015-07-15 13:27       ` Chen, Tiejun
  0 siblings, 0 replies; 25+ messages in thread
From: Chen, Tiejun @ 2015-07-15 13:27 UTC (permalink / raw)
  To: Stefano Stabellini, Michael S. Tsirkin
  Cc: qemu-devel, pbonzini, ehabkost, rth

On 2015/7/15 20:38, Stefano Stabellini wrote:
> On Wed, 15 Jul 2015, Michael S. Tsirkin wrote:
>> On Wed, Jul 15, 2015 at 12:46:29PM +0100, Stefano Stabellini wrote:
>>> Thanks Tiejun, the patch series looks OK to me now.
>>>
>>> It looks like it has all the required acks.
>>> Michael, are you OK with it? If so, should I add it to my queue, or do
>>> you want to add it to yours?
>>
>> I'm OK with the PC bits. Please merge it through your tree.
>
> OK. Added to my queue. I'll send it out after 2.4.

Really thanks both you guys.

Thanks
Tiejun

>
>
>>> On Wed, 15 Jul 2015, Tiejun Chen wrote:
>>>> v10:
>>>>
>>>> * Don't extern igd_passthrough_isa_bridge_create() in the
>>>>    include/hw/xen/xen.h file. Instead, move inside the
>>>>    include/hw/i386/pc.h file in patch #7
>>>>
>>>> v9:
>>>>
>>>> * Rebase on the latest
>>>> * Inside patch #8, move is_igd_vga_passthrough(dev)) from
>>>>    xen_igd_passthrough_isa_bridge_create() into xen_pt_initfn().
>>>> * Inside patch #9, simplify pc_xen_hvm_init_pci()
>>>> * Michael acked them on pc side
>>>> * Stefano ackes then on xen side
>>>>
>>>> v8:
>>>>
>>>> * Rebase on the latest qemu tree
>>>> * Cleanup one xen leftover in patch #3
>>>>
>>>> v7:
>>>>
>>>> * Instead of "-gfx_passthru" we'd like to make that a machine
>>>>    option, "-machine xxx,igd-passthru=on""
>>>> * try to make something as common shared by others like KvmGT in
>>>>    the future
>>>> * Just read those real value from host bridge pci
>>>>    configuration space when create host bridge then put in dev->config.
>>>>
>>>> v6:
>>>>
>>>> * Drop introducing a new machine specific to IGD passthrough
>>>> * Try to share some codes from KVM stuff in qemu to retrive VGA BIOS
>>>> * Currently IGD drivers always need to access PCH by 1f.0. But we
>>>>    don't want to poke that directly to get ID, and although 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).
>>>> * Drop igd write ops since its fine to emulate that, and we also shrink
>>>>    those igd read ops as necessary.
>>>> * Rebase and cleanup all patches.
>>>>
>>>> v5:
>>>>
>>>> * Simplify to make sure its really inherited from the standard one in patch #3
>>>> * Then drop the original patch #3
>>>>
>>>> v4:
>>>>
>>>> * Rebase on latest tree
>>>> * Drop patch #2
>>>> * Regenerate patches after Michael introduce patch #1
>>>> * We need to use this pci_type as a index to reuse I440FX_PCI_DEVICE()
>>>> * Test: boot with a preinstalled winxp
>>>>    ./i386-softmmu/qemu-system-i386 -hda winxp-32.img -m 2560 -boot c -machine pc
>>>>
>>>> v3:
>>>>
>>>> * Drop patch #4
>>>> * Add one patch #1 from Michael
>>>> * Rebase
>>>> * In./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc
>>>>
>>>> v2:
>>>>
>>>> * Fix some coding style
>>>> * New patch to separate i440fx_init
>>>> * Just add prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough
>>>> * Based on patch #2 to regenerate
>>>> * Unify prefix with XEN_IGD_PASSTHROUGH/xen_igd_passthrough like patch #3
>>>> * Test: boot with a preinstalled ubuntu 14.04
>>>>    ./i386-softmmu/qemu-system-i386 -hda test.img -m 2560 -boot c -machine pc
>>>>
>>>> As we discussed we need to create a separate machine to support current
>>>> IGD passthrough.
>>>>
>>>> ----------------------------------------------------------------
>>>> Michael S. Tsirkin (1):
>>>>        i440fx: make types configurable at run-time
>>>>
>>>> Tiejun Chen (9):
>>>>        pc_init1: pass parameters just with types
>>>>        piix: create host bridge to passthrough
>>>>        hw/pci-assign: split pci-assign.c
>>>>        xen, gfx passthrough: basic graphics passthrough support
>>>>        xen, gfx passthrough: retrieve VGA BIOS to work
>>>>        igd gfx passthrough: create a isa bridge
>>>>        xen, gfx passthrough: register a isa bridge
>>>>        xen, gfx passthrough: register host bridge specific to passthrough
>>>>        xen, gfx passthrough: add opregion mapping
>>>>
>>>>   hw/core/machine.c             |  20 +++
>>>>   hw/i386/Makefile.objs         |   1 +
>>>>   hw/i386/kvm/pci-assign.c      |  82 +---------
>>>>   hw/i386/pc_piix.c             | 139 ++++++++++++++++-
>>>>   hw/i386/pci-assign-load-rom.c |  93 ++++++++++++
>>>>   hw/pci-host/piix.c            |  91 +++++++++++-
>>>>   hw/xen/Makefile.objs          |   1 +
>>>>   hw/xen/xen-host-pci-device.c  |   5 +
>>>>   hw/xen/xen-host-pci-device.h  |   1 +
>>>>   hw/xen/xen_pt.c               |  36 +++++
>>>>   hw/xen/xen_pt.h               |  21 ++-
>>>>   hw/xen/xen_pt_config_init.c   |  51 ++++++-
>>>>   hw/xen/xen_pt_graphics.c      | 272 ++++++++++++++++++++++++++++++++++
>>>>   include/hw/boards.h           |   1 +
>>>>   include/hw/i386/pc.h          |   9 +-
>>>>   include/hw/pci/pci-assign.h   |  27 ++++
>>>>   qemu-options.hx               |   3 +
>>>>   vl.c                          |  10 ++
>>>>   19 files changed, 773 insertions(+), 92 deletions(-)
>>>>   create mode 100644 hw/i386/pci-assign-load-rom.c
>>>>   create mode 100644 hw/xen/xen_pt_graphics.c
>>>>   create mode 100644 include/hw/pci/pci-assign.h
>>>>
>>>> Thanks
>>>> Tiejun
>>>>
>>
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough Tiejun Chen
@ 2016-02-09  3:32   ` Alex Williamson
  2016-02-09 17:43     ` Alex Williamson
  0 siblings, 1 reply; 25+ messages in thread
From: Alex Williamson @ 2016-02-09  3:32 UTC (permalink / raw)
  To: Tiejun Chen; +Cc: ehabkost, mst, stefano.stabellini, qemu-devel, pbonzini, rth

On Wed, 15 Jul 2015 13:37:43 +0800
Tiejun Chen <tiejun.chen@intel.com> wrote:

> Implement a pci host bridge specific to passthrough. Actually
> this just inherits the standard one. And we also just expose
> a minimal real host bridge pci configuration subset.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> v10:
>  
> * Nothing is changed.
> 
> v9:
>  
> * Just rebase on the latest.
> 
>  hw/pci-host/piix.c   | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h |  2 ++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index a203d93..7adf645 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -734,6 +734,87 @@ 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 */
> +};

Hi,

I'm confused by these last 4 registers, could you please help me
understand them?

Offset 0x50 is the GMCH register for SandyBridge and newer processors,
that makes sense, but I suspect we really want to mask out the GMS
field to indicate the size of stolen memory is zero?  Is Xen providing
the VM with direct access to host stolen memory or does it have support
in the VM BIOS for matching the host address?

0x52 is unknown to me, it's listed as reserved for anything since
SandyBridge, does this date back to chipset-based graphics versus the
current processor-based graphics?

The comment on 0xa4 suggests it should be the BDSM regiser, but that's
at offset 0xb0 for everything since SandyBridge.  0xa4 is the second
DWORD of the Top Of Memory (TOM) register.

I'm guessing by the description of 0xa8 that it might be the BGSM
register, but that's at 0xb4.  0xa8 is the first DWORD of the Top Of
Upper Usable DRAM (TOUUD).  Neither TOM or TOUUD seem to make any sense
to expose to the VM, which really makes me wonder if they're actually
used.

I'm looking at volume 2 of the processor datasheets here:

http://www.intel.com/content/www/us/en/processors/core/core-technical-resources.html

Am I maybe looking in the wrong place?  Thanks for your time,

Alex

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-09  3:32   ` Alex Williamson
@ 2016-02-09 17:43     ` Alex Williamson
  2016-02-09 17:56       ` Paolo Bonzini
  2016-02-09 19:47       ` Kay, Allen M
  0 siblings, 2 replies; 25+ messages in thread
From: Alex Williamson @ 2016-02-09 17:43 UTC (permalink / raw)
  Cc: ehabkost, stefano.stabellini, mst, Kay, Allen M, qemu-devel,
	pbonzini, rth


Tiejun's email bounced, so adding Allen for comment.  Host bridge
registers 52h, a4h, and a8h seem to be for versions of IGD prior to
SandyBridge or just wrong.  Even the GMCH (50h) doesn't seem to be
necessary for the VM host bridge, so I'm thinking about dropping all of
these for vfio-based IGD assignment.  Do we know which guest drivers
have specific dependencies on which host bridge and LPC bridge
registers?  It would even be nice to document the standard
registers for future maintainability.  Thanks,

Alex


On Mon, 8 Feb 2016 20:32:35 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Wed, 15 Jul 2015 13:37:43 +0800
> Tiejun Chen <tiejun.chen@intel.com> wrote:
> 
> > Implement a pci host bridge specific to passthrough. Actually
> > this just inherits the standard one. And we also just expose
> > a minimal real host bridge pci configuration subset.
> > 
> > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > v10:
> >  
> > * Nothing is changed.
> > 
> > v9:
> >  
> > * Just rebase on the latest.
> > 
> >  hw/pci-host/piix.c   | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/i386/pc.h |  2 ++
> >  2 files changed, 84 insertions(+)
> > 
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index a203d93..7adf645 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -734,6 +734,87 @@ 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 */
> > +};  
> 
> Hi,
> 
> I'm confused by these last 4 registers, could you please help me
> understand them?
> 
> Offset 0x50 is the GMCH register for SandyBridge and newer processors,
> that makes sense, but I suspect we really want to mask out the GMS
> field to indicate the size of stolen memory is zero?  Is Xen providing
> the VM with direct access to host stolen memory or does it have support
> in the VM BIOS for matching the host address?
> 
> 0x52 is unknown to me, it's listed as reserved for anything since
> SandyBridge, does this date back to chipset-based graphics versus the
> current processor-based graphics?
> 
> The comment on 0xa4 suggests it should be the BDSM regiser, but that's
> at offset 0xb0 for everything since SandyBridge.  0xa4 is the second
> DWORD of the Top Of Memory (TOM) register.
> 
> I'm guessing by the description of 0xa8 that it might be the BGSM
> register, but that's at 0xb4.  0xa8 is the first DWORD of the Top Of
> Upper Usable DRAM (TOUUD).  Neither TOM or TOUUD seem to make any sense
> to expose to the VM, which really makes me wonder if they're actually
> used.
> 
> I'm looking at volume 2 of the processor datasheets here:
> 
> http://www.intel.com/content/www/us/en/processors/core/core-technical-resources.html
> 
> Am I maybe looking in the wrong place?  Thanks for your time,
> 
> Alex
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-09 17:43     ` Alex Williamson
@ 2016-02-09 17:56       ` Paolo Bonzini
  2016-02-09 19:47       ` Kay, Allen M
  1 sibling, 0 replies; 25+ messages in thread
From: Paolo Bonzini @ 2016-02-09 17:56 UTC (permalink / raw)
  To: Alex Williamson
  Cc: ehabkost, stefano.stabellini, mst, Kay, Allen M, qemu-devel, rth



On 09/02/2016 18:43, Alex Williamson wrote:
> Tiejun's email bounced, so adding Allen for comment.  Host bridge
> registers 52h, a4h, and a8h seem to be for versions of IGD prior to
> SandyBridge or just wrong.  Even the GMCH (50h) doesn't seem to be
> necessary for the VM host bridge, so I'm thinking about dropping all of
> these for vfio-based IGD assignment.  Do we know which guest drivers
> have specific dependencies on which host bridge and LPC bridge
> registers?  It would even be nice to document the standard
> registers for future maintainability.  Thanks,

I remember asking Tiejun for the same, but that was a year ago or more. :(

Paolo

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-09 17:43     ` Alex Williamson
  2016-02-09 17:56       ` Paolo Bonzini
@ 2016-02-09 19:47       ` Kay, Allen M
  2016-02-09 21:32         ` Alex Williamson
  1 sibling, 1 reply; 25+ messages in thread
From: Kay, Allen M @ 2016-02-09 19:47 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Ruan, Shuai, ehabkost, stefano.stabellini, mst, qemu-devel,
	pbonzini, rth



> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, February 09, 2016 9:44 AM
> Cc: ehabkost@redhat.com; mst@redhat.com;
> stefano.stabellini@eu.citrix.com; qemu-devel@nongnu.org;
> pbonzini@redhat.com; rth@twiddle.net; Kay, Allen M
> <allen.m.kay@intel.com>
> Subject: Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to
> passthrough
> 
> 
> Tiejun's email bounced, so adding Allen for comment.
>

Tiejun has left Intel.  Shuai Ruan is his replacement (cc'ed).

>  Host bridge registers
> 52h, a4h, and a8h seem to be for versions of IGD prior to SandyBridge or just
> wrong. Even the GMCH (50h) doesn't seem to be necessary for the VM host
> bridge, so I'm thinking about dropping all of these for vfio-based IGD
> assignment.

I think dropping those register access in GMCH should be fine.  52h/a4h/a8h were for Ironlake IGD.  50h is now mirrored in IGD.  They can be removed as BDW/SKL driver no longer access them.

>  Do we know which guest drivers have specific dependencies on
> which host bridge and LPC bridge registers?  It would even be nice to
> document the standard registers for future maintainability.  Thanks,
> 

BDW/SKL drivers no longer need to access those registers in GMCH, especially in Universal Passthrough Mode.  From my perspective,  I'm OK with limiting KVM IGD passthrough to SKL+ platforms so that we don't need to add those hacks for KVM IGD passthrough.

For older HW, there weren't anything documented.  We added those register accesses in Xen/QEMU as we brought up Ironlake/SNB platforms on Xen.

Allen

> 
> On Mon, 8 Feb 2016 20:32:35 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Wed, 15 Jul 2015 13:37:43 +0800
> > Tiejun Chen <tiejun.chen@intel.com> wrote:
> >
> > > Implement a pci host bridge specific to passthrough. Actually this
> > > just inherits the standard one. And we also just expose a minimal
> > > real host bridge pci configuration subset.
> > >
> > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > v10:
> > >
> > > * Nothing is changed.
> > >
> > > v9:
> > >
> > > * Just rebase on the latest.
> > >
> > >  hw/pci-host/piix.c   | 82
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/i386/pc.h |  2 ++
> > >  2 files changed, 84 insertions(+)
> > >
> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index
> > > a203d93..7adf645 100644
> > > --- a/hw/pci-host/piix.c
> > > +++ b/hw/pci-host/piix.c
> > > @@ -734,6 +734,87 @@ 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 */ };
> >
> > Hi,
> >
> > I'm confused by these last 4 registers, could you please help me
> > understand them?
> >
> > Offset 0x50 is the GMCH register for SandyBridge and newer processors,
> > that makes sense, but I suspect we really want to mask out the GMS
> > field to indicate the size of stolen memory is zero?  Is Xen providing
> > the VM with direct access to host stolen memory or does it have support
> > in the VM BIOS for matching the host address?
> >

Xen does provide 1:1 stolen memory mapping in the guest.  However, I do agree with you we should disable stolen memory in the guest by mask out GMS field.  This will avoid complications involving stolen memory/RMRR support.  The only features uses stolen memory are Frame Buffer Compression and PAVP content protection.  PAVP won't work in the guest anyways as it requires access to ME/HECI device.

> > 0x52 is unknown to me, it's listed as reserved for anything since
> > SandyBridge, does this date back to chipset-based graphics versus the
> > current processor-based graphics?
> >

I believe this is same as 0x50 but on old Ironlake platform.  Xen started IGD passthrough support in Ironlake.

> > The comment on 0xa4 suggests it should be the BDSM regiser, but that's
> > at offset 0xb0 for everything since SandyBridge.  0xa4 is the second
> > DWORD of the Top Of Memory (TOM) register.
> >
> > I'm guessing by the description of 0xa8 that it might be the BGSM
> > register, but that's at 0xb4.  0xa8 is the first DWORD of the Top Of
> > Upper Usable DRAM (TOUUD).  Neither TOM or TOUUD seem to make any
> sense
> > to expose to the VM, which really makes me wonder if they're actually
> > used.
> >

TOM (0xa0/0xa4) and TOUUD (0xa8) were used in Ironlake generation of Windows driver.  They are no longer needed in BDW/SKL drivers.

> > I'm looking at volume 2 of the processor datasheets here:
> >
> > http://www.intel.com/content/www/us/en/processors/core/core-
> technical-resources.html
> >
> > Am I maybe looking in the wrong place?  Thanks for your time,
> >
> > Alex
> >

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-09 19:47       ` Kay, Allen M
@ 2016-02-09 21:32         ` Alex Williamson
  2016-02-10  3:40           ` Kay, Allen M
  2016-02-15 11:28           ` Gerd Hoffmann
  0 siblings, 2 replies; 25+ messages in thread
From: Alex Williamson @ 2016-02-09 21:32 UTC (permalink / raw)
  To: Kay, Allen M
  Cc: Ruan, Shuai, ehabkost, stefano.stabellini, mst, qemu-devel,
	pbonzini, rth

On Tue, 9 Feb 2016 19:47:49 +0000
"Kay, Allen M" <allen.m.kay@intel.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Tuesday, February 09, 2016 9:44 AM
> > Cc: ehabkost@redhat.com; mst@redhat.com;
> > stefano.stabellini@eu.citrix.com; qemu-devel@nongnu.org;
> > pbonzini@redhat.com; rth@twiddle.net; Kay, Allen M
> > <allen.m.kay@intel.com>
> > Subject: Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to
> > passthrough
> > 
> > 
> > Tiejun's email bounced, so adding Allen for comment.
> >  
> 
> Tiejun has left Intel.  Shuai Ruan is his replacement (cc'ed).
> 
> >  Host bridge registers
> > 52h, a4h, and a8h seem to be for versions of IGD prior to SandyBridge or just
> > wrong. Even the GMCH (50h) doesn't seem to be necessary for the VM host
> > bridge, so I'm thinking about dropping all of these for vfio-based IGD
> > assignment.  
> 
> I think dropping those register access in GMCH should be fine.  52h/a4h/a8h were for Ironlake IGD.  50h is now mirrored in IGD.  They can be removed as BDW/SKL driver no longer access them.
> 
> >  Do we know which guest drivers have specific dependencies on
> > which host bridge and LPC bridge registers?  It would even be nice to
> > document the standard registers for future maintainability.  Thanks,
> >   
> 
> BDW/SKL drivers no longer need to access those registers in GMCH, especially in Universal Passthrough Mode.  From my perspective,  I'm OK with limiting KVM IGD passthrough to SKL+ platforms so that we don't need to add those hacks for KVM IGD passthrough.
> 
> For older HW, there weren't anything documented.  We added those register accesses in Xen/QEMU as we brought up Ironlake/SNB platforms on Xen.

Hi Allen,

Thanks for the reply.  I'm totally onboard with you for BDW/SKL for
supported platforms, but I'd like to understand what the incremental
investment is for each back step within reason for older GPUs, at least
for best-effort, community support.  If we want to assume BDW/SKL and
Universal Passthrough Mode, then we could abandon the host bridge and
ISA bridge modifications altogether, right?  On the other hand, it's
not clear to me that UPT drivers are that pervasive and if we need to
enable some degree of host bridge/ISA bridge poke thrus, why not enable
a fair chunk of users, including me.

My IVB desktop seems to be working well (win10 + linux) with only
poking through the standard host bridge and ISA bridge
identifications.  Yes, I need to know about the different GMCH layout
of IVB vs BDW in the IGD device, but I've already taken care of that.
It seems like SNB is pretty similar to IVB (they run on the same
chipsets), but I haven't yet figured out the magic to make an SNB
laptop light up with IGD assignment, which would be useful to show that
OpRegion passthrough is actually doing something.

If we ignore IronLake and older GPUs, how many VM chipset hacks do we
need?  What combinations would require GMCH mirrored in the host
bridge, and couldn't we mirror it from the IGD device itself since it's
present in the same location all the way back through SNB.

> > On Mon, 8 Feb 2016 20:32:35 -0700
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >   
> > > On Wed, 15 Jul 2015 13:37:43 +0800
> > > Tiejun Chen <tiejun.chen@intel.com> wrote:
> > > > +/* 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 */ };  
> > >
> > > Hi,
> > >
> > > I'm confused by these last 4 registers, could you please help me
> > > understand them?
> > >
> > > Offset 0x50 is the GMCH register for SandyBridge and newer processors,
> > > that makes sense, but I suspect we really want to mask out the GMS
> > > field to indicate the size of stolen memory is zero?  Is Xen providing
> > > the VM with direct access to host stolen memory or does it have support
> > > in the VM BIOS for matching the host address?
> > >  
> 
> Xen does provide 1:1 stolen memory mapping in the guest.  However, I do agree with you we should disable stolen memory in the guest by mask out GMS field.  This will avoid complications involving stolen memory/RMRR support.  The only features uses stolen memory are Frame Buffer Compression and PAVP content protection.  PAVP won't work in the guest anyways as it requires access to ME/HECI device.

We certainly don't want to get into the nastiness of RMRRs in a VM, but
do the stolen memory use cases you've outlined explain the DMAR faults
to that region that I was seeing just booting a VM with a Linux live
CD?  Again, it seems just as easy, if not easier to copy GMCH from the
IGD itself into the host bridge.
 
> > > 0x52 is unknown to me, it's listed as reserved for anything since
> > > SandyBridge, does this date back to chipset-based graphics versus the
> > > current processor-based graphics?
> > >  
> 
> I believe this is same as 0x50 but on old Ironlake platform.  Xen started IGD passthrough support in Ironlake.

Ok, unless anyone shouts really loudly and it doesn't affect anything
newer than IronLake, I'm happy to let those fall off the plate.  I don't
have any older systems that I care to make work with this.

> > > The comment on 0xa4 suggests it should be the BDSM regiser, but that's
> > > at offset 0xb0 for everything since SandyBridge.  0xa4 is the second
> > > DWORD of the Top Of Memory (TOM) register.
> > >
> > > I'm guessing by the description of 0xa8 that it might be the BGSM
> > > register, but that's at 0xb4.  0xa8 is the first DWORD of the Top Of
> > > Upper Usable DRAM (TOUUD).  Neither TOM or TOUUD seem to make any  
> > sense  
> > > to expose to the VM, which really makes me wonder if they're actually
> > > used.
> > >  
> 
> TOM (0xa0/0xa4) and TOUUD (0xa8) were used in Ironlake generation of Windows driver.  They are no longer needed in BDW/SKL drivers.

Ok, so the code comment is pretty misleading for these.  Would anything
since SNB need these?  What about the BDSM register in the host
bridge?  Is the Xen experience of not needing to copy it relevant since
stolen memory is identity mapped into the VM anyway?  Maybe Xen
achieves the same effect by not copying it and exposing it as zero.
Thanks,

Alex

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-09 21:32         ` Alex Williamson
@ 2016-02-10  3:40           ` Kay, Allen M
  2016-02-10  6:00             ` Alex Williamson
  2016-02-15 11:28           ` Gerd Hoffmann
  1 sibling, 1 reply; 25+ messages in thread
From: Kay, Allen M @ 2016-02-10  3:40 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Ruan, Shuai, ehabkost, stefano.stabellini, mst, qemu-devel,
	pbonzini, rth

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



> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, February 09, 2016 1:33 PM
> To: Kay, Allen M <allen.m.kay@intel.com>
> Cc: ehabkost@redhat.com; mst@redhat.com;
> stefano.stabellini@eu.citrix.com; qemu-devel@nongnu.org;
> pbonzini@redhat.com; rth@twiddle.net; Ruan, Shuai
> <shuai.ruan@intel.com>
> Subject: Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to
> passthrough
> 
> Hi Allen,
> 
Hi Alex,

> Thanks for the reply.  I'm totally onboard with you for BDW/SKL for
> supported platforms, but I'd like to understand what the incremental
> investment is for each back step within reason for older GPUs, at least for
> best-effort, community support.  If we want to assume BDW/SKL and
> Universal Passthrough Mode, then we could abandon the host bridge and
> ISA bridge modifications altogether, right?

Right.

> On the other hand, it's not clear
> to me that UPT drivers are that pervasive and if we need to enable some
> degree of host bridge/ISA bridge poke thrus, why not enable a fair chunk of
> users, including me.
> 

Sounds good.

> My IVB desktop seems to be working well (win10 + linux) with only poking
> through the standard host bridge and ISA bridge identifications.  Yes, I need
> to know about the different GMCH layout of IVB vs BDW in the IGD device,
> but I've already taken care of that.

Other than VendorID/DeviceID, which other PCI config fields in host bridge and ISA bridge fields did you have to passthrough to get your IVB to work.  Are you passing IGD through as primary or secondary?

> It seems like SNB is pretty similar to IVB (they run on the same chipsets),

Agree.

> but I haven't yet fgured out the magic to make an SNB laptop light up with IGD
> assignment, which would be useful to show that OpRegion passthrough is
> actually doing something.
> 

There might be a difference in BDSM definition.  You might want to add 0xb0 in host bridge passthrough and try SNB again.

It is also possible the difference might be caused by difference in driver version.  If I google "sandybridge windows graphics driver" and "ivybridge windows graphics driver" I get the following:

SNB driver: https://downloadcenter.intel.com/product/81502/Intel-HD-Graphics-2000-for-2nd-Generation-Intel-Core-Processors
IVB driver: https://downloadcenter.intel.com/product/81501/Intel-HD-Graphics-2500-for-3rd-Generation-Intel-Core-Processors

SNB driver points to version 15.28 driver binary.  This driver supports SNB and ILK graphics.
IVB driver points to version 15.33 driver binary.  This driver supports IVB and SNB graphics.

One experiment you can try is to install same IVB 15.33 binary on both IVB and SNB systems.  To prevent Windows update messing with driver version, you should do the following:

1)  disable MSFT driver update: Computer->"Advance system settings"->Hardware->"Device Installation Settings"->"No, let me choose what to do"->"Never install driver software from Windows Update".
2) Disable Windows update to never check or download updates.
3) remove "c:\windows\system32\DriverStore\FileRepository\igd*"
4) re-install IGD driver to desired binary.
5) Double check driver versions of both IVB and SNB are the same after reboot guest.  Unfortunately, it won't said 15.33 but have a version number starts with 10.*.

> If we ignore IronLake and older GPUs, how many VM chipset hacks do we
> need?  What combinations would require GMCH mirrored in the host bridge,

Base on original code in Xen, you can try the following host bridge offset passthrough.  I have also attached pt-graphics.c from Xen/QEMU for your reference.

       case 0x00:        /* vendor id */
        case 0x02:        /* device id */
        case 0x08:        /* revision id */
        case 0x2c:        /* sybsystem vendor id */
        case 0x2e:        /* sybsystem id */
        case 0x50:        /* SNB: processor graphics control register */
        case 0x52:        /* processor graphics control register */
        case 0xa0:        /* top of memory */
        case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
        case 0x58:        /* SNB: PAVPC Offset */
        case 0xa4:        /* SNB: graphics base of stolen memory */
        case 0xa8:        /* SNB: base of GTT stolen memory */

> and couldn't we mirror it from the IGD device itself since it's present in the
> same location all the way back through SNB.
> 

Yes, you can copy content from MCCG in IGD device to same 0x50 offset in host bridge.

> We certainly don't want to get into the nastiness of RMRRs in a VM, but do
> the stolen memory use cases you've outlined explain the DMAR faults to that
> region that I was seeing just booting a VM with a Linux live CD?

What I described about stolen memory was based on Windows driver usage.  I will need to try Linux live CD to see DMAR faults you encountered.  I will try that after I finish setting up my environment.  I have been following your blog.  Great instructions!

> Again, it seems just as easy, if not easier to copy GMCH from the IGD itself into the
> host bridge.
> 
Not sure if I follow ... how does this would solve stolen memory/RMRR issue...?

> 
> Ok, unless anyone shouts really loudly and it doesn't affect anything newer
> than IronLake, I'm happy to let those fall off the plate.  I don't have any older
> systems that I care to make work with this.
> 
Sounds good.

> 
> Ok, so the code comment is pretty misleading for these.  Would anything
> since SNB need these?
>
We did not do much pruning pre-SNB.  Tiejun and I did checked HSW last year, most (maybe all) of these register are not needed.

> What about the BDSM register in the host bridge?
>  Is the Xen experience of not needing to copy it relevant since stolen memory is
> identity mapped into the VM anyway?  Maybe Xen achieves the same effect
> by not copying it and exposing it as zero.

In SNB, BDSM is at offset 0xb0 in host bridge.  Xen/QEMU passes thru 0xb0 in host bridge.  Although the comment says it is for ILK.  It is also true for SNB.  In current BDW/SKL driver, BDSM is located at 0x5c in IGD device.

Allen

[-- Attachment #2: pt-graphics.c --]
[-- Type: text/plain, Size: 8442 bytes --]

/*
 * graphics passthrough
 */

#include "pass-through.h"
#include "pci.h"
#include "pci/header.h"
#include "pci/pci.h"

#include <unistd.h>
#include <sys/ioctl.h>
#include <assert.h>

extern int gfx_passthru;
extern int igd_passthru;

static uint32_t igd_guest_opregion = 0;

static int pch_map_irq(PCIDevice *pci_dev, int irq_num)
{
    PT_LOG("pch_map_irq called\n");
    return irq_num;
}

void intel_pch_init(PCIBus *bus)
{
    uint16_t vid, did;
    uint8_t  rid;
    struct pci_dev *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);

    if (vid == PCI_VENDOR_ID_INTEL) {
        pci_isa_bridge_init(bus, PCI_DEVFN(0x1f, 0), vid, did, rid,
                            pch_map_irq, "intel_bridge_1f");

    }
}

uint32_t igd_read_opregion(struct pt_dev *pci_dev)
{
    uint32_t val = -1;

    if ( igd_guest_opregion == 0 )
        return -1;

    val = igd_guest_opregion;
#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
    PT_LOG_DEV((PCIDevice*)pci_dev, "addr=%x len=%x val=%x\n",
            PCI_INTEL_OPREGION, 4, val);
#endif
    return val;
}

void igd_write_opregion(struct pt_dev *real_dev, uint32_t val)
{
    uint32_t host_opregion = 0;
    int ret;

    if ( igd_guest_opregion )
    {
        PT_LOG("opregion register already been set, ignoring %x\n", val);
        return;
    }

    host_opregion = pt_pci_host_read(real_dev->pci_dev, PCI_INTEL_OPREGION, 4);
    igd_guest_opregion = (val & ~0xfff) | (host_opregion & 0xfff);
    PT_LOG("Map OpRegion: %x -> %x\n", host_opregion, igd_guest_opregion);

    ret = xc_domain_memory_mapping(xc_handle, domid,
            igd_guest_opregion >> XC_PAGE_SHIFT,
            host_opregion >> XC_PAGE_SHIFT,
            2,
            DPCI_ADD_MAPPING);

    if ( ret != 0 )
    {
        PT_LOG("Error: Can't map opregion\n");
        igd_guest_opregion = 0;
    }
#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
    PT_LOG_DEV((PCIDevice*)real_dev, "addr=%x len=%lx val=%x\n",
            PCI_INTEL_OPREGION, len, val);
#endif

}

void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t val, int len)
{
    struct pci_dev *pci_dev_host_bridge;
    assert(pci_dev->devfn == 0x00);
    if ( !igd_passthru )
        goto write_default;

    switch (config_addr)
    {
        case 0x58:        // PAVPC Offset
            break;
        default:
            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;
    uint32_t val;

    assert(pci_dev->devfn == 0x00);
    if ( !igd_passthru )
        goto read_default;

    switch (config_addr)
    {
        case 0x00:        /* vendor id */
        case 0x02:        /* device id */
        case 0x08:        /* revision id */
        case 0x2c:        /* sybsystem vendor id */
        case 0x2e:        /* sybsystem id */
        case 0x50:        /* SNB: processor graphics control register */
        case 0x52:        /* processor graphics control register */
        case 0xa0:        /* top of memory */
        case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
        case 0x58:        /* SNB: PAVPC Offset */
        case 0xa4:        /* SNB: graphics base of stolen memory */
        case 0xa8:        /* SNB: base of GTT stolen memory */
            break;
        default:
            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);
}

/*
 * register VGA resources for the domain with assigned gfx
 */
int register_vga_regions(struct pt_dev *real_device)
{
    u16 vendor_id;
    int ret = 0;

    if ( !gfx_passthru || real_device->pci_dev->device_class != 0x0300 )
        return ret;

    ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0,
            0x3B0, 0xC, DPCI_ADD_MAPPING);

    ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3C0,
            0x3C0, 0x20, DPCI_ADD_MAPPING);

    ret |= xc_domain_memory_mapping(xc_handle, domid,
            0xa0000 >> XC_PAGE_SHIFT,
            0xa0000 >> XC_PAGE_SHIFT,
            0x20,
            DPCI_ADD_MAPPING);

    if ( ret != 0 )
        PT_LOG("VGA region mapping failed\n");

    return ret;
}

/*
 * unregister VGA resources for the domain with assigned gfx
 */
int unregister_vga_regions(struct pt_dev *real_device)
{
    u32 vendor_id;
    int ret = 0;

    if ( !gfx_passthru || real_device->pci_dev->device_class != 0x0300 )
        return ret;

    ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0,
            0x3B0, 0xC, DPCI_REMOVE_MAPPING);

    ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3C0,
            0x3C0, 0x20, DPCI_REMOVE_MAPPING);

    ret |= xc_domain_memory_mapping(xc_handle, domid,
            0xa0000 >> XC_PAGE_SHIFT,
            0xa0000 >> XC_PAGE_SHIFT,
            20,
            DPCI_REMOVE_MAPPING);

    vendor_id = pt_pci_host_read(real_device->pci_dev, PCI_VENDOR_ID, 2);
    if ( (vendor_id == PCI_VENDOR_ID_INTEL) && igd_guest_opregion )
    {
        ret |= xc_domain_memory_mapping(xc_handle, domid,
                igd_guest_opregion >> XC_PAGE_SHIFT,
                igd_guest_opregion >> XC_PAGE_SHIFT,
                2,
                DPCI_REMOVE_MAPPING);
    }

    if ( ret != 0 )
        PT_LOG("VGA region unmapping failed\n");

    return ret;
}

static int get_vgabios(unsigned char *buf)
{
    int fd;
    uint32_t bios_size = 0;
    uint32_t start = 0xC0000;
    uint16_t magic = 0;

    if ( (fd = open("/dev/mem", O_RDONLY)) < 0 )
    {
        PT_LOG("Error: Can't open /dev/mem: %s\n", strerror(errno));
        return 0;
    }

    /*
     * Check if it a real bios extension.
     * The magic number is 0xAA55.
     */
    if ( start != lseek(fd, start, SEEK_SET) )
        goto out;
    if ( read(fd, &magic, 2) != 2 )
        goto out;
    if ( magic != 0xAA55 )
        goto out;

    /* Find the size of the rom extension */
    if ( start != lseek(fd, start, SEEK_SET) )
        goto out;
    if ( lseek(fd, 2, SEEK_CUR) != (start + 2) )
        goto out;
    if ( read(fd, &bios_size, 1) != 1 )
        goto out;

    /* This size is in 512 bytes */
    bios_size *= 512;

    /*
     * Set the file to the begining of the rombios,
     * to start the copy.
     */
    if ( start != lseek(fd, start, SEEK_SET) )
        goto out;

    if ( bios_size != read(fd, buf, bios_size))
        bios_size = 0;

out:
    close(fd);
    return bios_size;
}

int setup_vga_pt(struct pt_dev *real_device)
{
    unsigned char *bios = NULL;
    int bios_size = 0;
    char *c = NULL;
    char checksum = 0;
    int rc = 0;

    if ( !gfx_passthru || real_device->pci_dev->device_class != 0x0300 )
        return rc;

    /* Allocated 64K for the vga bios */
    if ( !(bios = malloc(64 * 1024)) )
        return -1;

    bios_size = get_vgabios(bios);
    if ( bios_size == 0 || bios_size > 64 * 1024)
    {
        PT_LOG("vga bios size (0x%x) is invalid!\n", bios_size);
        rc = -1;
        goto out;
    }

    /* Adjust the bios checksum */
    for ( c = (char*)bios; c < ((char*)bios + bios_size); c++ )
        checksum += *c;
    if ( checksum )
    {
        bios[bios_size - 1] -= checksum;
        PT_LOG("vga bios checksum is adjusted!\n");
    }

    cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
out:
    free(bios);
    return rc;
}

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-10  3:40           ` Kay, Allen M
@ 2016-02-10  6:00             ` Alex Williamson
  2016-02-11  2:25               ` Kay, Allen M
  0 siblings, 1 reply; 25+ messages in thread
From: Alex Williamson @ 2016-02-10  6:00 UTC (permalink / raw)
  To: Kay, Allen M
  Cc: Ruan, Shuai, ehabkost, stefano.stabellini, mst, qemu-devel,
	pbonzini, rth

Hi Allen,

On Wed, 10 Feb 2016 03:40:54 +0000
"Kay, Allen M" <allen.m.kay@intel.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Tuesday, February 09, 2016 1:33 PM
> > To: Kay, Allen M <allen.m.kay@intel.com>
> > Cc: ehabkost@redhat.com; mst@redhat.com;
> > stefano.stabellini@eu.citrix.com; qemu-devel@nongnu.org;
> > pbonzini@redhat.com; rth@twiddle.net; Ruan, Shuai
> > <shuai.ruan@intel.com>
> > Subject: Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to
> > passthrough
> > 
> > Hi Allen,
> >   
> Hi Alex,
> 
> > Thanks for the reply.  I'm totally onboard with you for BDW/SKL for
> > supported platforms, but I'd like to understand what the incremental
> > investment is for each back step within reason for older GPUs, at least for
> > best-effort, community support.  If we want to assume BDW/SKL and
> > Universal Passthrough Mode, then we could abandon the host bridge and
> > ISA bridge modifications altogether, right?  
> 
> Right.
> 
> > On the other hand, it's not clear
> > to me that UPT drivers are that pervasive and if we need to enable some
> > degree of host bridge/ISA bridge poke thrus, why not enable a fair chunk of
> > users, including me.
> >   
> 
> Sounds good.
> 
> > My IVB desktop seems to be working well (win10 + linux) with only poking
> > through the standard host bridge and ISA bridge identifications.  Yes, I need
> > to know about the different GMCH layout of IVB vs BDW in the IGD device,
> > but I've already taken care of that.  
> 
> Other than VendorID/DeviceID, which other PCI config fields in host bridge and ISA bridge fields did you have to passthrough to get your IVB to work.  Are you passing IGD through as primary or secondary?

The host bridge I stripped down to:

    {PCI_REVISION_ID,         2},
    {PCI_SUBSYSTEM_VENDOR_ID, 2},
    {PCI_SUBSYSTEM_ID,        2},

And used this for the ISA bridge:

    {PCI_VENDOR_ID,           2},
    {PCI_DEVICE_ID,           2},
    {PCI_REVISION_ID,         2},
    {PCI_SUBSYSTEM_VENDOR_ID, 2},
    {PCI_SUBSYSTEM_ID,        2},

So nothing to do with GMCH/BDSM seems to be necessary for the IVB
desktop system. I didn't try to trim further.

I can run that system as either primary or secondary.  On both systems
I'm using pci-stub to keep i915 from interfering on the host, on the
SNB laptop I also use video=efifb:off since it's configured for UEFI
boot, I'm not sure if that play any role in why it's not working.

I've tried adding a bunch more registers on the SNB system to see if it
helps, for the host bridge:

    {0x50,                    2},
    {0x52,                    2},
    {0xa4,                    4},
    {0xa8,                    4},

(ie. the registers Tiejun added with this patch)

And on the ISA bridge:

    {0x40,        4},
    {0x44,        4},
    {0x48,        4},
    {0x4c,        4},
    {0xf0,        4},

These were registers I saw accessed with tracing enabled, but nothing
seemed to change by adding them. I don't see any accesses to the BDSM
register on the host bridge, but I do see GMCH accesses.  Sort of
interesting is that the guest reads 201h from that register and tries
to write 203h. In the read case we have a 2MB GGMS and GGCLCK set, the
guest tries to set IVD (IGD VGA Disable). I'm not sure why it bothers
to try to do this with GGCLCK indicated since the register is locked,
but it is slightly troubling that the spec indicates that the BIOS must
*not* set IVD to zero (VGA enabled) if GMS pre-allocates no memory,
which is exactly what we're doing to try to indicate no stolen memory.
If we could actually disable VGA on IGD, that might be a nice option,
but I know there are issues with that (iirc, there's no disable for one
of either the VGA MMIO or I/O port space, so the only option is to
disable that address space at the PCI command register, which has
obvious implications - I think it was probably I/O port space).

I'm currently trying a Linux live CD on the guest for the SNB, mostly
because I can see driver error messages and look at the driver source,
neither of which are available to me for the Windows driver.  Both of
the driver warnings I get are from the driver thinking a certain
feature should be disabled but finds it enabled.  They're also sort of
on a strange drm release path, maybe due to being in secondary mode and
Xorg not being configured to use the IGD on this image.  In case it
looks familiar:

[   12.397908] ------------[ cut here ]------------
[   12.399175] WARNING: CPU: 0 PID: 1135 at drivers/gpu/drm/i915/intel_display.c:1259 assert_fdi_rx_pll+0x78/0xb0 [i915]()
[   12.401720] FDI RX PLL assertion failure (expected off, current on)
[   12.403239] Modules linked in: ebtable_broute bridge stp llc ebtable_filter ebtable_nat ebtables ip6table_security ip6table_mangle ip6table_raw ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_filter ip6_tables iptable_security iptable_mangle iptable_raw iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iTCO_wdt gpio_ich iTCO_vendor_support ppdev joydev parport_pc lpc_ich i2c_piix4 parport acpi_cpufreq nfsd auth_rpcgss nfs_acl lockd grace isofs squashfs i915 bochs_drm video ttm i2c_algo_bit drm_kms_helper drm ata_generic serio_raw pata_acpi scsi_dh_rdac scsi_dh_emc scsi_dh_alua sunrpc loop
[   12.417994] CPU: 0 PID: 1135 Comm: Xorg Not tainted 4.2.3-300.fc23.x86_64 #1
[   12.419733] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.0-80-g2390eb5 04/01/2014
[   12.422100]  0000000000000000 00000000d92c128c ffff880078f53ab8 ffffffff81771fca
[   12.424023]  0000000000000000 ffff880078f53b10 ffff880078f53af8 ffffffff8109e4a6
[   12.425949]  ffff880078f53ad8 0000000000000000 ffff88007f680000 ffff88007fb72000
[   12.427719] Call Trace:
[   12.428354]  [<ffffffff81771fca>] dump_stack+0x45/0x57
[   12.429580]  [<ffffffff8109e4a6>] warn_slowpath_common+0x86/0xc0
[   12.431037]  [<ffffffff8109e535>] warn_slowpath_fmt+0x55/0x70
[   12.432497]  [<ffffffffa01e5a98>] assert_fdi_rx_pll+0x78/0xb0 [i915]
[   12.434033]  [<ffffffffa0221433>] intel_pre_enable_lvds+0x53/0x180 [i915]
[   12.435760]  [<ffffffffa01ef729>] ironlake_crtc_enable+0x199/0xd30 [i915]
[   12.437433]  [<ffffffffa0190c80>] ? intel_runtime_pm_put+0x50/0x60 [i915]
[   12.439083]  [<ffffffffa0190d98>] ? intel_display_power_put+0x108/0x160 [i915]
[   12.440914]  [<ffffffffa01ecff6>] __intel_set_mode+0x916/0xb60 [i915]
[   12.442458]  [<ffffffffa01f3ee6>] intel_crtc_set_config+0x2b6/0x580 [i915]
[   12.444180]  [<ffffffffa0099326>] drm_mode_set_config_internal+0x66/0x100 [drm]
[   12.445917]  [<ffffffffa016d512>] restore_fbdev_mode+0xc2/0xf0 [drm_kms_helper]
[   12.447695]  [<ffffffffa016f399>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x70 [drm_kms_helper]
[   12.450116]  [<ffffffffa020350e>] intel_fbdev_restore_mode+0x1e/0x50 [i915]
[   12.451794]  [<ffffffffa022c96e>] i915_driver_lastclose+0xe/0x20 [i915]
[   12.453360]  [<ffffffffa008c8ee>] drm_lastclose+0x2e/0x140 [drm]
[   12.454755]  [<ffffffffa008ccaf>] drm_release+0x2af/0x490 [drm]
[   12.456324]  [<ffffffff8121fbfc>] __fput+0xdc/0x1e0
[   12.457525]  [<ffffffff8121fd4e>] ____fput+0xe/0x10
[   12.458736]  [<ffffffff810bab65>] task_work_run+0x85/0xb0
[   12.460028]  [<ffffffff81014a2d>] do_notify_resume+0x8d/0x90
[   12.461424]  [<ffffffff81778b7c>] int_signal+0x12/0x17
[   12.462757] ---[ end trace 3278f15671a6fa94 ]---
[   12.546650] ------------[ cut here ]------------
[   12.547765] WARNING: CPU: 0 PID: 1135 at drivers/gpu/drm/i915/intel_display.c:1466 assert_pch_transcoder_disabled+0x67/0x70 [i915]()
[   12.550847] transcoder assertion failed, should be off on pipe A but is still active
[   12.552858] Modules linked in: ebtable_broute bridge stp llc ebtable_filter ebtable_nat ebtables ip6table_security ip6table_mangle ip6table_raw ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_filter ip6_tables iptable_security iptable_mangle iptable_raw iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iTCO_wdt gpio_ich iTCO_vendor_support ppdev joydev parport_pc lpc_ich i2c_piix4 parport acpi_cpufreq nfsd auth_rpcgss nfs_acl lockd grace isofs squashfs i915 bochs_drm video ttm i2c_algo_bit drm_kms_helper drm ata_generic serio_raw pata_acpi scsi_dh_rdac scsi_dh_emc scsi_dh_alua sunrpc loop
[   12.568444] CPU: 0 PID: 1135 Comm: Xorg Tainted: G        W       4.2.3-300.fc23.x86_64 #1
[   12.570599] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.0-80-g2390eb5 04/01/2014
[   12.572988]  0000000000000000 00000000d92c128c ffff880078f53b08 ffffffff81771fca
[   12.574973]  0000000000000000 ffff880078f53b60 ffff880078f53b48 ffffffff8109e4a6
[   12.576826]  ffff880078f53b28 0000000000000000 ffff88007f680000 ffff88007fb72000
[   12.578804] Call Trace:
[   12.579454]  [<ffffffff81771fca>] dump_stack+0x45/0x57
[   12.580775]  [<ffffffff8109e4a6>] warn_slowpath_common+0x86/0xc0
[   12.582419]  [<ffffffff8109e535>] warn_slowpath_fmt+0x55/0x70
[   12.583924]  [<ffffffffa01e1067>] assert_pch_transcoder_disabled+0x67/0x70 [i915]
[   12.585834]  [<ffffffffa01efb7c>] ironlake_crtc_enable+0x5ec/0xd30 [i915]
[   12.587634]  [<ffffffffa0190c80>] ? intel_runtime_pm_put+0x50/0x60 [i915]
[   12.589402]  [<ffffffffa0190d98>] ? intel_display_power_put+0x108/0x160 [i915]
[   12.591294]  [<ffffffffa01ecff6>] __intel_set_mode+0x916/0xb60 [i915]
[   12.593019]  [<ffffffffa01f3ee6>] intel_crtc_set_config+0x2b6/0x580 [i915]
[   12.594797]  [<ffffffffa0099326>] drm_mode_set_config_internal+0x66/0x100 [drm]
[   12.596651]  [<ffffffffa016d512>] restore_fbdev_mode+0xc2/0xf0 [drm_kms_helper]
[   12.598574]  [<ffffffffa016f399>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x70 [drm_kms_helper]
[   12.601022]  [<ffffffffa020350e>] intel_fbdev_restore_mode+0x1e/0x50 [i915]
[   12.602888]  [<ffffffffa022c96e>] i915_driver_lastclose+0xe/0x20 [i915]
[   12.604576]  [<ffffffffa008c8ee>] drm_lastclose+0x2e/0x140 [drm]
[   12.606098]  [<ffffffffa008ccaf>] drm_release+0x2af/0x490 [drm]
[   12.607662]  [<ffffffff8121fbfc>] __fput+0xdc/0x1e0
[   12.609019]  [<ffffffff8121fd4e>] ____fput+0xe/0x10
[   12.610283]  [<ffffffff810bab65>] task_work_run+0x85/0xb0
[   12.611718]  [<ffffffff81014a2d>] do_notify_resume+0x8d/0x90
[   12.613225]  [<ffffffff81778b7c>] int_signal+0x12/0x17
[   12.614600] ---[ end trace 3278f15671a6fa95 ]---
[   12.635700] [drm:intel_set_pch_fifo_underrun_reporting [i915]] *ERROR* uncleared pch fifo underrun on pch transcoder A
[   12.636007] [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder A FIFO underrun
[   12.642883] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO underrun

Otherwise i915 seems fairly happy:

[    4.940225] i915: No ACPI video bus found
[    4.943223] [drm] Initialized i915 1.6.0 20150522 for 0000:00:03.0 on minor 1
[    5.000053] [drm] GMBUS [i915 gmbus vga] timed out, falling back to bit banging on pin 2
[    5.038055] i915 0000:00:03.0: fb1: inteldrmfb frame buffer device

The bit banging messages is apparently normal on this system (Lenovo
W520, Nvidia disabled in BIOS - could optimus still be getting in the
way?). I don't know that any of the above is actually fatal though, I
just know that I don't get a signal out of the LVDS panel or any of the
other outputs. However, the Xorg log file looks normal and if I disable
mmap support in vfio and move the mouse cursor around on the VNC window,
the trace log looks a lot like graphics is running ok, the panel is just
turned off. I'm thinking that makes the above warnings more relevant
since they're in the space of FDI and PCH transcoders, which I think is
how we get to a display output.

> > It seems like SNB is pretty similar to IVB (they run on the same chipsets),  
> 
> Agree.
> 
> > but I haven't yet fgured out the magic to make an SNB laptop light up with IGD
> > assignment, which would be useful to show that OpRegion passthrough is
> > actually doing something.
> >   
> 
> There might be a difference in BDSM definition.  You might want to add 0xb0 in host bridge passthrough and try SNB again.

I'm not seeing any accesses there in the tracing, so I don't think the
driver cares about it.

> It is also possible the difference might be caused by difference in driver version.  If I google "sandybridge windows graphics driver" and "ivybridge windows graphics driver" I get the following:
> 
> SNB driver: https://downloadcenter.intel.com/product/81502/Intel-HD-Graphics-2000-for-2nd-Generation-Intel-Core-Processors
> IVB driver: https://downloadcenter.intel.com/product/81501/Intel-HD-Graphics-2500-for-3rd-Generation-Intel-Core-Processors
> 
> SNB driver points to version 15.28 driver binary.  This driver supports SNB and ILK graphics.
> IVB driver points to version 15.33 driver binary.  This driver supports IVB and SNB graphics.
> 
> One experiment you can try is to install same IVB 15.33 binary on both IVB and SNB systems.  To prevent Windows update messing with driver version, you should do the following:
> 
> 1)  disable MSFT driver update: Computer->"Advance system settings"->Hardware->"Device Installation Settings"->"No, let me choose what to do"->"Never install driver software from Windows Update".
> 2) Disable Windows update to never check or download updates.
> 3) remove "c:\windows\system32\DriverStore\FileRepository\igd*"
> 4) re-install IGD driver to desired binary.
> 5) Double check driver versions of both IVB and SNB are the same after reboot guest.  Unfortunately, it won't said 15.33 but have a version number starts with 10.*.

There are certainly a lot of subtle differences between IVB and SNB in
the Linux driver, if I have time I'll see about setting up a Windows VM
on the SNB system.  Linux feels pretty close though if I could just get
the panel turned on.

> > If we ignore IronLake and older GPUs, how many VM chipset hacks do we
> > need?  What combinations would require GMCH mirrored in the host bridge,  
> 
> Base on original code in Xen, you can try the following host bridge offset passthrough.  I have also attached pt-graphics.c from Xen/QEMU for your reference.
> 
>        case 0x00:        /* vendor id */
>         case 0x02:        /* device id */
>         case 0x08:        /* revision id */
>         case 0x2c:        /* sybsystem vendor id */
>         case 0x2e:        /* sybsystem id */
>         case 0x50:        /* SNB: processor graphics control register */
>         case 0x52:        /* processor graphics control register */
>         case 0xa0:        /* top of memory */
>         case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
>         case 0x58:        /* SNB: PAVPC Offset */
>         case 0xa4:        /* SNB: graphics base of stolen memory */
>         case 0xa8:        /* SNB: base of GTT stolen memory */
> 
> > and couldn't we mirror it from the IGD device itself since it's present in the
> > same location all the way back through SNB.
> >   
> 
> Yes, you can copy content from MCCG in IGD device to same 0x50 offset in host bridge.
> 
> > We certainly don't want to get into the nastiness of RMRRs in a VM, but do
> > the stolen memory use cases you've outlined explain the DMAR faults to that
> > region that I was seeing just booting a VM with a Linux live CD?  
> 
> What I described about stolen memory was based on Windows driver usage.  I will need to try Linux live CD to see DMAR faults you encountered.  I will try that after I finish setting up my environment.  I have been following your blog.  Great instructions!

Good, glad you found it.  I'm just running a simple VM like this:

/home/alwillia/local/bin/qemu-system-x86_64 -m 2G -bios /home/alwillia/Work/seabios.git/out/bios.bin -net none -cdrom /net/store/exports/ISOs/Fedora-Live-Cinnamon-x86_64-23-10.iso -boot d -monitor stdio -device vfio-pci,host=00:02.0,rombar=0,x-no-mmap=on -vnc :1 -vga std -enable-kvm -trace events=events.txt

events.txt contains:
vfio_region_read
vfio_region_write
vfio_pci_read_config
vfio_pci_write_config
vfio_pci_igd*
pci_cfg_*

If you base on the last patch series I posted on top of Gerd's changes,
you'll need -M pc,igd_passthru=on I believe too, the code I'm working
on passes the host and ISA bridge config through vfio and initiates the
ISA bridge automatically.  I'll try to post it this week.

> > Again, it seems just as easy, if not easier to copy GMCH from the IGD itself into the
> > host bridge.
> >   
> Not sure if I follow ... how does this would solve stolen memory/RMRR issue...?

In the vfio code I have now, the kernel virtualizes GMCH and BDSM on the
device to provide a pre-sanitized version, so if we need those on the
host bridge, it's easier to copy what's on the IGD instead of adding yet
more code that knows about the different GMCH layouts.  The solution is
the same, clear BDSM and GMCH.GMS to make the guest not try to access
the host stolen memory.

> > Ok, unless anyone shouts really loudly and it doesn't affect anything newer
> > than IronLake, I'm happy to let those fall off the plate.  I don't have any older
> > systems that I care to make work with this.
> >   
> Sounds good.
> 
> > 
> > Ok, so the code comment is pretty misleading for these.  Would anything
> > since SNB need these?
> >  
> We did not do much pruning pre-SNB.  Tiejun and I did checked HSW last year, most (maybe all) of these register are not needed.
> 
> > What about the BDSM register in the host bridge?
> >  Is the Xen experience of not needing to copy it relevant since stolen memory is
> > identity mapped into the VM anyway?  Maybe Xen achieves the same effect
> > by not copying it and exposing it as zero.  
> 
> In SNB, BDSM is at offset 0xb0 in host bridge.  Xen/QEMU passes thru 0xb0 in host bridge.  Although the comment says it is for ILK.  It is also true for SNB.  In current BDW/SKL driver, BDSM is located at 0x5c in IGD device.

If I could spot any accesses to BDSM on the host bridge, I'd jump on
this, but my guest driver doesn't seem to care about it according to
the tracing.  Lighting up an output seems to be the issue.  Thanks,

Alex

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-10  6:00             ` Alex Williamson
@ 2016-02-11  2:25               ` Kay, Allen M
  2016-02-11  6:07                 ` Alex Williamson
  0 siblings, 1 reply; 25+ messages in thread
From: Kay, Allen M @ 2016-02-11  2:25 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Ruan, Shuai, ehabkost, stefano.stabellini, mst, qemu-devel,
	pbonzini, rth


> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, February 09, 2016 10:01 PM
> To: Kay, Allen M <allen.m.kay@intel.com>
> Cc: ehabkost@redhat.com; mst@redhat.com;
> stefano.stabellini@eu.citrix.com; qemu-devel@nongnu.org;
> pbonzini@redhat.com; rth@twiddle.net; Ruan, Shuai
> <shuai.ruan@intel.com>
> Subject: Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to
> passthrough
> 
> I can run that system as either primary or secondary.  On both systems I'm
> using pci-stub to keep i915 from interfering on the host, on the SNB laptop I
> also use video=efifb:off since it's configured for UEFI boot, I'm not sure if
> that play any role in why it's not working.
> 

Hi Alex,

My understand if that your IVB system is a desktop running legacy mode BIOS and your SNB is a laptop running EFI mode BIOS, correct?  I'm curious how does your SNB system getting hold of VBT table, which is needed for lighting up local display.

There are two ways the driver can get VBT table:  1) VGA 0xc0000 memory,  2) OpRegion

On your IVB system, the guest driver would try to get VBT from 0xc0000 memory if IGD is configured as primary controller in the guest, assuming KVM/QEMU copies VGA 0xc0000 memory from host to guest 0xc0000 area.   If IGD is configured as secondary controller, the driver would get VBT from OpRegion.  Given IGD works in both configurations on IVB, this mean guest driver can successfully read VBT from both 0xc0000 area and OpRegion.

On your SNB system that is running in EFI mode on the host, 0xc0000 memory does not contain VBIOS/VBT.  This means if you configure IGD as primary controller in the guest, the driver cannot get to VBT at 0xc0000 memory even if KVM/QEMU copies 0xc0000 memory from host to guest.  If you configure IGD as secondary controller in the guest, and guest driver should be able to read VBT from the OpRegion.  You might want to trace i915 to see how does the  guest driver get to VBT via OpRegion in this configuration.

Another potential problem to watch out for with laptop vs. desktop has to do FLR timeout.  If you are working with a laptop with LCD panel attached (the usual case), FLR will take much longer than 100ms to finish.  The reason is the devices needs to power down the LCD panel before it can finish FLR. I have seen it takes more than 500ms for FLR to finish.  As a work around, I have increase FLR time out in Linux PCI driver to 1000ms when working with LCD panels.   Given that you have seen evidences that IGD HW is working, this might not be your issue.   I would focus tracing how does i915 get VBT from the OpRegion when IGD is configured as secondary controller in the guest.

Allen


> I've tried adding a bunch more registers on the SNB system to see if it helps,
> for the host bridge:
> 
>     {0x50,                    2},
>     {0x52,                    2},
>     {0xa4,                    4},
>     {0xa8,                    4},
> 
> (ie. the registers Tiejun added with this patch)
> 
> And on the ISA bridge:
> 
>     {0x40,        4},
>     {0x44,        4},
>     {0x48,        4},
>     {0x4c,        4},
>     {0xf0,        4},
> 
> These were registers I saw accessed with tracing enabled, but nothing
> seemed to change by adding them. I don't see any accesses to the BDSM
> register on the host bridge, but I do see GMCH accesses.  Sort of interesting
> is that the guest reads 201h from that register and tries to write 203h. In the
> read case we have a 2MB GGMS and GGCLCK set, the guest tries to set IVD
> (IGD VGA Disable). I'm not sure why it bothers to try to do this with GGCLCK
> indicated since the register is locked, but it is slightly troubling that the spec
> indicates that the BIOS must
> *not* set IVD to zero (VGA enabled) if GMS pre-allocates no memory, which
> is exactly what we're doing to try to indicate no stolen memory.
> If we could actually disable VGA on IGD, that might be a nice option, but I
> know there are issues with that (iirc, there's no disable for one of either the
> VGA MMIO or I/O port space, so the only option is to disable that address
> space at the PCI command register, which has obvious implications - I think it
> was probably I/O port space).
> 
> I'm currently trying a Linux live CD on the guest for the SNB, mostly because I
> can see driver error messages and look at the driver source, neither of which
> are available to me for the Windows driver.  Both of the driver warnings I get
> are from the driver thinking a certain feature should be disabled but finds it
> enabled.  They're also sort of on a strange drm release path, maybe due to
> being in secondary mode and Xorg not being configured to use the IGD on
> this image.  In case it looks familiar:
> 
> [   12.397908] ------------[ cut here ]------------
> [   12.399175] WARNING: CPU: 0 PID: 1135 at
> drivers/gpu/drm/i915/intel_display.c:1259 assert_fdi_rx_pll+0x78/0xb0
> [i915]()
> [   12.401720] FDI RX PLL assertion failure (expected off, current on)
> [   12.403239] Modules linked in: ebtable_broute bridge stp llc ebtable_filter
> ebtable_nat ebtables ip6table_security ip6table_mangle ip6table_raw
> ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_filter
> ip6_tables iptable_security iptable_mangle iptable_raw iptable_nat
> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack
> iTCO_wdt gpio_ich iTCO_vendor_support ppdev joydev parport_pc lpc_ich
> i2c_piix4 parport acpi_cpufreq nfsd auth_rpcgss nfs_acl lockd grace isofs
> squashfs i915 bochs_drm video ttm i2c_algo_bit drm_kms_helper drm
> ata_generic serio_raw pata_acpi scsi_dh_rdac scsi_dh_emc scsi_dh_alua
> sunrpc loop
> [   12.417994] CPU: 0 PID: 1135 Comm: Xorg Not tainted 4.2.3-300.fc23.x86_64
> #1
> [   12.419733] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.9.0-80-g2390eb5 04/01/2014
> [   12.422100]  0000000000000000 00000000d92c128c ffff880078f53ab8
> ffffffff81771fca
> [   12.424023]  0000000000000000 ffff880078f53b10 ffff880078f53af8
> ffffffff8109e4a6
> [   12.425949]  ffff880078f53ad8 0000000000000000 ffff88007f680000
> ffff88007fb72000
> [   12.427719] Call Trace:
> [   12.428354]  [<ffffffff81771fca>] dump_stack+0x45/0x57
> [   12.429580]  [<ffffffff8109e4a6>] warn_slowpath_common+0x86/0xc0
> [   12.431037]  [<ffffffff8109e535>] warn_slowpath_fmt+0x55/0x70
> [   12.432497]  [<ffffffffa01e5a98>] assert_fdi_rx_pll+0x78/0xb0 [i915]
> [   12.434033]  [<ffffffffa0221433>] intel_pre_enable_lvds+0x53/0x180 [i915]
> [   12.435760]  [<ffffffffa01ef729>] ironlake_crtc_enable+0x199/0xd30 [i915]
> [   12.437433]  [<ffffffffa0190c80>] ? intel_runtime_pm_put+0x50/0x60 [i915]
> [   12.439083]  [<ffffffffa0190d98>] ? intel_display_power_put+0x108/0x160
> [i915]
> [   12.440914]  [<ffffffffa01ecff6>] __intel_set_mode+0x916/0xb60 [i915]
> [   12.442458]  [<ffffffffa01f3ee6>] intel_crtc_set_config+0x2b6/0x580 [i915]
> [   12.444180]  [<ffffffffa0099326>]
> drm_mode_set_config_internal+0x66/0x100 [drm]
> [   12.445917]  [<ffffffffa016d512>] restore_fbdev_mode+0xc2/0xf0
> [drm_kms_helper]
> [   12.447695]  [<ffffffffa016f399>]
> drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x70
> [drm_kms_helper]
> [   12.450116]  [<ffffffffa020350e>] intel_fbdev_restore_mode+0x1e/0x50
> [i915]
> [   12.451794]  [<ffffffffa022c96e>] i915_driver_lastclose+0xe/0x20 [i915]
> [   12.453360]  [<ffffffffa008c8ee>] drm_lastclose+0x2e/0x140 [drm]
> [   12.454755]  [<ffffffffa008ccaf>] drm_release+0x2af/0x490 [drm]
> [   12.456324]  [<ffffffff8121fbfc>] __fput+0xdc/0x1e0
> [   12.457525]  [<ffffffff8121fd4e>] ____fput+0xe/0x10
> [   12.458736]  [<ffffffff810bab65>] task_work_run+0x85/0xb0
> [   12.460028]  [<ffffffff81014a2d>] do_notify_resume+0x8d/0x90
> [   12.461424]  [<ffffffff81778b7c>] int_signal+0x12/0x17
> [   12.462757] ---[ end trace 3278f15671a6fa94 ]---
> [   12.546650] ------------[ cut here ]------------
> [   12.547765] WARNING: CPU: 0 PID: 1135 at
> drivers/gpu/drm/i915/intel_display.c:1466
> assert_pch_transcoder_disabled+0x67/0x70 [i915]()
> [   12.550847] transcoder assertion failed, should be off on pipe A but is still
> active
> [   12.552858] Modules linked in: ebtable_broute bridge stp llc ebtable_filter
> ebtable_nat ebtables ip6table_security ip6table_mangle ip6table_raw
> ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_filter
> ip6_tables iptable_security iptable_mangle iptable_raw iptable_nat
> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack
> iTCO_wdt gpio_ich iTCO_vendor_support ppdev joydev parport_pc lpc_ich
> i2c_piix4 parport acpi_cpufreq nfsd auth_rpcgss nfs_acl lockd grace isofs
> squashfs i915 bochs_drm video ttm i2c_algo_bit drm_kms_helper drm
> ata_generic serio_raw pata_acpi scsi_dh_rdac scsi_dh_emc scsi_dh_alua
> sunrpc loop
> [   12.568444] CPU: 0 PID: 1135 Comm: Xorg Tainted: G        W       4.2.3-
> 300.fc23.x86_64 #1
> [   12.570599] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.9.0-80-g2390eb5 04/01/2014
> [   12.572988]  0000000000000000 00000000d92c128c ffff880078f53b08
> ffffffff81771fca
> [   12.574973]  0000000000000000 ffff880078f53b60 ffff880078f53b48
> ffffffff8109e4a6
> [   12.576826]  ffff880078f53b28 0000000000000000 ffff88007f680000
> ffff88007fb72000
> [   12.578804] Call Trace:
> [   12.579454]  [<ffffffff81771fca>] dump_stack+0x45/0x57
> [   12.580775]  [<ffffffff8109e4a6>] warn_slowpath_common+0x86/0xc0
> [   12.582419]  [<ffffffff8109e535>] warn_slowpath_fmt+0x55/0x70
> [   12.583924]  [<ffffffffa01e1067>]
> assert_pch_transcoder_disabled+0x67/0x70 [i915]
> [   12.585834]  [<ffffffffa01efb7c>] ironlake_crtc_enable+0x5ec/0xd30 [i915]
> [   12.587634]  [<ffffffffa0190c80>] ? intel_runtime_pm_put+0x50/0x60 [i915]
> [   12.589402]  [<ffffffffa0190d98>] ? intel_display_power_put+0x108/0x160
> [i915]
> [   12.591294]  [<ffffffffa01ecff6>] __intel_set_mode+0x916/0xb60 [i915]
> [   12.593019]  [<ffffffffa01f3ee6>] intel_crtc_set_config+0x2b6/0x580 [i915]
> [   12.594797]  [<ffffffffa0099326>]
> drm_mode_set_config_internal+0x66/0x100 [drm]
> [   12.596651]  [<ffffffffa016d512>] restore_fbdev_mode+0xc2/0xf0
> [drm_kms_helper]
> [   12.598574]  [<ffffffffa016f399>]
> drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x70
> [drm_kms_helper]
> [   12.601022]  [<ffffffffa020350e>] intel_fbdev_restore_mode+0x1e/0x50
> [i915]
> [   12.602888]  [<ffffffffa022c96e>] i915_driver_lastclose+0xe/0x20 [i915]
> [   12.604576]  [<ffffffffa008c8ee>] drm_lastclose+0x2e/0x140 [drm]
> [   12.606098]  [<ffffffffa008ccaf>] drm_release+0x2af/0x490 [drm]
> [   12.607662]  [<ffffffff8121fbfc>] __fput+0xdc/0x1e0
> [   12.609019]  [<ffffffff8121fd4e>] ____fput+0xe/0x10
> [   12.610283]  [<ffffffff810bab65>] task_work_run+0x85/0xb0
> [   12.611718]  [<ffffffff81014a2d>] do_notify_resume+0x8d/0x90
> [   12.613225]  [<ffffffff81778b7c>] int_signal+0x12/0x17
> [   12.614600] ---[ end trace 3278f15671a6fa95 ]---
> [   12.635700] [drm:intel_set_pch_fifo_underrun_reporting [i915]] *ERROR*
> uncleared pch fifo underrun on pch transcoder A
> [   12.636007] [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR*
> PCH transcoder A FIFO underrun
> [   12.642883] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR*
> CPU pipe A FIFO underrun
> 
> Otherwise i915 seems fairly happy:
> 
> [    4.940225] i915: No ACPI video bus found
> [    4.943223] [drm] Initialized i915 1.6.0 20150522 for 0000:00:03.0 on minor 1
> [    5.000053] [drm] GMBUS [i915 gmbus vga] timed out, falling back to bit
> banging on pin 2
> [    5.038055] i915 0000:00:03.0: fb1: inteldrmfb frame buffer device
> 
> The bit banging messages is apparently normal on this system (Lenovo W520,
> Nvidia disabled in BIOS - could optimus still be getting in the way?). I don't
> know that any of the above is actually fatal though, I just know that I don't
> get a signal out of the LVDS panel or any of the other outputs. However, the
> Xorg log file looks normal and if I disable mmap support in vfio and move the
> mouse cursor around on the VNC window, the trace log looks a lot like
> graphics is running ok, the panel is just turned off. I'm thinking that makes
> the above warnings more relevant since they're in the space of FDI and PCH
> transcoders, which I think is how we get to a display output.
> 
> 
> There are certainly a lot of subtle differences between IVB and SNB in the
> Linux driver, if I have time I'll see about setting up a Windows VM on the SNB
> system.  Linux feels pretty close though if I could just get the panel turned
> on.
> 
> 
> Good, glad you found it.  I'm just running a simple VM like this:
> 
> /home/alwillia/local/bin/qemu-system-x86_64 -m 2G -bios
> /home/alwillia/Work/seabios.git/out/bios.bin -net none -cdrom
> /net/store/exports/ISOs/Fedora-Live-Cinnamon-x86_64-23-10.iso -boot d -
> monitor stdio -device vfio-pci,host=00:02.0,rombar=0,x-no-mmap=on -vnc :1
> -vga std -enable-kvm -trace events=events.txt
> 
> events.txt contains:
> vfio_region_read
> vfio_region_write
> vfio_pci_read_config
> vfio_pci_write_config
> vfio_pci_igd*
> pci_cfg_*
> 
> If you base on the last patch series I posted on top of Gerd's changes, you'll
> need -M pc,igd_passthru=on I believe too, the code I'm working on passes
> the host and ISA bridge config through vfio and initiates the ISA bridge
> automatically.  I'll try to post it this week.
> 
> > > Again, it seems just as easy, if not easier to copy GMCH from the
> > > IGD itself into the host bridge.
> > >
> > Not sure if I follow ... how does this would solve stolen memory/RMRR
> issue...?
> 
> In the vfio code I have now, the kernel virtualizes GMCH and BDSM on the
> device to provide a pre-sanitized version, so if we need those on the host
> bridge, it's easier to copy what's on the IGD instead of adding yet more code
> that knows about the different GMCH layouts.  The solution is the same,
> clear BDSM and GMCH.GMS to make the guest not try to access the host
> stolen memory.
> 
.
> 
> If I could spot any accesses to BDSM on the host bridge, I'd jump on this, but
> my guest driver doesn't seem to care about it according to the tracing.
> Lighting up an output seems to be the issue.  Thanks,
> 
> Alex

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-11  2:25               ` Kay, Allen M
@ 2016-02-11  6:07                 ` Alex Williamson
  0 siblings, 0 replies; 25+ messages in thread
From: Alex Williamson @ 2016-02-11  6:07 UTC (permalink / raw)
  To: Kay, Allen M
  Cc: Ruan, Shuai, ehabkost, stefano.stabellini, mst, qemu-devel,
	pbonzini, rth

On Thu, 11 Feb 2016 02:25:51 +0000
"Kay, Allen M" <allen.m.kay@intel.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Tuesday, February 09, 2016 10:01 PM
> > To: Kay, Allen M <allen.m.kay@intel.com>
> > Cc: ehabkost@redhat.com; mst@redhat.com;
> > stefano.stabellini@eu.citrix.com; qemu-devel@nongnu.org;
> > pbonzini@redhat.com; rth@twiddle.net; Ruan, Shuai
> > <shuai.ruan@intel.com>
> > Subject: Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to
> > passthrough
> > 
> > I can run that system as either primary or secondary.  On both systems I'm
> > using pci-stub to keep i915 from interfering on the host, on the SNB laptop I
> > also use video=efifb:off since it's configured for UEFI boot, I'm not sure if
> > that play any role in why it's not working.
> >   
> 
> Hi Alex,
> 
> My understand if that your IVB system is a desktop running legacy mode BIOS and your SNB is a laptop running EFI mode BIOS, correct?  I'm curious how does your SNB system getting hold of VBT table, which is needed for lighting up local display.
> 
> There are two ways the driver can get VBT table:  1) VGA 0xc0000 memory,  2) OpRegion
> 
> On your IVB system, the guest driver would try to get VBT from 0xc0000 memory if IGD is configured as primary controller in the guest, assuming KVM/QEMU copies VGA 0xc0000 memory from host to guest 0xc0000 area.   If IGD is configured as secondary controller, the driver would get VBT from OpRegion.  Given IGD works in both configurations on IVB, this mean guest driver can successfully read VBT from both 0xc0000 area and OpRegion.
> 
> On your SNB system that is running in EFI mode on the host, 0xc0000 memory does not contain VBIOS/VBT.  This means if you configure IGD as primary controller in the guest, the driver cannot get to VBT at 0xc0000 memory even if KVM/QEMU copies 0xc0000 memory from host to guest.  If you configure IGD as secondary controller in the guest, and guest driver should be able to read VBT from the OpRegion.  You might want to trace i915 to see how does the  guest driver get to VBT via OpRegion in this configuration.

Hi Allen,

Success!  We don't directly pass 0xc0000 from the host, but we can use
PCI option ROMs loaded via files into QEMU.  For the IVB system I
haven't been bothering with this, even in primary mode I've just been
letting the display initialize later in the guest boot.  I also haven't
been enabling VGA mode access in that case.  With the SNB laptop, if I
extract the ROM from sysfs, modify the device ID and fix the checksum,
the panel lights up, with or without VGA mode access (though of course
I only see a flash of SeaBIOS when VGA mode is enabled).  So it seems
that either the user is going to need to hack their own ROM file or I'm
going to need to make vfio do this automatically and pretend that the
ROM is actually implemented as a BAR on IGD.  The latter seems far more
accessible.  Lacking an actual ROM BAR, we'll also of course only be
able to do that when IGD is primary graphics on the host.

I'm not sure how we can do secondary mode in the VM with this config
though since only the primary graphics gets the coveted 0xc0000
location.  FWIW, to make this work I added 'romfile=igd.rom' to the
vfio-pci device with igd.rom being the modified copy retrieved from
sysfs.  Then I used '-vga none' to disable the QEMU primary VGA device
(-nodefaults is probably an option too).  I then added 'addr=2.0' to
the vfio-pci device to make it be the primary graphics from SeaBIOS'
perspective and added '-device secondary-vga,addr=3.0 -vnc :1', which
predominantly gives me a VNC connection where I can interact with the
VM via mouse and keyboard.

I'll need to do some further investigation to see what we're really
getting with the OpRegion.  Prior to adding the ROM, the Xorg log file
sure seems like it knew the LVDS panel was 1920x1080, but it might be
seeing more modes now that it has a video BIOS.  I'll also try to prune
the registers copied into the virtual host bridge and ISA bridge config
space to the minimum we need.

> Another potential problem to watch out for with laptop vs. desktop has to do FLR timeout.  If you are working with a laptop with LCD panel attached (the usual case), FLR will take much longer than 100ms to finish.  The reason is the devices needs to power down the LCD panel before it can finish FLR. I have seen it takes more than 500ms for FLR to finish.  As a work around, I have increase FLR time out in Linux PCI driver to 1000ms when working with LCD panels.   Given that you have seen evidences that IGD HW is working, this might not be your issue.   I would focus tracing how does i915 get VBT from the OpRegion when IGD is configured as secondary controller in the guest.

Ok, I did see evidence of this.  In my case the VM would always fail to
start on the first try with errors in dmesg indicating that vfio was
reading back -1 from config space of the device.  Presumably it was
only the first attempt in my case since previously I was never getting
the panel turned back on and subsequent resets were faster.  I also
notice that I see DMAR faults on the first reset that seem to be
accesses to the host stolen memory region pointed to by the BDSM.  I
don't see this if I reset the device from sysfs before trying to use it
with the VM, so maybe vfio should try to do a reset on probing IGD,
before it gets placed in an IOMMU domain that blocks access to that
host stolen memory.  Of course the next question is whether we can
easily determine whether an IGD has an LCD panel so we know which
timeout to use.  I suppose we could use a 1000ms timeout for all Intel
VGA class devices, but handling it with a quirk only for panel attached
configs would probably be preferable.  Thanks,

Alex

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough
  2016-02-09 21:32         ` Alex Williamson
  2016-02-10  3:40           ` Kay, Allen M
@ 2016-02-15 11:28           ` Gerd Hoffmann
  1 sibling, 0 replies; 25+ messages in thread
From: Gerd Hoffmann @ 2016-02-15 11:28 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Ruan, Shuai, ehabkost, mst, stefano.stabellini, Kay, Allen M,
	qemu-devel, pbonzini, rth

  Hi,

> > I believe this is same as 0x50 but on old Ironlake platform.  Xen started IGD passthrough support in Ironlake.
> 
> Ok, unless anyone shouts really loudly and it doesn't affect anything
> newer than IronLake, I'm happy to let those fall off the plate.  I don't
> have any older systems that I care to make work with this.

We are also arriving at a hardware age where vt-d for igd isn't working
properly, so supporting only Ironlake+ make sense to me.

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2016-02-15 11:28 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-15  5:37 [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 01/10] i440fx: make types configurable at run-time Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 02/10] pc_init1: pass parameters just with types Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 03/10] piix: create host bridge to passthrough Tiejun Chen
2016-02-09  3:32   ` Alex Williamson
2016-02-09 17:43     ` Alex Williamson
2016-02-09 17:56       ` Paolo Bonzini
2016-02-09 19:47       ` Kay, Allen M
2016-02-09 21:32         ` Alex Williamson
2016-02-10  3:40           ` Kay, Allen M
2016-02-10  6:00             ` Alex Williamson
2016-02-11  2:25               ` Kay, Allen M
2016-02-11  6:07                 ` Alex Williamson
2016-02-15 11:28           ` Gerd Hoffmann
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 04/10] hw/pci-assign: split pci-assign.c Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 05/10] xen, gfx passthrough: basic graphics passthrough support Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 06/10] xen, gfx passthrough: retrieve VGA BIOS to work Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 07/10] igd gfx passthrough: create a isa bridge Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 08/10] xen, gfx passthrough: register " Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 09/10] xen, gfx passthrough: register host bridge specific to passthrough Tiejun Chen
2015-07-15  5:37 ` [Qemu-devel] [v10][PATCH 10/10] xen, gfx passthrough: add opregion mapping Tiejun Chen
2015-07-15 11:46 ` [Qemu-devel] [v10][PATCH 00/10] xen: add Intel IGD passthrough Stefano Stabellini
2015-07-15 12:01   ` Michael S. Tsirkin
2015-07-15 12:38     ` Stefano Stabellini
2015-07-15 13:27       ` Chen, Tiejun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).