On 2017-08-18 06:04:01, Ard Biesheuvel wrote: > On 18 August 2017 at 14:02, Ard Biesheuvel <[email protected]> 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? Did you get a chance to test x86 systems with the change? > > > Given that the ARM > > architecture requires device mappings to be non-executable (to avoid > > inadvertent speculative instruction fetches from device registers), > > retain the non-executable nature by adding the EFI_MEMORY_XP attribute > > as well. > > > > Note that the crashes that this patch aims to prevent can currently only > > occur under KVM, in which case the VGA driver does not operate correctly > > in the first place. However, this is an implementation detail of QEMU > > while running under KVM, and given that the ARM architecture simply does > > not permit unaligned accesses to device memory, it is best to conform > > to this in the code. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Ard Biesheuvel <[email protected]> > > --- > > OvmfPkg/QemuVideoDxe/Driver.c | 5 +++++ > > OvmfPkg/QemuVideoDxe/Gop.c | 18 ++++++++++++++++-- > > OvmfPkg/QemuVideoDxe/Qemu.h | 2 ++ > > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 1 + > > 4 files changed, 24 insertions(+), 2 deletions(-) > > > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > > index 0dce80e59ba8..d81be49d91f3 100644 > > --- a/OvmfPkg/QemuVideoDxe/Driver.c > > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > > @@ -69,6 +69,8 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = { > > } > > }; > > > > +EFI_CPU_ARCH_PROTOCOL *gCpu; > > + > > static QEMU_VIDEO_CARD* > > QemuVideoDetect( > > IN UINT16 VendorId, > > @@ -1103,5 +1105,8 @@ InitializeQemuVideo ( > > ); > > ASSERT_EFI_ERROR (Status); > > > > + Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID > > **)&gCpu); > > + ASSERT_EFI_ERROR(Status); > > + > > return Status; > > } > > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c > > index 512fd27acbda..a820524db293 100644 > > --- a/OvmfPkg/QemuVideoDxe/Gop.c > > +++ b/OvmfPkg/QemuVideoDxe/Gop.c > > @@ -53,7 +53,8 @@ QemuVideoCompleteModeData ( > > { > > EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info; > > EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *FrameBufDesc; > > - QEMU_VIDEO_MODE_DATA *ModeData; > > + QEMU_VIDEO_MODE_DATA *ModeData; > > + EFI_STATUS Status; > > > > ModeData = &Private->ModeData[Mode->Mode]; > > Info = Mode->Info; > > @@ -72,8 +73,21 @@ QemuVideoCompleteModeData ( > > DEBUG ((EFI_D_INFO, "FrameBufferBase: 0x%Lx, FrameBufferSize: 0x%Lx\n", > > Mode->FrameBufferBase, (UINT64)Mode->FrameBufferSize)); > > > > + // > > + // Remap the framebuffer region as write combining. On x86 systems, this > > is > > + // merely a performance optimization, but on ARM systems, it prevents > > + // crashes that may result from unaligned accesses, given that we treat > > the > > + // frame buffer as ordinary memory by using CopyMem()/SetMem() on it. > > While > > + // we're at it, set the non-exec attribute so the framebuffer is not > > + // exploitable by malware. I'm pretty sure Laszlo would disagree, but I think a more terse comment would do fine here. (We always have git blame to get the full story.) // // Set the framebuffer region as write combining (through?) and // non-executable. For ARM the memory range can't be left in the // uncachable state. // > > + // > > + Status = gCpu->SetMemoryAttributes (gCpu, Mode->FrameBufferBase, > > + ALIGN_VALUE (Mode->FrameBufferSize, EFI_PAGE_SIZE), > > + EFI_MEMORY_WC | EFI_MEMORY_XP); > > + ASSERT_EFI_ERROR (Status); > > + > > FreePool (FrameBufDesc); > > - return EFI_SUCCESS; > > + return Status; > > } > > > > STATIC > > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > > index 7fbb25b3efd3..2966c77c78b3 100644 > > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > > @@ -21,6 +21,7 @@ > > > > > > #include <Uefi.h> > > +#include <Protocol/Cpu.h> > > #include <Protocol/GraphicsOutput.h> > > #include <Protocol/PciIo.h> > > #include <Protocol/DriverSupportedEfiVersion.h> > > @@ -164,6 +165,7 @@ extern EFI_DRIVER_BINDING_PROTOCOL > > gQemuVideoDriverBinding; > > extern EFI_COMPONENT_NAME_PROTOCOL gQemuVideoComponentName; > > extern EFI_COMPONENT_NAME2_PROTOCOL gQemuVideoComponentName2; > > extern EFI_DRIVER_SUPPORTED_EFI_VERSION_PROTOCOL > > gQemuVideoDriverSupportedEfiVersion; > > +extern EFI_CPU_ARCH_PROTOCOL *gCpu; > > > > // > > // Io Registers defined by VGA > > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > index 346a5aed94fa..bbe11257c002 100644 > > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > > @@ -68,6 +68,7 @@ [LibraryClasses] > > UefiLib > > > > [Protocols] > > + gEfiCpuArchProtocolGuid # PROTOCOL ALWAYS_CONSUMED I don't think a 'Driver Model' driver needs to add arch protocols into the depex. -Jordan > > gEfiDriverSupportedEfiVersionProtocolGuid # PROTOCOL ALWAYS_PRODUCED > > gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START > > gEfiDevicePathProtocolGuid # PROTOCOL BY_START > > -- > > 2.11.0 > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
