xen: add Intel IGD passthrough support

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

+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;
+    int ret = 0;
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%d/rom",
+             dev->domain, dev->bus, dev->dev,
+             dev->func);

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

+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {
+        return -EACCES;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        XEN_PT_LOG("%s\n", "Failed to enable pci-sysfs rom file");
+        ret = -EIO;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    /*
+     * Check if it a real bios extension.
+     * The magic number is 0xAA55.
+     */
+    if (!fread(&magic, sizeof(magic), 1, fp)) {
+        XEN_PT_ERR(NULL, "VGA: can't get magic.\n");
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    if (magic != 0xAA55) {
+        XEN_PT_ERR(NULL, "VGA: wrong magic %x.\n", magic);
+        ret = -ENODEV;
+        goto close_rom;

Il 25/06/2014 04:17, Tiejun Chen ha scritto:

Quoted 17 lines

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

+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;
+    int ret = 0;
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%d/rom",
+             dev->domain, dev->bus, dev->dev,
+             dev->func);
+
+    if (stat(rom_file, &st)) {
+        return -ENODEV;
+    }

hw/xen/xen_pt_graphics.c line 141

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 -ENODEV;
+    }

These two checks (stat and access) are useless.

Quoted 35 lines

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

+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {
+        return -EACCES;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        XEN_PT_LOG("%s\n", "Failed to enable pci-sysfs rom file");
+        ret = -EIO;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    /*
+     * Check if it a real bios extension.
+     * The magic number is 0xAA55.
+     */
+    if (!fread(&magic, sizeof(magic), 1, fp)) {
+        XEN_PT_ERR(NULL, "VGA: can't get magic.\n");
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    if (magic != 0xAA55) {
+        XEN_PT_ERR(NULL, "VGA: wrong magic %x.\n", magic);
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    if (!fread(buf, 1, st.st_size, fp)) {
+        XEN_PT_ERR(NULL, "VGA: pci-assign: Cannot read from host %s",

rom_file);
+ XEN_PT_LOG(NULL, "VGA: Device option ROM contents are
probably invalid "

hw/xen/xen_pt_graphics.c line 178

Quoted 5 lines

+ "(check dmesg).\nSkip option ROM probe with
rombar=0, "

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

+                     "or load from file with romfile=\n");
+    }
+

Why is this graphics passthrough specific? Should QEMU's Xen PCI passthrough read ROM BAR contents from network cards too, for example?

basic gfx passthrough support:
- add a vga type for gfx passthrough
- retrieve VGA bios from sysfs, then load it to guest at 0xC0000
- register/unregister legacy VGA I/O ports and MMIOs for passthrough 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]>

---
v5:

* Just rebase.

v4:

* Fix some typos in the patch head description.
* Improve some comments.
* Given that xen_pt_register_vga_regions()/xen_pt_unregister_vga_regions() are called unconditionally, so we just return 0 there.
* Remove one spurious change.

v3:

* Fix some typos.
* Add more comments to make that readable.
* Improve some return paths.

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     | 232 +++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx              |   9 ++
 vl.c                         |  10 ++
 8 files changed, 272 insertions(+), 1 deletion(-)
 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..dac4238 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 passthrough GFX */
+    if (xen_pt_setup_vga(&s->real_device) < 0) {
+        XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+        xen_host_pci_device_put(&s->real_device);
+        return -1;
+    }
+
     /* 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 +208 -0

new file mode 100644
index 0000000..461e526
@@ -0,0 +1,232 @@
+/*
+ * 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));
+}
+
+typedef struct VGARegion {
+    int type;           /* Memory or port I/O */
+    uint64_t guest_base_addr;
+    uint64_t machine_base_addr;
+    uint64_t size;    /* size of the region */
+    int rc;
+} VGARegion;
+
+#define IORESOURCE_IO           0x00000100
+#define IORESOURCE_MEM          0x00000200
+
+static struct VGARegion vga_args[] = {
+    {
+        .type = IORESOURCE_IO,
+        .guest_base_addr = 0x3B0,
+        .machine_base_addr = 0x3B0,
+        .size = 0xC,
+        .rc = -1,
+    },
+    {
+        .type = IORESOURCE_IO,
+        .guest_base_addr = 0x3C0,
+        .machine_base_addr = 0x3C0,
+        .size = 0x20,
+        .rc = -1,
+    },
+    {
+        .type = IORESOURCE_MEM,
+        .guest_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+        .machine_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+        .size = 0x20,
+        .rc = -1,
+    },
+};
+
+/*
+ * register VGA resources for the domain with assigned gfx
+ */
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
+{
+    int i = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return 0;
+    }
+
+    for (i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+        if (vga_args[i].type == IORESOURCE_IO) {
+            vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_ADD_MAPPING);
+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_ADD_MAPPING);
+        }
+
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s mapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+                    vga_args[i].rc);
+            return vga_args[i].rc;
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx
+ */
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
+{
+    int i = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return 0;
+    }
+
+    for (i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+        if (vga_args[i].type == IORESOURCE_IO) {
+            vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_REMOVE_MAPPING);
+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_REMOVE_MAPPING);
+        }
+
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s unmapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+                    vga_args[i].rc);
+            return vga_args[i].rc;
+        }
+    }
+
+    return 0;
+}
+
+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;
+    int ret = 0;
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%d/rom",
+             dev->domain, dev->bus, dev->dev,
+             dev->func);
+
+    if (stat(rom_file, &st)) {
+        return -ENODEV;
+    }
+
+    if (access(rom_file, F_OK)) {
+        XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
+                    rom_file);
+        return -ENODEV;
+    }
+
+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {
+        return -EACCES;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        XEN_PT_LOG("%s\n", "Failed to enable pci-sysfs rom file");
+        ret = -EIO;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    /*
+     * Check if it a real bios extension.
+     * The magic number is 0xAA55.
+     */
+    if (!fread(&magic, sizeof(magic), 1, fp)) {
+        XEN_PT_ERR(NULL, "VGA: can't get magic.\n");
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    if (magic != 0xAA55) {
+        XEN_PT_ERR(NULL, "VGA: wrong magic %x.\n", magic);
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    if (!fread(buf, 1, st.st_size, fp)) {
+        XEN_PT_ERR(NULL, "VGA: pci-assign: Cannot read from host %s", rom_file);
+        XEN_PT_LOG(NULL, "VGA: Device option ROM contents are probably invalid "
+                     "(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)) {
+        ret = -EIO;
+        XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file");
+    }
+    fclose(fp);
+    return ret;
+}
+
+/* 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;
+
+    if (!is_vga_passthrough(dev)) {
+        return rc;
+    }
+
+    bios = g_malloc0(VGA_BIOS_DEFAULT_SIZE);
+
+    bios_size = get_vgabios(bios, dev);

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

+    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 ff76ad4..1c77baa 100644
@@ -1066,6 +1066,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 -0

index a1686ef..c5b572d 100644
@@ -1418,6 +1418,13 @@ static void configure_msg(QemuOpts *opts)
     enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true);
 }
 
+/* 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;
+}
+
 /***********************************************************/
 /* USB devices */
 

@@ -3945,6 +3952,9 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_gfx_passthru:
+                xen_gfx_passthru(optarg);
+                break;
             default:
                 os_parse_cmd_args(popt->index, optarg);
             }

Signature · 1 line

1.9.1

Quoted 5 lines · Tiejun Chen

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

Quoted 5 lines

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

ROM loading code is duplicated from assigned_dev_load_option_rom.

Would it be a problem for you to create a memory region holding the ROM?

You won't need cpu_physical_memory_rw then, either, or need

the VGA_BIOS_DEFAULT_SIZE hack.

Quoted 336 lines

---
v5:

* Just rebase.

v4:

* Fix some typos in the patch head description.
* Improve some comments.
* Given that xen_pt_register_vga_regions()/xen_pt_unregister_vga_regions() are called unconditionally, so we just return 0 there.
* Remove one spurious change.

v3:

* Fix some typos.
* Add more comments to make that readable.
* Improve some return paths.

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     | 232 +++++++++++++++++++++++++++++++++++++++++++
  qemu-options.hx              |   9 ++
  vl.c                         |  10 ++
  8 files changed, 272 insertions(+), 1 deletion(-)

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..dac4238 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 passthrough GFX */
+    if (xen_pt_setup_vga(&s->real_device) < 0) {
+        XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+        xen_host_pci_device_put(&s->real_device);
+        return -1;
+    }
+
      /* 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 +208 -0

new file mode 100644
index 0000000..461e526
@@ -0,0 +1,232 @@
+/*
+ * 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));
+}
+
+typedef struct VGARegion {
+    int type;           /* Memory or port I/O */
+    uint64_t guest_base_addr;
+    uint64_t machine_base_addr;
+    uint64_t size;    /* size of the region */
+    int rc;
+} VGARegion;
+
+#define IORESOURCE_IO           0x00000100
+#define IORESOURCE_MEM          0x00000200
+
+static struct VGARegion vga_args[] = {
+    {
+        .type = IORESOURCE_IO,
+        .guest_base_addr = 0x3B0,
+        .machine_base_addr = 0x3B0,
+        .size = 0xC,
+        .rc = -1,
+    },
+    {
+        .type = IORESOURCE_IO,
+        .guest_base_addr = 0x3C0,
+        .machine_base_addr = 0x3C0,
+        .size = 0x20,
+        .rc = -1,
+    },
+    {
+        .type = IORESOURCE_MEM,
+        .guest_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+        .machine_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+        .size = 0x20,
+        .rc = -1,
+    },
+};
+
+/*
+ * register VGA resources for the domain with assigned gfx
+ */
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
+{
+    int i = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return 0;
+    }
+
+    for (i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+        if (vga_args[i].type == IORESOURCE_IO) {
+            vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_ADD_MAPPING);
+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_ADD_MAPPING);
+        }
+
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s mapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+                    vga_args[i].rc);
+            return vga_args[i].rc;
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx
+ */
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
+{
+    int i = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return 0;
+    }
+
+    for (i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+        if (vga_args[i].type == IORESOURCE_IO) {
+            vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_REMOVE_MAPPING);
+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_REMOVE_MAPPING);
+        }
+
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s unmapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+                    vga_args[i].rc);
+            return vga_args[i].rc;
+        }
+    }
+
+    return 0;
+}
+
+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;
+    int ret = 0;
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%d/rom",
+             dev->domain, dev->bus, dev->dev,
+             dev->func);
+
+    if (stat(rom_file, &st)) {
+        return -ENODEV;
+    }
+
+    if (access(rom_file, F_OK)) {
+        XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
+                    rom_file);
+        return -ENODEV;
+    }
+
+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {
+        return -EACCES;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        XEN_PT_LOG("%s\n", "Failed to enable pci-sysfs rom file");
+        ret = -EIO;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    /*
+     * Check if it a real bios extension.
+     * The magic number is 0xAA55.
+     */
+    if (!fread(&magic, sizeof(magic), 1, fp)) {
+        XEN_PT_ERR(NULL, "VGA: can't get magic.\n");
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    if (magic != 0xAA55) {
+        XEN_PT_ERR(NULL, "VGA: wrong magic %x.\n", magic);
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    if (!fread(buf, 1, st.st_size, fp)) {
+        XEN_PT_ERR(NULL, "VGA: pci-assign: Cannot read from host %s", rom_file);
+        XEN_PT_LOG(NULL, "VGA: Device option ROM contents are probably invalid "
+                     "(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)) {
+        ret = -EIO;
+        XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file");
+    }
+    fclose(fp);
+    return ret;
+}
+
+/* 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;
+
+    if (!is_vga_passthrough(dev)) {
+        return rc;
+    }
+
+    bios = g_malloc0(VGA_BIOS_DEFAULT_SIZE);
+
+    bios_size = get_vgabios(bios, dev);

hw/xen/xen_pt_graphics.c line 218

Quoted 10 lines

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

+    if (bios_size) {
+        XEN_PT_ERR(NULL, "VGA: Error %d getting VBIOS!\n", bios_size);
+        if (bios_size < 0) {
+            XEN_PT_ERR(NULL, "VGA: Error (%s).\n", strerror(bios_size));
+        }
+        rc = -1;
+        goto out;
+    }
+
+    /* Adjust the bios checksum */

It's easy to see that you do this here, but *why* do you do it? That's what the comment should explain.
Do you see many ROMs with an invalid checksum
in the field?

Quoted 64 lines

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

+    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 ff76ad4..1c77baa 100644
@@ -1066,6 +1066,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 a1686ef..c5b572d 100644
@@ -1418,6 +1418,13 @@ static void configure_msg(QemuOpts *opts)
      enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true);
  }
 
+/* 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;
+}
+
  /***********************************************************/
  /* USB devices */
 

@@ -3945,6 +3952,9 @@ int main(int argc, char **argv, char **envp)
                      exit(1);
                  }
                  break;
+            case QEMU_OPTION_gfx_passthru:
+                xen_gfx_passthru(optarg);
+                break;
              default:
                  os_parse_cmd_args(popt->index, optarg);
              }
--

1.9.1

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Tiejun Chen

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

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

Quoted 5 lines

Quoted 3 lines

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

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

Cc Weidong Han <[email protected]>

ROM loading code is duplicated from assigned_dev_load_option_rom.

Yes. Do you hint we need to split this?

Quoted 2 lines

Would it be a problem for you to create a memory region holding the ROM?

Sorry, could you tell me what should do exactly?

Quoted 1 line

You won't need cpu_physical_memory_rw then, either, or need

How to make sure this is fixed at 0xc0000?

Thanks
Tiejun

Quoted 423 lines

the VGA_BIOS_DEFAULT_SIZE hack.

Quoted 336 lines

---
v5:

* Just rebase.

v4:

* Fix some typos in the patch head description.
* Improve some comments.
* Given that xen_pt_register_vga_regions()/xen_pt_unregister_vga_regions() are called unconditionally, so we just return 0 there.
* Remove one spurious change.

v3:

* Fix some typos.
* Add more comments to make that readable.
* Improve some return paths.

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     | 232 +++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx              |   9 ++
 vl.c                         |  10 ++
 8 files changed, 272 insertions(+), 1 deletion(-)
 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..dac4238 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 passthrough GFX */
+    if (xen_pt_setup_vga(&s->real_device) < 0) {
+        XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+        xen_host_pci_device_put(&s->real_device);
+        return -1;
+    }
+
     /* 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 +208 -0

new file mode 100644
index 0000000..461e526
@@ -0,0 +1,232 @@
+/*
+ * 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));
+}
+
+typedef struct VGARegion {
+    int type;           /* Memory or port I/O */
+    uint64_t guest_base_addr;
+    uint64_t machine_base_addr;
+    uint64_t size;    /* size of the region */
+    int rc;
+} VGARegion;
+
+#define IORESOURCE_IO           0x00000100
+#define IORESOURCE_MEM          0x00000200
+
+static struct VGARegion vga_args[] = {
+    {
+        .type = IORESOURCE_IO,
+        .guest_base_addr = 0x3B0,
+        .machine_base_addr = 0x3B0,
+        .size = 0xC,
+        .rc = -1,
+    },
+    {
+        .type = IORESOURCE_IO,
+        .guest_base_addr = 0x3C0,
+        .machine_base_addr = 0x3C0,
+        .size = 0x20,
+        .rc = -1,
+    },
+    {
+        .type = IORESOURCE_MEM,
+        .guest_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+        .machine_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+        .size = 0x20,
+        .rc = -1,
+    },
+};
+
+/*
+ * register VGA resources for the domain with assigned gfx
+ */
+int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
+{
+    int i = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return 0;
+    }
+
+    for (i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+        if (vga_args[i].type == IORESOURCE_IO) {
+            vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_ADD_MAPPING);
+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_ADD_MAPPING);
+        }
+
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s mapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+                    vga_args[i].rc);
+            return vga_args[i].rc;
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * unregister VGA resources for the domain with assigned gfx
+ */
+int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
+{
+    int i = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return 0;
+    }
+
+    for (i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+        if (vga_args[i].type == IORESOURCE_IO) {
+            vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_REMOVE_MAPPING);
+        } else {
+            vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                            vga_args[i].guest_base_addr,
+                            vga_args[i].machine_base_addr,
+                            vga_args[i].size, DPCI_REMOVE_MAPPING);
+        }
+
+        if (vga_args[i].rc) {
+            XEN_PT_ERR(NULL, "VGA %s unmapping failed! (rc: %i)\n",
+                    vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+                    vga_args[i].rc);
+            return vga_args[i].rc;
+        }
+    }
+
+    return 0;
+}
+
+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;
+    int ret = 0;
+
+    snprintf(rom_file, sizeof(rom_file),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%d/rom",
+             dev->domain, dev->bus, dev->dev,
+             dev->func);
+
+    if (stat(rom_file, &st)) {
+        return -ENODEV;
+    }
+
+    if (access(rom_file, F_OK)) {
+        XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
+                    rom_file);
+        return -ENODEV;
+    }
+
+    /* Write "1" to the ROM file to enable it */
+    fp = fopen(rom_file, "r+");
+    if (fp == NULL) {
+        return -EACCES;
+    }
+    val = 1;
+    if (fwrite(&val, 1, 1, fp) != 1) {
+        XEN_PT_LOG("%s\n", "Failed to enable pci-sysfs rom file");
+        ret = -EIO;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    /*
+     * Check if it a real bios extension.
+     * The magic number is 0xAA55.
+     */
+    if (!fread(&magic, sizeof(magic), 1, fp)) {
+        XEN_PT_ERR(NULL, "VGA: can't get magic.\n");
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    if (magic != 0xAA55) {
+        XEN_PT_ERR(NULL, "VGA: wrong magic %x.\n", magic);
+        ret = -ENODEV;
+        goto close_rom;
+    }
+    fseek(fp, 0, SEEK_SET);
+
+    if (!fread(buf, 1, st.st_size, fp)) {
+        XEN_PT_ERR(NULL, "VGA: pci-assign: Cannot read from host %s", rom_file);
+        XEN_PT_LOG(NULL, "VGA: Device option ROM contents are probably invalid "
+                     "(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)) {
+        ret = -EIO;
+        XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file");
+    }
+    fclose(fp);
+    return ret;
+}
+
+/* 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;
+
+    if (!is_vga_passthrough(dev)) {
+        return rc;
+    }
+
+    bios = g_malloc0(VGA_BIOS_DEFAULT_SIZE);
+
+    bios_size = get_vgabios(bios, dev);

hw/xen/xen_pt_graphics.c line 218

Quoted 10 lines

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

+    if (bios_size) {
+        XEN_PT_ERR(NULL, "VGA: Error %d getting VBIOS!\n", bios_size);
+        if (bios_size < 0) {
+            XEN_PT_ERR(NULL, "VGA: Error (%s).\n", strerror(bios_size));
+        }
+        rc = -1;
+        goto out;
+    }
+
+    /* Adjust the bios checksum */

It's easy to see that you do this here, but *why* do you do it? That's what the comment should explain.
Do you see many ROMs with an invalid checksum
in the field?

Quoted 64 lines

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

+    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 ff76ad4..1c77baa 100644
@@ -1066,6 +1066,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 a1686ef..c5b572d 100644
@@ -1418,6 +1418,13 @@ static void configure_msg(QemuOpts *opts)
     enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true);
 }
 
+/* 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;
+}
+
 /***********************************************************/
 /* USB devices */
 

@@ -3945,6 +3952,9 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_gfx_passthru:
+                xen_gfx_passthru(optarg);
+                break;
             default:
                 os_parse_cmd_args(popt->index, optarg);
             }
--

1.9.1

hw/xen/xen_pt_graphics.c +13 -0

+static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
+{
+    struct PCIDevice *dev;
+
+    char rid;
+
+    /* We havt to use a simple PCI device to fake this ISA bridge
+     * to avoid making some confusion to BIOS and ACPI.
+     */
+    dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge");
+
+    qdev_init_nofail(&dev->qdev);
+

Il 25/06/2014 04:17, Tiejun Chen ha scritto:

Quoted 15 lines

+static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)

hw/xen/xen_pt_graphics.c +9 -0

+{
+    struct PCIDevice *dev;
+
+    char rid;
+
+    /* We havt to use a simple PCI device to fake this ISA bridge
+     * to avoid making some confusion to BIOS and ACPI.
+     */
+    dev = pci_create(bus, PCI_DEVFN(0x1f, 0),

"pseudo-intel-pch-isa-bridge");

hw/xen/xen_pt_graphics.c +3 -0

+
+    qdev_init_nofail(&dev->qdev);
+

hw/xen/xen_pt_graphics.c out: · line 293

Quoted 10 lines

hw/xen/xen_pt_graphics.c +10 -0

+    pci_config_set_vendor_id(dev->config, hdev->vendor_id);
+    pci_config_set_device_id(dev->config, hdev->device_id);
+
+    xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1);
+
+    pci_config_set_revision(dev->config, rid);
+
+    XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n");
+    return 0;
+}

This patch doesn't compile on its own (this static function is unused).

pci_create_pch should be moved in this patch.

ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 to make graphics device passthrough work well for VMM, that only need to expose this pseudo ISA bridge to let driver know the real hardware underneath.

The original patch is from Allen Kay <[email protected]>

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

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

Cc Allen Kay <[email protected]>

---
v5:

* Don't set this ISA class property, instead, just fake this ISA bridge with 00:1f.0.

v4:

* Remove some unnecessary "return" in void foo().

v3:

* Fix some typos.
* Improve some return paths.

v2:

* Nothing is changed.

 hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

hw/xen/xen_pt_graphics.c +9 -0

index 461e526..974b7e9 100644
@@ -230,3 +230,64 @@ out:
     g_free(bios);
     return rc;
 }
+
+static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len)
+{
+    return pci_default_read_config(d, addr, len);
+}
+
+static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v,
+                                    int len)
+{

hw/xen/xen_pt_graphics.c +13 -0

+static TypeInfo isa_bridge_info = {
+    .name          = "pseudo-intel-pch-isa-bridge",
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(ISABridgeState),
+    .class_init = isa_bridge_class_init,
+};
+
+static void xen_pt_graphics_register_types(void)
+{
+    type_register_static(&isa_bridge_info);
+}
+
+type_init(xen_pt_graphics_register_types)

hw/xen/xen_pt_graphics.c +6 -0

+
+    pci_config_set_revision(dev->config, rid);
+
+    XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n");
+    return 0;
+}

Signature · 1 line

1.9.1

Quoted 54 lines · Tiejun Chen

ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 to make graphics device passthrough work well for VMM, that only need to expose this pseudo ISA bridge to let driver know the real hardware underneath.

The original patch is from Allen Kay <[email protected]>

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

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

Cc Allen Kay <[email protected]>

---
v5:

* Don't set this ISA class property, instead, just fake this ISA bridge with 00:1f.0.

v4:

* Remove some unnecessary "return" in void foo().

v3:

* Fix some typos.
* Improve some return paths.

v2:

* Nothing is changed.

  hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 61 insertions(+)

hw/xen/xen_pt_graphics.c +14 -0

index 461e526..974b7e9 100644
@@ -230,3 +230,64 @@ out:
      g_free(bios);
      return rc;
  }
+
+static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len)
+{
+    return pci_default_read_config(d, addr, len);
+}
+
+static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v,
+                                    int len)
+{
+    pci_default_write_config(d, addr, v, len);
+}
+
+static void isa_bridge_class_init(ObjectClass *klass, void *data)
+{

hw/xen/xen_pt_graphics.c out: · line 251

Quoted 5 lines

hw/xen/xen_pt_graphics.c +5 -0

+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->config_read = isa_bridge_read_config;
+    k->config_write = isa_bridge_write_config;
+};

You don't need these stubs, just don't fill anything, pci core will use defaults then.

hw/xen/xen_pt_graphics.c out: · line 256

Quoted 6 lines

hw/xen/xen_pt_graphics.c +5 -0

+
+typedef struct {
+    PCIDevice dev;
+} ISABridgeState;
+

Nor do you need an empty structure if it has no state.

Quoted 13 lines

hw/xen/xen_pt_graphics.c +13 -0

+static TypeInfo isa_bridge_info = {
+    .name          = "pseudo-intel-pch-isa-bridge",
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(ISABridgeState),
+    .class_init = isa_bridge_class_init,
+};
+
+static void xen_pt_graphics_register_types(void)
+{
+    type_register_static(&isa_bridge_info);
+}
+
+type_init(xen_pt_graphics_register_types)

hw/xen/xen_pt_graphics.c out: · line 279

Quoted 10 lines

hw/xen/xen_pt_graphics.c +10 -0

+
+static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
+{
+    struct PCIDevice *dev;
+
+    char rid;
+
+    /* We havt to use a simple PCI device to fake this ISA bridge
+     * to avoid making some confusion to BIOS and ACPI.
+     */

A typo and confusing wording above, I'm not really sure what this comment means.
Maybe:

/* Create a fake ISA bridge device at the location expected by guests. */

hw/xen/xen_pt_graphics.c out: · line 287

Quoted 9 lines

hw/xen/xen_pt_graphics.c +8 -0

+    dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge");
+
+    qdev_init_nofail(&dev->qdev);
+
+    pci_config_set_vendor_id(dev->config, hdev->vendor_id);
+    pci_config_set_device_id(dev->config, hdev->device_id);
+
+    xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1);

Host PCI device is the VGA card?

So why does it make sense to get it's vendor/device/revision and stick in the ISA bridge?

Also change rid to uint8_t, you won't need to cast then.

Quoted 8 lines

hw/xen/xen_pt_graphics.c +6 -1

+
+    pci_config_set_revision(dev->config, rid);
+
+    XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n");
+    return 0;
+}
--

1.9.1

Quoted 55 lines · Michael S. Tsirkin

Quoted 54 lines · Tiejun Chen

ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 to make graphics device passthrough work well for VMM, that only need to expose this pseudo ISA bridge to let driver know the real hardware underneath.

The original patch is from Allen Kay <[email protected]>

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

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

Cc Allen Kay <[email protected]>

---
v5:

* Don't set this ISA class property, instead, just fake this ISA bridge with 00:1f.0.

v4:

* Remove some unnecessary "return" in void foo().

v3:

* Fix some typos.
* Improve some return paths.

v2:

* Nothing is changed.

 hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

hw/xen/xen_pt_graphics.c +14 -0

index 461e526..974b7e9 100644
@@ -230,3 +230,64 @@ out:
     g_free(bios);
     return rc;
 }
+
+static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len)
+{
+    return pci_default_read_config(d, addr, len);
+}
+
+static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v,
+                                    int len)
+{
+    pci_default_write_config(d, addr, v, len);
+}
+
+static void isa_bridge_class_init(ObjectClass *klass, void *data)
+{

Quoted 8 lines

hw/xen/xen_pt_graphics.c out: · line 251

Quoted 5 lines

hw/xen/xen_pt_graphics.c +5 -0

+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->config_read = isa_bridge_read_config;
+    k->config_write = isa_bridge_write_config;
+};

You don't need these stubs, just don't fill anything, pci core will use defaults then.

I guess these stubs are left to extend something in the future. But maybe we can remove them now.

Quoted 37 lines

hw/xen/xen_pt_graphics.c out: · line 256

Quoted 6 lines

hw/xen/xen_pt_graphics.c +5 -0

+
+typedef struct {
+    PCIDevice dev;
+} ISABridgeState;
+

Nor do you need an empty structure if it has no state.

Quoted 13 lines

hw/xen/xen_pt_graphics.c +13 -0

+static TypeInfo isa_bridge_info = {
+    .name          = "pseudo-intel-pch-isa-bridge",
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(ISABridgeState),
+    .class_init = isa_bridge_class_init,
+};
+
+static void xen_pt_graphics_register_types(void)
+{
+    type_register_static(&isa_bridge_info);
+}
+
+type_init(xen_pt_graphics_register_types)

hw/xen/xen_pt_graphics.c out: · line 279

Quoted 10 lines

hw/xen/xen_pt_graphics.c +10 -0

+
+static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
+{
+    struct PCIDevice *dev;
+
+    char rid;
+
+    /* We havt to use a simple PCI device to fake this ISA bridge
+     * to avoid making some confusion to BIOS and ACPI.
+     */

A typo and confusing wording above, I'm not really sure what this comment means.
Maybe:

Quoted 2 lines

/* Create a fake ISA bridge device at the location expected by guests. */

Good comments so thanks so much.

Quoted 11 lines

hw/xen/xen_pt_graphics.c out: · line 287

Quoted 9 lines

hw/xen/xen_pt_graphics.c +8 -0

+    dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge");
+
+    qdev_init_nofail(&dev->qdev);
+
+    pci_config_set_vendor_id(dev->config, hdev->vendor_id);
+    pci_config_set_device_id(dev->config, hdev->device_id);
+
+    xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1);

Host PCI device is the VGA card?

This is a real ISA bridge.

Quoted 2 lines

So why does it make sense to get it's vendor/device/revision and stick in the ISA bridge?

The Intel generation of integrated graphics needs to probe this ISA bridge to initialize the i915 driver properly.

Thanks
Tiejun

Quoted 13 lines

Also change rid to uint8_t, you won't need to cast then.

Quoted 8 lines

hw/xen/xen_pt_graphics.c +6 -1

+
+    pci_config_set_revision(dev->config, rid);
+
+    XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n");
+    return 0;
+}
--

1.9.1

Quoted 127 lines · Tiejun

Quoted 55 lines · Michael S. Tsirkin

Quoted 54 lines · Tiejun Chen

