| Message ID | 1448032353-8770-1-git-send-email-mark.rutland@arm.com |
|---|---|
| State | New |
| Headers | show |
On 20 November 2015 at 16:12, Mark Rutland <mark.rutland@arm.com> wrote: > The kernel may use a page granularity of 4K, 16K, or 64K depending on > configuration. > > When mapping EFI runtime regions, we use memrange_efi_to_native to round > the physical base address of a region down to a kernel page boundary, > and round the size up to a kernel page boundary, adding the residue left > over from rounding down the physical base address. We do not round down > the virtual base address. > > In __create_mapping we account for the offset of the virtual base from a > granule boundary, adding the residue to the size before rounding the > base down to said granule boundary. > > Thus we account for the residue twice, and when the residue is non-zero > will cause __create_mapping to map an additional page at the end of the > region. Depending on the memory map, this page may be in a region we are > not intended/permitted to map, or may clash with a different region that > we which to map. > wish It is worth to note that, since the memory map is virtually always sorted in ascending order (especially now that we sort it explicitly in the stub), this is highly unlikely to cause trouble in practice, since the additional page mapping is always at the top, and will be corrected as it is overwritten by the subsequent entry. The page tables are not active at this stage, so there are no concerns regarding break-before-make etc. Obviously, we should still fix it. > As __create_mapping can cope with base addresses which are not page > aligned, we can instead rely on it to map the region appropriately, and > simplify efi_virtmap_init by removing the unnecessary code. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Cc: Will Deacon <will.deacon@arm.com> > --- > arch/arm64/kernel/efi.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > index de46b50..25b82d8 100644 > --- a/arch/arm64/kernel/efi.c > +++ b/arch/arm64/kernel/efi.c > @@ -225,7 +225,7 @@ static bool __init efi_virtmap_init(void) > efi_memory_desc_t *md; > > for_each_efi_memory_desc(&memmap, md) { > - u64 paddr, npages, size; > + u64 size; I would have dropped 'size' entirely, and use the expression as the function argument directly, but this is also fine by me. In either case: Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > pgprot_t prot; > > if (!(md->attribute & EFI_MEMORY_RUNTIME)) > @@ -233,10 +233,7 @@ static bool __init efi_virtmap_init(void) > if (md->virt_addr == 0) > return false; > > - paddr = md->phys_addr; > - npages = md->num_pages; > - memrange_efi_to_native(&paddr, &npages); > - size = npages << PAGE_SHIFT; > + size = md->num_pages << EFI_PAGE_SHIFT; > > pr_info(" EFI remap 0x%016llx => %p\n", > md->phys_addr, (void *)md->virt_addr); > @@ -254,7 +251,8 @@ static bool __init efi_virtmap_init(void) > else > prot = PAGE_KERNEL; > > - create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot); > + create_pgd_mapping(&efi_mm, md->phys_addr, md->virt_addr, > + size, prot); > } > return true; > } > -- > 1.9.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Nov 20, 2015 at 07:31:21PM +0100, Ard Biesheuvel wrote: > On 20 November 2015 at 16:12, Mark Rutland <mark.rutland@arm.com> wrote: > > Thus we account for the residue twice, and when the residue is non-zero > > will cause __create_mapping to map an additional page at the end of the > > region. Depending on the memory map, this page may be in a region we are > > not intended/permitted to map, or may clash with a different region that > > we which to map. > > > > wish Whoops; corrected locally. > It is worth to note that, since the memory map is virtually always > sorted in ascending order (especially now that we sort it explicitly > in the stub), this is highly unlikely to cause trouble in practice, > since the additional page mapping is always at the top, and will be > corrected as it is overwritten by the subsequent entry. I agree that in the common case we are extremely likely to overwrite and correct an erroneous entry (especially given that's how I detected this in the first place). I've added a note to that effect. If there is no next entry, or it's start address is sufficiently far away, we won't fix up the erroneous entry. It seems either that case is unlikely, or the region happened to be safe to map regardless. > The page tables are not active at this stage, so there are no concerns > regarding break-before-make etc. Yup. > > for_each_efi_memory_desc(&memmap, md) { > > - u64 paddr, npages, size; > > + u64 size; > > I would have dropped 'size' entirely, and use the expression as the > function argument directly, but this is also fine by me. Sure; I've folded that in locally. > In either case: > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cheers! Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index de46b50..25b82d8 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -225,7 +225,7 @@ static bool __init efi_virtmap_init(void) efi_memory_desc_t *md; for_each_efi_memory_desc(&memmap, md) { - u64 paddr, npages, size; + u64 size; pgprot_t prot; if (!(md->attribute & EFI_MEMORY_RUNTIME)) @@ -233,10 +233,7 @@ static bool __init efi_virtmap_init(void) if (md->virt_addr == 0) return false; - paddr = md->phys_addr; - npages = md->num_pages; - memrange_efi_to_native(&paddr, &npages); - size = npages << PAGE_SHIFT; + size = md->num_pages << EFI_PAGE_SHIFT; pr_info(" EFI remap 0x%016llx => %p\n", md->phys_addr, (void *)md->virt_addr); @@ -254,7 +251,8 @@ static bool __init efi_virtmap_init(void) else prot = PAGE_KERNEL; - create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot); + create_pgd_mapping(&efi_mm, md->phys_addr, md->virt_addr, + size, prot); } return true; }
The kernel may use a page granularity of 4K, 16K, or 64K depending on configuration. When mapping EFI runtime regions, we use memrange_efi_to_native to round the physical base address of a region down to a kernel page boundary, and round the size up to a kernel page boundary, adding the residue left over from rounding down the physical base address. We do not round down the virtual base address. In __create_mapping we account for the offset of the virtual base from a granule boundary, adding the residue to the size before rounding the base down to said granule boundary. Thus we account for the residue twice, and when the residue is non-zero will cause __create_mapping to map an additional page at the end of the region. Depending on the memory map, this page may be in a region we are not intended/permitted to map, or may clash with a different region that we which to map. As __create_mapping can cope with base addresses which are not page aligned, we can instead rely on it to map the region appropriately, and simplify efi_virtmap_init by removing the unnecessary code. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Leif Lindholm <leif.lindholm@linaro.org> Cc: Will Deacon <will.deacon@arm.com> --- arch/arm64/kernel/efi.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel