On 12/2/24 13:28, Alex Williamson wrote:
> On Mon, 2 Dec 2024 00:09:32 +0800
> Tomita Moeko <tomitamo...@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 <tomitamo...@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
>
