From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5F3ED21196225 for ; Mon, 3 Dec 2018 05:32:40 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EE4A14E920; Mon, 3 Dec 2018 13:32:39 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-61.rdu2.redhat.com [10.10.120.61]) by smtp.corp.redhat.com (Postfix) with ESMTP id CB1585C257; Mon, 3 Dec 2018 13:32:38 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: Andrew Jones References: <20181130112829.12173-1-ard.biesheuvel@linaro.org> <20181130112829.12173-5-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Mon, 3 Dec 2018 14:32:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181130112829.12173-5-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 03 Dec 2018 13:32:40 +0000 (UTC) Subject: Re: [PATCH v2 4/4] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Dec 2018 13:32:40 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/30/18 12:28, Ard Biesheuvel wrote: > QEMU/mach-virt is rather unhelpful when it comes to tracking down > NULL pointer dereferences that occur while running in UEFI: since > we have NOR flash mapped at address 0x0, inadvertent reads go > unnoticed, and even most writes are silently dropped, unless you're > unlucky and the instruction in question is one that KVM cannot > emulate, in which case you end up with a QEMU crash like this: > > error: kvm run failed Function not implemented > PC=000000013f7ff804 X00=000000013f7ab108 X01=0000000000000064 > X02=000000013f801988 X03=00000000800003c4 X04=0000000000000000 > X05=0000000096000044 X06=fffffffffffd8270 X07=000000013f7ab4a0 > X08=0000000000000001 X09=000000013f803b88 X10=000000013f7e88d0 > X11=0000000000000009 X12=000000013f7ab554 X13=0000000000000008 > X14=0000000000000002 X15=0000000000000000 X16=0000000000000000 > X17=0000000000000000 X18=0000000000000000 X19=0000000000000000 > X20=000000013f81c000 X21=000000013f7ab170 X22=000000013f81c000 > X23=0000000009000018 X24=000000013f407020 X25=000000013f81c000 > X26=000000013f803530 X27=000000013f802000 X28=000000013f7ab270 > X29=000000013f7ab0d0 X30=000000013f7fee10 SP=000000013f7a6f30 > PSTATE=800003c5 N--- EL1h > > and a warning in the host kernel log that load/store instruction > decoding is not supported by KVM. > > Given that the first page of the flash device is not actually > used anyway, let's reduce the mappings of the peripheral space > and the flash device (both of which cover page #0) to only cover > what is actually required: > > ArmVirtQemu.fdf: > > 0x00001000|0x001ff000 > > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > > ArmVirtQemuKernel.fdf: > > 0x00008000|0x001f8000 > > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > > For ArmVirtQemu, the resulting virtual mapping looks roughly like: > - [0, 4K) : flash, unmapped > - [4K, 2M) : flash, mapped as WB+X RAM > - [2M, 64M) : flash, unmapped > - [64M, 128M) : varstore flash, will be mapped by the NOR flash driver > - [128M, 256M) : peripherals, mapped as device > - [256M, 1GB) : 32-bit MMIO aperture, translated IO aperture, ECAM, > will be mapped by the PCI host bridge driver > - [1GB, ...) : RAM, mapped. > > After this change, any inadvertent read or write from/to the first > physical page will trigger a translation fault inside the guest, > regardless of the nature of the instruction, without crashing QEMU. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf | 4 ++-- > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf | 2 ++ > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 23 ++++++++++++++------ > 3 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > index 5c5b841051ad..b6abc52531a8 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > @@ -39,9 +39,9 @@ [LibraryClasses] > PcdLib > > [Pcd] > - gArmTokenSpaceGuid.PcdFdBaseAddress > + gArmTokenSpaceGuid.PcdFvBaseAddress > gArmTokenSpaceGuid.PcdSystemMemoryBase > gArmTokenSpaceGuid.PcdSystemMemorySize > > [FixedPcd] > - gArmTokenSpaceGuid.PcdFdSize > + gArmTokenSpaceGuid.PcdFvSize > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > index d12089760b22..16802c5c414b 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > @@ -43,9 +43,11 @@ [LibraryClasses] > > [Pcd] > gArmTokenSpaceGuid.PcdFdBaseAddress > + gArmTokenSpaceGuid.PcdFvBaseAddress > gArmTokenSpaceGuid.PcdSystemMemoryBase > gArmTokenSpaceGuid.PcdSystemMemorySize > > [FixedPcd] > gArmTokenSpaceGuid.PcdFdSize > + gArmTokenSpaceGuid.PcdFvSize > gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > index 0285a11b1d77..a26b2fbad9be 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > @@ -21,6 +21,15 @@ > // Number of Virtual Memory Map Descriptors > #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 5 > > +// > +// mach-virt's core peripherals such as the UART, the GIC and the RTC are > +// all mapped in the 'miscellaneous device I/O' region, which we just map > +// in its entirety rather than device by device. Note that it does not > +// cover any of the NOR flash banks or PCI resource windows. > +// > +#define MACH_VIRT_PERIPH_BASE 0x08000000 > +#define MACH_VIRT_PERIPH_SIZE SIZE_128MB > + > /** > Return the Virtual Memory Map of your platform > > @@ -66,16 +75,16 @@ ArmVirtGetMemoryMap ( > VirtualMemoryTable[0].VirtualBase, > VirtualMemoryTable[0].Length)); > > - // Peripheral space before DRAM > - VirtualMemoryTable[1].PhysicalBase = 0x0; > - VirtualMemoryTable[1].VirtualBase = 0x0; > - VirtualMemoryTable[1].Length = VirtualMemoryTable[0].PhysicalBase; > + // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc) > + VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE; > + VirtualMemoryTable[1].VirtualBase = MACH_VIRT_PERIPH_BASE; > + VirtualMemoryTable[1].Length = MACH_VIRT_PERIPH_SIZE; > VirtualMemoryTable[1].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > > - // Remap the FD region as normal executable memory > - VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress); > + // Map the FV region as normal executable memory > + VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress); > VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase; > - VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFdSize); > + VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFvSize); > VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; > > // End of Table > Reviewed-by: Laszlo Ersek Thanks! Laszlo