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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

'On 18 August 2017 at 18:49, Jordan Justen <jordan.l.justen@intel.com> 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*?
>

Both WC and WT go through a write buffer which may reorder writes and
does not offer any timeliness guarantees AFAIK.

> I'm not sure how well QEMU/KVM models this for x86 or ARM.
>

On ARM, we have coherency issues with framebuffers that are backed by
host memory that is mapped cacheable, because writes via the
uncached/write-through mapping of the guest do not invalidate
cachelines that cover it. Hence the 'the VGA driver does not operate
correctly in the first place' in the commit log. But crashing on an
alignment fault is unfriendly, to say the least, and I would prefer to
adhere to the architectural requirements in any case.

>> >> >
>> >> >  [Protocols]
>> >> > +  gEfiCpuArchProtocolGuid                       # PROTOCOL ALWAYS_CONSUMED
>> >
>> > I don't think a 'Driver Model' driver needs to add arch protocols into
>> > the depex.
>> >
>>
>> To be pedantic: this is not the depex. You can't rely on the protocol
>> header to declare gEfiCpuArchProtocolGuid, and declaring it here makes
>> the build tools add its declaration to AutoGen.h (I think this has to
>> do with the exact .dsc version. Perhaps Laszlo has a better
>> recollection of the details.)
>
> Whoops. You are correct. We want this change...
>
> -Jordan