From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-x244.google.com (mail-vk0-x244.google.com [IPv6:2607:f8b0:400c:c05::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C2D1021DFA7BA for ; Thu, 30 Mar 2017 03:51:25 -0700 (PDT) Received: by mail-vk0-x244.google.com with SMTP id z204so6294135vkd.2 for ; Thu, 30 Mar 2017 03:51:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philjordan-eu.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=KSklyUgdW7xjvjptAjmIPZ+Y4YCKNQu0hS9060x0ppA=; b=FDhQR92sxO1luiyUB8rBZlmW5OrrPOZ8OXyxhLQ6UZzCOJ7A+be+9OuEvK7JZI/FAJ jaAkp7tx56MpCpdTYdM4K5SxqNqaLg0R99pt3FpMwY9tHbj63TPcOdj89wpF3IG6jsUW 9L0x6wx2zfDaqfewCUe6D9K1CzX74OfMtEgucyTm5lOXnrtPR/agTf34f9fyvahBM9NN IU/3ZPH4gmL5+DbNXMmaj2QOe2MV0L+UCu5CJ/cg1gh9VyvKGXjF0HGaSqAEfDvTh2Ew 6qPhEYbpvHXNNnG80Pe4EVCzry6duExTnbOxNZSfRDAGjL02vfns+ZT/uokHb8tpHBci bo8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=KSklyUgdW7xjvjptAjmIPZ+Y4YCKNQu0hS9060x0ppA=; b=nJOUkcX1TsQQm2N9dKBIssiHnlwlY61k6ixrPKQkm1cpr/JTioeQyJ+99FKOHa988v K4zdgGLXNMG+DnywgLRL4iA6ImoldjHz31IKBeCG+mjosQeLVzsZYMKJ7fCw8DaBjx3e nkDXavw0u8niFaonj6Qu1Yh7//J/Zu54EzczXWdMGGJnRcZw1ODbAZOfTLBduENHs5nV ezPgF9EVgpGQTMurmsYrCDY8hFg13UEFzVldOssIVs5zfhV/Om2KD38KeWsT+EGLHJxp zixeIv3QYGPPeyrIV1fjAQXLTrSwYQgVPQpyWXFkL6UoHDx2o0sNjT4xQn82/qi1CNnE xECg== X-Gm-Message-State: AFeK/H0G0hI1Z4gU4cPFP9RMdClW74v2l+dFC4WkrF9y+pkVbT2TiqKu1RaGJEBeEMAx12bwO3erqIv/srXsuw== X-Received: by 10.31.14.140 with SMTP id 134mr2511664vko.176.1490871084724; Thu, 30 Mar 2017 03:51:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.31.58.83 with HTTP; Thu, 30 Mar 2017 03:51:03 -0700 (PDT) In-Reply-To: <2afb6198-7432-49e4-f2ec-b4e58b341bcd@redhat.com> References: <1490775597-24007-1-git-send-email-lists@philjordan.eu> <2afb6198-7432-49e4-f2ec-b4e58b341bcd@redhat.com> From: Phil Dennis-Jordan Date: Thu, 30 Mar 2017 23:51:03 +1300 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 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 10:51:26 -0000 Content-Type: text/plain; charset=UTF-8 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. 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 >> >