ISA bridge is needed since Intel gfx drive will probe with Dev31:Fun0 to make graphics device passthrough work well for VMM, that only need to expose this pseudo ISA bridge to let driver know the real hardware underneath.

The original patch is from Allen Kay <[email protected]>

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

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

Cc Allen Kay <[email protected]>

---
v5:

* Don't set this ISA class property, instead, just fake this ISA bridge with 00:1f.0.

v4:

* Remove some unnecessary "return" in void foo().

v3:

* Fix some typos.
* Improve some return paths.

v2:

* Nothing is changed.

  hw/xen/xen_pt_graphics.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 61 insertions(+)

hw/xen/xen_pt_graphics.c +14 -0

index 461e526..974b7e9 100644
@@ -230,3 +230,64 @@ out:
      g_free(bios);
      return rc;
  }
+
+static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len)
+{
+    return pci_default_read_config(d, addr, len);
+}
+
+static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v,
+                                    int len)
+{
+    pci_default_write_config(d, addr, v, len);
+}
+
+static void isa_bridge_class_init(ObjectClass *klass, void *data)
+{

Quoted 8 lines

hw/xen/xen_pt_graphics.c out: · line 251

Quoted 5 lines

hw/xen/xen_pt_graphics.c +5 -0

+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->config_read = isa_bridge_read_config;
+    k->config_write = isa_bridge_write_config;
+};

You don't need these stubs, just don't fill anything, pci core will use defaults then.

I guess these stubs are left to extend something in the future. But maybe we can remove them now.

Quoted 37 lines

hw/xen/xen_pt_graphics.c out: · line 256

Quoted 6 lines

hw/xen/xen_pt_graphics.c +5 -0

+
+typedef struct {
+    PCIDevice dev;
+} ISABridgeState;
+

Nor do you need an empty structure if it has no state.

Quoted 13 lines

hw/xen/xen_pt_graphics.c +13 -0

+static TypeInfo isa_bridge_info = {
+    .name          = "pseudo-intel-pch-isa-bridge",
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(ISABridgeState),
+    .class_init = isa_bridge_class_init,
+};
+
+static void xen_pt_graphics_register_types(void)
+{
+    type_register_static(&isa_bridge_info);
+}
+
+type_init(xen_pt_graphics_register_types)

hw/xen/xen_pt_graphics.c out: · line 279

Quoted 10 lines

hw/xen/xen_pt_graphics.c +10 -0

+
+static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
+{
+    struct PCIDevice *dev;
+
+    char rid;
+
+    /* We havt to use a simple PCI device to fake this ISA bridge
+     * to avoid making some confusion to BIOS and ACPI.
+     */

A typo and confusing wording above, I'm not really sure what this comment means.
Maybe:

Quoted 2 lines

/* Create a fake ISA bridge device at the location expected by guests. */

Good comments so thanks so much.

Quoted 11 lines

hw/xen/xen_pt_graphics.c out: · line 287

Quoted 9 lines

hw/xen/xen_pt_graphics.c +8 -0

+    dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "pseudo-intel-pch-isa-bridge");
+
+    qdev_init_nofail(&dev->qdev);
+
+    pci_config_set_vendor_id(dev->config, hdev->vendor_id);
+    pci_config_set_device_id(dev->config, hdev->device_id);
+
+    xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1);

Host PCI device is the VGA card?

This is a real ISA bridge.

Quoted 2 lines

So why does it make sense to get it's vendor/device/revision and stick in the ISA bridge?

Quoted 5 lines

The Intel generation of integrated graphics needs to probe this ISA bridge to initialize the i915 driver properly.

Thanks
Tiejun

So vendor/device/revision IDs must match real device then?

Stick this in the comment then.

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 16 lines

Quoted 16 lines

Also change rid to uint8_t, you won't need to cast then.

Quoted 8 lines

hw/xen/xen_pt_graphics.c +6 -1

+
+    pci_config_set_revision(dev->config, rid);
+
+    XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n");
+    return 0;
+}
--

1.9.1

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Yes, sysfs.

Thanks
Tiejun

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Thanks
Tiejun

Quoted 4 lines

Quoted 18 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Quoted 19 lines · Michael S. Tsirkin

Quoted 18 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)

	|
	+ xen_host_pci_config_read(d, pos, buf, len)
		|
		+ pread(d->config_fd, buf, len, pos)

I thinks this should be same as kvm.

Thanks
Tiejun

Quoted 37 lines · Tiejun

Quoted 19 lines · Michael S. Tsirkin

Quoted 18 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)

	|
	+ xen_host_pci_config_read(d, pos, buf, len)
		|
		+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Quoted 45 lines · Michael S. Tsirkin

Quoted 37 lines · Tiejun

Quoted 19 lines · Michael S. Tsirkin

Quoted 18 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)

	|
	+ xen_host_pci_config_read(d, pos, buf, len)
		|
		+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

Quoted 9 lines

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Looks you mean we can unify the entire PT infrastructure between kvm and xen inside qemu. But I'm afraid its not easy to do in a short time, so maybe we can queue this as next phase.

Thanks
Tiejun

Quoted 56 lines · Tiejun

Quoted 54 lines · Michael S. Tsirkin

Quoted 37 lines · Tiejun

Quoted 19 lines · Michael S. Tsirkin

Quoted 18 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)

	|
	+ xen_host_pci_config_read(d, pos, buf, len)
		|
		+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Quoted 6 lines

Looks you mean we can unify the entire PT infrastructure between kvm and xen inside qemu. But I'm afraid its not easy to do in a short time, so maybe we can queue this as next phase.

Thanks
Tiejun

I'm afraid once we merge your code, you'll lose interest :)

At least, don't add duplicate code for ROM.

Quoted 66 lines · Michael S. Tsirkin

Quoted 65 lines · Tiejun

Quoted 48 lines · Michael S. Tsirkin

Quoted 40 lines · Tiejun

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own
wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
|
+ xen_host_pci_config_read(d, pos, buf, len)
|
+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

Quoted 9 lines

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Looks you mean we can unify the entire PT infrastructure between kvm and xen
inside qemu. But I'm afraid its not easy to do in a short time, so maybe we
can queue this as next phase.

Quoted 5 lines

Quoted 2 lines

Thanks
Tiejun

I'm afraid once we merge your code, you'll lose interest :)

Currently we have to push this feature into upstream as our first priority, so unless something is really needed to address. Of course I hope this point what we're talking is not such a thing :)

But I can promise here I'd like to do this optimization with your guide next :)

Quoted 2 lines

At least, don't add duplicate code for ROM.

Let me try this.

Quoted 77 lines · Tiejun

Quoted 66 lines · Michael S. Tsirkin

Quoted 65 lines · Tiejun

Quoted 48 lines · Michael S. Tsirkin

Quoted 40 lines · Tiejun

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own
wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
|
+ xen_host_pci_config_read(d, pos, buf, len)
|
+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

Quoted 9 lines

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Looks you mean we can unify the entire PT infrastructure between kvm and xen
inside qemu. But I'm afraid its not easy to do in a short time, so maybe we
can queue this as next phase.

Quoted 5 lines

Quoted 2 lines

Thanks
Tiejun

I'm afraid once we merge your code, you'll lose interest :)

Currently we have to push this feature into upstream as our first priority, so unless something is really needed to address. Of course I hope this point what we're talking is not such a thing :)

Quoted 8 lines

But I can promise here I'd like to do this optimization with your guide next :)

Quoted 2 lines

At least, don't add duplicate code for ROM.

Let me try this.

Its not easy as expected.

