From: Phil Dennis-Jordan <phil@philjordan.eu>

In addition to the QXL, Cirrus, etc. VGA adapters, Qemu also implements
a basic version of VMWare's SVGA display device. Drivers for this
device exist for some guest OSes which do not support Qemu's other
display adapters, so supporting it in OVMF is useful in conjunction
with those OSes.

This change adds support for the SVGA device's framebuffer to
QemuVideoDxe's graphics output protocol implementation, based on
VMWare's documentation. The most basic initialisation, framebuffer
layout query, and mode setting operations are implemented.

The device relies on port-based 32-bit I/O, unfortunately on misaligned
addresses. This limits the driver's support to the x86 family of
platforms.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---

Notes:
    v2:
    - Unaligned I/O helper functions moved to separate commit [Laszlo]
    - Multi-line function call whitespace fixes.
    
    v3:
    - Dropped the "2" from "SVGA2" where appropriate. [Jordan, Laszlo]
    - Renamed various struct fields and functions with consistent prefixes [Laszlo]
    - #include orders fixed [Laszlo]
    - Renamedi/moved lots of local variables to comply with convention. [Laszlo]
    - Added error checking to PCI BAR queries. [Laszlo]
    - Moved some function definitions around for better grouping. [Laszlo]
    - Fixed ClearScreen() to use the correct VRAM BAR. [Laszlo]
    - Changed modelist initialisation to fetch all mode data on startup, so mode
      queries can return everything including channel masks without hitting the
      device. Mask calculations hopefully make more sense now. [Laszlo]
    - Whitespace fixes. [Laszlo]
    - Fixed a memory leak in BAR query.
    
    v4:
    - Simplified mode info pixel mask calculation & PCI BAR OOB check. [Laszlo]
    - Replaced struct assignment with CopyMem. [Laszlo]
    - Whitespace & comment typo fixes. [Laszlo]

 OvmfPkg/QemuVideoDxe/Qemu.h       |  29 ++++
 OvmfPkg/QemuVideoDxe/Driver.c     | 128 +++++++++++++++-
 OvmfPkg/QemuVideoDxe/Gop.c        |  61 +++++++-
 OvmfPkg/QemuVideoDxe/Initialize.c | 161 ++++++++++++++++++++
 4 files changed, 372 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 2ce37defc5b8..7fbb25b3efd3 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 {
@@ -115,10 +116,13 @@ typedef struct {
   //
   UINTN                                 MaxMode;
   QEMU_VIDEO_MODE_DATA                  *ModeData;
+  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *VmwareSvgaModeInfo;
 
   QEMU_VIDEO_VARIANT                    Variant;
   FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
   UINTN                                 FrameBufferBltConfigureSize;
+  UINT8                                 FrameBufferVramBarIndex;
+  UINT16                                VmwareSvgaBasePort;
 } QEMU_VIDEO_PRIVATE_DATA;
 
 ///
@@ -502,9 +506,34 @@ QemuVideoBochsModeSetup (
   BOOLEAN                  IsQxl
   );
 
+EFI_STATUS
+QemuVideoVmwareSvgaModeSetup (
+  QEMU_VIDEO_PRIVATE_DATA *Private
+  );
+
 VOID
 InstallVbeShim (
   IN CONST CHAR16         *CardName,
   IN EFI_PHYSICAL_ADDRESS FrameBufferBase
   );
+
+VOID
+VmwareSvgaWrite (
+  QEMU_VIDEO_PRIVATE_DATA *Private,
+  UINT16                  Register,
+  UINT32                  Value
+  );
+
+UINT32
+VmwareSvgaRead (
+  QEMU_VIDEO_PRIVATE_DATA *Private,
+  UINT16                  Register
+  );
+
+VOID
+InitializeVmwareSvgaGraphicsMode (
+  QEMU_VIDEO_PRIVATE_DATA  *Private,
+  QEMU_VIDEO_BOCHS_MODES   *ModeData
+  );
+
 #endif
diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index fc8025ec46de..2ed1506a1381 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -14,8 +14,10 @@
 
 **/
 
-#include "Qemu.h"
+#include <IndustryStandard/VmwareSvga.h>
 #include <IndustryStandard/Acpi.h>
+#include "Qemu.h"
+#include "UnalignedIoInternal.h"
 
 EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = {
   QemuVideoControllerDriverSupported,
@@ -58,6 +60,11 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
         QEMU_VIDEO_BOCHS_MMIO,
         L"QEMU VirtIO VGA"
     },{
+        VMWARE_PCI_VENDOR_ID_VMWARE,
+        VMWARE_PCI_DEVICE_ID_VMWARE_SVGA2,
+        QEMU_VIDEO_VMWARE_SVGA,
+        L"QEMU VMWare SVGA"
+    },{
         0 /* end of list */
     }
 };
