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

Groups > linux.kernel > #1741293 > unrolled thread

Re: [PATCHv7 12/19] x86/mm: Adjust virtual address space layout in early boot.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv7 12/19] x86/mm: Adjust virtual address space layout in  early boot. Ingo Molnar <mingo@kernel.org> - 2017-09-28 10:40 +0200
    Re: [PATCHv7 12/19] x86/mm: Adjust virtual address space layout in  early boot. "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-09-28 15:30 +0200
      Re: [PATCHv7 12/19] x86/mm: Adjust virtual address space layout in  early boot. Ingo Molnar <mingo@kernel.org> - 2017-09-28 15:40 +0200
        Re: [PATCHv7 12/19] x86/mm: Adjust virtual address space layout in  early boot. "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-09-28 16:30 +0200

#1741293 — Re: [PATCHv7 12/19] x86/mm: Adjust virtual address space layout in early boot.

* Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:

> We need to adjust virtual address space to support switching between
> paging modes.
> 
> The adjustment happens in __startup_64().

> +#ifdef CONFIG_X86_5LEVEL
> +	if (__read_cr4() & X86_CR4_LA57) {
> +		pgtable_l5_enabled = 1;
> +		pgdir_shift = 48;
> +		ptrs_per_p4d = 512;
> +	}
> +#endif

So CR4 really sucks as a parameter passing interface - was it us who enabled LA57 
in the early boot code, right? Couldn't we add a flag which gets set there, or 
something?

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1741556

On Thu, Sep 28, 2017 at 10:31:55AM +0200, Ingo Molnar wrote:
> 
> * Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:
> 
> > We need to adjust virtual address space to support switching between
> > paging modes.
> > 
> > The adjustment happens in __startup_64().
> 
> > +#ifdef CONFIG_X86_5LEVEL
> > +	if (__read_cr4() & X86_CR4_LA57) {
> > +		pgtable_l5_enabled = 1;
> > +		pgdir_shift = 48;
> > +		ptrs_per_p4d = 512;
> > +	}
> > +#endif
> 
> So CR4 really sucks as a parameter passing interface - was it us who enabled LA57 
> in the early boot code, right? Couldn't we add a flag which gets set there, or 
> something?

It's not necessary that we enabled LA57. At least I tried to write code
that doesn't assume this. We enable it if bootloader haven't done this
already for us.

What is so awful about using CR4 as passing interface? It's one-time
check, so performance shouldn't be an issue.

-- 
 Kirill A. Shutemov

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

#1741558

* Kirill A. Shutemov <kirill@shutemov.name> wrote:

> On Thu, Sep 28, 2017 at 10:31:55AM +0200, Ingo Molnar wrote:
> > 
> > * Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:
> > 
> > > We need to adjust virtual address space to support switching between
> > > paging modes.
> > > 
> > > The adjustment happens in __startup_64().
> > 
> > > +#ifdef CONFIG_X86_5LEVEL
> > > +	if (__read_cr4() & X86_CR4_LA57) {
> > > +		pgtable_l5_enabled = 1;
> > > +		pgdir_shift = 48;
> > > +		ptrs_per_p4d = 512;
> > > +	}
> > > +#endif
> > 
> > So CR4 really sucks as a parameter passing interface - was it us who enabled LA57 
> > in the early boot code, right? Couldn't we add a flag which gets set there, or 
> > something?
> 
> It's not necessary that we enabled LA57. At least I tried to write code
> that doesn't assume this. We enable it if bootloader haven't done this
> already for us.
> 
> What is so awful about using CR4 as passing interface? It's one-time
> check, so performance shouldn't be an issue.

As a starter, this code is in generic x86 code [choose_random_location()], is this 
CR4 bit known to AMD as well and is it guaranteed to be sane across all x86 CPUs? 
I don't think so.

CR4 is a poor interface to pass CPU features through. Generaly we try to enumerate 
CPU features via CPUID, and/or enable synthetic CPU features in certain cases, and 
work from there.

Thanks,

	Ingo

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

#1741592

On Thu, Sep 28, 2017 at 03:38:38PM +0200, Ingo Molnar wrote:
> 
> * Kirill A. Shutemov <kirill@shutemov.name> wrote:
> 
> > On Thu, Sep 28, 2017 at 10:31:55AM +0200, Ingo Molnar wrote:
> > > 
> > > * Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote:
> > > 
> > > > We need to adjust virtual address space to support switching between
> > > > paging modes.
> > > > 
> > > > The adjustment happens in __startup_64().
> > > 
> > > > +#ifdef CONFIG_X86_5LEVEL
> > > > +	if (__read_cr4() & X86_CR4_LA57) {
> > > > +		pgtable_l5_enabled = 1;
> > > > +		pgdir_shift = 48;
> > > > +		ptrs_per_p4d = 512;
> > > > +	}
> > > > +#endif
> > > 
> > > So CR4 really sucks as a parameter passing interface - was it us who enabled LA57 
> > > in the early boot code, right? Couldn't we add a flag which gets set there, or 
> > > something?
> > 
> > It's not necessary that we enabled LA57. At least I tried to write code
> > that doesn't assume this. We enable it if bootloader haven't done this
> > already for us.
> > 
> > What is so awful about using CR4 as passing interface? It's one-time
> > check, so performance shouldn't be an issue.
> 
> As a starter, this code is in generic x86 code [choose_random_location()], is this 
> CR4 bit known to AMD as well and is it guaranteed to be sane across all x86 CPUs? 
> I don't think so.

It's architectural thing, so it's consistent across all x86
implementations.

> CR4 is a poor interface to pass CPU features through. Generaly we try to enumerate 
> CPU features via CPUID, and/or enable synthetic CPU features in certain cases, and 
> work from there.

Okay, has_cpuflag(X86_FEATURE_LA57) seems would do.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web