* [PATCH v3 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA framebuffer support
@ 2017-04-05 9:57 Phil Dennis-Jordan
2017-04-05 9:57 ` [PATCH v3 1/3] OvmfPkg: VMWare SVGA display device register definitions Phil Dennis-Jordan
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Phil Dennis-Jordan @ 2017-04-05 9:57 UTC (permalink / raw)
To: edk2-devel
This extends the QemuVideoDxe driver to support the VMWare SVGA display
device implemented by Qemu. Drivers for this device exist for guest OSes
which do not support Qemu's other display adapters, so supporting it in
OVMF is useful in conjunction with those OSes.
I've tried to follow the existing pattern for device-specific code in
OVMF's QemuVideoDxe driver as much as possible, with a minimum of
additional code.
For the functionality this driver uses, 2 I/O ports are used with
32-bit wide reads and writes. Unfortunately, one of them is not 32-bit
aligned. This is fine as far as x86/x86-64 is concerned, but neither
EDK2's IoLib nor other platforms support such an access pattern.
This issue was already encountered/discussed on the edk2-devel list 4
years ago: http://edk2-devel.narkive.com/bwH3r0us/unaligned-i-o
I've therefore added UnalignedIoWrite/Read32() helper functions for
Ia32/X64, which I've based on IoLib's aligned ones.
Because querying mode properties on the device is only possible by
actually entering the mode, all display modes are queried on startup
and saved for when they are queried.
Feature branch: https://github.com/pmj/edk2/tree/ovmf_vmware_svga2_v3
v2:
- Unaligned I/O helpers are now in a separate commit. [Laszlo]
- New header file with only essential device constants [Laszlo]
- ArmVirtPkg build failures fixed [Laszlo]
- Code formatting improvements in main driver code.
v3:
- Hardware describing header definitions now prefixed [Jordan, Laszlo]
- "2" dropped from "SVGA2" wherever it made sense. [Jordan, Laszlo]
- Various formatting tweaks to the unaligned IO helpers [Laszlo]
- Mode properties are now queried and stored up front. [Laszlo]
- Lots of formatting fixes in the driver code. [Laszlo]
- PCI BAR confusion resolved. [Laszlo]
- Found & fixed a memory leak.
Phil Dennis-Jordan (3):
OvmfPkg: VMWare SVGA display device register definitions
OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.
OvmfPkg/QemuVideoDxe: VMWare SVGA device support
OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 6 +
OvmfPkg/Include/IndustryStandard/VmwareSvga.h | 104 +++++++++++++
OvmfPkg/QemuVideoDxe/Qemu.h | 29 ++++
OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h | 59 +++++++
OvmfPkg/QemuVideoDxe/Driver.c | 127 ++++++++++++++-
OvmfPkg/QemuVideoDxe/Gop.c | 61 +++++++-
OvmfPkg/QemuVideoDxe/Initialize.c | 161 ++++++++++++++++++++
OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c | 70 +++++++++
OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c | 80 ++++++++++
OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c | 78 ++++++++++
OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c | 66 ++++++++
11 files changed, 834 insertions(+), 7 deletions(-)
create mode 100644 OvmfPkg/Include/IndustryStandard/VmwareSvga.h
create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h
create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
--
2.3.2 (Apple Git-55)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/3] OvmfPkg: VMWare SVGA display device register definitions
2017-04-05 9:57 [PATCH v3 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA framebuffer support Phil Dennis-Jordan
@ 2017-04-05 9:57 ` Phil Dennis-Jordan
2017-04-05 10:05 ` Laszlo Ersek
2017-04-05 9:57 ` [PATCH v3 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O Phil Dennis-Jordan
2017-04-05 9:58 ` [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support Phil Dennis-Jordan
2 siblings, 1 reply; 9+ messages in thread
From: Phil Dennis-Jordan @ 2017-04-05 9:57 UTC (permalink / raw)
To: edk2-devel; +Cc: Phil Dennis-Jordan, Jordan Justen, Laszlo Ersek
From: Phil Dennis-Jordan <phil@philjordan.eu>
This adds a header file defining symbolic constants for the VMWare SVGA
virtual display device in preparation for supporting it in
QemuVideoDxe.
It is mostly an extract of the file lib/vmware/svga_reg.h from commit
329dd537456f93a806841ec8a8213aed11395def of VMWare's vmware-svga
repository at git://git.code.sf.net/p/vmware-svga/git (See also
http://vmware-svga.sourceforge.net/ )
Only the bare essentials necessary for initialisation, modesetting and
framebuffer access have been kept from the original file; macro names
have been prefixed with VMWARE_SVGA_ instead of SVGA2_, and the enum
definition has been adapted to comply with EDK2 naming conventions.
The original file was released by VMWare under the MIT license, this
has been retained.
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- New, custom header file instead of importing VMWare's verbatim. [Laszlo]
v3:
- Prefixed macros with VMWARE_SVGA_ instead of SVGA2_ [Jordan, Laszlo]
- Adjusted enum definition to comply with EDK2 convention [Jordan, Laszlo]
- Tweaks to definitions of numeric constants [Laszlo]
- Renamed the file to fit with convention [Jordan, Laszlo]
- Dropped the "2" from SVGA2 wherever appropriate.
OvmfPkg/Include/IndustryStandard/VmwareSvga.h | 104 ++++++++++++++++++++
1 file changed, 104 insertions(+)
diff --git a/OvmfPkg/Include/IndustryStandard/VmwareSvga.h b/OvmfPkg/Include/IndustryStandard/VmwareSvga.h
new file mode 100644
index 000000000000..693d44bab6c3
--- /dev/null
+++ b/OvmfPkg/Include/IndustryStandard/VmwareSvga.h
@@ -0,0 +1,104 @@
+/** @file
+
+ Macro and enum definitions of a subset of port numbers, register identifiers
+ and values required for driving the VMWare SVGA virtual display adapter,
+ also implemented by Qemu.
+
+ This file's contents was extracted from file lib/vmware/svga_reg.h in commit
+ 329dd537456f93a806841ec8a8213aed11395def of VMWare's vmware-svga repository:
+ git://git.code.sf.net/p/vmware-svga/git
+
+
+ Copyright 1998-2009 VMware, Inc. All rights reserved.
+ Portions Copyright 2017 Phil Dennis-Jordan <phil@philjordan.eu>
+
+ Permission is hereby granted, free of charge, to any person
+ obtaining a copy of this software and associated documentation
+ files (the "Software"), to deal in the Software without
+ restriction, including without limitation the rights to use, copy,
+ modify, merge, publish, distribute, sublicense, and/or sell copies
+ of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be
+ included in all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ SOFTWARE.
+
+**/
+
+#ifndef _VMWARE_SVGA_H_
+#define _VMWARE_SVGA_H_
+
+#include <Base.h>
+
+//
+// IDs for recognising the device
+//
+#define VMWARE_PCI_VENDOR_ID_VMWARE 0x15AD
+#define VMWARE_PCI_DEVICE_ID_VMWARE_SVGA2 0x0405
+
+//
+// I/O port BAR offsets for register selection and read/write.
+//
+// The register index is written to the 32-bit index port, followed by a 32-bit
+// read or write on the value port to read or set that register's contents.
+//
+#define VMWARE_SVGA_INDEX_PORT 0x0
+#define VMWARE_SVGA_VALUE_PORT 0x1
+
+//
+// Some of the device's register indices for basic framebuffer functionality.
+//
+typedef enum {
+ VmwareSvgaRegId = 0,
+ VmwareSvgaRegEnable = 1,
+ VmwareSvgaRegWidth = 2,
+ VmwareSvgaRegHeight = 3,
+ VmwareSvgaRegMaxWidth = 4,
+ VmwareSvgaRegMaxHeight = 5,
+
+ VmwareSvgaRegBitsPerPixel = 7,
+
+ VmwareSvgaRegRedMask = 9,
+ VmwareSvgaRegGreenMask = 10,
+ VmwareSvgaRegBlueMask = 11,
+ VmwareSvgaRegBytesPerLine = 12,
+
+ VmwareSvgaRegFbOffset = 14,
+
+ VmwareSvgaRegFbSize = 16,
+ VmwareSvgaRegCapabilities = 17,
+
+ VmwareSvgaRegHostBitsPerPixel = 28,
+} VMWARE_SVGA_REGISTER;
+
+//
+// Values used with VmwareSvgaRegId for sanity-checking the device and getting
+// its version.
+//
+#define VMWARE_SVGA_MAGIC 0x900000U
+#define VMWARE_SVGA_MAKE_ID(ver) (VMWARE_SVGA_MAGIC << 8 | (ver))
+
+#define VMWARE_SVGA_VERSION_2 2
+#define VMWARE_SVGA_ID_2 VMWARE_SVGA_MAKE_ID (VMWARE_SVGA_VERSION_2)
+
+#define VMWARE_SVGA_VERSION_1 1
+#define VMWARE_SVGA_ID_1 VMWARE_SVGA_MAKE_ID (VMWARE_SVGA_VERSION_1)
+
+#define VMWARE_SVGA_VERSION_0 0
+#define VMWARE_SVGA_ID_0 VMWARE_SVGA_MAKE_ID (VMWARE_SVGA_VERSION_0)
+
+//
+// One of the capability bits advertised by VmwareSvgaRegCapabilities.
+//
+#define VMWARE_SVGA_CAP_8BIT_EMULATION BIT8
+
+#endif
--
2.3.2 (Apple Git-55)
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.
2017-04-05 9:57 [PATCH v3 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA framebuffer support Phil Dennis-Jordan
2017-04-05 9:57 ` [PATCH v3 1/3] OvmfPkg: VMWare SVGA display device register definitions Phil Dennis-Jordan
@ 2017-04-05 9:57 ` Phil Dennis-Jordan
2017-04-05 10:16 ` Laszlo Ersek
2017-04-05 9:58 ` [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support Phil Dennis-Jordan
2 siblings, 1 reply; 9+ messages in thread
From: Phil Dennis-Jordan @ 2017-04-05 9:57 UTC (permalink / raw)
To: edk2-devel; +Cc: Phil Dennis-Jordan, Jordan Justen, Laszlo Ersek
From: Phil Dennis-Jordan <phil@philjordan.eu>
The VMWare SVGA display device implemented by Qemu (-vga vmware) uses
an I/O-type BAR which is laid out such that some register offsets are
not aligned to the read/write width with which they are expected to be
accessed. (The register value port has an offset of 1 and requires
32 bit wide read/write access.)
The EFI_PCI_IO_PROTOCOL's Io.Read/Io.Write functions do not support
such unaligned I/O.
Before a driver for this device can be added to QemuVideoDxe, helper
functions for unaligned I/O are therefore required. This adds the
functions UnalignedIoWrite32 and UnalignedIoRead32, based on IoLib's
IoWrite32 and IoRead32, for the Ia32 and X64 architectures. Port I/O
requires inline assembly, so implementations are provided for the GCC,
ICC, and Microsoft compiler families. Such I/O is not possible on other
architectures, a dummy (ASSERT()ing) implementation is therefore
provided to satisfy the linker.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- Separate commit for the unaligned I/O helper functions. [Laszlo]
- Dummy implementations return values despite ASSERT(). [Laszlo]
- Build failure in ArmVirtPkg fixed. [Laszlo]
- More consistent API docs and function ordering.
v3:
- Fixed typos in commit message [Laszlo]
- Added Suggested-by: tag [Laszlo]
- Rewrapped comment lines to 79 chars [Laszlo]
- Corrected whitespace in function calls [Laszlo]
- EFIAPI dropped. [Laszlo]
- Fixed return value in dummy UnsignedIoWrite32 [Laszlo]
- Dropped "N" imm8 constraint in GCC inline asm [Laszlo]
OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 6 ++
OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h | 59 +++++++++++++++
OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c | 70 +++++++++++++++++
OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c | 80 ++++++++++++++++++++
OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c | 78 +++++++++++++++++++
OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c | 66 ++++++++++++++++
6 files changed, 359 insertions(+)
diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
index affb6ffd88e0..346a5aed94fa 100644
--- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
@@ -41,6 +41,12 @@ [Sources.common]
[Sources.Ia32, Sources.X64]
VbeShim.c
+ UnalignedIoGcc.c | GCC
+ UnalignedIoMsc.c | MSFT
+ UnalignedIoIcc.c | INTEL
+
+[Sources.IPF, Sources.EBC, Sources.ARM, Sources.AARCH64]
+ UnalignedIoUnsupported.c
[Packages]
MdePkg/MdePkg.dec
diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h b/OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h
new file mode 100644
index 000000000000..234de6c21bd1
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h
@@ -0,0 +1,59 @@
+/** @file
+ Unaligned port I/O, with implementations for various x86 compilers and a
+ dummy for platforms which do not support unaligned port I/O.
+
+ Copyright (c) 2017, Phil Dennis-Jordan.<BR>
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef _UNALIGNED_IO_INTERNAL_H_
+#define _UNALIGNED_IO_INTERNAL_H_
+
+/**
+ Performs a 32-bit write to the specified, possibly unaligned I/O-type address.
+
+ Writes the 32-bit I/O port specified by Port with the value specified by Value
+ and returns Value. This function must guarantee that all I/O read and write
+ operations are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port I/O port address
+ @param[in] Value 32-bit word to write
+
+ @return The value written to the I/O port.
+
+**/
+UINT32
+UnalignedIoWrite32 (
+ IN UINTN Port,
+ IN UINT32 Value
+ );
+
+/**
+ Reads a 32-bit word from the specified, possibly unaligned I/O-type address.
+
+ Reads the 32-bit I/O port specified by Port. The 32-bit read value is
+ returned. This function must guarantee that all I/O read and write operations
+ are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port The I/O port to read.
+
+ @return The value read.
+
+**/
+UINT32
+UnalignedIoRead32 (
+ IN UINTN Port
+ );
+
+#endif
diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
new file mode 100644
index 000000000000..105d55d3b903
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
@@ -0,0 +1,70 @@
+/** @file
+ Unaligned Port I/O. This file has compiler specifics for GCC as there is no
+ ANSI C standard for doing IO.
+
+ Based on IoLibGcc.c.
+
+ Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+
+#include "UnalignedIoInternal.h"
+
+/**
+ Performs a 32-bit write to the specified, possibly unaligned I/O-type
+ address.
+
+ Writes the 32-bit I/O port specified by Port with the value specified by
+ Value and returns Value. This function must guarantee that all I/O read and
+ write operations are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port I/O port address
+ @param[in] Value 32-bit word to write
+
+ @return The value written to the I/O port.
+
+**/
+UINT32
+UnalignedIoWrite32 (
+ IN UINTN Port,
+ IN UINT32 Value
+ )
+{
+ __asm__ __volatile__ ( "outl %0, %1" : : "a" (Value), "d" ((UINT16)Port) );
+ return Value;
+}
+
+/**
+ Reads a 32-bit word from the specified, possibly unaligned I/O-type address.
+
+ Reads the 32-bit I/O port specified by Port. The 32-bit read value is
+ returned. This function must guarantee that all I/O read and write operations
+ are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port The I/O port to read.
+
+ @return The value read.
+
+**/
+UINT32
+UnalignedIoRead32 (
+ IN UINTN Port
+ )
+{
+ UINT32 Data;
+ __asm__ __volatile__ ( "inl %1, %0" : "=a" (Data) : "d" ((UINT16)Port) );
+ return Data;
+}
+
diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
new file mode 100644
index 000000000000..79f3e446ddba
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
@@ -0,0 +1,80 @@
+/** @file
+ Unaligned port I/O. This file has compiler specifics for ICC as there
+ is no ANSI C standard for doing IO.
+
+ Based on IoLibIcc.c.
+
+ Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+
+#include "UnalignedIoInternal.h"
+
+/**
+ Performs a 32-bit write to the specified, possibly unaligned I/O-type
+ address.
+
+ Writes the 32-bit I/O port specified by Port with the value specified by
+ Value and returns Value. This function must guarantee that all I/O read and
+ write operations are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port I/O port address
+ @param[in] Value 32-bit word to write
+
+ @return The value written to the I/O port.
+
+**/
+UINT32
+UnalignedIoWrite32 (
+ IN UINTN Port,
+ IN UINT32 Value
+ )
+{
+ __asm {
+ mov eax, dword ptr [Value]
+ mov dx, word ptr [Port]
+ out dx, eax
+ }
+
+ return Value;
+}
+
+/**
+ Reads a 32-bit word from the specified, possibly unaligned I/O-type address.
+
+ Reads the 32-bit I/O port specified by Port. The 32-bit read value is
+ returned. This function must guarantee that all I/O read and write operations
+ are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port The I/O port to read.
+
+ @return The value read.
+
+**/
+UINT32
+UnalignedIoRead32 (
+ IN UINTN Port
+ )
+{
+ UINT32 Data;
+
+ __asm {
+ mov dx, word ptr [Port]
+ in eax, dx
+ mov dword ptr [Data], eax
+ }
+
+ return Data;
+}
diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
new file mode 100644
index 000000000000..a466baee8486
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
@@ -0,0 +1,78 @@
+/** @file
+ Unaligned port I/O. This file has compiler specifics for Microsoft C as there
+ is no ANSI C standard for doing IO.
+
+ Based on IoLibMsc.c
+
+ Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+
+#include "UnalignedIoInternal.h"
+
+unsigned long _inpd (unsigned short port);
+unsigned long _outpd (unsigned short port, unsigned long dataword );
+void _ReadWriteBarrier (void);
+
+/**
+ Performs a 32-bit write to the specified, possibly unaligned I/O-type
+ address.
+
+ Writes the 32-bit I/O port specified by Port with the value specified by
+ Value and returns Value. This function must guarantee that all I/O read and
+ write operations are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port I/O port address
+ @param[in] Value 32-bit word to write
+
+ @return The value written to the I/O port.
+
+**/
+UINT32
+UnalignedIoWrite32 (
+ IN UINTN Port,
+ IN UINT32 Value
+ )
+{
+ _ReadWriteBarrier ();
+ _outpd ((UINT16)Port, Value);
+ _ReadWriteBarrier ();
+ return Value;
+}
+
+/**
+ Reads a 32-bit word from the specified, possibly unaligned I/O-type address.
+
+ Reads the 32-bit I/O port specified by Port. The 32-bit read value is
+ returned. This function must guarantee that all I/O read and write operations
+ are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port The I/O port to read.
+
+ @return The value read.
+
+**/
+UINT32
+UnalignedIoRead32 (
+ IN UINTN Port
+ )
+{
+ UINT32 Value;
+
+ _ReadWriteBarrier ();
+ Value = _inpd ((UINT16)Port);
+ _ReadWriteBarrier ();
+ return Value;
+}
diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
new file mode 100644
index 000000000000..57560ab38fcf
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
@@ -0,0 +1,66 @@
+/** @file
+ Unaligned port I/O dummy implementation for platforms which do not support it.
+
+ Copyright (c) 2017, Phil Dennis-Jordan.<BR>
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+
+#include <Library/DebugLib.h>
+#include "UnalignedIoInternal.h"
+
+/**
+ Performs a 32-bit write to the specified, possibly unaligned I/O-type
+ address.
+
+ Writes the 32-bit I/O port specified by Port with the value specified by
+ Value and returns Value. This function must guarantee that all I/O read and
+ write operations are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port I/O port address
+ @param[in] Value 32-bit word to write
+
+ @return The value written to the I/O port.
+
+**/
+UINT32
+UnalignedIoWrite32 (
+ IN UINTN Port,
+ IN UINT32 Value
+ )
+{
+ ASSERT (FALSE);
+ return Value;
+}
+
+/**
+ Reads a 32-bit word from the specified, possibly unaligned I/O-type address.
+
+ Reads the 32-bit I/O port specified by Port. The 32-bit read value is
+ returned. This function must guarantee that all I/O read and write operations
+ are serialized.
+
+ If 32-bit unaligned I/O port operations are not supported, then ASSERT().
+
+ @param[in] Port The I/O port to read.
+
+ @return The value read.
+
+**/
+UINT32
+UnalignedIoRead32 (
+ IN UINTN Port
+ )
+{
+ ASSERT (FALSE);
+ return 0;
+}
--
2.3.2 (Apple Git-55)
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support
2017-04-05 9:57 [PATCH v3 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA framebuffer support Phil Dennis-Jordan
2017-04-05 9:57 ` [PATCH v3 1/3] OvmfPkg: VMWare SVGA display device register definitions Phil Dennis-Jordan
2017-04-05 9:57 ` [PATCH v3 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O Phil Dennis-Jordan
@ 2017-04-05 9:58 ` Phil Dennis-Jordan
2017-04-05 11:41 ` Laszlo Ersek
2 siblings, 1 reply; 9+ messages in thread
From: Phil Dennis-Jordan @ 2017-04-05 9:58 UTC (permalink / raw)
To: edk2-devel; +Cc: Phil Dennis-Jordan, Jordan Justen, Laszlo Ersek
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.
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 <IndustryStandard/VmwareSvga.h>
#include <IndustryStandard/Acpi.h>
+#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) ||
+ IoDesc->ResType != ACPI_ADDRESS_SPACE_TYPE_IO ||
+ IoDesc->AddrRangeMin >= MAX_UINT16 ||
+ IoDesc->AddrRangeMin + VMWARE_SVGA_VALUE_PORT >= MAX_UINT16) {
+ if (IoDesc != NULL) {
+ FreePool (IoDesc);
+ }
+ Status = EFI_DEVICE_ERROR;
+ goto RestoreAttributes;
+ }
+ Private->VmwareSvgaBasePort = (UINT16) IoDesc->AddrRangeMin;
+ FreePool (IoDesc);
+
+ 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 --;
+ }
+
+ 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);
+}
+
+UINT32
+VmwareSvgaRead (
+ QEMU_VIDEO_PRIVATE_DATA *Private,
+ UINT16 Register
+ )
+{
+ UnalignedIoWrite32 (
+ Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
+ Register);
+ return UnalignedIoRead32 (
+ Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT);
+}
+
+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;
}
+
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 <IndustryStandard/VmwareSvga.h>
#include "Qemu.h"
STATIC
@@ -75,6 +76,42 @@ QemuVideoCompleteModeData (
return EFI_SUCCESS;
}
+STATIC
+EFI_STATUS
+QemuVideoVmwareSvgaCompleteModeData (
+ 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));
+ 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)) {
+ 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];
+ } 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 <IndustryStandard/VmwareSvga.h>
#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 ++) {
+ 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
+ // 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.
+ //
+ PixelMask = ((0x1u << 1u) << (BitsPerPixel - 1u)) - 1u;
+ ModeInfo->PixelInformation.ReservedMask =
+ PixelMask & ~(RedMask | GreenMask | BlueMask);
+
+ ModeInfo->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
+
+ ModeData ++;
+ ModeInfo ++;
+ }
+ VideoMode ++;
+ }
+ Private->MaxMode = ModeData - Private->ModeData;
+ return EFI_SUCCESS;
+
+Rollback:
+ FreePool(Private->VmwareSvgaModeInfo);
+ Private->VmwareSvgaModeInfo = NULL;
+
+ModeInfoAllocError:
+ FreePool(Private->ModeData);
+ Private->ModeData = NULL;
+
+ModeDataAllocError:
+ return Status;
+}
--
2.3.2 (Apple Git-55)
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] OvmfPkg: VMWare SVGA display device register definitions
2017-04-05 9:57 ` [PATCH v3 1/3] OvmfPkg: VMWare SVGA display device register definitions Phil Dennis-Jordan
@ 2017-04-05 10:05 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2017-04-05 10:05 UTC (permalink / raw)
To: Phil Dennis-Jordan, edk2-devel; +Cc: Jordan Justen, Phil Dennis-Jordan
On 04/05/17 11:57, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
>
> This adds a header file defining symbolic constants for the VMWare SVGA
> virtual display device in preparation for supporting it in
> QemuVideoDxe.
>
> It is mostly an extract of the file lib/vmware/svga_reg.h from commit
> 329dd537456f93a806841ec8a8213aed11395def of VMWare's vmware-svga
> repository at git://git.code.sf.net/p/vmware-svga/git (See also
> http://vmware-svga.sourceforge.net/ )
>
> Only the bare essentials necessary for initialisation, modesetting and
> framebuffer access have been kept from the original file; macro names
> have been prefixed with VMWARE_SVGA_ instead of SVGA2_, and the enum
> definition has been adapted to comply with EDK2 naming conventions.
>
> The original file was released by VMWare under the MIT license, this
> has been retained.
>
> 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v2:
> - New, custom header file instead of importing VMWare's verbatim. [Laszlo]
>
> v3:
> - Prefixed macros with VMWARE_SVGA_ instead of SVGA2_ [Jordan, Laszlo]
> - Adjusted enum definition to comply with EDK2 convention [Jordan, Laszlo]
> - Tweaks to definitions of numeric constants [Laszlo]
> - Renamed the file to fit with convention [Jordan, Laszlo]
> - Dropped the "2" from SVGA2 wherever appropriate.
>
> OvmfPkg/Include/IndustryStandard/VmwareSvga.h | 104 ++++++++++++++++++++
> 1 file changed, 104 insertions(+)
>
> diff --git a/OvmfPkg/Include/IndustryStandard/VmwareSvga.h b/OvmfPkg/Include/IndustryStandard/VmwareSvga.h
> new file mode 100644
> index 000000000000..693d44bab6c3
> --- /dev/null
> +++ b/OvmfPkg/Include/IndustryStandard/VmwareSvga.h
> @@ -0,0 +1,104 @@
> +/** @file
> +
> + Macro and enum definitions of a subset of port numbers, register identifiers
> + and values required for driving the VMWare SVGA virtual display adapter,
> + also implemented by Qemu.
> +
> + This file's contents was extracted from file lib/vmware/svga_reg.h in commit
> + 329dd537456f93a806841ec8a8213aed11395def of VMWare's vmware-svga repository:
> + git://git.code.sf.net/p/vmware-svga/git
> +
> +
> + Copyright 1998-2009 VMware, Inc. All rights reserved.
> + Portions Copyright 2017 Phil Dennis-Jordan <phil@philjordan.eu>
> +
> + Permission is hereby granted, free of charge, to any person
> + obtaining a copy of this software and associated documentation
> + files (the "Software"), to deal in the Software without
> + restriction, including without limitation the rights to use, copy,
> + modify, merge, publish, distribute, sublicense, and/or sell copies
> + of the Software, and to permit persons to whom the Software is
> + furnished to do so, subject to the following conditions:
> +
> + The above copyright notice and this permission notice shall be
> + included in all copies or substantial portions of the Software.
> +
> + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + SOFTWARE.
> +
> +**/
> +
> +#ifndef _VMWARE_SVGA_H_
> +#define _VMWARE_SVGA_H_
> +
> +#include <Base.h>
> +
> +//
> +// IDs for recognising the device
> +//
> +#define VMWARE_PCI_VENDOR_ID_VMWARE 0x15AD
> +#define VMWARE_PCI_DEVICE_ID_VMWARE_SVGA2 0x0405
> +
> +//
> +// I/O port BAR offsets for register selection and read/write.
> +//
> +// The register index is written to the 32-bit index port, followed by a 32-bit
> +// read or write on the value port to read or set that register's contents.
> +//
> +#define VMWARE_SVGA_INDEX_PORT 0x0
> +#define VMWARE_SVGA_VALUE_PORT 0x1
> +
> +//
> +// Some of the device's register indices for basic framebuffer functionality.
> +//
> +typedef enum {
> + VmwareSvgaRegId = 0,
> + VmwareSvgaRegEnable = 1,
> + VmwareSvgaRegWidth = 2,
> + VmwareSvgaRegHeight = 3,
> + VmwareSvgaRegMaxWidth = 4,
> + VmwareSvgaRegMaxHeight = 5,
> +
> + VmwareSvgaRegBitsPerPixel = 7,
> +
> + VmwareSvgaRegRedMask = 9,
> + VmwareSvgaRegGreenMask = 10,
> + VmwareSvgaRegBlueMask = 11,
> + VmwareSvgaRegBytesPerLine = 12,
> +
> + VmwareSvgaRegFbOffset = 14,
> +
> + VmwareSvgaRegFbSize = 16,
> + VmwareSvgaRegCapabilities = 17,
> +
> + VmwareSvgaRegHostBitsPerPixel = 28,
> +} VMWARE_SVGA_REGISTER;
> +
> +//
> +// Values used with VmwareSvgaRegId for sanity-checking the device and getting
> +// its version.
> +//
> +#define VMWARE_SVGA_MAGIC 0x900000U
> +#define VMWARE_SVGA_MAKE_ID(ver) (VMWARE_SVGA_MAGIC << 8 | (ver))
> +
> +#define VMWARE_SVGA_VERSION_2 2
> +#define VMWARE_SVGA_ID_2 VMWARE_SVGA_MAKE_ID (VMWARE_SVGA_VERSION_2)
> +
> +#define VMWARE_SVGA_VERSION_1 1
> +#define VMWARE_SVGA_ID_1 VMWARE_SVGA_MAKE_ID (VMWARE_SVGA_VERSION_1)
> +
> +#define VMWARE_SVGA_VERSION_0 0
> +#define VMWARE_SVGA_ID_0 VMWARE_SVGA_MAKE_ID (VMWARE_SVGA_VERSION_0)
> +
> +//
> +// One of the capability bits advertised by VmwareSvgaRegCapabilities.
> +//
> +#define VMWARE_SVGA_CAP_8BIT_EMULATION BIT8
> +
> +#endif
>
Looks good to me, thanks!
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.
2017-04-05 9:57 ` [PATCH v3 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O Phil Dennis-Jordan
@ 2017-04-05 10:16 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2017-04-05 10:16 UTC (permalink / raw)
To: Phil Dennis-Jordan, edk2-devel; +Cc: Jordan Justen, Phil Dennis-Jordan
On 04/05/17 11:57, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
>
> The VMWare SVGA display device implemented by Qemu (-vga vmware) uses
> an I/O-type BAR which is laid out such that some register offsets are
> not aligned to the read/write width with which they are expected to be
> accessed. (The register value port has an offset of 1 and requires
> 32 bit wide read/write access.)
>
> The EFI_PCI_IO_PROTOCOL's Io.Read/Io.Write functions do not support
> such unaligned I/O.
>
> Before a driver for this device can be added to QemuVideoDxe, helper
> functions for unaligned I/O are therefore required. This adds the
> functions UnalignedIoWrite32 and UnalignedIoRead32, based on IoLib's
> IoWrite32 and IoRead32, for the Ia32 and X64 architectures. Port I/O
> requires inline assembly, so implementations are provided for the GCC,
> ICC, and Microsoft compiler families. Such I/O is not possible on other
> architectures, a dummy (ASSERT()ing) implementation is therefore
> provided to satisfy the linker.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v2:
> - Separate commit for the unaligned I/O helper functions. [Laszlo]
> - Dummy implementations return values despite ASSERT(). [Laszlo]
> - Build failure in ArmVirtPkg fixed. [Laszlo]
> - More consistent API docs and function ordering.
>
> v3:
> - Fixed typos in commit message [Laszlo]
> - Added Suggested-by: tag [Laszlo]
> - Rewrapped comment lines to 79 chars [Laszlo]
> - Corrected whitespace in function calls [Laszlo]
> - EFIAPI dropped. [Laszlo]
> - Fixed return value in dummy UnsignedIoWrite32 [Laszlo]
> - Dropped "N" imm8 constraint in GCC inline asm [Laszlo]
>
> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 6 ++
> OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h | 59 +++++++++++++++
> OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c | 70 +++++++++++++++++
> OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c | 80 ++++++++++++++++++++
> OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c | 78 +++++++++++++++++++
> OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c | 66 ++++++++++++++++
> 6 files changed, 359 insertions(+)
Looks good, thank you for the update.
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support
2017-04-05 9:58 ` [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support Phil Dennis-Jordan
@ 2017-04-05 11:41 ` Laszlo Ersek
2017-04-05 12:46 ` Phil Dennis-Jordan
[not found] ` <CAAibmn3sm+QGwp1QaBq+_m4UQy1L1rcG7Z3RRq=L4sY-1WmJyQ@mail.gmail.com>
0 siblings, 2 replies; 9+ messages in thread
From: Laszlo Ersek @ 2017-04-05 11:41 UTC (permalink / raw)
To: Phil Dennis-Jordan, edk2-devel; +Cc: Jordan Justen, Phil Dennis-Jordan
On 04/05/17 11:58, 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.
>
> 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 <IndustryStandard/VmwareSvga.h>
> #include <IndustryStandard/Acpi.h>
> +#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 <IndustryStandard/VmwareSvga.h>
> #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 <IndustryStandard/VmwareSvga.h>
> #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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support
2017-04-05 11:41 ` Laszlo Ersek
@ 2017-04-05 12:46 ` Phil Dennis-Jordan
[not found] ` <CAAibmn3sm+QGwp1QaBq+_m4UQy1L1rcG7Z3RRq=L4sY-1WmJyQ@mail.gmail.com>
1 sibling, 0 replies; 9+ messages in thread
From: Phil Dennis-Jordan @ 2017-04-05 12:46 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen, Phil Dennis-Jordan
On Wed, Apr 5, 2017 at 11:41 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/05/17 11:58, 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.
>>
>> 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 <IndustryStandard/VmwareSvga.h>
>> #include <IndustryStandard/Acpi.h>
>> +#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 "(".
Augh, sorry about these, old habits die hard.
>> + 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.
The details of this, >/+1 vs >=, and +4 vs not +4, etc. are probably
debatable, but yes, there's no need (chance of overflow) to make this
2 separate conditions. I've gone with your version.
>> + 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.
Whoops, leftover from moved function.
>> 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 <IndustryStandard/VmwareSvga.h>
>> #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 <IndustryStandard/VmwareSvga.h>
>> #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?
I'm pretty sure you mean
PixelMask = (1u << BitsPerPixel) - 1;
but yes, that's definitely more readable.
>> + 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.
Thanks for your patience and perseverance with this patch! v4 coming
up, hopefully this one's a charm…
Phil
> Thank you,
> Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support
[not found] ` <CAAibmn3sm+QGwp1QaBq+_m4UQy1L1rcG7Z3RRq=L4sY-1WmJyQ@mail.gmail.com>
@ 2017-04-05 12:58 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2017-04-05 12:58 UTC (permalink / raw)
To: Phil Dennis-Jordan; +Cc: Phil Dennis-Jordan, edk2-devel, Jordan Justen
On 04/05/17 14:37, Phil Dennis-Jordan wrote:
>> 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?
> I'm pretty sure you mean
>
> PixelMask = (0x1u << BitsPerPixel) - 1;
Heh, right :)
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-04-05 12:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-05 9:57 [PATCH v3 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA framebuffer support Phil Dennis-Jordan
2017-04-05 9:57 ` [PATCH v3 1/3] OvmfPkg: VMWare SVGA display device register definitions Phil Dennis-Jordan
2017-04-05 10:05 ` Laszlo Ersek
2017-04-05 9:57 ` [PATCH v3 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O Phil Dennis-Jordan
2017-04-05 10:16 ` Laszlo Ersek
2017-04-05 9:58 ` [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support Phil Dennis-Jordan
2017-04-05 11:41 ` Laszlo Ersek
2017-04-05 12:46 ` Phil Dennis-Jordan
[not found] ` <CAAibmn3sm+QGwp1QaBq+_m4UQy1L1rcG7Z3RRq=L4sY-1WmJyQ@mail.gmail.com>
2017-04-05 12:58 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox