Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Dave Young]

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