xen: add Intel IGD passthrough support

Sometime we may need to reserve some specific devfn since some vgabios or drivers have to work well with a fixed bdf.

Signed-off-by Tiejun Chen <[email protected]>

---
v2:

* New patch

 hw/pci/pci.c         | 11 +++++++++++
 include/hw/pci/pci.h |  1 +
 2 files changed, 12 insertions(+)

hw/pci/pci.c +0 -0

index ef944cf..06b0c92 100644

hw/pci/pci.c +1 -0

+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn)

include/hw/pci/pci.h +1 -0

index 8c25ae5..cbb174e 100644
@@ -342,6 +342,7 @@ void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
                          MemoryRegion *address_space_mem,
                          MemoryRegion *address_space_io,
                          uint8_t devfn_min, const char *typename);
+void pci_reserve_pci_devfn(PCIBus *bus, uint8_t devfn);
 PCIBus *pci_bus_new(DeviceState *parent, const char *name,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,

Signature · 7 lines

1.9.1


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

-----Original Message-----
From: Konrad Rzeszutek Wilk [mailto:[email protected]]
Sent: Friday, May 16, 2014 10:07 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Xen-devel] [v2][PATCH 2/8] pci: provide a way to reserve some specific devfn

hw/pci/pci.c static void do_pci_unregister_device(PCIDevice

Quoted 17 lines · Tiejun Chen

Sometime we may need to reserve some specific devfn since some vgabios or drivers have to work well with a fixed bdf.

Signed-off-by Tiejun Chen <[email protected]>

---
v2:

* New patch

 hw/pci/pci.c         | 11 +++++++++++
 include/hw/pci/pci.h |  1 +
 2 files changed, 12 insertions(+)

hw/pci/pci.c +0 -0

@@ -813,6 +813,17 @@ static void do_pci_unregister_device(PCIDevice

*pci_dev)

hw/pci/pci.c static void do_pci_unregister_device(PCIDevice *pci_dev) · line 819

Quoted 6 lines

hw/pci/pci.c +3 -0

     memory_region_destroy(&pci_dev->bus_master_enable_region);
 }
 
+
+void pci_reserve_pci_devfn(PCIBus *bus, uint8_t devfn) {
+    int ret = test_and_set_bit(devfn, bus->pci_bdf_bitmap);

unsigned int please.

hw/pci/pci.c static void do_pci_unregister_device(PCIDevice *pci_dev)

Quoted 3 lines

hw/pci/pci.c +3 -0

+    if (ret) {
+        error_report("PCI: %02x:%02x:%02x is not available to reserve.",
+                    pci_bus_num(bus), PCI_SLOT(devfn),

PCI_FUNC(devfn));

hw/pci/pci.c static void do_pci_unregister_device(PCIDevice *pci_dev) · line 825

Quoted 3 lines

hw/pci/pci.c +3 -0

+        return;
+    }
+}

Don't we want to return whether we are failed or not?

basic gfx passthrough support:
- add a vga type for gfx passthrough
- retrieve VGA bios from sysfs, then load it to guest 0xC0000
- register/unregister legacy VGA I/O ports and MMIOs for passthroughed gfx

The original patch is from Weidong Han <[email protected]>

Signed-off-by Yang Zhang <[email protected]>

Signed-off-by Tiejun Chen <[email protected]>

Cc Weidong Han <[email protected]>

---
v2:

* retrieve VGA bios from sysfs properly.
* redefine some function name.

 hw/xen/Makefile.objs         |   2 +-
 hw/xen/xen-host-pci-device.c |   5 ++
 hw/xen/xen-host-pci-device.h |   1 +
 hw/xen/xen_pt.c              |  10 +++
 hw/xen/xen_pt.h              |   4 +
 hw/xen/xen_pt_graphics.c     | 177 +++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx              |   9 +++
 vl.c                         |  11 ++-
 8 files changed, 217 insertions(+), 2 deletions(-)
 create mode 100644 hw/xen/xen_pt_graphics.c

hw/xen/Makefile.objs +1 -1

index a0ca0aa..77b7dae 100644
@@ -2,4 +2,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

hw/xen/xen-host-pci-device.c +5 -0

index 743b37b..a54b7de 100644
@@ -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;

hw/xen/xen-host-pci-device.h +1 -0

index c2486f0..f1e1c30 100644
@@ -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];

hw/xen/xen_pt.c +10 -0

index be4220b..a0113ea 100644
@@ -450,6 +450,7 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s)
                    d->rom.size, d->rom.base_addr);
     }
 
+    xen_pt_register_vga_regions(d);
     return 0;
 }
 

@@ -470,6 +471,8 @@ static void xen_pt_unregister_regions(XenPCIPassthroughState *s)
     if (d->rom.base_addr && d->rom.size) {
         memory_region_destroy(&s->rom);
     }
+
+    xen_pt_unregister_vga_regions(d);
 }
 
 /* region mapping */

@@ -693,6 +696,13 @@ static int xen_pt_initfn(PCIDevice *d)
     /* Handle real device's MMIO/PIO BARs */
     xen_pt_register_regions(s);
 
+    /* Setup VGA bios for passthroughed gfx */
+    if (xen_pt_setup_vga(&s->real_device) < 0) {
+        XEN_PT_ERR(d, "Setup VGA BIOS of passthroughed gfx failed!\n");
+        xen_host_pci_device_put(&s->real_device);
+        return -1;
+    }
+
     /* reinitialize each config register to be emulated */
     if (xen_pt_config_init(s)) {
         XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");

hw/xen/xen_pt.h +4 -0

index 942dc60..4d3a18d 100644
@@ -298,5 +298,9 @@ static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
     return s->msix && s->msix->bar_index == bar;
 }
 
+extern int xen_has_gfx_passthru;
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);
+int xen_pt_setup_vga(XenHostPCIDevice *dev);
 
 #endif /* !XEN_PT_H */

hw/xen/xen_pt_graphics.c new file +28 -0

new file mode 100644
index 0000000..e1f0724
@@ -0,0 +1,177 @@
+/*
+ * graphics passthrough
+ */
+#include "xen_pt.h"
+#include "xen-host-pci-device.h"
+#include "hw/xen/xen_backend.h"
+
+static int is_vga_passthrough(XenHostPCIDevice *dev)
+{
+    return (xen_has_gfx_passthru
+            && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
+}
+
+/*
+ * register VGA resources for the domain with assigned gfx
+ */
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
+{
+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xA, DPCI_ADD_MAPPING);
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,

hw/xen/xen_pt_graphics.c new file +22 -0

+    }
+
+    return ret;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx
+ */
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
+{
+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xC, DPCI_REMOVE_MAPPING);
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
+            0x3C0, 0x20, DPCI_REMOVE_MAPPING);
+

hw/xen/xen_pt_graphics.c new file +5 -0

+        return -1;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        goto close_rom;

hw/xen/xen_pt_graphics.c new file +9 -0

+
+    if (magic != 0xAA55) {
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    if (!fread(buf, 1, st.st_size, fp)) {
+        XEN_PT_ERR(NULL, "pci-assign: Cannot read from host %s", rom_file);
+        XEN_PT_LOG(NULL, "Device option ROM contents are probably invalid "

hw/xen/xen_pt_graphics.c new file +12 -0

+}
+
+/* Allocated 128K for the vga bios */
+#define VGA_BIOS_DEFAULT_SIZE (0x20000)
+
+int xen_pt_setup_vga(XenHostPCIDevice *dev)
+{
+    unsigned char *bios = NULL;
+    int bios_size = 0;
+    char *c = NULL;
+    char checksum = 0;
+    int rc = 0;

hw/xen/xen_pt_graphics.c new file +19 -0

+        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;
+        XEN_PT_LOG(NULL, "vga bios checksum is adjusted!\n");
+    }
+
+    /* Currently we fixed this address as a primary. */
+    cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
+out:
+    g_free(bios);
+    return rc;
+}

qemu-options.hx +9 -0

index 781af14..cece134 100644
@@ -1047,6 +1047,15 @@ STEXI
 Rotate graphical output some deg left (only PXA LCD).
 ETEXI
 
+DEF("gfx_passthru", 0, QEMU_OPTION_gfx_passthru,
+    "-gfx_passthru   enable Intel IGD passthrough by XEN\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -gfx_passthru
+@findex -gfx_passthru
+Enable Intel IGD passthrough by XEN
+ETEXI
+
 DEF("vga", HAS_ARG, QEMU_OPTION_vga,
     "-vga [std|cirrus|vmware|qxl|xenfb|tcx|cg3|none]\n"
     "                select video card type\n", QEMU_ARCH_ALL)

vl.c +10 -1

index 73e0661..c86e95f 100644
@@ -1436,6 +1436,13 @@ static void smp_parse(QemuOpts *opts)
 
 }
 
+/* We still need this for compatibility with XEN. */
+int xen_has_gfx_passthru;
+static void xen_gfx_passthru(const char *optarg)
+{
+    xen_has_gfx_passthru = 1;
+}
+
 static void configure_realtime(QemuOpts *opts)
 {
     bool enable_mlock;

@@ -2988,7 +2995,6 @@ int main(int argc, char **argv, char **envp)
     const char *trace_file = NULL;
     const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
                                         1024 * 1024;
-
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
     qemu_init_exec_dir(argv[0]);

@@ -3956,6 +3962,9 @@ int main(int argc, char **argv, char **envp)
                 }
                 configure_msg(opts);
                 break;
+            case QEMU_OPTION_gfx_passthru:
+                xen_gfx_passthru(optarg);
+                break;
             default:
                 os_parse_cmd_args(popt->index, optarg);
             }

Signature · 7 lines

1.9.1


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

-----Original Message-----
From: Konrad Rzeszutek Wilk [mailto:[email protected]]
Sent: Friday, May 16, 2014 10:06 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support

Quoted 13 lines

(quoted diff) +13 -0

+/*
+ * register VGA resources for the domain with assigned gfx  */ int
+xen_pt_register_vga_regions(XenHostPCIDevice *dev) {
+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xA, DPCI_ADD_MAPPING);
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,

Review

Quoted 10 lines

(quoted diff) +10 -0

+            0x3C0, 0x20, DPCI_ADD_MAPPING);
+
+    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0x20,
+            DPCI_ADD_MAPPING);
+
+    if (ret) {
+        XEN_PT_ERR(NULL, "VGA region mapping failed\n");

It would be actually useful to know _which_ of them failed. Perhaps you could structure this a bit differently and do:

struct _args {
        uint32_t gport;
        uint32_t mport;
        uint32_t nport;
};

        struct _args args[3] = {{ 0x3B0, 0x3B0, 0xA }, {...}};
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(args); i++) {
		ret = xc_domain_ioport_mapping(xen_xc, xen_domid, args[i].gport,
args[i]..)
		if (ret) {
			XEN_PT_ERR_(NULL, "VGA region mapping of 0x%lx for 0x%x
pages failed with rc:%d\n",
					... fill in the values)
			return ret;
	}

Quoted 25 lines

(quoted diff) +24 -0

+    }
+
+    return ret;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx  */ int
+xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) {
+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xC, DPCI_REMOVE_MAPPING);
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
+            0x3C0, 0x20, DPCI_REMOVE_MAPPING);
+
+    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0xa0000 >> XC_PAGE_SHIFT,
+            20,

Review

Quoted 6 lines

(quoted diff) +6 -0

+            DPCI_REMOVE_MAPPING);
+
+    if (ret) {
+        XEN_PT_ERR(NULL, "VGA region unmapping failed\n");
+    }
+

The same pattern as above.

Quoted 8 lines

(quoted diff) +8 -0

+    return ret;
+}
+
+static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev) {
+    char rom_file[64];
+    FILE *fp;
+    uint8_t val;
+    struct stat st;

Review

Quoted 4 lines

(quoted diff) +4 -0

+    uint16_t magic = 0;
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",

I think the format changed to be: /%04x:%02x:%02x.%d in Linux (see pci_setup_device in drivers/pci/probe.c) - not that it makes that much difference as the function is only up to 7.

Review

Quoted 6 lines

(quoted diff) +5 -0

+             dev->domain, dev->bus, dev->dev,
+             dev->func);
+
+    if (stat(rom_file, &st)) {
+        return -1;

ENODEV?

Review

Quoted 7 lines

(quoted diff) +6 -0

+    }
+
+    if (access(rom_file, F_OK)) {
+        XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
+                    rom_file);
+        return -1;

EPERM?

Review

Quoted 5 lines

(quoted diff) +5 -0

+    }
+
+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {

EACCESS ?

Quoted 5 lines

(quoted diff) +5 -0

+        return -1;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        goto close_rom;

Review

Quoted 10 lines

(quoted diff) +10 -0

+    }
+    fseek(fp, 0, SEEK_SET);
+
+    /*
+     * Check if it a real bios extension.
+     * The magic number is 0xAA55.
+     */
+    if (fread(&magic, sizeof(magic), 1, fp)) {
+        goto close_rom;
+    }

Don't you want to do that before you write '1' in it?

Review

Quoted 8 lines

(quoted diff) +8 -0

+
+    if (magic != 0xAA55) {
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    if (!fread(buf, 1, st.st_size, fp)) {
+        XEN_PT_ERR(NULL, "pci-assign: Cannot read from host %s",

rom_file);

Quoted 1 line

+ XEN_PT_LOG(NULL, "Device option ROM contents are probably

invalid "

Quoted 1 line

+ "(check dmesg).\nSkip option ROM probe with

rombar=0, "

Review

Quoted 9 lines

(quoted diff) +9 -0

+                     "or load from file with romfile=\n");
+    }
+
+close_rom:
+    /* Write "0" to disable ROM */
+    fseek(fp, 0, SEEK_SET);
+    val = 0;
+    if (!fwrite(&val, 1, 1, fp)) {
+        XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file");

Should we return -1? (after closing the file of course)

Review

Quoted 3 lines

(quoted diff) +3 -0

+    }
+    fclose(fp);
+    return st.st_size;

Ah, that is why your return -1! In that case I presume the caller of this function will check the 'errno' to find the underlaying issue

Quoted 12 lines

-----Original Message-----
From: Konrad Rzeszutek Wilk [mailto:[email protected]]
Sent: Friday, May 16, 2014 10:06 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support

[snip]

Quoted 34 lines

Quoted 13 lines

(quoted diff) +13 -0

+/*
+ * register VGA resources for the domain with assigned gfx  */ int
+xen_pt_register_vga_regions(XenHostPCIDevice *dev) {
+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xA, DPCI_ADD_MAPPING);
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,

Review

Quoted 10 lines

(quoted diff) +10 -0

+            0x3C0, 0x20, DPCI_ADD_MAPPING);
+
+    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0x20,
+            DPCI_ADD_MAPPING);
+
+    if (ret) {
+        XEN_PT_ERR(NULL, "VGA region mapping failed\n");

It would be actually useful to know _which_ of them failed. Perhaps you could structure this a bit differently and do:

struct _args {
        uint32_t gport;
        uint32_t mport;
        uint32_t nport;
};

Quoted 13 lines

        struct _args args[3] = {{ 0x3B0, 0x3B0, 0xA }, {...}};
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(args); i++) {
		ret = xc_domain_ioport_mapping(xen_xc, xen_domid, args[i].gport,
args[i]..)
		if (ret) {
			XEN_PT_ERR_(NULL, "VGA region mapping of 0x%lx for 0x%x
pages failed with rc:%d\n",
					... fill in the values)
			return ret;
	}

Looks good so what about this based on the original,

hw/xen/xen_pt_graphics.c +69 -30

@@ -14,34 +14,73 @@ static int is_vga_passthrough(XenHostPCIDevice *dev)
             && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
 }
 
+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 ret = 0;
+    int i = 0;
 
     if (!is_vga_passthrough(dev)) {
-        return ret;
+        return -1;
     }
 
-    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
-            0x3B0, 0xA, DPCI_ADD_MAPPING);
-
-    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
-            0x3C0, 0x20, DPCI_ADD_MAPPING);
-
-    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
-            0xa0000 >> XC_PAGE_SHIFT,
-            0xa0000 >> XC_PAGE_SHIFT,
-            0x20,
-            DPCI_ADD_MAPPING);
+    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 (ret) {
-        XEN_PT_ERR(NULL, "VGA region mapping failed\n");
+        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 ret;
+    return 0;
 }
 
 /*

@@ -49,29 +88,33 @@ int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
  */
 int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
 {
-    int ret = 0;
+    int i = 0;
 
     if (!is_vga_passthrough(dev)) {
-        return ret;
+        return -1;
     }
 
-    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
-            0x3B0, 0xC, DPCI_REMOVE_MAPPING);
-
-    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
-            0x3C0, 0x20, DPCI_REMOVE_MAPPING);
-
-    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
-            0xa0000 >> XC_PAGE_SHIFT,
-            0xa0000 >> XC_PAGE_SHIFT,
-            20,
-            DPCI_REMOVE_MAPPING);
+    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 (ret) {

hw/xen/xen_pt_graphics.c +0 -0

 }
 
 static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev)

Quoted 27 lines

Quoted 27 lines

(quoted diff) +24 -0

+    }
+
+    return ret;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx  */ int
+xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) {
+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xC, DPCI_REMOVE_MAPPING);
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
+            0x3C0, 0x20, DPCI_REMOVE_MAPPING);
+
+    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0xa0000 >> XC_PAGE_SHIFT,
+            20,

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+            DPCI_REMOVE_MAPPING);
+
+    if (ret) {
+        XEN_PT_ERR(NULL, "VGA region unmapping failed\n");
+    }
+

The same pattern as above.

See the above.

Quoted 1 line

Quoted 1 line

+ return ret;

Quoted 7 lines

Quoted 7 lines

(quoted diff) +7 -0

+}
+
+static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev) {
+    char rom_file[64];
+    FILE *fp;
+    uint8_t val;
+    struct stat st;

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+    uint16_t magic = 0;
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",

I think the format changed to be: /%04x:%02x:%02x.%d in Linux (see pci_setup_device in drivers/pci/probe.c) - not that it makes that much difference as the function is only up to 7.

Will improved this as you pointed.

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +5 -0

+             dev->domain, dev->bus, dev->dev,
+             dev->func);
+
+    if (stat(rom_file, &st)) {
+        return -1;

ENODEV?

Fixed.

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +6 -0

+    }
+
+    if (access(rom_file, F_OK)) {
+        XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
+                    rom_file);
+        return -1;

EPERM?

Fixed.

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+    }
+
+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {

EACCESS ?

Fixed.

Quoted 16 lines

Quoted 5 lines

(quoted diff) +5 -0

+        return -1;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        goto close_rom;

Review

Quoted 10 lines

(quoted diff) +10 -0

+    }
+    fseek(fp, 0, SEEK_SET);
+
+    /*
+     * Check if it a real bios extension.
+     * The magic number is 0xAA55.
+     */
+    if (fread(&magic, sizeof(magic), 1, fp)) {
+        goto close_rom;
+    }

Quoted 2 lines

Don't you want to do that before you write '1' in it?

Definitely, but I really did this above this line :)

Quoted 13 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+
+    if (magic != 0xAA55) {
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    if (!fread(buf, 1, st.st_size, fp)) {
+        XEN_PT_ERR(NULL, "pci-assign: Cannot read from host %s",

rom_file);

Quoted 1 line

+ XEN_PT_LOG(NULL, "Device option ROM contents are probably

invalid "

Quoted 1 line

+ "(check dmesg).\nSkip option ROM probe with

rombar=0, "

Quoted 12 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+                     "or load from file with romfile=\n");
+    }
+
+close_rom:
+    /* Write "0" to disable ROM */
+    fseek(fp, 0, SEEK_SET);
+    val = 0;
+    if (!fwrite(&val, 1, 1, fp)) {
+        XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file");

Should we return -1? (after closing the file of course)

Fixed.

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+    }
+    fclose(fp);
+    return st.st_size;

Ah, that is why your return -1! In that case I presume the caller of this function will check the 'errno' to find the underlaying issue

I will modify something here and xen_pt_setup_vga(). Please see next version.

Thanks
Tiejun

-----Original Message-----
From: Chen, Tiejun
Sent: Tuesday, May 20, 2014 9:30 AM
To: 'Konrad Rzeszutek Wilk'
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z
Subject: RE: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support

Quoted 11 lines

-----Original Message-----
From: Konrad Rzeszutek Wilk [mailto:[email protected]]
Sent: Monday, May 19, 2014 9:36 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support

[snip]

Quoted 57 lines

hw/xen/xen_pt_graphics.c static int is_vga_passthrough(XenHostPCIDevice

Quoted 5 lines

Looks good so what about this based on the original,

hw/xen/xen_pt_graphics.c +0 -0

@@ -14,34 +14,73 @@ static int is_vga_passthrough(XenHostPCIDevice

*dev)

hw/xen/xen_pt_graphics.c static int is_vga_passthrough(XenHostPCIDevice · line 14

Quoted 1 line

hw/xen/xen_pt_graphics.c +0 -0

             && ((dev->class_code >> 0x8) ==

PCI_CLASS_DISPLAY_VGA));

Quoted 49 lines

hw/xen/xen_pt_graphics.c +37 -2

 }
 
+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 ret = 0;
+    int i = 0;
 
     if (!is_vga_passthrough(dev)) {
-        return ret;
+        return -1;
     }
 

Quoted 18 lines

Quoted 4 lines

hw/xen/xen_pt_graphics.c +0 -4

-    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
-            0x3B0, 0xA, DPCI_ADD_MAPPING);
-
-    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,

hw/xen/xen_pt_graphics.c static int is_vga_passthrough(XenHostPCIDevice *dev)

Quoted 10 lines

hw/xen/xen_pt_graphics.c +3 -7

-            0x3C0, 0x20, DPCI_ADD_MAPPING);
-
-    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
-            0xa0000 >> XC_PAGE_SHIFT,
-            0xa0000 >> XC_PAGE_SHIFT,
-            0x20,
-            DPCI_ADD_MAPPING);
+    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,

Quoted 3 lines

hw/xen/xen_pt_graphics.c +3 -0

+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size,

DPCI_ADD_MAPPING);

Quoted 6 lines

hw/xen/xen_pt_graphics.c static int is_vga_passthrough(XenHostPCIDevice · line 69

Quoted 2 lines

hw/xen/xen_pt_graphics.c +2 -0

+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc,

xen_domid,

Quoted 3 lines

hw/xen/xen_pt_graphics.c +3 -0

+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size,

DPCI_ADD_MAPPING);

Quoted 12 lines

hw/xen/xen_pt_graphics.c static int is_vga_passthrough(XenHostPCIDevice *dev)

Quoted 7 lines

hw/xen/xen_pt_graphics.c +4 -2

+        }
 
-    if (ret) {
-        XEN_PT_ERR(NULL, "VGA region mapping failed\n");
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s mapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" :

"memory",

Quoted 4 lines

hw/xen/xen_pt_graphics.c +2 -0

+                    vga_args[i].rc);
+        }
     }
 

Quoted 6 lines

hw/xen/xen_pt_graphics.c int

Quoted 6 lines

hw/xen/xen_pt_graphics.c +1 -1

-    return ret;
+    return 0;
 }
 
 /*

@@ -49,29 +88,33 @@ int

xen_pt_register_vga_regions(XenHostPCIDevice

Quoted 45 lines

*dev)

Quoted 14 lines

hw/xen/xen_pt_graphics.c +2 -6

  */
 int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)  {
-    int ret = 0;
+    int i = 0;
 
     if (!is_vga_passthrough(dev)) {
-        return ret;
+        return -1;
     }
 
-    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
-            0x3B0, 0xC, DPCI_REMOVE_MAPPING);
-
-    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,

hw/xen/xen_pt_graphics.c int xen_pt_register_vga_regions(XenHostPCIDevice *dev)

Quoted 10 lines

hw/xen/xen_pt_graphics.c +3 -7

-            0x3C0, 0x20, DPCI_REMOVE_MAPPING);
-
-    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
-            0xa0000 >> XC_PAGE_SHIFT,
-            0xa0000 >> XC_PAGE_SHIFT,
-            20,
-            DPCI_REMOVE_MAPPING);
+    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,

hw/xen/xen_pt_graphics.c static int is_vga_passthrough(XenHostPCIDevice *dev)

Quoted 3 lines

hw/xen/xen_pt_graphics.c +3 -0

+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size,

DPCI_REMOVE_MAPPING);

hw/xen/xen_pt_graphics.c int · line 103

Quoted 2 lines

hw/xen/xen_pt_graphics.c +2 -0

+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc,

xen_domid,

hw/xen/xen_pt_graphics.c static int is_vga_passthrough(XenHostPCIDevice *dev)

Quoted 3 lines

hw/xen/xen_pt_graphics.c +3 -0

+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size,

DPCI_REMOVE_MAPPING);

hw/xen/xen_pt_graphics.c int xen_pt_register_vga_regions(XenHostPCIDevice *dev)

Quoted 7 lines

hw/xen/xen_pt_graphics.c +4 -2

+        }
 
-    if (ret) {
-        XEN_PT_ERR(NULL, "VGA region unmapping failed\n");
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s unmapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" :

"memory",

Quoted 9 lines

hw/xen/xen_pt_graphics.c static int is_vga_passthrough(XenHostPCIDevice *dev) · line 83

Quoted 6 lines

hw/xen/xen_pt_graphics.c +3 -1

+                    vga_args[i].rc);
+        }
     }
 
-    return ret;
+    return 0;

I think you still need to return a non-zero value in case of failure.

Okay I will do this like,

hw/xen/xen_pt_graphics.c index renamed from hw/xen/xen_pt_graphics.c +0 -0

5603a8e..1342f4f 100644

hw/xen/xen_pt_graphics.c +1 -0

@@ -77,6 +77,7 @@ int xen_pt_register_vga_regions(XenHostPCIDevice
 *dev)
             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;
         }

     }

hw/xen/xen_pt_graphics.c +1 -0

@@ -111,6 +112,7 @@ int xen_pt_unregister_vga_regions(XenHostPCIDevice
 *dev)
             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;
         }

     }

Thanks
Tiejun

Quoted 138 lines · Tiejun Chen

basic gfx passthrough support:
- add a vga type for gfx passthrough
- retrieve VGA bios from sysfs, then load it to guest 0xC0000
- register/unregister legacy VGA I/O ports and MMIOs for passthroughed gfx

The original patch is from Weidong Han <[email protected]>

Signed-off-by Yang Zhang <[email protected]>

Signed-off-by Tiejun Chen <[email protected]>

Cc Weidong Han <[email protected]>

---
v2:

* retrieve VGA bios from sysfs properly.
* redefine some function name.

 hw/xen/Makefile.objs         |   2 +-
 hw/xen/xen-host-pci-device.c |   5 ++
 hw/xen/xen-host-pci-device.h |   1 +
 hw/xen/xen_pt.c              |  10 +++
 hw/xen/xen_pt.h              |   4 +
 hw/xen/xen_pt_graphics.c     | 177 +++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx              |   9 +++
 vl.c                         |  11 ++-
 8 files changed, 217 insertions(+), 2 deletions(-)
 create mode 100644 hw/xen/xen_pt_graphics.c

hw/xen/Makefile.objs +1 -1

index a0ca0aa..77b7dae 100644
@@ -2,4 +2,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

hw/xen/xen-host-pci-device.c +5 -0

index 743b37b..a54b7de 100644
@@ -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;

hw/xen/xen-host-pci-device.h +1 -0

index c2486f0..f1e1c30 100644
@@ -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];

hw/xen/xen_pt.c +10 -0

index be4220b..a0113ea 100644
@@ -450,6 +450,7 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s)
                    d->rom.size, d->rom.base_addr);
     }
 
+    xen_pt_register_vga_regions(d);
     return 0;
 }
 

@@ -470,6 +471,8 @@ static void xen_pt_unregister_regions(XenPCIPassthroughState *s)
     if (d->rom.base_addr && d->rom.size) {
         memory_region_destroy(&s->rom);
     }
+
+    xen_pt_unregister_vga_regions(d);
 }
 
 /* region mapping */

@@ -693,6 +696,13 @@ static int xen_pt_initfn(PCIDevice *d)
     /* Handle real device's MMIO/PIO BARs */
     xen_pt_register_regions(s);
 
+    /* Setup VGA bios for passthroughed gfx */
+    if (xen_pt_setup_vga(&s->real_device) < 0) {
+        XEN_PT_ERR(d, "Setup VGA BIOS of passthroughed gfx failed!\n");
+        xen_host_pci_device_put(&s->real_device);
+        return -1;
+    }
+
     /* reinitialize each config register to be emulated */
     if (xen_pt_config_init(s)) {
         XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");

hw/xen/xen_pt.h +4 -0

index 942dc60..4d3a18d 100644
@@ -298,5 +298,9 @@ static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
     return s->msix && s->msix->bar_index == bar;
 }
 
+extern int xen_has_gfx_passthru;
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);
+int xen_pt_setup_vga(XenHostPCIDevice *dev);
 
 #endif /* !XEN_PT_H */

hw/xen/xen_pt_graphics.c new file +18 -0

new file mode 100644
index 0000000..e1f0724
@@ -0,0 +1,177 @@
+/*
+ * graphics passthrough
+ */
+#include "xen_pt.h"
+#include "xen-host-pci-device.h"
+#include "hw/xen/xen_backend.h"
+
+static int is_vga_passthrough(XenHostPCIDevice *dev)
+{
+    return (xen_has_gfx_passthru
+            && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
+}
+
+/*
+ * register VGA resources for the domain with assigned gfx
+ */
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
+{

hw/xen/xen_pt_graphics.c line 26

Quoted 8 lines

hw/xen/xen_pt_graphics.c new file +8 -0

+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xA, DPCI_ADD_MAPPING);

hw/xen/xen_pt_graphics.c line 38

Quoted 11 lines

hw/xen/xen_pt_graphics.c new file +11 -0

+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
+            0x3C0, 0x20, DPCI_ADD_MAPPING);
+
+    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0x20,
+            DPCI_ADD_MAPPING);
+
+    if (ret) {
+        XEN_PT_ERR(NULL, "VGA region mapping failed\n");

It would be actually useful to know _which_ of them failed. Perhaps you could structure this a bit differently and do:

struct _args {
        uint32_t gport;
        uint32_t mport;
        uint32_t nport;
};

        struct _args args[3] = {{ 0x3B0, 0x3B0, 0xA }, {...}};
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(args); i++) {
		ret = xc_domain_ioport_mapping(xen_xc, xen_domid, args[i].gport, args[i]..)
		if (ret) {
			XEN_PT_ERR_(NULL, "VGA region mapping of 0x%lx for 0x%x pages failed with rc:%d\n",
					... fill in the values)
			return ret;
	}	

Quoted 10 lines

hw/xen/xen_pt_graphics.c new file +10 -0

+    }
+
+    return ret;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx
+ */
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
+{

hw/xen/xen_pt_graphics.c line 56

Quoted 8 lines

hw/xen/xen_pt_graphics.c new file +8 -0

+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xC, DPCI_REMOVE_MAPPING);

hw/xen/xen_pt_graphics.c line 70

Quoted 13 lines

hw/xen/xen_pt_graphics.c new file +13 -0

+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
+            0x3C0, 0x20, DPCI_REMOVE_MAPPING);
+
+    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0xa0000 >> XC_PAGE_SHIFT,
+            20,
+            DPCI_REMOVE_MAPPING);
+
+    if (ret) {
+        XEN_PT_ERR(NULL, "VGA region unmapping failed\n");
+    }
+

The same pattern as above.

hw/xen/xen_pt_graphics.c line 83

Quoted 13 lines

hw/xen/xen_pt_graphics.c new file +13 -0

+    return ret;
+}
+
+static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev)
+{
+    char rom_file[64];
+    FILE *fp;
+    uint8_t val;
+    struct stat st;
+    uint16_t magic = 0;
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",

I think the format changed to be: /%04x:%02x:%02x.%d in Linux (see pci_setup_device in drivers/pci/probe.c) - not that it makes that much difference as the function is only up to 7.

hw/xen/xen_pt_graphics.c line 88

Quoted 5 lines

hw/xen/xen_pt_graphics.c new file +5 -0

+             dev->domain, dev->bus, dev->dev,
+             dev->func);
+
+    if (stat(rom_file, &st)) {
+        return -1;

ENODEV?

hw/xen/xen_pt_graphics.c line 94

Quoted 6 lines

hw/xen/xen_pt_graphics.c new file +6 -0

+    }
+
+    if (access(rom_file, F_OK)) {
+        XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
+                    rom_file);
+        return -1;

EPERM?

hw/xen/xen_pt_graphics.c line 99

Quoted 5 lines

hw/xen/xen_pt_graphics.c new file +5 -0

+    }
+
+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {

EACCESS ?

Quoted 5 lines

hw/xen/xen_pt_graphics.c new file +5 -0

+        return -1;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        goto close_rom;

hw/xen/xen_pt_graphics.c line 114

Quoted 10 lines

hw/xen/xen_pt_graphics.c new file +10 -0

+    }
+    fseek(fp, 0, SEEK_SET);
+
+    /*
+     * Check if it a real bios extension.
+     * The magic number is 0xAA55.
+     */
+    if (fread(&magic, sizeof(magic), 1, fp)) {
+        goto close_rom;
+    }

Don't you want to do that before you write '1' in it?

Quoted 9 lines

hw/xen/xen_pt_graphics.c new file +9 -0

+
+    if (magic != 0xAA55) {
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    if (!fread(buf, 1, st.st_size, fp)) {
+        XEN_PT_ERR(NULL, "pci-assign: Cannot read from host %s", rom_file);
+        XEN_PT_LOG(NULL, "Device option ROM contents are probably invalid "

hw/xen/xen_pt_graphics.c line 133

Quoted 10 lines

hw/xen/xen_pt_graphics.c new file +10 -0

+                     "(check dmesg).\nSkip option ROM probe with rombar=0, "
+                     "or load from file with romfile=\n");
+    }
+
+close_rom:
+    /* Write "0" to disable ROM */
+    fseek(fp, 0, SEEK_SET);
+    val = 0;
+    if (!fwrite(&val, 1, 1, fp)) {
+        XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file");

Should we return -1? (after closing the file of course)

hw/xen/xen_pt_graphics.c line 136

Quoted 3 lines

hw/xen/xen_pt_graphics.c new file +3 -0

+    }
+    fclose(fp);
+    return st.st_size;

Ah, that is why your return -1! In that case I presume the caller of this function will check the 'errno' to find the underlaying issue

hw/xen/xen_pt_graphics.c line 140

Quoted 4 lines

hw/xen/xen_pt_graphics.c new file +4 -0

+}
+
+/* Allocated 128K for the vga bios */
+#define VGA_BIOS_DEFAULT_SIZE (0x20000)

Quoted 7 lines

hw/xen/xen_pt_graphics.c new file +7 -0

+int xen_pt_setup_vga(XenHostPCIDevice *dev)
+{
+    unsigned char *bios = NULL;
+    int bios_size = 0;
+    char *c = NULL;
+    char checksum = 0;
+    int rc = 0;

hw/xen/xen_pt_graphics.c line 158

Quoted 10 lines

hw/xen/xen_pt_graphics.c new file +10 -0

+
+    if (!is_vga_passthrough(dev)) {
+        return rc;
+    }
+
+    bios = g_malloc0(VGA_BIOS_DEFAULT_SIZE);
+
+    bios_size = get_vgabios(bios, dev);
+    if (bios_size == 0 || bios_size > VGA_BIOS_DEFAULT_SIZE) {
+        XEN_PT_ERR(NULL, "vga bios size (0x%x) is invalid!\n", bios_size);

Um, with an error code, the 'bios size (0xfffffffff)' is going to show up. Why don't you an extra code to check for this , like:

	if (bios_size < 0)
		XEN_PT_ERR(NULL,"Error %d (%s) getting BIOS!\n", errno, strerror(errno));
	else
		.. the other error.

Quoted 84 lines

hw/xen/xen_pt_graphics.c new file +19 -0

+        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;
+        XEN_PT_LOG(NULL, "vga bios checksum is adjusted!\n");
+    }
+
+    /* Currently we fixed this address as a primary. */
+    cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
+out:
+    g_free(bios);
+    return rc;
+}

qemu-options.hx +9 -0

index 781af14..cece134 100644
@@ -1047,6 +1047,15 @@ STEXI
 Rotate graphical output some deg left (only PXA LCD).
 ETEXI
 
+DEF("gfx_passthru", 0, QEMU_OPTION_gfx_passthru,
+    "-gfx_passthru   enable Intel IGD passthrough by XEN\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -gfx_passthru
+@findex -gfx_passthru
+Enable Intel IGD passthrough by XEN
+ETEXI
+
 DEF("vga", HAS_ARG, QEMU_OPTION_vga,
     "-vga [std|cirrus|vmware|qxl|xenfb|tcx|cg3|none]\n"
     "                select video card type\n", QEMU_ARCH_ALL)

vl.c +10 -1

index 73e0661..c86e95f 100644
@@ -1436,6 +1436,13 @@ static void smp_parse(QemuOpts *opts)
 
 }
 
+/* We still need this for compatibility with XEN. */
+int xen_has_gfx_passthru;
+static void xen_gfx_passthru(const char *optarg)
+{
+    xen_has_gfx_passthru = 1;
+}
+
 static void configure_realtime(QemuOpts *opts)
 {
     bool enable_mlock;

@@ -2988,7 +2995,6 @@ int main(int argc, char **argv, char **envp)
     const char *trace_file = NULL;
     const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
                                         1024 * 1024;
-
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
     qemu_init_exec_dir(argv[0]);

@@ -3956,6 +3962,9 @@ int main(int argc, char **argv, char **envp)
                 }
                 configure_msg(opts);
                 break;
+            case QEMU_OPTION_gfx_passthru:
+                xen_gfx_passthru(optarg);
+                break;
             default:
                 os_parse_cmd_args(popt->index, optarg);
             }

Signature · 7 lines

1.9.1


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

-----Original Message-----
From: Stefano Stabellini [mailto:[email protected]]
Sent: Monday, May 19, 2014 8:10 PM
To: Konrad Rzeszutek Wilk
Cc: Chen, Tiejun; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support

Quoted 18 lines · Konrad Rzeszutek Wilk

Quoted 17 lines · Tiejun Chen

basic gfx passthrough support:
- add a vga type for gfx passthrough
- retrieve VGA bios from sysfs, then load it to guest 0xC0000
- register/unregister legacy VGA I/O ports and MMIOs for passthroughed gfx

The original patch is from Weidong Han <[email protected]>

Signed-off-by Yang Zhang <[email protected]>

Signed-off-by Tiejun Chen <[email protected]>

Cc Weidong Han <[email protected]>

---
v2:

* retrieve VGA bios from sysfs properly.
* redefine some function name.

Quoted 6 lines

Quoted 6 lines

 hw/xen/Makefile.objs         |   2 +-
 hw/xen/xen-host-pci-device.c |   5 ++
 hw/xen/xen-host-pci-device.h |   1 +
 hw/xen/xen_pt.c              |  10 +++
 hw/xen/xen_pt.h              |   4 +
 hw/xen/xen_pt_graphics.c     | 177

+++++++++++++++++++++++++++++++++++++++++++

Quoted 11 lines

hw/xen/Makefile.objs line 2

Quoted 11 lines

 qemu-options.hx              |   9 +++
 vl.c                         |  11 ++-
 8 files changed, 217 insertions(+), 2 deletions(-)  create mode

100644 hw/xen/xen_pt_graphics.c

hw/xen/Makefile.objs index renamed from hw/xen/Makefile.objs +0 -0

a0ca0aa..77b7dae 100644

hw/xen/Makefile.objs +0 -0

@@ -2,4 +2,4 @@
 common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o

xen_devconfig.o

Quoted 11 lines

