From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41141) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQZMY-0008K7-MA for qemu-devel@nongnu.org; Tue, 02 Feb 2016 06:43:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQZMV-0005Vg-ER for qemu-devel@nongnu.org; Tue, 02 Feb 2016 06:43:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46108) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQZMV-0005VU-73 for qemu-devel@nongnu.org; Tue, 02 Feb 2016 06:43:11 -0500 References: <20160202041510.32092.58972.stgit@gimli.home> From: Laszlo Ersek Message-ID: <56B0964C.5000708@redhat.com> Date: Tue, 2 Feb 2016 12:43:08 +0100 MIME-Version: 1.0 In-Reply-To: <20160202041510.32092.58972.stgit@gimli.home> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [SeaBIOS] [RFC PATCH] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson , seabios@seabios.org Cc: qemu-devel@nongnu.org, kraxel@redhat.com On 02/02/16 05:15, Alex Williamson wrote: > The proposed IGD OpRegion support in QEMU via vfio maps the host > OpRegion into VM system memory at the address written to the ASL > Storage register (0xFC). The OpRegion contains a 16-byte signature > followed by a 4-byte size field. Therefore SeaBIOS can allocate a > buffer of the typical size (8KB), this results in a matching e820 > reserved entry for the range, then write the buffer address to the ASL > Storage register, blinking the OpRegion into the VM address space. At > that point SeaBIOS can validate the signature and size and remap if > necessary. If the OpRegion is larger than 8KB, this would result in > any conflicting ranges being temporarily mapped with the OpRegion, > which probably needs further discussion on what that could break. > Other options might be to use the same algorithm with an obscenely > sized initial buffer to make sure we do not overlap, always > re-allocating the proper sized buffer, or perhaps we could pass the > OpRegion itself or information about the OpRegion through fw_cfg. > > With the posted kernel series[1] and QEMU series[2] (on top of Gerd's > IGD patches[3]), this makes the OpRegion available to the VM and > tracing shows that the guest driver does access it. > > [1] https://lkml.org/lkml/2016/2/1/884 > [2] https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00202.html > [3] https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00244.html > > Signed-off-by: Alex Williamson > --- > src/fw/pciinit.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > index c31c2fa..4f3251e 100644 > --- a/src/fw/pciinit.c > +++ b/src/fw/pciinit.c > @@ -257,6 +257,52 @@ static void ich9_smbus_setup(struct pci_device *dev, void *arg) > pci_config_writeb(bdf, ICH9_SMB_HOSTC, ICH9_SMB_HOSTC_HST_EN); > } > > +static void intel_igd_opregion_setup(struct pci_device *dev, void *arg) > +{ > + u16 bdf = dev->bdf; > + u32 orig; > + void *opregion; > + int size = 8; > + > + if (!CONFIG_QEMU) > + return; > + > + orig = pci_config_readl(bdf, 0xFC); > + > +realloc: > + opregion = malloc_high(size * 1024); > + if (!opregion) { > + warn_noalloc(); > + return; > + } > + > + /* > + * QEMU maps the OpRegion into system memory at the address written here, > + * this overlaps our malloc, which marks the range e820 reserved. > + */ > + pci_config_writel(bdf, 0xFC, cpu_to_le32((u32)opregion)); > + > + if (memcmp(opregion, "IntelGraphicsMem", 16)) { > + pci_config_writel(bdf, 0xFC, orig); > + free(opregion); > + return; /* the opregion didn't magically appear, not supported */ > + } > + > + if (size == le32_to_cpu(*(u32 *)(opregion + 16))) { > + dprintf(1, "Intel IGD OpRegion enabled on %02x:%02x.%x\n", > + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf)); > + return; /* success! */ > + } > + > + pci_config_writel(bdf, 0xFC, orig); > + free(opregion); > + > + if (size == 8) { /* try once more with a new size */ > + size = le32_to_cpu(*(u32 *)(opregion + 16)); > + goto realloc; Is this idiomatic SeaBIOS coding style? How about "for (;;)" -- the code has many instances -- and reworking this last branch accordingly? (Apologies, not a very important comment.) Thanks Laszlo > + } > +} > + > static const struct pci_device_id pci_device_tbl[] = { > /* PIIX3/PIIX4 PCI to ISA bridge */ > PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371SB_0, > @@ -290,6 +336,10 @@ static const struct pci_device_id pci_device_tbl[] = { > PCI_DEVICE_CLASS(PCI_VENDOR_ID_APPLE, 0x0017, 0xff00, apple_macio_setup), > PCI_DEVICE_CLASS(PCI_VENDOR_ID_APPLE, 0x0022, 0xff00, apple_macio_setup), > > + /* Intel IGD OpRegion setup */ > + PCI_DEVICE_CLASS(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, > + intel_igd_opregion_setup), > + > PCI_DEVICE_END, > }; > > > > _______________________________________________ > SeaBIOS mailing list > SeaBIOS@seabios.org > http://www.seabios.org/mailman/listinfo/seabios >