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 824A72118C519 for ; Wed, 28 Nov 2018 15:38:34 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1E7BD3086258; Wed, 28 Nov 2018 23:38:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-170.rdu2.redhat.com [10.10.120.170]) by smtp.corp.redhat.com (Postfix) with ESMTP id A7ED0600C6; Wed, 28 Nov 2018 23:38:28 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: Eric Auger , Andrew Jones , Philippe Mathieu-Daude References: <20181128191646.31526-1-ard.biesheuvel@linaro.org> <20181128191646.31526-3-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <07ede99a-88df-72c4-531f-2c2ed6e789cf@redhat.com> Date: Thu, 29 Nov 2018 00:38:27 +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: <20181128191646.31526-3-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Wed, 28 Nov 2018 23:38:34 +0000 (UTC) Subject: Re: [RFC PATCH 2/2] 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: Wed, 28 Nov 2018 23:38:34 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/28/18 20:16, 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. Please insert a pointer here, to the PcdFvBaseAddress / PcdFvSize settings in the "ArmVirtQemu.fdf" and "ArmVirtQemuKernel.fdf" files. You could even quote them verbatim (from both FDF files): ArmVirtQemu.fdf: > 0x00001000|0x001ff000 > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize ArmVirtQemuKernel.fdf: > 0x00008000|0x001f8000 > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > 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 | 18 +++++++++++------- > 3 files changed, 15 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..0818d0b42d6c 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > @@ -21,6 +21,10 @@ > // Number of Virtual Memory Map Descriptors > #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 5 > > +// mach-virt's 'miscellaneous device I/O' region > +#define MACH_VIRT_PERIPH_BASE 0x08000000 > +#define MACH_VIRT_PERIPH_SIZE SIZE_128MB > + So this extends from the end of VIRT_FLASH (that is, one past it) to just before VIRT_PCIE_MMIO. Correct? If so, can you work that into the comment? (Not necessarily with the QEMU enum constants.) > /** > Return the Virtual Memory Map of your platform > > @@ -66,16 +70,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) The comment helps. > + 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; So we lose coverage for: [VIRT_PCIE_MMIO] = { 0x10000000, 0x2eff0000 }, [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 }, [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, Among these, VIRT_PCIE_MMIO is handled by our FDT-based PciHostBridgeLib (just the exposure), and then added to GCD / mapped indirectly by PciHostBridgeLib. VIRT_PCIE_PIO is covered by your "[edk2] [PATCH v3 03/16] ArmVirtPkg/FdtPciHostBridgeLib: map ECAM and I/O spaces in GCD memory map". Ditto for VIRT_PCIE_ECAM. Should be safe. > > - // 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 > Seems OK. The end remains exactly the same. Can you add a summary to the commit message, about the mappings we end up here with: - [0, 4K): flash, unmapped - [4K, 128M): flash, mapped as WB+X RAM - [128M, 256M): periferals, mapped as device - [256M, 1GB): 32-bit MMIO aperture, translated IO aperture, ECAM -- none of them mapped here just yet - [1GB, ...): RAM, mapped. ... So all my remarks are about documentation; please incorporate what you see fit. :) Thanks Laszlo