[PATCH v2 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA2 framebuffer support

[PATCH v2 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA2 framebuffer support

[Phil Dennis-Jordan]

This extends the QemuVideoDxe driver to support the VMWare SVGA2 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.

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.

Phil Dennis-Jordan (3):
  OvmfPkg: VMWare SVGA2 display device register definitions
  OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.
  OvmfPkg/QemuVideoDxe: VMWare SVGA II device support.

 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf          |   6 ++
 OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h | 102 +++++++++++++++++++
 OvmfPkg/QemuVideoDxe/Qemu.h                    |  37 +++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h     |  59 +++++++++++
 OvmfPkg/QemuVideoDxe/Driver.c                  |  76 ++++++++++++++
 OvmfPkg/QemuVideoDxe/Gop.c                     |  74 +++++++++++++-
 OvmfPkg/QemuVideoDxe/Initialize.c              | 107 ++++++++++++++++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c          |  69 +++++++++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c          |  79 +++++++++++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c          |  81 +++++++++++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c  |  65 ++++++++++++
 11 files changed, 754 insertions(+), 1 deletion(-)
 create mode 100644 OvmfPkg/Include/IndustryStandard/VMWareSVGA2.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)

[PATCH v2 1/3] OvmfPkg: VMWare SVGA2 display device register definitions

[Phil Dennis-Jordan]

From: Phil Dennis-Jordan <phil@philjordan.eu>

This adds a header file defining symbolic constants for the VMWare SVGA2
virtual display device in preparation for supporting it in QemuVideoDXE.

It is 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.

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>
---

Notes:
    v2:
    - New, custom header file instead of importing VMWare's verbatim. [Laszlo]

 OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h | 102 ++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
new file mode 100644
index 000000000000..9db553155957
--- /dev/null
+++ b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
@@ -0,0 +1,102 @@
+/** @file
+
+  Macro and enum definitions of a subset of port numbers, register identifiers
+  and values required for driving the VMWare SVGA2 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_SVGA2_H_
+#define _VMWARE_SVGA2_H_
+
+//
+// IDs for recognising the device
+//
+#define PCI_VENDOR_ID_VMWARE            0x15AD
+#define 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 SVGA_INDEX_PORT         0x0
+#define SVGA_VALUE_PORT         0x1
+
+//
+// Some of the device's register indices for basic framebuffer functionality.
+//
+enum {
+  SVGA_REG_ID = 0,
+  SVGA_REG_ENABLE = 1,
+  SVGA_REG_WIDTH = 2,
+  SVGA_REG_HEIGHT = 3,
+  SVGA_REG_MAX_WIDTH = 4,
+  SVGA_REG_MAX_HEIGHT = 5,
+
+  SVGA_REG_BITS_PER_PIXEL = 7,
+
+  SVGA_REG_RED_MASK = 9,
+  SVGA_REG_GREEN_MASK = 10,
+  SVGA_REG_BLUE_MASK = 11,
+  SVGA_REG_BYTES_PER_LINE = 12,
+
+  SVGA_REG_FB_OFFSET = 14,
+
+  SVGA_REG_FB_SIZE = 16,
+  SVGA_REG_CAPABILITIES = 17,
+
+  SVGA_REG_HOST_BITS_PER_PIXEL = 28,
+};
+
+//
+// Values used with SVGA_REG_ID for sanity-checking the device and getting
+// its version.
+//
+#define SVGA_MAGIC         0x900000UL
+#define SVGA_MAKE_ID(ver)  (SVGA_MAGIC << 8 | (ver))
+
+#define SVGA_VERSION_2     2
+#define SVGA_ID_2          SVGA_MAKE_ID(SVGA_VERSION_2)
+
+#define SVGA_VERSION_1     1
+#define SVGA_ID_1          SVGA_MAKE_ID(SVGA_VERSION_1)
+
+#define SVGA_VERSION_0     0
+#define SVGA_ID_0          SVGA_MAKE_ID(SVGA_VERSION_0)
+
+//
+// One of the capability bits advertised by SVGA_REG_CAPABILITIES.
+//
+#define SVGA_CAP_8BIT_EMULATION     0x00000100
+
+#endif
-- 
2.3.2 (Apple Git-55)

Re: [PATCH v2 1/3] OvmfPkg: VMWare SVGA2 display device register definitions

[Jordan Justen]

On 2017-04-02 15:44:55, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
> 
> This adds a header file defining symbolic constants for the VMWare SVGA2
> virtual display device in preparation for supporting it in QemuVideoDXE.
> 
> It is 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.
> 
> 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>
> ---
> 
> Notes:
>     v2:
>     - New, custom header file instead of importing VMWare's verbatim. [Laszlo]
> 
>  OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h | 102 ++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h

I think EDK II's file naming convention would prefer VmwareSvga2.h, or
possibly VmWareSvga2.h. (But, the former looks better to me.)

Since it covers svga and svga2, should we just drop the '2' from the
filename?

> new file mode 100644
> index 000000000000..9db553155957
> --- /dev/null
> +++ b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
> @@ -0,0 +1,102 @@
> +/** @file
> +
> +  Macro and enum definitions of a subset of port numbers, register identifiers
> +  and values required for driving the VMWare SVGA2 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_SVGA2_H_
> +#define _VMWARE_SVGA2_H_
> +
> +//
> +// IDs for recognising the device
> +//
> +#define PCI_VENDOR_ID_VMWARE            0x15AD
> +#define 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 SVGA_INDEX_PORT         0x0
> +#define SVGA_VALUE_PORT         0x1

Thanks for taking Laszlo's advice about pulling out the minimal
definitions and EDK II-ifying it. (I think he may be right about the
Xen contribution.)

One request I have is, how about prefixing all the SVGA items as
VMWARE_SVGA? SVGA seems to generic of a term here.

Thanks,

-Jordan

> +
> +//
> +// Some of the device's register indices for basic framebuffer functionality.
> +//
> +enum {
> +  SVGA_REG_ID = 0,
> +  SVGA_REG_ENABLE = 1,
> +  SVGA_REG_WIDTH = 2,
> +  SVGA_REG_HEIGHT = 3,
> +  SVGA_REG_MAX_WIDTH = 4,
> +  SVGA_REG_MAX_HEIGHT = 5,
> +
> +  SVGA_REG_BITS_PER_PIXEL = 7,
> +
> +  SVGA_REG_RED_MASK = 9,
> +  SVGA_REG_GREEN_MASK = 10,
> +  SVGA_REG_BLUE_MASK = 11,
> +  SVGA_REG_BYTES_PER_LINE = 12,
> +
> +  SVGA_REG_FB_OFFSET = 14,
> +
> +  SVGA_REG_FB_SIZE = 16,
> +  SVGA_REG_CAPABILITIES = 17,
> +
> +  SVGA_REG_HOST_BITS_PER_PIXEL = 28,
> +};
> +
> +//
> +// Values used with SVGA_REG_ID for sanity-checking the device and getting
> +// its version.
> +//
> +#define SVGA_MAGIC         0x900000UL
> +#define SVGA_MAKE_ID(ver)  (SVGA_MAGIC << 8 | (ver))
> +
> +#define SVGA_VERSION_2     2
> +#define SVGA_ID_2          SVGA_MAKE_ID(SVGA_VERSION_2)
> +
> +#define SVGA_VERSION_1     1
> +#define SVGA_ID_1          SVGA_MAKE_ID(SVGA_VERSION_1)
> +
> +#define SVGA_VERSION_0     0
> +#define SVGA_ID_0          SVGA_MAKE_ID(SVGA_VERSION_0)
> +
> +//
> +// One of the capability bits advertised by SVGA_REG_CAPABILITIES.
> +//
> +#define SVGA_CAP_8BIT_EMULATION     0x00000100
> +
> +#endif
> -- 
> 2.3.2 (Apple Git-55)
> 

Re: [PATCH v2 1/3] OvmfPkg: VMWare SVGA2 display device register definitions

[Laszlo Ersek]

On 04/04/17 01:17, Jordan Justen wrote:
> On 2017-04-02 15:44:55, Phil Dennis-Jordan wrote:
>> From: Phil Dennis-Jordan <phil@philjordan.eu>
>>
>> This adds a header file defining symbolic constants for the VMWare SVGA2
>> virtual display device in preparation for supporting it in QemuVideoDXE.
>>
>> It is 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.
>>
>> 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>
>> ---
>>
>> Notes:
>>     v2:
>>     - New, custom header file instead of importing VMWare's verbatim. [Laszlo]
>>
>>  OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h | 102 ++++++++++++++++++++
>>  1 file changed, 102 insertions(+)
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
> 
> I think EDK II's file naming convention would prefer VmwareSvga2.h, or
> possibly VmWareSvga2.h. (But, the former looks better to me.)
> 
> Since it covers svga and svga2, should we just drop the '2' from the
> filename?

Yeah, I suggested sort of the same things, after you -- I reviewed the
patches without reading your (earlier) feedback. I suggested VMW_ and
Vmw as prefixes, but I'm equally fine with the more verbose VMWARE_ and
Vmware.

I also agree with dropping "2" from file names / identifiers / macros
etc. (Or, at least, we should be consistent about the use of "2".)


> 
>> new file mode 100644
>> index 000000000000..9db553155957
>> --- /dev/null
>> +++ b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
>> @@ -0,0 +1,102 @@
>> +/** @file
>> +
>> +  Macro and enum definitions of a subset of port numbers, register identifiers
>> +  and values required for driving the VMWare SVGA2 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_SVGA2_H_
>> +#define _VMWARE_SVGA2_H_
>> +
>> +//
>> +// IDs for recognising the device
>> +//
>> +#define PCI_VENDOR_ID_VMWARE            0x15AD
>> +#define 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 SVGA_INDEX_PORT         0x0
>> +#define SVGA_VALUE_PORT         0x1
> 
> Thanks for taking Laszlo's advice about pulling out the minimal
> definitions and EDK II-ifying it. (I think he may be right about the
> Xen contribution.)
> 
> One request I have is, how about prefixing all the SVGA items as
> VMWARE_SVGA? SVGA seems to generic of a term here.

+1 (as commented elsewhere)

Thanks!
Laszlo

> 
> Thanks,
> 
> -Jordan
> 
>> +
>> +//
>> +// Some of the device's register indices for basic framebuffer functionality.
>> +//
>> +enum {
>> +  SVGA_REG_ID = 0,
>> +  SVGA_REG_ENABLE = 1,
>> +  SVGA_REG_WIDTH = 2,
>> +  SVGA_REG_HEIGHT = 3,
>> +  SVGA_REG_MAX_WIDTH = 4,
>> +  SVGA_REG_MAX_HEIGHT = 5,
>> +
>> +  SVGA_REG_BITS_PER_PIXEL = 7,
>> +
>> +  SVGA_REG_RED_MASK = 9,
>> +  SVGA_REG_GREEN_MASK = 10,
>> +  SVGA_REG_BLUE_MASK = 11,
>> +  SVGA_REG_BYTES_PER_LINE = 12,
>> +
>> +  SVGA_REG_FB_OFFSET = 14,
>> +
>> +  SVGA_REG_FB_SIZE = 16,
>> +  SVGA_REG_CAPABILITIES = 17,
>> +
>> +  SVGA_REG_HOST_BITS_PER_PIXEL = 28,
>> +};
>> +
>> +//
>> +// Values used with SVGA_REG_ID for sanity-checking the device and getting
>> +// its version.
>> +//
>> +#define SVGA_MAGIC         0x900000UL
>> +#define SVGA_MAKE_ID(ver)  (SVGA_MAGIC << 8 | (ver))
>> +
>> +#define SVGA_VERSION_2     2
>> +#define SVGA_ID_2          SVGA_MAKE_ID(SVGA_VERSION_2)
>> +
>> +#define SVGA_VERSION_1     1
>> +#define SVGA_ID_1          SVGA_MAKE_ID(SVGA_VERSION_1)
>> +
>> +#define SVGA_VERSION_0     0
>> +#define SVGA_ID_0          SVGA_MAKE_ID(SVGA_VERSION_0)
>> +
>> +//
>> +// One of the capability bits advertised by SVGA_REG_CAPABILITIES.
>> +//
>> +#define SVGA_CAP_8BIT_EMULATION     0x00000100
>> +
>> +#endif
>> -- 
>> 2.3.2 (Apple Git-55)
>>

Re: [PATCH v2 1/3] OvmfPkg: VMWare SVGA2 display device register definitions

[Laszlo Ersek]

On 04/03/17 00:44, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
> 
> This adds a header file defining symbolic constants for the VMWare SVGA2
> virtual display device in preparation for supporting it in QemuVideoDXE.
> 
> It is 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.
> 
> 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>
> ---
> 
> Notes:
>     v2:
>     - New, custom header file instead of importing VMWare's verbatim. [Laszlo]
> 
>  OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h | 102 ++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
> new file mode 100644
> index 000000000000..9db553155957
> --- /dev/null
> +++ b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
> @@ -0,0 +1,102 @@
> +/** @file
> +
> +  Macro and enum definitions of a subset of port numbers, register identifiers
> +  and values required for driving the VMWare SVGA2 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_SVGA2_H_
> +#define _VMWARE_SVGA2_H_
> +
> +//
> +// IDs for recognising the device
> +//
> +#define PCI_VENDOR_ID_VMWARE            0x15AD
> +#define 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 SVGA_INDEX_PORT         0x0
> +#define SVGA_VALUE_PORT         0x1

Please call these VMW_SVGA_...

> +
> +//
> +// Some of the device's register indices for basic framebuffer functionality.
> +//
> +enum {
> +  SVGA_REG_ID = 0,
> +  SVGA_REG_ENABLE = 1,
> +  SVGA_REG_WIDTH = 2,
> +  SVGA_REG_HEIGHT = 3,
> +  SVGA_REG_MAX_WIDTH = 4,
> +  SVGA_REG_MAX_HEIGHT = 5,
> +
> +  SVGA_REG_BITS_PER_PIXEL = 7,
> +
> +  SVGA_REG_RED_MASK = 9,
> +  SVGA_REG_GREEN_MASK = 10,
> +  SVGA_REG_BLUE_MASK = 11,
> +  SVGA_REG_BYTES_PER_LINE = 12,
> +
> +  SVGA_REG_FB_OFFSET = 14,
> +
> +  SVGA_REG_FB_SIZE = 16,
> +  SVGA_REG_CAPABILITIES = 17,
> +
> +  SVGA_REG_HOST_BITS_PER_PIXEL = 28,
> +};

In edk2, enums are usually defined like this:

typedef enum {
  VmwSvgaRegId = 0,
  VmwSvgaRegEnable = 1,
  ...
} VMW_SVGA_REGISTER;

Upper-case underscore-separated identifiers are used for macros and type
names (typedefs) only; please see 4.3.5 "Type and Macro Names" in the
EDK II C Coding Standards (v2.1 draft).

> +
> +//
> +// Values used with SVGA_REG_ID for sanity-checking the device and getting
> +// its version.
> +//
> +#define SVGA_MAGIC         0x900000UL

The macro names should all start with VMW_ please.

> +#define SVGA_MAKE_ID(ver)  (SVGA_MAGIC << 8 | (ver))

Please define WMV_SVGA_MAGIC only as 0x900000U, not 0x900000UL.

> +
> +#define SVGA_VERSION_2     2
> +#define SVGA_ID_2          SVGA_MAKE_ID(SVGA_VERSION_2)

Please insert a space between VMW_SVGA_MAKE_ID and the opening parenthesis.

> +
> +#define SVGA_VERSION_1     1
> +#define SVGA_ID_1          SVGA_MAKE_ID(SVGA_VERSION_1)
> +
> +#define SVGA_VERSION_0     0
> +#define SVGA_ID_0          SVGA_MAKE_ID(SVGA_VERSION_0)
> +
> +//
> +// One of the capability bits advertised by SVGA_REG_CAPABILITIES.
> +//
> +#define SVGA_CAP_8BIT_EMULATION     0x00000100

I suggest to #include <Base.h> in this header (inside the include
guard), and then defining this macro as BIT8.

> +
> +#endif
> 

With those changes, you can add:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

[PATCH v2 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.

[Phil Dennis-Jordan]

From: Phil Dennis-Jordan <phil@philjordan.eu>

The VMWare SVGA2 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 Pci.Read/Pci.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 Read32, 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>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---

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.

 OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf         |  6 ++
 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h    | 59 ++++++++++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c         | 69 +++++++++++++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c         | 79 +++++++++++++++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c         | 81 ++++++++++++++++++++
 OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c | 65 ++++++++++++++++
 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..a069f3b98087
--- /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 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  I/O port from which to read.
+
+  @return The value read from the specified location.
+
+**/
+UINT32
+UnalignedIoRead32 (
+  IN      UINTN                     Port
+  );
+
+#endif
diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
new file mode 100644
index 000000000000..8bb74c784c06
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
@@ -0,0 +1,69 @@
+/** @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), "Nd"((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  I/O port from which to read.
+
+  @return The value read from the specified location.
+
+**/
+UINT32
+UnalignedIoRead32 (
+  IN      UINTN                     Port
+  )
+{
+  UINT32 Data;
+  __asm__ __volatile__ ( "inl %1, %0" : "=a"(Data) : "Nd"((UINT16)Port) );
+  return Data;
+}
+
diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
new file mode 100644
index 000000000000..ac365a8b6be5
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
@@ -0,0 +1,79 @@
+/** @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  Port  The I/O port to write.
+  @param  Value The value to write to the I/O port.
+
+  @return The value written 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  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..2eda40a47e2b
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
@@ -0,0 +1,81 @@
+/** @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 I/O port operations are not supported, then ASSERT().
+  If Port is not aligned on a 32-bit boundary, then ASSERT().
+
+  @param  Port  The I/O port to write.
+  @param  Value The value to write to the I/O port.
+
+  @return The value written to the I/O port.
+
+**/
+UINT32
+EFIAPI
+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 I/O port operations are not supported, then ASSERT().
+  If Port is not aligned on a 32-bit boundary, then ASSERT().
+
+  @param  Port  The I/O port to read.
+
+  @return The value read.
+
+**/
+UINT32
+EFIAPI
+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..1d37ecb7bec0
--- /dev/null
+++ b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
@@ -0,0 +1,65 @@
+/** @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 "UnalignedIoInternal.h"
+#include <Library/DebugLib.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 0;
+}
+
+/**
+  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  I/O port from which to read.
+
+  @return The value read from the specified location.
+
+**/
+UINT32
+UnalignedIoRead32 (
+  IN      UINTN                     Port
+  )
+{
+  ASSERT (FALSE);
+  return 0;
+}
-- 
2.3.2 (Apple Git-55)

Re: [PATCH v2 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.

[Laszlo Ersek]

On 04/03/17 00:44, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
> 
> The VMWare SVGA2 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 Pci.Read/Pci.Write functions do not support
> such unaligned I/O.

Pci.Read and Pci.Write are for accessing config space; I think you mean
Io.Read and Io.Write.

> 
> 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 Read32, for the Ia32 and X64 architectures. Port I/O

s/Read32/IoRead32/ I believe

> 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>

Please add here:

Suggested-by: Jordan Justen <jordan.l.justen@intel.com>

as the idea comes from Jordan (from 4 years ago or so); is that correct?

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
> 
> 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.
> 
>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf         |  6 ++
>  OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h    | 59 ++++++++++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c         | 69 +++++++++++++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c         | 79 +++++++++++++++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c         | 81 ++++++++++++++++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c | 65 ++++++++++++++++
>  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..a069f3b98087
> --- /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.
> +
> +**/

Can you please rewrap all new files added in this commit to 79
characters? (Even comments that you are copying from under MdePkg.)

> +
> +#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 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  I/O port from which to read.
> +
> +  @return The value read from the specified location.
> +
> +**/
> +UINT32
> +UnalignedIoRead32 (
> +  IN      UINTN                     Port
> +  );
> +
> +#endif
> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
> new file mode 100644
> index 000000000000..8bb74c784c06
> --- /dev/null
> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
> @@ -0,0 +1,69 @@
> +/** @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), "Nd"((UINT16)Port) );

Please insert a space after each second quote character:

"a" (Value)

"Nd" ((UINT16)Port)

Also, a question: what does the N character (constraint?) do in the
input operand specification? I tried to check the gcc inline assembly
docs at <https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html>, and I
couldn't find it. Thanks.

> +  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  I/O port from which to read.
> +
> +  @return The value read from the specified location.
> +
> +**/
> +UINT32
> +UnalignedIoRead32 (
> +  IN      UINTN                     Port
> +  )
> +{
> +  UINT32 Data;
> +  __asm__ __volatile__ ( "inl %1, %0" : "=a"(Data) : "Nd"((UINT16)Port) );
> +  return Data;
> +}
> +

Same comment about inserting spaces.

> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
> new file mode 100644
> index 000000000000..ac365a8b6be5
> --- /dev/null
> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
> @@ -0,0 +1,79 @@
> +/** @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  Port  The I/O port to write.
> +  @param  Value The value to write to the I/O port.
> +
> +  @return The value written 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  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;
> +}

OK, these appear to be verbatim copies from
"MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c".

> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
> new file mode 100644
> index 000000000000..2eda40a47e2b
> --- /dev/null
> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
> @@ -0,0 +1,81 @@
> +/** @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 I/O port operations are not supported, then ASSERT().
> +  If Port is not aligned on a 32-bit boundary, then ASSERT().
> +
> +  @param  Port  The I/O port to write.
> +  @param  Value The value to write to the I/O port.
> +
> +  @return The value written to the I/O port.
> +
> +**/
> +UINT32
> +EFIAPI

Please drop EFIAPI here. Your internal header file doesn't specify it
(which is fine, as it is not a public library interface), so we
shouldn't add EFIAPI here either (even if it would indeed compile, as
this file is for VS only, and there EFIAPI is the only / default calling
convention).

> +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 I/O port operations are not supported, then ASSERT().
> +  If Port is not aligned on a 32-bit boundary, then ASSERT().
> +
> +  @param  Port  The I/O port to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT32
> +EFIAPI
> +UnalignedIoRead32 (
> +  IN      UINTN                     Port
> +  )
> +{
> +  UINT32                            Value;
> +
> +  _ReadWriteBarrier ();
> +  Value = _inpd ((UINT16)Port);
> +  _ReadWriteBarrier ();
> +  return Value;
> +}

Seems OK. (We'll only know for sure if someone builds this on VS :))

> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
> new file mode 100644
> index 000000000000..1d37ecb7bec0
> --- /dev/null
> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
> @@ -0,0 +1,65 @@
> +/** @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 "UnalignedIoInternal.h"
> +#include <Library/DebugLib.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 0;

Well, not really relevant, but I still suggest to return Value, not 0.

> +}
> +
> +/**
> +  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  I/O port from which to read.
> +
> +  @return The value read from the specified location.
> +
> +**/
> +UINT32
> +UnalignedIoRead32 (
> +  IN      UINTN                     Port
> +  )
> +{
> +  ASSERT (FALSE);
> +  return 0;
> +}
> 

With those changes:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Jordan, do you have any comments? (For the whole series too, of course?)

Thanks
Laszlo

Re: [PATCH v2 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.

[Phil Dennis-Jordan]

On Tue, Apr 4, 2017 at 8:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/03/17 00:44, Phil Dennis-Jordan wrote:
>> From: Phil Dennis-Jordan <phil@philjordan.eu>
>>
>> The VMWare SVGA2 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 Pci.Read/Pci.Write functions do not support
>> such unaligned I/O.
>
> Pci.Read and Pci.Write are for accessing config space; I think you mean
> Io.Read and Io.Write.
>
>>
>> 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 Read32, for the Ia32 and X64 architectures. Port I/O
>
> s/Read32/IoRead32/ I believe
>
>> 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>
>
> Please add here:
>
> Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
>
> as the idea comes from Jordan (from 4 years ago or so); is that correct?
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> ---
>>
>> 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.
>>
>>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf         |  6 ++
>>  OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h    | 59 ++++++++++++++
>>  OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c         | 69 +++++++++++++++++
>>  OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c         | 79 +++++++++++++++++++
>>  OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c         | 81 ++++++++++++++++++++
>>  OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c | 65 ++++++++++++++++
>>  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..a069f3b98087
>> --- /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.
>> +
>> +**/
>
> Can you please rewrap all new files added in this commit to 79
> characters? (Even comments that you are copying from under MdePkg.)
>
>> +
>> +#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 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  I/O port from which to read.
>> +
>> +  @return The value read from the specified location.
>> +
>> +**/
>> +UINT32
>> +UnalignedIoRead32 (
>> +  IN      UINTN                     Port
>> +  );
>> +
>> +#endif
>> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
>> new file mode 100644
>> index 000000000000..8bb74c784c06
>> --- /dev/null
>> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c
>> @@ -0,0 +1,69 @@
>> +/** @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), "Nd"((UINT16)Port) );
>
> Please insert a space after each second quote character:
>
> "a" (Value)
>
> "Nd" ((UINT16)Port)
>
> Also, a question: what does the N character (constraint?) do in the
> input operand specification? I tried to check the gcc inline assembly
> docs at <https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html>, and I
> couldn't find it. Thanks.

The N constraint is x86-specific and indicates an 8-bit unsigned
immediate value can be used as the operand. This enables emitting the
OUT  imm8, EAX
instruction variant, see
https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html
In practice, this is most likely not going to happen here, even if
link time optimisation happens to inline the call, as the port number
isn't hardcoded. I can remove it if you like.

>> +  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  I/O port from which to read.
>> +
>> +  @return The value read from the specified location.
>> +
>> +**/
>> +UINT32
>> +UnalignedIoRead32 (
>> +  IN      UINTN                     Port
>> +  )
>> +{
>> +  UINT32 Data;
>> +  __asm__ __volatile__ ( "inl %1, %0" : "=a"(Data) : "Nd"((UINT16)Port) );
>> +  return Data;
>> +}
>> +
>
> Same comment about inserting spaces.
>
>> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
>> new file mode 100644
>> index 000000000000..ac365a8b6be5
>> --- /dev/null
>> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c
>> @@ -0,0 +1,79 @@
>> +/** @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  Port  The I/O port to write.
>> +  @param  Value The value to write to the I/O port.
>> +
>> +  @return The value written 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  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;
>> +}
>
> OK, these appear to be verbatim copies from
> "MdePkg/Library/BaseIoLibIntrinsic/IoLibIcc.c".
>
>> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
>> new file mode 100644
>> index 000000000000..2eda40a47e2b
>> --- /dev/null
>> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c
>> @@ -0,0 +1,81 @@
>> +/** @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 I/O port operations are not supported, then ASSERT().
>> +  If Port is not aligned on a 32-bit boundary, then ASSERT().
>> +
>> +  @param  Port  The I/O port to write.
>> +  @param  Value The value to write to the I/O port.
>> +
>> +  @return The value written to the I/O port.
>> +
>> +**/
>> +UINT32
>> +EFIAPI
>
> Please drop EFIAPI here. Your internal header file doesn't specify it
> (which is fine, as it is not a public library interface), so we
> shouldn't add EFIAPI here either (even if it would indeed compile, as
> this file is for VS only, and there EFIAPI is the only / default calling
> convention).
>
>> +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 I/O port operations are not supported, then ASSERT().
>> +  If Port is not aligned on a 32-bit boundary, then ASSERT().
>> +
>> +  @param  Port  The I/O port to read.
>> +
>> +  @return The value read.
>> +
>> +**/
>> +UINT32
>> +EFIAPI
>> +UnalignedIoRead32 (
>> +  IN      UINTN                     Port
>> +  )
>> +{
>> +  UINT32                            Value;
>> +
>> +  _ReadWriteBarrier ();
>> +  Value = _inpd ((UINT16)Port);
>> +  _ReadWriteBarrier ();
>> +  return Value;
>> +}
>
> Seems OK. (We'll only know for sure if someone builds this on VS :))
>
>> diff --git a/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
>> new file mode 100644
>> index 000000000000..1d37ecb7bec0
>> --- /dev/null
>> +++ b/OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c
>> @@ -0,0 +1,65 @@
>> +/** @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 "UnalignedIoInternal.h"
>> +#include <Library/DebugLib.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 0;
>
> Well, not really relevant, but I still suggest to return Value, not 0.
>
>> +}
>> +
>> +/**
>> +  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  I/O port from which to read.
>> +
>> +  @return The value read from the specified location.
>> +
>> +**/
>> +UINT32
>> +UnalignedIoRead32 (
>> +  IN      UINTN                     Port
>> +  )
>> +{
>> +  ASSERT (FALSE);
>> +  return 0;
>> +}
>>
>
> With those changes:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks for the detailed review Laszlo, I've gone through and
implemented all of your suggestions and requests and will be posting a
new version of the patch series shortly.

Phil


> Jordan, do you have any comments? (For the whole series too, of course?)
>
> Thanks
> Laszlo

Re: [PATCH v2 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.

[Laszlo Ersek]

On 04/05/17 11:16, Phil Dennis-Jordan wrote:
> On Tue, Apr 4, 2017 at 8:19 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 04/03/17 00:44, Phil Dennis-Jordan wrote:

>>> +/**
>>> +  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), "Nd"((UINT16)Port) );
>>
>> Please insert a space after each second quote character:
>>
>> "a" (Value)
>>
>> "Nd" ((UINT16)Port)
>>
>> Also, a question: what does the N character (constraint?) do in the
>> input operand specification? I tried to check the gcc inline assembly
>> docs at <https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html>, and I
>> couldn't find it. Thanks.
> 
> The N constraint is x86-specific and indicates an 8-bit unsigned
> immediate value can be used as the operand. This enables emitting the
> OUT  imm8, EAX
> instruction variant, see
> https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html
> In practice, this is most likely not going to happen here, even if
> link time optimisation happens to inline the call, as the port number
> isn't hardcoded. I can remove it if you like.

Either a comment or removal would be fine, I think.

I feel slightly more attracted to removal because the "N" diverges from
IoWrite32() in "MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c", and that
difference seems to deserve justification.

Thanks
Laszlo

[PATCH v2 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA II device support.

[Phil Dennis-Jordan]

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 SVGA2 display device. Drivers for this
device exist for some guest OSes which do not support Qemu's other
display adapters, so supporting it in OVMF is useful in conjunction
with those OSes.

This change adds support for the SVGA2 device's framebuffer to
QemuVideoDxe's graphics output protocol implementation, based on
VMWare's documentation. The most basic initialisation, framebuffer
layout query, and mode setting operations are implemented.

The device relies on port-based 32-bit I/O, unfortunately on misaligned
addresses. This limits the driver's support to the x86 family of
platforms.

Cc: Jordan Justen <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.

 OvmfPkg/QemuVideoDxe/Qemu.h       |  37 +++++++
 OvmfPkg/QemuVideoDxe/Driver.c     |  76 ++++++++++++++
 OvmfPkg/QemuVideoDxe/Gop.c        |  74 +++++++++++++-
 OvmfPkg/QemuVideoDxe/Initialize.c | 107 ++++++++++++++++++++
 4 files changed, 293 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 2ce37defc5b8..b5c84c9e695e 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -56,6 +56,10 @@ typedef struct {
   UINT32  HorizontalResolution;
   UINT32  VerticalResolution;
   UINT32  ColorDepth;
+  //
+  // VMWare specific:
+  //
+  UINT32  PixelsPerLine; // includes any dead space
 } QEMU_VIDEO_MODE_DATA;
 
 #define PIXEL_RED_SHIFT   0
@@ -92,6 +96,7 @@ typedef enum {
   QEMU_VIDEO_CIRRUS_5446,
   QEMU_VIDEO_BOCHS,
   QEMU_VIDEO_BOCHS_MMIO,
+  QEMU_VIDEO_VMWARE_SVGA2,
 } QEMU_VIDEO_VARIANT;
 
 typedef struct {
@@ -119,6 +124,8 @@ typedef struct {
   QEMU_VIDEO_VARIANT                    Variant;
   FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
   UINTN                                 FrameBufferBltConfigureSize;
+
+  UINT16                                VMWareSVGA2_BasePort;
 } QEMU_VIDEO_PRIVATE_DATA;
 
 ///
@@ -502,9 +509,39 @@ QemuVideoBochsModeSetup (
   BOOLEAN                  IsQxl
   );
 
+EFI_STATUS
+QemuVideoVmwareModeSetup (
+  QEMU_VIDEO_PRIVATE_DATA *Private
+  );
+
 VOID
 InstallVbeShim (
   IN CONST CHAR16         *CardName,
   IN EFI_PHYSICAL_ADDRESS FrameBufferBase
   );
+
+VOID
+QemuVideoVMWSVGA2RegisterWrite (
+  QEMU_VIDEO_PRIVATE_DATA *Private,
+  UINT16                  reg,
+  UINT32                  value
+  );
+
+UINT32
+QemuVideoVMWSVGA2RegisterRead (
+  QEMU_VIDEO_PRIVATE_DATA *Private,
+  UINT16                  reg
+  );
+
+EFI_STATUS
+QemuVideoVMWSVGA2CompleteModeData (
+  IN  QEMU_VIDEO_PRIVATE_DATA           *Private,
+  OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
+  );
+
+void InitializeVMWSVGA2GraphicsMode (
+  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..f01aec6f7b0e 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -15,6 +15,7 @@
 **/
 
 #include "Qemu.h"
+#include <IndustryStandard/VMWareSVGA2.h>
 #include <IndustryStandard/Acpi.h>
 
 EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = {
@@ -58,6 +59,16 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
         QEMU_VIDEO_BOCHS_MMIO,
         L"QEMU VirtIO VGA"
     },{
+#if defined MDE_CPU_IA32 || defined MDE_CPU_X64
+        //
+        // Support only platforms which can do unaligned port I/O
+        //
+        PCI_VENDOR_ID_VMWARE,
+        PCI_DEVICE_ID_VMWARE_SVGA2,
+        QEMU_VIDEO_VMWARE_SVGA2,
+        L"QEMU VMWare SVGA2"
+    },{
+#endif
         0 /* end of list */
     }
 };
@@ -317,6 +328,45 @@ QemuVideoControllerDriverStart (
   }
 
   //
+  // Check if accessing VMWARE_SVGA2 interface works
+  //
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) {
+    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *iodesc;
+    UINT32 TargetId;
+    UINT32 Svga2IDRead;
+
+    Private->PciIo->GetBarAttributes (
+                      Private->PciIo,
+                      PCI_BAR_IDX0,
+                      NULL,
+                      (VOID**) &iodesc
+                      );
+    Private->VMWareSVGA2_BasePort = iodesc->AddrRangeMin;
+
+    TargetId = SVGA_ID_2;
+    while (1) {
+      QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ID, TargetId);
+      Svga2IDRead = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_ID);
+      if ((Svga2IDRead == TargetId) || (TargetId <= SVGA_ID_0)) {
+        break;
+      }
+      --TargetId;
+    }
+
+    if (Svga2IDRead != TargetId) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "QemuVideo: QEMU_VIDEO_VMWARE_SVGA2 ID mismatch "
+        "(got 0x%x, base address 0x%x)\n",
+        Svga2IDRead,
+        Private->VMWareSVGA2_BasePort
+        ));
+      Status = EFI_DEVICE_ERROR;
+      goto RestoreAttributes;
+    }
+  }
+
+  //
   // Get ParentDevicePath
   //
   Status = gBS->HandleProtocol (
@@ -371,6 +421,9 @@ QemuVideoControllerDriverStart (
   case QEMU_VIDEO_BOCHS:
     Status = QemuVideoBochsModeSetup (Private, IsQxl);
     break;
+  case QEMU_VIDEO_VMWARE_SVGA2:
+    Status = QemuVideoVmwareModeSetup (Private);
+    break;
   default:
     ASSERT (FALSE);
     Status = EFI_DEVICE_ERROR;
@@ -975,3 +1028,26 @@ InitializeQemuVideo (
 
   return Status;
 }
+
+void InitializeVMWSVGA2GraphicsMode (
+  QEMU_VIDEO_PRIVATE_DATA   *Private,
+  QEMU_VIDEO_BOCHS_MODES    *ModeData
+  )
+{
+  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_WIDTH, ModeData->Width);
+  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_HEIGHT, ModeData->Height);
+
+  UINT32 capabilities = QemuVideoVMWSVGA2RegisterRead (
+                          Private,
+                          SVGA_REG_CAPABILITIES
+                          );
+  if ((capabilities & SVGA_CAP_8BIT_EMULATION) != 0) {
+    QemuVideoVMWSVGA2RegisterWrite(
+      Private,
+      SVGA_REG_BITS_PER_PIXEL,
+      ModeData->ColorDepth
+      );
+  }
+  SetDefaultPalette (Private);
+  ClearScreen (Private);
+}
diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 359e9217d3d1..59918676e764 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -14,6 +14,7 @@
 **/
 
 #include "Qemu.h"
+#include <IndustryStandard/VMWareSVGA2.h>
 
 STATIC
 VOID
@@ -76,6 +77,63 @@ QemuVideoCompleteModeData (
 }
 
 
+EFI_STATUS
+QemuVMSVGA2VideoCompleteModeData (
+  IN  QEMU_VIDEO_PRIVATE_DATA           *Private,
+  OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
+  )
+{
+  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *Info;
+  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR     *FrameBufDesc;
+  UINT32                                RedMask, GreenMask, BlueMask;
+  UINT32                                BitsPerPixel, BytesPerLine, FBOffset;
+
+  Info = Mode->Info;
+  Info->Version = 0;
+  Info->PixelFormat = PixelBitMask;
+
+  RedMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_RED_MASK);
+  Info->PixelInformation.RedMask = RedMask;
+
+  GreenMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_GREEN_MASK);
+  Info->PixelInformation.GreenMask = GreenMask;
+
+  BlueMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BLUE_MASK);
+  Info->PixelInformation.BlueMask = BlueMask;
+
+  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ENABLE, 1);
+
+  FBOffset = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_FB_OFFSET);
+  BytesPerLine = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BYTES_PER_LINE);
+  BitsPerPixel = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BITS_PER_PIXEL);
+
+  if (BitsPerPixel == 32) {
+    if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
+      Info->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
+    } else if (BlueMask == 0xff0000 && GreenMask == 0xff00 && RedMask == 0xff) {
+      Info->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
+    }
+  }
+
+  Info->PixelInformation.ReservedMask = ((0x2u << (BitsPerPixel - 1u)) - 1u)
+                                         & ~(RedMask | GreenMask | BlueMask);
+  Info->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
+
+  Private->PciIo->GetBarAttributes (
+                        Private->PciIo,
+                        PCI_BAR_IDX1,
+                        NULL,
+                        (VOID**) &FrameBufDesc
+                        );
+
+  Mode->FrameBufferBase = FrameBufDesc->AddrRangeMin + FBOffset;
+  Mode->FrameBufferSize = BytesPerLine * Info->VerticalResolution;
+
+  FreePool (FrameBufDesc);
+  return EFI_SUCCESS;
+}
+
+
 //
 // Graphics Output Protocol Member Functions
 //
@@ -129,6 +187,10 @@ Routine Description:
   (*Info)->VerticalResolution   = ModeData->VerticalResolution;
   QemuVideoCompleteModeInfo (ModeData, *Info);
 
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) {
+    (*Info)->PixelsPerScanLine = ModeData->PixelsPerLine;
+  }
+
   return EFI_SUCCESS;
 }
 
@@ -176,6 +238,12 @@ Routine Description:
   case QEMU_VIDEO_BOCHS:
     InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]);
     break;
+  case QEMU_VIDEO_VMWARE_SVGA2:
+    InitializeVMWSVGA2GraphicsMode (
+      Private,
+      &QemuVideoBochsModes[ModeData->InternalModeIndex]
+      );
+    break;
   default:
     ASSERT (FALSE);
     return EFI_DEVICE_ERROR;
@@ -186,7 +254,11 @@ Routine Description:
   This->Mode->Info->VerticalResolution = ModeData->VerticalResolution;
   This->Mode->SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
 
-  QemuVideoCompleteModeData (Private, This->Mode);
+  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) {
+    QemuVMSVGA2VideoCompleteModeData(Private, This->Mode);
+  } else {
+    QemuVideoCompleteModeData (Private, This->Mode);
+  }
 
   //
   // Re-initialize the frame buffer configure when mode changes.
diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
index d5d8cfef9661..8a6db566ab05 100644
--- a/OvmfPkg/QemuVideoDxe/Initialize.c
+++ b/OvmfPkg/QemuVideoDxe/Initialize.c
@@ -14,6 +14,8 @@
 **/
 
 #include "Qemu.h"
+#include "UnalignedIoInternal.h"
+#include <IndustryStandard/VMWareSVGA2.h>
 
 
 ///
@@ -346,3 +348,108 @@ QemuVideoBochsModeSetup (
   return EFI_SUCCESS;
 }
 
+VOID
+QemuVideoVMWSVGA2RegisterWrite (
+  QEMU_VIDEO_PRIVATE_DATA   *Private,
+  UINT16                    reg,
+  UINT32                    value
+  )
+{
+  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_INDEX_PORT, reg);
+  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_VALUE_PORT, value);
+}
+
+UINT32
+QemuVideoVMWSVGA2RegisterRead (
+  QEMU_VIDEO_PRIVATE_DATA   *Private,
+  UINT16                    reg
+  )
+{
+  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_INDEX_PORT, reg);
+  return UnalignedIoRead32 (Private->VMWareSVGA2_BasePort + SVGA_VALUE_PORT);
+}
+
+EFI_STATUS
+QemuVideoVmwareModeSetup (
+  QEMU_VIDEO_PRIVATE_DATA *Private
+  )
+{
+  UINT32                 FBSize;
+  UINT32                 MaxWidth, MaxHeight;
+  UINT32                 Capabilities;
+  UINT32                 HostBitsPerPixel;
+  UINT32                 Index;
+  QEMU_VIDEO_MODE_DATA   *ModeData;
+  QEMU_VIDEO_BOCHS_MODES *VideoMode;
+
+  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ENABLE, 0);
+
+  Private->ModeData =
+    AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT);
+  if (Private->ModeData == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  FBSize =       QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_FB_SIZE);
+  MaxWidth =     QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_MAX_WIDTH);
+  MaxHeight =    QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_MAX_HEIGHT);
+  Capabilities = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_CAPABILITIES);
+  if ((Capabilities & SVGA_CAP_8BIT_EMULATION) != 0) {
+    HostBitsPerPixel = QemuVideoVMWSVGA2RegisterRead(
+                         Private,
+                         SVGA_REG_HOST_BITS_PER_PIXEL
+                         );
+    QemuVideoVMWSVGA2RegisterWrite (
+      Private,
+      SVGA_REG_BITS_PER_PIXEL,
+      HostBitsPerPixel
+      );
+  } else {
+    HostBitsPerPixel = QemuVideoVMWSVGA2RegisterRead(
+                         Private,
+                         SVGA_REG_BITS_PER_PIXEL
+                         );
+  }
+
+  ModeData = Private->ModeData;
+  VideoMode = &QemuVideoBochsModes[0];
+  for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {
+    UINTN RequiredFbSize;
+
+    ASSERT (HostBitsPerPixel % 8 == 0);
+    RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height *
+                     (HostBitsPerPixel / 8);
+    if (RequiredFbSize <= FBSize
+     && VideoMode->Width <= MaxWidth
+     && VideoMode->Height <= MaxHeight)
+    {
+      UINT32 BytesPerLine;
+
+      QemuVideoVMWSVGA2RegisterWrite (
+        Private,
+        SVGA_REG_WIDTH,
+        VideoMode->Width
+        );
+      QemuVideoVMWSVGA2RegisterWrite (
+        Private,
+        SVGA_REG_HEIGHT,
+        VideoMode->Height
+        );
+      BytesPerLine = QemuVideoVMWSVGA2RegisterRead (
+                       Private,
+                       SVGA_REG_BYTES_PER_LINE
+                       );
+      ModeData->PixelsPerLine = BytesPerLine / (HostBitsPerPixel / 8);
+
+      ModeData->InternalModeIndex    = Index;
+      ModeData->HorizontalResolution = VideoMode->Width;
+      ModeData->VerticalResolution   = VideoMode->Height;
+      ModeData->ColorDepth           = HostBitsPerPixel;
+
+      ModeData ++;
+    }
+    VideoMode ++;
+  }
+  Private->MaxMode = ModeData - Private->ModeData;
+  return EFI_SUCCESS;
+}
-- 
2.3.2 (Apple Git-55)

Re: [PATCH v2 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA II device support.

[Laszlo Ersek]

On 04/03/17 00:44, 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 SVGA2 display device. Drivers for this
> device exist for some guest OSes which do not support Qemu's other
> display adapters, so supporting it in OVMF is useful in conjunction
> with those OSes.
> 
> This change adds support for the SVGA2 device's framebuffer to
> QemuVideoDxe's graphics output protocol implementation, based on
> VMWare's documentation. The most basic initialisation, framebuffer
> layout query, and mode setting operations are implemented.
> 
> The device relies on port-based 32-bit I/O, unfortunately on misaligned
> addresses. This limits the driver's support to the x86 family of
> platforms.
> 
> Cc: Jordan Justen <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.
> 
>  OvmfPkg/QemuVideoDxe/Qemu.h       |  37 +++++++
>  OvmfPkg/QemuVideoDxe/Driver.c     |  76 ++++++++++++++
>  OvmfPkg/QemuVideoDxe/Gop.c        |  74 +++++++++++++-
>  OvmfPkg/QemuVideoDxe/Initialize.c | 107 ++++++++++++++++++++
>  4 files changed, 293 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
> index 2ce37defc5b8..b5c84c9e695e 100644
> --- a/OvmfPkg/QemuVideoDxe/Qemu.h
> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h
> @@ -56,6 +56,10 @@ typedef struct {
>    UINT32  HorizontalResolution;
>    UINT32  VerticalResolution;
>    UINT32  ColorDepth;
> +  //
> +  // VMWare specific:
> +  //
> +  UINT32  PixelsPerLine; // includes any dead space

I suggest to call this VmwPixelsPerLine.

>  } QEMU_VIDEO_MODE_DATA;
>  
>  #define PIXEL_RED_SHIFT   0
> @@ -92,6 +96,7 @@ typedef enum {
>    QEMU_VIDEO_CIRRUS_5446,
>    QEMU_VIDEO_BOCHS,
>    QEMU_VIDEO_BOCHS_MMIO,
> +  QEMU_VIDEO_VMWARE_SVGA2,

QEMU_VIDEO_VMW_SVGA? In the IndustryStandard header file, we just use
VMW_SVGA.

>  } QEMU_VIDEO_VARIANT;
>  
>  typedef struct {
> @@ -119,6 +124,8 @@ typedef struct {
>    QEMU_VIDEO_VARIANT                    Variant;
>    FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
>    UINTN                                 FrameBufferBltConfigureSize;
> +
> +  UINT16                                VMWareSVGA2_BasePort;

I suggest to drop the empty line, and to call this VmwSvgaBasePort.

>  } QEMU_VIDEO_PRIVATE_DATA;
>  
>  ///
> @@ -502,9 +509,39 @@ QemuVideoBochsModeSetup (
>    BOOLEAN                  IsQxl
>    );
>  
> +EFI_STATUS
> +QemuVideoVmwareModeSetup (
> +  QEMU_VIDEO_PRIVATE_DATA *Private
> +  );

Call this QemuVideoVmwSvgaModeSetup?

> +
>  VOID
>  InstallVbeShim (
>    IN CONST CHAR16         *CardName,
>    IN EFI_PHYSICAL_ADDRESS FrameBufferBase
>    );
> +
> +VOID
> +QemuVideoVMWSVGA2RegisterWrite (
> +  QEMU_VIDEO_PRIVATE_DATA *Private,
> +  UINT16                  reg,
> +  UINT32                  value
> +  );

VmwSvgaWrite?

> +
> +UINT32
> +QemuVideoVMWSVGA2RegisterRead (
> +  QEMU_VIDEO_PRIVATE_DATA *Private,
> +  UINT16                  reg
> +  );

VmwSvgaRead?

> +
> +EFI_STATUS
> +QemuVideoVMWSVGA2CompleteModeData (
> +  IN  QEMU_VIDEO_PRIVATE_DATA           *Private,
> +  OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
> +  );

QemuVideoVmwSvgaCompleteModeData?

> +
> +void InitializeVMWSVGA2GraphicsMode (
> +  QEMU_VIDEO_PRIVATE_DATA  *Private,
> +  QEMU_VIDEO_BOCHS_MODES   *ModeData
> +  );
> +

InitializeVmwSvga2GraphicsMode?

>  #endif
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index fc8025ec46de..f01aec6f7b0e 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -15,6 +15,7 @@
>  **/
>  
>  #include "Qemu.h"
> +#include <IndustryStandard/VMWareSVGA2.h>

Aha! So we did use "2" in the header file name. Haven't noticed that
until this point. I suggest we stay consistent in all spots, so either
please drop the "2" from the header file name in patch #1 (and the
include guard macro too), or else please use it everywhere.

Also, we should put <> includes before "" includes. (I guess some of the
current code doesn't conform to that, just below...)

>  #include <IndustryStandard/Acpi.h>
>  
>  EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = {
> @@ -58,6 +59,16 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
>          QEMU_VIDEO_BOCHS_MMIO,
>          L"QEMU VirtIO VGA"
>      },{
> +#if defined MDE_CPU_IA32 || defined MDE_CPU_X64
> +        //
> +        // Support only platforms which can do unaligned port I/O
> +        //
> +        PCI_VENDOR_ID_VMWARE,
> +        PCI_DEVICE_ID_VMWARE_SVGA2,
> +        QEMU_VIDEO_VMWARE_SVGA2,
> +        L"QEMU VMWare SVGA2"
> +    },{
> +#endif

Can you please drop the #if?

>          0 /* end of list */
>      }
>  };
> @@ -317,6 +328,45 @@ QemuVideoControllerDriverStart (
>    }
>  
>    //
> +  // Check if accessing VMWARE_SVGA2 interface works
> +  //
> +  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) {
> +    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *iodesc;

Please call this IoDesc.

> +    UINT32 TargetId;
> +    UINT32 Svga2IDRead;

Please call this Svga2IdRead.

Also, please aligh these two variable names with IoDesc horizontally.

> +
> +    Private->PciIo->GetBarAttributes (
> +                      Private->PciIo,
> +                      PCI_BAR_IDX0,
> +                      NULL,
> +                      (VOID**) &iodesc
> +                      );

Please check the return status of this protocol member call.

If
- the call fails, or
- IoDesc->ResType differs from ACPI_ADDRESS_SPACE_TYPE_IO, or
- IoDesc->AddrRangeMin is larger than or equal to MAX_UINT16,
then set Status to EFI_DEVICE_ERROR, and jump to RestoreAttributes.

> +    Private->VMWareSVGA2_BasePort = iodesc->AddrRangeMin;

I think you should do an explicit cast here, to UINT16; the AddrRangeMin
field has type UINT64, and VS will whine if you don't cast the RHS of
the assignment explicitly.

> +
> +    TargetId = SVGA_ID_2;
> +    while (1) {

Please use "while (TRUE)" or "for (;;)".

"while (1)" is not unused in edk2, but the two other forms are more
idiomatic.

> +      QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ID, TargetId);
> +      Svga2IDRead = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_ID);
> +      if ((Svga2IDRead == TargetId) || (TargetId <= SVGA_ID_0)) {
> +        break;
> +      }
> +      --TargetId;
> +    }
> +
> +    if (Svga2IDRead != TargetId) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "QemuVideo: QEMU_VIDEO_VMWARE_SVGA2 ID mismatch "
> +        "(got 0x%x, base address 0x%x)\n",
> +        Svga2IDRead,
> +        Private->VMWareSVGA2_BasePort
> +        ));
> +      Status = EFI_DEVICE_ERROR;
> +      goto RestoreAttributes;
> +    }
> +  }
> +
> +  //
>    // Get ParentDevicePath
>    //
>    Status = gBS->HandleProtocol (
> @@ -371,6 +421,9 @@ QemuVideoControllerDriverStart (
>    case QEMU_VIDEO_BOCHS:
>      Status = QemuVideoBochsModeSetup (Private, IsQxl);
>      break;
> +  case QEMU_VIDEO_VMWARE_SVGA2:
> +    Status = QemuVideoVmwareModeSetup (Private);
> +    break;
>    default:
>      ASSERT (FALSE);
>      Status = EFI_DEVICE_ERROR;
> @@ -975,3 +1028,26 @@ InitializeQemuVideo (
>  
>    return Status;
>  }
> +
> +void InitializeVMWSVGA2GraphicsMode (

Can you please move this function higher up, so that it follows
InitializeBochsGraphicsMode() directly?


Also, it should start like:

VOID
InitializeVMWSVGA2GraphicsMode (
  ...

Please update the function declaration in "Qemu.h" as well.

> +  QEMU_VIDEO_PRIVATE_DATA   *Private,
> +  QEMU_VIDEO_BOCHS_MODES    *ModeData
> +  )
> +{
> +  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_WIDTH, ModeData->Width);
> +  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_HEIGHT, ModeData->Height);
> +
> +  UINT32 capabilities = QemuVideoVMWSVGA2RegisterRead (
> +                          Private,
> +                          SVGA_REG_CAPABILITIES
> +                          );

Please
- call this variable Capabilities,
- move its definition to the top of the containing scope,
- use a separate assignment -- initialization of automatic storage
duration variables ("locals") is forbidden in edk2, even if the
initializer were a constant.

> +  if ((capabilities & SVGA_CAP_8BIT_EMULATION) != 0) {
> +    QemuVideoVMWSVGA2RegisterWrite(
> +      Private,
> +      SVGA_REG_BITS_PER_PIXEL,
> +      ModeData->ColorDepth
> +      );
> +  }
> +  SetDefaultPalette (Private);
> +  ClearScreen (Private);
> +}

I don't understand how ClearScreen() can work here. ClearScreen() writes
MMIO BAR 0. But, on this video card, BAR 0 is expected to be an IO BAR,
which contains the register selector and register value ports.

> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> index 359e9217d3d1..59918676e764 100644
> --- a/OvmfPkg/QemuVideoDxe/Gop.c
> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> @@ -14,6 +14,7 @@
>  **/
>  
>  #include "Qemu.h"
> +#include <IndustryStandard/VMWareSVGA2.h>

<> should go before ""

>  
>  STATIC
>  VOID
> @@ -76,6 +77,63 @@ QemuVideoCompleteModeData (
>  }
>  
>  
> +EFI_STATUS
> +QemuVMSVGA2VideoCompleteModeData (
> +  IN  QEMU_VIDEO_PRIVATE_DATA           *Private,
> +  OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
> +  )
> +{
> +  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *Info;
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR     *FrameBufDesc;
> +  UINT32                                RedMask, GreenMask, BlueMask;
> +  UINT32                                BitsPerPixel, BytesPerLine, FBOffset;
> +
> +  Info = Mode->Info;
> +  Info->Version = 0;
> +  Info->PixelFormat = PixelBitMask;
> +
> +  RedMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_RED_MASK);
> +  Info->PixelInformation.RedMask = RedMask;
> +
> +  GreenMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_GREEN_MASK);
> +  Info->PixelInformation.GreenMask = GreenMask;
> +
> +  BlueMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BLUE_MASK);
> +  Info->PixelInformation.BlueMask = BlueMask;
> +
> +  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ENABLE, 1);

