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 B560F219392FD for ; Tue, 4 Apr 2017 03:13:30 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F1B6C3D963; Tue, 4 Apr 2017 10:13:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F1B6C3D963 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com F1B6C3D963 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-91.phx2.redhat.com [10.3.116.91]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7C1CA7DE43; Tue, 4 Apr 2017 10:13:28 +0000 (UTC) To: Phil Dennis-Jordan , edk2-devel@lists.01.org References: <1491173097-37305-1-git-send-email-lists@philjordan.eu> <1491173097-37305-4-git-send-email-lists@philjordan.eu> Cc: Jordan Justen , Phil Dennis-Jordan From: Laszlo Ersek Message-ID: <114eee1c-7fd2-24c6-4c31-0fc5ec999dcc@redhat.com> Date: Tue, 4 Apr 2017 12:13:27 +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: <1491173097-37305-4-git-send-email-lists@philjordan.eu> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 04 Apr 2017 10:13:30 +0000 (UTC) Subject: Re: [PATCH v2 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA II 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: Tue, 04 Apr 2017 10:13:30 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 04/03/17 00:44, 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 SVGA2 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 SVGA2 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. > > OvmfPkg/QemuVideoDxe/Qemu.h | 37 +++++++ > OvmfPkg/QemuVideoDxe/Driver.c | 76 ++++++++++++++ > OvmfPkg/QemuVideoDxe/Gop.c | 74 +++++++++++++- > OvmfPkg/QemuVideoDxe/Initialize.c | 107 ++++++++++++++++++++ > 4 files changed, 293 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index 2ce37defc5b8..b5c84c9e695e 100644 > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > @@ -56,6 +56,10 @@ typedef struct { > UINT32 HorizontalResolution; > UINT32 VerticalResolution; > UINT32 ColorDepth; > + // > + // VMWare specific: > + // > + UINT32 PixelsPerLine; // includes any dead space I suggest to call this VmwPixelsPerLine. > } QEMU_VIDEO_MODE_DATA; > > #define PIXEL_RED_SHIFT 0 > @@ -92,6 +96,7 @@ typedef enum { > QEMU_VIDEO_CIRRUS_5446, > QEMU_VIDEO_BOCHS, > QEMU_VIDEO_BOCHS_MMIO, > + QEMU_VIDEO_VMWARE_SVGA2, QEMU_VIDEO_VMW_SVGA? In the IndustryStandard header file, we just use VMW_SVGA. > } QEMU_VIDEO_VARIANT; > > typedef struct { > @@ -119,6 +124,8 @@ typedef struct { > QEMU_VIDEO_VARIANT Variant; > FRAME_BUFFER_CONFIGURE *FrameBufferBltConfigure; > UINTN FrameBufferBltConfigureSize; > + > + UINT16 VMWareSVGA2_BasePort; I suggest to drop the empty line, and to call this VmwSvgaBasePort. > } QEMU_VIDEO_PRIVATE_DATA; > > /// > @@ -502,9 +509,39 @@ QemuVideoBochsModeSetup ( > BOOLEAN IsQxl > ); > > +EFI_STATUS > +QemuVideoVmwareModeSetup ( > + QEMU_VIDEO_PRIVATE_DATA *Private > + ); Call this QemuVideoVmwSvgaModeSetup? > + > VOID > InstallVbeShim ( > IN CONST CHAR16 *CardName, > IN EFI_PHYSICAL_ADDRESS FrameBufferBase > ); > + > +VOID > +QemuVideoVMWSVGA2RegisterWrite ( > + QEMU_VIDEO_PRIVATE_DATA *Private, > + UINT16 reg, > + UINT32 value > + ); VmwSvgaWrite? > + > +UINT32 > +QemuVideoVMWSVGA2RegisterRead ( > + QEMU_VIDEO_PRIVATE_DATA *Private, > + UINT16 reg > + ); VmwSvgaRead? > + > +EFI_STATUS > +QemuVideoVMWSVGA2CompleteModeData ( > + IN QEMU_VIDEO_PRIVATE_DATA *Private, > + OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode > + ); QemuVideoVmwSvgaCompleteModeData? > + > +void InitializeVMWSVGA2GraphicsMode ( > + QEMU_VIDEO_PRIVATE_DATA *Private, > + QEMU_VIDEO_BOCHS_MODES *ModeData > + ); > + InitializeVmwSvga2GraphicsMode? > #endif > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index fc8025ec46de..f01aec6f7b0e 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -15,6 +15,7 @@ > **/ > > #include "Qemu.h" > +#include Aha! So we did use "2" in the header file name. Haven't noticed that until this point. I suggest we stay consistent in all spots, so either please drop the "2" from the header file name in patch #1 (and the include guard macro too), or else please use it everywhere. Also, we should put <> includes before "" includes. (I guess some of the current code doesn't conform to that, just below...) > #include > > EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = { > @@ -58,6 +59,16 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = { > QEMU_VIDEO_BOCHS_MMIO, > L"QEMU VirtIO VGA" > },{ > +#if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > + // > + // Support only platforms which can do unaligned port I/O > + // > + PCI_VENDOR_ID_VMWARE, > + PCI_DEVICE_ID_VMWARE_SVGA2, > + QEMU_VIDEO_VMWARE_SVGA2, > + L"QEMU VMWare SVGA2" > + },{ > +#endif Can you please drop the #if? > 0 /* end of list */ > } > }; > @@ -317,6 +328,45 @@ QemuVideoControllerDriverStart ( > } > > // > + // Check if accessing VMWARE_SVGA2 interface works > + // > + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) { > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *iodesc; Please call this IoDesc. > + UINT32 TargetId; > + UINT32 Svga2IDRead; Please call this Svga2IdRead. Also, please aligh these two variable names with IoDesc horizontally. > + > + Private->PciIo->GetBarAttributes ( > + Private->PciIo, > + PCI_BAR_IDX0, > + NULL, > + (VOID**) &iodesc > + ); Please check the return status of this protocol member call. If - the call fails, or - IoDesc->ResType differs from ACPI_ADDRESS_SPACE_TYPE_IO, or - IoDesc->AddrRangeMin is larger than or equal to MAX_UINT16, then set Status to EFI_DEVICE_ERROR, and jump to RestoreAttributes. > + Private->VMWareSVGA2_BasePort = iodesc->AddrRangeMin; I think you should do an explicit cast here, to UINT16; the AddrRangeMin field has type UINT64, and VS will whine if you don't cast the RHS of the assignment explicitly. > + > + TargetId = SVGA_ID_2; > + while (1) { Please use "while (TRUE)" or "for (;;)". "while (1)" is not unused in edk2, but the two other forms are more idiomatic. > + QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ID, TargetId); > + Svga2IDRead = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_ID); > + if ((Svga2IDRead == TargetId) || (TargetId <= SVGA_ID_0)) { > + break; > + } > + --TargetId; > + } > + > + if (Svga2IDRead != TargetId) { > + DEBUG (( > + DEBUG_ERROR, > + "QemuVideo: QEMU_VIDEO_VMWARE_SVGA2 ID mismatch " > + "(got 0x%x, base address 0x%x)\n", > + Svga2IDRead, > + Private->VMWareSVGA2_BasePort > + )); > + Status = EFI_DEVICE_ERROR; > + goto RestoreAttributes; > + } > + } > + > + // > // Get ParentDevicePath > // > Status = gBS->HandleProtocol ( > @@ -371,6 +421,9 @@ QemuVideoControllerDriverStart ( > case QEMU_VIDEO_BOCHS: > Status = QemuVideoBochsModeSetup (Private, IsQxl); > break; > + case QEMU_VIDEO_VMWARE_SVGA2: > + Status = QemuVideoVmwareModeSetup (Private); > + break; > default: > ASSERT (FALSE); > Status = EFI_DEVICE_ERROR; > @@ -975,3 +1028,26 @@ InitializeQemuVideo ( > > return Status; > } > + > +void InitializeVMWSVGA2GraphicsMode ( Can you please move this function higher up, so that it follows InitializeBochsGraphicsMode() directly? Also, it should start like: VOID InitializeVMWSVGA2GraphicsMode ( ... Please update the function declaration in "Qemu.h" as well. > + QEMU_VIDEO_PRIVATE_DATA *Private, > + QEMU_VIDEO_BOCHS_MODES *ModeData > + ) > +{ > + QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_WIDTH, ModeData->Width); > + QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_HEIGHT, ModeData->Height); > + > + UINT32 capabilities = QemuVideoVMWSVGA2RegisterRead ( > + Private, > + SVGA_REG_CAPABILITIES > + ); Please - call this variable Capabilities, - move its definition to the top of the containing scope, - use a separate assignment -- initialization of automatic storage duration variables ("locals") is forbidden in edk2, even if the initializer were a constant. > + if ((capabilities & SVGA_CAP_8BIT_EMULATION) != 0) { > + QemuVideoVMWSVGA2RegisterWrite( > + Private, > + SVGA_REG_BITS_PER_PIXEL, > + ModeData->ColorDepth > + ); > + } > + SetDefaultPalette (Private); > + ClearScreen (Private); > +} I don't understand how ClearScreen() can work here. ClearScreen() writes MMIO BAR 0. But, on this video card, BAR 0 is expected to be an IO BAR, which contains the register selector and register value ports. > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c > index 359e9217d3d1..59918676e764 100644 > --- a/OvmfPkg/QemuVideoDxe/Gop.c > +++ b/OvmfPkg/QemuVideoDxe/Gop.c > @@ -14,6 +14,7 @@ > **/ > > #include "Qemu.h" > +#include <> should go before "" > > STATIC > VOID > @@ -76,6 +77,63 @@ QemuVideoCompleteModeData ( > } > > > +EFI_STATUS > +QemuVMSVGA2VideoCompleteModeData ( > + IN QEMU_VIDEO_PRIVATE_DATA *Private, > + OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode > + ) > +{ > + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *FrameBufDesc; > + UINT32 RedMask, GreenMask, BlueMask; > + UINT32 BitsPerPixel, BytesPerLine, FBOffset; > + > + Info = Mode->Info; > + Info->Version = 0; > + Info->PixelFormat = PixelBitMask; > + > + RedMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_RED_MASK); > + Info->PixelInformation.RedMask = RedMask; > + > + GreenMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_GREEN_MASK); > + Info->PixelInformation.GreenMask = GreenMask; > + > + BlueMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BLUE_MASK); > + Info->PixelInformation.BlueMask = BlueMask; > + > + QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ENABLE, 1); I don't think this belongs here. Can you put it in InitializeVMWSVGA2GraphicsMode() instead? > + > + FBOffset = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_FB_OFFSET); > + BytesPerLine = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BYTES_PER_LINE); > + BitsPerPixel = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BITS_PER_PIXEL); > + > + if (BitsPerPixel == 32) { > + if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) { > + Info->PixelFormat = PixelBlueGreenRedReserved8BitPerColor; > + } else if (BlueMask == 0xff0000 && GreenMask == 0xff00 && RedMask == 0xff) { > + Info->PixelFormat = PixelRedGreenBlueReserved8BitPerColor; > + } > + } > + > + Info->PixelInformation.ReservedMask = ((0x2u << (BitsPerPixel - 1u)) - 1u) > + & ~(RedMask | GreenMask | BlueMask); I have no idea what this does. Please add a comment that explains it in detail. > + Info->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8); > + > + Private->PciIo->GetBarAttributes ( > + Private->PciIo, > + PCI_BAR_IDX1, > + NULL, > + (VOID**) &FrameBufDesc > + ); So, indeed the video memory is in BAR1. ClearScreen() doesn't match that. You might want to introduce another field in QEMU_VIDEO_PRIVATE_DATA, to carry the BAR number that corresponds to the video memory. > + > + Mode->FrameBufferBase = FrameBufDesc->AddrRangeMin + FBOffset; > + Mode->FrameBufferSize = BytesPerLine * Info->VerticalResolution; > + > + FreePool (FrameBufDesc); > + return EFI_SUCCESS; > +} > + > + > // > // Graphics Output Protocol Member Functions > // > @@ -129,6 +187,10 @@ Routine Description: > (*Info)->VerticalResolution = ModeData->VerticalResolution; > QemuVideoCompleteModeInfo (ModeData, *Info); > > + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) { > + (*Info)->PixelsPerScanLine = ModeData->PixelsPerLine; > + } > + > return EFI_SUCCESS; > } This doesn't seem right. QemuVideoCompleteModeInfo() is called on two paths: QemuVideoGraphicsOutputSetMode() // // for non-vmware // QemuVideoCompleteModeData() QemuVideoCompleteModeInfo() and QemuVideoGraphicsOutputQueryMode() QemuVideoCompleteModeInfo() The first call path is for the mode being *selected*, the second call path is for querying *any* mode. Now, on the first call path, you found QemuVideoCompleteModeInfo() inappropriate for vmw, because you introduced the following in its place: QemuVideoGraphicsOutputSetMode() // // for vmw // QemuVMSVGA2VideoCompleteModeData() // // manual setting of pixel mask // no call to QemuVideoCompleteModeInfo() // But in that case, I don't see how calling QemuVideoCompleteModeInfo() in QemuVideoGraphicsOutputQueryMode() -- that is, on the second path --, and only overriding PixelsPerScanLine, could be correct for vmw. The pixel mask values filled in by QemuVideoCompleteModeInfo() will be incorrect. The mode information should be populated uniformly on both code paths, regardless of whether we are selecting a mode for actual use (== changing hardware state), or we are just querying some random mode (== not changing hardware state). Are you saying that the pixel mask values cannot be fetched & returned without actually selecting those graphics modes? In that case, I think you should pre-compute the pixel mask values in QemuVideoVmwareModeSetup(), where you already iterate through all the modes. (I.e., similarly to PixelsPerLine.) Then both the pixel format setting and the PixelsPerScanLine setting, for vmw, should be extracted to a function similar to QemuVideoCompleteModeInfo(). That new function should be called on both paths above; that is, from QemuVMSVGA2VideoCompleteModeData(), and also from QemuVideoGraphicsOutputQueryMode(). This new function should populate Info from the precomputed, cached values, including all the pixel format fields and the PixelsPerScanLine field. (Even with PixelsPerScanLine you have an inconsistency now: although in QueryMode, you use the PixelsPerLine value precomputed in QemuVideoVmwareModeSetup(), in QemuVMSVGA2VideoCompleteModeData(), you still read the SVGA_REG_BYTES_PER_LINE register directly again. I think that shouldn't be necessary.) > > @@ -176,6 +238,12 @@ Routine Description: > case QEMU_VIDEO_BOCHS: > InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]); > break; > + case QEMU_VIDEO_VMWARE_SVGA2: > + InitializeVMWSVGA2GraphicsMode ( > + Private, > + &QemuVideoBochsModes[ModeData->InternalModeIndex] > + ); > + break; > default: > ASSERT (FALSE); > return EFI_DEVICE_ERROR; > @@ -186,7 +254,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_SVGA2) { > + QemuVMSVGA2VideoCompleteModeData(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..8a6db566ab05 100644 > --- a/OvmfPkg/QemuVideoDxe/Initialize.c > +++ b/OvmfPkg/QemuVideoDxe/Initialize.c > @@ -14,6 +14,8 @@ > **/ > > #include "Qemu.h" > +#include "UnalignedIoInternal.h" > +#include <> includes should go before "" includes. > > > /// > @@ -346,3 +348,108 @@ QemuVideoBochsModeSetup ( > return EFI_SUCCESS; > } > > +VOID > +QemuVideoVMWSVGA2RegisterWrite ( > + QEMU_VIDEO_PRIVATE_DATA *Private, > + UINT16 reg, > + UINT32 value > + ) > +{ > + UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_INDEX_PORT, reg); > + UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_VALUE_PORT, value); > +} > + > +UINT32 > +QemuVideoVMWSVGA2RegisterRead ( > + QEMU_VIDEO_PRIVATE_DATA *Private, > + UINT16 reg > + ) > +{ > + UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_INDEX_PORT, reg); > + return UnalignedIoRead32 (Private->VMWareSVGA2_BasePort + SVGA_VALUE_PORT); > +} Please call "reg" and "value" Register and Value. (Don't forget to update "Qemu.h" too.) Also, I think these two functions should be moved to "Driver.c", just under BochsWrite() / BochsRead(). > + > +EFI_STATUS > +QemuVideoVmwareModeSetup ( > + QEMU_VIDEO_PRIVATE_DATA *Private > + ) > +{ > + UINT32 FBSize; Please call this FbSize. > + UINT32 MaxWidth, MaxHeight; > + UINT32 Capabilities; > + UINT32 HostBitsPerPixel; > + UINT32 Index; > + QEMU_VIDEO_MODE_DATA *ModeData; > + QEMU_VIDEO_BOCHS_MODES *VideoMode; > + > + QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ENABLE, 0); > + > + Private->ModeData = > + AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT); > + if (Private->ModeData == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + FBSize = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_FB_SIZE); > + MaxWidth = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_MAX_WIDTH); > + MaxHeight = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_MAX_HEIGHT); > + Capabilities = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_CAPABILITIES); > + if ((Capabilities & SVGA_CAP_8BIT_EMULATION) != 0) { > + HostBitsPerPixel = QemuVideoVMWSVGA2RegisterRead( Missing space right after "QemuVideoVMWSVGA2RegisterRead". > + Private, > + SVGA_REG_HOST_BITS_PER_PIXEL > + ); > + QemuVideoVMWSVGA2RegisterWrite ( > + Private, > + SVGA_REG_BITS_PER_PIXEL, > + HostBitsPerPixel > + ); > + } else { > + HostBitsPerPixel = QemuVideoVMWSVGA2RegisterRead( Missing space right after "QemuVideoVMWSVGA2RegisterRead". > + Private, > + SVGA_REG_BITS_PER_PIXEL > + ); > + } > + > + ModeData = Private->ModeData; > + VideoMode = &QemuVideoBochsModes[0]; > + for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) { > + UINTN RequiredFbSize; > + > + ASSERT (HostBitsPerPixel % 8 == 0); > + RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height * > + (HostBitsPerPixel / 8); > + if (RequiredFbSize <= FBSize > + && VideoMode->Width <= MaxWidth > + && VideoMode->Height <= MaxHeight) > + { This should be: if (Expression1 && Expression2 && Expression3) { // // dependent code // } > + UINT32 BytesPerLine; > + > + QemuVideoVMWSVGA2RegisterWrite ( > + Private, > + SVGA_REG_WIDTH, > + VideoMode->Width > + ); > + QemuVideoVMWSVGA2RegisterWrite ( > + Private, > + SVGA_REG_HEIGHT, > + VideoMode->Height > + ); > + BytesPerLine = QemuVideoVMWSVGA2RegisterRead ( > + Private, > + SVGA_REG_BYTES_PER_LINE > + ); > + ModeData->PixelsPerLine = BytesPerLine / (HostBitsPerPixel / 8); > + > + ModeData->InternalModeIndex = Index; > + ModeData->HorizontalResolution = VideoMode->Width; > + ModeData->VerticalResolution = VideoMode->Height; > + ModeData->ColorDepth = HostBitsPerPixel; > + > + ModeData ++; > + } > + VideoMode ++; > + } > + Private->MaxMode = ModeData - Private->ModeData; > + return EFI_SUCCESS; > +} > Thanks Laszlo