[RFC PATCH v2] fw/pci: Add support for mapping Intel IGD OpRegion via QEMU

[Alex Williamson]

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,
 };

[Gerd Hoffmann]

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

[Alex Williamson]

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

[Alex Williamson]

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

[Kevin O'Connor]

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

[Alex Williamson]

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

[Gerd Hoffmann]

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

[Kevin O'Connor]

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

[Igor Mammedov]

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
> [e-mail]
> http://www.seabios.org/mailman/listinfo/seabios

[Alex Williamson]

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
> > [e-mail]
> > http://www.seabios.org/mailman/listinfo/seabios

[Gerd Hoffmann]

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