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

[Laszlo Ersek <lersek@redhat.com>]

Apologies, I noticed something else:

On 04/05/17 14:55, 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.
>     
>     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