@@ -242,6 +249,7 @@ QemuVideoControllerDriverStart (
     goto ClosePciIo;
   }
   Private->Variant = Card->Variant;
+  Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
 
   //
   // IsQxl is based on the detected Card->Variant, which at a later point might
@@ -317,6 +325,58 @@ QemuVideoControllerDriverStart (
   }
 
   //
+  // Check if accessing Vmware SVGA interface works
+  //
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *IoDesc;
+    UINT32                            TargetId;
+    UINT32                            SvgaIdRead;
+
+    IoDesc = NULL;
+    Status = Private->PciIo->GetBarAttributes (
+                               Private->PciIo,
+                               PCI_BAR_IDX0,
+                               NULL,
+                               (VOID**) &IoDesc
+                               );
+    if (EFI_ERROR (Status) ||
+        IoDesc->ResType != ACPI_ADDRESS_SPACE_TYPE_IO ||
+        IoDesc->AddrRangeMin > MAX_UINT16 + 1 - (VMWARE_SVGA_VALUE_PORT + 4)) {
+      if (IoDesc != NULL) {
+        FreePool (IoDesc);
+      }
+      Status = EFI_DEVICE_ERROR;
+      goto RestoreAttributes;
+    }
+    Private->VmwareSvgaBasePort = (UINT16) IoDesc->AddrRangeMin;
+    FreePool (IoDesc);
+
+    TargetId = VMWARE_SVGA_ID_2;
+    while (TRUE) {
+      VmwareSvgaWrite (Private, VmwareSvgaRegId, TargetId);
+      SvgaIdRead = VmwareSvgaRead (Private, VmwareSvgaRegId);
+      if ((SvgaIdRead == TargetId) || (TargetId <= VMWARE_SVGA_ID_0)) {
+        break;
+      }
+      TargetId--;
+    }
+
+    if (SvgaIdRead != TargetId) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "QemuVideo: QEMU_VIDEO_VMWARE_SVGA ID mismatch "
+        "(got 0x%x, base address 0x%x)\n",
+        SvgaIdRead,
+        Private->VmwareSvgaBasePort
+        ));
+      Status = EFI_DEVICE_ERROR;
+      goto RestoreAttributes;
+    }
+
+    Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
+  }
+
+  //
   // Get ParentDevicePath
   //
   Status = gBS->HandleProtocol (
@@ -371,6 +431,9 @@ QemuVideoControllerDriverStart (
   case QEMU_VIDEO_BOCHS:
     Status = QemuVideoBochsModeSetup (Private, IsQxl);
     break;
+  case QEMU_VIDEO_VMWARE_SVGA:
+    Status = QemuVideoVmwareSvgaModeSetup (Private);
+    break;
   default:
     ASSERT (FALSE);
     Status = EFI_DEVICE_ERROR;
@@ -750,7 +813,7 @@ ClearScreen (
   Private->PciIo->Mem.Write (
                         Private->PciIo,
                         EfiPciIoWidthFillUint32,
-                        0,
+                        Private->FrameBufferVramBarIndex,
                         0,
                         0x400000 >> 2,
                         &Color
@@ -888,6 +951,38 @@ BochsRead (
 }
 
 VOID
+VmwareSvgaWrite (
+  QEMU_VIDEO_PRIVATE_DATA   *Private,
+  UINT16                    Register,
+  UINT32                    Value
+  )
+{
+  UnalignedIoWrite32 (
+    Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
+    Register
+    );
+  UnalignedIoWrite32 (
+    Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT,
+    Value
+    );
+}
+
+UINT32
+VmwareSvgaRead (
+  QEMU_VIDEO_PRIVATE_DATA   *Private,
+  UINT16                    Register
+  )
+{
+  UnalignedIoWrite32 (
+    Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
+    Register
+    );
+  return UnalignedIoRead32 (
+           Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT
+           );
+}
+
+VOID
 VgaOutb (
   QEMU_VIDEO_PRIVATE_DATA  *Private,
   UINTN                    Reg,
@@ -941,6 +1036,35 @@ InitializeBochsGraphicsMode (
   ClearScreen (Private);
 }
 
+VOID
+InitializeVmwareSvgaGraphicsMode (
+  QEMU_VIDEO_PRIVATE_DATA  *Private,
+  QEMU_VIDEO_BOCHS_MODES   *ModeData
+  )
+{
+  UINT32 Capabilities;
+
+  VmwareSvgaWrite (Private, VmwareSvgaRegWidth, ModeData->Width);
+  VmwareSvgaWrite (Private, VmwareSvgaRegHeight, ModeData->Height);
+
+  Capabilities = VmwareSvgaRead (
+                   Private,
+                   VmwareSvgaRegCapabilities
+                   );
+  if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
+    VmwareSvgaWrite (
+      Private,
+      VmwareSvgaRegBitsPerPixel,
+      ModeData->ColorDepth
+      );
+  }
+
+  VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 1);
+
+  SetDefaultPalette (Private);
+  ClearScreen (Private);
+}
+
 EFI_STATUS
 EFIAPI
 InitializeQemuVideo (
diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 359e9217d3d1..512fd27acbda 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -13,6 +13,7 @@
 
 **/
 
+#include <IndustryStandard/VmwareSvga.h>
 #include "Qemu.h"
 
 STATIC
@@ -75,6 +76,42 @@ QemuVideoCompleteModeData (
   return EFI_SUCCESS;
 }
 
+STATIC
+EFI_STATUS
+QemuVideoVmwareSvgaCompleteModeData (
+  IN  QEMU_VIDEO_PRIVATE_DATA           *Private,
+  OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
+  )
+{
+  EFI_STATUS                            Status;
+  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *Info;
+  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR     *FrameBufDesc;
+  UINT32                                BytesPerLine, FbOffset, BytesPerPixel;
+
+  Info = Mode->Info;
+  CopyMem (Info, &Private->VmwareSvgaModeInfo[Mode->Mode], sizeof (*Info));
+  BytesPerPixel = Private->ModeData[Mode->Mode].ColorDepth / 8;
+  BytesPerLine = Info->PixelsPerScanLine * BytesPerPixel;
+
+  FbOffset = VmwareSvgaRead (Private, VmwareSvgaRegFbOffset);
+
+  Status = Private->PciIo->GetBarAttributes (
+                             Private->PciIo,
+                             PCI_BAR_IDX1,
+                             NULL,
+                             (VOID**) &FrameBufDesc
+                             );
+  if (EFI_ERROR (Status)) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  Mode->FrameBufferBase = FrameBufDesc->AddrRangeMin + FbOffset;
+  Mode->FrameBufferSize = BytesPerLine * Info->VerticalResolution;
+
+  FreePool (FrameBufDesc);
+  return Status;
+}
+
 
 //
 // Graphics Output Protocol Member Functions
@@ -124,10 +161,14 @@ Routine Description:
 
   *SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
 
-  ModeData = &Private->ModeData[ModeNumber];
-  (*Info)->HorizontalResolution = ModeData->HorizontalResolution;
-  (*Info)->VerticalResolution   = ModeData->VerticalResolution;
-  QemuVideoCompleteModeInfo (ModeData, *Info);
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+    CopyMem (*Info, &Private->VmwareSvgaModeInfo[ModeNumber], sizeof (**Info));
+  } else {
+    ModeData = &Private->ModeData[ModeNumber];
+    (*Info)->HorizontalResolution = ModeData->HorizontalResolution;
+    (*Info)->VerticalResolution   = ModeData->VerticalResolution;
+    QemuVideoCompleteModeInfo (ModeData, *Info);
+  }
 
   return EFI_SUCCESS;
 }
@@ -176,6 +217,12 @@ Routine Description:
   case QEMU_VIDEO_BOCHS:
     InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]);
     break;
