[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [v5][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D



On 2014/6/25 15:04, Michael S. Tsirkin wrote:
On Wed, Jun 25, 2014 at 10:17:19AM +0800, Tiejun Chen wrote:
Some registers of Intel IGD are mapped in host bridge, so it needs to

[snip]

  static int is_vga_passthrough(XenHostPCIDevice *dev)
  {
@@ -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");
      return 0;
  }
+
+int pci_create_pch(PCIBus *bus)


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

Okay.

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.

+{
+    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) {
+        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?

IGD means this should work on Intel platform.

It seems to return success.

Okay, I'd like to change this a void.

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

Vendor id is enough.

fail otherwise.

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

So change like:

* passthrough device by that given devfn to avoid other devices access.

Comments like this need to document what function does.

Maybe

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

+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)]) {
+        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.

Okay.


+{
+    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?

So change like,

* probe ISA bridge to discover the IGD, see comment in Linux:i915_drv.c:


+     */
+
+    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);
+    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:

        if (config_addr == 0x58) {

Maybe we add other offset in the future, so we'd better keep in them in switch().

             /* Host write */
             r = xen_host_pci_device_get(&dev, 0, 0, 0, 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();
             }
        }

Note this does not work on e.g. BE.

Why do we need take BE into consideration here? Shouldn't PCI already be LE?

The best way is really to make the register writeable in wmask.
Then
     pci_default_write_config(pci_dev, config_addr, val, len);
     if (range_covers_byte(addr, len, 0x58)) {
                ....

             r = xen_host_pci_set_block(&dev, config_addr,
                 pci_dev->config + config_addr, len);
     }





+
+    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);
+
+    if (!is_igd_passthrough(pci_dev)) {
+        goto read_default;
+    }
+
+    /* Just work for the i915 driver. */
+    switch (config_addr) {
+    case 0x08:        /* revision id */
+    case 0x2c:        /* sybsystem vendor id */
+    case 0x2e:        /* sybsystem id */

Since you set them to match host previously,
should be no need to override now.

Just *read* the host bridge, no *write* here.


+    case 0x44:        /* MCHBAR I915 */
+    case 0x48:        /* MCHBAR I965 */
+    case 0x50:        /* SNB: processor graphics control register */
+    case 0x52:        /* processor graphics control register */
+    case 0xa0:        /* top of memory */
+    case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
+    case 0x58:        /* SNB: PAVPC Offset */
+    case 0xa4:        /* SNB: graphics base of stolen memory */
+    case 0xa8:        /* SNB: base of GTT stolen memory */

Move the actual code here.


Sorry, whats the actual code? Are you saying we can read all values from the host bridge in advance then restore them here?

Thanks
Tiejun

+        break;
+    default:
+        /* Just gets the emulated values. */

and here.

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.