hw/xen/xen-host-pci-device.c int xen_host_pci_device_get(XenHostPCIDevice

Quoted 11 lines

hw/xen/Makefile.objs +0 -1

 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

hw/xen/Makefile.objs +2 -0

+obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o
+xen_pt_msi.o xen_pt_graphics.o

(quoted diff) +0 -0

b/hw/xen/xen-host-pci-device.c index 743b37b..a54b7de 100644

hw/xen/xen-host-pci-device.c +0 -0

@@ -376,6 +376,11 @@ int xen_host_pci_device_get(XenHostPCIDevice

*d, uint16_t domain,

Quoted 22 lines

Quoted 22 lines

hw/xen/xen-host-pci-device.c +5 -0

         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;

(quoted diff) +0 -0

b/hw/xen/xen-host-pci-device.h index c2486f0..f1e1c30 100644

hw/xen/xen-host-pci-device.h +1 -0

@@ -25,6 +25,7 @@ typedef struct XenHostPCIDevice {
 
     uint16_t vendor_id;
     uint16_t device_id;
+    uint32_t class_code;
     int irq;
 

Quoted 5 lines

hw/xen/xen_pt.c static int

Quoted 5 lines

hw/xen/xen-host-pci-device.h +0 -0

     XenHostPCIIORegion io_regions[PCI_NUM_REGIONS - 1]; diff --git

a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index be4220b..a0113ea 100644

hw/xen/xen_pt.c +0 -0

@@ -450,6 +450,7 @@ static int

xen_pt_register_regions(XenPCIPassthroughState *s)

Quoted 8 lines

hw/xen/xen_pt.c static void

Quoted 8 lines

hw/xen/xen_pt.c +1 -0

                    d->rom.size, d->rom.base_addr);
     }
 
+    xen_pt_register_vga_regions(d);
     return 0;
 }
 

@@ -470,6 +471,8 @@ static void

xen_pt_unregister_regions(XenPCIPassthroughState *s)

Quoted 7 lines

Quoted 7 lines

hw/xen/xen_pt.c +2 -0

     if (d->rom.base_addr && d->rom.size) {
         memory_region_destroy(&s->rom);
     }
+
+    xen_pt_unregister_vga_regions(d);
 }
 

Quoted 8 lines

hw/xen/xen_pt.c static int xen_pt_initfn(PCIDevice *d) · line 701

Quoted 8 lines

hw/xen/xen_pt.c +3 -0

 /* region mapping */

@@ -693,6 +696,13 @@ static int xen_pt_initfn(PCIDevice *d)
     /* Handle real device's MMIO/PIO BARs */
     xen_pt_register_regions(s);
 
+    /* Setup VGA bios for passthroughed gfx */
+    if (xen_pt_setup_vga(&s->real_device) < 0) {
+        XEN_PT_ERR(d, "Setup VGA BIOS of passthroughed gfx

failed!\n");

Quoted 4 lines

Quoted 4 lines

hw/xen/xen_pt.c +4 -0

+        xen_host_pci_device_put(&s->real_device);
+        return -1;
+    }
+

Quoted 8 lines

hw/xen/xen_pt.h static inline bool

Quoted 8 lines

hw/xen/xen_pt.c +0 -0

     /* reinitialize each config register to be emulated */
     if (xen_pt_config_init(s)) {
         XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");

hw/xen/xen_pt.h index renamed from hw/xen/xen_pt.h +0 -0

942dc60..4d3a18d 100644

hw/xen/xen_pt.h +0 -0

@@ -298,5 +298,9 @@ static inline bool

xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)

Quoted 28 lines

Quoted 28 lines

hw/xen/xen_pt.h +4 -0

     return s->msix && s->msix->bar_index == bar;  }
 
+extern int xen_has_gfx_passthru;
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev); int
+xen_pt_unregister_vga_regions(XenHostPCIDevice *dev); int
+xen_pt_setup_vga(XenHostPCIDevice *dev);
 
 #endif /* !XEN_PT_H */

hw/xen/xen_pt_graphics.c new renamed from hw/xen/xen_pt_graphics.c +0 -0

file mode 100644 index 0000000..e1f0724

hw/xen/xen_pt_graphics.c new file +15 -0

@@ -0,0 +1,177 @@
+/*
+ * graphics passthrough
+ */
+#include "xen_pt.h"
+#include "xen-host-pci-device.h"
+#include "hw/xen/xen_backend.h"
+
+static int is_vga_passthrough(XenHostPCIDevice *dev) {
+    return (xen_has_gfx_passthru
+            && ((dev->class_code >> 0x8) ==
+PCI_CLASS_DISPLAY_VGA)); }
+
+/*
+ * register VGA resources for the domain with assigned gfx  */ int
+xen_pt_register_vga_regions(XenHostPCIDevice *dev) {

Quoted 8 lines

hw/xen/xen_pt_graphics.c line 26

Quoted 8 lines

hw/xen/xen_pt_graphics.c new file +8 -0

+    int ret = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xA, DPCI_ADD_MAPPING);

The original code does:

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

why are we remapping fewer ports now?

Quoted 22 lines

hw/xen/xen_pt_graphics.c line 38

Quoted 11 lines

hw/xen/xen_pt_graphics.c new file +11 -0

+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
+            0x3C0, 0x20, DPCI_ADD_MAPPING);
+
+    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0xa0000 >> XC_PAGE_SHIFT,
+            0x20,
+            DPCI_ADD_MAPPING);
+
+    if (ret) {
+        XEN_PT_ERR(NULL, "VGA region mapping failed\n");

It would be actually useful to know _which_ of them failed. Perhaps you could structure this a bit differently and do:

struct _args {
        uint32_t gport;
        uint32_t mport;
        uint32_t nport;
};

Quoted 5 lines

        struct _args args[3] = {{ 0x3B0, 0x3B0, 0xA }, {...}};
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(args); i++) {
		ret = xc_domain_ioport_mapping(xen_xc, xen_domid, args[i].gport,

args[i]..)

Quoted 2 lines

		if (ret) {
			XEN_PT_ERR_(NULL, "VGA region mapping of 0x%lx for 0x%x

pages failed with rc:%d\n",

Quoted 15 lines

					... fill in the values)
			return ret;
	}

Quoted 10 lines

hw/xen/xen_pt_graphics.c new file +10 -0

+    }
+
+    return ret;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx  */
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) {
+    int ret = 0;
+

Quoted 6 lines

hw/xen/xen_pt_graphics.c line 56

Quoted 6 lines

hw/xen/xen_pt_graphics.c new file +6 -0

+    if (!is_vga_passthrough(dev)) {
+        return ret;
+    }
+
+    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+            0x3B0, 0xC, DPCI_REMOVE_MAPPING);

But here we are unmapping 0xC ioports....

Some VBIOSs and drivers assume the IGD BDF (bus:device:function) is always 00:02.0, so we need to reserves 00:02.0 for assigned IGD in

guest.

Signed-off-by Tiejun Chen <[email protected]>

Signed-off-by Yang Zhang <[email protected]>

---
v2:

* Use a common way patch #2 introduce to reserve PCI devfn.

 hw/pci-host/piix.c | 8 ++++++++
 1 file changed, 8 insertions(+)

hw/pci-host/piix.c +0 -0

index ffdc853..b6f49bd 100644
@@ -329,6 +329,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     s = PCI_HOST_BRIDGE(dev);

hw/pci-host/piix.c +0 -0

     s->bus = b;
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
     qdev_init_nofail(dev);

Signature · 7 lines

1.9.1


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

-----Original Message-----
From: Konrad Rzeszutek Wilk [mailto:[email protected]]
Sent: Friday, May 16, 2014 10:09 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Xen-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Quoted 2 lines · Tiejun Chen

Some VBIOSs and drivers assume the IGD BDF (bus:device:function) is always 00:02.0, so we need to reserves 00:02.0 for assigned IGD in

reserve

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState

Quoted 18 lines

guest.

Signed-off-by Tiejun Chen <[email protected]>

Signed-off-by Yang Zhang <[email protected]>

---
v2:

* Use a common way patch #2 introduce to reserve PCI devfn.

 hw/pci-host/piix.c | 8 ++++++++
 1 file changed, 8 insertions(+)

hw/pci-host/piix.c index renamed from hw/pci-host/piix.c +0 -0

ffdc853..b6f49bd 100644

hw/pci-host/piix.c +0 -0

