[PATCH v2 0/9] vfio/igd: Enable legacy mode on more devices

[PATCH v2 0/9] vfio/igd: Enable legacy mode on more devices

[Tomita Moeko]

This patchset extends the support of legacy mode igd passthrough to
all Intel Gen 11 and 12 devices (including Ice Lake, Jasper Lake,
Rocket Lake, Alder Lake and Raptor Lake), and emulates GGC register
in MMIO BAR0 for better compatibiltiy (It is tested Windows and GOP
driver will read this MMIO register).

It also replaces magic numbers with macros to improve readability,
and aligns behavior (BDSM registor mirroring and GGMS calculation for
gen7) with i915 driver to avoid possible issues.

The x-igd-gms option removed in 971ca22f041b ("vfio/igd: don't set
stolen memory size to zero") is also added back so that data stolen
memory size can be specified for guest. It is tested that GMS may
related to framebuffer size, a small GMS value may cause display issues
like blackscreen. It can be changed by DVMT Pre-allocated option in
host BIOS, but not all BIOS comes with this option. Having it in QEMU
helps resolves such issues.

This patchset was verified on Intel i9-12900K CPU(UHD 770, 8086:4680)
with custom OVMF firmware [1] and IntelGopDriver extracted from host
bios. IGD device works well in both Windows and Linux guests, and
scored 726 in 3DMark Time Spy Graphics on Windows guest.

[1] https://github.com/tomitamoeko/edk2/commits/igd-pt-adl/

Btw, IO BAR4 seems never be used by guest, and it the IO BAR itself
is not working on Gen11+ devices in my experiments. There is no hints
about that in old commit message and mailing list. It would be greatly
appreciated if someone shares the background.

Changelog:
v2:
* Droped "vfio/igd: fix GTT stolen memory size calculation for gen 7".
* Fixed conditions when calculating GGMS size.
* Added Gemini Lake and Comet Lake device ids.
* Splited mirroring register declaration macro into a new patch.
* Minor fixes.
Link: https://lore.kernel.org/qemu-devel/20241201160938.44355-1-tomitamoeko@gmail.com/

Tomita Moeko (9):
  vfio/igd: remove unsupported device ids
  vfio/igd: align generation with i915 kernel driver
  vfio/igd: canonicalize memory size calculations
  vfio/igd: add Gemini Lake and Comet Lake device ids
  vfio/igd: add Alder/Raptor/Rocket/Ice/Jasper Lake device ids
  vfio/igd: add macro for declaring mirrored registers
  vfio/igd: emulate GGC register in mmio bar0
  vfio/igd: emulate BDSM in mmio bar0 for gen 6-10 devices
  vfio/igd: add x-igd-gms option back to set DSM region size for guest

 hw/vfio/igd.c | 248 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 151 insertions(+), 97 deletions(-)

-- 
2.45.2

[PATCH v2 1/9] vfio/igd: remove unsupported device ids

[Tomita Moeko]

Since e433f208973f ("vfio/igd: return an invalid generation for unknown
devices"), the default return of igd_gen() was changed to unsupported.
There is no need to filter out those unsupported devices.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Corvin Köhne <c.koehne@beckhoff.com>
Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 4047f4f071..6ba3045bf3 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -64,16 +64,6 @@ static int igd_gen(VFIOPCIDevice *vdev)
     }
 
     switch (vdev->device_id & 0xff00) {
-    /* Old, untested, unavailable, unknown */
-    case 0x0000:
-    case 0x2500:
-    case 0x2700:
-    case 0x2900:
-    case 0x2a00:
-    case 0x2e00:
-    case 0x3500:
-    case 0xa000:
-        return -1;
     /* SandyBridge, IvyBridge, ValleyView, Haswell */
     case 0x0100:
     case 0x0400:
-- 
2.45.2

[PATCH v2 2/9] vfio/igd: align generation with i915 kernel driver

[Tomita Moeko]

Define the igd device generations according to i915 kernel driver to
avoid confusion, and adjust comment placement to clearly reflect the
relationship between ids and devices.

The condition of how GTT stolen memory size is calculated is changed
accordingly as GGMS is in multiple of 2 starting from gen 8.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 6ba3045bf3..2ede72d243 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -59,33 +59,33 @@
  */
 static int igd_gen(VFIOPCIDevice *vdev)
 {
-    if ((vdev->device_id & 0xfff) == 0xa84) {
-        return 8; /* Broxton */
+    /*
+     * Device IDs for Broxton/Apollo Lake are 0x0a84, 0x1a84, 0x1a85, 0x5a84
+     * and 0x5a85
+     */
+    if ((vdev->device_id & 0xffe) == 0xa84) {
+        return 9;
     }
 
     switch (vdev->device_id & 0xff00) {
-    /* SandyBridge, IvyBridge, ValleyView, Haswell */
-    case 0x0100:
-    case 0x0400:
-    case 0x0a00:
-    case 0x0c00:
-    case 0x0d00:
-    case 0x0f00:
+    case 0x0100:    /* SandyBridge, IvyBridge */
         return 6;
-    /* BroadWell, CherryView, SkyLake, KabyLake */
-    case 0x1600:
-    case 0x1900:
-    case 0x2200:
-    case 0x5900:
+    case 0x0400:    /* Haswell */
+    case 0x0a00:    /* Haswell */
+    case 0x0c00:    /* Haswell */
+    case 0x0d00:    /* Haswell */
+    case 0x0f00:    /* Valleyview/Bay Trail */
+        return 7;
+    case 0x1600:    /* Broadwell */
+    case 0x2200:    /* Cherryview */
         return 8;
-    /* CoffeeLake */
-    case 0x3e00:
+    case 0x1900:    /* Skylake */
+    case 0x5900:    /* Kaby Lake */
+    case 0x3e00:    /* Coffee Lake */
         return 9;
-    /* ElkhartLake */
-    case 0x4500:
+    case 0x4500:    /* Elkhart Lake */
         return 11;
-    /* TigerLake */
-    case 0x9A00:
+    case 0x9A00:    /* Tiger Lake */
         return 12;
     }
 
@@ -258,7 +258,7 @@ static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
 
     gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
     ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
-    if (gen > 6) {
+    if (gen >= 8) {
         ggms = 1 << ggms;
     }
 
@@ -668,7 +668,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
 
     /* Determine the size of stolen memory needed for GTT */
     ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
-    if (gen > 6) {
+    if (gen >= 8) {
         ggms_mb = 1 << ggms_mb;
     }
 
-- 
2.45.2

Re: [PATCH v2 2/9] vfio/igd: align generation with i915 kernel driver

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 3561 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> Define the igd device generations according to i915 kernel driver to
> avoid confusion, and adjust comment placement to clearly reflect the
> relationship between ids and devices.
> 
> The condition of how GTT stolen memory size is calculated is changed
> accordingly as GGMS is in multiple of 2 starting from gen 8.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 6ba3045bf3..2ede72d243 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -59,33 +59,33 @@
>   */
>  static int igd_gen(VFIOPCIDevice *vdev)
>  {
> -    if ((vdev->device_id & 0xfff) == 0xa84) {
> -        return 8; /* Broxton */
> +    /*
> +     * Device IDs for Broxton/Apollo Lake are 0x0a84, 0x1a84, 0x1a85, 0x5a84
> +     * and 0x5a85
> +     */
> +    if ((vdev->device_id & 0xffe) == 0xa84) {
> +        return 9;
>      }
>  

I'd slightly prefer matching those ids explicitly. At some point it may even
make more sense to copy the pciids of Linux and reuse them [1].

Note that this is just a suggestion!

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/intel/i915_pciids.h?h=v6.12

>      switch (vdev->device_id & 0xff00) {
> -    /* SandyBridge, IvyBridge, ValleyView, Haswell */
> -    case 0x0100:
> -    case 0x0400:
> -    case 0x0a00:
> -    case 0x0c00:
> -    case 0x0d00:
> -    case 0x0f00:
> +    case 0x0100:    /* SandyBridge, IvyBridge */
>          return 6;
> -    /* BroadWell, CherryView, SkyLake, KabyLake */
> -    case 0x1600:
> -    case 0x1900:
> -    case 0x2200:
> -    case 0x5900:
> +    case 0x0400:    /* Haswell */
> +    case 0x0a00:    /* Haswell */
> +    case 0x0c00:    /* Haswell */
> +    case 0x0d00:    /* Haswell */
> +    case 0x0f00:    /* Valleyview/Bay Trail */
> +        return 7;
> +    case 0x1600:    /* Broadwell */
> +    case 0x2200:    /* Cherryview */
>          return 8;
> -    /* CoffeeLake */
> -    case 0x3e00:
> +    case 0x1900:    /* Skylake */
> +    case 0x5900:    /* Kaby Lake */
> +    case 0x3e00:    /* Coffee Lake */
>          return 9;
> -    /* ElkhartLake */
> -    case 0x4500:
> +    case 0x4500:    /* Elkhart Lake */
>          return 11;
> -    /* TigerLake */
> -    case 0x9A00:
> +    case 0x9A00:    /* Tiger Lake */
>          return 12;
>      }
>  
> @@ -258,7 +258,7 @@ static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
>  
>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
>      ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen > 6) {
> +    if (gen >= 8) {
>          ggms = 1 << ggms;
>      }
>  
> @@ -668,7 +668,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int
> nr)
>  
>      /* Determine the size of stolen memory needed for GTT */
>      ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen > 6) {
> +    if (gen >= 8) {
>          ggms_mb = 1 << ggms_mb;
>      }
>  

Reviewed-by: Corvin Köhne <c.koehne@beckhoff.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/9] vfio/igd: align generation with i915 kernel driver

[Alex Williamson]

On Tue,  3 Dec 2024 21:35:41 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> Define the igd device generations according to i915 kernel driver to
> avoid confusion, and adjust comment placement to clearly reflect the
> relationship between ids and devices.
> 
> The condition of how GTT stolen memory size is calculated is changed
> accordingly as GGMS is in multiple of 2 starting from gen 8.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 6ba3045bf3..2ede72d243 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -59,33 +59,33 @@
>   */
>  static int igd_gen(VFIOPCIDevice *vdev)
>  {
> -    if ((vdev->device_id & 0xfff) == 0xa84) {
> -        return 8; /* Broxton */
> +    /*
> +     * Device IDs for Broxton/Apollo Lake are 0x0a84, 0x1a84, 0x1a85, 0x5a84
> +     * and 0x5a85
> +     */

Your comment from review of v1 would be useful here that we can't use
the test below, at least for 0x0a84, because it conflicts with Haswell.
I'd forgotten that.

Since we're being more strict about what we support now, it may make
sense to list specific IDs rather than this sloppy match, as Corvin
suggested, but that could be a follow-on.  Thanks,

Alex

> +    if ((vdev->device_id & 0xffe) == 0xa84) {
> +        return 9;
>      }
>  
>      switch (vdev->device_id & 0xff00) {
> -    /* SandyBridge, IvyBridge, ValleyView, Haswell */
> -    case 0x0100:
> -    case 0x0400:
> -    case 0x0a00:
> -    case 0x0c00:
> -    case 0x0d00:
> -    case 0x0f00:
> +    case 0x0100:    /* SandyBridge, IvyBridge */
>          return 6;
> -    /* BroadWell, CherryView, SkyLake, KabyLake */
> -    case 0x1600:
> -    case 0x1900:
> -    case 0x2200:
> -    case 0x5900:
> +    case 0x0400:    /* Haswell */
> +    case 0x0a00:    /* Haswell */
> +    case 0x0c00:    /* Haswell */
> +    case 0x0d00:    /* Haswell */
> +    case 0x0f00:    /* Valleyview/Bay Trail */
> +        return 7;
> +    case 0x1600:    /* Broadwell */
> +    case 0x2200:    /* Cherryview */
>          return 8;
> -    /* CoffeeLake */
> -    case 0x3e00:
> +    case 0x1900:    /* Skylake */
> +    case 0x5900:    /* Kaby Lake */
> +    case 0x3e00:    /* Coffee Lake */
>          return 9;
> -    /* ElkhartLake */
> -    case 0x4500:
> +    case 0x4500:    /* Elkhart Lake */
>          return 11;
> -    /* TigerLake */
> -    case 0x9A00:
> +    case 0x9A00:    /* Tiger Lake */
>          return 12;
>      }
>  
> @@ -258,7 +258,7 @@ static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
>  
>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
>      ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen > 6) {
> +    if (gen >= 8) {
>          ggms = 1 << ggms;
>      }
>  
> @@ -668,7 +668,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>  
>      /* Determine the size of stolen memory needed for GTT */
>      ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen > 6) {
> +    if (gen >= 8) {
>          ggms_mb = 1 << ggms_mb;
>      }
>  

Re: [PATCH v2 2/9] vfio/igd: align generation with i915 kernel driver

[Tomita Moeko]

On 12/5/24 06:36, Alex Williamson wrote:
> On Tue,  3 Dec 2024 21:35:41 +0800
> Tomita Moeko <tomitamoeko@gmail.com> wrote:
> 
>> Define the igd device generations according to i915 kernel driver to
>> avoid confusion, and adjust comment placement to clearly reflect the
>> relationship between ids and devices.
>>
>> The condition of how GTT stolen memory size is calculated is changed
>> accordingly as GGMS is in multiple of 2 starting from gen 8.
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>>  hw/vfio/igd.c | 44 ++++++++++++++++++++++----------------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index 6ba3045bf3..2ede72d243 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -59,33 +59,33 @@
>>   */
>>  static int igd_gen(VFIOPCIDevice *vdev)
>>  {
>> -    if ((vdev->device_id & 0xfff) == 0xa84) {
>> -        return 8; /* Broxton */
>> +    /*
>> +     * Device IDs for Broxton/Apollo Lake are 0x0a84, 0x1a84, 0x1a85, 0x5a84
>> +     * and 0x5a85
>> +     */
> 
> Your comment from review of v1 would be useful here that we can't use
> the test below, at least for 0x0a84, because it conflicts with Haswell.
> I'd forgotten that.

Sure I will add it to comments in v3.

> Since we're being more strict about what we support now, it may make
> sense to list specific IDs rather than this sloppy match, as Corvin
> suggested, but that could be a follow-on.  Thanks,
> 
> Alex

I personally prefer do it later, it take some time to think a proper
way to maintain such a long device id list.

>> +    if ((vdev->device_id & 0xffe) == 0xa84) {
>> +        return 9;
>>      }
>>  
>>      switch (vdev->device_id & 0xff00) {
>> -    /* SandyBridge, IvyBridge, ValleyView, Haswell */
>> -    case 0x0100:
>> -    case 0x0400:
>> -    case 0x0a00:
>> -    case 0x0c00:
>> -    case 0x0d00:
>> -    case 0x0f00:
>> +    case 0x0100:    /* SandyBridge, IvyBridge */
>>          return 6;
>> -    /* BroadWell, CherryView, SkyLake, KabyLake */
>> -    case 0x1600:
>> -    case 0x1900:
>> -    case 0x2200:
>> -    case 0x5900:
>> +    case 0x0400:    /* Haswell */
>> +    case 0x0a00:    /* Haswell */
>> +    case 0x0c00:    /* Haswell */
>> +    case 0x0d00:    /* Haswell */
>> +    case 0x0f00:    /* Valleyview/Bay Trail */
>> +        return 7;
>> +    case 0x1600:    /* Broadwell */
>> +    case 0x2200:    /* Cherryview */
>>          return 8;
>> -    /* CoffeeLake */
>> -    case 0x3e00:
>> +    case 0x1900:    /* Skylake */
>> +    case 0x5900:    /* Kaby Lake */
>> +    case 0x3e00:    /* Coffee Lake */
>>          return 9;
>> -    /* ElkhartLake */
>> -    case 0x4500:
>> +    case 0x4500:    /* Elkhart Lake */
>>          return 11;
>> -    /* TigerLake */
>> -    case 0x9A00:
>> +    case 0x9A00:    /* Tiger Lake */
>>          return 12;
>>      }
>>  
>> @@ -258,7 +258,7 @@ static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
>>  
>>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
>>      ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
>> -    if (gen > 6) {
>> +    if (gen >= 8) {
>>          ggms = 1 << ggms;
>>      }
>>  
>> @@ -668,7 +668,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>  
>>      /* Determine the size of stolen memory needed for GTT */
>>      ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
>> -    if (gen > 6) {
>> +    if (gen >= 8) {
>>          ggms_mb = 1 << ggms_mb;
>>      }
>>  
> 

[PATCH v2 3/9] vfio/igd: canonicalize memory size calculations

[Tomita Moeko]

Add helper functions igd_gtt_memory_size() and igd_stolen_size() for
calculating GTT stolen memory and Data stolen memory size in bytes,
and use macros to replace the hardware-related magic numbers for
better readability.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 99 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 55 insertions(+), 44 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 2ede72d243..b5bfdc6580 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -106,6 +106,51 @@ typedef struct VFIOIGDQuirk {
 #define IGD_BDSM 0x5c /* Base Data of Stolen Memory */
 #define IGD_BDSM_GEN11 0xc0 /* Base Data of Stolen Memory of gen 11 and later */
 
+#define IGD_GMCH_GEN6_GMS_SHIFT     3       /* SNB_GMCH in i915 */
+#define IGD_GMCH_GEN6_GMS_MASK      0x1f
+#define IGD_GMCH_GEN6_GGMS_SHIFT    8
+#define IGD_GMCH_GEN6_GGMS_MASK     0x3
+#define IGD_GMCH_GEN8_GMS_SHIFT     8       /* BDW_GMCH in i915 */
+#define IGD_GMCH_GEN8_GMS_MASK      0xff
+#define IGD_GMCH_GEN8_GGMS_SHIFT    6
+#define IGD_GMCH_GEN8_GGMS_MASK     0x3
+
+static uint64_t igd_gtt_memory_size(int gen, uint16_t gmch)
+{
+    uint64_t ggms;
+
+    if (gen < 8) {
+        ggms = (gmch >> IGD_GMCH_GEN6_GGMS_SHIFT) & IGD_GMCH_GEN6_GGMS_MASK;
+    } else {
+        ggms = (gmch >> IGD_GMCH_GEN8_GGMS_SHIFT) & IGD_GMCH_GEN8_GGMS_MASK;
+        ggms *= 2;
+    }
+
+    return ggms * MiB;
+}
+
+static uint64_t igd_stolen_memory_size(int gen, uint32_t gmch)
+{
+    uint64_t gms;
+
+    if (gen < 8) {
+        gms = (gmch >> IGD_GMCH_GEN6_GMS_SHIFT) & IGD_GMCH_GEN6_GMS_MASK;
+    } else {
+        gms = (gmch >> IGD_GMCH_GEN8_GMS_SHIFT) & IGD_GMCH_GEN8_GMS_MASK;
+    }
+
+    if (gen < 9) {
+            return gms * 32 * MiB;
+    } else {
+        if (gms < 0xf0) {
+            return gms * 32 * MiB;
+        } else {
+            return (gms - 0xf0 + 1) * 4 * MiB;
+        }
+    }
+
+    return 0;
+}
 
 /*
  * The rather short list of registers that we copy from the host devices.
@@ -254,17 +299,10 @@ static int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
 static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
 {
     uint32_t gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
-    int ggms, gen = igd_gen(vdev);
-
-    gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
-    ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
-    if (gen >= 8) {
-        ggms = 1 << ggms;
-    }
-
-    ggms *= MiB;
+    int gen = igd_gen(vdev);
+    uint64_t ggms_size = igd_gtt_memory_size(gen, gmch);
 
-    return (ggms / (4 * KiB)) * (gen < 8 ? 4 : 8);
+    return (ggms_size / (4 * KiB)) * (gen < 8 ? 4 : 8);
 }
 
 /*
@@ -471,30 +509,6 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
 }
 
-static int igd_get_stolen_mb(int gen, uint32_t gmch)
-{
-    int gms;
-
-    if (gen < 8) {
-        gms = (gmch >> 3) & 0x1f;
-    } else {
-        gms = (gmch >> 8) & 0xff;
-    }
-
-    if (gen < 9) {
-        if (gms > 0x10) {
-            error_report("Unsupported IGD GMS value 0x%x", gms);
-            return 0;
-        }
-        return gms * 32;
-    } else {
-        if (gms < 0xf0)
-            return gms * 32;
-        else
-            return (gms - 0xf0) * 4 + 4;
-    }
-}
-
 void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
 {
     g_autofree struct vfio_region_info *rom = NULL;
@@ -504,7 +518,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     VFIOQuirk *quirk;
     VFIOIGDQuirk *igd;
     PCIDevice *lpc_bridge;
-    int i, ret, ggms_mb, gms_mb = 0, gen;
+    int i, ret, gen;
+    uint64_t ggms_size, gms_size;
     uint64_t *bdsm_size;
     uint32_t gmch;
     uint16_t cmd_orig, cmd;
@@ -666,13 +681,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
 
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
 
-    /* Determine the size of stolen memory needed for GTT */
-    ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
-    if (gen >= 8) {
-        ggms_mb = 1 << ggms_mb;
-    }
-
-    gms_mb = igd_get_stolen_mb(gen, gmch);
+    ggms_size = igd_gtt_memory_size(gen, gmch);
+    gms_size = igd_stolen_memory_size(gen, gmch);
 
     /*
      * Request reserved memory for stolen memory via fw_cfg.  VM firmware
@@ -683,7 +693,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
      * config offset 0x5C.
      */
     bdsm_size = g_malloc(sizeof(*bdsm_size));
-    *bdsm_size = cpu_to_le64((ggms_mb + gms_mb) * MiB);
+    *bdsm_size = cpu_to_le64(ggms_size + gms_size);
     fw_cfg_add_file(fw_cfg_find(), "etc/igd-bdsm-size",
                     bdsm_size, sizeof(*bdsm_size));
 
@@ -734,5 +744,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
                      vdev->vbasedev.name);
     }
 
-    trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb);
+    trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name,
+                                    (ggms_size + gms_size) / MiB);
 }
-- 
2.45.2

Re: [PATCH v2 3/9] vfio/igd: canonicalize memory size calculations

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 6906 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> Add helper functions igd_gtt_memory_size() and igd_stolen_size() for
> calculating GTT stolen memory and Data stolen memory size in bytes,
> and use macros to replace the hardware-related magic numbers for
> better readability.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 99 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 55 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 2ede72d243..b5bfdc6580 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -106,6 +106,51 @@ typedef struct VFIOIGDQuirk {
>  #define IGD_BDSM 0x5c /* Base Data of Stolen Memory */
>  #define IGD_BDSM_GEN11 0xc0 /* Base Data of Stolen Memory of gen 11 and later
> */
>  
> +#define IGD_GMCH_GEN6_GMS_SHIFT     3       /* SNB_GMCH in i915 */
> +#define IGD_GMCH_GEN6_GMS_MASK      0x1f
> +#define IGD_GMCH_GEN6_GGMS_SHIFT    8
> +#define IGD_GMCH_GEN6_GGMS_MASK     0x3
> +#define IGD_GMCH_GEN8_GMS_SHIFT     8       /* BDW_GMCH in i915 */
> +#define IGD_GMCH_GEN8_GMS_MASK      0xff
> +#define IGD_GMCH_GEN8_GGMS_SHIFT    6
> +#define IGD_GMCH_GEN8_GGMS_MASK     0x3
> +
> +static uint64_t igd_gtt_memory_size(int gen, uint16_t gmch)
> +{
> +    uint64_t ggms;
> +
> +    if (gen < 8) {
> +        ggms = (gmch >> IGD_GMCH_GEN6_GGMS_SHIFT) & IGD_GMCH_GEN6_GGMS_MASK;
> +    } else {
> +        ggms = (gmch >> IGD_GMCH_GEN8_GGMS_SHIFT) & IGD_GMCH_GEN8_GGMS_MASK;
> +        ggms *= 2;
> +    }
> +
> +    return ggms * MiB;
> +}
> +
> +static uint64_t igd_stolen_memory_size(int gen, uint32_t gmch)
> +{
> +    uint64_t gms;
> +
> +    if (gen < 8) {
> +        gms = (gmch >> IGD_GMCH_GEN6_GMS_SHIFT) & IGD_GMCH_GEN6_GMS_MASK;
> +    } else {
> +        gms = (gmch >> IGD_GMCH_GEN8_GMS_SHIFT) & IGD_GMCH_GEN8_GMS_MASK;
> +    }
> +
> +    if (gen < 9) {
> +            return gms * 32 * MiB;
> +    } else {
> +        if (gms < 0xf0) {
> +            return gms * 32 * MiB;
> +        } else {
> +            return (gms - 0xf0 + 1) * 4 * MiB;
> +        }
> +    }
> +
> +    return 0;
> +}
>  
>  /*
>   * The rather short list of registers that we copy from the host devices.
> @@ -254,17 +299,10 @@ static int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
>  static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
>  {
>      uint32_t gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH,
> sizeof(gmch));
> -    int ggms, gen = igd_gen(vdev);
> -
> -    gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
> -    ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen >= 8) {
> -        ggms = 1 << ggms;
> -    }
> -
> -    ggms *= MiB;
> +    int gen = igd_gen(vdev);
> +    uint64_t ggms_size = igd_gtt_memory_size(gen, gmch);
>  
> -    return (ggms / (4 * KiB)) * (gen < 8 ? 4 : 8);
> +    return (ggms_size / (4 * KiB)) * (gen < 8 ? 4 : 8);
>  }
>  
>  /*
> @@ -471,30 +509,6 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
>  }
>  
> -static int igd_get_stolen_mb(int gen, uint32_t gmch)
> -{
> -    int gms;
> -
> -    if (gen < 8) {
> -        gms = (gmch >> 3) & 0x1f;
> -    } else {
> -        gms = (gmch >> 8) & 0xff;
> -    }
> -
> -    if (gen < 9) {
> -        if (gms > 0x10) {
> -            error_report("Unsupported IGD GMS value 0x%x", gms);
> -            return 0;
> -        }
> -        return gms * 32;
> -    } else {
> -        if (gms < 0xf0)
> -            return gms * 32;
> -        else
> -            return (gms - 0xf0) * 4 + 4;
> -    }
> -}
> -
>  void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>  {
>      g_autofree struct vfio_region_info *rom = NULL;
> @@ -504,7 +518,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int
> nr)
>      VFIOQuirk *quirk;
>      VFIOIGDQuirk *igd;
>      PCIDevice *lpc_bridge;
> -    int i, ret, ggms_mb, gms_mb = 0, gen;
> +    int i, ret, gen;
> +    uint64_t ggms_size, gms_size;
>      uint64_t *bdsm_size;
>      uint32_t gmch;
>      uint16_t cmd_orig, cmd;
> @@ -666,13 +681,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int
> nr)
>  
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
>  
> -    /* Determine the size of stolen memory needed for GTT */
> -    ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen >= 8) {
> -        ggms_mb = 1 << ggms_mb;
> -    }
> -
> -    gms_mb = igd_get_stolen_mb(gen, gmch);
> +    ggms_size = igd_gtt_memory_size(gen, gmch);
> +    gms_size = igd_stolen_memory_size(gen, gmch);
>  
>      /*
>       * Request reserved memory for stolen memory via fw_cfg.  VM firmware
> @@ -683,7 +693,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int
> nr)
>       * config offset 0x5C.
>       */
>      bdsm_size = g_malloc(sizeof(*bdsm_size));
> -    *bdsm_size = cpu_to_le64((ggms_mb + gms_mb) * MiB);
> +    *bdsm_size = cpu_to_le64(ggms_size + gms_size);
>      fw_cfg_add_file(fw_cfg_find(), "etc/igd-bdsm-size",
>                      bdsm_size, sizeof(*bdsm_size));
>  
> @@ -734,5 +744,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int
> nr)
>                       vdev-
> >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAADIUcZasZ36mwAAABM0
> 01Yig6LNmON7mS202pDuIRRNT-
> tIucgsQHKYe8WitBfoANcxBM2L6i4Sg4fLLMkXL_LDVUSEswEqsunuGsAr4DjgOogXtNMivhsyeaj9
> xYl9AgydV4QqrKMV29P7y3uAuqQcYz1GacVJRg1 );
>      }
>  
> -    trace_vfio_pci_igd_bdsm_enabled(vdev-
> >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAADIUcZasZ36mwAAABM0
> 01Yig6LNmON7mS202pDuIRRNT-
> tIucgsQHKYe8WitBfoANcxBM2L6i4Sg4fLLMkXL_LDVUSEswEqsunuGsAr4DjgOogXtNMivhsyeaj9
> xYl9AgydV4QqrKMV29P7y3uAuqQcYz1GacVJRg1 , ggms_mb + gms_mb);
> +    trace_vfio_pci_igd_bdsm_enabled(vdev-
> >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAADIUcZasZ36mwAAABM0
> 01Yig6LNmON7mS202pDuIRRNT-
> tIucgsQHKYe8WitBfoANcxBM2L6i4Sg4fLLMkXL_LDVUSEswEqsunuGsAr4DjgOogXtNMivhsyeaj9
> xYl9AgydV4QqrKMV29P7y3uAuqQcYz1GacVJRg1 ,
> +                                    (ggms_size + gms_size) / MiB);
>  }

Reviewed-by: Corvin Köhne <c.koehne@beckhoff.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/9] vfio/igd: canonicalize memory size calculations

[Alex Williamson]

On Tue,  3 Dec 2024 21:35:42 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> Add helper functions igd_gtt_memory_size() and igd_stolen_size() for
> calculating GTT stolen memory and Data stolen memory size in bytes,
> and use macros to replace the hardware-related magic numbers for
> better readability.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 99 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 55 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 2ede72d243..b5bfdc6580 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -106,6 +106,51 @@ typedef struct VFIOIGDQuirk {
>  #define IGD_BDSM 0x5c /* Base Data of Stolen Memory */
>  #define IGD_BDSM_GEN11 0xc0 /* Base Data of Stolen Memory of gen 11 and later */
>  
> +#define IGD_GMCH_GEN6_GMS_SHIFT     3       /* SNB_GMCH in i915 */
> +#define IGD_GMCH_GEN6_GMS_MASK      0x1f
> +#define IGD_GMCH_GEN6_GGMS_SHIFT    8
> +#define IGD_GMCH_GEN6_GGMS_MASK     0x3
> +#define IGD_GMCH_GEN8_GMS_SHIFT     8       /* BDW_GMCH in i915 */
> +#define IGD_GMCH_GEN8_GMS_MASK      0xff
> +#define IGD_GMCH_GEN8_GGMS_SHIFT    6
> +#define IGD_GMCH_GEN8_GGMS_MASK     0x3
> +
> +static uint64_t igd_gtt_memory_size(int gen, uint16_t gmch)
> +{
> +    uint64_t ggms;
> +
> +    if (gen < 8) {
> +        ggms = (gmch >> IGD_GMCH_GEN6_GGMS_SHIFT) & IGD_GMCH_GEN6_GGMS_MASK;
> +    } else {
> +        ggms = (gmch >> IGD_GMCH_GEN8_GGMS_SHIFT) & IGD_GMCH_GEN8_GGMS_MASK;
> +        ggms *= 2;

I tried to ask whether this was a bug fix in the previous iteration,
but I think it was overlooked.  These are not the same:

	ggms *= 2;

	ggms = 1 << ggms;

Comparing the 4th processor generation datasheet[1] to that of the 5th
generation processor[2], I see:

4th:
	0x0 = No Preallocated Memory
	0x1 = 1MB of Preallocated Memory
	0x2 = 2MB of Preallocated Memory
	0x3 = Reserved

5th:
	0x0 = No Preallocated Memory
	0x1 = 2MB of Preallocated Memory
	0x2 = 4MB of Preallocated Memory
	0x3 = 8MB of Preallocated Memory

In your update, we'd get ggms values of 2, 4, and 6, which is
incorrect.  The existing code is correct to use the ggms value as the
exponent, 2^1 = 2, 2^2 = 4, 2^3 = 8.  It does seem there's a bug at
zero though since 2^0 = 1, so maybe we should pull out the fix to a
separate patch:

	if (ggms) {
		ggms = 1 << ggms;
	}

Thanks,
Alex

[1]https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf (3.1.13)
[2]https://www.intel.com/content/www/us/en/content-details/330835/5th-generation-intel-core-processor-family-volume-2-of-2-datasheet.html (3.1.13)

> +    }
> +
> +    return ggms * MiB;
> +}
> +
> +static uint64_t igd_stolen_memory_size(int gen, uint32_t gmch)
> +{
> +    uint64_t gms;
> +
> +    if (gen < 8) {
> +        gms = (gmch >> IGD_GMCH_GEN6_GMS_SHIFT) & IGD_GMCH_GEN6_GMS_MASK;
> +    } else {
> +        gms = (gmch >> IGD_GMCH_GEN8_GMS_SHIFT) & IGD_GMCH_GEN8_GMS_MASK;
> +    }
> +
> +    if (gen < 9) {
> +            return gms * 32 * MiB;
> +    } else {
> +        if (gms < 0xf0) {
> +            return gms * 32 * MiB;
> +        } else {
> +            return (gms - 0xf0 + 1) * 4 * MiB;
> +        }
> +    }
> +
> +    return 0;
> +}
>  
>  /*
>   * The rather short list of registers that we copy from the host devices.
> @@ -254,17 +299,10 @@ static int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
>  static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
>  {
>      uint32_t gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
> -    int ggms, gen = igd_gen(vdev);
> -
> -    gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
> -    ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen >= 8) {
> -        ggms = 1 << ggms;
> -    }
> -
> -    ggms *= MiB;
> +    int gen = igd_gen(vdev);
> +    uint64_t ggms_size = igd_gtt_memory_size(gen, gmch);
>  
> -    return (ggms / (4 * KiB)) * (gen < 8 ? 4 : 8);
> +    return (ggms_size / (4 * KiB)) * (gen < 8 ? 4 : 8);
>  }
>  
>  /*
> @@ -471,30 +509,6 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
>  }
>  
> -static int igd_get_stolen_mb(int gen, uint32_t gmch)
> -{
> -    int gms;
> -
> -    if (gen < 8) {
> -        gms = (gmch >> 3) & 0x1f;
> -    } else {
> -        gms = (gmch >> 8) & 0xff;
> -    }
> -
> -    if (gen < 9) {
> -        if (gms > 0x10) {
> -            error_report("Unsupported IGD GMS value 0x%x", gms);
> -            return 0;
> -        }
> -        return gms * 32;
> -    } else {
> -        if (gms < 0xf0)
> -            return gms * 32;
> -        else
> -            return (gms - 0xf0) * 4 + 4;
> -    }
> -}
> -
>  void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>  {
>      g_autofree struct vfio_region_info *rom = NULL;
> @@ -504,7 +518,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>      VFIOQuirk *quirk;
>      VFIOIGDQuirk *igd;
>      PCIDevice *lpc_bridge;
> -    int i, ret, ggms_mb, gms_mb = 0, gen;
> +    int i, ret, gen;
> +    uint64_t ggms_size, gms_size;
>      uint64_t *bdsm_size;
>      uint32_t gmch;
>      uint16_t cmd_orig, cmd;
> @@ -666,13 +681,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>  
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
>  
> -    /* Determine the size of stolen memory needed for GTT */
> -    ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen >= 8) {
> -        ggms_mb = 1 << ggms_mb;
> -    }
> -
> -    gms_mb = igd_get_stolen_mb(gen, gmch);
> +    ggms_size = igd_gtt_memory_size(gen, gmch);
> +    gms_size = igd_stolen_memory_size(gen, gmch);
>  
>      /*
>       * Request reserved memory for stolen memory via fw_cfg.  VM firmware
> @@ -683,7 +693,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>       * config offset 0x5C.
>       */
>      bdsm_size = g_malloc(sizeof(*bdsm_size));
> -    *bdsm_size = cpu_to_le64((ggms_mb + gms_mb) * MiB);
> +    *bdsm_size = cpu_to_le64(ggms_size + gms_size);
>      fw_cfg_add_file(fw_cfg_find(), "etc/igd-bdsm-size",
>                      bdsm_size, sizeof(*bdsm_size));
>  
> @@ -734,5 +744,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>                       vdev->vbasedev.name);
>      }
>  
> -    trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb);
> +    trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name,
> +                                    (ggms_size + gms_size) / MiB);
>  }

Re: [PATCH v2 3/9] vfio/igd: canonicalize memory size calculations

[Tomita Moeko]

On 12/5/24 06:35, Alex Williamson wrote:
> On Tue,  3 Dec 2024 21:35:42 +0800
> Tomita Moeko <tomitamoeko@gmail.com> wrote:
> 
>> Add helper functions igd_gtt_memory_size() and igd_stolen_size() for
>> calculating GTT stolen memory and Data stolen memory size in bytes,
>> and use macros to replace the hardware-related magic numbers for
>> better readability.
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>>  hw/vfio/igd.c | 99 ++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 55 insertions(+), 44 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index 2ede72d243..b5bfdc6580 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -106,6 +106,51 @@ typedef struct VFIOIGDQuirk {
>>  #define IGD_BDSM 0x5c /* Base Data of Stolen Memory */
>>  #define IGD_BDSM_GEN11 0xc0 /* Base Data of Stolen Memory of gen 11 and later */
>>  
>> +#define IGD_GMCH_GEN6_GMS_SHIFT     3       /* SNB_GMCH in i915 */
>> +#define IGD_GMCH_GEN6_GMS_MASK      0x1f
>> +#define IGD_GMCH_GEN6_GGMS_SHIFT    8
>> +#define IGD_GMCH_GEN6_GGMS_MASK     0x3
>> +#define IGD_GMCH_GEN8_GMS_SHIFT     8       /* BDW_GMCH in i915 */
>> +#define IGD_GMCH_GEN8_GMS_MASK      0xff
>> +#define IGD_GMCH_GEN8_GGMS_SHIFT    6
>> +#define IGD_GMCH_GEN8_GGMS_MASK     0x3
>> +
>> +static uint64_t igd_gtt_memory_size(int gen, uint16_t gmch)
>> +{
>> +    uint64_t ggms;
>> +
>> +    if (gen < 8) {
>> +        ggms = (gmch >> IGD_GMCH_GEN6_GGMS_SHIFT) & IGD_GMCH_GEN6_GGMS_MASK;
>> +    } else {
>> +        ggms = (gmch >> IGD_GMCH_GEN8_GGMS_SHIFT) & IGD_GMCH_GEN8_GGMS_MASK;
>> +        ggms *= 2;
> 
> I tried to ask whether this was a bug fix in the previous iteration,
> but I think it was overlooked.  These are not the same:
> 
> 	ggms *= 2;
> 
> 	ggms = 1 << ggms;
> 
> Comparing the 4th processor generation datasheet[1] to that of the 5th
> generation processor[2], I see:
> 
> 4th:
> 	0x0 = No Preallocated Memory
> 	0x1 = 1MB of Preallocated Memory
> 	0x2 = 2MB of Preallocated Memory
> 	0x3 = Reserved
> 
> 5th:
> 	0x0 = No Preallocated Memory
> 	0x1 = 2MB of Preallocated Memory
> 	0x2 = 4MB of Preallocated Memory
> 	0x3 = 8MB of Preallocated Memory
> 
> In your update, we'd get ggms values of 2, 4, and 6, which is
> incorrect.  The existing code is correct to use the ggms value as the
> exponent, 2^1 = 2, 2^2 = 4, 2^3 = 8.  It does seem there's a bug at
> zero though since 2^0 = 1, so maybe we should pull out the fix to a
> separate patch:
> 
> 	if (ggms) {
> 		ggms = 1 << ggms;
> 	}
> 
> Thanks,
> Alex
> 
> [1]https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf (3.1.13)
> [2]https://www.intel.com/content/www/us/en/content-details/330835/5th-generation-intel-core-processor-family-volume-2-of-2-datasheet.html (3.1.13)

Sorry it was my mistake, the original code `1 << ggms` is correct.

I will create a new patch to fix it at first in v3. Thank you for
pointing out it.

>> +    }
>> +
>> +    return ggms * MiB;
>> +}
>> +
>> +static uint64_t igd_stolen_memory_size(int gen, uint32_t gmch)
>> +{
>> +    uint64_t gms;
>> +
>> +    if (gen < 8) {
>> +        gms = (gmch >> IGD_GMCH_GEN6_GMS_SHIFT) & IGD_GMCH_GEN6_GMS_MASK;
>> +    } else {
>> +        gms = (gmch >> IGD_GMCH_GEN8_GMS_SHIFT) & IGD_GMCH_GEN8_GMS_MASK;
>> +    }
>> +
>> +    if (gen < 9) {
>> +            return gms * 32 * MiB;
>> +    } else {
>> +        if (gms < 0xf0) {
>> +            return gms * 32 * MiB;
>> +        } else {
>> +            return (gms - 0xf0 + 1) * 4 * MiB;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>>  
>>  /*
>>   * The rather short list of registers that we copy from the host devices.
>> @@ -254,17 +299,10 @@ static int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
>>  static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
>>  {
>>      uint32_t gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
>> -    int ggms, gen = igd_gen(vdev);
>> -
>> -    gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
>> -    ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
>> -    if (gen >= 8) {
>> -        ggms = 1 << ggms;
>> -    }
>> -
>> -    ggms *= MiB;
>> +    int gen = igd_gen(vdev);
>> +    uint64_t ggms_size = igd_gtt_memory_size(gen, gmch);
>>  
>> -    return (ggms / (4 * KiB)) * (gen < 8 ? 4 : 8);
>> +    return (ggms_size / (4 * KiB)) * (gen < 8 ? 4 : 8);
>>  }
>>  
>>  /*
>> @@ -471,30 +509,6 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
>>  }
>>  
>> -static int igd_get_stolen_mb(int gen, uint32_t gmch)
>> -{
>> -    int gms;
>> -
>> -    if (gen < 8) {
>> -        gms = (gmch >> 3) & 0x1f;
>> -    } else {
>> -        gms = (gmch >> 8) & 0xff;
>> -    }
>> -
>> -    if (gen < 9) {
>> -        if (gms > 0x10) {
>> -            error_report("Unsupported IGD GMS value 0x%x", gms);
>> -            return 0;
>> -        }
>> -        return gms * 32;
>> -    } else {
>> -        if (gms < 0xf0)
>> -            return gms * 32;
>> -        else
>> -            return (gms - 0xf0) * 4 + 4;
>> -    }
>> -}
>> -
>>  void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>  {
>>      g_autofree struct vfio_region_info *rom = NULL;
>> @@ -504,7 +518,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>      VFIOQuirk *quirk;
>>      VFIOIGDQuirk *igd;
>>      PCIDevice *lpc_bridge;
>> -    int i, ret, ggms_mb, gms_mb = 0, gen;
>> +    int i, ret, gen;
>> +    uint64_t ggms_size, gms_size;
>>      uint64_t *bdsm_size;
>>      uint32_t gmch;
>>      uint16_t cmd_orig, cmd;
>> @@ -666,13 +681,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>  
>>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
>>  
>> -    /* Determine the size of stolen memory needed for GTT */
>> -    ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
>> -    if (gen >= 8) {
>> -        ggms_mb = 1 << ggms_mb;
>> -    }
>> -
>> -    gms_mb = igd_get_stolen_mb(gen, gmch);
>> +    ggms_size = igd_gtt_memory_size(gen, gmch);
>> +    gms_size = igd_stolen_memory_size(gen, gmch);
>>  
>>      /*
>>       * Request reserved memory for stolen memory via fw_cfg.  VM firmware
>> @@ -683,7 +693,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>       * config offset 0x5C.
>>       */
>>      bdsm_size = g_malloc(sizeof(*bdsm_size));
>> -    *bdsm_size = cpu_to_le64((ggms_mb + gms_mb) * MiB);
>> +    *bdsm_size = cpu_to_le64(ggms_size + gms_size);
>>      fw_cfg_add_file(fw_cfg_find(), "etc/igd-bdsm-size",
>>                      bdsm_size, sizeof(*bdsm_size));
>>  
>> @@ -734,5 +744,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>>                       vdev->vbasedev.name);
>>      }
>>  
>> -    trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, ggms_mb + gms_mb);
>> +    trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name,
>> +                                    (ggms_size + gms_size) / MiB);
>>  }
> 

[PATCH v2 4/9] vfio/igd: add Gemini Lake and Comet Lake device ids

[Tomita Moeko]

Both Gemini Lake and Comet Lake are gen 9 devices. Many user reports
on internet shows legacy mode of igd passthrough works as qemu treats
them as gen 8 devices by default before e433f208973f ("vfio/igd:
return an invalid generation for unknown devices").

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index b5bfdc6580..7f389de7ac 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -80,8 +80,10 @@ static int igd_gen(VFIOPCIDevice *vdev)
     case 0x2200:    /* Cherryview */
         return 8;
     case 0x1900:    /* Skylake */
+    case 0x3100:    /* Gemini Lake */
     case 0x5900:    /* Kaby Lake */
     case 0x3e00:    /* Coffee Lake */
+    case 0x9B00:    /* Comet Lake */
         return 9;
     case 0x4500:    /* Elkhart Lake */
         return 11;
-- 
2.45.2

Re: [PATCH v2 4/9] vfio/igd: add Gemini Lake and Comet Lake device ids

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> Both Gemini Lake and Comet Lake are gen 9 devices. Many user reports
> on internet shows legacy mode of igd passthrough works as qemu treats
> them as gen 8 devices by default before e433f208973f ("vfio/igd:
> return an invalid generation for unknown devices").
> 

Are there any user reports you can link to in the commit message?

> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index b5bfdc6580..7f389de7ac 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -80,8 +80,10 @@ static int igd_gen(VFIOPCIDevice *vdev)
>      case 0x2200:    /* Cherryview */
>          return 8;
>      case 0x1900:    /* Skylake */
> +    case 0x3100:    /* Gemini Lake */
>      case 0x5900:    /* Kaby Lake */
>      case 0x3e00:    /* Coffee Lake */
> +    case 0x9B00:    /* Comet Lake */
>          return 9;
>      case 0x4500:    /* Elkhart Lake */
>          return 11;

Reviewed-by: Corvin Köhne <c.koehne@beckhoff.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 5/9] vfio/igd: add Alder/Raptor/Rocket/Ice/Jasper Lake device ids

[Tomita Moeko]

All gen 11 and 12 igd devices have 64 bit BDSM register at 0xC0 in its
config space, add them to the list to support igd passthrough on Alder/
Raptor/Rocket/Ice/Jasper Lake platforms.

Tested legacy mode of igd passthrough works properly on both linux and
windows guests with AlderLake-S GT1 (8086:4680).

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 7f389de7ac..fea9be0b2d 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -85,9 +85,14 @@ static int igd_gen(VFIOPCIDevice *vdev)
     case 0x3e00:    /* Coffee Lake */
     case 0x9B00:    /* Comet Lake */
         return 9;
+    case 0x8A00:    /* Ice Lake */
     case 0x4500:    /* Elkhart Lake */
+    case 0x4E00:    /* Jasper Lake */
         return 11;
     case 0x9A00:    /* Tiger Lake */
+    case 0x4C00:    /* Rocket Lake */
+    case 0x4600:    /* Alder Lake */
+    case 0xA700:    /* Raptor Lake */
         return 12;
     }
 
-- 
2.45.2

Re: [PATCH v2 5/9] vfio/igd: add Alder/Raptor/Rocket/Ice/Jasper Lake device ids

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> All gen 11 and 12 igd devices have 64 bit BDSM register at 0xC0 in its
> config space, add them to the list to support igd passthrough on Alder/
> Raptor/Rocket/Ice/Jasper Lake platforms.
> 
> Tested legacy mode of igd passthrough works properly on both linux and
> windows guests with AlderLake-S GT1 (8086:4680).
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 7f389de7ac..fea9be0b2d 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -85,9 +85,14 @@ static int igd_gen(VFIOPCIDevice *vdev)
>      case 0x3e00:    /* Coffee Lake */
>      case 0x9B00:    /* Comet Lake */
>          return 9;
> +    case 0x8A00:    /* Ice Lake */
>      case 0x4500:    /* Elkhart Lake */
> +    case 0x4E00:    /* Jasper Lake */
>          return 11;
>      case 0x9A00:    /* Tiger Lake */
> +    case 0x4C00:    /* Rocket Lake */
> +    case 0x4600:    /* Alder Lake */
> +    case 0xA700:    /* Raptor Lake */
>          return 12;
>      }
>  

Reviewed-by: Corvin Köhne <c.koehne@beckhoff.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 6/9] vfio/igd: add macro for declaring mirrored registers

[Tomita Moeko]

igd devices have multipe registers mirroring mmio address and pci
config space, more than a single BDSM register. To support this,
the read/write functions are made common and a macro is defined to
simplify the declaration of MemoryRegionOps.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 60 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index fea9be0b2d..522845c509 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -418,16 +418,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-#define IGD_BDSM_MMIO_OFFSET 0x1080C0
-
-static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
-                                          hwaddr addr, unsigned size)
+static uint64_t vfio_igd_pci_config_read(VFIOPCIDevice *vdev, uint64_t offset,
+                                         unsigned size)
 {
-    VFIOPCIDevice *vdev = opaque;
-    uint64_t offset;
-
-    offset = IGD_BDSM_GEN11 + addr;
-
     switch (size) {
     case 1:
         return pci_get_byte(vdev->pdev.config + offset);
@@ -438,21 +431,17 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
     case 8:
         return pci_get_quad(vdev->pdev.config + offset);
     default:
-        hw_error("igd: unsupported read size, %u bytes", size);
+        hw_error("igd: unsupported pci config read at %lx, size %u",
+                 offset, size);
         break;
     }
 
     return 0;
 }
 
-static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
-                                       uint64_t data, unsigned size)
+static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
+                                      uint64_t data, unsigned size)
 {
-    VFIOPCIDevice *vdev = opaque;
-    uint64_t offset;
-
-    offset = IGD_BDSM_GEN11 + addr;
-
     switch (size) {
     case 1:
         pci_set_byte(vdev->pdev.config + offset, data);
@@ -467,17 +456,37 @@ static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
         pci_set_quad(vdev->pdev.config + offset, data);
         break;
     default:
-        hw_error("igd: unsupported read size, %u bytes", size);
+        hw_error("igd: unsupported pci config write at %lx, size %u",
+                 offset, size);
         break;
     }
 }
 
-static const MemoryRegionOps vfio_igd_bdsm_quirk = {
-    .read = vfio_igd_quirk_bdsm_read,
-    .write = vfio_igd_quirk_bdsm_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
+#define VFIO_IGD_QUIRK_MIRROR_REG(reg, name)                            \
+static uint64_t vfio_igd_quirk_read_##name(void *opaque,                \
+                                           hwaddr addr, unsigned size)  \
+{                                                                       \
+    VFIOPCIDevice *vdev = opaque;                                       \
+    return vfio_igd_pci_config_read(vdev, reg + addr, size);            \
+}                                                                       \
+                                                                        \
+static void vfio_igd_quirk_write_##name(void *opaque, hwaddr addr,      \
+                                        uint64_t data, unsigned size)   \
+{                                                                       \
+    VFIOPCIDevice *vdev = opaque;                                       \
+    vfio_igd_pci_config_write(vdev, reg + addr, data, size);            \
+}                                                                       \
+                                                                        \
+static const MemoryRegionOps vfio_igd_quirk_mirror_##name = {           \
+    .read = vfio_igd_quirk_read_##name,                                 \
+    .write = vfio_igd_quirk_write_##name,                               \
+    .endianness = DEVICE_LITTLE_ENDIAN,                                 \
 };
 
+VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
+
+#define IGD_BDSM_MMIO_OFFSET    0x1080C0
+
 void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
 {
     VFIOQuirk *quirk;
@@ -507,10 +516,11 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
     quirk = vfio_quirk_alloc(1);
     quirk->data = vdev;
 
-    memory_region_init_io(&quirk->mem[0], OBJECT(vdev), &vfio_igd_bdsm_quirk,
-                          vdev, "vfio-igd-bdsm-quirk", 8);
+    memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
+                          &vfio_igd_quirk_mirror_bdsm, vdev,
+                          "vfio-igd-bdsm-quirk", 8);
     memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
-                                        IGD_BDSM_MMIO_OFFSET, &quirk->mem[0],
+                                        IGD_BDSM_MMIO_OFFSET, &quirk->mem[1],
                                         1);
 
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
-- 
2.45.2

Re: [PATCH v2 6/9] vfio/igd: add macro for declaring mirrored registers

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 6754 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> igd devices have multipe registers mirroring mmio address and pci
> config space, more than a single BDSM register. To support this,
> the read/write functions are made common and a macro is defined to
> simplify the declaration of MemoryRegionOps.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 60 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index fea9be0b2d..522845c509 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -418,16 +418,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> -#define IGD_BDSM_MMIO_OFFSET 0x1080C0
> -
> -static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
> -                                          hwaddr addr, unsigned size)
> +static uint64_t vfio_igd_pci_config_read(VFIOPCIDevice *vdev, uint64_t
> offset,
> +                                         unsigned size)
>  {
> -    VFIOPCIDevice *vdev = opaque;
> -    uint64_t offset;
> -
> -    offset = IGD_BDSM_GEN11 + addr;
> -
>      switch (size) {
>      case 1:
>          return pci_get_byte(vdev->pdev.config + offset);
> @@ -438,21 +431,17 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
>      case 8:
>          return pci_get_quad(vdev->pdev.config + offset);
>      default:
> -        hw_error("igd: unsupported read size, %u bytes", size);
> +        hw_error("igd: unsupported pci config read at %lx, size %u",
> +                 offset, size);
>          break;
>      }
>  
>      return 0;
>  }
>  
> -static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
> -                                       uint64_t data, unsigned size)
> +static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
> +                                      uint64_t data, unsigned size)
>  {
> -    VFIOPCIDevice *vdev = opaque;
> -    uint64_t offset;
> -
> -    offset = IGD_BDSM_GEN11 + addr;
> -
>      switch (size) {
>      case 1:
>          pci_set_byte(vdev->pdev.config + offset, data);
> @@ -467,17 +456,37 @@ static void vfio_igd_quirk_bdsm_write(void *opaque,
> hwaddr addr,
>          pci_set_quad(vdev->pdev.config + offset, data);
>          break;
>      default:
> -        hw_error("igd: unsupported read size, %u bytes", size);
> +        hw_error("igd: unsupported pci config write at %lx, size %u",
> +                 offset, size);
>          break;
>      }
>  }
>  
> -static const MemoryRegionOps vfio_igd_bdsm_quirk = {
> -    .read = vfio_igd_quirk_bdsm_read,
> -    .write = vfio_igd_quirk_bdsm_write,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> +#define VFIO_IGD_QUIRK_MIRROR_REG(reg, name)                            \
> +static uint64_t vfio_igd_quirk_read_##name(void *opaque,                \
> +                                           hwaddr addr, unsigned size)  \
> +{                                                                       \
> +    VFIOPCIDevice *vdev = opaque;                                       \
> +    return vfio_igd_pci_config_read(vdev, reg + addr, size);            \
> +}                                                                       \
> +                                                                        \
> +static void vfio_igd_quirk_write_##name(void *opaque, hwaddr addr,      \
> +                                        uint64_t data, unsigned size)   \
> +{                                                                       \
> +    VFIOPCIDevice *vdev = opaque;                                       \
> +    vfio_igd_pci_config_write(vdev, reg + addr, data, size);            \
> +}                                                                       \
> +                                                                        \
> +static const MemoryRegionOps vfio_igd_quirk_mirror_##name = {           \
> +    .read = vfio_igd_quirk_read_##name,                                 \
> +    .write = vfio_igd_quirk_write_##name,                               \
> +    .endianness = DEVICE_LITTLE_ENDIAN,                                 \
>  };
>  
> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
> +
> +#define IGD_BDSM_MMIO_OFFSET    0x1080C0
> +
>  void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>  {
>      VFIOQuirk *quirk;
> @@ -507,10 +516,11 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
>      quirk = vfio_quirk_alloc(1);
>      quirk->data = vdev;
>  
> -    memory_region_init_io(&quirk->mem[0], OBJECT(vdev), &vfio_igd_bdsm_quirk,
> -                          vdev, "vfio-igd-bdsm-quirk", 8);
> +    memory_region_init_io(&quirk->mem[1], OBJECT(vdev),

This should still be quirk->mem[0]. It slipped into the wrong commit.

> +                          &vfio_igd_quirk_mirror_bdsm, vdev,
> +                          "vfio-igd-bdsm-quirk", 8);
>      memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> -                                        IGD_BDSM_MMIO_OFFSET, &quirk->mem[0],
> +                                        IGD_BDSM_MMIO_OFFSET, &quirk->mem[1],

Same here.

>                                          1);
>  
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);

-- 
Kind regards,
Corvin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/9] vfio/igd: add macro for declaring mirrored registers

[Alex Williamson]

On Tue,  3 Dec 2024 21:35:45 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> igd devices have multipe registers mirroring mmio address and pci
> config space, more than a single BDSM register. To support this,
> the read/write functions are made common and a macro is defined to
> simplify the declaration of MemoryRegionOps.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 60 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index fea9be0b2d..522845c509 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -418,16 +418,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> -#define IGD_BDSM_MMIO_OFFSET 0x1080C0
> -
> -static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
> -                                          hwaddr addr, unsigned size)
> +static uint64_t vfio_igd_pci_config_read(VFIOPCIDevice *vdev, uint64_t offset,
> +                                         unsigned size)
>  {
> -    VFIOPCIDevice *vdev = opaque;
> -    uint64_t offset;
> -
> -    offset = IGD_BDSM_GEN11 + addr;
> -
>      switch (size) {
>      case 1:
>          return pci_get_byte(vdev->pdev.config + offset);
> @@ -438,21 +431,17 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
>      case 8:
>          return pci_get_quad(vdev->pdev.config + offset);
>      default:
> -        hw_error("igd: unsupported read size, %u bytes", size);
> +        hw_error("igd: unsupported pci config read at %lx, size %u",
> +                 offset, size);
>          break;
>      }
>  
>      return 0;
>  }
>  
> -static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
> -                                       uint64_t data, unsigned size)
> +static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
> +                                      uint64_t data, unsigned size)
>  {
> -    VFIOPCIDevice *vdev = opaque;
> -    uint64_t offset;
> -
> -    offset = IGD_BDSM_GEN11 + addr;
> -
>      switch (size) {
>      case 1:
>          pci_set_byte(vdev->pdev.config + offset, data);
> @@ -467,17 +456,37 @@ static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
>          pci_set_quad(vdev->pdev.config + offset, data);
>          break;
>      default:
> -        hw_error("igd: unsupported read size, %u bytes", size);
> +        hw_error("igd: unsupported pci config write at %lx, size %u",
> +                 offset, size);
>          break;
>      }
>  }
>  
> -static const MemoryRegionOps vfio_igd_bdsm_quirk = {
> -    .read = vfio_igd_quirk_bdsm_read,
> -    .write = vfio_igd_quirk_bdsm_write,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> +#define VFIO_IGD_QUIRK_MIRROR_REG(reg, name)                            \
> +static uint64_t vfio_igd_quirk_read_##name(void *opaque,                \
> +                                           hwaddr addr, unsigned size)  \
> +{                                                                       \
> +    VFIOPCIDevice *vdev = opaque;                                       \

I'm not sure if QEMU coding style requires this, but I'd still prefer
to see a blank line after variable declaration, even in a macro.

> +    return vfio_igd_pci_config_read(vdev, reg + addr, size);            \
> +}                                                                       \
> +                                                                        \
> +static void vfio_igd_quirk_write_##name(void *opaque, hwaddr addr,      \
> +                                        uint64_t data, unsigned size)   \
> +{                                                                       \
> +    VFIOPCIDevice *vdev = opaque;                                       \
> +    vfio_igd_pci_config_write(vdev, reg + addr, data, size);            \
> +}                                                                       \
> +                                                                        \
> +static const MemoryRegionOps vfio_igd_quirk_mirror_##name = {           \
> +    .read = vfio_igd_quirk_read_##name,                                 \
> +    .write = vfio_igd_quirk_write_##name,                               \
> +    .endianness = DEVICE_LITTLE_ENDIAN,                                 \
>  };
>  
> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
> +
> +#define IGD_BDSM_MMIO_OFFSET    0x1080C0
> +
>  void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>  {
>      VFIOQuirk *quirk;
> @@ -507,10 +516,11 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>      quirk = vfio_quirk_alloc(1);
>      quirk->data = vdev;
>  
> -    memory_region_init_io(&quirk->mem[0], OBJECT(vdev), &vfio_igd_bdsm_quirk,
> -                          vdev, "vfio-igd-bdsm-quirk", 8);
> +    memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
> +                          &vfio_igd_quirk_mirror_bdsm, vdev,
> +                          "vfio-igd-bdsm-quirk", 8);
>      memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> -                                        IGD_BDSM_MMIO_OFFSET, &quirk->mem[0],
> +                                        IGD_BDSM_MMIO_OFFSET, &quirk->mem[1],

