> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabell...@eu.citrix.com]
> Sent: Wednesday, May 28, 2014 1:36 AM
> To: Chen, Tiejun
> Cc: anthony.per...@citrix.com; stefano.stabell...@eu.citrix.com;
> m...@redhat.com; kelly.zyta...@amd.com; qemu-devel@nongnu.org;
> xen-de...@lists.xensource.com; peter.mayd...@linaro.org;
> anth...@codemonkey.ws; Kay, Allen M; Zhang, Yang Z
> Subject: Re: [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping
>
> On Mon, 26 May 2014, Tiejun Chen wrote:
> > The OpRegion shouldn't be mapped 1:1 because the address in the host
> > can't be used in the guest directly.
> >
> > This patch traps read and write access to the opregion of the Intel
> > GPU config space (offset 0xfc).
> >
> > The original patch is from Jean Guyader <jean.guya...@eu.citrix.com>
> >
> > Signed-off-by: Yang Zhang <yang.z.zh...@intel.com>
> > Signed-off-by: Tiejun Chen <tiejun.c...@intel.com>
> > Cc: Jean Guyader <jean.guya...@eu.citrix.com>
> > ---
> > v3:
> >
> > * Fix some typos.
> > * Add more comments to make that readable.
> > * To unmap igd_opregion when call xen_pt_unregister_vga_regions().
> > * Improve some return paths.
> > * We need to map 3 pages for opregion as hvmloader set.
> > * Force to convert igd_guest/host_opoegion as an unsigned long type
> > while calling xc_domain_memory_mapping().
> >
> > v2:
> >
> > * We should return zero as an invalid address value while calling
> > igd_read_opregion().
> >
[snip]
> > +
> > +uint32_t igd_read_opregion(XenPCIPassthroughState *s) {
> > + uint32_t val = 0;
> > +
> > + if (igd_guest_opregion == 0) {
> > + return val;
>
> Sorry for not having spotted it before, but isn't this supposed to return an
> error?
> The old code returns -1 in this case.
I already commented this in v2 log above.
We shouldn't return "-1" to indicate an invalid address since we often think
"!0" is a valid address value.
In Linux instance,
drivers/gpu/drm/i915/intel_opregion.c:
int intel_opregion_setup(struct drm_device *dev)
{
...
pci_read_config_dword(dev->pdev, PCI_ASLS, &asls);
DRM_DEBUG_DRIVER("graphic opregion physical addr: 0x%x\n", asls);
if (asls == 0) {
DRM_DEBUG_DRIVER("ACPI OpRegion not supported!\n");
return -ENOTSUPP;
}
...
>
>
> > + }
> > +
> > + 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);
> > +
> > + 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),
> > + 3,
>
> Why 3 pages instead of 2?
commit 408a9e56343b006c9e58a334f0b97dd2deedf9ac
Author: Keir Fraser <k...@xen.org>
Date: Thu Jan 10 17:26:24 2013 +0000
hvmloader: Allocate 3 pages for Intel GPU OpRegion passthrough.
The 8kB region may not be page aligned, hence requiring 3 pages to
be mapped through.
Signed-off-by: Keir Fraser <k...@xen.org>
diff --git a/tools/firmware/hvmloader/config.h
b/tools/firmware/hvmloader/config.h
index 3a4e145..7f8a90f 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -5,7 +5,9 @@
enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt };
extern enum virtual_vga virtual_vga;
+
extern unsigned long igd_opregion_pgbase;
+#define IGD_OPREGION_PAGES 3
...
Thanks
Tiejun
