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