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 B8E3421DFA8FE for ; Thu, 30 Mar 2017 08:53:34 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 338F99E60F; Thu, 30 Mar 2017 15:53:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 338F99E60F Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 338F99E60F Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-143.phx2.redhat.com [10.3.116.143]) by smtp.corp.redhat.com (Postfix) with ESMTP id 573D1881FA; Thu, 30 Mar 2017 15:53:33 +0000 (UTC) To: Phil Dennis-Jordan References: <1490775597-24007-1-git-send-email-lists@philjordan.eu> <2afb6198-7432-49e4-f2ec-b4e58b341bcd@redhat.com> Cc: edk2-devel-01 From: Laszlo Ersek Message-ID: <66e51b83-7dd6-dbbb-2036-2f38bc9f5bf4@redhat.com> Date: Thu, 30 Mar 2017 17:53:31 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 30 Mar 2017 15:53:34 +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 15:53:34 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/30/17 12:51, Phil Dennis-Jordan wrote: > On Thu, Mar 30, 2017 at 2:33 PM, Laszlo Ersek 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 >>> >>