From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22d.google.com (mail-io0-x22d.google.com [IPv6:2607:f8b0:4001:c06::22d]) (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 7D77721A04811 for ; Thu, 6 Apr 2017 02:43:58 -0700 (PDT) Received: by mail-io0-x22d.google.com with SMTP id l7so25982862ioe.3 for ; Thu, 06 Apr 2017 02:43:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=lRjgsHf1hIdMoSpFDXP7vj+qRuvYul9LgFHC5QPDwwA=; b=ZjgXCCDyce4Na4RyKxgom/PcByX5Tcexf7crvihOUfrBb1MoE7M4toGSsINPFL29qv Hh1ehGG/y8f6zokpS2QQPXjGslxUpQsuGKgwuJwOSoYVVsGN6j9P3oos1GJfxGorkqye 064j36OqqF2SPrv+m2GwlViFErBLrL0SRw5so= 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=lRjgsHf1hIdMoSpFDXP7vj+qRuvYul9LgFHC5QPDwwA=; b=AJBBb4GhkccK3sO2QgtAqlk0ZuC3LECTf94at/2k2n+HMwGWGThjXsW5oH51gpjOli 08E8DjjiH2pvBZJoOVDBMvkWoli3v0R8I3k3nol9BhGJDi8FK5fKNnFQnm12ife3ZnLb dJeViUgZD/BB/wTpsfO99QyIfwV4CWYg75TWC3DxOKZg4RwYoj1uSxkJgeTMXus4qO8u uIbyBMDJVQmCOTW8smMBDlnO34cte6vIOKUhJiQc6gdvB/G0os5Er/KI+aZut0EoPZDW jHAVwEdx1KtEXINKYCVd4jt1z2+jRyR/CSHr0FteJBx26+TSx2U98b9Bjt/PcYpEeCKF EfIA== X-Gm-Message-State: AFeK/H37wT21gK74cKGC/qhRD9hFi1V/N521WBPkAx4lXqbyKsIcucjsTBCWO94VfhtDdTGgW/F5ckX2ZDHuuVAT X-Received: by 10.107.168.21 with SMTP id r21mr30074430ioe.45.1491471837809; Thu, 06 Apr 2017 02:43:57 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Thu, 6 Apr 2017 02:43:57 -0700 (PDT) In-Reply-To: <20170406093547.GR25239@bivouac.eciton.net> References: <1473429644-13480-1-git-send-email-ard.biesheuvel@linaro.org> <1473429644-13480-5-git-send-email-ard.biesheuvel@linaro.org> <8871a794-80f8-049f-5abc-ca1d4a8fb3a3@arm.com> <20170406093547.GR25239@bivouac.eciton.net> From: Ard Biesheuvel Date: Thu, 6 Apr 2017 10:43:57 +0100 Message-ID: To: Leif Lindholm Cc: Jeremy Linton , "edk2-devel@lists.01.org" , "Gao, Liming" , "Kinney, Michael D" , Charles Garcia-Tobin , Dong Wei , Evan Lloyd Subject: Re: [PATCH v5 4/4] MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: disallow use in SEC & PEI phases 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, 06 Apr 2017 09:43:58 -0000 Content-Type: text/plain; charset=UTF-8 On 6 April 2017 at 10:35, Leif Lindholm wrote: > On Wed, Apr 05, 2017 at 10:55:49PM +0100, Ard Biesheuvel wrote: >> >>> I think this is a problem because nowhere in the UEFI specs do I see such >> >>> restrictions on those memory operations. >> >> >> >> Using device attributes for memory is something we should ban for >> >> AArch64 in the spec. > > Yes, completely agree. And doing so is generally the result of > misinderstanding the memory model (i.e., it probably won't provide the > guarantee that was sought). > Charles/Dong? Something to add to list? > As an additional note, the UEFI spec mandates that unaligned accesses are enabled for AArch64, which clearly expresses the intent that routines operating on memory should be able to do so without going out of its way to avoid unaligned accesses. > Can we insert a test preventing device memory type to be set for > regions with _WB attribute? Or is that already only possible through > manual trickery? > We should simply remove the _UC attribute from all memory. I have already done so for many of the platforms I more or less maintain (and for virt, we removed _WT and _WC as well, because KVM only supports _WB) Note that this does not prevent the NOR and RTC drivers from creating _UC regions for their own MMIO registers, it just prevents them from being remapped _UC via the DXE services. >> >>> For a specific problematic example, the LcdGraphicsOutputBlt.c uses it >> >>> for >> >>> BltVideoFill() and the target of that is likely not regular cached video >> >>> memory. >> >> >> >> Those drivers should be using EFI_MEMORY_WC not EFI_MEMORY_UC for the >> >> VRAM mapping. Note that EFI_MEMORY_UC is nGnRnE which is unnecessarily >> >> restrictive. >> >> >> >> I agree there is a general issue here which we should address by >> >> tightening the spec. I don't see a lot of value in avoiding DC ZVA and >> >> unaligned accesses altogether, I'd rather fix the code instead. >> > >> > While I agree with the general sentiment, I find the result brittle. If it >> > were used as a DEBUG build way to locate sub-optmimal code I would be more >> > on board. But shipping it like this, puts it into situations where the user >> > inadvertently changes something (say making the background black and >> > therefore triggering the DC) or some obscure option ROM (we will get there >> > right??!!) triggers it in a place where it can't be debugged. >> > >> > Particularly since we are talking boot, where the few percent perf >> > improvement on this operation is likely completely undetectable. The one >> > place where I can think it might even be measurable is in routines to clear >> > system memory, and those routines could be a special case anyway. >> >> I guess this depends on the use case. For server, it may not matter, >> but the case is different for mobile, and the Broadcom engineers that >> did some benchmarks on this code were very pleased with the result >> (and the speedup was significant, although I don't know which routines >> are the hotspots) >> >> As for option ROMs: those will link to their own BaseMemoryLib >> implementation (assuming that they are EDK2 based) so the only way >> they would have access to these routines is via the CopyMem() and >> SetMem() boot services. Note that that does not dismiss the concern at >> all, it is just a clarification. >> >> Leif, any thoughts? > > I would prefer if we could resolve this without waiting for a new spec > version. > > My gut feeling is that the (end-user, I care a _lot_ less > about development platforms) devices that _could_ be affected by this > won't be releasing updated firmwares completely rebased onto a newer > edk2 HEAD. Rather they're likely to be cherry-picking individual > bugfixes and improvements. > > But certainly having some input from abovementioned Broadcom team, > Evan & co, and others is important before we make a call. > > / > Leif