kvm always work with this structure, AssignedDevice, and especially this is just activated in kvm_enabled(). And then set all properties to this structure.

In xen case, the similar structure, XenHostPCIDevice, is not easy transferred into the structure, AssignedDevice. So this mean we have to split assigned_dev_load_option_rom() as line by line for xen and kvm, respectively.

I really agree we definitely need to unify PT infrastructure between kvm and xen after this try since I can't understand why we originally introduce same way to do same thing :(

Do you have better idea? If not, I prefer we open this completely as next action to follow-up. But this time I'm afraid I can't get in this.

Quoted 101 lines · Tiejun

Quoted 77 lines · Tiejun

Quoted 66 lines · Michael S. Tsirkin

Quoted 65 lines · Tiejun

Quoted 48 lines · Michael S. Tsirkin

Quoted 40 lines · Tiejun

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own
wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
|
+ xen_host_pci_config_read(d, pos, buf, len)
|
+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

Quoted 9 lines

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Looks you mean we can unify the entire PT infrastructure between kvm and xen
inside qemu. But I'm afraid its not easy to do in a short time, so maybe we
can queue this as next phase.

Quoted 5 lines

Quoted 2 lines

Thanks
Tiejun

I'm afraid once we merge your code, you'll lose interest :)

Currently we have to push this feature into upstream as our first priority, so unless something is really needed to address. Of course I hope this point what we're talking is not such a thing :)

Quoted 8 lines

But I can promise here I'd like to do this optimization with your guide next :)

Quoted 2 lines

At least, don't add duplicate code for ROM.

Let me try this.

Its not easy as expected.

kvm always work with this structure, AssignedDevice, and especially this is just activated in kvm_enabled(). And then set all properties to this structure.

In xen case, the similar structure, XenHostPCIDevice, is not easy transferred into the structure, AssignedDevice. So this mean we have to split assigned_dev_load_option_rom() as line by line for xen and kvm, respectively.

I really agree we definitely need to unify PT infrastructure between kvm and xen after this try since I can't understand why we originally introduce same way to do same thing :(

Quoted 2 lines

Do you have better idea? If not, I prefer we open this completely as next action to follow-up. But this time I'm afraid I can't get in this.

The Xen PT code came first. At the time Anthony Liguori and I argued for sharing the PT code with KVM going forward but Avi wanted to retain the KVM PT code as is in order not to introduce any regressions on KVM.

Ah.. the good old times :-)

http://lists.xen.org/archives/html/xen-devel/2011-10/msg00195.html

Quoted 97 lines · Tiejun

Quoted 77 lines · Tiejun

Quoted 66 lines · Michael S. Tsirkin

Quoted 65 lines · Tiejun

Quoted 48 lines · Michael S. Tsirkin

Quoted 40 lines · Tiejun

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own
wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
|
+ xen_host_pci_config_read(d, pos, buf, len)
|
+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

Quoted 9 lines

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Looks you mean we can unify the entire PT infrastructure between kvm and xen
inside qemu. But I'm afraid its not easy to do in a short time, so maybe we
can queue this as next phase.

Quoted 5 lines

Quoted 2 lines

Thanks
Tiejun

I'm afraid once we merge your code, you'll lose interest :)

Currently we have to push this feature into upstream as our first priority, so unless something is really needed to address. Of course I hope this point what we're talking is not such a thing :)

Quoted 8 lines

But I can promise here I'd like to do this optimization with your guide next :)

Quoted 2 lines

At least, don't add duplicate code for ROM.

Let me try this.

Its not easy as expected.

kvm always work with this structure, AssignedDevice, and especially this is just activated in kvm_enabled(). And then set all properties to this structure.

In xen case, the similar structure, XenHostPCIDevice, is not easy transferred into the structure, AssignedDevice. So this mean we have to split assigned_dev_load_option_rom() as line by line for xen and kvm, respectively.

Quoted 6 lines

I really agree we definitely need to unify PT infrastructure between kvm and xen after this try since I can't understand why we originally introduce same way to do same thing :(

Do you have better idea? If not, I prefer we open this completely as next action to follow-up. But this time I'm afraid I can't get in this.

The Xen PT code came first. At the time Anthony Liguori and I argued for sharing the PT code with KVM going forward but Avi wanted to retain the KVM PT code as is in order not to introduce any regressions on KVM.

Ah.. the good old times :-)

http://lists.xen.org/archives/html/xen-devel/2011-10/msg00195.html

Quoted 105 lines · Stefano Stabellini

Quoted 97 lines · Tiejun

Quoted 77 lines · Tiejun

Quoted 66 lines · Michael S. Tsirkin

Quoted 65 lines · Tiejun

Quoted 48 lines · Michael S. Tsirkin

Quoted 40 lines · Tiejun

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c
can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own
wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
|
+ xen_host_pci_config_read(d, pos, buf, len)
|
+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

Quoted 9 lines

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Looks you mean we can unify the entire PT infrastructure between kvm and xen
inside qemu. But I'm afraid its not easy to do in a short time, so maybe we
can queue this as next phase.

Quoted 5 lines

Quoted 2 lines

Thanks
Tiejun

I'm afraid once we merge your code, you'll lose interest :)

Currently we have to push this feature into upstream as our first priority, so unless something is really needed to address. Of course I hope this point what we're talking is not such a thing :)

Quoted 8 lines

But I can promise here I'd like to do this optimization with your guide next :)

Quoted 2 lines

At least, don't add duplicate code for ROM.

Let me try this.

Its not easy as expected.

kvm always work with this structure, AssignedDevice, and especially this is just activated in kvm_enabled(). And then set all properties to this structure.

In xen case, the similar structure, XenHostPCIDevice, is not easy transferred into the structure, AssignedDevice. So this mean we have to split assigned_dev_load_option_rom() as line by line for xen and kvm, respectively.

Quoted 6 lines

I really agree we definitely need to unify PT infrastructure between kvm and xen after this try since I can't understand why we originally introduce same way to do same thing :(

Do you have better idea? If not, I prefer we open this completely as next action to follow-up. But this time I'm afraid I can't get in this.

Quoted 7 lines

The Xen PT code came first. At the time Anthony Liguori and I argued for sharing the PT code with KVM going forward but Avi wanted to retain the KVM PT code as is in order not to introduce any regressions on KVM.

Ah.. the good old times :-)

http://lists.xen.org/archives/html/xen-devel/2011-10/msg00195.html

Right, I remember that there was an idea to first make the code reusable before merging either Xen or KVM PT, but in the end the plan to merge both and then abstract it out, won. We never got to abstracting it out but we should!

Yet, I do not think we should require abstracting all of Xen PT as a pre-requisite for this work, but I think we can
avoid adding to the duplication without a lot of effort. To start sharing with ROM code, I think we should rename AssignedDevice->KvmAssignedDevice, and move fields that can be shared with Xen from there to AssignedDevice.

Quoted 119 lines · Michael S. Tsirkin

Quoted 105 lines · Stefano Stabellini

Quoted 97 lines · Tiejun

Quoted 77 lines · Tiejun

Quoted 66 lines · Michael S. Tsirkin

Quoted 65 lines · Tiejun

Quoted 48 lines · Michael S. Tsirkin

Quoted 40 lines · Tiejun

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c
can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own
wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
|
+ xen_host_pci_config_read(d, pos, buf, len)
|
+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

Quoted 9 lines

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Looks you mean we can unify the entire PT infrastructure between kvm and xen
inside qemu. But I'm afraid its not easy to do in a short time, so maybe we
can queue this as next phase.

Quoted 5 lines

Quoted 2 lines

Thanks
Tiejun

I'm afraid once we merge your code, you'll lose interest :)

Currently we have to push this feature into upstream as our first priority, so unless something is really needed to address. Of course I hope this point what we're talking is not such a thing :)

Quoted 8 lines

But I can promise here I'd like to do this optimization with your guide next :)

Quoted 2 lines

At least, don't add duplicate code for ROM.

Let me try this.

Its not easy as expected.

kvm always work with this structure, AssignedDevice, and especially this is just activated in kvm_enabled(). And then set all properties to this structure.

In xen case, the similar structure, XenHostPCIDevice, is not easy transferred into the structure, AssignedDevice. So this mean we have to split assigned_dev_load_option_rom() as line by line for xen and kvm, respectively.

Quoted 6 lines

I really agree we definitely need to unify PT infrastructure between kvm and xen after this try since I can't understand why we originally introduce same way to do same thing :(

Do you have better idea? If not, I prefer we open this completely as next action to follow-up. But this time I'm afraid I can't get in this.

Quoted 7 lines

The Xen PT code came first. At the time Anthony Liguori and I argued for sharing the PT code with KVM going forward but Avi wanted to retain the KVM PT code as is in order not to introduce any regressions on KVM.

Ah.. the good old times :-)

http://lists.xen.org/archives/html/xen-devel/2011-10/msg00195.html

Right, I remember that there was an idea to first make the code reusable before merging either Xen or KVM PT, but in the end the plan to merge both and then abstract it out, won. We never got to abstracting it out but we should!

Quoted 6 lines

Yet, I do not think we should require abstracting all of Xen PT as a pre-requisite for this work, but I think we can
avoid adding to the duplication without a lot of effort. To start sharing with ROM code, I think we should rename AssignedDevice->KvmAssignedDevice, and move fields that can be shared with Xen from there to AssignedDevice.

Its not simple to share something between AssignedDevice and the similar structure, XenHostPCIDevice.

As I said previously AssignedDevice is just activated in kvm_enabled(), then set all properties to this structure.

But kvm_enabled() is zero since we always disable this in xen scenario. So this means we have to invoke pci-assign framework completely if we try to force to use this. So actually this would be equal to merge kvm and xen.

Or if you already figure out to avoid this happened let me know.

Thanks
Tiejun

Quoted 109 lines · Stefano Stabellini

Quoted 97 lines · Tiejun

Quoted 77 lines · Tiejun

Quoted 66 lines · Michael S. Tsirkin

Quoted 65 lines · Tiejun

Quoted 48 lines · Michael S. Tsirkin

Quoted 40 lines · Tiejun

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Tiejun

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Tiejun

Quoted 3 lines

In fact it's exactly what passthrough does.
I wonder if more bits from ./hw/i386/kvm/pci-assign.c
can be reused. How do you poke at the host device? sysfs?

Quoted 8 lines

Quoted 4 lines

Yes, sysfs.

Thanks
Tiejun

Then you should be able to re-use large chunks of
./hw/i386/kvm/pci-assign.c: basically everything
that deals with emulation.

Do you mean those hooks to get info from the real device? Xen have its own
wrapper, xen_host_pci_get_block(), so we always go there in xen scenario.

Quoted 8 lines

Quoted 2 lines

Thanks
Tiejun

Yes and that's not good. We have two pieces of code doing mostly identical things slightly differently. hw/i386/kvm/pci-assign.c is a bit younger so it's cleaner, but these really need to be unified.

Sorry, take a look at this again,

xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int len)
|
+ xen_host_pci_config_read(d, pos, buf, len)
|
+ pread(d->config_fd, buf, len, pos)

Quoted 4 lines

I thinks this should be same as kvm.

Thanks
Tiejun

get_block is trivial.

Quoted 9 lines

I really mean the whole PT infrastructure for
- discovering host devices through sysfs
- virtualizing devices

rom, bars, msi ...
the list goes on.

logic is mostly the same.

Looks you mean we can unify the entire PT infrastructure between kvm and xen
inside qemu. But I'm afraid its not easy to do in a short time, so maybe we
can queue this as next phase.

Quoted 5 lines

Quoted 2 lines

Thanks
Tiejun

I'm afraid once we merge your code, you'll lose interest :)

Currently we have to push this feature into upstream as our first priority, so unless something is really needed to address. Of course I hope this point what we're talking is not such a thing :)

Quoted 8 lines

But I can promise here I'd like to do this optimization with your guide next :)

Quoted 2 lines

At least, don't add duplicate code for ROM.

Let me try this.

Its not easy as expected.

kvm always work with this structure, AssignedDevice, and especially this is just activated in kvm_enabled(). And then set all properties to this structure.

In xen case, the similar structure, XenHostPCIDevice, is not easy transferred into the structure, AssignedDevice. So this mean we have to split assigned_dev_load_option_rom() as line by line for xen and kvm, respectively.

Quoted 6 lines

I really agree we definitely need to unify PT infrastructure between kvm and xen after this try since I can't understand why we originally introduce same way to do same thing :(

Do you have better idea? If not, I prefer we open this completely as next action to follow-up. But this time I'm afraid I can't get in this.

The Xen PT code came first. At the time Anthony Liguori and I argued for sharing the PT code with KVM going forward but Avi wanted to retain the KVM PT code as is in order not to introduce any regressions on KVM.

Quoted 3 lines

Ah.. the good old times :-)

http://lists.xen.org/archives/html/xen-devel/2011-10/msg00195.html

Right, I remember that there was an idea to first make the code reusable before merging either Xen or KVM PT, but in the end the plan to merge both and then abstract it out, won. We never got to abstracting it out but we should!

Yet, I do not think we should require abstracting all of Xen PT as a pre-requisite for this work, but I think we can
avoid adding to the duplication without a lot of effort.

To start sharing with ROM code, I think we should rename AssignedDevice->KvmAssignedDevice, and move fields that can be shared with Xen from there to AssignedDevice.

Signature · 1 line

MST

Il 25/06/2014 04:17, Tiejun Chen ha scritto:

hw/xen/xen_pt_graphics.c static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) · line 304

Quoted 9 lines

hw/xen/xen_pt_graphics.c +9 -0

+int pci_create_pch(PCIBus *bus)
+{
+    XenHostPCIDevice hdev;
+    int r = 0;
+
+    if (!xen_has_gfx_passthru) {
+        return r;
+    }
+

You could make this an assertion, since the function is never called with xen_has_gfx_passthru == 0. Or just drop the check completely, so

the function can be moved in patch 2.

Paolo

Some registers of Intel IGD are mapped in host bridge, so it needs to passthrough these registers of physical host bridge to guest because emulated host bridge in guest doesn't have these mappings.

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

---
v5:

* Don't pass vendor/device ids in igd_pci_read().
* Add to support offset 0x44/0x48.

v4:

* Given that pci_create_pch() is called unconditionally, so we just return 0 even if its failed to check xen_has_gfx_passthru.
* Remove one spurious change.

v3:

* Improve comments to make that readable.

v2:

* To introduce is_igd_passthrough() to make sure we touch physical host bridge only in IGD case.

 hw/xen/xen_pt.h          |   4 ++
 hw/xen/xen_pt_graphics.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 160 insertions(+)

hw/xen/xen_pt.h +4 -0

index 4d3a18d..507165c 100644
@@ -302,5 +302,9 @@ 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);
+int pci_create_pch(PCIBus *bus);
+void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr,
+                   uint32_t val, int len);
+uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len);
 
 #endif /* !XEN_PT_H */

