diff mbox series

[2/8] vfio/igd: canonicalize memory size calculations

Message ID 20241201160938.44355-3-tomitamoeko@gmail.com (mailing list archive)
State New
Headers show
Series vfio/igd: Enable legacy mode on more devices | expand

Commit Message

Tomita Moeko Dec. 1, 2024, 4:09 p.m. UTC 
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(-)

Comments

Alex Williamson Dec. 2, 2024, 5:28 a.m. UTC | #1 
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.

> +    } 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
Corvin Köhne Dec. 2, 2024, 9:10 a.m. UTC | #2 
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
Tomita Moeko Dec. 2, 2024, 4:48 p.m. UTC | #3 
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
>
Tomita Moeko Dec. 2, 2024, 5:01 p.m. UTC | #4 
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
>
diff mbox series

Patch

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