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 | 101 ++++++++++++++++++++++++++++----------------------
1 file changed, 57 insertions(+), 44 deletions(-)
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index e40e601026..b449316ec0 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -116,6 +116,54 @@ 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
+#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
+#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) {
+ if (gms < 0x10) {
+ return gms * 32 * MiB;
+ }
+ } else {
+ if (gms < 0xf0) {
+ return gms * 32 * MiB;
+ } else {
+ return (gms - 0xf0 + 1) * 4 * MiB;
+ }
+ }
+
+ error_report("Unsupported IGD GMS value 0x%lx", gms);
+ return 0;
+}
/*
* The rather short list of registers that we copy from the host devices.
@@ -264,17 +312,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 > 7) {
- 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);
}
/*
@@ -481,30 +522,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;
@@ -514,7 +531,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;
@@ -676,13 +694,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 > 7) {
- 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
@@ -693,7 +706,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));
@@ -744,5 +757,5 @@ 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, *bdsm_size / MiB);
}
--
2.45.2On Mon, 2 Dec 2024 00:09:32 +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 | 101 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 57 insertions(+), 44 deletions(-)
>
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index e40e601026..b449316ec0 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -116,6 +116,54 @@ 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
> +#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
> +#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) {
> + if (gms < 0x10) {
> + return gms * 32 * MiB;
> + }
igd_get_stolen_mb() used:
if (gms > 0x10) {
error_report("Unsupported IGD GMS value 0x%x", gms);
return 0;
}
return gms * 32;
Therefore this should use <= to be equivalent.
> + } else {
> + if (gms < 0xf0) {
> + return gms * 32 * MiB;
> + } else {
> + return (gms - 0xf0 + 1) * 4 * MiB;
This does not match the line from igd_get_stolen_mb():
return (gms - 0xf0) * 4 + 4;
Explain.
> + }
> + }
> +
> + error_report("Unsupported IGD GMS value 0x%lx", gms);
> + return 0;
> +}
>
> /*
> * The rather short list of registers that we copy from the host devices.
> @@ -264,17 +312,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 > 7) {
> - 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);
> }
>
> /*
> @@ -481,30 +522,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;
> @@ -514,7 +531,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;
> @@ -676,13 +694,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 > 7) {
> - 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
> @@ -693,7 +706,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));
>
> @@ -744,5 +757,5 @@ 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, *bdsm_size / MiB);
Technically this should use le64_to_cpu(*bdsm_size), or retain the
addition logic. Thanks,
Alex
On 12/2/24 13:28, Alex Williamson wrote:
> On Mon, 2 Dec 2024 00:09:32 +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 | 101 ++++++++++++++++++++++++++++----------------------
>> 1 file changed, 57 insertions(+), 44 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index e40e601026..b449316ec0 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -116,6 +116,54 @@ 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
>> +#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
>> +#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) {
>> + if (gms < 0x10) {
>> + return gms * 32 * MiB;
>> + }
>
> igd_get_stolen_mb() used:
>
> if (gms > 0x10) {
> error_report("Unsupported IGD GMS value 0x%x", gms);
> return 0;
> }
> return gms * 32;
>
> Therefore this should use <= to be equivalent.
Got it, will fix in v2
>> + } else {
>> + if (gms < 0xf0) {
>> + return gms * 32 * MiB;
>> + } else {
>> + return (gms - 0xf0 + 1) * 4 * MiB;
>
> This does not match the line from igd_get_stolen_mb():
>
> return (gms - 0xf0) * 4 + 4;
>
> Explain.
It was changed to return size in bytes, instead of size in MB, to make
the behavior of 2 size calculation functions consistent. Original one
should be changed to.
return ((gms - 0xf0) * 4 + 4) * MiB;
Then I changed it to
return (gms - 0xf0 + 1) * 4 * MiB;
to make it easier to read. gms is uint64, the masked value is 0xff at
most, +1 won't cause overflow here.
>> + }
>> + }
>> +
>> + error_report("Unsupported IGD GMS value 0x%lx", gms);
>> + return 0;
>> +}
>>
>> /*
>> * The rather short list of registers that we copy from the host devices.
>> @@ -264,17 +312,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 > 7) {
>> - 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);
>> }
>>
>> /*
>> @@ -481,30 +522,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;
>> @@ -514,7 +531,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;
>> @@ -676,13 +694,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 > 7) {
>> - 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
>> @@ -693,7 +706,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));
>>
>> @@ -744,5 +757,5 @@ 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, *bdsm_size / MiB);
>
> Technically this should use le64_to_cpu(*bdsm_size), or retain the
> addition logic. Thanks,
Got it.
> Alex
>
On Sun, 2024-12-01 at 22:28 -0700, Alex Williamson wrote:
> CAUTION: External Email!!
> On Mon, 2 Dec 2024 00:09:32 +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 | 101 ++++++++++++++++++++++++++++-------------------
> > ---
> > 1 file changed, 57 insertions(+), 44 deletions(-)
> >
> > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> > index e40e601026..b449316ec0 100644
> > --- a/hw/vfio/igd.c
> > +++ b/hw/vfio/igd.c
> > @@ -116,6 +116,54 @@ 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
> > +#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
> > +#define IGD_GMCH_GEN8_GMS_MASK 0xff
> > +#define IGD_GMCH_GEN8_GGMS_SHIFT 6
> > +#define IGD_GMCH_GEN8_GGMS_MASK 0x3
> > +
You should consider reusing the names used by Linux [1].
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/intel/i915_drm.h?h=v6.12#n50
> > +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) {
> > + if (gms < 0x10) {
> > + return gms * 32 * MiB;
> > + }
>
> igd_get_stolen_mb() used:
>
> if (gms > 0x10) {
> error_report("Unsupported IGD GMS value 0x%x", gms);
> return 0;
> }
> return gms * 32;
>
> Therefore this should use <= to be equivalent.
I'd personally prefer moving the code in one commit and change the
format in a second one.
If you want to touch this, I'd align it with Linux, which multiplies gms
by 32 MB unconditionally for gen 6 and 8 [1].
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v6.12#n404
> > + } else {
> > + if (gms < 0xf0) {
> > + return gms * 32 * MiB;
> > + } else {
> > + return (gms - 0xf0 + 1) * 4 * MiB;
>
> This does not match the line from igd_get_stolen_mb():
>
> return (gms - 0xf0) * 4 + 4;
>
> Explain.
>
I've chosen the current format because it aligns with Linux [1]. I would
keep it as is to make it easier to sync with Linux.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v6.12#n460
> > + }
> > + }
> > +
> > + error_report("Unsupported IGD GMS value 0x%lx", gms);
> > + return 0;
> > +}
> >
> > /*
> > * The rather short list of registers that we copy from the host
> > devices.
> > @@ -264,17 +312,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 > 7) {
> > - 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);
> > }
> >
> > /*
> > @@ -481,30 +522,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;
> > @@ -514,7 +531,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;
> > @@ -676,13 +694,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 > 7) {
> > - 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
> > @@ -693,7 +706,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));
> >
> > @@ -744,5 +757,5 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice
> > *vdev, int nr)
> > vdev-
> > >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e
> > If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN-
> > EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY
> > Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 );
> > }
> >
> > - trace_vfio_pci_igd_bdsm_enabled(vdev-
> > >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e
> > If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN-
> > EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY
> > Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 , ggms_mb + gms_mb);
> > + trace_vfio_pci_igd_bdsm_enabled(vdev-
> > >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e
> > If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN-
> > EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY
> > Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 , *bdsm_size / MiB);
>
> Technically this should use le64_to_cpu(*bdsm_size), or retain the
> addition logic. Thanks,
>
> Alex
--
Kind regards,
Corvin
disable-disclaimer-BADE
On 12/2/24 17:10, Corvin Köhne wrote:
> On Sun, 2024-12-01 at 22:28 -0700, Alex Williamson wrote:
>> CAUTION: External Email!!
>> On Mon, 2 Dec 2024 00:09:32 +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 | 101 ++++++++++++++++++++++++++++-------------------
>>> ---
>>> 1 file changed, 57 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>>> index e40e601026..b449316ec0 100644
>>> --- a/hw/vfio/igd.c
>>> +++ b/hw/vfio/igd.c
>>> @@ -116,6 +116,54 @@ 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
>>> +#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
>>> +#define IGD_GMCH_GEN8_GMS_MASK 0xff
>>> +#define IGD_GMCH_GEN8_GGMS_SHIFT 6
>>> +#define IGD_GMCH_GEN8_GGMS_MASK 0x3
>>> +
>
> You should consider reusing the names used by Linux [1].
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/intel/i915_drm.h?h=v6.12#n50
Original SNB/BDW in i915 is not easy to understand I think, probably
adding a comment here is better?
>>> +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) {
>>> + if (gms < 0x10) {
>>> + return gms * 32 * MiB;
>>> + }
>>
>> igd_get_stolen_mb() used:
>>
>> if (gms > 0x10) {
>> error_report("Unsupported IGD GMS value 0x%x", gms);
>> return 0;
>> }
>> return gms * 32;
>>
>> Therefore this should use <= to be equivalent.
>
> I'd personally prefer moving the code in one commit and change the
> format in a second one.
>
> If you want to touch this, I'd align it with Linux, which multiplies gms
> by 32 MB unconditionally for gen 6 and 8 [1].
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v6.12#n404
>
>>> + } else {
>>> + if (gms < 0xf0) {
>>> + return gms * 32 * MiB;
>>> + } else {
>>> + return (gms - 0xf0 + 1) * 4 * MiB;
>>
>> This does not match the line from igd_get_stolen_mb():
>>
>> return (gms - 0xf0) * 4 + 4;
>>
>> Explain.
>>
>
> I've chosen the current format because it aligns with Linux [1]. I would
> keep it as is to make it easier to sync with Linux.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v6.12#n460
Will use
return ((gms - 0xf0) * 4 + 4) * MiB;
in v2
>>> + }
>>> + }
>>> +
>>> + error_report("Unsupported IGD GMS value 0x%lx", gms);
>>> + return 0;
>>> +}
>>>
>>> /*
>>> * The rather short list of registers that we copy from the host
>>> devices.
>>> @@ -264,17 +312,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 > 7) {
>>> - 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);
>>> }
>>>
>>> /*
>>> @@ -481,30 +522,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;
>>> @@ -514,7 +531,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;
>>> @@ -676,13 +694,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 > 7) {
>>> - 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
>>> @@ -693,7 +706,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));
>>>
>>> @@ -744,5 +757,5 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice
>>> *vdev, int nr)
>>> vdev-
>>>> https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e
>>> If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN-
>>> EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY
>>> Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 );
>>> }
>>>
>>> - trace_vfio_pci_igd_bdsm_enabled(vdev-
>>>> https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e
>>> If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN-
>>> EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY
>>> Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 , ggms_mb + gms_mb);
>>> + trace_vfio_pci_igd_bdsm_enabled(vdev-
>>>> https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e
>>> If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN-
>>> EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY
>>> Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 , *bdsm_size / MiB);
>>
>> Technically this should use le64_to_cpu(*bdsm_size), or retain the
>> addition logic. Thanks,
>>
>> Alex
>
© 2016 - 2026 Red Hat, Inc.