@@ -329,6 +329,14 @@ PCIBus *i440fx_init(PCII440FXState

**pi440fx_state,

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state,

Quoted 6 lines

hw/pci-host/piix.c +3 -0

     s = PCI_HOST_BRIDGE(dev);
     b = pci_bus_new(dev, NULL, pci_address_space,
                     address_space_io, 0, TYPE_PCI_BUS);
+
+    /*
+     * Some video bioses and gfx drivers will assume the bdf of IGD is

00:02.0.

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 339

Quoted 5 lines

hw/pci-host/piix.c +5 -0

+     * So user need to set it to 00:02.0 in Xen configure file explicitly,
+     * otherwise IGD will fail to work.
+     */
+    pci_reserve_pci_devfn(b, PCI_DEVFN(2, 0));
+

And we do this without checking whether PCI passthrough is done. Should it be gated on that? Or is the reason you do it unconditionally because you want to be able to hot-plug an GFX in?

   Hi,

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 338

Quoted 6 lines

hw/pci-host/piix.c +6 -0

+    /*
+     * Some video bioses and gfx drivers will assume the bdf of IGD is 00:02.0.
+     * So user need to set it to 00:02.0 in Xen configure file explicitly,
+     * otherwise IGD will fail to work.
+     */
+    pci_reserve_pci_devfn(b, PCI_DEVFN(2, 0));

That is asking for trouble. Slot 2 is used by the qemu vga cards by default, and for quite a while (before memory api was merged) it even was impossible to change it. libvirt still places the vga card at slot 2 for that reason -> boom. I wouldn't be surprised if you find that assumption in other management libs / apps too.

Why do you need that patch in the first place? It should be possible to configure qemu to not occupy slot 2 if you need it that way. Just pass '-vga none' to qemu. Which you probably want anyway if you pass-through a vga to the guest. And explicitly configure a slot (via addr= property) for all your pci devices. Doing it only for the IGD works too if you list the device before any other pci device on the qemu command line.

cheers,
Gerd

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Il 19/05/2014 08:44, Gerd Hoffmann ha scritto:

Quoted 19 lines

   Hi,

Quoted 4 lines

+ /* + * Some video bioses and gfx drivers will assume the bdf
of IGD is 00:02.0. + * So user need to set it to 00:02.0 in Xen
configure file explicitly, + * otherwise IGD will fail to work. +
*/ + pci_reserve_pci_devfn(b, PCI_DEVFN(2, 0));

That is asking for trouble. Slot 2 is used by the qemu vga cards by default, and for quite a while (before memory api was merged) it even was impossible to change it. libvirt still places the vga card at slot 2 for that reason -> boom. I wouldn't be surprised if you find that assumption in other management libs / apps too.

Why do you need that patch in the first place? It should be possible to configure qemu to not occupy slot 2 if you need it that way. Just pass '-vga none' to qemu. Which you probably want anyway if you pass-through a vga to the guest. And explicitly configure a slot (via addr= property) for all your pci devices. Doing it only for the IGD works too if you list the device before any other pci device on the qemu command line.

Quoted 2 lines

cheers,
Gerd

I already added vga none support on libxl, useful also for this cases:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=2e5738ff47b9d2e19480
24100f87b1a25fcf004a

This patch is already tested and working, for now I not tested and I can't test with intel gpu passthrough, someone can test it with intel gpu passthrough?

Thanks for any reply and sorry for my bad english.

Quoted 7 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

-----Original Message-----
From: Fabio Fantoni [mailto:[email protected]]
Sent: Monday, May 19, 2014 3:48 PM
To: Gerd Hoffmann; Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Zhang, Yang Z; [email protected]; [email protected]
Subject: Re: [Xen-devel] [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough:
reserve 00:02.0 for INTEL IGD

Il 19/05/2014 08:44, Gerd Hoffmann ha scritto:

Quoted 4 lines

   Hi,

Quoted 2 lines

(quoted diff) +2 -0

+    /*
+     * Some video bioses and gfx drivers will assume the bdf of IGD is

00:02.0.

Quoted 20 lines

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 338

Quoted 4 lines

hw/pci-host/piix.c +4 -0

+     * So user need to set it to 00:02.0 in Xen configure file explicitly,
+     * otherwise IGD will fail to work.
+     */
+    pci_reserve_pci_devfn(b, PCI_DEVFN(2, 0));

That is asking for trouble. Slot 2 is used by the qemu vga cards by default, and for quite a while (before memory api was merged) it even was impossible to change it. libvirt still places the vga card at slot
2 for that reason -> boom. I wouldn't be surprised if you find that assumption in other management libs / apps too.

Why do you need that patch in the first place? It should be possible to configure qemu to not occupy slot 2 if you need it that way. Just pass '-vga none' to qemu. Which you probably want anyway if you pass-through a vga to the guest. And explicitly configure a slot (via addr=
property) for all your pci devices. Doing it only for the IGD works too if you list the device before any other pci device on the qemu command line.

Quoted 2 lines

cheers,
Gerd

I already added vga none support on libxl, useful also for this cases:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=2e5738ff47b9d2e19480
24100f87b1a25fcf004a

This patch is already tested and working, for now I not tested and I can't test with intel gpu passthrough, someone can test it with intel gpu passthrough?

Thanks for any reply and sorry for my bad english.

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Monday, May 19, 2014 2:45 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Hi,

Review

Quoted 2 lines

(quoted diff) +2 -0

+    /*
+     * Some video bioses and gfx drivers will assume the bdf of IGD is

00:02.0.

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 338

Quoted 4 lines

hw/pci-host/piix.c +4 -0

+     * So user need to set it to 00:02.0 in Xen configure file explicitly,
+     * otherwise IGD will fail to work.
+     */
+    pci_reserve_pci_devfn(b, PCI_DEVFN(2, 0));

That is asking for trouble. Slot 2 is used by the qemu vga cards by default, and for quite a while (before memory api was merged) it even was impossible to change it. libvirt still places the vga card at slot
2 for that reason -> boom. I wouldn't be surprised if you find that assumption in other management libs / apps too.

Why do you need that patch in the first place? It should be possible to configure qemu to not occupy slot 2 if you need it that way. Just pass '-vga none' to qemu. Which you probably want anyway if you pass-through a vga to the guest. And explicitly configure a slot (via addr=

property) for all your pci devices. Doing it only for the IGD works too if you list the device before any other pci device on the qemu command line.

Quoted 28 lines

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Monday, May 19, 2014 2:45 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Hi,

Review

Quoted 2 lines

(quoted diff) +2 -0

+    /*
+     * Some video bioses and gfx drivers will assume the bdf of IGD is

00:02.0.

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 338

Quoted 4 lines

hw/pci-host/piix.c +4 -0

+     * So user need to set it to 00:02.0 in Xen configure file explicitly,
+     * otherwise IGD will fail to work.
+     */
+    pci_reserve_pci_devfn(b, PCI_DEVFN(2, 0));

That is asking for trouble. Slot 2 is used by the qemu vga cards by default, and for quite a while (before memory api was merged) it even was impossible to change it. libvirt still places the vga card at slot
2 for that reason -> boom. I wouldn't be surprised if you find that assumption in other management libs / apps too.

Quoted 4 lines

Why do you need that patch in the first place? It should be possible to configure qemu to not occupy slot 2 if you need it that way. Just pass '-vga none' to qemu. Which you probably want anyway if you pass-through a vga to the guest. And explicitly configure a slot (via addr=

I think '-vga none' just guarantees the qemu vga cards doesn't occupy 00:02.0, but this doesn't mean others use this specific slot since in qemu internal, we always pass -1 to assign a slot automatically to register a PCI device. So in some cases, we can't get this slot as we expect since that is already assigned previously before we need this.

Quoted 2 lines

property) for all your pci devices. Doing it only for the IGD works too if you list the device before any other pci device on the qemu command line.

So in my test scenario, we can see this information:

PCI: slot 2 function 0 not available for xen-pci-passthrough, in use by xen-platform Thanks
Tiejun

-----Original Message-----
From: Michael S. Tsirkin [mailto:[email protected]]
Sent: Monday, May 19, 2014 6:13 PM
To: Chen, Tiejun
Cc: Gerd Hoffmann; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough:
reserve
00:02.0 for INTEL IGD

Quoted 31 lines · Tiejun

Quoted 31 lines

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Monday, May 19, 2014 2:45 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough:
reserve
00:02.0 for INTEL IGD

Hi,

Review

Quoted 3 lines

(quoted diff) +3 -0

+    /*
+     * Some video bioses and gfx drivers will assume the bdf of 
+ IGD is

00:02.0.

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 338

Quoted 4 lines

hw/pci-host/piix.c +4 -0

+     * So user need to set it to 00:02.0 in Xen configure file explicitly,
+     * otherwise IGD will fail to work.
+     */
+    pci_reserve_pci_devfn(b, PCI_DEVFN(2, 0));

That is asking for trouble. Slot 2 is used by the qemu vga cards by default, and for quite a while (before memory api was merged) it even was impossible to change it. libvirt still places the vga card at slot
2 for that reason -> boom. I wouldn't be surprised if you find that assumption in other management libs / apps too.

Quoted 8 lines

Quoted 5 lines

Why do you need that patch in the first place? It should be possible to configure qemu to not occupy slot 2 if you need it that way. Just pass '-vga none' to qemu. Which you probably want anyway if you pass-through a vga to the guest. And explicitly configure a slot (via addr=

I think '-vga none' just guarantees the qemu vga cards doesn't occupy 00:02.0,

but this doesn't mean others use this specific slot since in qemu internal, we always pass -1 to assign a slot automatically to register a PCI device. So in some cases, we can't get this slot as we expect since that is already assigned previously before we need this.

So stop doing this.
Address -1 is a short-cut intended for humans.

In particular we have no way to guarantee that migration works unless you specify addresses explicitly.

Quoted 3 lines

Quoted 3 lines

property) for all your pci devices. Doing it only for the IGD works too if you list the device before any other pci device on the qemu command

line.

Quoted 5 lines

So in my test scenario, we can see this information:

PCI: slot 2 function 0 not available for xen-pci-passthrough, in use by xen-platform Thanks Tiejun

I think the best fix is to give priority to devices that supply a specific slot.

Quoted 53 lines

-----Original Message-----
From: Michael S. Tsirkin [mailto:[email protected]]
Sent: Monday, May 19, 2014 6:13 PM
To: Chen, Tiejun
Cc: Gerd Hoffmann; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough:
reserve
00:02.0 for INTEL IGD

Quoted 31 lines · Tiejun

Quoted 31 lines

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Monday, May 19, 2014 2:45 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough:
reserve
00:02.0 for INTEL IGD

Hi,

Review

Quoted 3 lines

(quoted diff) +3 -0

+    /*
+     * Some video bioses and gfx drivers will assume the bdf of 
+ IGD is

00:02.0.

hw/pci-host/piix.c PCIBus *i440fx_init(PCII440FXState **pi440fx_state, · line 338

Quoted 4 lines

hw/pci-host/piix.c +4 -0

+     * So user need to set it to 00:02.0 in Xen configure file explicitly,
+     * otherwise IGD will fail to work.
+     */
+    pci_reserve_pci_devfn(b, PCI_DEVFN(2, 0));

That is asking for trouble. Slot 2 is used by the qemu vga cards by default, and for quite a while (before memory api was merged) it even was impossible to change it. libvirt still places the vga card at slot
2 for that reason -> boom. I wouldn't be surprised if you find that assumption in other management libs / apps too.

Quoted 8 lines

Quoted 5 lines

Why do you need that patch in the first place? It should be possible to configure qemu to not occupy slot 2 if you need it that way. Just pass '-vga none' to qemu. Which you probably want anyway if you pass-through a vga to the guest. And explicitly configure a slot (via addr=

I think '-vga none' just guarantees the qemu vga cards doesn't occupy 00:02.0,

Quoted 7 lines

but this doesn't mean others use this specific slot since in qemu internal, we always pass -1 to assign a slot automatically to register a PCI device. So in some cases, we can't get this slot as we expect since that is already assigned previously before we need this.

So stop doing this.
Address -1 is a short-cut intended for humans.

Are you saying we need remove all auto assigned places like this,

piix.c: i440fx_init()

    if (xen_enabled()) {
        piix3 = DO_UPCAST(PIIX3State, dev,
                pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
        pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
                piix3, XEN_PIIX_NUM_PIRQS);
    } else {
        piix3 = DO_UPCAST(PIIX3State, dev,
                pci_create_simple_multifunction(b, -1, true, "PIIX3"));

Quoted 6 lines

In particular we have no way to guarantee that migration works unless you specify addresses explicitly.

Quoted 3 lines

Quoted 3 lines

property) for all your pci devices. Doing it only for the IGD works too if you list the device before any other pci device on the qemu command

Quoted 9 lines

line.

Quoted 5 lines

So in my test scenario, we can see this information:

PCI: slot 2 function 0 not available for xen-pci-passthrough, in use by xen-platform Thanks Tiejun

I think the best fix is to give priority to devices that supply a specific slot.

Maybe I can extend that bitmap I implemented in patch #1 to do this.

But anyway you also need to concern if this is compatible with libxl in xen case.

Thanks
Tiejun

I think '-vga none' just guarantees the qemu vga cards doesn't occupy 00:02.0, but this doesn't mean others use this specific slot since in qemu internal, we always pass -1 to assign a slot automatically to register a PCI device. So in some cases, we can't get this slot as we expect since that is already assigned previously before we need this.

PCI: slot 2 function 0 not available for xen-pci-passthrough, in use by xen-platform

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Monday, May 19, 2014 7:23 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Hi,

Quoted 5 lines

I think '-vga none' just guarantees the qemu vga cards doesn't occupy 00:02.0, but this doesn't mean others use this specific slot since in qemu internal, we always pass -1 to assign a slot automatically to register a PCI device. So in some cases, we can't get this slot as we expect since that is already assigned previously before we need this.

Yes, -vga, -net nic, -drive if=scsi (maybe more) can internally create pci devices with auto slot assignment, which will occupy slot 2 indeed.
Use -device instead to create the devices.

Quoted 2 lines

PCI: slot 2 function 0 not available for xen-pci-passthrough, in use by xen-platform

Ah, the xen platform device. /me looks. Ah, pc_xen_hvm_init creates this automatically. Two options here IMHO:

(1) Just move it somewhere else explicitly. For example slot 3, or make it a southbridge function (say 00:01.7).
(2) Don't create it automatically, instead expect management add it if needed, using -device xen-plaform,addr=...

I personally would suggest to go for #2. As far I know the platform device is only needed if you want attach xenbus devices to the guest (correct?), so creating virtual machines without the xen platform device is a valid use case and you should allow it.

Quoted 4 lines

Yes, -vga, -net nic, -drive if=scsi (maybe more) can internally create pci devices with auto slot assignment, which will occupy slot 2 indeed.
Use -device instead to create the devices.

Are you saying we have to create the devices explicitly when we want to work IGD vga with passthrough? But how to make sure all user know this workable way? Maybe you suggest we should document somewhere.

Quoted 8 lines

Ah, the xen platform device. /me looks. Ah, pc_xen_hvm_init creates this automatically. Two options here IMHO:

(1) Just move it somewhere else explicitly. For example slot 3, or make it a southbridge function (say 00:01.7).
(2) Don't create it automatically, instead expect management add it if needed, using -device xen-plaform,addr=...

Quoted 4 lines

I personally would suggest to go for #2. As far I know the platform device is only needed if you want attach xenbus devices to the guest (correct?), so creating virtual machines without the xen platform device is a valid use case and you should allow it.

Looks you recommend we should change current xen platform design, I'm not sure if something in libxl also need to be modified. Especially, this may not be compatible with those old xen version.

And especially, how to guarantee no one occupy 00:02.0 in the future with auto assign?

Hi,

Quoted 8 lines

Quoted 4 lines

Yes, -vga, -net nic, -drive if=scsi (maybe more) can internally create pci devices with auto slot assignment, which will occupy slot 2 indeed.
Use -device instead to create the devices.

Are you saying we have to create the devices explicitly when we want to work IGD vga with passthrough? But how to make sure all user know this workable way? Maybe you suggest we should document somewhere.

libvirt does this unconditionally, because it is a good idea anyway for a number of reasons. Example: create a machine with three pci devices, hot-unplug the second, then live-migrate to another machine. The only way to create the correct config on the target machine is to explicitly assign slots, otherwise the third pci device ends up in the wrong slot.

Don't know how the libxl (and xl tool) work. Maybe it does the same anyway. Maybe it can handle the address assignment transparently for the user.

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Monday, May 19, 2014 9:51 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Hi,

Quoted 2 lines

Quoted 2 lines

Yes, -vga, -net nic, -drive if=scsi (maybe more) can internally create pci devices with auto slot assignment, which will occupy slot 2

indeed.

Quoted 6 lines

Quoted 2 lines

Use -device instead to create the devices.

Are you saying we have to create the devices explicitly when we want to work IGD vga with passthrough? But how to make sure all user know this workable way? Maybe you suggest we should document somewhere.

libvirt does this unconditionally, because it is a good idea anyway for a number of reasons. Example: create a machine with three pci devices, hot-unplug the second, then live-migrate to another machine. The only way to create the correct config on the target machine is to explicitly assign slots, otherwise the third pci device ends up in the wrong slot.

Don't know how the libxl (and xl tool) work. Maybe it does the same anyway. Maybe it can handle the address assignment transparently for the user.

Quoted 8 lines

Quoted 8 lines

Ah, the xen platform device. /me looks. Ah, pc_xen_hvm_init creates this automatically. Two options here IMHO:

(1) Just move it somewhere else explicitly. For example slot 3, or make it a southbridge function (say 00:01.7).
(2) Don't create it automatically, instead expect management add it if needed, using -device xen-plaform,addr=...

Quoted 8 lines

Quoted 4 lines

I personally would suggest to go for #2. As far I know the platform device is only needed if you want attach xenbus devices to the guest (correct?), so creating virtual machines without the xen platform device is a valid use case and you should allow it.

Looks you recommend we should change current xen platform design, I'm not sure if something in libxl also need to be modified. Especially, this may not be compatible with those old xen version.

Going for (1) certainly is easier as (2) indeed will need changes in the libxl, and might be tricky to get work with both old+new versions.

Quoted 2 lines

And especially, how to guarantee no one occupy 00:02.0 in the future with auto assign?

Creating devices automatically turned out to have a number of problems. So it is very unlikely we'll ever do this again.

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Chen, Tiejun
Sent: Wednesday, May 21, 2014 3:08 PM
To: Gerd Hoffmann; Anthony PERARD; Daniel P. Berrange
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z; [email protected]; [email protected]
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Quoted 25 lines

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Monday, May 19, 2014 9:51 PM
To: Chen, Tiejun
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough:
reserve
00:02.0 for INTEL IGD

Hi,

Quoted 3 lines

Quoted 3 lines

Yes, -vga, -net nic, -drive if=scsi (maybe more) can internally create pci devices with auto slot assignment, which will occupy slot 2

indeed.

Quoted 6 lines

Quoted 2 lines

Use -device instead to create the devices.

Are you saying we have to create the devices explicitly when we want to work IGD vga with passthrough? But how to make sure all user know this workable way? Maybe you suggest we should document somewhere.

Quoted 5 lines

libvirt does this unconditionally, because it is a good idea anyway for a number of reasons. Example: create a machine with three pci devices, hot-unplug the second, then live-migrate to another machine. The only way to create the correct config on the target machine is to explicitly assign slots, otherwise the third pci device ends up in the wrong

slot.

Quoted 2 lines

Don't know how the libxl (and xl tool) work. Maybe it does the same

anyway.

Quoted 19 lines

Maybe it can handle the address assignment transparently for the user.

Quoted 8 lines

Quoted 8 lines

Ah, the xen platform device. /me looks. Ah, pc_xen_hvm_init creates this automatically. Two options here IMHO:

(1) Just move it somewhere else explicitly. For example slot 3, or make it a southbridge function (say 00:01.7).
(2) Don't create it automatically, instead expect management add it if needed, using -device xen-plaform,addr=...

Quoted 8 lines

Quoted 4 lines

I personally would suggest to go for #2. As far I know the platform device is only needed if you want attach xenbus devices to the guest (correct?), so creating virtual machines without the xen platform device is a valid use case and you should allow it.

Looks you recommend we should change current xen platform design, I'm not sure if something in libxl also need to be modified. Especially, this may not be compatible with those old xen version.

Quoted 9 lines

Going for (1) certainly is easier as (2) indeed will need changes in the libxl, and might be tricky to get work with both old+new versions.

Quoted 2 lines

And especially, how to guarantee no one occupy 00:02.0 in the future with auto assign?

Creating devices automatically turned out to have a number of problems. So it is very unlikely we'll ever do this again.

According to our discussions, I realize we may have some plans or policies dedicated to how to assign devfn, but to support GFX passthrough for XEN, I think currently it may be a better solution to adopt #1 simply like this:

hw/i386/pc_piix.c index eaf3e61..500b3c2 renamed from hw/i386/pc_piix.c +0 -0

100644

hw/i386/pc_piix.c +1 -1

@@ -386,7 +386,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs
 *args)
 
     bus = pci_find_primary_bus();
     if (bus != NULL) {
-        pci_create_simple(bus, -1, "xen-platform");
+        pci_create_simple(bus, PCI_DEVFN(3,0), "xen-platform");
     }
 }

#endif

Then we can go out to plan how to assign devfn in common, is this fine?

Thanks
Tiejun

Quoted 19 lines

According to our discussions, I realize we may have some plans or policies dedicated to how to assign devfn, but to support GFX passthrough for XEN, I think currently it may be a better solution to adopt #1 simply like this:

hw/i386/pc_piix.c index eaf3e61..500b3c2 renamed from hw/i386/pc_piix.c +0 -0

100644

hw/i386/pc_piix.c +1 -1

@@ -386,7 +386,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs
 *args)
 
     bus = pci_find_primary_bus();
     if (bus != NULL) {
-        pci_create_simple(bus, -1, "xen-platform");
+        pci_create_simple(bus, PCI_DEVFN(3,0), "xen-platform");
     }
 }

