| Message ID | CCCF29FA0F52DC4B8E2D1EF38AE07FF7B449F3@SHSMSX101.ccr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 28 May 2014, Chen, Tiejun wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > Sent: Wednesday, May 28, 2014 1:36 AM > > To: Chen, Tiejun > > Cc: anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com; > > mst@redhat.com; Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; > > xen-devel@lists.xensource.com; peter.maydell@linaro.org; > > anthony@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.guyader@eu.citrix.com> > > > > > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com> > > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > > > Cc: Jean Guyader <jean.guyader@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; > } > ... 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@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 <keir@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 > ...
> -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > Sent: Wednesday, May 28, 2014 8:24 PM > To: Chen, Tiejun > Cc: Stefano Stabellini; anthony.perard@citrix.com; mst@redhat.com; > Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; > xen-devel@lists.xensource.com; peter.maydell@linaro.org; > anthony@codemonkey.ws; Kay, Allen M; Zhang, Yang Z > Subject: RE: [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping > > On Wed, 28 May 2014, Chen, Tiejun wrote: > > > -----Original Message----- > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > > Sent: Wednesday, May 28, 2014 1:36 AM > > > To: Chen, Tiejun > > > Cc: anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com; > > > mst@redhat.com; Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; > > > xen-devel@lists.xensource.com; peter.maydell@linaro.org; > > > anthony@codemonkey.ws; Kay, Allen M; Zhang, Yang Z > > > Subject: Re: [v3][PATCH 5/5] xen, gfx passthrough: add opregion > > > mapping > > > [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. > I don't think so. As I understand, we does check if that return value is valid, but not check if this read behavior is good. This pci read transmit should be guaranteed by the hardware, and this should be transparent to driver. In qemu emulator, qemu should do this check on behavior of the hardware so we need an error code when use xen_pt_intel_oprerion() as a wrapper of igd_read_opregion(). But that return value we're talking about should be delivered directly if this pci read behavior is fine. But even if this read is failed, we should do something in pci host controller of qemu to further emulate this issue. But as emulator, I think qemu often never do this thing just specific to a normal pci read if you have no any obvious reason. Thanks Tiejun
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