public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Jeremy Linton <jeremy.linton@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>,
	 "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	"Gao, Liming" <liming.gao@intel.com>,
	 "Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [PATCH v5 4/4] MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: disallow use in SEC & PEI phases
Date: Wed, 5 Apr 2017 22:55:49 +0100	[thread overview]
Message-ID: <CAKv+Gu8mQ4Lk_hyWSTfj_-mvGX2399aJKf+=Ygp3-mXWAnTviw@mail.gmail.com> (raw)
In-Reply-To: <8871a794-80f8-049f-5abc-ca1d4a8fb3a3@arm.com>

On 5 April 2017 at 22:28, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Hi,
>
>
> On 04/05/2017 03:34 PM, Ard Biesheuvel wrote:
>>
>> On 5 April 2017 at 21:12, Jeremy Linton <jeremy.linton@arm.com> wrote:
>>>
>>> Hi,
>>>
>>> On 09/09/2016 09:00 AM, Ard Biesheuvel wrote:
>>>>
>>>>
>>>> The new accelerated ARM and AARCH64 implementations take advantage of
>>>> features that are only available when the MMU and Dcache are on. So
>>>> restrict the use of this library to the DXE phase or later.
>>>
>>>
>>>
>>> I don't think this is sufficient because DC ZVA doesn't work against
>>> device
>>> memory/etc. That means that users have to somehow know the page/etc
>>> attributes of memory regions before they call SetMemXX() on them.
>>>
>>
>> Yes. I literally found this out myself yesterday. Note that this
>> applies equally to unaligned accesses.
>>
>>
>>> 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.
>>
>>> 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?


  reply	other threads:[~2017-04-05 21:55 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-09 14:00 [PATCH v5 0/4] MdePkg: add ARM/AARCH64 support to BaseMemoryLib Ard Biesheuvel
2016-09-09 14:00 ` [PATCH v5 1/4] MdePkg/BaseMemoryLib: widen aligned accesses to 32 or 64 bits Ard Biesheuvel
2016-09-09 14:00 ` [PATCH v5 2/4] MdePkg/BaseMemoryLibOptDxe: add accelerated ARM routines Ard Biesheuvel
2016-09-09 14:00 ` [PATCH v5 3/4] MdePkg/BaseMemoryLibOptDxe: add accelerated AARCH64 routines Ard Biesheuvel
2016-09-09 14:00 ` [PATCH v5 4/4] MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: disallow use in SEC & PEI phases Ard Biesheuvel
2016-09-13 14:49   ` Ard Biesheuvel
2016-09-13 15:00     ` Gao, Liming
2017-04-05 20:12   ` Jeremy Linton
2017-04-05 20:34     ` Ard Biesheuvel
2017-04-05 21:28       ` Jeremy Linton
2017-04-05 21:55         ` Ard Biesheuvel [this message]
2017-04-06  9:35           ` Leif Lindholm
2017-04-06  9:43             ` Ard Biesheuvel
2017-04-06 10:16               ` Leif Lindholm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKv+Gu8mQ4Lk_hyWSTfj_-mvGX2399aJKf+=Ygp3-mXWAnTviw@mail.gmail.com' \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox