| Message ID | 1443218539-7610-3-git-send-email-matt@codeblueprint.co.uk |
|---|---|
| State | Accepted |
| Commit | 0ce3cc008ec04258b6a6314b09f1a6012810881a |
| Headers | show |
* Matt Fleming <matt@codeblueprint.co.uk> wrote: > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > The new Properties Table feature introduced in UEFIv2.5 may split > memory regions that cover PE/COFF memory images into separate code > and data regions. Since these regions only differ in the type (runtime > code vs runtime data) and the permission bits, but not in the memory > type attributes (UC/WC/WT/WB), the spec does not require them to be > aligned to 64 KB. > > Since the relative offset of PE/COFF .text and .data segments cannot > be changed on the fly, this means that we can no longer pad out those > regions to be mappable using 64 KB pages. > Unfortunately, there is no annotation in the UEFI memory map that > identifies data regions that were split off from a code region, so we > must apply this logic to all adjacent runtime regions whose attributes > only differ in the permission bits. > > So instead of rounding each memory region to 64 KB alignment at both > ends, only round down regions that are not directly preceded by another > runtime region with the same type attributes. Since the UEFI spec does > not mandate that the memory map be sorted, this means we also need to > sort it first. So I think this is fundamentally wrong as well, similarly to the related x86 fix. I think for compatibility reasons the whole 'EFI runtime image' should be mapped in a single go, as closely matching the EFI layouts and offsets as possible. We are not talking about gigabytes here, right? Even if technically they are 'separate sections', the x86 bug shows that they aren't. So we should not pretend so on the Linux side either and we should not tear them apart (and then work hard to preserve the interdependencies, some declared, some hidden!). If we allocate the EFI runtime as a single virtual memory block then issues like rounding between sections does not even come up as a problem: we map the original offsets and sizes byte by byte. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 September 2015 at 23:01, Ingo Molnar <mingo@kernel.org> wrote: > > * Matt Fleming <matt@codeblueprint.co.uk> wrote: > >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> The new Properties Table feature introduced in UEFIv2.5 may split >> memory regions that cover PE/COFF memory images into separate code >> and data regions. Since these regions only differ in the type (runtime >> code vs runtime data) and the permission bits, but not in the memory >> type attributes (UC/WC/WT/WB), the spec does not require them to be >> aligned to 64 KB. >> >> Since the relative offset of PE/COFF .text and .data segments cannot >> be changed on the fly, this means that we can no longer pad out those >> regions to be mappable using 64 KB pages. >> Unfortunately, there is no annotation in the UEFI memory map that >> identifies data regions that were split off from a code region, so we >> must apply this logic to all adjacent runtime regions whose attributes >> only differ in the permission bits. >> >> So instead of rounding each memory region to 64 KB alignment at both >> ends, only round down regions that are not directly preceded by another >> runtime region with the same type attributes. Since the UEFI spec does >> not mandate that the memory map be sorted, this means we also need to >> sort it first. > > So I think this is fundamentally wrong as well, similarly to the related x86 fix. > > I think for compatibility reasons the whole 'EFI runtime image' should be mapped > in a single go, as closely matching the EFI layouts and offsets as possible. We > are not talking about gigabytes here, right? > As I explained in the other thread, this is really not necessary, and never has been until the firmware started splitting up PE/COFF images into several sections each. As long as we keep those PE/COFF images together, everything will work as before, and the only complication is that the memory map does not contain any clues about which regions belong to a single PE/COFF image, so we need to keep all adjacent EFI_MEMORY_RUNTIME regions adjacent in the VA mapping. > Even if technically they are 'separate sections', the x86 bug shows that they > aren't. So we should not pretend so on the Linux side either and we should not > tear them apart (and then work hard to preserve the interdependencies, some > declared, some hidden!). > This is about relocations and interdependencies at the symbol level, and such interdependencies only exist internally inside PE/COFF images. > If we allocate the EFI runtime as a single virtual memory block then issues like > rounding between sections does not even come up as a problem: we map the original > offsets and sizes byte by byte. > Well, by that reasoning, we should not call SetVirtualAddressMap() in the first place, and just use the 1:1 mapping UEFI uses natively. This is more than feasible on arm64, and I actually fought hard against using SetVirtualAddressMap() at all, but I was overruled by others. I think this is also trivially possible on X64, since the 1:1 mapping is already active alongside the VA mapping.
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > If we allocate the EFI runtime as a single virtual memory block then issues > > like rounding between sections does not even come up as a problem: we map the > > original offsets and sizes byte by byte. > > Well, by that reasoning, we should not call SetVirtualAddressMap() in the first > place, and just use the 1:1 mapping UEFI uses natively. This is more than > feasible on arm64, and I actually fought hard against using > SetVirtualAddressMap() at all, but I was overruled by others. I think this is > also trivially possible on X64, since the 1:1 mapping is already active > alongside the VA mapping. Could we please re-list all the arguments pro and contra of 1:1 physical mappings, in a post that also explains the background so that more people can chime in, not just people versed in EFI internals? It's very much possible that a bad decision was made. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Sep 27, 2015 at 09:06:44AM +0200, Ingo Molnar wrote: > Could we please re-list all the arguments pro and contra of 1:1 physical mappings, > in a post that also explains the background so that more people can chime in, not > just people versed in EFI internals? It's very much possible that a bad decision > was made. The main reason why we did the additional, top-down mapping was kexec kernel wanting to use UEFI runtime facilities too and the braindead design of SetVirtualAddressMap() being callable only once per system boot. So we had to have stable mappings which are valid in the kexec-ed kernel too. But this was long time ago and I most certainly have forgotten all the details. And now I'm wondering why didn't we do the 1:1 thing and rebuild the exact same EFI pagetable in the kexec-ed kernel? Because when we do an EFI call, we switch to the special pagetable so why didn't we make the kexec-ed kernel rebuild the 1:1 pagetable which it can use for EFI calls... Hmm, again, I've forgotten a lot of details so I'm sure Matt will come in and say "No, you can't do that because..."
* Borislav Petkov <bp@alien8.de> wrote: > On Sun, Sep 27, 2015 at 09:06:44AM +0200, Ingo Molnar wrote: > > > Could we please re-list all the arguments pro and contra of 1:1 physical > > mappings, in a post that also explains the background so that more people can > > chime in, not just people versed in EFI internals? It's very much possible > > that a bad decision was made. > > The main reason why we did the additional, top-down mapping was kexec kernel > wanting to use UEFI runtime facilities too and the braindead design of > SetVirtualAddressMap() being callable only once per system boot. So we had to > have stable mappings which are valid in the kexec-ed kernel too. > > But this was long time ago and I most certainly have forgotten all the details. > > And now I'm wondering why didn't we do the 1:1 thing and rebuild the exact same > EFI pagetable in the kexec-ed kernel? Because when we do an EFI call, we switch > to the special pagetable so why didn't we make the kexec-ed kernel rebuild the > 1:1 pagetable which it can use for EFI calls... Yeah. > Hmm, again, I've forgotten a lot of details so I'm sure Matt will come in and > say "No, you can't do that because..." Would be nice to re-examine all this. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/27/15 at 12:40pm, Borislav Petkov wrote: > On Sun, Sep 27, 2015 at 09:06:44AM +0200, Ingo Molnar wrote: > > Could we please re-list all the arguments pro and contra of 1:1 physical mappings, > > in a post that also explains the background so that more people can chime in, not > > just people versed in EFI internals? It's very much possible that a bad decision > > was made. > > The main reason why we did the additional, top-down mapping was kexec > kernel wanting to use UEFI runtime facilities too and the braindead > design of SetVirtualAddressMap() being callable only once per system > boot. So we had to have stable mappings which are valid in the kexec-ed > kernel too. > > But this was long time ago and I most certainly have forgotten all the > details. > > And now I'm wondering why didn't we do the 1:1 thing and rebuild the > exact same EFI pagetable in the kexec-ed kernel? Because when we do > an EFI call, we switch to the special pagetable so why didn't we make > the kexec-ed kernel rebuild the 1:1 pagetable which it can use for EFI > calls... > > Hmm, again, I've forgotten a lot of details so I'm sure Matt will come > in and say "No, you can't do that because..." > http://permalink.gmane.org/gmane.linux.kernel.efi/822 And more replies here: http://comments.gmane.org/gmane.linux.kernel.efi/820 Thanks Dave -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 29, 2015 at 05:31:00PM +0800, Dave Young wrote: > http://permalink.gmane.org/gmane.linux.kernel.efi/822 > > And more replies here: > http://comments.gmane.org/gmane.linux.kernel.efi/820 Whatever we did think back then, it all is moot as long as some UEFI vendors do funky crap or the spec has documentation holes. So we need a reality check and just do exactly what the OS does with which UEFI writers are testing with. Be it windoze, be it some open firmware testing tool or whatever. Provided there's even a tool with which UEFI validation is done and it is used by everybody...
On Sun, 27 Sep, at 12:40:14PM, Borislav Petkov wrote: > On Sun, Sep 27, 2015 at 09:06:44AM +0200, Ingo Molnar wrote: > > Could we please re-list all the arguments pro and contra of 1:1 physical mappings, > > in a post that also explains the background so that more people can chime in, not > > just people versed in EFI internals? It's very much possible that a bad decision > > was made. > > The main reason why we did the additional, top-down mapping was kexec > kernel wanting to use UEFI runtime facilities too and the braindead > design of SetVirtualAddressMap() being callable only once per system > boot. So we had to have stable mappings which are valid in the kexec-ed > kernel too. > > But this was long time ago and I most certainly have forgotten all the > details. That's a pretty good summary for x86. I think specifically the reason we map the EFI memmap entries "backwards" (entry N has higher VA than entry N+1) is because the code was easier to write that way, but you'll know better than me ;-) > And now I'm wondering why didn't we do the 1:1 thing and rebuild the > exact same EFI pagetable in the kexec-ed kernel? Because when we do > an EFI call, we switch to the special pagetable so why didn't we make > the kexec-ed kernel rebuild the 1:1 pagetable which it can use for EFI > calls... > > Hmm, again, I've forgotten a lot of details so I'm sure Matt will come > in and say "No, you can't do that because..." I *think* the only reason was the Apple firmware problem where it explodes if you pass the 1:1 mappings to SetVirtualAddressMap(). And obviously people do want to use kexec with Apple machines. It's probably worth revisiting this whole thing from the x86 side.
On 09/29/2015 07:36 AM, Matt Fleming wrote: > > That's a pretty good summary for x86. I think specifically the reason > we map the EFI memmap entries "backwards" (entry N has higher VA than > entry N+1) is because the code was easier to write that way, but > you'll know better than me ;-) > There were two reasons: 1. The code was easier to write. 2. Windows did it that way. Windows apparently broke and was changed due to this feature, too. -hpa -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/27/2015 12:06 AM, Ingo Molnar wrote: > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >>> If we allocate the EFI runtime as a single virtual memory block then issues >>> like rounding between sections does not even come up as a problem: we map the >>> original offsets and sizes byte by byte. >> >> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first >> place, and just use the 1:1 mapping UEFI uses natively. This is more than >> feasible on arm64, and I actually fought hard against using >> SetVirtualAddressMap() at all, but I was overruled by others. I think this is >> also trivially possible on X64, since the 1:1 mapping is already active >> alongside the VA mapping. > > Could we please re-list all the arguments pro and contra of 1:1 physical mappings, > in a post that also explains the background so that more people can chime in, not > just people versed in EFI internals? It's very much possible that a bad decision > was made. > Pro: by far the sanest way to map the UEFI tables. Con: doesn't actually work (breaks on several known platforms.) -hpa -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 29, 2015 at 6:03 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 09/27/2015 12:06 AM, Ingo Molnar wrote: >> >> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >>>> If we allocate the EFI runtime as a single virtual memory block then issues >>>> like rounding between sections does not even come up as a problem: we map the >>>> original offsets and sizes byte by byte. >>> >>> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first >>> place, and just use the 1:1 mapping UEFI uses natively. This is more than >>> feasible on arm64, and I actually fought hard against using >>> SetVirtualAddressMap() at all, but I was overruled by others. I think this is >>> also trivially possible on X64, since the 1:1 mapping is already active >>> alongside the VA mapping. >> >> Could we please re-list all the arguments pro and contra of 1:1 physical mappings, >> in a post that also explains the background so that more people can chime in, not >> just people versed in EFI internals? It's very much possible that a bad decision >> was made. >> > > Pro: by far the sanest way to map the UEFI tables. > Con: doesn't actually work (breaks on several known platforms.) Can we at least do 1:1 without an offset on arm64? Given that Linux seems to be more of a reference on arm64 than Windows is, maybe that would give everyone something vaguely sane to work with. --Andy -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/29/2015 06:16 PM, Andy Lutomirski wrote: > > Can we at least do 1:1 without an offset on arm64? Given that Linux > seems to be more of a reference on arm64 than Windows is, maybe that > would give everyone something vaguely sane to work with. > I have no idea. That's a question for the ARM folks. -hpa -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30 September 2015 at 03:16, Andy Lutomirski <luto@amacapital.net> wrote: > On Tue, Sep 29, 2015 at 6:03 PM, H. Peter Anvin <hpa@zytor.com> wrote: >> On 09/27/2015 12:06 AM, Ingo Molnar wrote: >>> >>> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> >>>>> If we allocate the EFI runtime as a single virtual memory block then issues >>>>> like rounding between sections does not even come up as a problem: we map the >>>>> original offsets and sizes byte by byte. >>>> >>>> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first >>>> place, and just use the 1:1 mapping UEFI uses natively. This is more than >>>> feasible on arm64, and I actually fought hard against using >>>> SetVirtualAddressMap() at all, but I was overruled by others. I think this is >>>> also trivially possible on X64, since the 1:1 mapping is already active >>>> alongside the VA mapping. >>> >>> Could we please re-list all the arguments pro and contra of 1:1 physical mappings, >>> in a post that also explains the background so that more people can chime in, not >>> just people versed in EFI internals? It's very much possible that a bad decision >>> was made. >>> >> >> Pro: by far the sanest way to map the UEFI tables. >> Con: doesn't actually work (breaks on several known platforms.) > > Can we at least do 1:1 without an offset on arm64? Given that Linux > seems to be more of a reference on arm64 than Windows is, maybe that > would give everyone something vaguely sane to work with. > Yes, as I mentioned before in this thread, on arm64 this is very much feasible, and it was my strong preference all along. However, the arguments made by others that outweighed my preference were (a) x86 uses it (b) if we don't use it now, we will never be able to start using it later since it will undoubtedly be broken in /some/ implementation in the field. As I also mentioned, the only minor complication is that arm64's VA space may be configured to be smaller than the physical base of DRAM, but I already had to address that for the boot ID map and KVM as well. I will cook up a patch later today.
On Tue, Sep 29, 2015 at 05:56:07PM -0700, H. Peter Anvin wrote: > On 09/29/2015 07:36 AM, Matt Fleming wrote: > > > > That's a pretty good summary for x86. I think specifically the reason > > we map the EFI memmap entries "backwards" (entry N has higher VA than > > entry N+1) is because the code was easier to write that way, but > > you'll know better than me ;-) > > > > There were two reasons: > > 1. The code was easier to write. > 2. Windows did it that way. > > Windows apparently broke and was changed due to this feature, too. So can we do the 1:1 thing again? I mean, we do create a special pagetable for EFI anyway, we can put in there whatever we want. I know, some apple boxes reportedly fail when 1:1 mapping is in use but we can do the VA mapping as a workaround for them. I.e., have the 1:1 mapping be the default. Apparently, there's not a single OS or tool which is used by fw writers to test their brain dumplings. Windoze breakage case-in-point. Because if there were, we'd simply do what that OS/tool does and be done with it. What really makes me climb the walls is when half-cooked, untested fw hits the wild and we have to support it indefinitely.
* H. Peter Anvin <hpa@zytor.com> wrote: > On 09/27/2015 12:06 AM, Ingo Molnar wrote: > > > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > >>> If we allocate the EFI runtime as a single virtual memory block then issues > >>> like rounding between sections does not even come up as a problem: we map the > >>> original offsets and sizes byte by byte. > >> > >> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first > >> place, and just use the 1:1 mapping UEFI uses natively. This is more than > >> feasible on arm64, and I actually fought hard against using > >> SetVirtualAddressMap() at all, but I was overruled by others. I think this is > >> also trivially possible on X64, since the 1:1 mapping is already active > >> alongside the VA mapping. > > > > Could we please re-list all the arguments pro and contra of 1:1 physical mappings, > > in a post that also explains the background so that more people can chime in, not > > just people versed in EFI internals? It's very much possible that a bad decision > > was made. > > > > Pro: by far the sanest way to map the UEFI tables. > Con: doesn't actually work (breaks on several known platforms.) You knew this next question was coming: in what way does it break on known platforms? I.e. do those platforms require a SetVirtualAddressMap() call and break if one does not come? Note that there's 3 models possible: - pure 1:1 - 1:1 plus offset, with SetVirtualAddressMap(offset) - bottom up allocator I don't think we want 'pure' 1:1 physical/virtual (for security reasons, etc.). So the question is, in what way does our current proposed bottom-up allocator differ from 1:1 plus offset? My impression is that they are mostly identical. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index e8ca6eaedd02..13671a9cf016 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void) */ if (!is_normal_ram(md)) prot = __pgprot(PROT_DEVICE_nGnRE); - else if (md->type == EFI_RUNTIME_SERVICES_CODE) + else if (md->type == EFI_RUNTIME_SERVICES_CODE || + !PAGE_ALIGNED(md->phys_addr)) prot = PAGE_KERNEL_EXEC; else prot = PAGE_KERNEL; diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c index e29560e6b40b..950c87f5d279 100644 --- a/drivers/firmware/efi/libstub/arm-stub.c +++ b/drivers/firmware/efi/libstub/arm-stub.c @@ -13,6 +13,7 @@ */ #include <linux/efi.h> +#include <linux/sort.h> #include <asm/efi.h> #include "efistub.h" @@ -305,6 +306,44 @@ fail: */ #define EFI_RT_VIRTUAL_BASE 0x40000000 +static int cmp_mem_desc(const void *l, const void *r) +{ + const efi_memory_desc_t *left = l, *right = r; + + return (left->phys_addr > right->phys_addr) ? 1 : -1; +} + +/* + * Returns whether region @left ends exactly where region @right starts, + * or false if either argument is NULL. + */ +static bool regions_are_adjacent(efi_memory_desc_t *left, + efi_memory_desc_t *right) +{ + u64 left_end; + + if (left == NULL || right == NULL) + return false; + + left_end = left->phys_addr + left->num_pages * EFI_PAGE_SIZE; + + return left_end == right->phys_addr; +} + +/* + * Returns whether region @left and region @right have compatible memory type + * mapping attributes, and are both EFI_MEMORY_RUNTIME regions. + */ +static bool regions_have_compatible_memory_type_attrs(efi_memory_desc_t *left, + efi_memory_desc_t *right) +{ + static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT | + EFI_MEMORY_WC | EFI_MEMORY_UC | + EFI_MEMORY_RUNTIME; + + return ((left->attribute ^ right->attribute) & mem_type_mask) == 0; +} + /* * efi_get_virtmap() - create a virtual mapping for the EFI memory map * @@ -317,33 +356,52 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size, int *count) { u64 efi_virt_base = EFI_RT_VIRTUAL_BASE; - efi_memory_desc_t *out = runtime_map; + efi_memory_desc_t *in, *prev = NULL, *out = runtime_map; int l; - for (l = 0; l < map_size; l += desc_size) { - efi_memory_desc_t *in = (void *)memory_map + l; + /* + * To work around potential issues with the Properties Table feature + * introduced in UEFI 2.5, which may split PE/COFF executable images + * in memory into several RuntimeServicesCode and RuntimeServicesData + * regions, we need to preserve the relative offsets between adjacent + * EFI_MEMORY_RUNTIME regions with the same memory type attributes. + * The easiest way to find adjacent regions is to sort the memory map + * before traversing it. + */ + sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL); + + for (l = 0; l < map_size; l += desc_size, prev = in) { u64 paddr, size; + in = (void *)memory_map + l; if (!(in->attribute & EFI_MEMORY_RUNTIME)) continue; + paddr = in->phys_addr; + size = in->num_pages * EFI_PAGE_SIZE; + /* * Make the mapping compatible with 64k pages: this allows * a 4k page size kernel to kexec a 64k page size kernel and * vice versa. */ - paddr = round_down(in->phys_addr, SZ_64K); - size = round_up(in->num_pages * EFI_PAGE_SIZE + - in->phys_addr - paddr, SZ_64K); - - /* - * Avoid wasting memory on PTEs by choosing a virtual base that - * is compatible with section mappings if this region has the - * appropriate size and physical alignment. (Sections are 2 MB - * on 4k granule kernels) - */ - if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M) - efi_virt_base = round_up(efi_virt_base, SZ_2M); + if (!regions_are_adjacent(prev, in) || + !regions_have_compatible_memory_type_attrs(prev, in)) { + + paddr = round_down(in->phys_addr, SZ_64K); + size += in->phys_addr - paddr; + + /* + * Avoid wasting memory on PTEs by choosing a virtual + * base that is compatible with section mappings if this + * region has the appropriate size and physical + * alignment. (Sections are 2 MB on 4k granule kernels) + */ + if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M) + efi_virt_base = round_up(efi_virt_base, SZ_2M); + else + efi_virt_base = round_up(efi_virt_base, SZ_64K); + } in->virt_addr = efi_virt_base + in->phys_addr - paddr; efi_virt_base += size;