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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 28 May 2014, Chen, Tiejun wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@xxxxxxxxxxxxx]
> > Sent: Wednesday, May 28, 2014 1:36 AM
> > To: Chen, Tiejun
> > Cc: anthony.perard@xxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> > mst@xxxxxxxxxx; Kelly.Zytaruk@xxxxxxx; qemu-devel@xxxxxxxxxx;
> > xen-devel@xxxxxxxxxxxxxxxxxxx; peter.maydell@xxxxxxxxxx;
> > anthony@xxxxxxxxxxxxx; 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.guyader@xxxxxxxxxxxxx>
> > >
> > > Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
> > > Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> > > Cc: Jean Guyader <jean.guyader@xxxxxxxxxxxxx>
> > > ---
> > > 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;
>     }
>       ...

Ops, you are right! igd_read_opregion returns an address not an error
code.
In that case, given that xen_pt_intel_opregion_read can return an error
code as well as an address, maybe it makes sense to do the same in
igd_read_opregion and allow the function to return an error code and set
the value by pointer.


> > 
> > 
> > > +    }
> > > +
> > > +    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?

Thanks.


> commit 408a9e56343b006c9e58a334f0b97dd2deedf9ac
> Author: Keir Fraser <keir@xxxxxxx>
> 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 <keir@xxxxxxx>
>  
> 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
> ...

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel