[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapp
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping |
Date: |
Wed, 28 May 2014 13:23:37 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
On Wed, 28 May 2014, Chen, Tiejun wrote:
> > -----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;
> }
> ...
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 <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
> ...
- Re: [Qemu-devel] [Xen-devel] [v3][PATCH 2/5] xen, gfx passthrough: create intel isa bridge, (continued)
- [Qemu-devel] [v3][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D, Tiejun Chen, 2014/05/26
- Re: [Qemu-devel] [v3][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D, Stefano Stabellini, 2014/05/27
- Re: [Qemu-devel] [v3][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D, Chen, Tiejun, 2014/05/27
- Re: [Qemu-devel] [v3][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D, Stefano Stabellini, 2014/05/28
- Re: [Qemu-devel] [v3][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D, Chen, Tiejun, 2014/05/28
[Qemu-devel] [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping, Tiejun Chen, 2014/05/26
- Re: [Qemu-devel] [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping, Stefano Stabellini, 2014/05/27
- Re: [Qemu-devel] [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping, Chen, Tiejun, 2014/05/27
- Re: [Qemu-devel] [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping,
Stefano Stabellini <=
- Re: [Qemu-devel] [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping, Chen, Tiejun, 2014/05/28
[Qemu-devel] [v3][PATCH 4/5] xen, gfx passthrough: create host bridge to passthrough, Tiejun Chen, 2014/05/26
Re: [Qemu-devel] [v3][PATCH 0/5] xen: add Intel IGD passthrough support, Chen, Tiejun, 2014/05/27