Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1594914 > unrolled thread

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

Started byBaoquan He <bhe@redhat.com>
First post2017-03-08 08:50 +0100
Last post2017-03-08 14:00 +0100
Articles 15 — 5 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Baoquan He <bhe@redhat.com> - 2017-03-08 08:50 +0100
    Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Dave Young <dyoung@redhat.com> - 2017-03-08 09:20 +0100
      Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Baoquan He <bhe@redhat.com> - 2017-03-08 10:10 +0100
      Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Bhupesh Sharma <bhsharma@redhat.com> - 2017-03-08 10:10 +0100
        Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Baoquan He <bhe@redhat.com> - 2017-03-08 10:30 +0100
          Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Borislav Petkov <bp@alien8.de> - 2017-03-08 10:40 +0100
            Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Baoquan He <bhe@redhat.com> - 2017-03-08 11:30 +0100
              Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Borislav Petkov <bp@alien8.de> - 2017-03-08 15:00 +0100
                Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Dave Young <dyoung@redhat.com> - 2017-03-09 02:00 +0100
            Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Bhupesh Sharma <bhsharma@redhat.com> - 2017-03-08 21:10 +0100
        Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Baoquan He <bhe@redhat.com> - 2017-03-08 11:20 +0100
      Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Baoquan He <bhe@redhat.com> - 2017-03-08 10:10 +0100
        Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Borislav Petkov <bp@suse.de> - 2017-03-08 14:10 +0100
        Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment Dave Young <dyoung@redhat.com> - 2017-03-09 02:50 +0100
    [PATCH v2 1/2] x86/efi/64: Clean up code comment about efi region Baoquan He <bhe@redhat.com> - 2017-03-08 14:00 +0100

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

FromBaoquan He <bhe@redhat.com>
Date2017-03-08 08:50 +0100
Subject[PATCH 1/2] x86/efi: Correct a tiny mistake in code comment
Message-ID<tiEw9-3fU-13@gated-at.bofh.it>
EFI allocate runtime services regions down from EFI_VA_START, -4G.
It should be top-down handling.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a4695da..6cbf9e0 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.
  * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
  */
 static u64 efi_va = EFI_VA_START;
-- 
2.5.5

[toc] | [next] | [standalone]


#1594923

FromDave Young <dyoung@redhat.com>
Date2017-03-08 09:20 +0100
Message-ID<tiEZc-3FD-19@gated-at.bofh.it>
In reply to#1594914
On 03/08/17 at 03:47pm, Baoquan He wrote:
> EFI allocate runtime services regions down from EFI_VA_START, -4G.
> It should be top-down handling.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/platform/efi/efi_64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index a4695da..6cbf9e0 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -47,7 +47,7 @@
>  #include <asm/pgalloc.h>
>  
>  /*
> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> + * We allocate runtime services regions top-down, starting from -4G, i.e.

Baoquan, I think original bottom-up is right, it is just considering
-68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
although from mathematics view it is lower then positive addresses.

>   * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
>   */
>  static u64 efi_va = EFI_VA_START;
> -- 
> 2.5.5
> 

Thanks
Dave

[toc] | [prev] | [next] | [standalone]


#1594958

FromBaoquan He <bhe@redhat.com>
Date2017-03-08 10:10 +0100
Message-ID<tiFLA-4fF-29@gated-at.bofh.it>
In reply to#1594923
On 03/08/17 at 09:54am, Borislav Petkov wrote:
> On Wed, Mar 08, 2017 at 04:45:13PM +0800, Baoquan He wrote:
> > -4G and -68G just a trick which makes people understand easily, still we
> > think kernel text mapping region is in higher addr area then vmalloc. I
> > personnally think.
> 
> Just remove the direction: bottom-up or top-down, it will confuse people.
> 
> "We allocate runtime services regions starting from -4G, i.e.
> 0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space
> is limited to 64G."

Right, will repost one.

Thanks!

[toc] | [prev] | [next] | [standalone]


#1594961

FromBhupesh Sharma <bhsharma@redhat.com>
Date2017-03-08 10:10 +0100
Message-ID<tiFLA-4fF-27@gated-at.bofh.it>
In reply to#1594923
Hi Dave,

