From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 425DF21DFA8E1 for ; Wed, 29 Mar 2017 18:33:48 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B2697369C3; Thu, 30 Mar 2017 01:33:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B2697369C3 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B2697369C3 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-134.phx2.redhat.com [10.3.116.134]) by smtp.corp.redhat.com (Postfix) with ESMTP id D6FAC5B82F; Thu, 30 Mar 2017 01:33:46 +0000 (UTC) To: Phil Dennis-Jordan , edk2-devel@lists.01.org References: <1490775597-24007-1-git-send-email-lists@philjordan.eu> From: Laszlo Ersek Message-ID: <2afb6198-7432-49e4-f2ec-b4e58b341bcd@redhat.com> Date: Thu, 30 Mar 2017 03:33:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1490775597-24007-1-git-send-email-lists@philjordan.eu> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 30 Mar 2017 01:33:47 +0000 (UTC) Subject: Re: [RFC PATCH v1 0/2] OvmfPkg/QemuVideoDxe: Add VMWare SVGA2 framebuffer support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Mar 2017 01:33:48 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 >