hw/xen/xen_pt_graphics.c +1 -0

index 974b7e9..f3fbfed 100644
@@ -4,6 +4,7 @@
 #include "xen_pt.h"
 #include "xen-host-pci-device.h"
 #include "hw/xen/xen_backend.h"
+#include "hw/pci/pci_bus.h"
 
 static int is_vga_passthrough(XenHostPCIDevice *dev)
 {

hw/xen/xen_pt_graphics.c +11 -0

+{
+    XenHostPCIDevice hdev;
+    int r = 0;
+
+    if (!xen_has_gfx_passthru) {
+        return r;
+    }
+
+    r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0);
+    if (r) {
+        XEN_PT_ERR(NULL, "Failed to find Intel PCH on host\n");

hw/xen/xen_pt_graphics.c +4 -0

+static int is_igd_passthrough(PCIDevice *pci_dev)
+{
+    PCIDevice *f = pci_dev->bus->devices[PCI_DEVFN(2, 0)];
+    if (pci_dev->bus->devices[PCI_DEVFN(2, 0)]) {

hw/xen/xen_pt_graphics.c +19 -0

+     */
+
+    assert(pci_dev->devfn == 0x00);
+
+    if (!is_igd_passthrough(pci_dev)) {
+        goto write_default;
+    }
+
+    /* Just work for the i915 driver. */
+    switch (config_addr) {
+    case 0x58:      /* PAVPC Offset */
+        break;
+    default:
+        /* Just sets the emulated values. */
+        goto write_default;
+    }
+
+    /* Host write */
+    r = xen_host_pci_device_get(&dev, 0, 0, 0, 0);

hw/xen/xen_pt_graphics.c +21 -0

+
+    xen_host_pci_device_put(&dev);
+
+    return;
+
+write_default:
+    pci_default_write_config(pci_dev, config_addr, val, len);
+}
+
+uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
+{
+    XenHostPCIDevice dev;
+    uint32_t val;
+    int r;
+
+    /* IGD read/write is through the host bridge.
+     * ISA bridge is only for detect purpose. In i915 driver it will
+     * probe ISA bridge to discover the IGD, see comment in i915_drv.c:
+     * intel_detect_pch().
+     */
+    assert(pci_dev->devfn == 0x00);

hw/xen/xen_pt_graphics.c +25 -0

+        goto read_default;
+    }
+
+    /* Host read */
+    r = xen_host_pci_device_get(&dev, 0, 0, 0, 0);
+    if (r) {
+        goto err_out;
+    }
+
+    r = xen_host_pci_get_block(&dev, config_addr, (uint8_t *)&val, len);
+    if (r) {
+        goto err_out;
+    }
+
+    xen_host_pci_device_put(&dev);
+
+    return val;
+
+read_default:
+    return pci_default_read_config(pci_dev, config_addr, len);
+
+err_out:
+    XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
+    return -1;
+}

Signature · 1 line

1.9.1

Quoted 1 line · Tiejun Chen

Some registers of Intel IGD are mapped in host bridge, so it needs to

Quoted 4 lines

  static int is_vga_passthrough(XenHostPCIDevice *dev)
  {

hw/xen/xen_pt_graphics.c +0 -0

@@ -291,3 +292,158 @@ static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
      XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n");

hw/xen/xen_pt_graphics.c static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) · line 296

Quoted 4 lines

hw/xen/xen_pt_graphics.c +2 -0

      return 0;
  }
+
+int pci_create_pch(PCIBus *bus)

Please prefix all xen specific non static functions with xen_ or something like this.

pci_ is for pci core.

In fact it's a good idea to do this for static functions as well, in case we add a conflicting function in some header.

Quoted 15 lines

hw/xen/xen_pt_graphics.c +15 -0

+{
+    XenHostPCIDevice hdev;
+    int r = 0;
+
+    if (!xen_has_gfx_passthru) {
+        return r;
+    }
+
+    r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0);
+    if (r) {
+        XEN_PT_ERR(NULL, "Failed to find Intel PCH on host\n");
+        goto err;
+    }
+
+    if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) {

hw/xen/xen_pt_graphics.c static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) · line 317

Quoted 6 lines

hw/xen/xen_pt_graphics.c +6 -0

+        r = create_pseudo_pch_isa_bridge(bus, &hdev);
+        if (r) {
+            XEN_PT_ERR(NULL, "Failed to create PCH ISA bridge.\n");
+            goto err;
+        }
+    }

Does it work on non intel?

It seems to return success.

Maybe you should just verify that vendor and device ID have the expected values on the host, and

fail otherwise.

hw/xen/xen_pt_graphics.c static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) · line 330

Quoted 13 lines

hw/xen/xen_pt_graphics.c +13 -0

+
+    xen_host_pci_device_put(&hdev);
+
+err:
+    return r;
+}
+
+/*
+ * Currently we just pass this physical host bridge for IGD, 00:02.0.
+ *
+ * Here pci_dev is just that host bridge, so we have to get that real
+ * passthrough device by that given devfn to further confirm.
+ */

confirm what?

Comments like this need to document what function does.

Maybe

/* Can we support IGD passthrough for this device?
* We require ... <XYZ - fill in here>
*/

Quoted 4 lines

hw/xen/xen_pt_graphics.c +4 -0

+static int is_igd_passthrough(PCIDevice *pci_dev)
+{
+    PCIDevice *f = pci_dev->bus->devices[PCI_DEVFN(2, 0)];
+    if (pci_dev->bus->devices[PCI_DEVFN(2, 0)]) {

hw/xen/xen_pt_graphics.c static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) · line 344

Quoted 10 lines

hw/xen/xen_pt_graphics.c +10 -0

+        XenPCIPassthroughState *s = DO_UPCAST(XenPCIPassthroughState, dev, f);
+        return (is_vga_passthrough(&s->real_device)
+                    && (s->real_device.vendor_id == PCI_VENDOR_ID_INTEL));
+    } else {
+        return 0;
+    }
+}
+
+void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr,
+                   uint32_t val, int len)

Same here, xen_ everywhere please.

hw/xen/xen_pt_graphics.c static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) · line 352

Quoted 9 lines

hw/xen/xen_pt_graphics.c +8 -0

+{
+    XenHostPCIDevice dev;
+    int r;
+
+    /* IGD read/write is through the host bridge.
+     * ISA bridge is only for detect purpose. In i915 driver it will
+     * probe ISA bridge to discover the IGD, see comment in i915_drv.c:
+     * intel_detect_pch().

You mean in linux kernel I guess?

Quoted 20 lines

hw/xen/xen_pt_graphics.c +19 -0

+     */
+
+    assert(pci_dev->devfn == 0x00);
+
+    if (!is_igd_passthrough(pci_dev)) {
+        goto write_default;
+    }
+
+    /* Just work for the i915 driver. */
+    switch (config_addr) {
+    case 0x58:      /* PAVPC Offset */
+        break;
+    default:
+        /* Just sets the emulated values. */
+        goto write_default;
+    }
+
+    /* Host write */
+    r = xen_host_pci_device_get(&dev, 0, 0, 0, 0);

hw/xen/xen_pt_graphics.c static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) · line 381

Quoted 10 lines

hw/xen/xen_pt_graphics.c +10 -0

+    if (r) {
+        XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
+        abort();
+    }
+
+    r = xen_host_pci_set_block(&dev, config_addr, (uint8_t *)&val, len);
+    if (r) {
+        XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
+        abort();
+    }

Cleaner: