Re: [PATCH] OvmfPkg/QemuVideoDxe: map framebuffer as write-combining/non-executable

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-08-18 11:08:23, Leif Lindholm wrote:
> On Fri, Aug 18, 2017 at 10:49:35AM -0700, Jordan Justen wrote:
> > On 2017-08-18 10:25:14, Ard Biesheuvel wrote:
> > > On 18 August 2017 at 18:20, Jordan Justen <jordan.l.justen@intel.com> wrote:
> > > > On 2017-08-18 06:04:01, Ard Biesheuvel wrote:
> > > >> On 18 August 2017 at 14:02, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > >> > When QemuVideoDxe takes control of the framebuffer, it is already
> > > >> > mapped EFI_MEMORY_UC by core code, and QemuVideoDxe simply records
> > > >> > the base and size from the PCI BAR.
> > > >> >
> > > >> > On x86 systems, this is sufficient, but on ARM systems, the semantics
> > > >> > of EFI_MEMORY_UC regions are quite different from EFI_MEMORY_WC regions,
> > > >> > and treating a region like memory (i.e., dereferencing pointers into it
> > > >> > or using ordinary CopyMem()/SetMem() functions on it) requires that it
> > > >> > be mapped with memory semantics, i.e., EFI_MEMORY_WC, EFI_MEMORY_WT or
> > > >> > EFI_MEMORY_WB.
> > > >> >
> > > >> > Since caching is not appropriate for regions where we rely on side
> > > >> > effects, remap the frame buffer EFI_MEMORY_WT.
> > > >>
> > > >> EFI_MEMORY_WC not WT
> > > >
> > > > If a single pixel is written, then WC may not write it through
> > > > immediately. Would WT be more appropriate?
> > > 
> > > For ARM, that applies equally to WT AFAIK.
> > 
> > Write-through will not actually write-*through*?
> 
> Not immediately, no.

Hmm, looking the SDM, it looks like WT is similar to WC for writes,
but it may service reads from cache, whereas WC will not.

Since I assume reading the FB is somewhat rare, it sounds like WC is
probably better.

> 
> Conversely, does IA32/X64 WC not guarantee writing out within a
> finite period?

I'm not sure, but the SDM does say:

"If the WC buffer is partially filled, the writes may be delayed until
 the next occurrence of a serializing event; such as, an SFENCE or
 MFENCE instruction, CPUID execution, a read or write to uncached
 memory, an interrupt occurrence, or a LOCK instruction execution."

And, even more arguing in favor of your change:

"This type of cache-control is appropriate for video frame buffers,
 where the order of writes is unimportant as long as the writes update
 memory so they can be seen on the graphics display."

So, it sound like there is no strict guarantee, but practically
speaking it will be flushed often enough that a user will never notice
the difference.

Based on this, if you tweak the comment, and test on X64, then:

Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>