From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2D19021DFA8F1 for ; Wed, 5 Apr 2017 04:41:38 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9131DC059721; Wed, 5 Apr 2017 11:41:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9131DC059721 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 9131DC059721 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-13.phx2.redhat.com [10.3.116.13]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1D6EF94138; Wed, 5 Apr 2017 11:41:35 +0000 (UTC) To: Phil Dennis-Jordan , edk2-devel@lists.01.org References: <1491386280-50077-1-git-send-email-lists@philjordan.eu> <1491386280-50077-4-git-send-email-lists@philjordan.eu> Cc: Jordan Justen , Phil Dennis-Jordan From: Laszlo Ersek Message-ID: <31192690-0c6a-ce61-8c8f-a19be1c3ccb4@redhat.com> Date: Wed, 5 Apr 2017 13:41:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1491386280-50077-4-git-send-email-lists@philjordan.eu> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 05 Apr 2017 11:41:37 +0000 (UTC) Subject: Re: [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Apr 2017 11:41:38 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 04/05/17 11:58, Phil Dennis-Jordan wrote: > From: Phil Dennis-Jordan > > 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 > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Phil Dennis-Jordan > --- > > 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 > #include > +#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 "(". > + 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. > + 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. > 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 > #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 > #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? > + 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. Thank you, Laszlo