As Corvin notes, changing the quirk memory region index here is a bug.
Thanks,

Alex

[PATCH v2 7/9] vfio/igd: emulate GGC register in mmio bar0

[Tomita Moeko]

The GGC register at 0x50 of pci config space is a mirror of the same
register at 0x108040 of mmio bar0 [1]. i915 driver also reads that
register from mmio bar0 instead of config space. As GGC is programmed
and emulated by qemu, the mmio address should also be emulated, in the
same way of BDSM register.

[1] 4.1.28, 12th Generation Intel Core Processors Datasheet Volume 2
    https://www.intel.com/content/www/us/en/content-details/655259

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 522845c509..bc18fc8cc0 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -483,8 +483,10 @@ static const MemoryRegionOps vfio_igd_quirk_mirror_##name = {           \
     .endianness = DEVICE_LITTLE_ENDIAN,                                 \
 };
 
+VFIO_IGD_QUIRK_MIRROR_REG(IGD_GMCH, ggc)
 VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
 
+#define IGD_GGC_MMIO_OFFSET     0x108040
 #define IGD_BDSM_MMIO_OFFSET    0x1080C0
 
 void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
@@ -513,9 +515,16 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
         return;
     }
 
-    quirk = vfio_quirk_alloc(1);
+    quirk = vfio_quirk_alloc(2);
     quirk->data = vdev;
 
+    memory_region_init_io(&quirk->mem[0], OBJECT(vdev),
+                          &vfio_igd_quirk_mirror_ggc, vdev,
+                          "vfio-igd-ggc-quirk", 2);
+    memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
+                                        IGD_GGC_MMIO_OFFSET, &quirk->mem[0],
+                                        1);
+
     memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
                           &vfio_igd_quirk_mirror_bdsm, vdev,
                           "vfio-igd-bdsm-quirk", 8);
-- 
2.45.2

Re: [PATCH v2 7/9] vfio/igd: emulate GGC register in mmio bar0

[Alex Williamson]

On Tue,  3 Dec 2024 21:35:46 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> The GGC register at 0x50 of pci config space is a mirror of the same
> register at 0x108040 of mmio bar0 [1]. i915 driver also reads that
> register from mmio bar0 instead of config space. As GGC is programmed
> and emulated by qemu, the mmio address should also be emulated, in the
> same way of BDSM register.
> 
> [1] 4.1.28, 12th Generation Intel Core Processors Datasheet Volume 2
>     https://www.intel.com/content/www/us/en/content-details/655259
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 522845c509..bc18fc8cc0 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -483,8 +483,10 @@ static const MemoryRegionOps vfio_igd_quirk_mirror_##name = {           \
>      .endianness = DEVICE_LITTLE_ENDIAN,                                 \
>  };
>  
> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_GMCH, ggc)
>  VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
>  
> +#define IGD_GGC_MMIO_OFFSET     0x108040
>  #define IGD_BDSM_MMIO_OFFSET    0x1080C0
>  
>  void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
> @@ -513,9 +515,16 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>          return;
>      }
>  
> -    quirk = vfio_quirk_alloc(1);
> +    quirk = vfio_quirk_alloc(2);
>      quirk->data = vdev;
>  
> +    memory_region_init_io(&quirk->mem[0], OBJECT(vdev),
> +                          &vfio_igd_quirk_mirror_ggc, vdev,
> +                          "vfio-igd-ggc-quirk", 2);
> +    memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> +                                        IGD_GGC_MMIO_OFFSET, &quirk->mem[0],
> +                                        1);
> +
>      memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
>                            &vfio_igd_quirk_mirror_bdsm, vdev,
>                            "vfio-igd-bdsm-quirk", 8);

Seems like trying to keep these ordered by offset is what introduced
the bug in the previous patch.  Let's not care about that, use the next
index and setup in index order.  Thanks,

Alex

[PATCH v2 8/9] vfio/igd: emulate BDSM in mmio bar0 for gen 6-10 devices

[Tomita Moeko]

A recent commit in i915 driver [1] claims the BDSM register at 0x1080c0
of mmio bar0 has been there since gen 6. Mirror this register to the 32
bit BDSM register at 0x5c in pci config space for gen6-10 devices.

[1] https://patchwork.freedesktop.org/patch/msgid/20240202224340.30647-7-ville.syrjala@linux.intel.com

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index bc18fc8cc0..e464cd6949 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -484,7 +484,8 @@ static const MemoryRegionOps vfio_igd_quirk_mirror_##name = {           \
 };
 
 VFIO_IGD_QUIRK_MIRROR_REG(IGD_GMCH, ggc)
-VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
+VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM, bdsm)
+VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm64)
 
 #define IGD_GGC_MMIO_OFFSET     0x108040
 #define IGD_BDSM_MMIO_OFFSET    0x1080C0
@@ -511,7 +512,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
      * into MMIO space and read from MMIO space by the Windows driver.
      */
     gen = igd_gen(vdev);
-    if (gen < 11) {
+    if (gen < 6) {
         return;
     }
 
@@ -525,12 +526,21 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
                                         IGD_GGC_MMIO_OFFSET, &quirk->mem[0],
                                         1);
 
-    memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
-                          &vfio_igd_quirk_mirror_bdsm, vdev,
-                          "vfio-igd-bdsm-quirk", 8);
-    memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
-                                        IGD_BDSM_MMIO_OFFSET, &quirk->mem[1],
-                                        1);
+    if (gen < 11) {
+        memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
+                              &vfio_igd_quirk_mirror_bdsm, vdev,
+                              "vfio-igd-bdsm-quirk", 4);
+        memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
+                                            IGD_BDSM_MMIO_OFFSET,
+                                            &quirk->mem[1], 1);
+    } else {
+        memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
+                              &vfio_igd_quirk_mirror_bdsm64, vdev,
+                              "vfio-igd-bdsm-quirk", 8);
+        memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
+                                            IGD_BDSM_MMIO_OFFSET,
+                                            &quirk->mem[1], 1);
+    }
 
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
 }
-- 
2.45.2

Re: [PATCH v2 8/9] vfio/igd: emulate BDSM in mmio bar0 for gen 6-10 devices

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 3775 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> A recent commit in i915 driver [1] claims the BDSM register at 0x1080c0
> of mmio bar0 has been there since gen 6. Mirror this register to the 32
> bit BDSM register at 0x5c in pci config space for gen6-10 devices.
> 
> [1]
> https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAACDiMFDAVNh97kAAACXccDLV2jHgpqoPX7MzgRYJ_QvhHcIgEu_cy5EZCuBkJVjGfUQRHJIyDHnpjPw2eMGixZwJxpCSnGEWYbfEqWqFznDz1dMDFEfcjfnUY47OWkKRXq2MCufjXyHUAP2VyS_NKnpJ0Va7uajM38HaSV4dvGWO_CRkUuqInMmibaFge3X_Sk0bzFI_MfYGVBN1TvIzITrYCLcmKwyp9-oS61DI7iv29IZB9I0WJURq0t1Nk0h-22zlXjxrQ2
>  
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index bc18fc8cc0..e464cd6949 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -484,7 +484,8 @@ static const MemoryRegionOps vfio_igd_quirk_mirror_##name
> = {           \
>  };
>  
>  VFIO_IGD_QUIRK_MIRROR_REG(IGD_GMCH, ggc)
> -VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM, bdsm)
> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm64)
>  
>  #define IGD_GGC_MMIO_OFFSET     0x108040
>  #define IGD_BDSM_MMIO_OFFSET    0x1080C0
> @@ -511,7 +512,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
>       * into MMIO space and read from MMIO space by the Windows driver.
>       */
>      gen = igd_gen(vdev);
> -    if (gen < 11) {
> +    if (gen < 6) {
>          return;
>      }
>  
> @@ -525,12 +526,21 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
>                                          IGD_GGC_MMIO_OFFSET, &quirk->mem[0],
>                                          1);
>  
> -    memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
> -                          &vfio_igd_quirk_mirror_bdsm, vdev,
> -                          "vfio-igd-bdsm-quirk", 8);
> -    memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> -                                        IGD_BDSM_MMIO_OFFSET, &quirk->mem[1],
> -                                        1);
> +    if (gen < 11) {
> +        memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
> +                              &vfio_igd_quirk_mirror_bdsm, vdev,
> +                              "vfio-igd-bdsm-quirk", 4);
> +        memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> +                                            IGD_BDSM_MMIO_OFFSET,
> +                                            &quirk->mem[1], 1);
> +    } else {
> +        memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
> +                              &vfio_igd_quirk_mirror_bdsm64, vdev,
> +                              "vfio-igd-bdsm-quirk", 8);
> +        memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> +                                            IGD_BDSM_MMIO_OFFSET,
> +                                            &quirk->mem[1], 1);
> +    }
>  
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
>  }

Reviewed-by: Corvin Köhne <c.koehne@beckhoff.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 9/9] vfio/igd: add x-igd-gms option back to set DSM region size for guest

[Tomita Moeko]

DSM region is likely to store framebuffer in Windows, a small DSM
region may cause display issues (e.g. half of the screen is black).
By default, QEMU uses host's original value, which is determined by
DVMT Pre-Allocated option in Intel FSP of host bios. Some vendors
do not expose this config item to users. In such cases, x-igd-gms
option can be used to manually set the data stolen memory size for
guest.

When DVMT Pre-Allocated option is available in host BIOS, user should
set DSM region size there instead of using x-igd-gms option.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index e464cd6949..0814730f40 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -717,6 +717,23 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
 
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
 
+    /*
+     * Allow user to override dsm size using x-igd-gms option, in multiples of
+     * 32MiB. This option should only be used when the desired size cannot be
+     * set from DVMT Pre-Allocated option in host BIOS.
+     */
+    if (vdev->igd_gms) {
+        if (gen < 8 && vdev->igd_gms <= 0x10) {
+            gmch &= ~(IGD_GMCH_GEN6_GMS_MASK << IGD_GMCH_GEN6_GMS_SHIFT);
+            gmch |= vdev->igd_gms << IGD_GMCH_GEN6_GMS_SHIFT;
+        } else if (vdev->igd_gms <= 0x40) {
+            gmch &= ~(IGD_GMCH_GEN8_GMS_MASK << IGD_GMCH_GEN8_GMS_SHIFT);
+            gmch |= vdev->igd_gms << IGD_GMCH_GEN8_GMS_SHIFT;
+        } else {
+            error_report("Unsupported IGD GMS value 0x%x", vdev->igd_gms);
+        }
+    }
+
     ggms_size = igd_gtt_memory_size(gen, gmch);
     gms_size = igd_stolen_memory_size(gen, gmch);
 
-- 
2.45.2

Re: [PATCH v2 9/9] vfio/igd: add x-igd-gms option back to set DSM region size for guest

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> DSM region is likely to store framebuffer in Windows, a small DSM
> region may cause display issues (e.g. half of the screen is black).
> By default, QEMU uses host's original value, which is determined by
> DVMT Pre-Allocated option in Intel FSP of host bios. Some vendors
> do not expose this config item to users. In such cases, x-igd-gms
> option can be used to manually set the data stolen memory size for
> guest.
> 
> When DVMT Pre-Allocated option is available in host BIOS, user should
> set DSM region size there instead of using x-igd-gms option.
> 

It might be worth linking the commit which has removed x-igd-gms and mention
that the behaviour changed slightly. Previously, the DSM region size was set to
0 if x-igd-gms was unset. This patch keeps the host value if x-igd-gms is unset.

> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index e464cd6949..0814730f40 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -717,6 +717,23 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int
> nr)
>  
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
>  
> +    /*
> +     * Allow user to override dsm size using x-igd-gms option, in multiples
> of
> +     * 32MiB. This option should only be used when the desired size cannot be
> +     * set from DVMT Pre-Allocated option in host BIOS.
> +     */
> +    if (vdev->igd_gms) {
> +        if (gen < 8 && vdev->igd_gms <= 0x10) {

This doesn't work as intended for values larger than 0x10. For those values,
qemu ignores the generation and set GMS as it would be a gen 8 or later device.

> +            gmch &= ~(IGD_GMCH_GEN6_GMS_MASK << IGD_GMCH_GEN6_GMS_SHIFT);
> +            gmch |= vdev->igd_gms << IGD_GMCH_GEN6_GMS_SHIFT;
> +        } else if (vdev->igd_gms <= 0x40) {
> +            gmch &= ~(IGD_GMCH_GEN8_GMS_MASK << IGD_GMCH_GEN8_GMS_SHIFT);
> +            gmch |= vdev->igd_gms << IGD_GMCH_GEN8_GMS_SHIFT;
> +        } else {
> +            error_report("Unsupported IGD GMS value 0x%x", vdev->igd_gms);
> +        }
> +    }
> +
>      ggms_size = igd_gtt_memory_size(gen, gmch);
>      gms_size = igd_stolen_memory_size(gen, gmch);
>  

-- 
Kind regards,
Corvin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 9/9] vfio/igd: add x-igd-gms option back to set DSM region size for guest

[Alex Williamson]

On Tue, 3 Dec 2024 16:30:56 +0000
Corvin Köhne <C.Koehne@beckhoff.com> wrote:

> On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> > CAUTION: External Email!!
> > DSM region is likely to store framebuffer in Windows, a small DSM
> > region may cause display issues (e.g. half of the screen is black).
> > By default, QEMU uses host's original value, which is determined by
> > DVMT Pre-Allocated option in Intel FSP of host bios. Some vendors
> > do not expose this config item to users. In such cases, x-igd-gms
> > option can be used to manually set the data stolen memory size for
> > guest.
> > 
> > When DVMT Pre-Allocated option is available in host BIOS, user should
> > set DSM region size there instead of using x-igd-gms option.
> >   
> 
> It might be worth linking the commit which has removed x-igd-gms and mention
> that the behaviour changed slightly. Previously, the DSM region size was set to
> 0 if x-igd-gms was unset. This patch keeps the host value if x-igd-gms is unset.

Yes, this is really quite confusing.  We never actually removed the
x-igd-gms option as $Subject implies, we only gutted it from doing
anything, *sigh*.  Let's please summarize how 971ca22f041b left the
option but removed any implementation behind that option and this
brings it back to provide some function.

> > Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> > ---
> >  hw/vfio/igd.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> > index e464cd6949..0814730f40 100644
> > --- a/hw/vfio/igd.c
> > +++ b/hw/vfio/igd.c
> > @@ -717,6 +717,23 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int
> > nr)
> >  
> >      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
> >  
> > +    /*
> > +     * Allow user to override dsm size using x-igd-gms option, in multiples
> > of
> > +     * 32MiB. This option should only be used when the desired size cannot be
> > +     * set from DVMT Pre-Allocated option in host BIOS.
> > +     */
> > +    if (vdev->igd_gms) {
> > +        if (gen < 8 && vdev->igd_gms <= 0x10) {  
> 
> This doesn't work as intended for values larger than 0x10. For those values,
> qemu ignores the generation and set GMS as it would be a gen 8 or later device.
> 
> > +            gmch &= ~(IGD_GMCH_GEN6_GMS_MASK << IGD_GMCH_GEN6_GMS_SHIFT);
> > +            gmch |= vdev->igd_gms << IGD_GMCH_GEN6_GMS_SHIFT;
> > +        } else if (vdev->igd_gms <= 0x40) {
> > +            gmch &= ~(IGD_GMCH_GEN8_GMS_MASK << IGD_GMCH_GEN8_GMS_SHIFT);
> > +            gmch |= vdev->igd_gms << IGD_GMCH_GEN8_GMS_SHIFT;
> > +        } else {
> > +            error_report("Unsupported IGD GMS value 0x%x", vdev->igd_gms);

We clearly know the upper limit of valid values for the device
generation, can we include that in the error report?  Thanks,

Alex

> > +        }
> > +    }
> > +
> >      ggms_size = igd_gtt_memory_size(gen, gmch);
> >      gms_size = igd_stolen_memory_size(gen, gmch);
> >    
> 

Re: [PATCH v2 0/9] vfio/igd: Enable legacy mode on more devices

[Alex Williamson]

