* [Qemu-devel] [RFC][PATCH 1/2] hw:xen:xen_pt: register isa bridge specific to IGD passthrough
@ 2014-11-05 7:22 ` Tiejun Chen
0 siblings, 0 replies; 22+ messages in thread
From: Tiejun Chen @ 2014-11-05 7:22 UTC (permalink / raw)
To: mst, pbonzini, rth, aliguori; +Cc: xen-devel, allen.m.kay, qemu-devel
We need this instance to passthrough some config fields of PCH.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
hw/xen/xen_pt.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index c1bf357..403c33f 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -60,6 +60,7 @@
#include "xen_pt.h"
#include "qemu/range.h"
#include "exec/address-spaces.h"
+#include "qapi/visitor.h"
#define XEN_PT_NR_IRQS (256)
static uint8_t xen_pt_mapped_machine_irq[XEN_PT_NR_IRQS] = {0};
@@ -829,3 +830,114 @@ static void xen_pci_passthrough_register_types(void)
}
type_init(xen_pci_passthrough_register_types)
+
+typedef struct {
+ uint16_t gpu_device_id;
+ uint16_t pch_device_id;
+} XenIGDDeviceIDInfo;
+
+/* 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 XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
+ /* HSW Classic */
+ {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
+ {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
+ {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
+ {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
+ {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
+ /* HSW ULT */
+ {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
+ {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
+ {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
+ {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
+ {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
+ {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
+ /* HSW CRW */
+ {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
+ {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
+ /* HSW Server */
+ {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
+ /* HSW SRVR */
+ {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
+ /* BSW */
+ {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
+ {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
+ {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
+ {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
+ {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
+ {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
+ {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
+ {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
+ {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
+ {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
+ {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
+};
+
+static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
+ Visitor *v,
+ void *opaque,
+ const char *name,
+ Error **errp)
+{
+ uint32_t gpu_id = 0xffff, pch_id = 0xffff;
+ XenHostPCIDevice hdev;
+ int r = 0, num;
+
+ r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
+ if (!r) {
+ gpu_id = hdev.device_id;
+
+ num = ARRAY_SIZE(xen_igd_combo_id_infos);
+ for (r = 0; r < num; r++)
+ if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
+ pch_id = xen_igd_combo_id_infos[r].pch_device_id;
+ }
+
+ visit_type_uint32(v, &pch_id, name, errp);
+}
+
+static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
+{
+ object_property_add(obj, "device-id", "int",
+ xen_igd_passthrough_pciisabridge_get_pci_device_id,
+ NULL, NULL, NULL, NULL);
+}
+
+static void xen_igd_passthrough_isa_bridge_class_init(ObjectClass *klass,
+ void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ dc->desc = "XEN IGD PASSTHROUGH ISA bridge";
+ k->class_id = PCI_CLASS_BRIDGE_ISA;
+ k->vendor_id = PCI_VENDOR_ID_INTEL;
+};
+
+static TypeInfo xen_igd_passthrough_isa_bridge_info = {
+ .name = "xen-igd-passthrough-isa-bridge",
+ .parent = TYPE_PCI_DEVICE,
+ .instance_size = sizeof(PCIDevice),
+ .instance_init = xen_igd_passthrough_isa_bridge_initfn,
+ .class_init = xen_igd_passthrough_isa_bridge_class_init,
+};
+
+static void xen_igd_passthrough_isa_bridge_register_types(void)
+{
+ type_register_static(&xen_igd_passthrough_isa_bridge_info);
+}
+
+type_init(xen_igd_passthrough_isa_bridge_register_types);
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC][PATCH 1/2] hw:xen:xen_pt: register isa bridge specific to IGD passthrough
@ 2014-11-05 7:22 ` Tiejun Chen
0 siblings, 0 replies; 22+ messages in thread
From: Tiejun Chen @ 2014-11-05 7:22 UTC (permalink / raw)
To: mst, pbonzini, rth, aliguori; +Cc: xen-devel, allen.m.kay, qemu-devel
We need this instance to passthrough some config fields of PCH.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
hw/xen/xen_pt.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index c1bf357..403c33f 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -60,6 +60,7 @@
#include "xen_pt.h"
#include "qemu/range.h"
#include "exec/address-spaces.h"
+#include "qapi/visitor.h"
#define XEN_PT_NR_IRQS (256)
static uint8_t xen_pt_mapped_machine_irq[XEN_PT_NR_IRQS] = {0};
@@ -829,3 +830,114 @@ static void xen_pci_passthrough_register_types(void)
}
type_init(xen_pci_passthrough_register_types)
+
+typedef struct {
+ uint16_t gpu_device_id;
+ uint16_t pch_device_id;
+} XenIGDDeviceIDInfo;
+
+/* 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 XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
+ /* HSW Classic */
+ {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
+ {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
+ {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
+ {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
+ {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
+ /* HSW ULT */
+ {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
+ {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
+ {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
+ {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
+ {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
+ {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
+ /* HSW CRW */
+ {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
+ {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
+ /* HSW Server */
+ {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
+ /* HSW SRVR */
+ {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
+ /* BSW */
+ {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
+ {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
+ {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
+ {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
+ {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
+ {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
+ {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
+ {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
+ {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
+ {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
+ {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
+};
+
+static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
+ Visitor *v,
+ void *opaque,
+ const char *name,
+ Error **errp)
+{
+ uint32_t gpu_id = 0xffff, pch_id = 0xffff;
+ XenHostPCIDevice hdev;
+ int r = 0, num;
+
+ r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
+ if (!r) {
+ gpu_id = hdev.device_id;
+
+ num = ARRAY_SIZE(xen_igd_combo_id_infos);
+ for (r = 0; r < num; r++)
+ if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
+ pch_id = xen_igd_combo_id_infos[r].pch_device_id;
+ }
+
+ visit_type_uint32(v, &pch_id, name, errp);
+}
+
+static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
+{
+ object_property_add(obj, "device-id", "int",
+ xen_igd_passthrough_pciisabridge_get_pci_device_id,
+ NULL, NULL, NULL, NULL);
+}
+
+static void xen_igd_passthrough_isa_bridge_class_init(ObjectClass *klass,
+ void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ dc->desc = "XEN IGD PASSTHROUGH ISA bridge";
+ k->class_id = PCI_CLASS_BRIDGE_ISA;
+ k->vendor_id = PCI_VENDOR_ID_INTEL;
+};
+
+static TypeInfo xen_igd_passthrough_isa_bridge_info = {
+ .name = "xen-igd-passthrough-isa-bridge",
+ .parent = TYPE_PCI_DEVICE,
+ .instance_size = sizeof(PCIDevice),
+ .instance_init = xen_igd_passthrough_isa_bridge_initfn,
+ .class_init = xen_igd_passthrough_isa_bridge_class_init,
+};
+
+static void xen_igd_passthrough_isa_bridge_register_types(void)
+{
+ type_register_static(&xen_igd_passthrough_isa_bridge_info);
+}
+
+type_init(xen_igd_passthrough_isa_bridge_register_types);
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-05 7:22 ` Tiejun Chen
@ 2014-11-05 7:22 ` Tiejun Chen
-1 siblings, 0 replies; 22+ messages in thread
From: Tiejun Chen @ 2014-11-05 7:22 UTC (permalink / raw)
To: mst, pbonzini, rth, aliguori; +Cc: xen-devel, allen.m.kay, qemu-devel
Currently IGD drivers always need to access PCH by 1f.0, and
PCH vendor/device id is used to identify the card.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b559181..b19c7a9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -50,7 +50,7 @@
#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>
#endif
#define MAX_IDE_BUS 2
@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
}
#ifdef CONFIG_XEN
+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
+{
+ struct PCIDevice *dev;
+ Error *local_err = NULL;
+ uint16_t device_id = 0xffff;
+
+ /* Currently IGD drivers always need to access PCH by 1f.0. */
+ dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
+ "xen-igd-passthrough-isa-bridge");
+
+ /* Identify PCH card with its own real vendor/device ids.
+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
+ */
+ if (dev) {
+ device_id = object_property_get_int(OBJECT(dev), "device-id",
+ &local_err);
+ if (!local_err && device_id != 0xffff) {
+ pci_config_set_device_id(dev->config, device_id);
+ return;
+ }
+ }
+
+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
+}
+
static void pc_xen_hvm_init(MachineState *machine)
{
PCIBus *bus;
@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
bus = pci_find_primary_bus();
if (bus != NULL) {
pci_create_simple(bus, -1, "xen-platform");
+ xen_igd_passthrough_isa_bridge_create(bus);
}
}
#endif
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-05 7:22 ` Tiejun Chen
0 siblings, 0 replies; 22+ messages in thread
From: Tiejun Chen @ 2014-11-05 7:22 UTC (permalink / raw)
To: mst, pbonzini, rth, aliguori; +Cc: xen-devel, allen.m.kay, qemu-devel
Currently IGD drivers always need to access PCH by 1f.0, and
PCH vendor/device id is used to identify the card.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b559181..b19c7a9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -50,7 +50,7 @@
#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>
#endif
#define MAX_IDE_BUS 2
@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
}
#ifdef CONFIG_XEN
+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
+{
+ struct PCIDevice *dev;
+ Error *local_err = NULL;
+ uint16_t device_id = 0xffff;
+
+ /* Currently IGD drivers always need to access PCH by 1f.0. */
+ dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
+ "xen-igd-passthrough-isa-bridge");
+
+ /* Identify PCH card with its own real vendor/device ids.
+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
+ */
+ if (dev) {
+ device_id = object_property_get_int(OBJECT(dev), "device-id",
+ &local_err);
+ if (!local_err && device_id != 0xffff) {
+ pci_config_set_device_id(dev->config, device_id);
+ return;
+ }
+ }
+
+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
+}
+
static void pc_xen_hvm_init(MachineState *machine)
{
PCIBus *bus;
@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
bus = pci_find_primary_bus();
if (bus != NULL) {
pci_create_simple(bus, -1, "xen-platform");
+ xen_igd_passthrough_isa_bridge_create(bus);
}
}
#endif
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-05 7:22 ` Tiejun Chen
@ 2014-11-05 14:09 ` Michael S. Tsirkin
-1 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-05 14:09 UTC (permalink / raw)
To: Tiejun Chen; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> Currently IGD drivers always need to access PCH by 1f.0, and
> PCH vendor/device id is used to identify the card.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b559181..b19c7a9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -50,7 +50,7 @@
> #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>
> #endif
>
> #define MAX_IDE_BUS 2
> @@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
> }
>
> #ifdef CONFIG_XEN
> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> +{
> + struct PCIDevice *dev;
> + Error *local_err = NULL;
> + uint16_t device_id = 0xffff;
> +
> + /* Currently IGD drivers always need to access PCH by 1f.0. */
> + dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> + "xen-igd-passthrough-isa-bridge");
> +
> + /* Identify PCH card with its own real vendor/device ids.
> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> + */
> + if (dev) {
> + device_id = object_property_get_int(OBJECT(dev), "device-id",
> + &local_err);
> + if (!local_err && device_id != 0xffff) {
> + pci_config_set_device_id(dev->config, device_id);
> + return;
> + }
> + }
> +
> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> +}
> +
> static void pc_xen_hvm_init(MachineState *machine)
> {
> PCIBus *bus;
> @@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> bus = pci_find_primary_bus();
> if (bus != NULL) {
> pci_create_simple(bus, -1, "xen-platform");
> + xen_igd_passthrough_isa_bridge_create(bus);
> }
> }
> #endif
Can't we defer this step until the GPU is added?
This way there won't be need to poke at host device
directly, you could get all info from dev->config
of the host device.
Additionally the correct bridge would be initialized automatically.
> --
> 1.9.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-05 14:09 ` Michael S. Tsirkin
0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-05 14:09 UTC (permalink / raw)
To: Tiejun Chen; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> Currently IGD drivers always need to access PCH by 1f.0, and
> PCH vendor/device id is used to identify the card.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b559181..b19c7a9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -50,7 +50,7 @@
> #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>
> #endif
>
> #define MAX_IDE_BUS 2
> @@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
> }
>
> #ifdef CONFIG_XEN
> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> +{
> + struct PCIDevice *dev;
> + Error *local_err = NULL;
> + uint16_t device_id = 0xffff;
> +
> + /* Currently IGD drivers always need to access PCH by 1f.0. */
> + dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> + "xen-igd-passthrough-isa-bridge");
> +
> + /* Identify PCH card with its own real vendor/device ids.
> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> + */
> + if (dev) {
> + device_id = object_property_get_int(OBJECT(dev), "device-id",
> + &local_err);
> + if (!local_err && device_id != 0xffff) {
> + pci_config_set_device_id(dev->config, device_id);
> + return;
> + }
> + }
> +
> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> +}
> +
> static void pc_xen_hvm_init(MachineState *machine)
> {
> PCIBus *bus;
> @@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> bus = pci_find_primary_bus();
> if (bus != NULL) {
> pci_create_simple(bus, -1, "xen-platform");
> + xen_igd_passthrough_isa_bridge_create(bus);
> }
> }
> #endif
Can't we defer this step until the GPU is added?
This way there won't be need to poke at host device
directly, you could get all info from dev->config
of the host device.
Additionally the correct bridge would be initialized automatically.
> --
> 1.9.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-05 14:09 ` Michael S. Tsirkin
@ 2014-11-17 2:47 ` Chen, Tiejun
-1 siblings, 0 replies; 22+ messages in thread
From: Chen, Tiejun @ 2014-11-17 2:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
>> Currently IGD drivers always need to access PCH by 1f.0, and
>> PCH vendor/device id is used to identify the card.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
>> 1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index b559181..b19c7a9 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -50,7 +50,7 @@
>> #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>
>> #endif
>>
>> #define MAX_IDE_BUS 2
>> @@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
>> }
>>
>> #ifdef CONFIG_XEN
>> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
>> +{
>> + struct PCIDevice *dev;
>> + Error *local_err = NULL;
>> + uint16_t device_id = 0xffff;
>> +
>> + /* Currently IGD drivers always need to access PCH by 1f.0. */
>> + dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
>> + "xen-igd-passthrough-isa-bridge");
>> +
>> + /* Identify PCH card with its own real vendor/device ids.
>> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
>> + */
>> + if (dev) {
>> + device_id = object_property_get_int(OBJECT(dev), "device-id",
>> + &local_err);
>> + if (!local_err && device_id != 0xffff) {
>> + pci_config_set_device_id(dev->config, device_id);
>> + return;
>> + }
>> + }
>> +
>> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
>> +}
>> +
>> static void pc_xen_hvm_init(MachineState *machine)
>> {
>> PCIBus *bus;
>> @@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
>> bus = pci_find_primary_bus();
>> if (bus != NULL) {
>> pci_create_simple(bus, -1, "xen-platform");
>> + xen_igd_passthrough_isa_bridge_create(bus);
>> }
>> }
>> #endif
>
> Can't we defer this step until the GPU is added?
Sounds great but I can't figure out where we can to do this exactly.
> This way there won't be need to poke at host device
> directly, you could get all info from dev->config
> of the host device.
As I understand We have two steps here:
#1 At first I have to write something to check if we're registering
00:02.0 & IGD, right? But where? While registering each pci device?
#2 Then if that condition is matched, we register this ISA bridge on its
associated bus.
Thanks
Tiejun
> Additionally the correct bridge would be initialized automatically.
>
>
>> --
>> 1.9.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-17 2:47 ` Chen, Tiejun
0 siblings, 0 replies; 22+ messages in thread
From: Chen, Tiejun @ 2014-11-17 2:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
>> Currently IGD drivers always need to access PCH by 1f.0, and
>> PCH vendor/device id is used to identify the card.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
>> 1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index b559181..b19c7a9 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -50,7 +50,7 @@
>> #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>
>> #endif
>>
>> #define MAX_IDE_BUS 2
>> @@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
>> }
>>
>> #ifdef CONFIG_XEN
>> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
>> +{
>> + struct PCIDevice *dev;
>> + Error *local_err = NULL;
>> + uint16_t device_id = 0xffff;
>> +
>> + /* Currently IGD drivers always need to access PCH by 1f.0. */
>> + dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
>> + "xen-igd-passthrough-isa-bridge");
>> +
>> + /* Identify PCH card with its own real vendor/device ids.
>> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
>> + */
>> + if (dev) {
>> + device_id = object_property_get_int(OBJECT(dev), "device-id",
>> + &local_err);
>> + if (!local_err && device_id != 0xffff) {
>> + pci_config_set_device_id(dev->config, device_id);
>> + return;
>> + }
>> + }
>> +
>> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
>> +}
>> +
>> static void pc_xen_hvm_init(MachineState *machine)
>> {
>> PCIBus *bus;
>> @@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
>> bus = pci_find_primary_bus();
>> if (bus != NULL) {
>> pci_create_simple(bus, -1, "xen-platform");
>> + xen_igd_passthrough_isa_bridge_create(bus);
>> }
>> }
>> #endif
>
> Can't we defer this step until the GPU is added?
Sounds great but I can't figure out where we can to do this exactly.
> This way there won't be need to poke at host device
> directly, you could get all info from dev->config
> of the host device.
As I understand We have two steps here:
#1 At first I have to write something to check if we're registering
00:02.0 & IGD, right? But where? While registering each pci device?
#2 Then if that condition is matched, we register this ISA bridge on its
associated bus.
Thanks
Tiejun
> Additionally the correct bridge would be initialized automatically.
>
>
>> --
>> 1.9.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-17 2:47 ` Chen, Tiejun
@ 2014-11-17 6:10 ` Michael S. Tsirkin
-1 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 6:10 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>Currently IGD drivers always need to access PCH by 1f.0, and
> >>PCH vendor/device id is used to identify the card.
> >>
> >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>---
> >> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
> >> 1 file changed, 27 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>index b559181..b19c7a9 100644
> >>--- a/hw/i386/pc_piix.c
> >>+++ b/hw/i386/pc_piix.c
> >>@@ -50,7 +50,7 @@
> >> #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>
> >> #endif
> >>
> >> #define MAX_IDE_BUS 2
> >>@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
> >> }
> >>
> >> #ifdef CONFIG_XEN
> >>+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> >>+{
> >>+ struct PCIDevice *dev;
> >>+ Error *local_err = NULL;
> >>+ uint16_t device_id = 0xffff;
> >>+
> >>+ /* Currently IGD drivers always need to access PCH by 1f.0. */
> >>+ dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> >>+ "xen-igd-passthrough-isa-bridge");
> >>+
> >>+ /* Identify PCH card with its own real vendor/device ids.
> >>+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> >>+ */
> >>+ if (dev) {
> >>+ device_id = object_property_get_int(OBJECT(dev), "device-id",
> >>+ &local_err);
> >>+ if (!local_err && device_id != 0xffff) {
> >>+ pci_config_set_device_id(dev->config, device_id);
> >>+ return;
> >>+ }
> >>+ }
> >>+
> >>+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> >>+}
> >>+
> >> static void pc_xen_hvm_init(MachineState *machine)
> >> {
> >> PCIBus *bus;
> >>@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> >> bus = pci_find_primary_bus();
> >> if (bus != NULL) {
> >> pci_create_simple(bus, -1, "xen-platform");
> >>+ xen_igd_passthrough_isa_bridge_create(bus);
> >> }
> >> }
> >> #endif
> >
> >Can't we defer this step until the GPU is added?
>
> Sounds great but I can't figure out where we can to do this exactly.
>
> >This way there won't be need to poke at host device
> >directly, you could get all info from dev->config
> >of the host device.
>
> As I understand We have two steps here:
>
> #1 At first I have to write something to check if we're registering 00:02.0
> & IGD, right? But where? While registering each pci device?
In xen_pt_initfn.
Just check the device and vendor ID against the table you have.
> #2 Then if that condition is matched, we register this ISA bridge on its
> associated bus.
>
> Thanks
> Tiejun
Yep.
> >Additionally the correct bridge would be initialized automatically.
> >
> >
> >>--
> >>1.9.1
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-17 6:10 ` Michael S. Tsirkin
0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 6:10 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>Currently IGD drivers always need to access PCH by 1f.0, and
> >>PCH vendor/device id is used to identify the card.
> >>
> >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>---
> >> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
> >> 1 file changed, 27 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>index b559181..b19c7a9 100644
> >>--- a/hw/i386/pc_piix.c
> >>+++ b/hw/i386/pc_piix.c
> >>@@ -50,7 +50,7 @@
> >> #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>
> >> #endif
> >>
> >> #define MAX_IDE_BUS 2
> >>@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
> >> }
> >>
> >> #ifdef CONFIG_XEN
> >>+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> >>+{
> >>+ struct PCIDevice *dev;
> >>+ Error *local_err = NULL;
> >>+ uint16_t device_id = 0xffff;
> >>+
> >>+ /* Currently IGD drivers always need to access PCH by 1f.0. */
> >>+ dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> >>+ "xen-igd-passthrough-isa-bridge");
> >>+
> >>+ /* Identify PCH card with its own real vendor/device ids.
> >>+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> >>+ */
> >>+ if (dev) {
> >>+ device_id = object_property_get_int(OBJECT(dev), "device-id",
> >>+ &local_err);
> >>+ if (!local_err && device_id != 0xffff) {
> >>+ pci_config_set_device_id(dev->config, device_id);
> >>+ return;
> >>+ }
> >>+ }
> >>+
> >>+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> >>+}
> >>+
> >> static void pc_xen_hvm_init(MachineState *machine)
> >> {
> >> PCIBus *bus;
> >>@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> >> bus = pci_find_primary_bus();
> >> if (bus != NULL) {
> >> pci_create_simple(bus, -1, "xen-platform");
> >>+ xen_igd_passthrough_isa_bridge_create(bus);
> >> }
> >> }
> >> #endif
> >
> >Can't we defer this step until the GPU is added?
>
> Sounds great but I can't figure out where we can to do this exactly.
>
> >This way there won't be need to poke at host device
> >directly, you could get all info from dev->config
> >of the host device.
>
> As I understand We have two steps here:
>
> #1 At first I have to write something to check if we're registering 00:02.0
> & IGD, right? But where? While registering each pci device?
In xen_pt_initfn.
Just check the device and vendor ID against the table you have.
> #2 Then if that condition is matched, we register this ISA bridge on its
> associated bus.
>
> Thanks
> Tiejun
Yep.
> >Additionally the correct bridge would be initialized automatically.
> >
> >
> >>--
> >>1.9.1
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-17 6:10 ` Michael S. Tsirkin
@ 2014-11-17 8:48 ` Chen, Tiejun
-1 siblings, 0 replies; 22+ messages in thread
From: Chen, Tiejun @ 2014-11-17 8:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
>> On 2014/11/5 22:09, Michael S. Tsirkin wrote:
>>> On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
>>>> Currently IGD drivers always need to access PCH by 1f.0, and
>>>> PCH vendor/device id is used to identify the card.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>> ---
>>>> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
>>>> 1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index b559181..b19c7a9 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -50,7 +50,7 @@
>>>> #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>
>>>> #endif
>>>>
>>>> #define MAX_IDE_BUS 2
>>>> @@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
>>>> }
>>>>
>>>> #ifdef CONFIG_XEN
>>>> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
>>>> +{
>>>> + struct PCIDevice *dev;
>>>> + Error *local_err = NULL;
>>>> + uint16_t device_id = 0xffff;
>>>> +
>>>> + /* Currently IGD drivers always need to access PCH by 1f.0. */
>>>> + dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
>>>> + "xen-igd-passthrough-isa-bridge");
>>>> +
>>>> + /* Identify PCH card with its own real vendor/device ids.
>>>> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
>>>> + */
>>>> + if (dev) {
>>>> + device_id = object_property_get_int(OBJECT(dev), "device-id",
>>>> + &local_err);
>>>> + if (!local_err && device_id != 0xffff) {
>>>> + pci_config_set_device_id(dev->config, device_id);
>>>> + return;
>>>> + }
>>>> + }
>>>> +
>>>> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
>>>> +}
>>>> +
>>>> static void pc_xen_hvm_init(MachineState *machine)
>>>> {
>>>> PCIBus *bus;
>>>> @@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
>>>> bus = pci_find_primary_bus();
>>>> if (bus != NULL) {
>>>> pci_create_simple(bus, -1, "xen-platform");
>>>> + xen_igd_passthrough_isa_bridge_create(bus);
>>>> }
>>>> }
>>>> #endif
>>>
>>> Can't we defer this step until the GPU is added?
>>
>> Sounds great but I can't figure out where we can to do this exactly.
>>
>>> This way there won't be need to poke at host device
>>> directly, you could get all info from dev->config
>>> of the host device.
>>
>> As I understand We have two steps here:
>>
>> #1 At first I have to write something to check if we're registering 00:02.0
>> & IGD, right? But where? While registering each pci device?
>
> In xen_pt_initfn.
> Just check the device and vendor ID against the table you have.
>
Okay. Please see the follows which is just compiled:
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index c6466dc..f3ea313 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -632,6 +632,94 @@ static const MemoryListener xen_pt_io_listener = {
.priority = 10,
};
+typedef struct {
+ uint16_t gpu_device_id;
+ uint16_t pch_device_id;
+} XenIGDDeviceIDInfo;
+
+/* 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 XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
+ /* HSW Classic */
+ {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
+ {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
+ {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
+ {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
+ {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
+ /* HSW ULT */
+ {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
+ {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
+ {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
+ {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
+ {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
+ {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
+ /* HSW CRW */
+ {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
+ {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
+ /* HSW Server */
+ {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
+ /* HSW SRVR */
+ {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
+ /* BSW */
+ {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
+ {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
+ {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
+ {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
+ {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
+ {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
+ {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
+ {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
+ {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
+ {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
+ {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
+};
+
+static void
+xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
+ XenHostPCIDevice *dev)
+{
+ struct PCIDevice *pci_dev;
+ int i, num;
+ uint16_t gpu_id = 0xffff, pch_id = 0xffff;
+ PCIDevice *d = &s->dev;
+
+ if (is_vga_passthrough(dev)) {
+ gpu_id = dev->device_id;
+ num = ARRAY_SIZE(xen_igd_combo_id_infos);
+ for (i = 0; i < num; i++)
+ if (gpu_id == xen_igd_combo_id_infos[i].gpu_device_id)
+ pch_id = xen_igd_combo_id_infos[i].pch_device_id;
+
+ /* Currently IGD drivers always need to access PCH by 1f.0. */
+ pci_dev = pci_create_simple(d->bus, PCI_DEVFN(0x1f, 0),
+ "xen-igd-passthrough-isa-bridge");
+
+ /*
+ * Identify PCH card with its own real vendor/device ids.
+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
+ */
+ if (pci_dev) {
+ pci_config_set_device_id(pci_dev->config, pch_id);
+ return;
+ }
+
+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge
failed!\n");
+ }
+}
+
/* init */
static int xen_pt_initfn(PCIDevice *d)
@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
return -1;
}
+ /* Register ISA bridge for passthrough GFX. */
+ xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
+
/* reinitialize each config register to be emulated */
if (xen_pt_config_init(s)) {
XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
Note I will introduce a inline function in another patch,
+static inline int is_vga_passthrough(XenHostPCIDevice *dev)
+{
+ return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
+ && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
+}
Thanks
Tiejun
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-17 8:48 ` Chen, Tiejun
0 siblings, 0 replies; 22+ messages in thread
From: Chen, Tiejun @ 2014-11-17 8:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
>> On 2014/11/5 22:09, Michael S. Tsirkin wrote:
>>> On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
>>>> Currently IGD drivers always need to access PCH by 1f.0, and
>>>> PCH vendor/device id is used to identify the card.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>> ---
>>>> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
>>>> 1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index b559181..b19c7a9 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -50,7 +50,7 @@
>>>> #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>
>>>> #endif
>>>>
>>>> #define MAX_IDE_BUS 2
>>>> @@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
>>>> }
>>>>
>>>> #ifdef CONFIG_XEN
>>>> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
>>>> +{
>>>> + struct PCIDevice *dev;
>>>> + Error *local_err = NULL;
>>>> + uint16_t device_id = 0xffff;
>>>> +
>>>> + /* Currently IGD drivers always need to access PCH by 1f.0. */
>>>> + dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
>>>> + "xen-igd-passthrough-isa-bridge");
>>>> +
>>>> + /* Identify PCH card with its own real vendor/device ids.
>>>> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
>>>> + */
>>>> + if (dev) {
>>>> + device_id = object_property_get_int(OBJECT(dev), "device-id",
>>>> + &local_err);
>>>> + if (!local_err && device_id != 0xffff) {
>>>> + pci_config_set_device_id(dev->config, device_id);
>>>> + return;
>>>> + }
>>>> + }
>>>> +
>>>> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
>>>> +}
>>>> +
>>>> static void pc_xen_hvm_init(MachineState *machine)
>>>> {
>>>> PCIBus *bus;
>>>> @@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
>>>> bus = pci_find_primary_bus();
>>>> if (bus != NULL) {
>>>> pci_create_simple(bus, -1, "xen-platform");
>>>> + xen_igd_passthrough_isa_bridge_create(bus);
>>>> }
>>>> }
>>>> #endif
>>>
>>> Can't we defer this step until the GPU is added?
>>
>> Sounds great but I can't figure out where we can to do this exactly.
>>
>>> This way there won't be need to poke at host device
>>> directly, you could get all info from dev->config
>>> of the host device.
>>
>> As I understand We have two steps here:
>>
>> #1 At first I have to write something to check if we're registering 00:02.0
>> & IGD, right? But where? While registering each pci device?
>
> In xen_pt_initfn.
> Just check the device and vendor ID against the table you have.
>
Okay. Please see the follows which is just compiled:
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index c6466dc..f3ea313 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -632,6 +632,94 @@ static const MemoryListener xen_pt_io_listener = {
.priority = 10,
};
+typedef struct {
+ uint16_t gpu_device_id;
+ uint16_t pch_device_id;
+} XenIGDDeviceIDInfo;
+
+/* 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 XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
+ /* HSW Classic */
+ {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
+ {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
+ {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
+ {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
+ {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
+ /* HSW ULT */
+ {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
+ {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
+ {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
+ {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
+ {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
+ {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
+ /* HSW CRW */
+ {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
+ {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
+ /* HSW Server */
+ {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
+ /* HSW SRVR */
+ {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
+ /* BSW */
+ {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
+ {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
+ {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
+ {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
+ {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
+ {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
+ {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
+ {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
+ {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
+ {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
+ {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
+};
+
+static void
+xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
+ XenHostPCIDevice *dev)
+{
+ struct PCIDevice *pci_dev;
+ int i, num;
+ uint16_t gpu_id = 0xffff, pch_id = 0xffff;
+ PCIDevice *d = &s->dev;
+
+ if (is_vga_passthrough(dev)) {
+ gpu_id = dev->device_id;
+ num = ARRAY_SIZE(xen_igd_combo_id_infos);
+ for (i = 0; i < num; i++)
+ if (gpu_id == xen_igd_combo_id_infos[i].gpu_device_id)
+ pch_id = xen_igd_combo_id_infos[i].pch_device_id;
+
+ /* Currently IGD drivers always need to access PCH by 1f.0. */
+ pci_dev = pci_create_simple(d->bus, PCI_DEVFN(0x1f, 0),
+ "xen-igd-passthrough-isa-bridge");
+
+ /*
+ * Identify PCH card with its own real vendor/device ids.
+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
+ */
+ if (pci_dev) {
+ pci_config_set_device_id(pci_dev->config, pch_id);
+ return;
+ }
+
+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge
failed!\n");
+ }
+}
+
/* init */
static int xen_pt_initfn(PCIDevice *d)
@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
return -1;
}
+ /* Register ISA bridge for passthrough GFX. */
+ xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
+
/* reinitialize each config register to be emulated */
if (xen_pt_config_init(s)) {
XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
Note I will introduce a inline function in another patch,
+static inline int is_vga_passthrough(XenHostPCIDevice *dev)
+{
+ return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
+ && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
+}
Thanks
Tiejun
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-17 8:48 ` Chen, Tiejun
@ 2014-11-17 9:25 ` Michael S. Tsirkin
-1 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 9:25 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> >On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> >>On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>>>Currently IGD drivers always need to access PCH by 1f.0, and
> >>>>PCH vendor/device id is used to identify the card.
> >>>>
> >>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>>---
> >>>> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
> >>>> 1 file changed, 27 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>>index b559181..b19c7a9 100644
> >>>>--- a/hw/i386/pc_piix.c
> >>>>+++ b/hw/i386/pc_piix.c
> >>>>@@ -50,7 +50,7 @@
> >>>> #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>
> >>>> #endif
> >>>>
> >>>> #define MAX_IDE_BUS 2
> >>>>@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
> >>>> }
> >>>>
> >>>> #ifdef CONFIG_XEN
> >>>>+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> >>>>+{
> >>>>+ struct PCIDevice *dev;
> >>>>+ Error *local_err = NULL;
> >>>>+ uint16_t device_id = 0xffff;
> >>>>+
> >>>>+ /* Currently IGD drivers always need to access PCH by 1f.0. */
> >>>>+ dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> >>>>+ "xen-igd-passthrough-isa-bridge");
> >>>>+
> >>>>+ /* Identify PCH card with its own real vendor/device ids.
> >>>>+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> >>>>+ */
> >>>>+ if (dev) {
> >>>>+ device_id = object_property_get_int(OBJECT(dev), "device-id",
> >>>>+ &local_err);
> >>>>+ if (!local_err && device_id != 0xffff) {
> >>>>+ pci_config_set_device_id(dev->config, device_id);
> >>>>+ return;
> >>>>+ }
> >>>>+ }
> >>>>+
> >>>>+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> >>>>+}
> >>>>+
> >>>> static void pc_xen_hvm_init(MachineState *machine)
> >>>> {
> >>>> PCIBus *bus;
> >>>>@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> >>>> bus = pci_find_primary_bus();
> >>>> if (bus != NULL) {
> >>>> pci_create_simple(bus, -1, "xen-platform");
> >>>>+ xen_igd_passthrough_isa_bridge_create(bus);
> >>>> }
> >>>> }
> >>>> #endif
> >>>
> >>>Can't we defer this step until the GPU is added?
> >>
> >>Sounds great but I can't figure out where we can to do this exactly.
> >>
> >>>This way there won't be need to poke at host device
> >>>directly, you could get all info from dev->config
> >>>of the host device.
> >>
> >>As I understand We have two steps here:
> >>
> >>#1 At first I have to write something to check if we're registering 00:02.0
> >>& IGD, right? But where? While registering each pci device?
> >
> >In xen_pt_initfn.
> >Just check the device and vendor ID against the table you have.
> >
>
> Okay. Please see the follows which is just compiled:
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index c6466dc..f3ea313 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -632,6 +632,94 @@ static const MemoryListener xen_pt_io_listener = {
> .priority = 10,
> };
>
> +typedef struct {
> + uint16_t gpu_device_id;
> + uint16_t pch_device_id;
> +} XenIGDDeviceIDInfo;
> +
> +/* 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 XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
> + /* HSW Classic */
> + {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
> + {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
> + {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
> + {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
> + {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
> + /* HSW ULT */
> + {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
> + {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
> + {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
> + {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
> + {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
> + {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
> + /* HSW CRW */
> + {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
> + {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
> + /* HSW Server */
> + {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
> + /* HSW SRVR */
> + {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
> + /* BSW */
> + {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
> + {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
> + {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
> + {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
> + {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
> + {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
> + {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
> + {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
> + {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
> + {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
> + {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
> +};
> +
> +static void
> +xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
> + XenHostPCIDevice *dev)
> +{
> + struct PCIDevice *pci_dev;
> + int i, num;
> + uint16_t gpu_id = 0xffff,
no need to init gpu_id.
> pch_id = 0xffff;
> + PCIDevice *d = &s->dev;
> +
> + if (is_vga_passthrough(dev)) {
I would do if (!is_vga_passthrough) { return; } here.
Makes following code shorter.
> + gpu_id = dev->device_id;
> + num = ARRAY_SIZE(xen_igd_combo_id_infos);
> + for (i = 0; i < num; i++)
> + if (gpu_id == xen_igd_combo_id_infos[i].gpu_device_id)
> + pch_id = xen_igd_combo_id_infos[i].pch_device_id;
Add {} to match our coding style please.
At this point I would do
if (pch_id == 0xffff) {
return;
}
maybe replace 0xffff with 0x0, then you can do if (!pch_id) { return; }.
> +
> + /* Currently IGD drivers always need to access PCH by 1f.0. */
> + pci_dev = pci_create_simple(d->bus, PCI_DEVFN(0x1f, 0),
> + "xen-igd-passthrough-isa-bridge");
> +
> + /*
> + * Identify PCH card with its own real vendor/device ids.
> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
s/Here/Note/
> + */
> + if (pci_dev) {
> + pci_config_set_device_id(pci_dev->config, pch_id);
> + return;
> + }
> +
> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge
> failed!\n");
Cleaner:
if (!pci_dev) {
fprintf
return;
}
pci_config_set_device_id(pci_dev->config, pch_id);
> + }
> +}
> +
> /* init */
>
> static int xen_pt_initfn(PCIDevice *d)
> @@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
> return -1;
> }
>
> + /* Register ISA bridge for passthrough GFX. */
> + xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
> +
> /* reinitialize each config register to be emulated */
> if (xen_pt_config_init(s)) {
> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
>
> Note I will introduce a inline function in another patch,
>
> +static inline int is_vga_passthrough(XenHostPCIDevice *dev)
> +{
> + return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
> + && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> +}
>
> Thanks
> Tiejun
Why bother with all these conditions?
Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
--
MST
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-17 9:25 ` Michael S. Tsirkin
0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 9:25 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> >On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> >>On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>>>Currently IGD drivers always need to access PCH by 1f.0, and
> >>>>PCH vendor/device id is used to identify the card.
> >>>>
> >>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>>---
> >>>> hw/i386/pc_piix.c | 28 +++++++++++++++++++++++++++-
> >>>> 1 file changed, 27 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>>index b559181..b19c7a9 100644
> >>>>--- a/hw/i386/pc_piix.c
> >>>>+++ b/hw/i386/pc_piix.c
> >>>>@@ -50,7 +50,7 @@
> >>>> #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>
> >>>> #endif
> >>>>
> >>>> #define MAX_IDE_BUS 2
> >>>>@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
> >>>> }
> >>>>
> >>>> #ifdef CONFIG_XEN
> >>>>+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> >>>>+{
> >>>>+ struct PCIDevice *dev;
> >>>>+ Error *local_err = NULL;
> >>>>+ uint16_t device_id = 0xffff;
> >>>>+
> >>>>+ /* Currently IGD drivers always need to access PCH by 1f.0. */
> >>>>+ dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> >>>>+ "xen-igd-passthrough-isa-bridge");
> >>>>+
> >>>>+ /* Identify PCH card with its own real vendor/device ids.
> >>>>+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> >>>>+ */
> >>>>+ if (dev) {
> >>>>+ device_id = object_property_get_int(OBJECT(dev), "device-id",
> >>>>+ &local_err);
> >>>>+ if (!local_err && device_id != 0xffff) {
> >>>>+ pci_config_set_device_id(dev->config, device_id);
> >>>>+ return;
> >>>>+ }
> >>>>+ }
> >>>>+
> >>>>+ fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> >>>>+}
> >>>>+
> >>>> static void pc_xen_hvm_init(MachineState *machine)
> >>>> {
> >>>> PCIBus *bus;
> >>>>@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> >>>> bus = pci_find_primary_bus();
> >>>> if (bus != NULL) {
> >>>> pci_create_simple(bus, -1, "xen-platform");
> >>>>+ xen_igd_passthrough_isa_bridge_create(bus);
> >>>> }
> >>>> }
> >>>> #endif
> >>>
> >>>Can't we defer this step until the GPU is added?
> >>
> >>Sounds great but I can't figure out where we can to do this exactly.
> >>
> >>>This way there won't be need to poke at host device
> >>>directly, you could get all info from dev->config
> >>>of the host device.
> >>
> >>As I understand We have two steps here:
> >>
> >>#1 At first I have to write something to check if we're registering 00:02.0
> >>& IGD, right? But where? While registering each pci device?
> >
> >In xen_pt_initfn.
> >Just check the device and vendor ID against the table you have.
> >
>
> Okay. Please see the follows which is just compiled:
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index c6466dc..f3ea313 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -632,6 +632,94 @@ static const MemoryListener xen_pt_io_listener = {
> .priority = 10,
> };
>
> +typedef struct {
> + uint16_t gpu_device_id;
> + uint16_t pch_device_id;
> +} XenIGDDeviceIDInfo;
> +
> +/* 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 XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
> + /* HSW Classic */
> + {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
> + {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
> + {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
> + {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
> + {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
> + /* HSW ULT */
> + {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
> + {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
> + {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
> + {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
> + {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
> + {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
> + /* HSW CRW */
> + {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
> + {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
> + /* HSW Server */
> + {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
> + /* HSW SRVR */
> + {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
> + /* BSW */
> + {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
> + {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
> + {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
> + {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
> + {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
> + {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
> + {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
> + {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
> + {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
> + {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
> + {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
> +};
> +
> +static void
> +xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
> + XenHostPCIDevice *dev)
> +{
> + struct PCIDevice *pci_dev;
> + int i, num;
> + uint16_t gpu_id = 0xffff,
no need to init gpu_id.
> pch_id = 0xffff;
> + PCIDevice *d = &s->dev;
> +
> + if (is_vga_passthrough(dev)) {
I would do if (!is_vga_passthrough) { return; } here.
Makes following code shorter.
> + gpu_id = dev->device_id;
> + num = ARRAY_SIZE(xen_igd_combo_id_infos);
> + for (i = 0; i < num; i++)
> + if (gpu_id == xen_igd_combo_id_infos[i].gpu_device_id)
> + pch_id = xen_igd_combo_id_infos[i].pch_device_id;
Add {} to match our coding style please.
At this point I would do
if (pch_id == 0xffff) {
return;
}
maybe replace 0xffff with 0x0, then you can do if (!pch_id) { return; }.
> +
> + /* Currently IGD drivers always need to access PCH by 1f.0. */
> + pci_dev = pci_create_simple(d->bus, PCI_DEVFN(0x1f, 0),
> + "xen-igd-passthrough-isa-bridge");
> +
> + /*
> + * Identify PCH card with its own real vendor/device ids.
> + * Here that vendor id is always PCI_VENDOR_ID_INTEL.
s/Here/Note/
> + */
> + if (pci_dev) {
> + pci_config_set_device_id(pci_dev->config, pch_id);
> + return;
> + }
> +
> + fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge
> failed!\n");
Cleaner:
if (!pci_dev) {
fprintf
return;
}
pci_config_set_device_id(pci_dev->config, pch_id);
> + }
> +}
> +
> /* init */
>
> static int xen_pt_initfn(PCIDevice *d)
> @@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
> return -1;
> }
>
> + /* Register ISA bridge for passthrough GFX. */
> + xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
> +
> /* reinitialize each config register to be emulated */
> if (xen_pt_config_init(s)) {
> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
>
> Note I will introduce a inline function in another patch,
>
> +static inline int is_vga_passthrough(XenHostPCIDevice *dev)
> +{
> + return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
> + && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> +}
>
> Thanks
> Tiejun
Why bother with all these conditions?
Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
--
MST
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-17 9:25 ` Michael S. Tsirkin
@ 2014-11-17 9:42 ` Chen, Tiejun
-1 siblings, 0 replies; 22+ messages in thread
From: Chen, Tiejun @ 2014-11-17 9:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
>> On 2014/11/17 14:10, Michael S. Tsirkin wrote:
>>> On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
>>>> On 2014/11/5 22:09, Michael S. Tsirkin wrote:
>>>>> On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
>>>>>> Currently IGD drivers always need to access PCH by 1f.0, and
>>>>>> PCH vendor/device id is used to identify the card.
>>>>>>
>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>> ---
[snip]
> Cleaner:
> if (!pci_dev) {
> fprintf
> return;
> }
> pci_config_set_device_id(pci_dev->config, pch_id);
I will address all comments and thanks.
>
>> + }
>> +}
>> +
>> /* init */
>>
>> static int xen_pt_initfn(PCIDevice *d)
>> @@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
>> return -1;
>> }
>>
>> + /* Register ISA bridge for passthrough GFX. */
>> + xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
>> +
>> /* reinitialize each config register to be emulated */
>> if (xen_pt_config_init(s)) {
>> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
>>
>> Note I will introduce a inline function in another patch,
>>
>> +static inline int is_vga_passthrough(XenHostPCIDevice *dev)
>> +{
>> + return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
>> + && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
>> +}
>>
>> Thanks
>> Tiejun
>
> Why bother with all these conditions?
> Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
>
If this is just used for IGD, its always fine without checking vendor_id.
So after remove that check, I guess I need to rename that as
is_igd_vga_passthrough() to make sense.
Thanks
Tiejun
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-17 9:42 ` Chen, Tiejun
0 siblings, 0 replies; 22+ messages in thread
From: Chen, Tiejun @ 2014-11-17 9:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
>> On 2014/11/17 14:10, Michael S. Tsirkin wrote:
>>> On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
>>>> On 2014/11/5 22:09, Michael S. Tsirkin wrote:
>>>>> On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
>>>>>> Currently IGD drivers always need to access PCH by 1f.0, and
>>>>>> PCH vendor/device id is used to identify the card.
>>>>>>
>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>> ---
[snip]
> Cleaner:
> if (!pci_dev) {
> fprintf
> return;
> }
> pci_config_set_device_id(pci_dev->config, pch_id);
I will address all comments and thanks.
>
>> + }
>> +}
>> +
>> /* init */
>>
>> static int xen_pt_initfn(PCIDevice *d)
>> @@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
>> return -1;
>> }
>>
>> + /* Register ISA bridge for passthrough GFX. */
>> + xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
>> +
>> /* reinitialize each config register to be emulated */
>> if (xen_pt_config_init(s)) {
>> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
>>
>> Note I will introduce a inline function in another patch,
>>
>> +static inline int is_vga_passthrough(XenHostPCIDevice *dev)
>> +{
>> + return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
>> + && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
>> +}
>>
>> Thanks
>> Tiejun
>
> Why bother with all these conditions?
> Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
>
If this is just used for IGD, its always fine without checking vendor_id.
So after remove that check, I guess I need to rename that as
is_igd_vga_passthrough() to make sense.
Thanks
Tiejun
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-17 9:42 ` Chen, Tiejun
@ 2014-11-17 10:13 ` Michael S. Tsirkin
-1 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 10:13 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> >On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
> >>On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> >>>On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> >>>>On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >>>>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>>>>>Currently IGD drivers always need to access PCH by 1f.0, and
> >>>>>>PCH vendor/device id is used to identify the card.
> >>>>>>
> >>>>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>>>>---
>
> [snip]
>
> >Cleaner:
> > if (!pci_dev) {
> > fprintf
> > return;
> > }
> > pci_config_set_device_id(pci_dev->config, pch_id);
>
> I will address all comments and thanks.
>
> >
> >>+ }
> >>+}
> >>+
> >> /* init */
> >>
> >> static int xen_pt_initfn(PCIDevice *d)
> >>@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
> >> return -1;
> >> }
> >>
> >>+ /* Register ISA bridge for passthrough GFX. */
> >>+ xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
> >>+
> >> /* reinitialize each config register to be emulated */
> >> if (xen_pt_config_init(s)) {
> >> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
> >>
> >>Note I will introduce a inline function in another patch,
> >>
> >>+static inline int is_vga_passthrough(XenHostPCIDevice *dev)
> >>+{
> >>+ return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
> >>+ && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> >>+}
> >>
> >>Thanks
> >>Tiejun
> >
> >Why bother with all these conditions?
> >Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
> >
>
> If this is just used for IGD, its always fine without checking vendor_id.
You need to match device ID to *know* it's IGD.
> So after remove that check, I guess I need to rename that as
> is_igd_vga_passthrough() to make sense.
>
> Thanks
> Tiejun
There is no need to check class code or xen_has_gfx_passthru flag.
Device ID + vendor ID identifies each device uniquely.
--
MST
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-17 10:13 ` Michael S. Tsirkin
0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 10:13 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> >On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
> >>On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> >>>On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> >>>>On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >>>>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>>>>>Currently IGD drivers always need to access PCH by 1f.0, and
> >>>>>>PCH vendor/device id is used to identify the card.
> >>>>>>
> >>>>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>>>>---
>
> [snip]
>
> >Cleaner:
> > if (!pci_dev) {
> > fprintf
> > return;
> > }
> > pci_config_set_device_id(pci_dev->config, pch_id);
>
> I will address all comments and thanks.
>
> >
> >>+ }
> >>+}
> >>+
> >> /* init */
> >>
> >> static int xen_pt_initfn(PCIDevice *d)
> >>@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
> >> return -1;
> >> }
> >>
> >>+ /* Register ISA bridge for passthrough GFX. */
> >>+ xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
> >>+
> >> /* reinitialize each config register to be emulated */
> >> if (xen_pt_config_init(s)) {
> >> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
> >>
> >>Note I will introduce a inline function in another patch,
> >>
> >>+static inline int is_vga_passthrough(XenHostPCIDevice *dev)
> >>+{
> >>+ return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
> >>+ && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> >>+}
> >>
> >>Thanks
> >>Tiejun
> >
> >Why bother with all these conditions?
> >Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
> >
>
> If this is just used for IGD, its always fine without checking vendor_id.
You need to match device ID to *know* it's IGD.
> So after remove that check, I guess I need to rename that as
> is_igd_vga_passthrough() to make sense.
>
> Thanks
> Tiejun
There is no need to check class code or xen_has_gfx_passthru flag.
Device ID + vendor ID identifies each device uniquely.
--
MST
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-17 10:13 ` Michael S. Tsirkin
@ 2014-11-17 11:18 ` Chen, Tiejun
-1 siblings, 0 replies; 22+ messages in thread
From: Chen, Tiejun @ 2014-11-17 11:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On 2014/11/17 18:13, Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
>> On 2014/11/17 17:25, Michael S. Tsirkin wrote:
>>> On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
>>>> On 2014/11/17 14:10, Michael S. Tsirkin wrote:
>>>>> On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
>>>>>> On 2014/11/5 22:09, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
>>>>>>>> Currently IGD drivers always need to access PCH by 1f.0, and
>>>>>>>> PCH vendor/device id is used to identify the card.
>>>>>>>>
>>>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>>>> ---
>>
>> [snip]
>>
>>> Cleaner:
>>> if (!pci_dev) {
>>> fprintf
>>> return;
>>> }
>>> pci_config_set_device_id(pci_dev->config, pch_id);
>>
>> I will address all comments and thanks.
>>
>>>
>>>> + }
>>>> +}
>>>> +
>>>> /* init */
>>>>
>>>> static int xen_pt_initfn(PCIDevice *d)
>>>> @@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
>>>> return -1;
>>>> }
>>>>
>>>> + /* Register ISA bridge for passthrough GFX. */
>>>> + xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
>>>> +
>>>> /* reinitialize each config register to be emulated */
>>>> if (xen_pt_config_init(s)) {
>>>> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
>>>>
>>>> Note I will introduce a inline function in another patch,
>>>>
>>>> +static inline int is_vga_passthrough(XenHostPCIDevice *dev)
>>>> +{
>>>> + return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
>>>> + && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
>>>> +}
>>>>
>>>> Thanks
>>>> Tiejun
>>>
>>> Why bother with all these conditions?
>>> Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
>>>
>>
>> If this is just used for IGD, its always fine without checking vendor_id.
>
> You need to match device ID to *know* it's IGD.
>
>> So after remove that check, I guess I need to rename that as
>> is_igd_vga_passthrough() to make sense.
>>
>> Thanks
>> Tiejun
>
> There is no need to check class code or xen_has_gfx_passthru flag.
> Device ID + vendor ID identifies each device uniquely.
>
Yeah.
Here I assume vendor ID is always PCI_VENDOR_ID_INTEL so looks you means
I also need to check that table to do something like,
is_igd_vga_passthugh(dev)
{
int i;
int num = ARRAY_SIZE(xen_igd_combo_id_infos);
for (i = 0; i < num; i++) {
if (dev->device_id == xen_igd_combo_id_infos[i].gpu_device_id)
return 1;
return 0;
}
Then we can simplify the subsequent codes based on this, right?
Thanks
Tiejun
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-17 11:18 ` Chen, Tiejun
0 siblings, 0 replies; 22+ messages in thread
From: Chen, Tiejun @ 2014-11-17 11:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On 2014/11/17 18:13, Michael S. Tsirkin wrote:
> On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
>> On 2014/11/17 17:25, Michael S. Tsirkin wrote:
>>> On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
>>>> On 2014/11/17 14:10, Michael S. Tsirkin wrote:
>>>>> On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
>>>>>> On 2014/11/5 22:09, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
>>>>>>>> Currently IGD drivers always need to access PCH by 1f.0, and
>>>>>>>> PCH vendor/device id is used to identify the card.
>>>>>>>>
>>>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>>>> ---
>>
>> [snip]
>>
>>> Cleaner:
>>> if (!pci_dev) {
>>> fprintf
>>> return;
>>> }
>>> pci_config_set_device_id(pci_dev->config, pch_id);
>>
>> I will address all comments and thanks.
>>
>>>
>>>> + }
>>>> +}
>>>> +
>>>> /* init */
>>>>
>>>> static int xen_pt_initfn(PCIDevice *d)
>>>> @@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
>>>> return -1;
>>>> }
>>>>
>>>> + /* Register ISA bridge for passthrough GFX. */
>>>> + xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
>>>> +
>>>> /* reinitialize each config register to be emulated */
>>>> if (xen_pt_config_init(s)) {
>>>> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
>>>>
>>>> Note I will introduce a inline function in another patch,
>>>>
>>>> +static inline int is_vga_passthrough(XenHostPCIDevice *dev)
>>>> +{
>>>> + return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
>>>> + && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
>>>> +}
>>>>
>>>> Thanks
>>>> Tiejun
>>>
>>> Why bother with all these conditions?
>>> Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
>>>
>>
>> If this is just used for IGD, its always fine without checking vendor_id.
>
> You need to match device ID to *know* it's IGD.
>
>> So after remove that check, I guess I need to rename that as
>> is_igd_vga_passthrough() to make sense.
>>
>> Thanks
>> Tiejun
>
> There is no need to check class code or xen_has_gfx_passthru flag.
> Device ID + vendor ID identifies each device uniquely.
>
Yeah.
Here I assume vendor ID is always PCI_VENDOR_ID_INTEL so looks you means
I also need to check that table to do something like,
is_igd_vga_passthugh(dev)
{
int i;
int num = ARRAY_SIZE(xen_igd_combo_id_infos);
for (i = 0; i < num; i++) {
if (dev->device_id == xen_igd_combo_id_infos[i].gpu_device_id)
return 1;
return 0;
}
Then we can simplify the subsequent codes based on this, right?
Thanks
Tiejun
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
2014-11-17 11:18 ` Chen, Tiejun
@ 2014-11-17 12:10 ` Michael S. Tsirkin
-1 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 12:10 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Mon, Nov 17, 2014 at 07:18:17PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 18:13, Michael S. Tsirkin wrote:
> >On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
> >>On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> >>>On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
> >>>>On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> >>>>>On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> >>>>>>On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >>>>>>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>>>>>>>Currently IGD drivers always need to access PCH by 1f.0, and
> >>>>>>>>PCH vendor/device id is used to identify the card.
> >>>>>>>>
> >>>>>>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>>>>>>---
> >>
> >>[snip]
> >>
> >>>Cleaner:
> >>> if (!pci_dev) {
> >>> fprintf
> >>> return;
> >>> }
> >>> pci_config_set_device_id(pci_dev->config, pch_id);
> >>
> >>I will address all comments and thanks.
> >>
> >>>
> >>>>+ }
> >>>>+}
> >>>>+
> >>>> /* init */
> >>>>
> >>>> static int xen_pt_initfn(PCIDevice *d)
> >>>>@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
> >>>> return -1;
> >>>> }
> >>>>
> >>>>+ /* Register ISA bridge for passthrough GFX. */
> >>>>+ xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
> >>>>+
> >>>> /* reinitialize each config register to be emulated */
> >>>> if (xen_pt_config_init(s)) {
> >>>> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
> >>>>
> >>>>Note I will introduce a inline function in another patch,
> >>>>
> >>>>+static inline int is_vga_passthrough(XenHostPCIDevice *dev)
> >>>>+{
> >>>>+ return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
> >>>>+ && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> >>>>+}
> >>>>
> >>>>Thanks
> >>>>Tiejun
> >>>
> >>>Why bother with all these conditions?
> >>>Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
> >>>
> >>
> >>If this is just used for IGD, its always fine without checking vendor_id.
> >
> >You need to match device ID to *know* it's IGD.
> >
> >>So after remove that check, I guess I need to rename that as
> >>is_igd_vga_passthrough() to make sense.
> >>
> >>Thanks
> >>Tiejun
> >
> >There is no need to check class code or xen_has_gfx_passthru flag.
> >Device ID + vendor ID identifies each device uniquely.
> >
>
> Yeah.
>
> Here I assume vendor ID is always PCI_VENDOR_ID_INTEL so looks you means I
> also need to check that table to do something like,
>
> is_igd_vga_passthugh(dev)
> {
> int i;
> int num = ARRAY_SIZE(xen_igd_combo_id_infos);
> for (i = 0; i < num; i++) {
> if (dev->device_id == xen_igd_combo_id_infos[i].gpu_device_id)
> return 1;
> return 0;
> }
>
> Then we can simplify the subsequent codes based on this, right?
>
> Thanks
> Tiejun
Yea.
Basically let's try to treat this simply as a device quirk,
and see where this gets us.
--
MST
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
@ 2014-11-17 12:10 ` Michael S. Tsirkin
0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 12:10 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: xen-devel, allen.m.kay, qemu-devel, aliguori, pbonzini, rth
On Mon, Nov 17, 2014 at 07:18:17PM +0800, Chen, Tiejun wrote:
> On 2014/11/17 18:13, Michael S. Tsirkin wrote:
> >On Mon, Nov 17, 2014 at 05:42:12PM +0800, Chen, Tiejun wrote:
> >>On 2014/11/17 17:25, Michael S. Tsirkin wrote:
> >>>On Mon, Nov 17, 2014 at 04:48:32PM +0800, Chen, Tiejun wrote:
> >>>>On 2014/11/17 14:10, Michael S. Tsirkin wrote:
> >>>>>On Mon, Nov 17, 2014 at 10:47:56AM +0800, Chen, Tiejun wrote:
> >>>>>>On 2014/11/5 22:09, Michael S. Tsirkin wrote:
> >>>>>>>On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
> >>>>>>>>Currently IGD drivers always need to access PCH by 1f.0, and
> >>>>>>>>PCH vendor/device id is used to identify the card.
> >>>>>>>>
> >>>>>>>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>>>>>>>---
> >>
> >>[snip]
> >>
> >>>Cleaner:
> >>> if (!pci_dev) {
> >>> fprintf
> >>> return;
> >>> }
> >>> pci_config_set_device_id(pci_dev->config, pch_id);
> >>
> >>I will address all comments and thanks.
> >>
> >>>
> >>>>+ }
> >>>>+}
> >>>>+
> >>>> /* init */
> >>>>
> >>>> static int xen_pt_initfn(PCIDevice *d)
> >>>>@@ -682,6 +770,9 @@ static int xen_pt_initfn(PCIDevice *d)
> >>>> return -1;
> >>>> }
> >>>>
> >>>>+ /* Register ISA bridge for passthrough GFX. */
> >>>>+ xen_igd_passthrough_isa_bridge_create(s, &s->real_device);
> >>>>+
> >>>> /* reinitialize each config register to be emulated */
> >>>> if (xen_pt_config_init(s)) {
> >>>> XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
> >>>>
> >>>>Note I will introduce a inline function in another patch,
> >>>>
> >>>>+static inline int is_vga_passthrough(XenHostPCIDevice *dev)
> >>>>+{
> >>>>+ return (xen_has_gfx_passthru && (dev->vendor_id == PCI_VENDOR_ID_INTEL)
> >>>>+ && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> >>>>+}
> >>>>
> >>>>Thanks
> >>>>Tiejun
> >>>
> >>>Why bother with all these conditions?
> >>>Won't it be enough to check dev->vendor_id == PCI_VENDOR_ID_INTEL?
> >>>
> >>
> >>If this is just used for IGD, its always fine without checking vendor_id.
> >
> >You need to match device ID to *know* it's IGD.
> >
> >>So after remove that check, I guess I need to rename that as
> >>is_igd_vga_passthrough() to make sense.
> >>
> >>Thanks
> >>Tiejun
> >
> >There is no need to check class code or xen_has_gfx_passthru flag.
> >Device ID + vendor ID identifies each device uniquely.
> >
>
> Yeah.
>
> Here I assume vendor ID is always PCI_VENDOR_ID_INTEL so looks you means I
> also need to check that table to do something like,
>
> is_igd_vga_passthugh(dev)
> {
> int i;
> int num = ARRAY_SIZE(xen_igd_combo_id_infos);
> for (i = 0; i < num; i++) {
> if (dev->device_id == xen_igd_combo_id_infos[i].gpu_device_id)
> return 1;
> return 0;
> }
>
> Then we can simplify the subsequent codes based on this, right?
>
> Thanks
> Tiejun
Yea.
Basically let's try to treat this simply as a device quirk,
and see where this gets us.
--
MST
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2014-11-17 12:10 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-05 7:22 [Qemu-devel] [RFC][PATCH 1/2] hw:xen:xen_pt: register isa bridge specific to IGD passthrough Tiejun Chen
2014-11-05 7:22 ` Tiejun Chen
2014-11-05 7:22 ` [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create " Tiejun Chen
2014-11-05 7:22 ` Tiejun Chen
2014-11-05 14:09 ` [Qemu-devel] " Michael S. Tsirkin
2014-11-05 14:09 ` Michael S. Tsirkin
2014-11-17 2:47 ` [Qemu-devel] " Chen, Tiejun
2014-11-17 2:47 ` Chen, Tiejun
2014-11-17 6:10 ` [Qemu-devel] " Michael S. Tsirkin
2014-11-17 6:10 ` Michael S. Tsirkin
2014-11-17 8:48 ` [Qemu-devel] " Chen, Tiejun
2014-11-17 8:48 ` Chen, Tiejun
2014-11-17 9:25 ` [Qemu-devel] " Michael S. Tsirkin
2014-11-17 9:25 ` Michael S. Tsirkin
2014-11-17 9:42 ` [Qemu-devel] " Chen, Tiejun
2014-11-17 9:42 ` Chen, Tiejun
2014-11-17 10:13 ` [Qemu-devel] " Michael S. Tsirkin
2014-11-17 10:13 ` Michael S. Tsirkin
2014-11-17 11:18 ` [Qemu-devel] " Chen, Tiejun
2014-11-17 11:18 ` Chen, Tiejun
2014-11-17 12:10 ` [Qemu-devel] " Michael S. Tsirkin
2014-11-17 12:10 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.