Re: [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support

[Phil Dennis-Jordan <lists@philjordan.eu>]

On Wed, Apr 5, 2017 at 11:41 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/05/17 11:58, 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.
>>
>>  OvmfPkg/QemuVideoDxe/Qemu.h       |  29 ++++
>>  OvmfPkg/QemuVideoDxe/Driver.c     | 127 ++++++++++++++-
>>  OvmfPkg/QemuVideoDxe/Gop.c        |  61 +++++++-
>>  OvmfPkg/QemuVideoDxe/Initialize.c | 161 ++++++++++++++++++++
>>  4 files changed, 371 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..79c430e920d7 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,59 @@ 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) ||
>
> Missing space between "EFI_ERROR" and "(".

Augh, sorry about these, old habits die hard.

>> +        IoDesc->ResType != ACPI_ADDRESS_SPACE_TYPE_IO ||
>> +        IoDesc->AddrRangeMin >= MAX_UINT16 ||
>> +        IoDesc->AddrRangeMin + VMWARE_SVGA_VALUE_PORT >= MAX_UINT16) {
>
> If we wanted to be exact, we would likely express the last two
> conditions with a single
>
>   IoDesc->AddrRangeMin > MAX_UINT16 + 1 - (VMWARE_SVGA_VALUE_PORT + 4)
>
> but I don't necessarily want to obsess about this.

The details of this, >/+1 vs >=, and +4 vs not +4, etc. are probably
debatable, but yes, there's no need (chance of overflow) to make this
2 separate conditions. I've gone with your version.

>> +      if (IoDesc != NULL) {
>> +        FreePool (IoDesc);
>> +      }
>
> Good catch :)
>
>> +      Status = EFI_DEVICE_ERROR;
>> +      goto RestoreAttributes;
>> +    }
>> +    Private->VmwareSvgaBasePort = (UINT16) IoDesc->AddrRangeMin;
>> +    FreePool (IoDesc);
>
> Here too :)
>
>> +
>> +    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 --;
>
> Hm, this change wasn't called for.
>
>   3.2.3 Formatting: Horizontal spacing
>
>         * Never put space between unary operators and the operand. See
>           "Horizontal Spacing".
>
>   5.2.2.3 Do not put space between unary operators and their object
>
>> +    }
>> +
>> +    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 +432,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 +814,7 @@ ClearScreen (
>>    Private->PciIo->Mem.Write (
>>                          Private->PciIo,
>>                          EfiPciIoWidthFillUint32,
>> -                        0,
>> +                        Private->FrameBufferVramBarIndex,
>>                          0,
>>                          0x400000 >> 2,
>>                          &Color
>> @@ -888,6 +952,35 @@ 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);
>
> Please break the closing paren off to a new line.
>
>> +}
>> +
>> +UINT32
>> +VmwareSvgaRead (
>> +  QEMU_VIDEO_PRIVATE_DATA   *Private,
>> +  UINT16                    Register
>> +  )
>> +{
>> +  UnalignedIoWrite32 (
>> +    Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
>> +    Register);
>
> Please break the closing paren off to a new line.
>
>> +  return UnalignedIoRead32 (
>> +           Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT);
>
> Please break the closing paren off to a new line.
>
>> +}
>> +
>> +VOID
>>  VgaOutb (
>>    QEMU_VIDEO_PRIVATE_DATA  *Private,
>>    UINTN                    Reg,
>> @@ -941,6 +1034,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 (
>> @@ -975,3 +1097,4 @@ InitializeQemuVideo (
>>
>>    return Status;
>>  }
>> +
>
> No need to add an empty line here.

Whoops, leftover from moved function.

>> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
>> index 359e9217d3d1..934c9f39b459 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 (
>
> Huh, thanks for making this STATIC :)
>
>> +  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));
>
> Please write "sizeof (*Info)" or "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)) {
>
> Missing space between "EFI_ERROR" and "(".
>
>> +    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) {
>> +    **Info = Private->VmwareSvgaModeInfo[ModeNumber];
>
> In edk2, structure assignment is not allowed (because some compilers
> cannot be prevented from generating intrinsics for them). Please use an
> explicit CopyMem (), like you do above in
> QemuVideoVmwareSvgaCompleteModeData ().
>
>> +  } 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..8c5c993c7d0e 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 ++) {
>
> Hm, yeah, I see this comes from existing code, but please drop the " "
> in "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 all properties of each mode up front
>
> I think you accidentally the verb :)
>
>> +      // 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;
>> +
>> +      BytesPerLine = VmwareSvgaRead (Private, VmwareSvgaRegBytesPerLine);
>> +
>> +      if (BitsPerPixel == 32) {
>> +        if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
>> +          ModeInfo->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
>> +        } else if (BlueMask == 0xff0000 &&
>> +                   GreenMask == 0xff00 &&
>> +                   RedMask == 0xff) {
>> +          ModeInfo->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
>> +        }
>> +      }
>> +
>> +      //
>> +      // Reserved mask is whatever bits in the pixel are not used for RGB.
>> +      // PixelMask is as many binary 1s as BitsPerPixel, but shifting UINT32 by
>> +      // 32 is undefined, so work around that. BitsPerPixel isn't 0 so
>> +      // (BitsPerPixel - 1u) is ok.
>> +      //
>
> Ah, OK. I've used this elsewhere, just in reverse order (first shift by
> variable count - 1, then by 1). However, Jordan disliked the trick, and
> ultimately we replaced it with a simple "if".
>
> In fact, above you already have a (BitsPerPixel == 32) check. At the end
> of that block, you could set PixelMask to MAX_UINT32 explicitly. And,
> you could add an "else" branch, simply with
>
>   PixelMask = (1u << (BitsPerPixel - 1)) - 1;
>
> What do you think?

I'm pretty sure you mean

  PixelMask = (1u << BitsPerPixel) - 1;

but yes, that's definitely more readable.

>> +      PixelMask = ((0x1u << 1u) << (BitsPerPixel - 1u)) - 1u;
>> +      ModeInfo->PixelInformation.ReservedMask =
>> +        PixelMask & ~(RedMask | GreenMask | BlueMask);
>> +
>> +      ModeInfo->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
>> +
>> +      ModeData ++;
>> +      ModeInfo ++;
>> +    }
>> +    VideoMode ++;
>
> Please drop all the spaces before "++".
>
>> +  }
>> +  Private->MaxMode = ModeData - Private->ModeData;
>> +  return EFI_SUCCESS;
>> +
>> +Rollback:
>> +  FreePool(Private->VmwareSvgaModeInfo);
>
> Missing space between "FreePool" and "(".
>
>> +  Private->VmwareSvgaModeInfo = NULL;
>> +
>> +ModeInfoAllocError:
>> +  FreePool(Private->ModeData);
>
> Missing space between "FreePool" and "(".
>
>> +  Private->ModeData = NULL;
>> +
>> +ModeDataAllocError:
>> +  return Status;
>> +}
>>
>
> I think this is a very good update. All my fresh comments have been
> cosmetic. Please submit v4; I expect it to be the final version.

Thanks for your patience and perseverance with this patch! v4 coming
up, hopefully this one's a charm…

Phil

> Thank you,
> Laszlo