On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung@redhat.com> wrote:
> On 03/08/17 at 03:47pm, Baoquan He wrote:
>> EFI allocate runtime services regions down from EFI_VA_START, -4G.
>> It should be top-down handling.
>>
>> Signed-off-by: Baoquan He <bhe@redhat.com>
>> ---
>>  arch/x86/platform/efi/efi_64.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
>> index a4695da..6cbf9e0 100644
>> --- a/arch/x86/platform/efi/efi_64.c
>> +++ b/arch/x86/platform/efi/efi_64.c
>> @@ -47,7 +47,7 @@
>>  #include <asm/pgalloc.h>
>>
>>  /*
>> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
>> + * We allocate runtime services regions top-down, starting from -4G, i.e.
>
> Baoquan, I think original bottom-up is right, it is just considering
> -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> although from mathematics view it is lower then positive addresses.

I think you have a valid point, but I think the -4G convention is
probably too confusing to read and may lead to issues when we use this
for future feature addition as well. It would be more useful to use
the macros similar to the MODULES_{} addresses we use currently in
'arch/x86/include/asm/pgtable_64_types.h':

#define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
#define MODULES_END      _AC(0xffffffffff000000, UL)
#define MODULES_LEN   (MODULES_END - MODULES_VADDR)

May be we can use the following convention for the EFI_VA_{} addresses
as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':

#define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
#define EFI_VA_END    _AC(0xffffffef00000000, UL)

which is less confusing to read in my opinion.

@Baoquan: Please share your views.

Regards,
Bhupesh

[toc] | [prev] | [next] | [standalone]


#1594965

FromBaoquan He <bhe@redhat.com>
Date2017-03-08 10:30 +0100
Message-ID<tiG4W-4sw-3@gated-at.bofh.it>
In reply to#1594961
On 03/08/17 at 02:30pm, Bhupesh Sharma wrote:
> Hi Dave,
> 
> On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung@redhat.com> wrote:
> > On 03/08/17 at 03:47pm, Baoquan He wrote:
> >> EFI allocate runtime services regions down from EFI_VA_START, -4G.
> >> It should be top-down handling.
> >>
> >> Signed-off-by: Baoquan He <bhe@redhat.com>
> >> ---
> >>  arch/x86/platform/efi/efi_64.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> >> index a4695da..6cbf9e0 100644
> >> --- a/arch/x86/platform/efi/efi_64.c
> >> +++ b/arch/x86/platform/efi/efi_64.c
> >> @@ -47,7 +47,7 @@
> >>  #include <asm/pgalloc.h>
> >>
> >>  /*
> >> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> >> + * We allocate runtime services regions top-down, starting from -4G, i.e.
> >
> > Baoquan, I think original bottom-up is right, it is just considering
> > -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> > although from mathematics view it is lower then positive addresses.
> 
> I think you have a valid point, but I think the -4G convention is
> probably too confusing to read and may lead to issues when we use this
> for future feature addition as well. It would be more useful to use
> the macros similar to the MODULES_{} addresses we use currently in
> 'arch/x86/include/asm/pgtable_64_types.h':
> 
> #define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
> #define MODULES_END      _AC(0xffffffffff000000, UL)
> #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
> 
> May be we can use the following convention for the EFI_VA_{} addresses
> as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':
> 
> #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> #define EFI_VA_END    _AC(0xffffffef00000000, UL)
> 
> which is less confusing to read in my opinion.
> 
> @Baoquan: Please share your views.

Yes, it looks better. I can repost with this change. Thanks.

[toc] | [prev] | [next] | [standalone]


#1594983

FromBorislav Petkov <bp@alien8.de>
Date2017-03-08 10:40 +0100
Message-ID<tiGeC-4xJ-21@gated-at.bofh.it>
In reply to#1594965
On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
> Yes, it looks better. I can repost with this change. Thanks.

No it doesn't:

#define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
#define EFI_VA_END       (-68 * (_AC(1, UL) << 30))

That's -4G (the shift by 30) and -68G, respectively.

> > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> > #define EFI_VA_END    _AC(0xffffffef00000000, UL)

That is something which I need to type into a calculator first.

Can you guys point your attention to something which is really broken
and stop wasting your time? And there's enough really broken crap left
and right...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]


#1595027

FromBaoquan He <bhe@redhat.com>
Date2017-03-08 11:30 +0100
Message-ID<tiH10-5e8-23@gated-at.bofh.it>
In reply to#1594983
On 03/08/17 at 10:35am, Borislav Petkov wrote:
> On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
> > Yes, it looks better. I can repost with this change. Thanks.
> 
> No it doesn't:

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.

Change or keep it, both is fine to me.

Thanks!

> 
> #define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
> #define EFI_VA_END       (-68 * (_AC(1, UL) << 30))
> 
> That's -4G (the shift by 30) and -68G, respectively.
> 
> > > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> > > #define EFI_VA_END    _AC(0xffffffef00000000, UL)
> 
> That is something which I need to type into a calculator first.
> 
> Can you guys point your attention to something which is really broken
> and stop wasting your time? And there's enough really broken crap left
> and right...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]


#1595188

FromBorislav Petkov <bp@alien8.de>
Date2017-03-08 15:00 +0100
Message-ID<tiKid-7jK-5@gated-at.bofh.it>
In reply to#1595027
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.

Again, as previously, this is a maintainer decision.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]


#1595612

FromDave Young <dyoung@redhat.com>
Date2017-03-09 02:00 +0100
Message-ID<tiUAV-5SX-5@gated-at.bofh.it>
In reply to#1595188
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

[toc] | [prev] | [next] | [standalone]


#1595473

FromBhupesh Sharma <bhsharma@redhat.com>
Date2017-03-08 21:10 +0100
Message-ID<tiQ4i-2ZW-21@gated-at.bofh.it>
In reply to#1594983
On Wed, Mar 8, 2017 at 3:05 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
>> Yes, it looks better. I can repost with this change. Thanks.
>
> No it doesn't:
>
> #define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
> #define EFI_VA_END       (-68 * (_AC(1, UL) << 30))
>
> That's -4G (the shift by 30) and -68G, respectively.
>
>> > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
>> > #define EFI_VA_END    _AC(0xffffffef00000000, UL)
>
> That is something which I need to type into a calculator first.

Right, my point was that this -4G convention stands out, as compared
to the rest of the addressing convention used throughout
'arch/x86/include/asm/pgtable_64_types.h'.

For e.g.:

#define __VMALLOC_BASE    _AC(0xffffc90000000000, UL)
#define __VMEMMAP_BASE    _AC(0xffffea0000000000, UL)
..
#define MODULES_END      _AC(0xffffffffff000000, UL)
..
and so on.

Also it seems inconsistent to the convention used in
'Documentation/x86/x86_64/mm.txt'

As you noted in one of your other comments in this thread, this causes
a confusion as to whether the EFI_VA_END and EFI_VA_START macros need
to be swapped or the comments elsewhere in the x86 code which use
these MACROS are incorrect.

However, I think it is more a matter of code readability and each of
these styles have their own advantages.

I would be happy to cook up a patch to have uniformity in the
addressing conventions used across pgtable_64_types.h (if it is
required at all), but it would be a different topic and not related to
this patch.

Regards,
Bhupesh

[toc] | [prev] | [next] | [standalone]


#1595019

FromBaoquan He <bhe@redhat.com>
Date2017-03-08 11:20 +0100
Message-ID<tiGRk-5aS-23@gated-at.bofh.it>
In reply to#1594961
On 03/08/17 at 02:30pm, Bhupesh Sharma wrote:
> Hi Dave,
> 
> On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung@redhat.com> wrote:
> > On 03/08/17 at 03:47pm, Baoquan He wrote:
> >> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> >> + * We allocate runtime services regions top-down, starting from -4G, i.e.
> >
> > Baoquan, I think original bottom-up is right, it is just considering
> > -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> > although from mathematics view it is lower then positive addresses.
> 
> I think you have a valid point, but I think the -4G convention is
> probably too confusing to read and may lead to issues when we use this
> for future feature addition as well. It would be more useful to use
> the macros similar to the MODULES_{} addresses we use currently in
> 'arch/x86/include/asm/pgtable_64_types.h':
> 
> #define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
> #define MODULES_END      _AC(0xffffffffff000000, UL)
> #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
> 
> May be we can use the following convention for the EFI_VA_{} addresses
> as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':
> 
> #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> #define EFI_VA_END    _AC(0xffffffef00000000, UL)

Isn't it like this:

#define EFI_VA_START    _AC(0xffffffff00000000, UL)
#define EFI_VA_END    _AC(0xffffffef00000000, UL)

Just make them be equal to value which computer stores -4G and -68G?

You can see in arch/x86/platform/efi/efi_64.c, it's using efi_va minus
size directly, here size should be 4K, page aligned. 

	efi_va -= size;

Above formula has considered the open interval attribute of
EFI_VA_START. Making EFI_VA_START be 0xfffffffeffffffff could be wrong.

Right?

[toc] | [prev] | [next] | [standalone]


#1594962

FromBaoquan He <bhe@redhat.com>
Date2017-03-08 10:10 +0100
Message-ID<tiFLA-4fF-31@gated-at.bofh.it>
In reply to#1594923
Forgot cc to Boris, add him.

On 03/08/17 at 04:18pm, Dave Young wrote:
> On 03/08/17 at 03:47pm, Baoquan He wrote:
> > EFI allocate runtime services regions down from EFI_VA_START, -4G.
> > It should be top-down handling.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/platform/efi/efi_64.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index a4695da..6cbf9e0 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -47,7 +47,7 @@
> >  #include <asm/pgalloc.h>
> >  
> >  /*
> > - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> > + * We allocate runtime services regions top-down, starting from -4G, i.e.
> 
> Baoquan, I think original bottom-up is right, it is just considering
> -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> although from mathematics view it is lower then positive addresses.

Thanks for reviewing!

I am not sure. Just in efi_map_region() it gets the starting va to map
'size' big of region by below code:
	efi_va -= size;

-4G and -68G just a trick which makes people understand easily, still we
think kernel text mapping region is in higher addr area then vmalloc. I
personnally think.

Thanks
Baoquan

[toc] | [prev] | [next] | [standalone]


#1595147

FromBorislav Petkov <bp@suse.de>
Date2017-03-08 14:10 +0100
Message-ID<tiFLA-4fF-33@gated-at.bofh.it>
In reply to#1594962
On Wed, Mar 08, 2017 at 04:45:13PM +0800, Baoquan He wrote:
> -4G and -68G just a trick which makes people understand easily, still we
> think kernel text mapping region is in higher addr area then vmalloc. I
> personnally think.

Just remove the direction: bottom-up or top-down, it will confuse people.

"We allocate runtime services regions starting from -4G, i.e.
0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space
is limited to 64G."

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]


#1595629

FromDave Young <dyoung@redhat.com>
Date2017-03-09 02:50 +0100
Message-ID<tiVnj-6qV-3@gated-at.bofh.it>
In reply to#1594962
Hi,

On 03/08/17 at 04:45pm, Baoquan He wrote:
> Forgot cc to Boris, add him.
> 
> On 03/08/17 at 04:18pm, Dave Young wrote:
> > On 03/08/17 at 03:47pm, Baoquan He wrote:
> > > EFI allocate runtime services regions down from EFI_VA_START, -4G.
> > > It should be top-down handling.
> > > 
> > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > ---
> > >  arch/x86/platform/efi/efi_64.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > index a4695da..6cbf9e0 100644
> > > --- a/arch/x86/platform/efi/efi_64.c
> > > +++ b/arch/x86/platform/efi/efi_64.c
> > > @@ -47,7 +47,7 @@
> > >  #include <asm/pgalloc.h>
> > >  
> > >  /*
> > > - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> > > + * We allocate runtime services regions top-down, starting from -4G, i.e.
> > 
> > Baoquan, I think original bottom-up is right, it is just considering
> > -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> > although from mathematics view it is lower then positive addresses.
> 
> Thanks for reviewing!
> 
> I am not sure. Just in efi_map_region() it gets the starting va to map
> 'size' big of region by below code:
> 	efi_va -= size;
> 
> -4G and -68G just a trick which makes people understand easily, still we
> think kernel text mapping region is in higher addr area then vmalloc. I
> personnally think.

I understand your points, there is not right or wrong. So I think drop
the words like the change in your V2 looks good.

Thanks
Dave

[toc] | [prev] | [next] | [standalone]


#1595135 — [PATCH v2 1/2] x86/efi/64: Clean up code comment about efi region

FromBaoquan He <bhe@redhat.com>
Date2017-03-08 14:00 +0100
Subject[PATCH v2 1/2] x86/efi/64: Clean up code comment about efi region
Message-ID<tiJm9-6HB-5@gated-at.bofh.it>
In reply to#1594914
EFI allocates runtime services regions starting from EFI_VA_START, -4G,
decrement to EFI_VA_END. So remove the bottom-up term to avoid confusion.

Signed-off-by: Baoquan He <bhe@redhat.com>
---

 arch/x86/platform/efi/efi_64.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a4695da..e941b29 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -47,8 +47,9 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
- * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
+ * We allocate runtime services regions starting from -4G, i.e.
+ * 0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space
+ * is limited to 64G.
  */
 static u64 efi_va = EFI_VA_START;
 
-- 
2.5.5

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web