This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.

The original patch intend to prevent linux i915 driver from using
stolen meory. But this patch breaks windows IGD driver loading on
Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
driver see zero size stolen memory, it will unload.
Meanwhile stolen memory will be disabled in 915 when i915 run as
a guest.

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 hw/vfio/pci-quirks.c | 65 ++++++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 38 deletions(-)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index e995e32..e9b493b 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1367,45 +1367,14 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     uint16_t cmd_orig, cmd;
     Error *err = NULL;
 
-    /* This must be an Intel VGA device. */
-    if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
-        !vfio_is_vga(vdev) || nr != 4) {
-        return;
-    }
-
-    /*
-     * IGD is not a standard, they like to change their specs often.  We
-     * only attempt to support back to SandBridge and we hope that newer
-     * devices maintain compatibility with generation 8.
-     */
-    gen = igd_gen(vdev);
-    if (gen != 6 && gen != 8) {
-        error_report("IGD device %s is unsupported by IGD quirks, "
-                     "try SandyBridge or newer", vdev->vbasedev.name);
-        return;
-    }
-
-    /*
-     * Regardless of running in UPT or legacy mode, the guest graphics
-     * driver may attempt to use stolen memory, however only legacy mode
-     * has BIOS support for reserving stolen memory in the guest VM.
-     * Emulate the GMCH register in all cases and zero out the stolen
-     * memory size here. Legacy mode may request allocation and re-write
-     * this below.
-     */
-    gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
-    gmch &= ~((gen < 8 ? 0x1f : 0xff) << (gen < 8 ? 3 : 8));
-
-    /* GMCH is read-only, emulated */
-    pci_set_long(vdev->pdev.config + IGD_GMCH, gmch);
-    pci_set_long(vdev->pdev.wmask + IGD_GMCH, 0);
-    pci_set_long(vdev->emulated_config_bits + IGD_GMCH, ~0);
-
     /*
-     * This must be at address 00:02.0 for us to even onsider enabling
-     * legacy mode.  The vBIOS has dependencies on the PCI bus address.
+     * This must be an Intel VGA device at address 00:02.0 for us to even
+     * consider enabling legacy mode.  The vBIOS has dependencies on the
+     * PCI bus address.
      */
-    if (&vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
+    if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
+        !vfio_is_vga(vdev) || nr != 4 ||
+        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
                                        0, PCI_DEVFN(0x2, 0))) {
         return;
     }
@@ -1425,6 +1394,18 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     }
 
     /*
+     * IGD is not a standard, they like to change their specs often.  We
+     * only attempt to support back to SandBridge and we hope that newer
+     * devices maintain compatibility with generation 8.
+     */
+    gen = igd_gen(vdev);
+    if (gen != 6 && gen != 8) {
+        error_report("IGD device %s is unsupported in legacy mode, "
+                     "try SandyBridge or newer", vdev->vbasedev.name);
+        return;
+    }
+
+    /*
      * Most of what we're doing here is to enable the ROM to run, so if
      * there's no ROM, there's no point in setting up this quirk.
      * NB. We only seem to get BIOS ROMs, so a UEFI VM would need CSM support.
@@ -1479,6 +1460,8 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         goto out;
     }
 
+    gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
+
     /*
      * If IGD VGA Disable is clear (expected) and VGA is not already enabled,
      * try to enable it.  Probably shouldn't be using legacy mode without VGA,
@@ -1549,11 +1532,12 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
      * when IVD (IGD VGA Disable) is clear, but the claim is that it's unused,
      * so let's not waste VM memory for it.
      */
