Change in libgfxinit[master]: [TESTME]gma: Map dummy PTEs for buggy VT-d

[Nico Huber (Code Review)]

Hello Matt DeVillier, Michael Niewöhner,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/libgfxinit/+/40499

to review the following change.

Change subject: [TESTME]gma: Map dummy PTEs for buggy VT-d
......................................................................

[TESTME]gma: Map dummy PTEs for buggy VT-d

Extend Setup_Default_GTT() to add 128 dummy page table entries
after the framebuffer. Apparently the IOMMU may report spurious
type 6 errors if nothing is mapped in this range. Also check for
correct alignment of the start of the framebuffer and that the
additonal PTEs fit into the GTT.

Change-Id: I1b97c6b44c1ffb37d119541c1c97ffe21d244da8
Signed-off-by: Nico Huber 
---
M common/hw-gfx-gma.adb
1 file changed, 18 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/99/40499/1

diff --git a/common/hw-gfx-gma.adb b/common/hw-gfx-gma.adb
index be623e4..083b83c 100644
--- a/common/hw-gfx-gma.adb
+++ b/common/hw-gfx-gma.adb
@@ -603,9 +603,10 @@
    function Valid_FB (FB : Framebuffer_Type) return Boolean is
      (Valid_Stride (FB) and
       FB_First_Page (FB) in GTT_Range and
-      FB_Last_Page (FB) in GTT_Range and
+      FB_Last_Page (FB) + 128 in GTT_Range and
       (not Rotation_90 (FB) or
-       (FB_Last_Page (FB) + GTT_Rotation_Offset in GTT_Range and
+       (FB_First_Page (FB) mod 64 = 0 and
+        FB_Last_Page (FB) + 128 + GTT_Rotation_Offset in GTT_Range and
         FB.Offset >= Word32 (GTT_Rotation_Offset) * GTT_Page_Size)));

-- Also check that we don't overflow the GTT's 39-bit space
@@ -650,6 +651,10 @@
             Valid          => True);
          Phys_Addr := Phys_Addr + GTT_Page_Size;
       end loop;
+      -- Add another 128 dummy pages to workaround buggy VT-d
+      for Idx in FB_Last_Page (FB) + 1 .. FB_Last_Page (FB) + 128 loop
+         Registers.Write_GTT (Idx, Phys_Addr, True);
+      end loop;

if Rotation_90 (FB) and FB.Tiling = Y_Tiled and FB.V_Stride >= 32 then
          declare
@@ -673,6 +678,10 @@
                end if;
             end loop;
          end;
+         -- Add another 128 dummy pages to workaround buggy VT-d
+         for Idx in FB_Last_Page (FB) + 1 .. FB_Last_Page (FB) + 128 loop
+            Registers.Write_GTT (GTT_Rotation_Offset + Idx, Phys_Addr, True);
+         end loop;
       end if;
    end Setup_Default_GTT;

@@ -752,6 +761,9 @@
       Pre => Is_Initialized,
       Post => (if Valid then Valid_FB (FB))
    is
+      GTT_Off : constant Natural :=
+         (if Rotation_90 (FB) then GTT_Rotation_Offset else 0);
+
       GTT_Size, Aperture_Size : Natural;
       Stolen_Size : Stolen_Size_Range;
    begin
@@ -761,8 +773,10 @@
          Decode_Stolen (GTT_Size, Stolen_Size);
          Dev.Resource_Size (Aperture_Size, PCI.Res2);
          Valid :=
-            FB_Last_Page (FB) < GTT_Size / Config.GTT_PTE_Size and
-            FB_Last_Page (FB) < Natural (Stolen_Size / GTT_Page_Size) and
+            FB_Last_Page (FB) + 128 + GTT_Off < GTT_Size / Config.GTT_PTE_Size
+            and
+            FB_Last_Page (FB) < Natural (Stolen_Size / GTT_Page_Size)
+            and
             FB_Last_Page (FB) < Aperture_Size / GTT_Page_Size;
          pragma Debug (not Valid, Debug.Put_Line
            ("Stolen memory too small to hold framebuffer."));

[Nico Huber (Code Review)]

Hello Matt DeVillier, Michael Niewöhner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/libgfxinit/+/40499

