[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/29 19:43, Michael S. Tsirkin wrote:
On Fri, Jun 27, 2014 at 05:22:18PM +0800, Chen, Tiejun wrote:
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.

They are?  Where in the PCI spec does it say 48?
Same for PCI_INTEL_OPREGION.

Why should we add such a prefix?

So that people working on core pci do not have to worry about breaking
your devices by adding a symbol in the global header.

Okay.






[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.

So code is clean and reusable. Copy and paste is a fact of life,
you don't want people to inherit bugs.

Understood.

If some code absolutely must be LE specific,
it needs a comment that explains this and cautions
people against trying to use it elsewhere in QEMU.

I think its fine enough to add a comment.

Thanks
Tiejun



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