Re: [edk2] [PATCH v2 5/5] OvmfPkg: simply use the Bochs interface for vmsvga

Tue, 06 Nov 2018 18:37:20 -0800 

On 2018-11-06 19:47, Laszlo Ersek wrote:

I suggest the following:

On 11/02/18 04:24, yuchenlin via edk2-devel wrote:

From: yuchenlin <[email protected]>

BAR  |    std vga     |  vmsvga
---------------------------------
0    |   Framebuffer  | I/O space
1    |   Reserved     | Framebuffer
2    |   MMIO         | FIFO

Because of the PCI BARs difference between std vga and
vmsvga, we can not simply recognize the "QEMU VMWare SVGA"
as the QEMU_VIDEO_BOCHS_MMIO variant.

Instead, we remain variant QEMU_VIDEO_VMWARE_SVGA, and use
it for:

(1) Get framebuffer from correct PCI BAR
(2) Prevent using BAR2 for MMIO


[a] The commit message should be udpated as follows:

- We cannot recognize VMW SVGA as BOCHS because that would confuse the
  IsQxl setting in QemuVideoControllerDriverStart(),
- We cannot recognize VMW SVGA as BOCHS_MMIO because BAR2 on VMW SVGA is 
  not the BOCHS MMIO BAR (we can only use port IO).

Therefore the list of reasons for which we should introduce
QEMU_VIDEO_VMWARE_SVGA should name three reasons: both of the currently
listed reasons, plus "prevent mis-recognizing VMW SVGA as QXL" as the
third one.



Will do.

Thanks,
yuchenlin



Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: yuchenlin <[email protected]>
---
 OvmfPkg/QemuVideoDxe/Driver.c | 18 ++++++++++++++++--
 OvmfPkg/QemuVideoDxe/Gop.c    |  3 ++-
 OvmfPkg/QemuVideoDxe/Qemu.h   |  2 ++
 3 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c 
index 2304afd1e6..76d4a2d27e 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -69,6 +69,12 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
         0x1050,
         QEMU_VIDEO_BOCHS_MMIO,
         L"QEMU VirtIO VGA"
+    },{
+        PCI_CLASS_DISPLAY_VGA,
+        0x15ad,
+        0x0405,
+        QEMU_VIDEO_VMWARE_SVGA,
+        L"QEMU VMWare SVGA"
     },{
         0 /* end of list */
     }
@@ -256,6 +262,12 @@ QemuVideoControllerDriverStart (
     goto ClosePciIo;
   }
   Private->Variant = Card->Variant;
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+    Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
+  } else {
+    Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
+  }
+

   //
// IsQxl is based on the detected Card->Variant, which at a later point might 
@@ -320,7 +332,8 @@ QemuVideoControllerDriverStart (
   // Check if accessing the bochs interface works.
   //
   if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
-      Private->Variant == QEMU_VIDEO_BOCHS) {
+      Private->Variant == QEMU_VIDEO_BOCHS ||
+      Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
     UINT16 BochsId;
     BochsId = BochsRead(Private, VBE_DISPI_INDEX_ID);
     if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) {
@@ -383,6 +396,7 @@ QemuVideoControllerDriverStart (
     break;
   case QEMU_VIDEO_BOCHS_MMIO:
   case QEMU_VIDEO_BOCHS:
+  case QEMU_VIDEO_VMWARE_SVGA:
     Status = QemuVideoBochsModeSetup (Private, IsQxl);
     break;
   default:
@@ -764,7 +778,7 @@ ClearScreen (
   Private->PciIo->Mem.Write (
                         Private->PciIo,
                         EfiPciIoWidthFillUint32,
-                        0,
+                        Private->FrameBufferVramBarIndex,
                         0,
                         0x400000 >> 2,
                         &Color
diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index d490fa7a2e..3abc5eeb36 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -60,7 +60,7 @@ QemuVideoCompleteModeData (

   Private->PciIo->GetBarAttributes (
                         Private->PciIo,
-                        0,
+                        Private->FrameBufferVramBarIndex,
                         NULL,
                         (VOID**) &FrameBufDesc
                         );
@@ -177,6 +177,7 @@ Routine Description:
     break;
   case QEMU_VIDEO_BOCHS_MMIO:
   case QEMU_VIDEO_BOCHS:
+  case QEMU_VIDEO_VMWARE_SVGA:
InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]); 
     break;
   default:
diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index d7da761705..3aac9eeca6 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -92,6 +92,7 @@ typedef enum {
   QEMU_VIDEO_CIRRUS_5446,
   QEMU_VIDEO_BOCHS,
   QEMU_VIDEO_BOCHS_MMIO,
+  QEMU_VIDEO_VMWARE_SVGA,
 } QEMU_VIDEO_VARIANT;

 typedef struct {
@@ -120,6 +121,7 @@ typedef struct {
   QEMU_VIDEO_VARIANT                    Variant;
   FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
   UINTN                                 FrameBufferBltConfigureSize;
+  UINT8                                 FrameBufferVramBarIndex;
 } QEMU_VIDEO_PRIVATE_DATA;

 ///



[b] How about the following -- incremental, to be squashed -- patch:
diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c 
index 76d4a2d27e7e..8e02700d3960 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -262,12 +262,6 @@ QemuVideoControllerDriverStart (
     goto ClosePciIo;
   }
   Private->Variant = Card->Variant;
-  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
-    Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
-  } else {
-    Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
-  }
-

   //
// IsQxl is based on the detected Card->Variant, which at a later point 
   might
@@ -328,12 +322,19 @@ QemuVideoControllerDriverStart (
     }
   }

+  //
+  // VMWare SVGA is handled like Bochs (with port IO only).
+  //
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+    Private->Variant = QEMU_VIDEO_BOCHS;
+    Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
+  }
+
   //
   // Check if accessing the bochs interface works.
   //
   if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
-      Private->Variant == QEMU_VIDEO_BOCHS ||
-      Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+      Private->Variant == QEMU_VIDEO_BOCHS) {
     UINT16 BochsId;
     BochsId = BochsRead(Private, VBE_DISPI_INDEX_ID);
     if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) {
@@ -396,7 +397,6 @@ QemuVideoControllerDriverStart (
     break;
   case QEMU_VIDEO_BOCHS_MMIO:
   case QEMU_VIDEO_BOCHS:
-  case QEMU_VIDEO_VMWARE_SVGA:
     Status = QemuVideoBochsModeSetup (Private, IsQxl);
     break;
   default:
diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 3abc5eeb36a6..6f542d9eac05 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -177,7 +177,6 @@ Routine Description:
     break;
   case QEMU_VIDEO_BOCHS_MMIO:
   case QEMU_VIDEO_BOCHS:
-  case QEMU_VIDEO_VMWARE_SVGA:
     InitializeBochsGraphicsMode (Private,
     &QemuVideoBochsModes[ModeData->InternalModeIndex]);
     break;
   default:


This would produce the following -- squashed -- patch, for v3:


diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index d7da761705a1..3aac9eeca687 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -92,6 +92,7 @@ typedef enum {
   QEMU_VIDEO_CIRRUS_5446,
   QEMU_VIDEO_BOCHS,
   QEMU_VIDEO_BOCHS_MMIO,
+  QEMU_VIDEO_VMWARE_SVGA,
 } QEMU_VIDEO_VARIANT;

 typedef struct {
@@ -120,6 +121,7 @@ typedef struct {
   QEMU_VIDEO_VARIANT                    Variant;
   FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
   UINTN                                 FrameBufferBltConfigureSize;
+  UINT8                                 FrameBufferVramBarIndex;
 } QEMU_VIDEO_PRIVATE_DATA;

 ///
diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c 
index 2304afd1e686..8e02700d3960 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -69,6 +69,12 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
         0x1050,
         QEMU_VIDEO_BOCHS_MMIO,
         L"QEMU VirtIO VGA"
+    },{
+        PCI_CLASS_DISPLAY_VGA,
+        0x15ad,
+        0x0405,
+        QEMU_VIDEO_VMWARE_SVGA,
+        L"QEMU VMWare SVGA"
     },{
         0 /* end of list */
     }
@@ -316,6 +322,14 @@ QemuVideoControllerDriverStart (
     }
   }

+  //
+  // VMWare SVGA is handled like Bochs (with port IO only).
+  //
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+    Private->Variant = QEMU_VIDEO_BOCHS;
+    Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
+  }
+
   //
   // Check if accessing the bochs interface works.
   //
@@ -764,7 +778,7 @@ ClearScreen (
   Private->PciIo->Mem.Write (
                         Private->PciIo,
                         EfiPciIoWidthFillUint32,
-                        0,
+                        Private->FrameBufferVramBarIndex,
                         0,
                         0x400000 >> 2,
                         &Color
diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index d490fa7a2e6f..6f542d9eac05 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -60,7 +60,7 @@ QemuVideoCompleteModeData (

   Private->PciIo->GetBarAttributes (
                         Private->PciIo,
-                        0,
+                        Private->FrameBufferVramBarIndex,
                         NULL,
                         (VOID**) &FrameBufDesc
                         );


For me this is much easier to understand.


I agree. Will do

Thanks,
yuchenlin



... While we discuss this, I'll go ahead and push the first four
patches. The code being reverted is dead anyway. I'll report back about
the commit hashes.

Thanks,
Laszlo


_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to