On 03/08/17 at 11:50am, Borislav Petkov wrote:
> On Wed, Mar 08, 2017 at 06:17:50PM +0800, Baoquan He wrote:
> > All right, I will just update the code comment. Just back ported kaslr
> > to our OS product, people reviewed and found the upper boundary of kaslr
> > mm region is EFI_VA_START, that's not correct, it has to be corrected
> > firstly in upstream. Then found the confusion in code comment.
>
> BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
> + vaddr_end >= EFI_VA_END);
>
> so I think that once we've done the mapping, we won't need anymore VA
> space so we could simply check the range [efi_va, EFI_VA_START] instead.
>
> However, that won't work currently because evi_va is not valid at
> build time. And it won't work at boot time either because, AFAICT,
> kernel_randomize_memory() runs before efi_enter_virtual_mode() so ...
>
> So yours is probably OK.
>
> I guess what's confusing there is the naming - EFI_VA_START and
> EFI_VA_END. They're kinda swapped because of the direction we take when
> we start mapping runtime services, i.e., from the higher (unsigned)
> address to lower.
>
> I guess we could swap the naming so that it doesn't confuse people but
> that would be up to EFI maintainers.
>
> Then stuff like that:
>
> # ifdef CONFIG_EFI
> { EFI_VA_END, "EFI Runtime Services" },
> # endif
>
> will make more sense when they are:
>
> # ifdef CONFIG_EFI
> { EFI_VA_START, "EFI Runtime Services" },
> # endif
>
> But changing it now could confuse more people who have the current
> mental picture of the mapping direction so I'd vote for the simple fix
> above.
People should understand the meaning of the macro then use it correctly,
one should not assume START == lower address unless they are sure.
>
> Again, as previously, this is a maintainer decision.
>
Personally I think current way is just fine, but agreed it is up to efi
maintainer.
Thanks
Dave
--
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