+    gmch &= ~((gen < 8 ? 0x1f : 0xff) << (gen < 8 ? 3 : 8));
+
     if (vdev->igd_gms) {
         if (vdev->igd_gms <= 0x10) {
             gms_mb = vdev->igd_gms * 32;
             gmch |= vdev->igd_gms << (gen < 8 ? 3 : 8);
-            pci_set_long(vdev->pdev.config + IGD_GMCH, gmch);
         } else {
             error_report("Unsupported IGD GMS value 0x%x", vdev->igd_gms);
             vdev->igd_gms = 0;
@@ -1573,6 +1557,11 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     fw_cfg_add_file(fw_cfg_find(), "etc/igd-bdsm-size",
                     bdsm_size, sizeof(*bdsm_size));
 
+    /* GMCH is read-only, emulated */
+    pci_set_long(vdev->pdev.config + IGD_GMCH, gmch);
+    pci_set_long(vdev->pdev.wmask + IGD_GMCH, 0);
+    pci_set_long(vdev->emulated_config_bits + IGD_GMCH, ~0);
+
     /* BDSM is read-write, emulated.  The BIOS needs to be able to write it */
     pci_set_long(vdev->pdev.config + IGD_BDSM, 0);
     pci_set_long(vdev->pdev.wmask + IGD_BDSM, ~0);
-- 
1.9.1

On Thu, 30 Mar 2017 18:27:21 +0800
Xiong Zhang <xiong.y.zhang@intel.com> wrote:

> This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> 
> The original patch intend to prevent linux i915 driver from using
> stolen meory. But this patch breaks windows IGD driver loading on
> Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
> driver see zero size stolen memory, it will unload.
> Meanwhile stolen memory will be disabled in 915 when i915 run as
> a guest.

Does this mean that legacy mode IGD assignment is not going to work
on Gen9+ with Windows?  Will it continue to work with Gen8-?

Please clarify Gen9+, is this Kaby Lake?

I assume this patch is intended for QEMU 2.9, it's helpful to make that
explicit during the rc freeze.  Thanks,

Alex

> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> ---
>  hw/vfio/pci-quirks.c | 65 ++++++++++++++++++++++------------------------------
>  1 file changed, 27 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index e995e32..e9b493b 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1367,45 +1367,14 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>      uint16_t cmd_orig, cmd;
>      Error *err = NULL;
>  
> -    /* This must be an Intel VGA device. */
> -    if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> -        !vfio_is_vga(vdev) || nr != 4) {
> -        return;
> -    }
> -
> -    /*
> -     * IGD is not a standard, they like to change their specs often.  We
> -     * only attempt to support back to SandBridge and we hope that newer
> -     * devices maintain compatibility with generation 8.
> -     */
> -    gen = igd_gen(vdev);
> -    if (gen != 6 && gen != 8) {
> -        error_report("IGD device %s is unsupported by IGD quirks, "
> -                     "try SandyBridge or newer", vdev->vbasedev.name);
> -        return;
> -    }
> -
> -    /*
> -     * Regardless of running in UPT or legacy mode, the guest graphics
> -     * driver may attempt to use stolen memory, however only legacy mode
> -     * has BIOS support for reserving stolen memory in the guest VM.
> -     * Emulate the GMCH register in all cases and zero out the stolen
> -     * memory size here. Legacy mode may request allocation and re-write
> -     * this below.
> -     */
> -    gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
> -    gmch &= ~((gen < 8 ? 0x1f : 0xff) << (gen < 8 ? 3 : 8));
> -
> -    /* GMCH is read-only, emulated */
> -    pci_set_long(vdev->pdev.config + IGD_GMCH, gmch);
> -    pci_set_long(vdev->pdev.wmask + IGD_GMCH, 0);
> -    pci_set_long(vdev->emulated_config_bits + IGD_GMCH, ~0);
> -
>      /*
> -     * This must be at address 00:02.0 for us to even onsider enabling
> -     * legacy mode.  The vBIOS has dependencies on the PCI bus address.
> +     * This must be an Intel VGA device at address 00:02.0 for us to even
> +     * consider enabling legacy mode.  The vBIOS has dependencies on the
> +     * PCI bus address.
>       */
> -    if (&vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
> +    if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> +        !vfio_is_vga(vdev) || nr != 4 ||
> +        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>                                         0, PCI_DEVFN(0x2, 0))) {
>          return;
>      }
> @@ -1425,6 +1394,18 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>      }
>  
>      /*
> +     * IGD is not a standard, they like to change their specs often.  We
> +     * only attempt to support back to SandBridge and we hope that newer
> +     * devices maintain compatibility with generation 8.
> +     */
> +    gen = igd_gen(vdev);
> +    if (gen != 6 && gen != 8) {
> +        error_report("IGD device %s is unsupported in legacy mode, "
> +                     "try SandyBridge or newer", vdev->vbasedev.name);
> +        return;
> +    }
> +
> +    /*
>       * Most of what we're doing here is to enable the ROM to run, so if
>       * there's no ROM, there's no point in setting up this quirk.
>       * NB. We only seem to get BIOS ROMs, so a UEFI VM would need CSM support.
> @@ -1479,6 +1460,8 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>          goto out;
>      }
>  
> +    gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
> +
>      /*
>       * If IGD VGA Disable is clear (expected) and VGA is not already enabled,
>       * try to enable it.  Probably shouldn't be using legacy mode without VGA,
> @@ -1549,11 +1532,12 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>       * when IVD (IGD VGA Disable) is clear, but the claim is that it's unused,
>       * so let's not waste VM memory for it.
>       */
> +    gmch &= ~((gen < 8 ? 0x1f : 0xff) << (gen < 8 ? 3 : 8));
> +
>      if (vdev->igd_gms) {
>          if (vdev->igd_gms <= 0x10) {
>              gms_mb = vdev->igd_gms * 32;
>              gmch |= vdev->igd_gms << (gen < 8 ? 3 : 8);
> -            pci_set_long(vdev->pdev.config + IGD_GMCH, gmch);
>          } else {
>              error_report("Unsupported IGD GMS value 0x%x", vdev->igd_gms);
>              vdev->igd_gms = 0;
> @@ -1573,6 +1557,11 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>      fw_cfg_add_file(fw_cfg_find(), "etc/igd-bdsm-size",
>                      bdsm_size, sizeof(*bdsm_size));
>  
> +    /* GMCH is read-only, emulated */
> +    pci_set_long(vdev->pdev.config + IGD_GMCH, gmch);
> +    pci_set_long(vdev->pdev.wmask + IGD_GMCH, 0);
> +    pci_set_long(vdev->emulated_config_bits + IGD_GMCH, ~0);
> +
>      /* BDSM is read-write, emulated.  The BIOS needs to be able to write it */
>      pci_set_long(vdev->pdev.config + IGD_BDSM, 0);
>      pci_set_long(vdev->pdev.wmask + IGD_BDSM, ~0);

> On Thu, 30 Mar 2017 18:27:21 +0800
> Xiong Zhang <xiong.y.zhang@intel.com> wrote:
> 
> > This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> >
> > The original patch intend to prevent linux i915 driver from using
> > stolen meory. But this patch breaks windows IGD driver loading on
> > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
> > driver see zero size stolen memory, it will unload.
> > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > a guest.
> 
> Does this mean that legacy mode IGD assignment is not going to work
> on Gen9+ with Windows?  Will it continue to work with Gen8-?
[Zhang, Xiong Y] I try to use the following qemu command to enable legacy mode on SKyLake, but It seems the entry point of wins IGD driver isn't called(I couldn't confirm this as I don't have the source code, but I didn't see any IGD driver info from windbg while I could see many info in upt mode), so driver doesn't bind to IGD after win 8.1 boot up.
  #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none -nographic -cpu host -hda "$IMAGE" -device vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
Is this the right method to enable legacy mode ?

> Please clarify Gen9+, is this Kaby Lake?
[Zhang, Xiong Y] Gen 9+ is SkyLake and later.

> I assume this patch is intended for QEMU 2.9, it's helpful to make that
> explicit during the rc freeze.  Thanks,
[Zhang, Xiong Y] Yes, as the original patch has entered into Qemu 2.9 rc1. So this reverted patch should be entered into the later 2.9 rc.
Sorry for the troubles.

On Fri, 31 Mar 2017 02:27:11 +0000
"Zhang, Xiong Y" <xiong.y.zhang@intel.com> wrote:

> > On Thu, 30 Mar 2017 18:27:21 +0800
> > Xiong Zhang <xiong.y.zhang@intel.com> wrote:
> >   
> > > This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> > >
> > > The original patch intend to prevent linux i915 driver from using
> > > stolen meory. But this patch breaks windows IGD driver loading on
> > > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
> > > driver see zero size stolen memory, it will unload.
> > > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > > a guest.  
> > 
> > Does this mean that legacy mode IGD assignment is not going to work
> > on Gen9+ with Windows?  Will it continue to work with Gen8-?  
> [Zhang, Xiong Y] I try to use the following qemu command to enable legacy mode on SKyLake, but It seems the entry point of wins IGD driver isn't called(I couldn't confirm this as I don't have the source code, but I didn't see any IGD driver info from windbg while I could see many info in upt mode), so driver doesn't bind to IGD after win 8.1 boot up.
>   #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none -nographic -cpu host -hda "$IMAGE" -device vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
> Is this the right method to enable legacy mode ?

Yeah, that should do it.  x-vga should not be necessary, but shouldn't
hurt IIRC.  Any dmesg errors regarding the ROM?  I think we have
trouble with the ROM if the host is booted in UEFI mode.

> 
> > Please clarify Gen9+, is this Kaby Lake?  
> [Zhang, Xiong Y] Gen 9+ is SkyLake and later.

Ok, then I cannot test since I only have access to BDW.  We do have
users that might start complaining if this is a new change in the
Windows driver for SKL+.
 
> > I assume this patch is intended for QEMU 2.9, it's helpful to make that
> > explicit during the rc freeze.  Thanks,  
> [Zhang, Xiong Y] Yes, as the original patch has entered into Qemu 2.9 rc1. So this reverted patch should be entered into the later 2.9 rc.
> Sorry for the troubles.

Ok, no problem.  Thanks,

Alex 

> On Fri, 31 Mar 2017 02:27:11 +0000
> "Zhang, Xiong Y" <xiong.y.zhang@intel.com> wrote:
> 
> > > On Thu, 30 Mar 2017 18:27:21 +0800
> > > Xiong Zhang <xiong.y.zhang@intel.com> wrote:
> > >
> > > > This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> > > >
> > > > The original patch intend to prevent linux i915 driver from using
> > > > stolen meory. But this patch breaks windows IGD driver loading on
> > > > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
> > > > driver see zero size stolen memory, it will unload.
> > > > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > > > a guest.
> > >
> > > Does this mean that legacy mode IGD assignment is not going to work
> > > on Gen9+ with Windows?  Will it continue to work with Gen8-?
> > [Zhang, Xiong Y] I try to use the following qemu command to enable legacy
> mode on SKyLake, but It seems the entry point of wins IGD driver isn't called(I
> couldn't confirm this as I don't have the source code, but I didn't see any IGD
> driver info from windbg while I could see many info in upt mode), so driver
> doesn't bind to IGD after win 8.1 boot up.
> >   #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none
> -nographic -cpu host -hda "$IMAGE" -device
> vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
> > Is this the right method to enable legacy mode ?
> 
> Yeah, that should do it.  x-vga should not be necessary, but shouldn't
> hurt IIRC.  Any dmesg errors regarding the ROM?  I think we have
> trouble with the ROM if the host is booted in UEFI mode.
[Zhang, Xiong Y] My host boot in legacy bios mode. After adding x-igd-gms in legacy mode, win 8.1 IGD driver could bind to IGD, and win 8.1 runs good. 
thanks
> 
> >
> > > Please clarify Gen9+, is this Kaby Lake?
> > [Zhang, Xiong Y] Gen 9+ is SkyLake and later.
> 
> Ok, then I cannot test since I only have access to BDW.  We do have
> users that might start complaining if this is a new change in the
> Windows driver for SKL+.
> 
> > > I assume this patch is intended for QEMU 2.9, it's helpful to make that
> > > explicit during the rc freeze.  Thanks,
> > [Zhang, Xiong Y] Yes, as the original patch has entered into Qemu 2.9 rc1. So
> this reverted patch should be entered into the later 2.9 rc.
> > Sorry for the troubles.
> 
> Ok, no problem.  Thanks,
> 
> Alex

On Thu, 30 Mar 2017 20:55:11 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Fri, 31 Mar 2017 02:27:11 +0000
> "Zhang, Xiong Y" <xiong.y.zhang@intel.com> wrote:
> 
> > > On Thu, 30 Mar 2017 18:27:21 +0800
> > > Xiong Zhang <xiong.y.zhang@intel.com> wrote:
> > >   
> > > > This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> > > >
> > > > The original patch intend to prevent linux i915 driver from using
> > > > stolen meory. But this patch breaks windows IGD driver loading on
> > > > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
> > > > driver see zero size stolen memory, it will unload.
> > > > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > > > a guest.  
> > > 
> > > Does this mean that legacy mode IGD assignment is not going to work
> > > on Gen9+ with Windows?  Will it continue to work with Gen8-?  
> > [Zhang, Xiong Y] I try to use the following qemu command to enable legacy mode on SKyLake, but It seems the entry point of wins IGD driver isn't called(I couldn't confirm this as I don't have the source code, but I didn't see any IGD driver info from windbg while I could see many info in upt mode), so driver doesn't bind to IGD after win 8.1 boot up.
> >   #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none -nographic -cpu host -hda "$IMAGE" -device vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
> > Is this the right method to enable legacy mode ?
> 
> Yeah, that should do it.  x-vga should not be necessary, but shouldn't
> hurt IIRC.  Any dmesg errors regarding the ROM?  I think we have
> trouble with the ROM if the host is booted in UEFI mode.

