|
From: | Chen, Tiejun |
Subject: | Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping |
Date: | Fri, 27 Jun 2014 17:22:18 +0800 |
User-agent: | Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 |
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 0xfcXEN_.... 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
[Prev in Thread] | Current Thread | [Next in Thread] |