+  case QEMU_VIDEO_VMWARE_SVGA:
+    InitializeVmwareSvgaGraphicsMode (
+      Private,
+      &QemuVideoBochsModes[ModeData->InternalModeIndex]
+      );
+    break;
   default:
     ASSERT (FALSE);
     return EFI_DEVICE_ERROR;
@@ -186,7 +233,11 @@ Routine Description:
   This->Mode->Info->VerticalResolution = ModeData->VerticalResolution;
   This->Mode->SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
 
-  QemuVideoCompleteModeData (Private, This->Mode);
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+    QemuVideoVmwareSvgaCompleteModeData (Private, This->Mode);
+  } else {
+    QemuVideoCompleteModeData (Private, This->Mode);
+  }
 
   //
   // Re-initialize the frame buffer configure when mode changes.
diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
index d5d8cfef9661..849186a6f05d 100644
--- a/OvmfPkg/QemuVideoDxe/Initialize.c
+++ b/OvmfPkg/QemuVideoDxe/Initialize.c
@@ -13,6 +13,7 @@
 
 **/
 
+#include <IndustryStandard/VmwareSvga.h>
 #include "Qemu.h"
 
 
@@ -346,3 +347,163 @@ QemuVideoBochsModeSetup (
   return EFI_SUCCESS;
 }
 