I don't think this belongs here. Can you put it in
InitializeVMWSVGA2GraphicsMode() instead?

> +
> +  FBOffset = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_FB_OFFSET);
> +  BytesPerLine = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BYTES_PER_LINE);
> +  BitsPerPixel = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BITS_PER_PIXEL);
> +
> +  if (BitsPerPixel == 32) {
> +    if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
> +      Info->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
> +    } else if (BlueMask == 0xff0000 && GreenMask == 0xff00 && RedMask == 0xff) {
> +      Info->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
> +    }
> +  }
> +
> +  Info->PixelInformation.ReservedMask = ((0x2u << (BitsPerPixel - 1u)) - 1u)
> +                                         & ~(RedMask | GreenMask | BlueMask);

I have no idea what this does. Please add a comment that explains it in
detail.

> +  Info->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
> +
> +  Private->PciIo->GetBarAttributes (
> +                        Private->PciIo,
> +                        PCI_BAR_IDX1,
> +                        NULL,
> +                        (VOID**) &FrameBufDesc
> +                        );

So, indeed the video memory is in BAR1. ClearScreen() doesn't match
that. You might want to introduce another field in
QEMU_VIDEO_PRIVATE_DATA, to carry the BAR number that corresponds to the
video memory.

> +
> +  Mode->FrameBufferBase = FrameBufDesc->AddrRangeMin + FBOffset;
> +  Mode->FrameBufferSize = BytesPerLine * Info->VerticalResolution;
> +
> +  FreePool (FrameBufDesc);
> +  return EFI_SUCCESS;
> +}
> +
> +
>  //
>  // Graphics Output Protocol Member Functions
>  //
> @@ -129,6 +187,10 @@ Routine Description:
>    (*Info)->VerticalResolution   = ModeData->VerticalResolution;
>    QemuVideoCompleteModeInfo (ModeData, *Info);
>  
> +  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) {
> +    (*Info)->PixelsPerScanLine = ModeData->PixelsPerLine;
> +  }
> +
>    return EFI_SUCCESS;
>  }

This doesn't seem right. QemuVideoCompleteModeInfo() is called on two paths:

  QemuVideoGraphicsOutputSetMode()
    //
    // for non-vmware
    //
    QemuVideoCompleteModeData()
      QemuVideoCompleteModeInfo()

and

  QemuVideoGraphicsOutputQueryMode()
    QemuVideoCompleteModeInfo()

The first call path is for the mode being *selected*, the second call
path is for querying *any* mode.

Now, on the first call path, you found QemuVideoCompleteModeInfo()
inappropriate for vmw, because you introduced the following in its place:

  QemuVideoGraphicsOutputSetMode()
    //
    // for vmw
    //
    QemuVMSVGA2VideoCompleteModeData()
      //
      // manual setting of pixel mask
      // no call to QemuVideoCompleteModeInfo()
      //

But in that case, I don't see how calling QemuVideoCompleteModeInfo() in
QemuVideoGraphicsOutputQueryMode() -- that is, on the second path --,
and only overriding PixelsPerScanLine, could be correct for vmw. The
pixel mask values filled in by QemuVideoCompleteModeInfo() will be
incorrect.

The mode information should be populated uniformly on both code paths,
regardless of whether we are selecting a mode for actual use (==
changing hardware state), or we are just querying some random mode (==
not changing hardware state).

Are you saying that the pixel mask values cannot be fetched & returned
without actually selecting those graphics modes?

In that case, I think you should pre-compute the pixel mask values in
QemuVideoVmwareModeSetup(), where you already iterate through all the
modes. (I.e., similarly to PixelsPerLine.)

Then both the pixel format setting and the PixelsPerScanLine setting,
for vmw, should be extracted to a function similar to
QemuVideoCompleteModeInfo(). That new function should be called on both
paths above; that is, from QemuVMSVGA2VideoCompleteModeData(), and also
from QemuVideoGraphicsOutputQueryMode(). This new function should
populate Info from the precomputed, cached values, including all the
pixel format fields and the PixelsPerScanLine field.

(Even with PixelsPerScanLine you have an inconsistency now: although in
QueryMode, you use the PixelsPerLine value precomputed in
QemuVideoVmwareModeSetup(), in QemuVMSVGA2VideoCompleteModeData(), you
still read the SVGA_REG_BYTES_PER_LINE register directly again. I think
that shouldn't be necessary.)

>  
> @@ -176,6 +238,12 @@ Routine Description:
>    case QEMU_VIDEO_BOCHS:
>      InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]);
>      break;
> +  case QEMU_VIDEO_VMWARE_SVGA2:
> +    InitializeVMWSVGA2GraphicsMode (
> +      Private,
> +      &QemuVideoBochsModes[ModeData->InternalModeIndex]
> +      );
> +    break;
>    default:
>      ASSERT (FALSE);
>      return EFI_DEVICE_ERROR;
> @@ -186,7 +254,11 @@ Routine Description:
>    This->Mode->Info->VerticalResolution = ModeData->VerticalResolution;
>    This->Mode->SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
>  
> -  QemuVideoCompleteModeData (Private, This->Mode);
> +  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) {
> +    QemuVMSVGA2VideoCompleteModeData(Private, This->Mode);
> +  } else {
> +    QemuVideoCompleteModeData (Private, This->Mode);
> +  }
>  
>    //
>    // Re-initialize the frame buffer configure when mode changes.

> diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
> index d5d8cfef9661..8a6db566ab05 100644
> --- a/OvmfPkg/QemuVideoDxe/Initialize.c
> +++ b/OvmfPkg/QemuVideoDxe/Initialize.c
> @@ -14,6 +14,8 @@
>  **/
>  
>  #include "Qemu.h"
> +#include "UnalignedIoInternal.h"
> +#include <IndustryStandard/VMWareSVGA2.h>

<> includes should go before "" includes.

>  
>  
>  ///
> @@ -346,3 +348,108 @@ QemuVideoBochsModeSetup (
>    return EFI_SUCCESS;
>  }
>  
> +VOID
> +QemuVideoVMWSVGA2RegisterWrite (
> +  QEMU_VIDEO_PRIVATE_DATA   *Private,
> +  UINT16                    reg,
> +  UINT32                    value
> +  )
> +{
> +  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_INDEX_PORT, reg);
> +  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_VALUE_PORT, value);
> +}
> +
> +UINT32
> +QemuVideoVMWSVGA2RegisterRead (
> +  QEMU_VIDEO_PRIVATE_DATA   *Private,
> +  UINT16                    reg
> +  )
> +{
> +  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_INDEX_PORT, reg);
> +  return UnalignedIoRead32 (Private->VMWareSVGA2_BasePort + SVGA_VALUE_PORT);
> +}

Please call "reg" and "value" Register and Value. (Don't forget to
update "Qemu.h" too.)

Also, I think these two functions should be moved to "Driver.c", just
under BochsWrite() / BochsRead().

> +
> +EFI_STATUS
> +QemuVideoVmwareModeSetup (
> +  QEMU_VIDEO_PRIVATE_DATA *Private
> +  )
> +{
> +  UINT32                 FBSize;

Please call this FbSize.

> +  UINT32                 MaxWidth, MaxHeight;
> +  UINT32                 Capabilities;
> +  UINT32                 HostBitsPerPixel;
> +  UINT32                 Index;
> +  QEMU_VIDEO_MODE_DATA   *ModeData;
> +  QEMU_VIDEO_BOCHS_MODES *VideoMode;
> +
> +  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ENABLE, 0);
> +
> +  Private->ModeData =
> +    AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT);
> +  if (Private->ModeData == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  FBSize =       QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_FB_SIZE);
> +  MaxWidth =     QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_MAX_WIDTH);
> +  MaxHeight =    QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_MAX_HEIGHT);
> +  Capabilities = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_CAPABILITIES);
> +  if ((Capabilities & SVGA_CAP_8BIT_EMULATION) != 0) {
> +    HostBitsPerPixel = QemuVideoVMWSVGA2RegisterRead(

Missing space right after "QemuVideoVMWSVGA2RegisterRead".

> +                         Private,
> +                         SVGA_REG_HOST_BITS_PER_PIXEL
> +                         );
> +    QemuVideoVMWSVGA2RegisterWrite (
> +      Private,
> +      SVGA_REG_BITS_PER_PIXEL,
> +      HostBitsPerPixel
> +      );
> +  } else {
> +    HostBitsPerPixel = QemuVideoVMWSVGA2RegisterRead(

Missing space right after "QemuVideoVMWSVGA2RegisterRead".

> +                         Private,
> +                         SVGA_REG_BITS_PER_PIXEL
> +                         );
> +  }
> +
> +  ModeData = Private->ModeData;
> +  VideoMode = &QemuVideoBochsModes[0];
> +  for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {
> +    UINTN RequiredFbSize;
> +
> +    ASSERT (HostBitsPerPixel % 8 == 0);
> +    RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height *
> +                     (HostBitsPerPixel / 8);
> +    if (RequiredFbSize <= FBSize
> +     && VideoMode->Width <= MaxWidth
> +     && VideoMode->Height <= MaxHeight)
> +    {

This should be:

  if (Expression1 &&
      Expression2 &&
      Expression3) {
    //
    // dependent code
    //
  }


> +      UINT32 BytesPerLine;
> +
> +      QemuVideoVMWSVGA2RegisterWrite (
> +        Private,
> +        SVGA_REG_WIDTH,
> +        VideoMode->Width
> +        );
> +      QemuVideoVMWSVGA2RegisterWrite (
> +        Private,
> +        SVGA_REG_HEIGHT,
> +        VideoMode->Height
> +        );
> +      BytesPerLine = QemuVideoVMWSVGA2RegisterRead (
> +                       Private,
> +                       SVGA_REG_BYTES_PER_LINE
> +                       );
> +      ModeData->PixelsPerLine = BytesPerLine / (HostBitsPerPixel / 8);
> +
> +      ModeData->InternalModeIndex    = Index;
> +      ModeData->HorizontalResolution = VideoMode->Width;
> +      ModeData->VerticalResolution   = VideoMode->Height;
> +      ModeData->ColorDepth           = HostBitsPerPixel;
> +
> +      ModeData ++;
> +    }
> +    VideoMode ++;
> +  }
> +  Private->MaxMode = ModeData - Private->ModeData;
> +  return EFI_SUCCESS;
> +}
> 

Thanks
Laszlo

Re: [PATCH v2 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA2 framebuffer support

[Phil Dennis-Jordan]

Looks like I missed off the branch link, sorry:

https://github.com/pmj/edk2/commits/ovmf_vmware_svga2_v2

On Mon, Apr 3, 2017 at 10:44 AM, Phil Dennis-Jordan <lists@philjordan.eu> wrote:
> This extends the QemuVideoDxe driver to support the VMWare SVGA2 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.
>
> 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.
>
> Phil Dennis-Jordan (3):
>   OvmfPkg: VMWare SVGA2 display device register definitions
>   OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O.
>   OvmfPkg/QemuVideoDxe: VMWare SVGA II device support.
>
>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf          |   6 ++
>  OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h | 102 +++++++++++++++++++
>  OvmfPkg/QemuVideoDxe/Qemu.h                    |  37 +++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h     |  59 +++++++++++
>  OvmfPkg/QemuVideoDxe/Driver.c                  |  76 ++++++++++++++
>  OvmfPkg/QemuVideoDxe/Gop.c                     |  74 +++++++++++++-
>  OvmfPkg/QemuVideoDxe/Initialize.c              | 107 ++++++++++++++++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c          |  69 +++++++++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c          |  79 +++++++++++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c          |  81 +++++++++++++++
>  OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c  |  65 ++++++++++++
>  11 files changed, 754 insertions(+), 1 deletion(-)
>  create mode 100644 OvmfPkg/Include/IndustryStandard/VMWareSVGA2.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)
>