Re: [RFC PATCH v1 0/2] OvmfPkg/QemuVideoDxe: Add VMWare SVGA2 framebuffer support

[Laszlo Ersek <lersek@redhat.com>]

On 03/29/17 10:19, Phil Dennis-Jordan 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 the minimum of
> additional code. I've marked this patch as RFC for 2 main reasons:
> 
> 1. I've imported VMWare's own header file with device register constants
> etc. verbatim. (patch 1/2) This doesn't follow any of the EDK2 coding
> conventions, and it uses the MIT license, not BSD. Only a small percentage
> of symbols are actually used in the driver. On the other hand, it's
> obviously the authoritative source. I'm not sure what the correct
> etiquette is here, define our own constants, or import the authoritative
> header file?

The MIT license is OK (see "OvmfPkg/Contributions.txt").

I strongly prefer hand-crafted, minimal header files in OvmfPkg. I've
done that for all of the virtio stuff, for example.

At least one counter-example exists as well
("OvmfPkg/Include/IndustryStandard/Xen"), and I dislike that very much.

If hand-crafting the minimal required subset is not too much trouble,
and you don't expect frequent updates (header file syncs) from the
public (MIT-licensed) vmware-svga repo, I suggest that you write a brand
new header file, and place it under OvmfPkg/Include/IndustryStandard. As
reference you should indeed identify the original file (preferabily with
a commit hash / SVN revision identifier into the original repo, at which
the file looked like that). I think this new header file would still
qualify as derivative work, so it should be under MIT, and carry both
the original and your (C) notices. I think.

> 
> 2. 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. It seems
> this issue was already encountered/discussed on the edk2-devel list 4
> years ago: http://edk2-devel.narkive.com/bwH3r0us/unaligned-i-o I couldn't
> find any code resulting from that discussion, and Qemu definitely uses
> unaligned port numbers for the SVGA2 device. (SVGA_IO_MUL is 1 in
> hw/display/vmware_vga.c) It does not appear to make any provision for
> non-x86 architectures, so I assume there's no sensible way to drive the
> device in those cases. The patch therefore only detects the device on x86,
> where it uses UnalignedIoWrite/Read32() helper functions which I've based
> on IoLib's aligned ones. I have only tested the GCC version of these.
> Feel free to suggest a better way of handling the issue.

Right now I can only say very generic things about patch 2:

- The idea to pull in & customize the primitives from IoLib matches
Jordan's idea from 4 years ago, so I think it's sane. Please do that in
a separate patch however.

- In the UnalignedIoRead32() and UnalignedIoWrite32() functions, you
should always return a value, even if you ASSERT(FALSE) first. Those
asserts can be compiled out.

- QemuVideDxe is used by ArmVirtPkg as well (it works OK on x86 TCG --
it's broken on aarch64 KVM); have you build tested the change with that
platform?

More on this later I hope.
Laszlo

> 
> Github feature branch: https://github.com/pmj/edk2/tree/ovmf_vmware_svga2_v1
> 
> Phil Dennis-Jordan (2):
>   OvmfPkg: Add SVGA2 device register definition header from VMWare
>   OvmfPkg: Add VMWare SVGA II support in QemuVideoDxe.
> 
>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf         |    6 +
>  OvmfPkg/QemuVideoDxe/Qemu.h                   |   50 +
>  OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h    |   51 +
>  OvmfPkg/QemuVideoDxe/svga_reg.h               | 1558 ++++++++++++++++++++
>  OvmfPkg/QemuVideoDxe/Driver.c                 |   67 +
>  OvmfPkg/QemuVideoDxe/Gop.c                    |   71 +-
>  OvmfPkg/QemuVideoDxe/Initialize.c             |   88 ++
>  OvmfPkg/QemuVideoDxe/UnalignedIoGcc.c         |   59 +
>  OvmfPkg/QemuVideoDxe/UnalignedIoIcc.c         |   79 +
>  OvmfPkg/QemuVideoDxe/UnalignedIoMsc.c         |   81 +
>  OvmfPkg/QemuVideoDxe/UnalignedIoUnsupported.c |   53 +
>  11 files changed, 2162 insertions(+), 1 deletion(-)
>  create mode 100644 OvmfPkg/QemuVideoDxe/UnalignedIoInternal.h
>  create mode 100644 OvmfPkg/QemuVideoDxe/svga_reg.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
>