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

Re: [Xen-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping



On 2014/6/25 15:13, Michael S. Tsirkin wrote:
On Wed, Jun 25, 2014 at 10:17:21AM +0800, Tiejun Chen wrote:

[snip]

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 507165c..25147cf 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -63,7 +63,7 @@ typedef int (*xen_pt_conf_byte_read)
  #define XEN_PT_BAR_UNMAPPED (-1)

  #define PCI_CAP_MAX 48
-
+#define PCI_INTEL_OPREGION 0xfc


XEN_.... please

PCI_CAP_MAX should be fixed too.

They are specific to PCI, not XEN. Why should we add such a prefix?




[snip]


+    if (igd_guest_opregion) {
+        ret = xc_domain_memory_mapping(xen_xc, xen_domid,
+                (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT),
+                (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),

don't spread casts all around.
Should be a last resort.

Okay.


+                3,
+                DPCI_REMOVE_MAPPING);
+        if (ret) {
+            return ret;
+        }
+    }
+
      return 0;
  }

@@ -447,3 +462,52 @@ err_out:
      XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
      return -1;
  }
+
+uint32_t igd_read_opregion(XenPCIPassthroughState *s)
+{
+    uint32_t val = 0;
+
+    if (igd_guest_opregion == 0) {

!igd_guest_opregion is shorter and does the same,

Okay.


+        return val;
+    }
+
+    val = igd_guest_opregion;
+
+    XEN_PT_LOG(&s->dev, "Read opregion val=%x\n", val);
+    return val;
+}
+
+void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val)
+{
+    int ret;
+
+    if (igd_guest_opregion) {
+        XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring 
%x\n",
+                   val);
+        return;
+    }
+
+    xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION,
+            (uint8_t *)&igd_host_opregion, 4);
+    igd_guest_opregion = (unsigned long)(val & ~0xfff)
+                            | (igd_host_opregion & 0xfff);
+

Clearly broken on BE.

I still can't understand why we need to address this in BE case.

Maybe not important here but writing clean code is
just as easy.
uint8_t igd_host_opregion[4];

...

     xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION,
                           igd_host_opregion, sizeof igd_host_opregion);

     igd_guest_opregion = (val & ~0xfff) |
            (pci_get_word(igd_host_opregion) & 0xfff);

0xfff should be a macro too to avoid duplication.


Okay.

Thanks
Tiejun

_______________________________________________
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®.