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 D70C421942325 for ; Wed, 5 Apr 2017 22:12:56 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 439FD80F63; Thu, 6 Apr 2017 05:12:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 439FD80F63 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 439FD80F63 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-84.phx2.redhat.com [10.3.116.84]) by smtp.corp.redhat.com (Postfix) with ESMTP id 073BA81886; Thu, 6 Apr 2017 05:12:54 +0000 (UTC) To: Phil Dennis-Jordan , edk2-devel@lists.01.org References: <1491396916-53170-1-git-send-email-lists@philjordan.eu> <1491396916-53170-4-git-send-email-lists@philjordan.eu> Cc: Jordan Justen , Phil Dennis-Jordan From: Laszlo Ersek Message-ID: Date: Thu, 6 Apr 2017 07:12:53 +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: <1491396916-53170-4-git-send-email-lists@philjordan.eu> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 06 Apr 2017 05:12:56 +0000 (UTC) Subject: Re: [PATCH v4 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: Thu, 06 Apr 2017 05:12:57 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Apologies, I noticed something else: On 04/05/17 14:55, 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. > > 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