+EFI_STATUS
+QemuVideoVmwareSvgaModeSetup (
+  QEMU_VIDEO_PRIVATE_DATA *Private
+  )
+{
+  EFI_STATUS                            Status;
+  UINT32                                FbSize;
+  UINT32                                MaxWidth, MaxHeight;
+  UINT32                                Capabilities;
+  UINT32                                BitsPerPixel;
+  UINT32                                Index;
+  QEMU_VIDEO_MODE_DATA                  *ModeData;
+  QEMU_VIDEO_BOCHS_MODES                *VideoMode;
+  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *ModeInfo;
+
+  VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 0);
+
+  Private->ModeData =
+    AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT);
+  if (Private->ModeData == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto ModeDataAllocError;
+  }
+
+  Private->VmwareSvgaModeInfo =
+    AllocatePool (
+      sizeof (Private->VmwareSvgaModeInfo[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT
+      );
+  if (Private->VmwareSvgaModeInfo == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto ModeInfoAllocError;
+  }
+
+  FbSize =       VmwareSvgaRead (Private, VmwareSvgaRegFbSize);
+  MaxWidth =     VmwareSvgaRead (Private, VmwareSvgaRegMaxWidth);
+  MaxHeight =    VmwareSvgaRead (Private, VmwareSvgaRegMaxHeight);
+  Capabilities = VmwareSvgaRead (Private, VmwareSvgaRegCapabilities);
+  if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
+    BitsPerPixel = VmwareSvgaRead (
+                     Private,
+                     VmwareSvgaRegHostBitsPerPixel
+                     );
+    VmwareSvgaWrite (
+      Private,
+      VmwareSvgaRegBitsPerPixel,
+      BitsPerPixel
+      );
+  } else {
+    BitsPerPixel = VmwareSvgaRead (
+                     Private,
+                     VmwareSvgaRegBitsPerPixel
+                     );
+  }
+
+  if (FbSize == 0       ||
+      MaxWidth == 0     ||
+      MaxHeight == 0    ||
+      BitsPerPixel == 0 ||
+      BitsPerPixel % 8 != 0) {
+    Status = EFI_DEVICE_ERROR;
+    goto Rollback;
+  }
+
+  ModeData = Private->ModeData;
+  ModeInfo = Private->VmwareSvgaModeInfo;
+  VideoMode = &QemuVideoBochsModes[0];
+  for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index++) {
+    UINTN RequiredFbSize;
+
+    RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height *
+                     (BitsPerPixel / 8);
+    if (RequiredFbSize <= FbSize     &&
+        VideoMode->Width <= MaxWidth &&
+        VideoMode->Height <= MaxHeight) {
+      UINT32  BytesPerLine;
+      UINT32  RedMask, GreenMask, BlueMask, PixelMask;
+
+      VmwareSvgaWrite (
+        Private,
+        VmwareSvgaRegWidth,
+        VideoMode->Width
+        );
+      VmwareSvgaWrite (
+        Private,
+        VmwareSvgaRegHeight,
+        VideoMode->Height
+        );
+      BytesPerLine = VmwareSvgaRead (
+                       Private,
+                       VmwareSvgaRegBytesPerLine
+                       );
+
+      ModeData->InternalModeIndex    = Index;
+      ModeData->HorizontalResolution = VideoMode->Width;
+      ModeData->VerticalResolution   = VideoMode->Height;
+      ModeData->ColorDepth           = BitsPerPixel;
+
+      //
+      // Setting VmwareSvgaRegWidth/VmwareSvgaRegHeight actually changes
+      // the device's display mode, so we save all properties of each mode up
+      // front to avoid inadvertent mode changes later.
+      //
+      ModeInfo->Version = 0;
+      ModeInfo->HorizontalResolution = ModeData->HorizontalResolution;
+      ModeInfo->VerticalResolution   = ModeData->VerticalResolution;
+
+      ModeInfo->PixelFormat = PixelBitMask;
+
+      RedMask   = VmwareSvgaRead (Private, VmwareSvgaRegRedMask);
+      ModeInfo->PixelInformation.RedMask = RedMask;
+
+      GreenMask = VmwareSvgaRead (Private, VmwareSvgaRegGreenMask);
+      ModeInfo->PixelInformation.GreenMask = GreenMask;
+
+      BlueMask  = VmwareSvgaRead (Private, VmwareSvgaRegBlueMask);
+      ModeInfo->PixelInformation.BlueMask = BlueMask;
+
+      //
+      // Reserved mask is whatever bits in the pixel not containing RGB data,
+      // so start with binary 1s for every bit in the pixel, then mask off
+      // bits already used for RGB. Special case 32 to avoid undefined
+      // behaviour in the shift.
+      //
+      if (BitsPerPixel == 32) {
+        if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
+          ModeInfo->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
+        } else if (BlueMask == 0xff0000 &&
+                   GreenMask == 0xff00 &&
+                   RedMask == 0xff) {
+          ModeInfo->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
+        }
+        PixelMask = MAX_UINT32;
+      } else {
+        PixelMask = (1u << BitsPerPixel) - 1;
+      }
+      ModeInfo->PixelInformation.ReservedMask =
+        PixelMask & ~(RedMask | GreenMask | BlueMask);
+
+      BytesPerLine = VmwareSvgaRead (Private, VmwareSvgaRegBytesPerLine);
+      ModeInfo->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
+
+      ModeData++;
+      ModeInfo++;
+    }
+    VideoMode++;
+  }
+  Private->MaxMode = ModeData - Private->ModeData;
+  return EFI_SUCCESS;
+
+Rollback:
+  FreePool (Private->VmwareSvgaModeInfo);
+  Private->VmwareSvgaModeInfo = NULL;
+
+ModeInfoAllocError:
+  FreePool (Private->ModeData);
+  Private->ModeData = NULL;
+
+ModeDataAllocError:
+  return Status;
+}
-- 
2.3.2 (Apple Git-55)

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 04/05/17 14:55, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
> 
> In addition to the QXL, Cirrus, etc. VGA adapters, Qemu also implements
> a basic version of VMWare's SVGA display device. Drivers for this
> device exist for some guest OSes which do not support Qemu's other
> display adapters, so supporting it in OVMF is useful in conjunction
> with those OSes.
> 
> This change adds support for the SVGA device's framebuffer to
> QemuVideoDxe's graphics output protocol implementation, based on
> VMWare's documentation. The most basic initialisation, framebuffer
> layout query, and mode setting operations are implemented.
> 
> The device relies on port-based 32-bit I/O, unfortunately on misaligned
> addresses. This limits the driver's support to the x86 family of
> platforms.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
> 
> Notes:
>     v2:
>     - Unaligned I/O helper functions moved to separate commit [Laszlo]
>     - Multi-line function call whitespace fixes.
>     
>     v3:
>     - Dropped the "2" from "SVGA2" where appropriate. [Jordan, Laszlo]
>     - Renamed various struct fields and functions with consistent prefixes [Laszlo]
>     - #include orders fixed [Laszlo]
>     - Renamedi/moved lots of local variables to comply with convention. [Laszlo]
>     - Added error checking to PCI BAR queries. [Laszlo]
>     - Moved some function definitions around for better grouping. [Laszlo]
>     - Fixed ClearScreen() to use the correct VRAM BAR. [Laszlo]
>     - Changed modelist initialisation to fetch all mode data on startup, so mode
>       queries can return everything including channel masks without hitting the
>       device. Mask calculations hopefully make more sense now. [Laszlo]
>     - Whitespace fixes. [Laszlo]
>     - Fixed a memory leak in BAR query.
>     
>     v4:
>     - Simplified mode info pixel mask calculation & PCI BAR OOB check. [Laszlo]
>     - Replaced struct assignment with CopyMem. [Laszlo]
>     - Whitespace & comment typo fixes. [Laszlo]

Also, you moved around the "BytesPerLine" assignment.

Then I noticed that you actually moved around the *second* instance of
it, so you continue to have two identical assignments, one of which
should be superfluous:

[snip]

> +EFI_STATUS
> +QemuVideoVmwareSvgaModeSetup (
> +  QEMU_VIDEO_PRIVATE_DATA *Private
> +  )
> +{
> +  EFI_STATUS                            Status;
> +  UINT32                                FbSize;
> +  UINT32                                MaxWidth, MaxHeight;
> +  UINT32                                Capabilities;
> +  UINT32                                BitsPerPixel;
> +  UINT32                                Index;
> +  QEMU_VIDEO_MODE_DATA                  *ModeData;
> +  QEMU_VIDEO_BOCHS_MODES                *VideoMode;
> +  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *ModeInfo;
> +
> +  VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 0);
> +
> +  Private->ModeData =
> +    AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT);
> +  if (Private->ModeData == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto ModeDataAllocError;
> +  }
> +
> +  Private->VmwareSvgaModeInfo =
> +    AllocatePool (
> +      sizeof (Private->VmwareSvgaModeInfo[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT
> +      );
> +  if (Private->VmwareSvgaModeInfo == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto ModeInfoAllocError;
> +  }
> +
> +  FbSize =       VmwareSvgaRead (Private, VmwareSvgaRegFbSize);
> +  MaxWidth =     VmwareSvgaRead (Private, VmwareSvgaRegMaxWidth);
> +  MaxHeight =    VmwareSvgaRead (Private, VmwareSvgaRegMaxHeight);
> +  Capabilities = VmwareSvgaRead (Private, VmwareSvgaRegCapabilities);
> +  if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
> +    BitsPerPixel = VmwareSvgaRead (
> +                     Private,
> +                     VmwareSvgaRegHostBitsPerPixel
> +                     );
> +    VmwareSvgaWrite (
> +      Private,
> +      VmwareSvgaRegBitsPerPixel,
> +      BitsPerPixel
> +      );
> +  } else {
> +    BitsPerPixel = VmwareSvgaRead (
> +                     Private,
> +                     VmwareSvgaRegBitsPerPixel
> +                     );
> +  }
> +
> +  if (FbSize == 0       ||
> +      MaxWidth == 0     ||
> +      MaxHeight == 0    ||
> +      BitsPerPixel == 0 ||
> +      BitsPerPixel % 8 != 0) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto Rollback;
> +  }
> +
> +  ModeData = Private->ModeData;
> +  ModeInfo = Private->VmwareSvgaModeInfo;
> +  VideoMode = &QemuVideoBochsModes[0];
> +  for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index++) {
> +    UINTN RequiredFbSize;
> +
> +    RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height *
> +                     (BitsPerPixel / 8);
> +    if (RequiredFbSize <= FbSize     &&
> +        VideoMode->Width <= MaxWidth &&
> +        VideoMode->Height <= MaxHeight) {
> +      UINT32  BytesPerLine;
> +      UINT32  RedMask, GreenMask, BlueMask, PixelMask;
> +
> +      VmwareSvgaWrite (
> +        Private,
> +        VmwareSvgaRegWidth,
> +        VideoMode->Width
> +        );
> +      VmwareSvgaWrite (
> +        Private,
> +        VmwareSvgaRegHeight,
> +        VideoMode->Height
> +        );
> +      BytesPerLine = VmwareSvgaRead (
> +                       Private,
> +                       VmwareSvgaRegBytesPerLine
> +                       );

This is #1.

> +
> +      ModeData->InternalModeIndex    = Index;
> +      ModeData->HorizontalResolution = VideoMode->Width;
> +      ModeData->VerticalResolution   = VideoMode->Height;
> +      ModeData->ColorDepth           = BitsPerPixel;
> +
> +      //
> +      // Setting VmwareSvgaRegWidth/VmwareSvgaRegHeight actually changes
> +      // the device's display mode, so we save all properties of each mode up
> +      // front to avoid inadvertent mode changes later.
> +      //
> +      ModeInfo->Version = 0;
> +      ModeInfo->HorizontalResolution = ModeData->HorizontalResolution;
> +      ModeInfo->VerticalResolution   = ModeData->VerticalResolution;
> +
> +      ModeInfo->PixelFormat = PixelBitMask;
> +
> +      RedMask   = VmwareSvgaRead (Private, VmwareSvgaRegRedMask);
> +      ModeInfo->PixelInformation.RedMask = RedMask;
> +
> +      GreenMask = VmwareSvgaRead (Private, VmwareSvgaRegGreenMask);
> +      ModeInfo->PixelInformation.GreenMask = GreenMask;
> +
> +      BlueMask  = VmwareSvgaRead (Private, VmwareSvgaRegBlueMask);
> +      ModeInfo->PixelInformation.BlueMask = BlueMask;
> +
> +      //
> +      // Reserved mask is whatever bits in the pixel not containing RGB data,
> +      // so start with binary 1s for every bit in the pixel, then mask off
> +      // bits already used for RGB. Special case 32 to avoid undefined
> +      // behaviour in the shift.
> +      //
> +      if (BitsPerPixel == 32) {
> +        if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
> +          ModeInfo->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
> +        } else if (BlueMask == 0xff0000 &&
> +                   GreenMask == 0xff00 &&
> +                   RedMask == 0xff) {
> +          ModeInfo->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
> +        }
> +        PixelMask = MAX_UINT32;
> +      } else {
> +        PixelMask = (1u << BitsPerPixel) - 1;
> +      }
> +      ModeInfo->PixelInformation.ReservedMask =
> +        PixelMask & ~(RedMask | GreenMask | BlueMask);
> +
> +      BytesPerLine = VmwareSvgaRead (Private, VmwareSvgaRegBytesPerLine);

This is #2.

Which one do you want to keep? I think we should drop #1, and keep #2.

Do you wish to submit v5, or are you okay if I remove #1 for you at
commit time?

With this tweak,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Before committing, I would like to give Jordan a chance to comment as
well. I think Friday this week should be a good day to commit the series.

Impressive work!

Thank you,
Laszlo

> +      ModeInfo->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
> +
> +      ModeData++;
> +      ModeInfo++;
> +    }
> +    VideoMode++;
> +  }
> +  Private->MaxMode = ModeData - Private->ModeData;
> +  return EFI_SUCCESS;
> +
> +Rollback:
> +  FreePool (Private->VmwareSvgaModeInfo);
> +  Private->VmwareSvgaModeInfo = NULL;
> +
> +ModeInfoAllocError:
> +  FreePool (Private->ModeData);
> +  Private->ModeData = NULL;
> +
> +ModeDataAllocError:
> +  return Status;
> +}
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 2017-04-05 06:58:33, Laszlo Ersek wrote:
> 
> This is #2.
> 
> Which one do you want to keep? I think we should drop #1, and keep #2.
> 
> Do you wish to submit v5, or are you okay if I remove #1 for you at
> commit time?
> 
> With this tweak,
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Before committing, I would like to give Jordan a chance to comment as
> well. I think Friday this week should be a good day to commit the series.