1.
here is dmesg messages when host is booted with CSM mode enabled
and host's bios load option rom on boot:

[165041.359929] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
[165073.898940] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
[165074.687515] vfio-pci 0000:00:02.0: enabling device (0400 -> 0403)
[165074.791598] vfio_ecap_init: 0000:00:02.0 hiding ecap 0x1b@0x100

have output on monitor connected via HDMI

2.
and here is dmesg in pure UEFI mode (where host's bios doesn't load option rom):

[   21.034983] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
[   22.025361] vfio_ecap_init: 0000:00:02.0 hiding ecap 0x1b@0x100
[   22.030970] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55, got 0xffff
[   22.031036] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55, got 0xffff
[   24.738793] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55, got 0xffff
[   27.776904] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem

no output on monitor connected via HDMI when using Kabylake CPU (HD630)
but pure UEFI mode used to work (external output) with Skylake CPU (HD510)


---
my test config is Win10 guest + IGD drivers 4534 build (installed by win update)
i3-7100T on C236 chipset with following libvirt config

  <devices>
    ...
    <hostdev mode='subsystem' type='pci' managed='yes'>
      <driver name='vfio'/>
      <source>
        <address domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
      </source>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
    </hostdev>
    <hostdev mode='subsystem' type='pci' managed='yes'>
      <driver name='vfio'/>
      <source>
        <address domain='0x0000' bus='0x00' slot='0x1f' function='0x3'/>
      </source>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
    </hostdev>
  </devices>
  <qemu:commandline>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.hostdev0.x-igd-opregion=on'/>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.hostdev0.x-vga=on'/>
  </qemu:commandline>

> > 
> > > Please clarify Gen9+, is this Kaby Lake?  
> > [Zhang, Xiong Y] Gen 9+ is SkyLake and later.
> 
> Ok, then I cannot test since I only have access to BDW.  We do have
> users that might start complaining if this is a new change in the
> Windows driver for SKL+.
I can with testing it, just tell me what host/guest settings combination I should test it with.

>  
> > > I assume this patch is intended for QEMU 2.9, it's helpful to make that
> > > explicit during the rc freeze.  Thanks,  
> > [Zhang, Xiong Y] Yes, as the original patch has entered into Qemu 2.9 rc1. So this reverted patch should be entered into the later 2.9 rc.
> > Sorry for the troubles.
> 
> Ok, no problem.  Thanks,
> 
> Alex 
> 
> 

On Fri, 31 Mar 2017 19:03:49 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 30 Mar 2017 20:55:11 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Fri, 31 Mar 2017 02:27:11 +0000
> > "Zhang, Xiong Y" <xiong.y.zhang@intel.com> wrote:
> >   
> > > > On Thu, 30 Mar 2017 18:27:21 +0800
> > > > Xiong Zhang <xiong.y.zhang@intel.com> wrote:
> > > >     
> > > > > This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> > > > >
> > > > > The original patch intend to prevent linux i915 driver from using
> > > > > stolen meory. But this patch breaks windows IGD driver loading on
> > > > > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
> > > > > driver see zero size stolen memory, it will unload.
> > > > > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > > > > a guest.    
> > > > 
> > > > Does this mean that legacy mode IGD assignment is not going to work
> > > > on Gen9+ with Windows?  Will it continue to work with Gen8-?    
> > > [Zhang, Xiong Y] I try to use the following qemu command to enable legacy mode on SKyLake, but It seems the entry point of wins IGD driver isn't called(I couldn't confirm this as I don't have the source code, but I didn't see any IGD driver info from windbg while I could see many info in upt mode), so driver doesn't bind to IGD after win 8.1 boot up.
> > >   #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none -nographic -cpu host -hda "$IMAGE" -device vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
> > > Is this the right method to enable legacy mode ?  
> > 
> > Yeah, that should do it.  x-vga should not be necessary, but shouldn't
> > hurt IIRC.  Any dmesg errors regarding the ROM?  I think we have
> > trouble with the ROM if the host is booted in UEFI mode.  
> 
> 1.
> here is dmesg messages when host is booted with CSM mode enabled
> and host's bios load option rom on boot:
> 
> [165041.359929] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
> [165073.898940] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
> [165074.687515] vfio-pci 0000:00:02.0: enabling device (0400 -> 0403)
> [165074.791598] vfio_ecap_init: 0000:00:02.0 hiding ecap 0x1b@0x100
> 
> have output on monitor connected via HDMI
> 
> 2.
> and here is dmesg in pure UEFI mode (where host's bios doesn't load option rom):
> 
> [   21.034983] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
> [   22.025361] vfio_ecap_init: 0000:00:02.0 hiding ecap 0x1b@0x100
> [   22.030970] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55, got 0xffff
> [   22.031036] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55, got 0xffff
> [   24.738793] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55, got 0xffff
> [   27.776904] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
> 
> no output on monitor connected via HDMI when using Kabylake CPU (HD630)
> but pure UEFI mode used to work (external output) with Skylake CPU (HD510)

