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

[Chen, Tiejun]

> -----Original Message-----
> From: Stefano Stabellini [mailto:address@hidden
> Sent: Wednesday, May 28, 2014 1:36 AM
> To: Chen, Tiejun
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; 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 <address@hidden>
> >
> > Signed-off-by: Yang Zhang <address@hidden>
> > Signed-off-by: Tiejun Chen <address@hidden>
> > Cc: Jean Guyader <address@hidden>
> > ---
> > 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 <address@hidden>
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 <address@hidden>
 
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