From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 BBBF6211B6C08 for ; Mon, 11 Jun 2018 08:57:00 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 370DD7D84D for ; Mon, 11 Jun 2018 15:56:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-125.rdu2.redhat.com [10.10.120.125]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6B0E22035721; Mon, 11 Jun 2018 15:56:58 +0000 (UTC) To: Gerd Hoffmann References: <20180608113942.17009-1-kraxel@redhat.com> <20180608113942.17009-3-kraxel@redhat.com> From: Laszlo Ersek Cc: edk2-devel@lists.01.org Message-ID: Date: Mon, 11 Jun 2018 17:56:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180608113942.17009-3-kraxel@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Mon, 11 Jun 2018 15:56:59 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Mon, 11 Jun 2018 15:56:59 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 2/4] OvmfPkg: add QemuRamfbDxe X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Jun 2018 15:57:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/08/18 13:39, Gerd Hoffmann wrote: > diff --git a/OvmfPkg/QemuRamfbDxe/QemuRamfb.c b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c > new file mode 100644 > index 0000000000..f04a314c24 > --- /dev/null > +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c > @@ -0,0 +1,308 @@ > +#include (1) I think we should be able to drop this. See the leading comment in "MdePkg/Include/Uefi.h" -- and this is not a UEFI_DRIVER but a platform DXE_DRIVER module. > +#include > + > +#include OK, ZeroMem() is from this one. > +#include OK, DEBUG() needs this. > +#include OK, AppendDevicePathNode(). > +#include OK, FrameBufferBltConfigure(). > +#include OK, AllocateCopyPool(). > +#include (2) I think we can drop this. (Please also remove it from the [LibraryClasses] section in the INF file.) > +#include OK, gBS->InstallMultipleProtocolInterfaces(). > +#include (3) Well, strictly speaking, this isn't necessary as an #include -- the corresponding lib class under [LibraryClasses] *is* necessary though. I've grepped the tree for this #include directive, and usage is inconsistent. In new drivers we introduce, I prefer that we not add the include. Can you please drop it? (Again, do keep it in the INF file.) > +#include (4) I tried to see if we use anything from UefiLib.h, and came up empty. Can you please attempt to drop it both here and from [LibraryClasses] in the INF file? > +#include Yup, needed. > + > +#include > + > +#define RAMFB_FORMAT 0x34325258 /* DRM_FORMAT_XRGB8888 */ > +#define RAMFB_BPP 4 > + > +EFI_GUID gQemuRamfbGuid = QEMU_RAMFB_GUID; (5) Please drop this -- we have the declaration of gQemuRamfbGuid from , and the definition comes from source code auto-generated by the build tools. (I'll comment more on the usage below.) > + > +typedef struct RAMFB_CONFIG { > + UINT64 Address; > + UINT32 FourCC; > + UINT32 Flags; > + UINT32 Width; > + UINT32 Height; > + UINT32 Stride; > +} RAMFB_CONFIG; (6) Please wrap this in: #pragma pack (1) ... #pragma pack () (7) We could also add this type definition in a new file "OvmfPkg/Include/IndustryStandard/QemuRamfbConfig.h". However, I do realize that's somewhat overkill. Up to you; I don't mind if we keep it here. > + > +EFI_HANDLE RamfbHandle; > +EFI_HANDLE GopHandle; > +FRAME_BUFFER_CONFIGURE *QemuRamfbFrameBufferBltConfigure; > +UINTN QemuRamfbFrameBufferBltConfigureSize; (8) Please make all these variables STATIC, and also prepend an "m" prefix to their names (it stands for "module"): STATIC EFI_HANDLE mRamfbHandle; > + > +EFI_GRAPHICS_OUTPUT_MODE_INFORMATION QemuRamfbModeInfo[] = { (9) Same comment as (8). > + { > + .HorizontalResolution = 640, > + .VerticalResolution = 480, > + },{ > + .HorizontalResolution = 800, > + .VerticalResolution = 600, > + },{ > + .HorizontalResolution = 1024, > + .VerticalResolution = 768, > + } > +}; (10) In edk2 we cannot use designated initializers. I suggest (for example) assigning these values in the entry point function. Alternatively, please use the C90-style syntax for aggregate initialization (i.e. just insert enough zeros): STATIC EFI_GRAPHICS_OUTPUT_MODE_INFORMATION QemuRamfbModeInfo[] = { { 0, // Version 640, // HorizontalResolution 400, // VerticalResolution 0, // PixelFormat -- filled in later 0, // PixelInformation -- will be ignored 0 // PixelsPerScanLine -- filled in later }, ... }; > +#define QemuRamfbModeCount (sizeof(QemuRamfbModeInfo)/sizeof(QemuRamfbModeInfo[0])) (11) Please use ARRAY_SIZE() instead, wherever necessary. > + > +EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE QemuRamfbMode = { > + .MaxMode = QemuRamfbModeCount, > + .Mode = 0, > + .Info = QemuRamfbModeInfo, > + .SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION), > +}; (12) See (10). (13) Please insert a space between "sizeof" and "(". In edk2, we put one space between function identifier / function-like macro name, and the opening paren, in function calls / macro invocations. (Obviously, I know that "sizeof" is neither a function nor a macro; it is an operator. In fact my preferred style is just "sizeof object", no parens. However, if we use the parens, which is indeed required for typenames, then we should use one space.) > + > +EFI_STATUS > +EFIAPI > +QemuRamfbGraphicsOutputQueryMode ( (14) Please also make all functions STATIC, except InitializeQemuRamfb(). > + IN EFI_GRAPHICS_OUTPUT_PROTOCOL *This, > + IN UINT32 ModeNumber, > + OUT UINTN *SizeOfInfo, > + OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION **Info > + ) > +{ > + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *ModeInfo; > + > + if (Info == NULL || SizeOfInfo == NULL || ModeNumber > QemuRamfbMode.MaxMode) { (15) I think the ModeNumber comparison is off-by-one; equality should be rejected too. > + return EFI_INVALID_PARAMETER; > + } > + ModeInfo = &QemuRamfbModeInfo[ModeNumber]; > + > + *Info = AllocateCopyPool (sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION), > + ModeInfo); (16) The indentation is incorrect; "ModeInfo" should start 2 colums to the right of the "A" in AllocateCopyPool(). In general, in edk2 there are two accepted indentation styles for function calls that extend to multiple lines: variant #1: all arguments (including the first one) on separate lines, with the closing paren also on a separate line: Status = StructPointer->Function ( Arg1, Arg2, Arg3 ); variant #2: filling horizontal space with arguments up to column 80; and whenever a line break is needed, stick with the same indentation as seen in variant #1. The closing paren is not broken to a separate line. Status = StructPointer->Function (LongArgumentOne, LongArgumentTwo, LongArgumentThree, LongArgumentFour); For both styles, if StructPointer doesn't exist, then the indentation remains the same, relatively speaking, anchored to "Function". > + if (*Info == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + *SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION); > + > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +EFIAPI > +QemuRamfbGraphicsOutputSetMode ( > + IN EFI_GRAPHICS_OUTPUT_PROTOCOL *This, > + IN UINT32 ModeNumber > + ) > +{ > + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *ModeInfo; > + RAMFB_CONFIG Config; > + EFI_GRAPHICS_OUTPUT_BLT_PIXEL Black; > + RETURN_STATUS Ret; (17) We generally call such variables "Status". > + FIRMWARE_CONFIG_ITEM Item; > + UINTN Size; > + > + if (ModeNumber > QemuRamfbMode.MaxMode) { > + return EFI_UNSUPPORTED; > + } (18) See (15). > + ModeInfo = &QemuRamfbModeInfo[ModeNumber]; > + > + DEBUG ((EFI_D_INFO, "Ramfb: SetMode %d (%dx%d)\n", ModeNumber, (19) We've stopped adding EFI_D_* macros; instead we use DEBUG_* macros in new code. (20) Please print UINT32 values with "%u" rather than "%d". > + ModeInfo->HorizontalResolution, > + ModeInfo->VerticalResolution)); (21) Please see (16). > + > + QemuRamfbMode.Mode = ModeNumber; > + QemuRamfbMode.Info = ModeInfo; > + > + Config.Address = SwapBytes64( QemuRamfbMode.FrameBufferBase ); (22) Please #include for SwapBytes*(); also please add it to [LibraryClasses] in the INF file. (23) The space characters near the parens are not idiomatic; we'd use Config.Address = SwapBytes64 (QemuRamfbMode.FrameBufferBase); > + Config.FourCC = SwapBytes32( RAMFB_FORMAT ); > + Config.Flags = SwapBytes32( 0 ); > + Config.Width = SwapBytes32( ModeInfo->HorizontalResolution ); > + Config.Height = SwapBytes32( ModeInfo->VerticalResolution ); > + Config.Stride = SwapBytes32( ModeInfo->HorizontalResolution * RAMFB_BPP ); > + > + QemuFwCfgFindFile("etc/ramfb", &Item, &Size); (24) You perform the same lookup in the entry point function; can you please cache the selector value ("Item") in a new STATIC FIRMWARE_CONFIG_ITEM mRamFbFwCfgItem; global variable? And then use that here. Furthermore, instead of Size, you can use "sizeof Config". > + QemuFwCfgSelectItem(Item); > + QemuFwCfgWriteBytes(sizeof(Config), &Config); (25) (several counts:) please see (13). > + > + Ret = FrameBufferBltConfigure ( > + (VOID*)(UINTN)QemuRamfbMode.FrameBufferBase, > + ModeInfo, > + QemuRamfbFrameBufferBltConfigure, > + &QemuRamfbFrameBufferBltConfigureSize); (26) Please see (16). > + > + if (Ret == RETURN_BUFFER_TOO_SMALL) { > + if (QemuRamfbFrameBufferBltConfigure != NULL) { > + FreePool(QemuRamfbFrameBufferBltConfigure); (27) Please see (13). > + } > + QemuRamfbFrameBufferBltConfigure = > + AllocatePool(QemuRamfbFrameBufferBltConfigureSize); (28) Please see (13). (29) Please reorganize the function as follows: this AllocatePool() call should be moved near the top, so that, if it fails, we can return EFI_OUT_OF_RESOURCES without actually changing fw_cfg or any of the GOP-related data structures. Then, please check the return value for NULL. My understanding is that QemuRamfbMode.FrameBufferBase is invariant (= independent of the mode requested), after the initial setup, so this should be possible. > + > + Ret = FrameBufferBltConfigure ( > + (VOID*)(UINTN)QemuRamfbMode.FrameBufferBase, > + ModeInfo, > + QemuRamfbFrameBufferBltConfigure, > + &QemuRamfbFrameBufferBltConfigureSize); > + } (30) Please see (16). (31) At this point, "Ret" (well, "Status") must either be RETURN_SUCCESS or RETURN_UNSUPPORTED. Please add: if (RETURN_ERROR (Status)) { ASSERT (Status == RETURN_UNSUPPORTED); return Status; } > + > + /* clear screen */ (32) The edk2 style for such comments is: // // clear screen // > + ZeroMem (&Black, sizeof (Black)); > + Ret = FrameBufferBlt ( > + QemuRamfbFrameBufferBltConfigure, > + &Black, > + EfiBltVideoFill, > + 0, 0, > + 0, 0, > + ModeInfo->HorizontalResolution, > + ModeInfo->VerticalResolution, > + 0 > + ); (33) We've got a load of arguments here, can we please do: Status = FrameBufferBlt ( QemuRamfbFrameBufferBltConfigure, &Black, EfiBltVideoFill, 0, // SourceX -- ignored 0, // SourceY -- ignored 0, // DestinationX 0, // DestinationY ModeInfo->HorizontalResolution, // Width ModeInfo->VerticalResolution, // Height 0 // Delta -- ignored ); (34) I agree that this call should never fail, and even if it does, we should still return EFI_SUCCESS about the mode change. But, it might be nice to log a debug message on failure, such as: DEBUG ((DEBUG_WARN, "%a: clearing the screen failed: %r\n", __FUNCTION__, Status)); > + > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +EFIAPI > +QemuRamfbGraphicsOutputBlt ( > + IN EFI_GRAPHICS_OUTPUT_PROTOCOL *This, > + IN EFI_GRAPHICS_OUTPUT_BLT_PIXEL *BltBuffer, OPTIONAL > + IN EFI_GRAPHICS_OUTPUT_BLT_OPERATION BltOperation, > + IN UINTN SourceX, > + IN UINTN SourceY, > + IN UINTN DestinationX, > + IN UINTN DestinationY, > + IN UINTN Width, > + IN UINTN Height, > + IN UINTN Delta > + ) > +{ > + return FrameBufferBlt ( > + QemuRamfbFrameBufferBltConfigure, > + BltBuffer, > + BltOperation, > + SourceX, > + SourceY, > + DestinationX, > + DestinationY, > + Width, > + Height, > + Delta); > +} (35) Please see (16). > + > +EFI_GRAPHICS_OUTPUT_PROTOCOL QemuRamfbGraphicsOutput = { > + .QueryMode = QemuRamfbGraphicsOutputQueryMode, > + .SetMode = QemuRamfbGraphicsOutputSetMode, > + .Blt = QemuRamfbGraphicsOutputBlt, > + .Mode = &QemuRamfbMode, > +}; (36) Please see (10). > + > +EFI_STATUS > +EFIAPI > +InitializeQemuRamfb ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_DEVICE_PATH_PROTOCOL *RamfbDevicePath; > + EFI_DEVICE_PATH_PROTOCOL *GopDevicePath; > + VOID *DevicePath; > + VENDOR_DEVICE_PATH VendorDeviceNode; > + ACPI_ADR_DEVICE_PATH AcpiDeviceNode; > + EFI_STATUS Status; > + RETURN_STATUS Ret; Another background story... EFI_STATUS is for (a) anything defined in the Platform Init and UEFI specifications, (b) library classes that are meant to be used *solely* in modules defined by those specifications. RETURN_STATUS is for libraries that are lower-level than those, at least conceptually; in other words, if they provide functionality that is independent of client module type ("BASE" libraries). In theory such libraries are OK to use in SEC modules even. Technically, there isn't any difference between these types and their values, however. Thus, if you have a function in which you already have an EFI_STATUS local variable (for storing EFI_STATUS values), you can freely store RETURN_STATUS retvals to it as well. If you only assign RETURN_STATUS values, then it's more idiomatic to use RETURN_STATUS. (Which is what you do in QemuRamfbGraphicsOutputSetMode(), correctly.) For RETURN_STATUS variables, we have macros such as: - RETURN_ERROR (Status) - ASSERT_RETURN_ERROR (Status) For EFI_STATUS variables, we have the same, just replace RETURN with EFI. (37) So, I suggest dropping "Ret", and just using Status. > + FIRMWARE_CONFIG_ITEM Item; (38) See (24). > + EFI_PHYSICAL_ADDRESS FbBase; > + UINTN Size, FbSize, MaxFbSize, Pages, Index; (39) I know it's annoying, but please break at least *some* of those to separate declarations. I guess keeping "FbSize" and "MaxFbSize" on the same line makes sense. Furthermore, "Size" should be called "FwCfgSize" (for clarity). > + > + DEBUG ((EFI_D_INFO, "InitializeQemuRamfb\n")); (40) Please see (19). (There are some more occurrences below.) (41) Also, I suggest using "%a" with __FUNCTION__. > + > + if (!QemuFwCfgIsAvailable()) { > + DEBUG ((EFI_D_INFO, "Ramfb: no FwCfg\n")); > + return EFI_NOT_FOUND; > + } > + > + Ret = QemuFwCfgFindFile("etc/ramfb", &Item, &Size); > + if (Ret != RETURN_SUCCESS) { > + DEBUG ((EFI_D_INFO, "Ramfb: no etc/ramfb in FwCfg\n")); > + return EFI_NOT_FOUND; > + } (42) For whatever reason, the idiomatic spelling for the comparison is if (EFI_ERROR (Status)) { ... } (43) You might want to drop the debug message for this case, as in most setups, I expect ramfb will not be present; plus, if the driver exits with an error code, the DXE core will log that anyway. If you'd really like to keep the message, I suggest DEBUG_VERBOSE. (44) If the lookup succeeds, please compare "FwCfgSize" against sizeof (RAMFB_CONFIG). In case of mismatch, log an error (DEBUG_ERROR) and exit. > + > + MaxFbSize = 0; > + for (Index = 0; Index < QemuRamfbModeCount; Index++) { > + QemuRamfbModeInfo[Index].PixelsPerScanLine = > + QemuRamfbModeInfo[Index].HorizontalResolution; > + QemuRamfbModeInfo[Index].PixelFormat = > + PixelBlueGreenRedReserved8BitPerColor, (45) Nice use of the comma operator :) but I think it may not be intended. Please let's stick with the semicolon. > + FbSize = RAMFB_BPP * > + QemuRamfbModeInfo[Index].HorizontalResolution * > + QemuRamfbModeInfo[Index].VerticalResolution; > + if (MaxFbSize < FbSize) > + MaxFbSize = FbSize; (46) The edk2 coding style requires braces. > + DEBUG ((EFI_D_INFO, "Ramfb: Mode %d: %dx%d, %d kB\n", Index, > + QemuRamfbModeInfo[Index].HorizontalResolution, > + QemuRamfbModeInfo[Index].VerticalResolution, > + FbSize / 1024)); > + } (47) indentation, please see (16). (48) For printing UINTN values portably across 32-bit and 64-bit builds, the safest way is to cast the variable to UINT64, and then print it with "%Lu" (or, if you like hex, "%Lx"). This refers to both "Index" and "FbSize". (Also, feel free to use lower-case "l" in place of my suggested upper-case "L"; in edk2 they are interchangeable for printing integers. I just dislike lower-case "l" because in ISO C, it stands for "long", whereas "L" is not defined in ISO C for integers. I think that makes "L" a better fit for a non-standard purpose such as "print UINT64". And, no, edk2 does not have PRIx64.) (49) EFI_D_*, please see (19) (50) Please see (20) as well, for printing the resolutions. > + > + Pages = EFI_SIZE_TO_PAGES(MaxFbSize); > + MaxFbSize = EFI_PAGES_TO_SIZE(Pages); > + FbBase = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateRuntimePages(Pages); (51) Multiple instances of (13). (52) What Ard said about runtime allocations. Please call AllocateReservedPages() instead. > + if (!FbBase) { (53) In edk2, we only use the logical negation operator (!) for BOOLEANs. Please compare FbBase against 0 explicitly. > + DEBUG ((EFI_D_INFO, "Ramfb: memory allocation failed\n")); > + return EFI_OUT_OF_RESOURCES; > + } (54) please see (19). > + DEBUG ((EFI_D_INFO, "Ramfb: Framebuffer at 0x%lx, %d kB, %d pages\n", > + FbBase, MaxFbSize / 1024, Pages)); (55) Please see (48) for "MaxFbSize / 1024" and "Pages". > + QemuRamfbMode.FrameBufferSize = MaxFbSize; > + QemuRamfbMode.FrameBufferBase = FbBase; > + > + /* 800 x 600 */ > + QemuRamfbGraphicsOutputSetMode (&QemuRamfbGraphicsOutput, 1); > + > + /* ramfb vendor devpath */ (56) Please refer to (32); two instances above. > + ZeroMem (&VendorDeviceNode, sizeof (VENDOR_DEVICE_PATH)); > + VendorDeviceNode.Header.Type = HARDWARE_DEVICE_PATH; > + VendorDeviceNode.Header.SubType = HW_VENDOR_DP; > + VendorDeviceNode.Guid = gQemuRamfbGuid; (57) Unfortunately, structure assignment is forbidden in edk2; please use the CopyGuid() function from BaseMemoryLib. See more below. > + SetDevicePathNodeLength (&VendorDeviceNode.Header, sizeof (VENDOR_DEVICE_PATH)); (58) This line seems overlong (82 characters); please stick with 80 chars. (59) I think you've filled in all members of VendorDeviceNode; can you drop the ZeroMem()? > + > + RamfbDevicePath = AppendDevicePathNode ( > + NULL, > + (EFI_DEVICE_PATH_PROTOCOL *) &VendorDeviceNode); > + (60) Please see (16). (61) Please check the return value. AppendDevicePathNode() allocates memory dynamically. If it fails, we should roll back earlier operations (if any) and exit with EFI_OUT_OF_RESOURCES. > + Status = gBS->InstallMultipleProtocolInterfaces ( > + &RamfbHandle, > + &gEfiDevicePathProtocolGuid, RamfbDevicePath, > + NULL); (62) Please see (16). > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_INFO, "Ramfb: install Ramfb Vendor DevicePath failed\n")); (63) We should use DEBUG_ERROR here. (64) In such cases it is helpful to print Status too; please use the "%r" format specifier for that. > + FreePool((VOID*)(UINTN)QemuRamfbMode.FrameBufferBase); (65) Please see (13). > + return Status; (65) This leaks two objects: - the RamfbHandle handle created with gBS->InstallMultipleProtocolInterfaces(), - the RamfbDevicePath devpath created with AppendDevicePathNode(). In edk2 we like the "cascading error path", with several labels and goto statements; I suggest we put it to use as well. On the error path, we should tear down resources in reverse order of construction, as usual: - in UEFI, a handle is created when the first protocol interface is installed on an initially NULL handle; and a handle destroyed when the last protocol interface is uninstalled from it. Common pitfall: InstallMultipleProtocolInterfaces() takes a *pointer to* a handle (because if the handle is NULL on input, the function wants to store the new handle on output); however, UninstallMultipleProtocolInterfaces() takes the handle itself! Be careful with the address-of ("&") operator. - "RamfbDevicePath" should be freed with FreePool(). > + } > + > + /* gop devpath + protocol */ (66) please see (32). > + ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH)); (67) Does (59) apply here? > + AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH; > + AcpiDeviceNode.Header.SubType = ACPI_ADR_DP; > + AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (1, 0, 0, 1, 0, > + ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL, > + 0, 0); (68) Please add comments to the individual arguments. Example: "OvmfPkg/VirtioGpuDxe/DriverBinding.c", the initializer of "mAcpiAdr". > + SetDevicePathNodeLength (&AcpiDeviceNode.Header, sizeof (ACPI_ADR_DEVICE_PATH)); > + > + GopDevicePath = AppendDevicePathNode ( > + RamfbDevicePath, > + (EFI_DEVICE_PATH_PROTOCOL *) &AcpiDeviceNode); > + > + Status = gBS->InstallMultipleProtocolInterfaces ( > + &GopHandle, > + &gEfiDevicePathProtocolGuid, GopDevicePath, > + &gEfiGraphicsOutputProtocolGuid, &QemuRamfbGraphicsOutput, > + NULL); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_INFO, "Ramfb: install GOP DevicePath failed\n")); > + FreePool((VOID*)(UINTN)QemuRamfbMode.FrameBufferBase); > + return Status; > + } (69) Same comments as (60) through (65). > + > + gBS->OpenProtocol ( > + RamfbHandle, > + &gEfiDevicePathProtocolGuid, > + &DevicePath, > + gImageHandle, > + GopHandle, > + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER); (70) Please see (16). (71) Please check the return status. If the call fails, please roll back the earlier actions and propagate the error out of the entry point function. > + > + return EFI_SUCCESS; > +} > diff --git a/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > new file mode 100644 > index 0000000000..75a7d86832 > --- /dev/null > +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > @@ -0,0 +1,34 @@ (72) Here's a shared request for both new files ("QemuRamfb.c" and "QemuRamfbDxe.inf"): please refer to the standard .c and .inf file "banners" for example under OvmfPkg/VirtioGpuDxe. Files should start with: - a @file comment that briefly states what the file is good for, - a list of copyright notices, - the copyright license / reference (2-clause BSDL usually). > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = QemuRamfbDxe > + FILE_GUID = dce1b094-7dc6-45d0-9fdd-d7fc3cc3e4ef > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + > + ENTRY_POINT = InitializeQemuRamfb > + > +[Sources.common] > + QemuRamfb.c (73) We can simply say [Sources] here. > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseMemoryLib > + DebugLib > + DevicePathLib > + FrameBufferBltLib > + MemoryAllocationLib > + UefiBootManagerLib > + UefiBootServicesTableLib > + UefiDriverEntryPoint > + UefiLib > + QemuFwCfgLib > + > +[Protocols] > + gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START (74) The "BY_START" protocol usage comment is incorrect here, because we're not producing instances of the protocol in a DriverBindingStart() function. (In other words, the driver is not a UEFI_DRIVER that conforms to the UEFI driver model, instead it's a platform DXE driver.) Please use the comment "## PRODUCES" (you can also drop "PROTOCOL"). (75) In turn, is where I refer back to (5) and (57). Please add a section like this: [Guids] gQemuRamfbGuid This will ensure that the build utilities generate a "gQemuRamfbGuid" *definition* (not just a declaration) for you. > + > +[Depex] > + TRUE > I realize this review ended up quite long and tedious. I think (hope!) that you're going to contribute to edk2 in the longer term, thus I intended to use this opportunity to spell out idioms and such that might apply to your future patches too. Thank you! Laszlo