[SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
When assigning Intel IGD graphics via QEMU/vfio, the OpRegion for the device may be exposed as a fw_cfg file. Allocate space for this, copy the contents and write the ASL Storage register (0xFC) to point to this buffer. NB, it's possible for QEMU to use the write to the ASL Storage register to map access to the host OpRegion overlapping the allocated buffer, but we shouldn't care if it does. References: kernel vfio opregion support: https://lkml.org/lkml/2016/2/1/884 QEMU vfio opregion support (revised v2 of 7/7 adds fw_cfg): https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00202.html Gerd's IGD assignment series: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00244.html Signed-off-by: Alex Williamson --- src/fw/pciinit.c | 30 ++ 1 file changed, 30 insertions(+) diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index c31c2fa..92170d5 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -257,6 +257,32 @@ 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) +{ +struct romfile_s *file = romfile_find("etc/igd-opregion"); +void *opregion; +u16 bdf = dev->bdf; + +if (!file || !file->size) +return; + +opregion = memalign_high(PAGE_SIZE, file->size); +if (!opregion) { +warn_noalloc(); +return; +} + +if (file->copy(file, opregion, file->size) < 0) { +free(opregion); +return; +} + +pci_config_writel(bdf, 0xFC, cpu_to_le32((u32)opregion)); + +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)); +} + 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 +316,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
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
Hi, > +static void intel_igd_opregion_setup(struct pci_device *dev, void *arg) > +{ > +struct romfile_s *file = romfile_find("etc/igd-opregion"); Is it possible to have multiple igd devices in a single machine? So, should we include the pci address in the file name? Guess not needed, it's chipset graphics after all ... > +pci_config_writel(bdf, 0xFC, cpu_to_le32((u32)opregion)); Looks a bit funny in code which is never ever going to run on big endian machines ;) Patch looks good to me. cheers, Gerd ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
On Wed, 2016-02-03 at 10:04 +0100, Gerd Hoffmann wrote: > Hi, > > > +static void intel_igd_opregion_setup(struct pci_device *dev, void *arg) > > +{ > > +struct romfile_s *file = romfile_find("etc/igd-opregion"); > > Is it possible to have multiple igd devices in a single machine? > So, should we include the pci address in the file name? > > Guess not needed, it's chipset graphics after all ... Hmm, I think that's probably a pretty good observation, we don't want to revisit this if vGPUs need/want an OpRegion or if Intel decides to start allowing more than one per system. Either could pretty easily introduce multiple into a VM. > > +pci_config_writel(bdf, 0xFC, cpu_to_le32((u32)opregion)); > > Looks a bit funny in code which is never ever going to run on big endian > machines ;) Clearly I stole it from code that wasn't assuming little endian and figured "why not", it's a nop anyway and one less thing to care about when IBM licenses Intel graphics for POWER systems... ;) > Patch looks good to me. Thanks! Alex ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
On Wed, 2016-02-03 at 12:43 -0700, Alex Williamson wrote: > On Wed, 2016-02-03 at 10:04 +0100, Gerd Hoffmann wrote: > > Hi, > > > > > +static void intel_igd_opregion_setup(struct pci_device *dev, void *arg) > > > +{ > > > +struct romfile_s *file = romfile_find("etc/igd-opregion"); > > > > Is it possible to have multiple igd devices in a single machine? > > So, should we include the pci address in the file name? > > > > Guess not needed, it's chipset graphics after all ... > > Hmm, I think that's probably a pretty good observation, we don't want to > revisit this if vGPUs need/want an OpRegion or if Intel decides to start > allowing more than one per system. Either could pretty easily introduce > multiple into a VM. Naming is always more complicated than it seems. For anything other than a root bus devices, the PCI address doesn't exist until SeaBIOS enumerates devices and assigns bus numbers for bridges. So unless we want to provide a path to the device like ACPI defines, maybe we should just stick with "etc/igd-opregion". It seems easily extensible to add more specific files later and default to this one if those aren't found. Thanks, Alex ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
On Wed, Feb 03, 2016 at 02:38:47PM -0700, Alex Williamson wrote: > On Wed, 2016-02-03 at 12:43 -0700, Alex Williamson wrote: > > On Wed, 2016-02-03 at 10:04 +0100, Gerd Hoffmann wrote: > > > Hi, > > > > > > > +static void intel_igd_opregion_setup(struct pci_device *dev, void *arg) > > > > +{ > > > > +struct romfile_s *file = romfile_find("etc/igd-opregion"); > > > > > > Is it possible to have multiple igd devices in a single machine? > > > So, should we include the pci address in the file name? > > > > > > Guess not needed, it's chipset graphics after all ... > > > > Hmm, I think that's probably a pretty good observation, we don't want to > > revisit this if vGPUs need/want an OpRegion or if Intel decides to start > > allowing more than one per system. Either could pretty easily introduce > > multiple into a VM. > > Naming is always more complicated than it seems. For anything other > than a root bus devices, the PCI address doesn't exist until SeaBIOS > enumerates devices and assigns bus numbers for bridges. So unless we > want to provide a path to the device like ACPI defines, maybe we should > just stick with "etc/igd-opregion". It seems easily extensible to add > more specific files later and default to this one if those aren't found. Perhaps a simpler solution is to just make sure "etc/igd-opregion" is only deployed for the "active" VGA device (ie, the device that is_pci_vga() returns true for). Looks like pci_enable_default_vga() already has code for something similar. -Kevin ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
On Wed, 2016-02-03 at 17:09 -0500, Kevin O'Connor wrote: > On Wed, Feb 03, 2016 at 02:38:47PM -0700, Alex Williamson wrote: > > On Wed, 2016-02-03 at 12:43 -0700, Alex Williamson wrote: > > > On Wed, 2016-02-03 at 10:04 +0100, Gerd Hoffmann wrote: > > > > Hi, > > > > > > > > > +static void intel_igd_opregion_setup(struct pci_device *dev, void > > > > > *arg) > > > > > +{ > > > > > +struct romfile_s *file = romfile_find("etc/igd-opregion"); > > > > > > > > Is it possible to have multiple igd devices in a single machine? > > > > So, should we include the pci address in the file name? > > > > > > > > Guess not needed, it's chipset graphics after all ... > > > > > > Hmm, I think that's probably a pretty good observation, we don't want to > > > revisit this if vGPUs need/want an OpRegion or if Intel decides to start > > > allowing more than one per system. Either could pretty easily introduce > > > multiple into a VM. > > > > Naming is always more complicated than it seems. For anything other > > than a root bus devices, the PCI address doesn't exist until SeaBIOS > > enumerates devices and assigns bus numbers for bridges. So unless we > > want to provide a path to the device like ACPI defines, maybe we should > > just stick with "etc/igd-opregion". It seems easily extensible to add > > more specific files later and default to this one if those aren't found. > > Perhaps a simpler solution is to just make sure "etc/igd-opregion" is > only deployed for the "active" VGA device (ie, the device that > is_pci_vga() returns true for). Looks like pci_enable_default_vga() > already has code for something similar. Comments we've seen from Intel suggest they prefer assigning IGD as a secondary VGA device though, so the "active" VGA device might not be IGD at all. I'd also be surprised if QEMU ever sets an active VGA device itself versus relying on SeaBIOS to do it via pci_enable_default_vga(). Then it seems like we get into the realm of QEMU needing to know in advance how SeaBIOS' search algorithms work to know which is the "next" device. One thing in our favor, maybe, is that there can't be multiple "etc/igd-opregion" fw_cfg files, QEMU will assert if that happens. So if we get to SeaBIOS and there are multiple Intel VGA devices, there can only be at most one OpRegion for them and we can either give them all the same or just use it on the first one we encounter. We're probably putting way too much thought into this scenario that Intel likely has no expectation of supporting though ;) Thanks, Alex ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
Hi, > the same or just use it on the first one we encounter. We're probably > putting way too much thought into this scenario that Intel likely has no > expectation of supporting though ;) Thanks, Yea, lets stick with the plain simple "igd-opregion". Even if intel allows multiple igd devices per host some day things continue to work as long as you assign only one of them to a guest. cheers, Gerd ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
On Thu, Feb 04, 2016 at 08:52:44AM +0100, Gerd Hoffmann wrote: > Hi, > > > the same or just use it on the first one we encounter. We're probably > > putting way too much thought into this scenario that Intel likely has no > > expectation of supporting though ;) Thanks, > > Yea, lets stick with the plain simple "igd-opregion". Even if intel > allows multiple igd devices per host some day things continue to work as > long as you assign only one of them to a guest. Sounds good. The patch looks fine to me. I do think we should commit to SeaBIOS only after the QEMU support is accepted though. -Kevin ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
On Tue, 02 Feb 2016 13:10:37 -0700 Alex Williamson wrote: > When assigning Intel IGD graphics via QEMU/vfio, the OpRegion for the > device may be exposed as a fw_cfg file. Allocate space for this, copy > the contents and write the ASL Storage register (0xFC) to point to > this buffer. NB, it's possible for QEMU to use the write to the ASL > Storage register to map access to the host OpRegion overlapping the > allocated buffer, but we shouldn't care if it does. > > References: > kernel vfio opregion support: > https://lkml.org/lkml/2016/2/1/884 > QEMU vfio opregion support (revised v2 of 7/7 adds fw_cfg): > https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00202.html > Gerd's IGD assignment series: > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00244.html > > Signed-off-by: Alex Williamson > --- > src/fw/pciinit.c | 30 ++ > 1 file changed, 30 insertions(+) > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > index c31c2fa..92170d5 100644 > --- a/src/fw/pciinit.c > +++ b/src/fw/pciinit.c > @@ -257,6 +257,32 @@ 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) > +{ > +struct romfile_s *file = romfile_find("etc/igd-opregion"); > +void *opregion; > +u16 bdf = dev->bdf; > + > +if (!file || !file->size) > +return; > + > +opregion = memalign_high(PAGE_SIZE, file->size); > +if (!opregion) { > +warn_noalloc(); > +return; > +} > + > +if (file->copy(file, opregion, file->size) < 0) { Is opregion content on host immutable? if not then copying it probably wrong and it should be passed-through. > +free(opregion); > +return; > +} > + > +pci_config_writel(bdf, 0xFC, cpu_to_le32((u32)opregion)); > + > +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)); > +} > + > 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 +316,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 ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
On Thu, 4 Feb 2016 17:58:23 +0100 Igor Mammedov wrote: > On Tue, 02 Feb 2016 13:10:37 -0700 > Alex Williamson wrote: > > > When assigning Intel IGD graphics via QEMU/vfio, the OpRegion for > > the device may be exposed as a fw_cfg file. Allocate space for > > this, copy the contents and write the ASL Storage register (0xFC) > > to point to this buffer. NB, it's possible for QEMU to use the > > write to the ASL Storage register to map access to the host > > OpRegion overlapping the allocated buffer, but we shouldn't care if > > it does. > > > > References: > > kernel vfio opregion support: > > https://lkml.org/lkml/2016/2/1/884 > > QEMU vfio opregion support (revised v2 of 7/7 adds fw_cfg): > > https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00202.html > > Gerd's IGD assignment series: > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00244.html > > > > Signed-off-by: Alex Williamson > > --- > > src/fw/pciinit.c | 30 ++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > > index c31c2fa..92170d5 100644 > > --- a/src/fw/pciinit.c > > +++ b/src/fw/pciinit.c > > @@ -257,6 +257,32 @@ 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) +{ > > +struct romfile_s *file = romfile_find("etc/igd-opregion"); > > +void *opregion; > > +u16 bdf = dev->bdf; > > + > > +if (!file || !file->size) > > +return; > > + > > +opregion = memalign_high(PAGE_SIZE, file->size); > > +if (!opregion) { > > +warn_noalloc(); > > +return; > > +} > > + > > +if (file->copy(file, opregion, file->size) < 0) { > Is opregion content on host immutable? > if not then copying it probably wrong and it should be passed-through. The content is not immutable, but for the first round of things that we're interested in, it probably is. It's not clear that we'll ever move beyond that first level though. Part of the benefit of this approach is that SeaBIOS allocates the correct size, copies a static version of the OpRegion data into place, then effectively tells QEMU that it has done this by writing to the ASL Storage register. At that point QEMU can simply virtualize the register for the guest or it can map a live version of the OpRegion over top of the SeaBIOS copy. So we certainly have the option to go beyond an immutable copy with no further change to SeaBIOS. Thanks, Alex > > +free(opregion); > > +return; > > +} > > + > > +pci_config_writel(bdf, 0xFC, cpu_to_le32((u32)opregion)); > > + > > +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)); +} > > + > > 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 +316,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 > ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU
Hi, > > +if (file->copy(file, opregion, file->size) < 0) { > Is opregion content on host immutable? > if not then copying it probably wrong and it should be passed-through. The OpRegion is used for communication between bios and driver, it is *not* something going directly to the hardware. There is also the VBT (video bios table) with some configuration information (most notably laptop panel info). The latter will be used by the guest driver. To make stuff like brightness control work is is not enough to pass-through the opregion, we would also have to somehow forward acpi notifications and acpi calls between host and guest, so the guest driver can talk to the host bios. That is seriously non-trivial, for little (if any) gain. cheers, Gerd ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios