> On 6 jun. 2016, at 19:09, Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
>
>> On Mon, Jun 06, 2016 at 11:43:01AM +0200, Ard Biesheuvel wrote:
>>> On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
>>> Since the EFI page size is 4KB, it is possible for a !4KB page kernel to
>>> align an EFI runtime map boundaries in a way that they can overlap
>>> within the same page. This requires the current create_pgd_mapping()
>>> code to be able to split existing larger mappings when an overlapping
>>> region needs to be mapped.
>>>
>>> With this patch, efi_create_mapping() scans the EFI memory map for
>>> overlapping regions and trims the length of the current map to avoid a
>>> large block mapping and subsequent split.
>>
>> I remember the discussion, but IIRC this was about failure to do
>> break-before-make in general rather than a problem with splitting in
>> particular.
>
> In this instance, we don't have break-before-make issue since the pgd
> used for EFI runtime services is not active. So it's just about
> splitting a larger block and simplifying the arch code.
>
>> So first of all, I would like to point out that this failure to do
>> break-before-make is not a problem in practice, given that the EFI
>> page tables are not live until much later. The reason we want to fix
>> this is because the EFI page tables routines are shared with the
>> swapper, where it /is/ a concern, and there is a desire to make those
>> routines more strict when it comes to architectural rules regarding
>> page table manipulation.
>
> Exactly.
>
>> Also, the reference to block mappings on !4K pages kernels is a bit
>> misleading here: such block mappings are at least 32 MB in size, and
>> we never map runtime code or data regions that big.
>
> Jeremy has some patches in flight to use the contiguous bit on such
> mappings. With 64KB x 32 pages, we get a 2MB block. I want to avoid code
> splitting such contiguous block as well (which with the contiguous bit
> means going back and clearing it).
>
>>> Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>>> ---
>>> arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---
>>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>>> index 78f52488f9ff..0d5753c31c7f 100644
>>> --- a/arch/arm64/kernel/efi.c
>>> +++ b/arch/arm64/kernel/efi.c
>>> @@ -62,10 +62,26 @@ struct screen_info screen_info __section(.data);
>>> int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
>>> {
>>> pteval_t prot_val = create_mapping_protection(md);
>>> + phys_addr_t length = md->num_pages << EFI_PAGE_SHIFT;
>>> + efi_memory_desc_t *next = md;
>>>
>>> - create_pgd_mapping(mm, md->phys_addr, md->virt_addr,
>>> - md->num_pages << EFI_PAGE_SHIFT,
>>> - __pgprot(prot_val | PTE_NG));
>>> + /*
>>> + * Search for the next EFI runtime map and check for any overlap with
>>> + * the current map when aligned to PAGE_SIZE. In such case, defer
>>> + * mapping the end of the current range until the next
>>> + * efi_create_mapping() call.
>>> + */
>>> + for_each_efi_memory_desc_continue(next) {
>>> + if (!(next->attribute & EFI_MEMORY_RUNTIME))
>>> + continue;
>>> + if (next->phys_addr < PAGE_ALIGN(md->phys_addr + length))
>>> + length -= (md->phys_addr + length) & ~PAGE_MASK;
>>
>> This looks fishy. We could have more than two runtime regions sharing
>> a 64 KB page frame, for instance, and the middle regions will have
>> unaligned start and end addresses, and sizes smaller than 64 KB. This
>> subtraction may wrap in that case, producing a bogus length.
>
> I don't think I get it. This subtraction is meant to reduce the length
> of the current md so that it is page aligned, deferring the mapping of
> the last page to the next efi_create_mapping() call. Note that there is
> a "break" just after the hunk you quoted, so we only care about the next
> runtime map.
>
If the current entry is only 4k in size, and starts 56k into the 64k page frame, you subtract 60k, and you end up with a length of -56k--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