to look at the new patch set (#2).

Change subject: [TESTME]gma: Map dummy PTEs for buggy VT-d
......................................................................

[TESTME]gma: Map dummy PTEs for buggy VT-d

Extend Setup_Default_GTT() to add 128 dummy page table entries
after the framebuffer. Apparently the IOMMU may report spurious
type 6 errors if nothing is mapped in this range. Also check for
correct alignment of the start of the framebuffer and that the
additonal PTEs fit into the GTT.

Change-Id: I1b97c6b44c1ffb37d119541c1c97ffe21d244da8
Signed-off-by: Nico Huber 
---
M common/hw-gfx-gma.adb
1 file changed, 18 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/99/40499/2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: [TESTME]gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 2: Code-Review+1

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: [TESTME]gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 2: Code-Review+1

no DHRD / PTE errors 😊

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: [TESTME]gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/libgfxinit/+/40499/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/libgfxinit/+/40499/2//COMMIT_MSG@10 
PS2, Line 10: Apparently the IOMMU may report spurious
            : type 6 errors if nothing is mapped in this range.
Is that done in the device or OS driver?

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: [TESTME]gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
>
> no DHRD / PTE errors 😊

Can you paste one of these error for reference?

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Nico Huber (Code Review)]

Hello Matt DeVillier, Angel Pons, Michael Niewöhner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/libgfxinit/+/40499

to look at the new patch set (#3).

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

gma: Map dummy PTEs for buggy VT-d

Extend Setup_Default_GTT() to add 128 dummy page table entries
after the framebuffer. Apparently the IOMMU may report spurious
type 6 errors if nothing is mapped in this range. Also check for
correct alignment of the start of the framebuffer and that the
additonal PTEs fit into the GTT.

An OS driver might report failed reads at random addresses. It
may or may not look like this:

DMAR: [DMA Read] Request device [00:02.0] PASID ffffffff fault addr \
    5669eb8000 [fault reason 06] PTE Read access is not set

Change-Id: I1b97c6b44c1ffb37d119541c1c97ffe21d244da8
Signed-off-by: Nico Huber 
---
M common/hw-gfx-gma.adb
1 file changed, 18 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/99/40499/3

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Nico Huber (Code Review)]

Hello Matt DeVillier, Angel Pons, Michael Niewöhner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/libgfxinit/+/40499

to look at the new patch set (#4).

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

gma: Map dummy PTEs for buggy VT-d

Extend Setup_Default_GTT() to add 128 dummy page table entries
after the framebuffer. Apparently the IOMMU may report spurious
type 6 errors if nothing is mapped in this range. Also check for
correct alignment of the start of the framebuffer and that the
additonal PTEs fit into the GTT.

Without such dummy entries, an OS driver might report failed
reads at random addresses. It may or may not look like this:

DMAR: [DMA Read] Request device [00:02.0] PASID ffffffff fault addr\
    5669eb8000 [fault reason 06] PTE Read access is not set

Change-Id: I1b97c6b44c1ffb37d119541c1c97ffe21d244da8
Signed-off-by: Nico Huber 
---
M common/hw-gfx-gma.adb
1 file changed, 18 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/99/40499/4

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 4: Code-Review+1

Fixes the erros for me, too, on Acer ES1-572

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 4: Code-Review+1

(1 comment)

https://review.coreboot.org/c/libgfxinit/+/40499/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/libgfxinit/+/40499/2//COMMIT_MSG@10 
PS2, Line 10: Apparently the IOMMU may report spurious
            : type 6 errors if nothing is mapped in this range.
> Is that done in the device or OS driver?

Done

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 4: Code-Review+2

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/libgfxinit/+/40499/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/libgfxinit/+/40499/2//COMMIT_MSG@10 
PS2, Line 10: Apparently the IOMMU may report spurious
            : type 6 errors if nothing is mapped in this range.
> Done

The IOMMU is hardware (a device). Not sure if that was the question.
The OS driver would read the report from the hardware.

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 4:

*ping* :-)

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

Patch Set 4: Verified+1

Change in libgfxinit[master]: gma: Map dummy PTEs for buggy VT-d

[Matt DeVillier (Code Review)]

Matt DeVillier has submitted this change. ( https://review.coreboot.org/c/libgfxinit/+/40499 )

Change subject: gma: Map dummy PTEs for buggy VT-d
......................................................................

gma: Map dummy PTEs for buggy VT-d

Extend Setup_Default_GTT() to add 128 dummy page table entries
after the framebuffer. Apparently the IOMMU may report spurious
type 6 errors if nothing is mapped in this range. Also check for
correct alignment of the start of the framebuffer and that the
additonal PTEs fit into the GTT.

Without such dummy entries, an OS driver might report failed
reads at random addresses. It may or may not look like this:

DMAR: [DMA Read] Request device [00:02.0] PASID ffffffff fault addr\
    5669eb8000 [fault reason 06] PTE Read access is not set

Change-Id: I1b97c6b44c1ffb37d119541c1c97ffe21d244da8
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/libgfxinit/+/40499
Reviewed-by: Michael Niewöhner
Reviewed-by: Paul Menzel 
Reviewed-by: Angel Pons 
Reviewed-by: Matt DeVillier 
---
M common/hw-gfx-gma.adb
1 file changed, 18 insertions(+), 4 deletions(-)

Approvals:
  Nico Huber: Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Matt DeVillier: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved
  Michael Niewöhner: Looks good to me, but someone else must approve

diff --git a/common/hw-gfx-gma.adb b/common/hw-gfx-gma.adb
index be623e4..9157bbe 100644
--- a/common/hw-gfx-gma.adb
+++ b/common/hw-gfx-gma.adb
@@ -603,9 +603,10 @@
    function Valid_FB (FB : Framebuffer_Type) return Boolean is
      (Valid_Stride (FB) and
       FB_First_Page (FB) in GTT_Range and
-      FB_Last_Page (FB) in GTT_Range and
+      FB_Last_Page (FB) + 128 in GTT_Range and
       (not Rotation_90 (FB) or
-       (FB_Last_Page (FB) + GTT_Rotation_Offset in GTT_Range and
+       (FB_First_Page (FB) mod 64 = 0 and
+        FB_Last_Page (FB) + 128 + GTT_Rotation_Offset in GTT_Range and
         FB.Offset >= Word32 (GTT_Rotation_Offset) * GTT_Page_Size)));

-- Also check that we don't overflow the GTT's 39-bit space
@@ -650,6 +651,10 @@
             Valid          => True);
          Phys_Addr := Phys_Addr + GTT_Page_Size;
       end loop;
+      -- Add another 128 dummy pages to work around buggy VT-d
+      for Idx in FB_Last_Page (FB) + 1 .. FB_Last_Page (FB) + 128 loop
+         Registers.Write_GTT (Idx, Phys_Addr, True);
+      end loop;

if Rotation_90 (FB) and FB.Tiling = Y_Tiled and FB.V_Stride >= 32 then
          declare
@@ -673,6 +678,10 @@
                end if;
             end loop;
          end;
+         -- Add another 128 dummy pages to work around buggy VT-d
+         for Idx in FB_Last_Page (FB) + 1 .. FB_Last_Page (FB) + 128 loop
+            Registers.Write_GTT (GTT_Rotation_Offset + Idx, Phys_Addr, True);
+         end loop;
       end if;
    end Setup_Default_GTT;

@@ -752,6 +761,9 @@
       Pre => Is_Initialized,
       Post => (if Valid then Valid_FB (FB))
    is
+      GTT_Off : constant Natural :=
+         (if Rotation_90 (FB) then GTT_Rotation_Offset else 0);
+
       GTT_Size, Aperture_Size : Natural;
       Stolen_Size : Stolen_Size_Range;
    begin
@@ -761,8 +773,10 @@
          Decode_Stolen (GTT_Size, Stolen_Size);
          Dev.Resource_Size (Aperture_Size, PCI.Res2);
          Valid :=
-            FB_Last_Page (FB) < GTT_Size / Config.GTT_PTE_Size and
-            FB_Last_Page (FB) < Natural (Stolen_Size / GTT_Page_Size) and
+            FB_Last_Page (FB) + 128 + GTT_Off < GTT_Size / Config.GTT_PTE_Size
+            and
+            FB_Last_Page (FB) < Natural (Stolen_Size / GTT_Page_Size)
+            and
             FB_Last_Page (FB) < Aperture_Size / GTT_Page_Size;
          pragma Debug (not Valid, Debug.Put_Line
            ("Stolen memory too small to hold framebuffer."));