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

[Laszlo Ersek <lersek@redhat.com>]

On 03/30/17 12:51, Phil Dennis-Jordan wrote:
> 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.

Thank you for that; I secretly wished you would do this. I'm grasping at
straws to catch a break in my review load, and any cleanup in these
driver additions before I get to look at them in a bit more depth will
be highly appreciated.

Thanks
Laszlo


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