#endif

Quoted 1 line

Then we can go out to plan how to assign devfn in common, is this fine?

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Thursday, May 22, 2014 1:40 PM
To: Chen, Tiejun
Cc: Anthony PERARD; Daniel P. Berrange; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z; [email protected]
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Hi,

Quoted 3 lines

Quoted 3 lines

According to our discussions, I realize we may have some plans or policies dedicated to how to assign devfn, but to support GFX passthrough for XEN, I think currently it may be a better solution to adopt

#1 simply like this:

Quoted 7 lines

hw/i386/pc_piix.c static void

Quoted 7 lines

hw/i386/pc_piix.c index renamed from hw/i386/pc_piix.c +0 -0

eaf3e61..500b3c2
100644

hw/i386/pc_piix.c +0 -0

@@ -386,7 +386,7 @@ static void

pc_xen_hvm_init(QEMUMachineInitArgs

Quoted 11 lines

Quoted 11 lines

hw/i386/pc_piix.c +1 -1

 *args)
 
     bus = pci_find_primary_bus();
     if (bus != NULL) {
-        pci_create_simple(bus, -1, "xen-platform");
+        pci_create_simple(bus, PCI_DEVFN(3,0), "xen-platform");
     }
 }

#endif

Then we can go out to plan how to assign devfn in common, is this fine?

Somewhere else in the thread someone listed a libxl config file snippet which is supposed to disable the xen platform device. Which doesn't work with upstream qemu for some reason.

Searching ... ah, here:
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03996.html

So, how about making "xen_platform_pci=0" working correctly ?

xen_platform_pci=0
device_model_args_hvm = [ '-device', 'xen-platform,addr=0x3' ]

Another useful thing would be to not create the xen platform device in case "-nodefaults" was specified on the command line (that switch turns off a bunch of other devices present by default: vga, nic, cdrom, ...).

Quoted 3 lines

Another useful thing would be to not create the xen platform device in case "-nodefaults" was specified on the command line (that switch turns off a bunch of other devices present by default: vga, nic, cdrom, ...).

Currently looks 'xen-platform' itself can't be created, not those devices existed on that.

From 405349bf38fc05653bfb8c971c61796e981eee98 Mon Sep 17 00:00:00 2001

Hi,

Quoted 5 lines

Quoted 3 lines

Another useful thing would be to not create the xen platform device in case "-nodefaults" was specified on the command line (that switch turns off a bunch of other devices present by default: vga, nic, cdrom, ...).

Currently looks 'xen-platform' itself can't be created, not those devices existed on that.

The error message looks more like libxl tries to hot-unplug the xen platform device.

Attached patch (untested!) hooks up the xen platform device to the default device code we have in qemu. Two effects:

(1) As mentioned above the device will not be created in case -nodefaults is specified on the command line.
(2) Autocreating the device is also turned off in case xen-platform is added manually via -device.

With the patch applied you should be able to move the xen-platform device to some other place with a simple 'qemu -device xen-platform,addr=$slot'.

cheers,
Gerd

Quoted 1 line

From 405349bf38fc05653bfb8c971c61796e981eee98 Mon Sep 17 00:00:00 2001

From: Gerd Hoffmann <[email protected]>
Date: Thu, 22 May 2014 08:34:28 +0200
Subject: [PATCH] xen: make xen-platform a default device

---

 hw/i386/pc_piix.c    | 2 +-
 include/hw/xen/xen.h | 1 +
 vl.c                 | 3 +++
 3 files changed, 5 insertions(+), 1 deletion(-)

hw/i386/pc_piix.c +1 -1

index eaf3e61..f987d03 100644
@@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs *args)
     pc_init_pci(args);
 
     bus = pci_find_primary_bus();
-    if (bus != NULL) {
+    if (bus != NULL && default_xenplatform) {
         pci_create_simple(bus, -1, "xen-platform");
     }
 }

include/hw/xen/xen.h +1 -0

index 85fda3d..b350413 100644
@@ -20,6 +20,7 @@ enum xen_mode {
 
 extern uint32_t xen_domid;
 extern enum xen_mode xen_mode;
+extern int default_xenplatform;
 
 extern bool xen_allowed;
 

vl.c +3 -0

index 709d8cd..673148e 100644
@@ -226,6 +226,7 @@ static int default_floppy = 1;
 static int default_cdrom = 1;
 static int default_sdcard = 1;
 static int default_vga = 1;
+int default_xenplatform = 1;
 
 static struct {
     const char *driver;

@@ -247,6 +248,7 @@ static struct {
     { .driver = "isa-cirrus-vga",       .flag = &default_vga       },
     { .driver = "vmware-svga",          .flag = &default_vga       },
     { .driver = "qxl-vga",              .flag = &default_vga       },
+    { .driver = "xen-platform",         .flag = &default_xenplatform },
 };
 
 static QemuOptsList qemu_rtc_opts = {

@@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp)
         default_monitor = 0;
         default_net = 0;
         default_vga = 0;
+        default_xenplatform = 0;
     }
 
     if (is_daemonized()) {

Signature · 1 line

1.8.3.1

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Thursday, May 22, 2014 2:45 PM
To: Chen, Tiejun
Cc: Anthony PERARD; Daniel P. Berrange; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z; [email protected]
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Hi,

Quoted 5 lines

Quoted 3 lines

Another useful thing would be to not create the xen platform device in case "-nodefaults" was specified on the command line (that switch turns off a bunch of other devices present by default: vga, nic, cdrom, ...).

Currently looks 'xen-platform' itself can't be created, not those devices

existed on that.

The error message looks more like libxl tries to hot-unplug the xen platform device.

Attached patch (untested!) hooks up the xen platform device to the default device code we have in qemu. Two effects:

(1) As mentioned above the device will not be created in case -nodefaults is specified on the command line.

(2) Autocreating the device is also turned off in case xen-platform is added manually via -device.

With the patch applied you should be able to move the xen-platform device to some other place with a simple 'qemu -device xen-platform,addr=$slot'.

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Thursday, May 22, 2014 2:45 PM
To: Chen, Tiejun
Cc: Anthony PERARD; Daniel P. Berrange; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z; [email protected]
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Hi,

Quoted 5 lines

Quoted 3 lines

Another useful thing would be to not create the xen platform device in case "-nodefaults" was specified on the command line (that switch turns off a bunch of other devices present by default: vga, nic, cdrom, ...).

Currently looks 'xen-platform' itself can't be created, not those devices

existed on that.

The error message looks more like libxl tries to hot-unplug the xen platform device.

Attached patch (untested!) hooks up the xen platform device to the default device code we have in qemu. Two effects:

(1) As mentioned above the device will not be created in case -nodefaults is specified on the command line.
(2) Autocreating the device is also turned off in case xen-platform is added manually via -device.

With the patch applied you should be able to move the xen-platform device to some other place with a simple 'qemu -device xen-platform,addr=$slot'.

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Chen, Tiejun
Sent: Thursday, May 22, 2014 6:50 PM
To: Gerd Hoffmann
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z; [email protected]; Anthony PERARD
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Quoted 26 lines

-----Original Message-----
From: Gerd Hoffmann [mailto:[email protected]]
Sent: Thursday, May 22, 2014 2:45 PM
To: Chen, Tiejun
Cc: Anthony PERARD; Daniel P. Berrange; [email protected]; [email protected]; [email protected]; [email protected]; Kay, Allen M; [email protected]; [email protected]; Zhang, Yang Z; [email protected]
Subject: Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

Hi,

Quoted 5 lines

Quoted 3 lines

Another useful thing would be to not create the xen platform device in case "-nodefaults" was specified on the command line (that switch turns off a bunch of other devices present by default: vga, nic, cdrom, ...).

Currently looks 'xen-platform' itself can't be created, not those devices

existed on that.

The error message looks more like libxl tries to hot-unplug the xen platform device.

Attached patch (untested!) hooks up the xen platform device to the default device code we have in qemu. Two effects:

Quoted 8 lines

(1) As mentioned above the device will not be created in case -nodefaults is specified on the command line.
(2) Autocreating the device is also turned off in case xen-platform is added manually via -device.

With the patch applied you should be able to move the xen-platform device to some other place with a simple 'qemu -device xen-platform,addr=$slot'.

Gerd,

Sorry, I may misunderstand what you mean previously then have a wrong test.

So this still doesn't work actually.

After applied your patch, 'xen-platform' is always disabled by default, right? So 00:02.0 is left naturally to be assigned to IGD as we expect like this,

tchen0@tchen0-HVM-domU:~$ lspci
00:00.0 Host bridge: Intel Corporation 4th Gen Core Processor DRAM
Controller (rev 06)
00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II]
00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II]
00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)
00:02.0 VGA compatible controller: Intel Corporation Xeon E3-1200 v3/4th Gen
Core Processor Integrated Graphics Controller (rev 06)
00:03.0 USB controller: Intel Corporation 8 Series/C220 Series Chipset Family
USB EHCI #2 (rev 04)
00:1f.0 ISA bridge: Intel Corporation Q87 Express LPC Controller (rev 04)

Then this is fine but if you intend to add a 'qemu -device xen-platform,addr=$slot', this doesn't work well. In my case,

gfx_passthru=1
pci=["00:02.0@2", "00:1a.0"]
xen_platform_pci=0
device_model_args_hvm = ['-device', 'xen-platform,addr=0x3']

tchen0@tchen0-linux:~/workspace$ sudo xl cr domu-cfg
Parsing config from domu-cfg
libxl: error: libxl_qmp.c:287:qmp_handle_error_response: received an error
message from QMP server: Unsupported bus. Bus doesn't have property
'acpi-pcihp-bsel' set
libxl: error: libxl_create.c:1277:domcreate_attach_pci: libxl_device_pci_add
failed: -3