In my limited experience, I've never been able to use IGD assignment on
a pure UEFI host unless I provide the ROM via file.  For instance on my
laptop I switched to CSM and booted a live CD to dump the ROM, which I
can then use regardless of the host BIOS mode.

I just updated my rom-parser to include a version to fixup ROMs for
vendor/device ID and checksum which should help with this:

https://github.com/awilliam/rom-parser

Update the device ID to match your ROM and let it fix the checksum.
Maybe Xiong has some insight into why the VGA ROM often doesn't seem to
exist on native pure UEFI hosts.

> > ---
> my test config is Win10 guest + IGD drivers 4534 build (installed by win update)
> i3-7100T on C236 chipset with following libvirt config
> 
>   <devices>
>     ...
>     <hostdev mode='subsystem' type='pci' managed='yes'>
>       <driver name='vfio'/>
>       <source>
>         <address domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
>       </source>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
>     </hostdev>
>     <hostdev mode='subsystem' type='pci' managed='yes'>
>       <driver name='vfio'/>
>       <source>
>         <address domain='0x0000' bus='0x00' slot='0x1f' function='0x3'/>
>       </source>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
>     </hostdev>
>   </devices>
>   <qemu:commandline>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.hostdev0.x-igd-opregion=on'/>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.hostdev0.x-vga=on'/>

Hmm, neither of these should be necessary, legacy mode IGD support
should active both automatically.  Thanks,

Alex

> On Fri, 31 Mar 2017 19:03:49 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Thu, 30 Mar 2017 20:55:11 -0600
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >
> > > On Fri, 31 Mar 2017 02:27:11 +0000
> > > "Zhang, Xiong Y" <xiong.y.zhang@intel.com> wrote:
> > >
> > > > > On Thu, 30 Mar 2017 18:27:21 +0800
> > > > > Xiong Zhang <xiong.y.zhang@intel.com> wrote:
> > > > >
> > > > > > This reverts commit
> c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> > > > > >
> > > > > > The original patch intend to prevent linux i915 driver from using
> > > > > > stolen meory. But this patch breaks windows IGD driver loading on
> > > > > > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows
> IGD
> > > > > > driver see zero size stolen memory, it will unload.
> > > > > > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > > > > > a guest.
> > > > >
> > > > > Does this mean that legacy mode IGD assignment is not going to work
> > > > > on Gen9+ with Windows?  Will it continue to work with Gen8-?
> > > > [Zhang, Xiong Y] I try to use the following qemu command to enable
> legacy mode on SKyLake, but It seems the entry point of wins IGD driver isn't
> called(I couldn't confirm this as I don't have the source code, but I didn't see
> any IGD driver info from windbg while I could see many info in upt mode), so
> driver doesn't bind to IGD after win 8.1 boot up.
> > > >   #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none
> -nographic -cpu host -hda "$IMAGE" -device
> vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
> > > > Is this the right method to enable legacy mode ?
> > >
> > > Yeah, that should do it.  x-vga should not be necessary, but shouldn't
> > > hurt IIRC.  Any dmesg errors regarding the ROM?  I think we have
> > > trouble with the ROM if the host is booted in UEFI mode.
> >
> > 1.
> > here is dmesg messages when host is booted with CSM mode enabled
> > and host's bios load option rom on boot:
> >
> > [165041.359929] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes:
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> > [165073.898940] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes:
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> > [165074.687515] vfio-pci 0000:00:02.0: enabling device (0400 -> 0403)
> > [165074.791598] vfio_ecap_init: 0000:00:02.0 hiding ecap 0x1b@0x100
> >
> > have output on monitor connected via HDMI
> >
> > 2.
> > and here is dmesg in pure UEFI mode (where host's bios doesn't load option
> rom):
> >
> > [   21.034983] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes:
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> > [   22.025361] vfio_ecap_init: 0000:00:02.0 hiding ecap 0x1b@0x100
> > [   22.030970] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature:
> expecting 0xaa55, got 0xffff
> > [   22.031036] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature:
> expecting 0xaa55, got 0xffff
> > [   24.738793] vfio-pci 0000:00:02.0: Invalid PCI ROM header signature:
> expecting 0xaa55, got 0xffff
> > [   27.776904] vfio-pci 0000:00:02.0: vgaarb: changed VGA decodes:
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> >
> > no output on monitor connected via HDMI when using Kabylake CPU
> (HD630)
> > but pure UEFI mode used to work (external output) with Skylake CPU
> (HD510)
> 
> In my limited experience, I've never been able to use IGD assignment on
> a pure UEFI host unless I provide the ROM via file.  For instance on my
> laptop I switched to CSM and booted a live CD to dump the ROM, which I
> can then use regardless of the host BIOS mode.
> 
> I just updated my rom-parser to include a version to fixup ROMs for
> vendor/device ID and checksum which should help with this:
> 
> https://github.com/awilliam/rom-parser
> 
> Update the device ID to match your ROM and let it fix the checksum.
> Maybe Xiong has some insight into why the VGA ROM often doesn't seem to
> exist on native pure UEFI hosts.
> 
[Zhang, Xiong Y] According to my limited UEFI knowledge, I could explain this, but it may be wrong.
VGA ROM seems a graphic driver in bios, and supply graphic service to grub and the early phase of OS.
The content of IGD VGA ROM is filled by system bios.
1. turn on csm, bios supply legacy bios intxx service to grub and OS. VGA ROM contain a 16 bit image as a vga bios. 
This image is what we want in vfio legacy mode. 
2. turn off csm, it is native uefi mode and supply uefi runtime service to grub and OS, the interface is gst->ConOut and gst->RuntimeServices.
The backend of gst->ConOut for graphic card is GOP(graphics output protocol). How to implement IGD GOP, it could be a 32 bit image in VGA ROM, or a uefi driver. So in native uefi, VGA ROM could be one of three form:
a. no contents,  GOP uefi driver in bios could implement all the function.
b. 32 bit image   supply driver for GOP
c. 16 bit image + 32 bit image

Attach UEFI GOP definition:
11.9 Graphics Output Protocol
The goal of this section is to replace the functionality that currently exists with VGA hardware and
its corresponding video BIOS. The Graphics Output Protocol is a software abstraction and its goal is
to support any foreseeable graphics hardware and not require VGA hardware, while at the same time
also lending itself to implementation on the current generation of VGA hardware.
Graphics output is important in the pre-boot space to support modern firmware features. These
features include the display of logos, the localization of output to any language, and setup and
configuration screens.
Graphics output may also be required as part of the startup of an operating system. There are
potentially times in modern operating systems prior to the loading of a high performance OS
graphics driver where access to graphics output device is required. The Graphics Output Protocol
supports this capability by providing the EFI OS loader access to a hardware frame buffer and
enough information to allow the OS to draw directly to the graphics output device.
The EFI_GRAPHICS_OUTPUT_PROTOCOL supports three member functions to support the
limited graphics needs of the pre-boot environment. These member functions allow the caller to
draw to a virtualized frame buffer, retrieve the supported video modes, and to set a video mode.
These simple primitives are sufficient to support the general needs of pre-OS firmware code.
The EFI_GRAPHICS_OUTPUT_PROTOCOL also exports enough information about the current
mode for operating system startup software to access the linear frame buffer directly.
thanks
> Alex