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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
