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

[Phil Dennis-Jordan <lists@philjordan.eu>]

On Thu, Mar 30, 2017 at 2:33 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.

OK, I will do exactly that. The number of constants/macros used in the
driver is minimal.

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

Sure, makes sense.

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

Thanks for the pointer, I'll check out the ArmVirtPkg tutorial and try
to get a buildchain and testing environment set up for that. aarch64
TCG should hopefully suffice, I don't have access to an ARM system
capable of running KVM.

> More on this later I hope.

I'll fix up these high level issues and submit a new patchset in a few
days, no need to review the driver in depth until I've done that.

Thanks,
Phil


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