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

["Michael S. Tsirkin" <mst@redhat.com>]

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.


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


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