After convincing myself that, yes, the unaligned i/o was *actually*
required. :) / :(

Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

The one comment I had, is given how verbose the 3 inline assembly
source files are in patch 2, maybe nasm code would actually be cleaner
looking. But, considering we'd need both an IA32 and X64 version, it
isn't much better.

> Impressive work!

Agreed. Thanks for your contribution Phil!

-Jordan

> 
> > +      ModeInfo->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
> > +
> > +      ModeData++;
> > +      ModeInfo++;
> > +    }
> > +    VideoMode++;
> > +  }
> > +  Private->MaxMode = ModeData - Private->ModeData;
> > +  return EFI_SUCCESS;
> > +
> > +Rollback:
> > +  FreePool (Private->VmwareSvgaModeInfo);
> > +  Private->VmwareSvgaModeInfo = NULL;
> > +
> > +ModeInfoAllocError:
> > +  FreePool (Private->ModeData);
> > +  Private->ModeData = NULL;
> > +
> > +ModeDataAllocError:
> > +  return Status;
> > +}
> > 
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Apologies, I noticed something else:

On 04/05/17 14:55, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
> 
> In addition to the QXL, Cirrus, etc. VGA adapters, Qemu also implements
> a basic version of VMWare's SVGA display device. Drivers for this
> device exist for some guest OSes which do not support Qemu's other
> display adapters, so supporting it in OVMF is useful in conjunction
> with those OSes.
> 
> This change adds support for the SVGA device's framebuffer to
> QemuVideoDxe's graphics output protocol implementation, based on
> VMWare's documentation. The most basic initialisation, framebuffer
> layout query, and mode setting operations are implemented.
> 
> The device relies on port-based 32-bit I/O, unfortunately on misaligned
> addresses. This limits the driver's support to the x86 family of
> platforms.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
> 
> Notes:
>     v2:
>     - Unaligned I/O helper functions moved to separate commit [Laszlo]
>     - Multi-line function call whitespace fixes.
>     
>     v3:
>     - Dropped the "2" from "SVGA2" where appropriate. [Jordan, Laszlo]
>     - Renamed various struct fields and functions with consistent prefixes [Laszlo]
>     - #include orders fixed [Laszlo]
>     - Renamedi/moved lots of local variables to comply with convention. [Laszlo]
>     - Added error checking to PCI BAR queries. [Laszlo]
>     - Moved some function definitions around for better grouping. [Laszlo]
>     - Fixed ClearScreen() to use the correct VRAM BAR. [Laszlo]
>     - Changed modelist initialisation to fetch all mode data on startup, so mode
>       queries can return everything including channel masks without hitting the
>       device. Mask calculations hopefully make more sense now. [Laszlo]
>     - Whitespace fixes. [Laszlo]
>     - Fixed a memory leak in BAR query.
>     
>     v4:
>     - Simplified mode info pixel mask calculation & PCI BAR OOB check. [Laszlo]
>     - Replaced struct assignment with CopyMem. [Laszlo]
>     - Whitespace & comment typo fixes. [Laszlo]
> 
>  OvmfPkg/QemuVideoDxe/Qemu.h       |  29 ++++
>  OvmfPkg/QemuVideoDxe/Driver.c     | 128 +++++++++++++++-
>  OvmfPkg/QemuVideoDxe/Gop.c        |  61 +++++++-
>  OvmfPkg/QemuVideoDxe/Initialize.c | 161 ++++++++++++++++++++
>  4 files changed, 372 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
> index 2ce37defc5b8..7fbb25b3efd3 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 {
> @@ -115,10 +116,13 @@ typedef struct {
>    //
>    UINTN                                 MaxMode;
>    QEMU_VIDEO_MODE_DATA                  *ModeData;
> +  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *VmwareSvgaModeInfo;

This array is dynamically allocated in QemuVideoVmwareSvgaModeSetup(),
which is called from QemuVideoControllerDriverStart(). That's OK.

However, it is not freed (except on the error path in
QemuVideoVmwareSvgaModeSetup(), under the Rollback label). It should be
freed in two additional places:

- in QemuVideoControllerDriverStart(), under the FreeModeData label
(which is also part of an (outer) error path),

- in QemuVideoControllerDriverStop(), near

  FreePool (Private->ModeData);

Of course, these additional FreePool() calls should be conditional, as
FreePool() doesn't handle NULL automatically.

I should have noticed this earlier (in v3); sorry about that.

Thanks,
Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel