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