Re: [PATCH 1/8] vfio/igd: fix GTT stolen memory size calculation for gen 7

[Tomita Moeko]

On 12/2/24 16:54, Corvin Köhne wrote:
> On Mon, 2024-12-02 at 00:09 +0800, Tomita Moeko wrote:
>> CAUTION: External Email!!
>> Both intel documentation [1][2] and i915 driver shows GGMS represents
>> GTT stolen memory size in multiple of 1MB, not 2MB starting from gen
>> 8.
>>
>> [1]
>> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/3rd-gen-core-desktop-vol-2-datasheet.pdf
>> [2]
>> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>>  hw/vfio/igd.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index 4047f4f071..e40e601026 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -268,7 +268,7 @@ static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
>>  
>>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
>>      ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
>> -    if (gen > 6) {
>> +    if (gen > 7) {
> 
> This seems odd. The commit message talks about gen 8 but it changes the
> behaviour for gen 7 only. Additionally, ggms_mb is still shifted for gen
> 8 and later, so it's still increment by steps of 2 MB. Shouldn't this be
> something like gen < 8?

Yes it should be. I think putting the change here is not correct,
as alex said. I am going to remove this change in v2.

>>          ggms = 1 << ggms;
>>      }
>>  
>> @@ -678,7 +678,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice
>> *vdev, int nr)
>>  
>>      /* Determine the size of stolen memory needed for GTT */
>>      ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
>> -    if (gen > 6) {
>> +    if (gen > 7) {
>>          ggms_mb = 1 << ggms_mb;
>>      }
>>  
> 
> Btw. you could consider adding a link to the source code of i915 too.
> 
>