Re: [PATCH v2 2/9] vfio/igd: align generation with i915 kernel driver

[Tomita Moeko]

On 12/5/24 06:36, Alex Williamson wrote:
> On Tue,  3 Dec 2024 21:35:41 +0800
> Tomita Moeko <tomitamoeko@gmail.com> wrote:
> 
>> Define the igd device generations according to i915 kernel driver to
>> avoid confusion, and adjust comment placement to clearly reflect the
>> relationship between ids and devices.
>>
>> The condition of how GTT stolen memory size is calculated is changed
>> accordingly as GGMS is in multiple of 2 starting from gen 8.
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>>  hw/vfio/igd.c | 44 ++++++++++++++++++++++----------------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index 6ba3045bf3..2ede72d243 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -59,33 +59,33 @@
>>   */
>>  static int igd_gen(VFIOPCIDevice *vdev)
>>  {
>> -    if ((vdev->device_id & 0xfff) == 0xa84) {
>> -        return 8; /* Broxton */
>> +    /*
>> +     * Device IDs for Broxton/Apollo Lake are 0x0a84, 0x1a84, 0x1a85, 0x5a84
>> +     * and 0x5a85
>> +     */
> 
> Your comment from review of v1 would be useful here that we can't use
> the test below, at least for 0x0a84, because it conflicts with Haswell.
> I'd forgotten that.

Sure I will add it to comments in v3.

> Since we're being more strict about what we support now, it may make
> sense to list specific IDs rather than this sloppy match, as Corvin
> suggested, but that could be a follow-on.  Thanks,
> 
> Alex

I personally prefer do it later, it take some time to think a proper
way to maintain such a long device id list.

>> +    if ((vdev->device_id & 0xffe) == 0xa84) {
>> +        return 9;
>>      }
>>  
>>      switch (vdev->device_id & 0xff00) {
>> -    /* SandyBridge, IvyBridge, ValleyView, Haswell */
>> -    case 0x0100:
>> -    case 0x0400:
>> -    case 0x0a00:
>> -    case 0x0c00:
>> -    case 0x0d00:
>> -    case 0x0f00:
>> +    case 0x0100:    /* SandyBridge, IvyBridge */
>>          return 6;
>> -    /* BroadWell, CherryView, SkyLake, KabyLake */
>> -    case 0x1600:
>> -    case 0x1900:
>> -    case 0x2200:
>> -    case 0x5900:
>> +    case 0x0400:    /* Haswell */
>> +    case 0x0a00:    /* Haswell */
>> +    case 0x0c00:    /* Haswell */
>> +    case 0x0d00:    /* Haswell */
>> +    case 0x0f00:    /* Valleyview/Bay Trail */
>> +        return 7;
>> +    case 0x1600:    /* Broadwell */
>> +    case 0x2200:    /* Cherryview */
>>          return 8;
>> -    /* CoffeeLake */
>> -    case 0x3e00:
>> +    case 0x1900:    /* Skylake */
>> +    case 0x5900:    /* Kaby Lake */
>> +    case 0x3e00:    /* Coffee Lake */
>>          return 9;
>> -    /* ElkhartLake */
>> -    case 0x4500:
>> +    case 0x4500:    /* Elkhart Lake */
>>          return 11;
>> -    /* TigerLake */
>> -    case 0x9A00:
>> +    case 0x9A00:    /* Tiger Lake */
>>          return 12;
>>      }
>>  
>> @@ -258,7 +258,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 >= 8) {
>>          ggms = 1 << ggms;
>>      }
>>  
>> @@ -668,7 +668,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 >= 8) {
>>          ggms_mb = 1 << ggms_mb;
>>      }
>>  
>