1. Patchew
  2. QEMU
  3. [PATCH 0/8] vfio/igd: Enable legacy mode on more devices
  4. View patch

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

Tomita Moeko posted 8 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH 1/8] vfio/igd: fix GTT stolen memory size calculation for gen 7
Posted by Tomita Moeko 1 year, 2 months ago 
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) {
         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;
     }
 
-- 
2.45.2
Re: [PATCH 1/8] vfio/igd: fix GTT stolen memory size calculation for gen 7
Posted by Corvin Köhne 1 year, 2 months ago 
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?

>          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.


-- 
Kind regards,
Corvin
disable-disclaimer-BADE
Re: [PATCH 1/8] vfio/igd: fix GTT stolen memory size calculation for gen 7
Posted by Tomita Moeko 1 year, 2 months ago 
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.
> 
>
Re: [PATCH 1/8] vfio/igd: fix GTT stolen memory size calculation for gen 7
Posted by Alex Williamson 1 year, 2 months ago 
On Mon,  2 Dec 2024 00:09:31 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

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

I'd argue this should be rolled into patch 4.  It's not really fixing
anything because igd_gen() can't return anything between 6 and 8.  This
only allows for several device versions that we currently consider to
be gen 6 to align with i915 kernel driver generation by calling them
generation 7.  We'd previously lumped them into generation 6 because
there's no functional difference we care about here between 6 & 7.

In the next patch you replace this '1 << ggms_mb' with '*= 2', which
would be equivalent to 'ggms_mb << 1' and matches your description that
the increment is doubled.  Is there a separate bug fix that needs to be
pulled out here?

Also, please send a cover letter for any series longer than a single
patch and please configure your tools to thread the series.  Thanks,

Alex
Re: [PATCH 1/8] vfio/igd: fix GTT stolen memory size calculation for gen 7
Posted by Alex Williamson 1 year, 2 months ago 
On Sun, 1 Dec 2024 22:11:29 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon,  2 Dec 2024 00:09:31 +0800
> Tomita Moeko <tomitamoeko@gmail.com> wrote:
> 
> > 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) {
> >          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;
> >      }
> >    
> 
> I'd argue this should be rolled into patch 4.  It's not really fixing
> anything because igd_gen() can't return anything between 6 and 8.  This
> only allows for several device versions that we currently consider to
> be gen 6 to align with i915 kernel driver generation by calling them
> generation 7.  We'd previously lumped them into generation 6 because
> there's no functional difference we care about here between 6 & 7.
> 
> In the next patch you replace this '1 << ggms_mb' with '*= 2', which
> would be equivalent to 'ggms_mb << 1' and matches your description that
> the increment is doubled.  Is there a separate bug fix that needs to be
> pulled out here?
> 
> Also, please send a cover letter for any series longer than a single
> patch and please configure your tools to thread the series.  Thanks,

Disregard this latter comment, I just wasn't copied on the cover letter
and didn't have it in my inbox to root the thread.  Thanks,

Alex
Re: [PATCH 1/8] vfio/igd: fix GTT stolen memory size calculation for gen 7
Posted by Tomita Moeko 1 year, 2 months ago 
On 12/2/24 14:12, Alex Williamson wrote:
> On Sun, 1 Dec 2024 22:11:29 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
>> On Mon,  2 Dec 2024 00:09:31 +0800
>> Tomita Moeko <tomitamoeko@gmail.com> wrote:
>>
>>> 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) {
>>>          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;
>>>      }
>>>    
>>
>> I'd argue this should be rolled into patch 4.  It's not really fixing
>> anything because igd_gen() can't return anything between 6 and 8.  This
>> only allows for several device versions that we currently consider to
>> be gen 6 to align with i915 kernel driver generation by calling them
>> generation 7.  We'd previously lumped them into generation 6 because
>> there's no functional difference we care about here between 6 & 7.
>>
>> In the next patch you replace this '1 << ggms_mb' with '*= 2', which
>> would be equivalent to 'ggms_mb << 1' and matches your description that
>> the increment is doubled.  Is there a separate bug fix that needs to be
>> pulled out here?

Sorry this was a mistake when I composing my patches. At this time,
there was no gen 7, original condition won't cause any issue. It is
more suitable to be included in patch 4. I will drop this one in v2.

>> Also, please send a cover letter for any series longer than a single
>> patch and please configure your tools to thread the series.  Thanks,
> 
> Disregard this latter comment, I just wasn't copied on the cover letter
> and didn't have it in my inbox to root the thread.  Thanks,
> 
> Alex

CC of cover letter is not generated by get_mainainer.pl hook. Sorry for
not checking it before sending, I will manually set correct CC in v2.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.