On Tue,  3 Dec 2024 21:35:39 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> This patchset extends the support of legacy mode igd passthrough to
> all Intel Gen 11 and 12 devices (including Ice Lake, Jasper Lake,
> Rocket Lake, Alder Lake and Raptor Lake), and emulates GGC register
> in MMIO BAR0 for better compatibiltiy (It is tested Windows and GOP
> driver will read this MMIO register).
> 
> It also replaces magic numbers with macros to improve readability,
> and aligns behavior (BDSM registor mirroring and GGMS calculation for
> gen7) with i915 driver to avoid possible issues.
> 
> The x-igd-gms option removed in 971ca22f041b ("vfio/igd: don't set
> stolen memory size to zero") is also added back so that data stolen
> memory size can be specified for guest. It is tested that GMS may
> related to framebuffer size, a small GMS value may cause display issues
> like blackscreen. It can be changed by DVMT Pre-allocated option in
> host BIOS, but not all BIOS comes with this option. Having it in QEMU
> helps resolves such issues.
> 
> This patchset was verified on Intel i9-12900K CPU(UHD 770, 8086:4680)
> with custom OVMF firmware [1] and IntelGopDriver extracted from host
> bios. IGD device works well in both Windows and Linux guests, and
> scored 726 in 3DMark Time Spy Graphics on Windows guest.
> 
> [1] https://github.com/tomitamoeko/edk2/commits/igd-pt-adl/
> 
> Btw, IO BAR4 seems never be used by guest, and it the IO BAR itself
> is not working on Gen11+ devices in my experiments. There is no hints
> about that in old commit message and mailing list. It would be greatly
> appreciated if someone shares the background.

The quirks related to BAR4 access are generally for the vBIOS, we
wouldn't expect guest OS level drivers to use them.  IIRC this is
handling moving the stolen memory from the HPA to the GPA when the
vBIOS is writing the GTT.

Maybe that brings up an interesting topic.  Traditionally "legacy mode"
IGD assignment has been only for 440fx machines with SeaBIOS and last I
was aware edk2 wasn't willing to accept the same hack for the BDSM as
we had put into SeaBIOS, instead indicating that it should be
implemented in the device ROM.  Your branch in [1] above seems to
indicate edk2 does now have assigned IGD specific code.

Are these patches developing full stack support of these new devices,
from BIOS hand-off, through pre-boot environments, and through to guest
OS drivers, or are we only concerned that the guest OS level driver
lights up a display?

If you're using q35 and OVMF then you must be operating in the realm of
the mythical "Universal Pass-through" mode that I thought Intel had
abandoned.  It seems like we need an update to docs/igd-assign.txt as
it's likely very out of date based on recent improvements here and by
Corvin.

Also, are you proposing the noted edk2 change upstream?  It seems like
edk2 would need some sort of device version detection to know whether
to use a 32 or 64-bit BDSM value.  Thanks,

Alex

> Changelog:
> v2:
> * Droped "vfio/igd: fix GTT stolen memory size calculation for gen 7".
> * Fixed conditions when calculating GGMS size.
> * Added Gemini Lake and Comet Lake device ids.
> * Splited mirroring register declaration macro into a new patch.
> * Minor fixes.
> Link: https://lore.kernel.org/qemu-devel/20241201160938.44355-1-tomitamoeko@gmail.com/
> 
> Tomita Moeko (9):
>   vfio/igd: remove unsupported device ids
>   vfio/igd: align generation with i915 kernel driver
>   vfio/igd: canonicalize memory size calculations
>   vfio/igd: add Gemini Lake and Comet Lake device ids
>   vfio/igd: add Alder/Raptor/Rocket/Ice/Jasper Lake device ids
>   vfio/igd: add macro for declaring mirrored registers
>   vfio/igd: emulate GGC register in mmio bar0
>   vfio/igd: emulate BDSM in mmio bar0 for gen 6-10 devices
>   vfio/igd: add x-igd-gms option back to set DSM region size for guest
> 
>  hw/vfio/igd.c | 248 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 151 insertions(+), 97 deletions(-)
> 

Re: [PATCH v2 0/9] vfio/igd: Enable legacy mode on more devices

[Tomita Moeko]

On 12/4/24 04:12, Alex Williamson wrote:
> On Tue,  3 Dec 2024 21:35:39 +0800
> Tomita Moeko <tomitamoeko@gmail.com> wrote:
> 
>> This patchset extends the support of legacy mode igd passthrough to
>> all Intel Gen 11 and 12 devices (including Ice Lake, Jasper Lake,
>> Rocket Lake, Alder Lake and Raptor Lake), and emulates GGC register
>> in MMIO BAR0 for better compatibiltiy (It is tested Windows and GOP
>> driver will read this MMIO register).
>>
>> It also replaces magic numbers with macros to improve readability,
>> and aligns behavior (BDSM registor mirroring and GGMS calculation for
>> gen7) with i915 driver to avoid possible issues.
>>
>> The x-igd-gms option removed in 971ca22f041b ("vfio/igd: don't set
>> stolen memory size to zero") is also added back so that data stolen
>> memory size can be specified for guest. It is tested that GMS may
>> related to framebuffer size, a small GMS value may cause display issues
>> like blackscreen. It can be changed by DVMT Pre-allocated option in
>> host BIOS, but not all BIOS comes with this option. Having it in QEMU
>> helps resolves such issues.
>>
>> This patchset was verified on Intel i9-12900K CPU(UHD 770, 8086:4680)
>> with custom OVMF firmware [1] and IntelGopDriver extracted from host
>> bios. IGD device works well in both Windows and Linux guests, and
>> scored 726 in 3DMark Time Spy Graphics on Windows guest.
>>
>> [1] https://github.com/tomitamoeko/edk2/commits/igd-pt-adl/
>>
>> Btw, IO BAR4 seems never be used by guest, and it the IO BAR itself
>> is not working on Gen11+ devices in my experiments. There is no hints
>> about that in old commit message and mailing list. It would be greatly
>> appreciated if someone shares the background.
> 
> The quirks related to BAR4 access are generally for the vBIOS, we
> wouldn't expect guest OS level drivers to use them.  IIRC this is
> handling moving the stolen memory from the HPA to the GPA when the
> vBIOS is writing the GTT.

Got it. I'm wondering why vBIOS still writes HPA instead of GPA when
it's in virtual machine, maybe the address is hardcoded?

> Maybe that brings up an interesting topic.  Traditionally "legacy mode"
> IGD assignment has been only for 440fx machines with SeaBIOS and last I
> was aware edk2 wasn't willing to accept the same hack for the BDSM as
> we had put into SeaBIOS, instead indicating that it should be
> implemented in the device ROM.  Your branch in [1] above seems to
> indicate edk2 does now have assigned IGD specific code.
>
> Are these patches developing full stack support of these new devices,
> from BIOS hand-off, through pre-boot environments, and through to guest
> OS drivers, or are we only concerned that the guest OS level driver
> lights up a display?

Yes these patches provide a complete legacy mode passthrough solution,
from EFI DXE phase to guest OS, but the EFI part requires specific
changes in edk2.

> If you're using q35 and OVMF then you must be operating in the realm of
> the mythical "Universal Pass-through" mode that I thought Intel had
> abandoned.  It seems like we need an update to docs/igd-assign.txt as
> it's likely very out of date based on recent improvements here and by
> Corvin.

Actually the only machine supports legacy mode is i440fx, windows driver
checks the vendor and device id of LPC bridge device at 00:1f.0, if it
doesn't match, display driver won't work [1]. On q35 machine, there is
already a emulated ICH9 LPC at 00.1f.0. Previous there was a try in
modifying the id, but it breaks functionality [2].

[1] https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/lpc.c#L519
[2] https://lore.kernel.org/all/1457080913-30018-1-git-send-email-kraxel@redhat.com/

I had a try "Univerisal Pass-through" mode as igd-assign.txt with my
UHD 770. On linux guest, it works just as a normal gpu, except there
is no display output before i915 driver loaded. Even the device's vbdf
is not 00:02.0, and i915 log shows the DSM is 0M, it works perfectly.
intel_gpu_top shows igpu is working when I am playing a youtube video.

I also tried setting primary gpu in bios to discrete gpu, which makes
the pci class code of igpu changed from 0x030000 (VGA compatible 
controller) to 0x308000 (Display controller), it can still output to
monitor connected to it on Linux guest. All with a simple
    -device vfio-pci,host=00:02.0,id=hostdev0

But for windows guest, I never had any luck. I attached a virtio-gpu to
it, with intel grahics drivers installed in guest. If igd is not at
00:02.0 or gop driver is not provided, windows BSOD immediately on boot

> Also, are you proposing the noted edk2 change upstream?  It seems like
> edk2 would need some sort of device version detection to know whether
> to use a 32 or 64-bit BDSM value.  Thanks,
> 
> Alex

I'm afraid the answer is no, these edk2 changes are not fully open
source as they were taken from inten directly [3], except the last
patch. (It seems intel uses a modified qemu as "etc/igd-dsm-base"
used in patch 5 doesn't exist in qemu, probably they are creating
a identical GPA->HPA mapping for igd?).

One of these intel edk2 patches is also included in a edk2 bug [4] and
markd as hack.

Besides these edk changes, GOP driver is also needed. Intel never
released them to public. Even acrn hypervisor developed by intel says
"Fetch the VBT and GOP drivers from the board manufacturer". The only
way for users to get it seems to be using tools like "UEFI BIOS
Updater" to extract driver from host bios image, or try the luck by
downloading the gop driver shared on internet.

After having the GOP driver, a virtual rom for igd device in qemu
can be created by
    EfiRom -f 0x8086 -i <device_id> -e IgdAssignmentDxe.efi \
    PlatformGOPPolicy.efi IntelGopDriver.efi
IgdAssignementDxe.efi and PlatformGOPPolicy.efi are built with the
edk2 changes.

I'm not sure whether this can be added to official document.

[3] https://eci.intel.com/docs/3.0/components/kvm-hypervisor.html#build-ovmf-fd-for-kvm
[4] https://bugzilla.tianocore.org/show_bug.cgi?id=935

>> Changelog:
>> v2:
>> * Droped "vfio/igd: fix GTT stolen memory size calculation for gen 7".
>> * Fixed conditions when calculating GGMS size.
>> * Added Gemini Lake and Comet Lake device ids.
>> * Splited mirroring register declaration macro into a new patch.
>> * Minor fixes.
>> Link: https://lore.kernel.org/qemu-devel/20241201160938.44355-1-tomitamoeko@gmail.com/
>>
>> Tomita Moeko (9):
>>   vfio/igd: remove unsupported device ids
>>   vfio/igd: align generation with i915 kernel driver
>>   vfio/igd: canonicalize memory size calculations
>>   vfio/igd: add Gemini Lake and Comet Lake device ids
>>   vfio/igd: add Alder/Raptor/Rocket/Ice/Jasper Lake device ids
>>   vfio/igd: add macro for declaring mirrored registers
>>   vfio/igd: emulate GGC register in mmio bar0
>>   vfio/igd: emulate BDSM in mmio bar0 for gen 6-10 devices
>>   vfio/igd: add x-igd-gms option back to set DSM region size for guest
>>
>>  hw/vfio/igd.c | 248 ++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 151 insertions(+), 97 deletions(-)
>>
> 

Re: [PATCH v2 0/9] vfio/igd: Enable legacy mode on more devices

[Tomita Moeko]

On 12/4/24 23:08, Tomita Moeko wrote:
> On 12/4/24 04:12, Alex Williamson wrote:
>> On Tue,  3 Dec 2024 21:35:39 +0800
>> Tomita Moeko <tomitamoeko@gmail.com> wrote:
>>
>>> This patchset extends the support of legacy mode igd passthrough to
>>> all Intel Gen 11 and 12 devices (including Ice Lake, Jasper Lake,
>>> Rocket Lake, Alder Lake and Raptor Lake), and emulates GGC register
>>> in MMIO BAR0 for better compatibiltiy (It is tested Windows and GOP
>>> driver will read this MMIO register).
>>>
>>> It also replaces magic numbers with macros to improve readability,
>>> and aligns behavior (BDSM registor mirroring and GGMS calculation for
>>> gen7) with i915 driver to avoid possible issues.
>>>
>>> The x-igd-gms option removed in 971ca22f041b ("vfio/igd: don't set
>>> stolen memory size to zero") is also added back so that data stolen
>>> memory size can be specified for guest. It is tested that GMS may
>>> related to framebuffer size, a small GMS value may cause display issues
>>> like blackscreen. It can be changed by DVMT Pre-allocated option in
>>> host BIOS, but not all BIOS comes with this option. Having it in QEMU
>>> helps resolves such issues.
>>>
>>> This patchset was verified on Intel i9-12900K CPU(UHD 770, 8086:4680)
>>> with custom OVMF firmware [1] and IntelGopDriver extracted from host
>>> bios. IGD device works well in both Windows and Linux guests, and
>>> scored 726 in 3DMark Time Spy Graphics on Windows guest.
>>>
>>> [1] https://github.com/tomitamoeko/edk2/commits/igd-pt-adl/
>>>
>>> Btw, IO BAR4 seems never be used by guest, and it the IO BAR itself
>>> is not working on Gen11+ devices in my experiments. There is no hints
>>> about that in old commit message and mailing list. It would be greatly
>>> appreciated if someone shares the background.
>>
>> The quirks related to BAR4 access are generally for the vBIOS, we
>> wouldn't expect guest OS level drivers to use them.  IIRC this is
>> handling moving the stolen memory from the HPA to the GPA when the
>> vBIOS is writing the GTT.
> 
> Got it. I'm wondering why vBIOS still writes HPA instead of GPA when
> it's in virtual machine, maybe the address is hardcoded?
> 
>> Maybe that brings up an interesting topic.  Traditionally "legacy mode"
>> IGD assignment has been only for 440fx machines with SeaBIOS and last I
>> was aware edk2 wasn't willing to accept the same hack for the BDSM as
>> we had put into SeaBIOS, instead indicating that it should be
>> implemented in the device ROM.  Your branch in [1] above seems to
>> indicate edk2 does now have assigned IGD specific code.
>>
>> Are these patches developing full stack support of these new devices,
>> from BIOS hand-off, through pre-boot environments, and through to guest
>> OS drivers, or are we only concerned that the guest OS level driver
>> lights up a display?
> 
> Yes these patches provide a complete legacy mode passthrough solution,
> from EFI DXE phase to guest OS, but the EFI part requires specific
> changes in edk2.
> 
>> If you're using q35 and OVMF then you must be operating in the realm of
>> the mythical "Universal Pass-through" mode that I thought Intel had
>> abandoned.  It seems like we need an update to docs/igd-assign.txt as
>> it's likely very out of date based on recent improvements here and by
>> Corvin.
> 
> Actually the only machine supports legacy mode is i440fx, windows driver
> checks the vendor and device id of LPC bridge device at 00:1f.0, if it
> doesn't match, display driver won't work [1]. On q35 machine, there is
> already a emulated ICH9 LPC at 00.1f.0. Previous there was a try in
> modifying the id, but it breaks functionality [2].
> 
> [1] https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/lpc.c#L519
> [2] https://lore.kernel.org/all/1457080913-30018-1-git-send-email-kraxel@redhat.com/
> 
> I had a try "Univerisal Pass-through" mode as igd-assign.txt with my
> UHD 770. On linux guest, it works just as a normal gpu, except there
> is no display output before i915 driver loaded. Even the device's vbdf
> is not 00:02.0, and i915 log shows the DSM is 0M, it works perfectly.
> intel_gpu_top shows igpu is working when I am playing a youtube video.
> 
> I also tried setting primary gpu in bios to discrete gpu, which makes
> the pci class code of igpu changed from 0x030000 (VGA compatible 
> controller) to 0x308000 (Display controller), it can still output to
> monitor connected to it on Linux guest. All with a simple
>     -device vfio-pci,host=00:02.0,id=hostdev0
> 
> But for windows guest, I never had any luck. I attached a virtio-gpu to
> it, with intel grahics drivers installed in guest. If igd is not at
> 00:02.0 or gop driver is not provided, windows BSOD immediately on boot

After further investigation of the i915 kernel driver and additional
testing, I found out the root cause is the VBT inside opregion. VBT
seems to contain configs like how the igd output (Intel call it DVO) is
mapped to physical ports. If VBT is not present, i915 driver is able to
mock one (in drivers/gpu/drm/i915/display/intel_bios.c:init_vbt_missing_defaults).
However, windows driver can't, reporting error 43 in device manager or
BSOD. Adding x-igd-opregion=on makes windows guest happy with the UPT
mode. My previous success on linux was just luck, my platform doesn't
has much difference with intel default configuration. OpRegion should
be a must for igd passthrough.

The PCI class code difference is only about whether the device is
chosen as the primary output in BIOS and is mapped to host VGA memory
range or not. The class code change is also observed on intel discrete
GPUs like A750. Both mode can have OpRegion present. Currently kernel
side only probes OpRegion when igd is in VGA class code. I submitted
a patch [1] to enable the probe for display controller mode, both
linux and windows guests can have output to monitor in this mode.  

Probably legacy mode means VGA support at first?

[1] https://lore.kernel.org/linux-kernel/20241229155140.7434-1-tomitamoeko@gmail.com/

>> Also, are you proposing the noted edk2 change upstream?  It seems like
>> edk2 would need some sort of device version detection to know whether
>> to use a 32 or 64-bit BDSM value.  Thanks,
>>
>> Alex
> 
> I'm afraid the answer is no, these edk2 changes are not fully open
> source as they were taken from inten directly [3], except the last
> patch. (It seems intel uses a modified qemu as "etc/igd-dsm-base"
> used in patch 5 doesn't exist in qemu, probably they are creating
> a identical GPA->HPA mapping for igd?).
> 
> One of these intel edk2 patches is also included in a edk2 bug [4] and
> markd as hack.
> 
> Besides these edk changes, GOP driver is also needed. Intel never
> released them to public. Even acrn hypervisor developed by intel says
> "Fetch the VBT and GOP drivers from the board manufacturer". The only
> way for users to get it seems to be using tools like "UEFI BIOS
> Updater" to extract driver from host bios image, or try the luck by
> downloading the gop driver shared on internet.
> 
> After having the GOP driver, a virtual rom for igd device in qemu
> can be created by
>     EfiRom -f 0x8086 -i <device_id> -e IgdAssignmentDxe.efi \
>     PlatformGOPPolicy.efi IntelGopDriver.efi
> IgdAssignementDxe.efi and PlatformGOPPolicy.efi are built with the
> edk2 changes.
> 
> I'm not sure whether this can be added to official document.
> 
> [3] https://eci.intel.com/docs/3.0/components/kvm-hypervisor.html#build-ovmf-fd-for-kvm
> [4] https://bugzilla.tianocore.org/show_bug.cgi?id=935
> 
>>> Changelog:
>>> v2:
>>> * Droped "vfio/igd: fix GTT stolen memory size calculation for gen 7".
>>> * Fixed conditions when calculating GGMS size.
>>> * Added Gemini Lake and Comet Lake device ids.
>>> * Splited mirroring register declaration macro into a new patch.
>>> * Minor fixes.
>>> Link: https://lore.kernel.org/qemu-devel/20241201160938.44355-1-tomitamoeko@gmail.com/
>>>
>>> Tomita Moeko (9):
>>>   vfio/igd: remove unsupported device ids
>>>   vfio/igd: align generation with i915 kernel driver
>>>   vfio/igd: canonicalize memory size calculations
>>>   vfio/igd: add Gemini Lake and Comet Lake device ids
>>>   vfio/igd: add Alder/Raptor/Rocket/Ice/Jasper Lake device ids
>>>   vfio/igd: add macro for declaring mirrored registers
>>>   vfio/igd: emulate GGC register in mmio bar0
>>>   vfio/igd: emulate BDSM in mmio bar0 for gen 6-10 devices
>>>   vfio/igd: add x-igd-gms option back to set DSM region size for guest
>>>
>>>  hw/vfio/igd.c | 248 ++++++++++++++++++++++++++++++--------------------
>>>  1 file changed, 151 insertions(+), 97 deletions(-)
>>>
>>