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

[Tomita Moeko <tomitamoeko@gmail.com>]

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