Borislav Petkov | 22 Jul 15:15 2011

[PATCH] x86, AMD: Correct F15h IC aliasing issue

From: Borislav Petkov <borislav.petkov <at> amd.com>

This patch provides performance tuning for the "Bulldozer" CPU. With its
shared instruction cache there is a chance of generating an excessive
number of cache cross-invalidates when running specific workloads on the
cores of a compute module.

This excessive amount of cross-invalidations can be observed if cache
lines backed by shared physical memory alias in bits [14:12] of their
virtual addresses, as those bits are used for the index generation.

This patch addresses the issue by zeroing out the slice [14:12] of
the file mapping's virtual address at generation time, thus forcing
those bits the same for all mappings of a single shared library across
processes and, in doing so, avoids instruction cache aliases.

It also adds the kernel command line option
"unalias_va_addr=(32|64|off)" with which virtual address unaliasing
can be enabled for 32-bit or 64-bit x86 individually, or be completely
disabled.

This change leaves virtual region address allocation on other families
and/or vendors unaffected.

Signed-off-by: Andre Przywara <andre.przywara <at> amd.com>
Signed-off-by: Martin Pohlack <martin.pohlack <at> amd.com>
Signed-off-by: Borislav Petkov <borislav.petkov <at> amd.com>
---
 Documentation/kernel-parameters.txt |    6 ++++
 arch/x86/include/asm/elf.h          |   36 +++++++++++++++++++++++++++
(Continue reading)

Ingo Molnar | 24 Jul 13:13 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


(Cc:-ed Linus and akpm)

* Borislav Petkov <bp <at> amd64.org> wrote:

> From: Borislav Petkov <borislav.petkov <at> amd.com>
> 
> This patch provides performance tuning for the "Bulldozer" CPU. With its
> shared instruction cache there is a chance of generating an excessive
> number of cache cross-invalidates when running specific workloads on the
> cores of a compute module.
> 
> This excessive amount of cross-invalidations can be observed if cache
> lines backed by shared physical memory alias in bits [14:12] of their
> virtual addresses, as those bits are used for the index generation.
> 
> This patch addresses the issue by zeroing out the slice [14:12] of
> the file mapping's virtual address at generation time, thus forcing
> those bits the same for all mappings of a single shared library across
> processes and, in doing so, avoids instruction cache aliases.

So all commonly-aliased virtual mappings of the same pages should be 
32K granular aligned for good performance? Seems rather unfortunate 
for a modern CPU - is this going to be the case for all family 0x15 
CPUs?

> It also adds the kernel command line option
> "unalias_va_addr=(32|64|off)" with which virtual address unaliasing
> can be enabled for 32-bit or 64-bit x86 individually, or be completely
> disabled.
(Continue reading)

Borislav Petkov | 24 Jul 15:40 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

Hi Ingo,

thanks for taking the time to look at this in detail.

On Sun, Jul 24, 2011 at 07:13:50AM -0400, Ingo Molnar wrote:
> 
> (Cc:-ed Linus and akpm)
> 
> * Borislav Petkov <bp <at> amd64.org> wrote:
> 
> > From: Borislav Petkov <borislav.petkov <at> amd.com>
> > 
> > This patch provides performance tuning for the "Bulldozer" CPU. With its
> > shared instruction cache there is a chance of generating an excessive
> > number of cache cross-invalidates when running specific workloads on the
> > cores of a compute module.
> > 
> > This excessive amount of cross-invalidations can be observed if cache
> > lines backed by shared physical memory alias in bits [14:12] of their
> > virtual addresses, as those bits are used for the index generation.
> > 
> > This patch addresses the issue by zeroing out the slice [14:12] of
> > the file mapping's virtual address at generation time, thus forcing
> > those bits the same for all mappings of a single shared library across
> > processes and, in doing so, avoids instruction cache aliases.
> 
> So all commonly-aliased virtual mappings of the same pages should be 
> 32K granular aligned for good performance? Seems rather unfortunate 
> for a modern CPU - is this going to be the case for all family 0x15 
> CPUs?
(Continue reading)

Ingo Molnar | 24 Jul 15:47 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


* Borislav Petkov <bp <at> amd64.org> wrote:

> > > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> > > index b13ed39..2d380c6 100644
> > > --- a/arch/x86/kernel/cpu/amd.c
> > > +++ b/arch/x86/kernel/cpu/amd.c
> > >  <at>  <at>  -458,6 +458,13  <at>  <at>  static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
> > >  					"with P0 frequency!\n");
> > >  		}
> > >  	}
> > > +
> > > +	if (unalias_va_addr == -1) {
> > > +		if (c->x86 == 0x15)
> > > +			unalias_va_addr = UNALIAS_VA_32 | UNALIAS_VA_64;
> > > +		else
> > > +			unalias_va_addr = 0;
> > 
> > the placement here feels a bit wrong - don't we have run-once CPU 
> > quirks?
> 
> Maybe another func ptr in struct x86_cpuinit_ops
> (arch/x86/kernel/x86_init.c) would be a better place?

Yeah.

Assuming no objections come i'd suggest to do that as an add-on patch 
- so the first patch can be backported, the second patch cleans up 
the x86_cpuinit_ops aspect.

(Continue reading)

Andrew Morton | 24 Jul 18:16 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Sun, 24 Jul 2011 15:40:46 +0200 Borislav Petkov <bp <at> amd64.org> wrote:

> > > +	unalias_va_addr
> > > +			[X86-64] Unalias VA address by clearing slice [14:12]
> > > +			1: only on 32-bit
> > > +			2: only on 64-bit
> > > +			off: disabled on both 32 and 64-bit

How much performance difference does this actually make?

> > This says nothing about why it's cleared, what it does and why one 
> > would want to handle it differently on 32-bit and 64-bit.
> 
> This is there in case users care more about the 3bits ASLR granularity
> than invalidations amount, for example.

To make this decision the users will want to know how much their
computers will slow down.  We should tell them.
Borislav Petkov | 26 Jul 20:33 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Sun, Jul 24, 2011 at 09:40:46AM -0400, Borislav Petkov wrote:
> > also, the PAGE_ALIGN() complication here looks unnecessary - can the
> > vdso 'addr' ever be not 4K aligned above?
> 
> Yeah, it can. I did some trace_printk runs and 'addr' wasn't 4K aligned
> in some cases. I think this is because of the stack randomization we do
> and we use mm->start_stack to get the vdso base address. Unfortunately,
> I can't find those traces anymore but will do some again tomorrow.

Better late than never :).

So yes, mm->start_stack comes in unaligned from setup_arg_pages() and we
use it to generate the vdso VA because the vdso is below the stack:

7fff761f9000-7fff7621a000 rw-p 00000000 00:00 0                          [stack]
7fff763ec000-7fff763ed000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]

here's a trace_printk excerpt from vdso_addr() (gets inlined into
arch_setup_additional_pages()):

           <...>-2516  [006]   624.364274: arch_setup_additional_pages: mm->start_stack: 0x7fff7621942a, len: 4096
           <...>-2516  [006]   624.364278: arch_setup_additional_pages: randomized: 0x7fff763eb42a
           <...>-2516  [006]   624.364279: arch_setup_additional_pages: final addr: 0x7fff763eb42a

Then, both arch_get_unmapped_area{,_topdown} page-align it.

For our case, we PAGE_ALIGN it before randomizing it so that we can
control the [14:12] setting.

(Continue reading)

Linus Torvalds | 24 Jul 18:04 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

Argh. This is a small disaster, you know that, right? Suddenly we have
user-visible allocation changes depending on which CPU you are running
on. I just hope that the address-space randomization has caught all
the code that depended on specific layouts.

And even with ASLR, I wouldn't be surprised if there are binaries out
there that "know" that they get dense virtual memory when they do
back-to-back allocations, even when they don't pass in the address
explicitly.

How much testing has AMD done with this change and various legacy
Linux distros? The 32-bit case in particular makes me nervous, that's
where I'd expect a higher likelihood of binaries that depend on the
layout.

You guys do realize that we had to disable ASLR on many machines?

So at a MINIMUM, I would say that this is acceptable only when the
process doing the allocation hasn't got ASLR disabled.

On Fri, Jul 22, 2011 at 6:15 AM, Borislav Petkov <bp <at> amd64.org> wrote:
> +static inline unsigned long unalias_addr(unsigned long addr, bool incr)
> +{
> +       /* handle both 32- and 64-bit with a single conditional */
> +       if (!(unalias_va_addr & (2 - mmap_is_ia32())))
> +               return addr;

Ugh. I guess it works, but the actual values you used did not have a
comment about those particular values being magical. You should do
that, otherwise somebody will start adding bits and moving things
(Continue reading)

Borislav Petkov | 24 Jul 19:22 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

Hi Linus,

On Sun, Jul 24, 2011 at 12:04:27PM -0400, Linus Torvalds wrote:
> Argh. This is a small disaster, you know that, right? Suddenly we have
> user-visible allocation changes depending on which CPU you are running
> on. I just hope that the address-space randomization has caught all
> the code that depended on specific layouts.
> 
> And even with ASLR, I wouldn't be surprised if there are binaries out
> there that "know" that they get dense virtual memory when they do
> back-to-back allocations, even when they don't pass in the address
> explicitly.

That's what I'm afraid of. Thus the boot option to disable it, I'm
afraid the patch won't be of help in such situations.

> How much testing has AMD done with this change and various legacy
> Linux distros?

Currently underway. Just to make sure: this change currently addresses
only the case with 32-bit binaries running on a 64-bit kernel and not
the 32-bit kernel case.

> The 32-bit case in particular makes me nervous, that's where I'd
> expect a higher likelihood of binaries that depend on the layout.

Ok, noted.

> You guys do realize that we had to disable ASLR on many machines?

(Continue reading)

Linus Torvalds | 24 Jul 19:39 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Sun, Jul 24, 2011 at 10:22 AM, Borislav Petkov <bp <at> amd64.org> wrote:
>
>> So at a MINIMUM, I would say that this is acceptable only when the
>> process doing the allocation hasn't got ASLR disabled.
>
> I guess I could look at randomize_va_space before enabling it.

That's not what I meant - I meant the per-process PF_RANDOMIZE and
ADDR_NO_RANDOMIZE personality flags (although the global
"randomize_va_space" thing obviously is one input to that too)

In fact, if 99% of your problem is ASLR-induced, might I suggest just
making the whole thing a tweak to ASLR instead, and not use ASLR for
bits 14:12? That should be fundamentally much safer: it doesn't change
any semantics at all, it just makes for slightly less random bits to
be used.

So I really think that you might be *much* better off just changing
mmap_rnd(), and nothing else. Just make *that* mask off the three low
bits of the random address, ie something like

  diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
  index 1dab5194fd9d..6b62ab5a5ae1 100644
  --- a/arch/x86/mm/mmap.c
  +++ b/arch/x86/mm/mmap.c
   <at>  <at>  -90,6 +90,9  <at>  <at>  static unsigned long mmap_rnd(void)
                          rnd = (long)get_random_int() % (1<<8);
                  else
                          rnd = (long)(get_random_int() % (1<<28));
  +
(Continue reading)

Ingo Molnar | 24 Jul 20:12 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


* Linus Torvalds <torvalds <at> linux-foundation.org> wrote:

> On Sun, Jul 24, 2011 at 10:22 AM, Borislav Petkov <bp <at> amd64.org> wrote:
> >
> >> So at a MINIMUM, I would say that this is acceptable only when the
> >> process doing the allocation hasn't got ASLR disabled.
> >
> > I guess I could look at randomize_va_space before enabling it.
> 
> That's not what I meant - I meant the per-process PF_RANDOMIZE and 
> ADDR_NO_RANDOMIZE personality flags (although the global 
> "randomize_va_space" thing obviously is one input to that too)
> 
> In fact, if 99% of your problem is ASLR-induced, might I suggest 
> just making the whole thing a tweak to ASLR instead, and not use 
> ASLR for bits 14:12? That should be fundamentally much safer: it 
> doesn't change any semantics at all, it just makes for slightly 
> less random bits to be used.

Indeed - that would be much nicer and smaller as well. It could also 
go away easily if this bug is fixed in a future CPU.

Thanks,

	Ingo
Borislav Petkov | 24 Jul 20:23 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Sun, Jul 24, 2011 at 01:39:25PM -0400, Linus Torvalds wrote:
> On Sun, Jul 24, 2011 at 10:22 AM, Borislav Petkov <bp <at> amd64.org> wrote:
> >
> >> So at a MINIMUM, I would say that this is acceptable only when the
> >> process doing the allocation hasn't got ASLR disabled.
> >
> > I guess I could look at randomize_va_space before enabling it.
> 
> That's not what I meant - I meant the per-process PF_RANDOMIZE and
> ADDR_NO_RANDOMIZE personality flags (although the global
> "randomize_va_space" thing obviously is one input to that too)
> 
> In fact, if 99% of your problem is ASLR-induced, might I suggest just
> making the whole thing a tweak to ASLR instead, and not use ASLR for
> bits 14:12? That should be fundamentally much safer: it doesn't change
> any semantics at all, it just makes for slightly less random bits to
> be used.
> 
> So I really think that you might be *much* better off just changing
> mmap_rnd(), and nothing else. Just make *that* mask off the three low
> bits of the random address, ie something like
> 
>   diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
>   index 1dab5194fd9d..6b62ab5a5ae1 100644
>   --- a/arch/x86/mm/mmap.c
>   +++ b/arch/x86/mm/mmap.c
>    <at>  <at>  -90,6 +90,9  <at>  <at>  static unsigned long mmap_rnd(void)
>                           rnd = (long)get_random_int() % (1<<8);
>                   else
>                           rnd = (long)(get_random_int() % (1<<28));
(Continue reading)

Ingo Molnar | 24 Jul 20:30 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


* Borislav Petkov <bp <at> amd64.org> wrote:

> On Sun, Jul 24, 2011 at 01:39:25PM -0400, Linus Torvalds wrote:
> > On Sun, Jul 24, 2011 at 10:22 AM, Borislav Petkov <bp <at> amd64.org> wrote:
> > >
> > >> So at a MINIMUM, I would say that this is acceptable only when the
> > >> process doing the allocation hasn't got ASLR disabled.
> > >
> > > I guess I could look at randomize_va_space before enabling it.
> > 
> > That's not what I meant - I meant the per-process PF_RANDOMIZE and
> > ADDR_NO_RANDOMIZE personality flags (although the global
> > "randomize_va_space" thing obviously is one input to that too)
> > 
> > In fact, if 99% of your problem is ASLR-induced, might I suggest just
> > making the whole thing a tweak to ASLR instead, and not use ASLR for
> > bits 14:12? That should be fundamentally much safer: it doesn't change
> > any semantics at all, it just makes for slightly less random bits to
> > be used.
> > 
> > So I really think that you might be *much* better off just changing
> > mmap_rnd(), and nothing else. Just make *that* mask off the three low
> > bits of the random address, ie something like
> > 
> >   diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
> >   index 1dab5194fd9d..6b62ab5a5ae1 100644
> >   --- a/arch/x86/mm/mmap.c
> >   +++ b/arch/x86/mm/mmap.c
> >    <at>  <at>  -90,6 +90,9  <at>  <at>  static unsigned long mmap_rnd(void)
(Continue reading)

Borislav Petkov | 24 Jul 21:07 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Sun, Jul 24, 2011 at 02:30:46PM -0400, Ingo Molnar wrote:
> > > So I really think that you might be *much* better off just changing
> > > mmap_rnd(), and nothing else. Just make *that* mask off the three low
> > > bits of the random address, ie something like
> > > 
> > >   diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
> > >   index 1dab5194fd9d..6b62ab5a5ae1 100644
> > >   --- a/arch/x86/mm/mmap.c
> > >   +++ b/arch/x86/mm/mmap.c
> > >    <at>  <at>  -90,6 +90,9  <at>  <at>  static unsigned long mmap_rnd(void)
> > >                           rnd = (long)get_random_int() % (1<<8);
> > >                   else
> > >                           rnd = (long)(get_random_int() % (1<<28));
> > >   +
> > >   +               if (avoid_aliasing_in_bits_14_12)
> > >   +                       rnd &= ~7;
> > >           }
> > >           return rnd << PAGE_SHIFT;
> > >    }
> > > 
> > > would be fundamentally very safe - it would already take all our
> > > current anti-randomization code into account.
> > > 
> > > No?
> > 
> > Hehe, we had that idea initially. However, the special 1% case I was
> > hinting at is this:
> > 
> > process P0, mapping libraries A, B, C
> > 
(Continue reading)

Ingo Molnar | 24 Jul 22:44 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


* Borislav Petkov <bp <at> amd64.org> wrote:

> On Sun, Jul 24, 2011 at 02:30:46PM -0400, Ingo Molnar wrote:
> > > > So I really think that you might be *much* better off just changing
> > > > mmap_rnd(), and nothing else. Just make *that* mask off the three low
> > > > bits of the random address, ie something like
> > > > 
> > > >   diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
> > > >   index 1dab5194fd9d..6b62ab5a5ae1 100644
> > > >   --- a/arch/x86/mm/mmap.c
> > > >   +++ b/arch/x86/mm/mmap.c
> > > >    <at>  <at>  -90,6 +90,9  <at>  <at>  static unsigned long mmap_rnd(void)
> > > >                           rnd = (long)get_random_int() % (1<<8);
> > > >                   else
> > > >                           rnd = (long)(get_random_int() % (1<<28));
> > > >   +
> > > >   +               if (avoid_aliasing_in_bits_14_12)
> > > >   +                       rnd &= ~7;
> > > >           }
> > > >           return rnd << PAGE_SHIFT;
> > > >    }
> > > > 
> > > > would be fundamentally very safe - it would already take all our
> > > > current anti-randomization code into account.
> > > > 
> > > > No?
> > > 
> > > Hehe, we had that idea initially. However, the special 1% case I was
> > > hinting at is this:
(Continue reading)

Borislav Petkov | 25 Jul 22:00 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Sun, Jul 24, 2011 at 04:44:50PM -0400, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp <at> amd64.org> wrote:
> 
> > On Sun, Jul 24, 2011 at 02:30:46PM -0400, Ingo Molnar wrote:
> > > > > So I really think that you might be *much* better off just changing
> > > > > mmap_rnd(), and nothing else. Just make *that* mask off the three low
> > > > > bits of the random address, ie something like
> > > > > 
> > > > >   diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
> > > > >   index 1dab5194fd9d..6b62ab5a5ae1 100644
> > > > >   --- a/arch/x86/mm/mmap.c
> > > > >   +++ b/arch/x86/mm/mmap.c
> > > > >    <at>  <at>  -90,6 +90,9  <at>  <at>  static unsigned long mmap_rnd(void)
> > > > >                           rnd = (long)get_random_int() % (1<<8);
> > > > >                   else
> > > > >                           rnd = (long)(get_random_int() % (1<<28));
> > > > >   +
> > > > >   +               if (avoid_aliasing_in_bits_14_12)
> > > > >   +                       rnd &= ~7;
> > > > >           }
> > > > >           return rnd << PAGE_SHIFT;
> > > > >    }
> > > > > 
> > > > > would be fundamentally very safe - it would already take all our
> > > > > current anti-randomization code into account.
> > > > > 
> > > > > No?
> > > > 
> > > > Hehe, we had that idea initially. However, the special 1% case I was
(Continue reading)

Ingo Molnar | 25 Jul 22:06 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


* Borislav Petkov <bp <at> amd64.org> wrote:

> So yeah, the simpler fix works for processes mapping libraries in 
> the same order but not for processes mapping a subset of libraries 
> in a different order.

but what is the proportion of 'good' versus bad alignment of 
libraries, could you collect some stats on a representative enough, 
fully booted up Linux system?

Are only 1% of mappings 'bad'? 5%? 10%? 50%? We have no idea and the 
actual number matters a lot.

Thanks,

	Ingo
Borislav Petkov | 25 Jul 23:53 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Mon, Jul 25, 2011 at 04:06:45PM -0400, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp <at> amd64.org> wrote:
> 
> > So yeah, the simpler fix works for processes mapping libraries in 
> > the same order but not for processes mapping a subset of libraries 
> > in a different order.
> 
> but what is the proportion of 'good' versus bad alignment of 
> libraries, could you collect some stats on a representative enough, 
> fully booted up Linux system?
> 
> Are only 1% of mappings 'bad'? 5%? 10%? 50%? We have no idea and the 
> actual number matters a lot.

Actually, it's hard to say what is a 'good' and 'bad' allocation. If
they don't alias, they're always good :).

Anyway, I did some beginners awk-ery on a gentoo system with ca. 150
processes. Below is some data for the "r-xp" mappings.

ld-2.12.2.so, for example, is always loaded at an address with [14:12]
= 000b while libc's addresses differ in those bits because a different
number of allocations is being done between ld and libc, thus libc lands
at different offsets.

I'll do a more comprehensive collection tomorrow and distribute the
mappings by [14:12] value, that should give us a better idea.

/lib64/ld-2.12.2.so|r-xp|7f07f61d8000 1
(Continue reading)

Ray Lee | 26 Jul 07:58 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Mon, Jul 25, 2011 at 2:53 PM, Borislav Petkov <bp <at> amd64.org> wrote:
>
> On Mon, Jul 25, 2011 at 04:06:45PM -0400, Ingo Molnar wrote:
> >
> > * Borislav Petkov <bp <at> amd64.org> wrote:
> >
> > > So yeah, the simpler fix works for processes mapping libraries in
> > > the same order but not for processes mapping a subset of libraries
> > > in a different order.
> >
> > but what is the proportion of 'good' versus bad alignment of
> > libraries, could you collect some stats on a representative enough,
> > fully booted up Linux system?
> >
> > Are only 1% of mappings 'bad'? 5%? 10%? 50%? We have no idea and the
> > actual number matters a lot.
>
> Actually, it's hard to say what is a 'good' and 'bad' allocation. If
> they don't alias, they're always good :).

Count the number of aliases per total mappings, and report that as a
failure percentage? It doesn't have to be a perfect metric, it just
needs to be a meaningful and repeatable one.
Borislav Petkov | 26 Jul 19:28 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Tue, Jul 26, 2011 at 01:58:43AM -0400, Ray Lee wrote:
> > > Are only 1% of mappings 'bad'? 5%? 10%? 50%? We have no idea and the
> > > actual number matters a lot.
> >
> > Actually, it's hard to say what is a 'good' and 'bad' allocation. If
> > they don't alias, they're always good :).
> 
> Count the number of aliases per total mappings, and report that as a
> failure percentage? It doesn't have to be a perfect metric, it just
> needs to be a meaningful and repeatable one.

Ok, I went and installed a Debian wheezy with gnome and started almost
everything I could find in the startup menu. Kernel is 3.0 with Linus'
simpler fix.

The attached file shows all libraries which would create aliases (i.e. 2
or more unique values for bits [14:12] along with the respective count)
in the total of:

Total r-xp mappings: 831, aliasing: 240 (0.289%)

For example, libc gets mapped into all possible slots for [14:12]

Library                                 [14:12] count
=======                                 ======= =====
/lib/libc-2.11.2.so
                                        0       12
                                        1       12
                                        2       11
                                        3       12
(Continue reading)

Ingo Molnar | 26 Jul 20:34 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


* Borislav Petkov <bp <at> amd64.org> wrote:

> Ok, I went and installed a Debian wheezy with gnome and started 
> almost everything I could find in the startup menu. Kernel is 3.0 
> with Linus' simpler fix.
> 
> The attached file shows all libraries which would create aliases 
> (i.e. 2 or more unique values for bits [14:12] along with the 
> respective count) in the total of:
> 
> Total r-xp mappings: 831, aliasing: 240 (0.289%)

28.9%, right?

> For example, libc gets mapped into all possible slots for [14:12]
> 
> Library                                 [14:12] count
> =======                                 ======= =====
> /lib/libc-2.11.2.so
>                                         0       12
>                                         1       12
>                                         2       11
>                                         3       12
>                                         4       13
>                                         5       16
>                                         6       13
>                                         7       9
> 
> 
(Continue reading)

Borislav Petkov | 26 Jul 20:39 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Tue, Jul 26, 2011 at 02:34:30PM -0400, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp <at> amd64.org> wrote:
> 
> > Ok, I went and installed a Debian wheezy with gnome and started 
> > almost everything I could find in the startup menu. Kernel is 3.0 
> > with Linus' simpler fix.
> > 
> > The attached file shows all libraries which would create aliases 
> > (i.e. 2 or more unique values for bits [14:12] along with the 
> > respective count) in the total of:
> > 
> > Total r-xp mappings: 831, aliasing: 240 (0.289%)
> 
> 28.9%, right?

Yessir.

> > For example, libc gets mapped into all possible slots for [14:12]
> > 
> > Library                                 [14:12] count
> > =======                                 ======= =====
> > /lib/libc-2.11.2.so
> >                                         0       12
> >                                         1       12
> >                                         2       11
> >                                         3       12
> >                                         4       13
> >                                         5       16
> >                                         6       13
(Continue reading)

Ingo Molnar | 26 Jul 20:47 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


* Borislav Petkov <bp <at> amd64.org> wrote:

> > Was this done with a stock kernel, or with the simple patch 
> > applied?
> 
> with the simplest version of Linus' patch:
> 
> diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
> index 1dab519..6b1094d 100644
> --- a/arch/x86/mm/mmap.c
> +++ b/arch/x86/mm/mmap.c
>  <at>  <at>  -90,6 +90,8  <at>  <at>  static unsigned long mmap_rnd(void)
>                         rnd = (long)get_random_int() % (1<<8);
>                 else
>                         rnd = (long)(get_random_int() % (1<<28));
> +
> +               rnd &= ~0x7;

So, unless i got the stats right, unmodified kernel has like a 75% 
mis-aligned rate, while with the simple fix this goes down to 30%?

Have you run the numbers on the stock, unmodified kernel by any 
chance, just to make sure the stats are ok?

Thanks,

	Ingo
Borislav Petkov | 26 Jul 21:33 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Tue, Jul 26, 2011 at 02:47:44PM -0400, Ingo Molnar wrote:
> So, unless i got the stats right, unmodified kernel has like a 75%
> mis-aligned rate, while with the simple fix this goes down to 30%?

Nah, I don't think this workaround has such an impact. Due
to the differing link order and the workaround tweaking
_only_ the start of the VA space of the process (see
http://marc.info/?l=linux-kernel&m=131163083618197), it actually doesn't
change a lot:

Total r-xp mappings: 902, aliasing: 282 (31.264%)

(I know, I know, those are different numbers but the percentage is in
the same ballpark).

> Have you run the numbers on the stock, unmodified kernel by any 
> chance, just to make sure the stats are ok?

attached.

--

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
(Continue reading)

Borislav Petkov | 27 Jul 19:10 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Sun, Jul 24, 2011 at 01:39:25PM -0400, Linus Torvalds wrote:
> > Yeah, I like the BITS() thing - will change. I actually have a similar
> > macro GENMASK(o, hi) in <drivers/edac/amd64_edac.h> - I should move it
> > to <linux/bitops.h> and rename it to BITS().
> 
> So it may be that BITS() is much too generic a name, and will cause
> problems. A quick "git grep -w BITS" certainly finds a fair number of
> hits. So I don't think it's usable as-is, it was meant more as
> pseudo-code.

How about something like the following instead - it should take care
of all your bitmask generating needs. There are also a couple of
GENMASK/BITMASK identical definitions around the tree which can be
unified while I'm at it too.

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index a3ef66a..b1970e3 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
 <at>  <at>  -6,6 +6,19  <at>  <at> 
 #define BIT(nr)                        (1UL << (nr))
 #define BIT_MASK(nr)           (1UL << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)           ((nr) / BITS_PER_LONG)
+
+/*
+ * Create a contiguous bitmask starting at bit position  <at> lo and ending at
+ * position  <at> hi. For example
+ *
+ * GENMASK_ULL(21, 39) gives us the 64bit vector 0x000000ffffe00000.
+ */
(Continue reading)

H. Peter Anvin | 27 Jul 19:16 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/27/2011 10:10 AM, Borislav Petkov wrote:
> 
> How about something like the following instead - it should take care
> of all your bitmask generating needs. There are also a couple of
> GENMASK/BITMASK identical definitions around the tree which can be
> unified while I'm at it too.
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index a3ef66a..b1970e3 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
>  <at>  <at>  -6,6 +6,19  <at>  <at> 
>  #define BIT(nr)                        (1UL << (nr))
>  #define BIT_MASK(nr)           (1UL << ((nr) % BITS_PER_LONG))
>  #define BIT_WORD(nr)           ((nr) / BITS_PER_LONG)
> +
> +/*
> + * Create a contiguous bitmask starting at bit position  <at> lo and ending at
> + * position  <at> hi. For example
> + *
> + * GENMASK_ULL(21, 39) gives us the 64bit vector 0x000000ffffe00000.
> + */
> +#define _GENMASK_T(cast, type, lo, hi) \
> +       (((cast)(1##type << ((hi) - (lo) + 1)) - 1) << (lo))
> +#define GENMASK(lo, hi)                _GENMASK_T(unsigned, U, lo, hi)
> +#define GENMASK_UL(lo, hi)     _GENMASK_T(unsigned long, UL, lo, hi)
> +#define GENMASK_ULL(lo, hi)    _GENMASK_T(unsigned long long, ULL, lo, hi)
> +

These really need to be usable from assembly language, too (in which
(Continue reading)

Borislav Petkov | 28 Jul 15:44 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Wed, Jul 27, 2011 at 01:16:04PM -0400, H. Peter Anvin wrote:
> > + * Create a contiguous bitmask starting at bit position  <at> lo and ending at
> > + * position  <at> hi. For example
> > + *
> > + * GENMASK_ULL(21, 39) gives us the 64bit vector 0x000000ffffe00000.
> > + */
> > +#define _GENMASK_T(cast, type, lo, hi) \
> > +       (((cast)(1##type << ((hi) - (lo) + 1)) - 1) << (lo))
> > +#define GENMASK(lo, hi)                _GENMASK_T(unsigned, U, lo, hi)
> > +#define GENMASK_UL(lo, hi)     _GENMASK_T(unsigned long, UL, lo, hi)
> > +#define GENMASK_ULL(lo, hi)    _GENMASK_T(unsigned long long, ULL, lo, hi)
> > +
> 
> These really need to be usable from assembly language, too (in which
> case you of course need to not have the cast and suffix), so it probably
> should be defined in <linux/const.h> with the other constant macros.

How about that:

#define _GENMASK_T(cast, type, lo, hi) \
	        ((_AT(cast, (_AC(1,type) << ((hi) - (lo) + 1))) - 1) << (lo))
#define GENMASK(lo, hi)         _GENMASK_T(unsigned, U, lo, hi)
#define GENMASK_UL(lo, hi)      _GENMASK_T(unsigned long, UL, lo, hi)
#define GENMASK_ULL(lo, hi)     _GENMASK_T(unsigned long long, ULL, lo, hi)

outside of __KERNEL__ scope?

--

-- 
Regards/Gruss,
Boris.
(Continue reading)

H. Peter Anvin | 28 Jul 16:02 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/28/2011 06:44 AM, Borislav Petkov wrote:
> On Wed, Jul 27, 2011 at 01:16:04PM -0400, H. Peter Anvin wrote:
>>> + * Create a contiguous bitmask starting at bit position  <at> lo and ending at
>>> + * position  <at> hi. For example
>>> + *
>>> + * GENMASK_ULL(21, 39) gives us the 64bit vector 0x000000ffffe00000.
>>> + */
>>> +#define _GENMASK_T(cast, type, lo, hi) \
>>> +       (((cast)(1##type << ((hi) - (lo) + 1)) - 1) << (lo))
>>> +#define GENMASK(lo, hi)                _GENMASK_T(unsigned, U, lo, hi)
>>> +#define GENMASK_UL(lo, hi)     _GENMASK_T(unsigned long, UL, lo, hi)
>>> +#define GENMASK_ULL(lo, hi)    _GENMASK_T(unsigned long long, ULL, lo, hi)
>>> +
>>
>> These really need to be usable from assembly language, too (in which
>> case you of course need to not have the cast and suffix), so it probably
>> should be defined in <linux/const.h> with the other constant macros.
> 
> How about that:
> 
> #define _GENMASK_T(cast, type, lo, hi) \
> 	        ((_AT(cast, (_AC(1,type) << ((hi) - (lo) + 1))) - 1) << (lo))
> #define GENMASK(lo, hi)         _GENMASK_T(unsigned, U, lo, hi)
> #define GENMASK_UL(lo, hi)      _GENMASK_T(unsigned long, UL, lo, hi)
> #define GENMASK_ULL(lo, hi)     _GENMASK_T(unsigned long long, ULL, lo, hi)
> 
> outside of __KERNEL__ scope?
> 

Then everything needs another underscore.
(Continue reading)

Borislav Petkov | 28 Jul 16:13 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Thu, Jul 28, 2011 at 10:02:46AM -0400, H. Peter Anvin wrote:
> On 07/28/2011 06:44 AM, Borislav Petkov wrote:
> > On Wed, Jul 27, 2011 at 01:16:04PM -0400, H. Peter Anvin wrote:
> >>> + * Create a contiguous bitmask starting at bit position  <at> lo and ending at
> >>> + * position  <at> hi. For example
> >>> + *
> >>> + * GENMASK_ULL(21, 39) gives us the 64bit vector 0x000000ffffe00000.
> >>> + */
> >>> +#define _GENMASK_T(cast, type, lo, hi) \
> >>> +       (((cast)(1##type << ((hi) - (lo) + 1)) - 1) << (lo))
> >>> +#define GENMASK(lo, hi)                _GENMASK_T(unsigned, U, lo, hi)
> >>> +#define GENMASK_UL(lo, hi)     _GENMASK_T(unsigned long, UL, lo, hi)
> >>> +#define GENMASK_ULL(lo, hi)    _GENMASK_T(unsigned long long, ULL, lo, hi)
> >>> +
> >>
> >> These really need to be usable from assembly language, too (in which
> >> case you of course need to not have the cast and suffix), so it probably
> >> should be defined in <linux/const.h> with the other constant macros.
> > 
> > How about that:
> > 
> > #define _GENMASK_T(cast, type, lo, hi) \
> > 	        ((_AT(cast, (_AC(1,type) << ((hi) - (lo) + 1))) - 1) << (lo))
> > #define GENMASK(lo, hi)         _GENMASK_T(unsigned, U, lo, hi)
> > #define GENMASK_UL(lo, hi)      _GENMASK_T(unsigned long, UL, lo, hi)
> > #define GENMASK_ULL(lo, hi)     _GENMASK_T(unsigned long long, ULL, lo, hi)
> > 
> > outside of __KERNEL__ scope?
> > 
> 
(Continue reading)

H. Peter Anvin | 28 Jul 16:18 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/28/2011 07:13 AM, Borislav Petkov wrote:
> 
> You want them to be _GENMASK{,_UL,_ULL}? Why? Because they're "calling"
> macros with a single underscore? Actually, I've always wondered on what
> our exact rules on underscores are. I can deduce the nesting level of
> macros based on number of underscores but what else?
> 

Internal symbols used in userspace-visible headers must be from the
namespace:

	^_[A-Z_].*

	-hpa

--

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Borislav Petkov | 28 Jul 16:35 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Thu, Jul 28, 2011 at 10:18:45AM -0400, H. Peter Anvin wrote:
> On 07/28/2011 07:13 AM, Borislav Petkov wrote:
> > 
> > You want them to be _GENMASK{,_UL,_ULL}? Why? Because they're "calling"
> > macros with a single underscore? Actually, I've always wondered on what
> > our exact rules on underscores are. I can deduce the nesting level of
> > macros based on number of underscores but what else?
> > 
> 
> Internal symbols used in userspace-visible headers must be from the
> namespace:
> 
> 	^_[A-Z_].*

Ah, thanks.

--

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
Avi Kivity | 26 Jul 19:59 2011
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/22/2011 04:15 PM, Borislav Petkov wrote:
> From: Borislav Petkov<borislav.petkov <at> amd.com>
>
> This patch provides performance tuning for the "Bulldozer" CPU. With its
> shared instruction cache there is a chance of generating an excessive
> number of cache cross-invalidates when running specific workloads on the
> cores of a compute module.
>
> This excessive amount of cross-invalidations can be observed if cache
> lines backed by shared physical memory alias in bits [14:12] of their
> virtual addresses, as those bits are used for the index generation.
>
> This patch addresses the issue by zeroing out the slice [14:12] of
> the file mapping's virtual address at generation time, thus forcing
> those bits the same for all mappings of a single shared library across
> processes and, in doing so, avoids instruction cache aliases.
>
> It also adds the kernel command line option
> "unalias_va_addr=(32|64|off)" with which virtual address unaliasing
> can be enabled for 32-bit or 64-bit x86 individually, or be completely
> disabled.
>
> This change leaves virtual region address allocation on other families
> and/or vendors unaffected.
>

Is it possible to derive the bit positions (and the need to mask them) 
from the cpuid description of the cache topology and sizes?

--

-- 
(Continue reading)

Borislav Petkov | 26 Jul 20:13 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

Hi Avi,

On Tue, Jul 26, 2011 at 01:59:04PM -0400, Avi Kivity wrote:
> > This change leaves virtual region address allocation on other families
> > and/or vendors unaffected.
> >
> 
> Is it possible to derive the bit positions (and the need to mask them) 
> from the cpuid description of the cache topology and sizes?

As far as I understand your question, there's no need for deriving the
bit positions because they're not special. You just have to have bits
[14:12] the same across all processes - we simply opted for clearing
them in order to keep the patch as simple as possible. But we could just
as well hashed the library name and generated the bits from it and thus
keep them same per library (we have that version too, btw. :)).

FWIW, in both cases, the patch should fix even the virtualization
scenario with and without KSM.

Does that answer your question?

--

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
(Continue reading)

H. Peter Anvin | 26 Jul 20:16 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/26/2011 11:13 AM, Borislav Petkov wrote:
> Hi Avi,
> 
> On Tue, Jul 26, 2011 at 01:59:04PM -0400, Avi Kivity wrote:
>>> This change leaves virtual region address allocation on other families
>>> and/or vendors unaffected.
>>>
>>
>> Is it possible to derive the bit positions (and the need to mask them) 
>> from the cpuid description of the cache topology and sizes?
> 
> As far as I understand your question, there's no need for deriving the
> bit positions because they're not special. You just have to have bits
> [14:12] the same across all processes - we simply opted for clearing
> them in order to keep the patch as simple as possible. But we could just
> as well hashed the library name and generated the bits from it and thus
> keep them same per library (we have that version too, btw. :)).
> 
> FWIW, in both cases, the patch should fix even the virtualization
> scenario with and without KSM.
> 
> Does that answer your question?
> 

I think the question was the width (and position) for the mask... i.e.
your [14:12] above which *is* magic.

	-hpa

--

-- 
(Continue reading)

Borislav Petkov | 26 Jul 20:37 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Tue, Jul 26, 2011 at 02:16:56PM -0400, H. Peter Anvin wrote:
> >> Is it possible to derive the bit positions (and the need to mask them) 
> >> from the cpuid description of the cache topology and sizes?
> > 
> > As far as I understand your question, there's no need for deriving the
> > bit positions because they're not special. You just have to have bits
> > [14:12] the same across all processes - we simply opted for clearing
> > them in order to keep the patch as simple as possible. But we could just
> > as well hashed the library name and generated the bits from it and thus
> > keep them same per library (we have that version too, btw. :)).
> > 
> > FWIW, in both cases, the patch should fix even the virtualization
> > scenario with and without KSM.
> > 
> > Does that answer your question?
> > 
> 
> I think the question was the width (and position) for the mask... i.e.
> your [14:12] above which *is* magic.

Oh, that's easy: family 15h means bits [14:12] - those bits are used for
I$ index generation.

--

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
(Continue reading)

H. Peter Anvin | 26 Jul 20:38 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/26/2011 11:37 AM, Borislav Petkov wrote:
>>
>> I think the question was the width (and position) for the mask... i.e.
>> your [14:12] above which *is* magic.
> 
> Oh, that's easy: family 15h means bits [14:12] - those bits are used for
> I$ index generation.
> 

In other words, it's completely ad hoc.

	-hpa

--

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Andre Przywara | 26 Jul 21:42 2011
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/26/2011 08:38 PM, H. Peter Anvin wrote:
> On 07/26/2011 11:37 AM, Borislav Petkov wrote:
>>>
>>> I think the question was the width (and position) for the mask... i.e.
>>> your [14:12] above which *is* magic.
>>
>> Oh, that's easy: family 15h means bits [14:12] - those bits are used for
>> I$ index generation.
>>
>
> In other words, it's completely ad hoc.

There is no need to determine it by calculating, because it caused by 
the special design of the BD L1 cache and thus fixed.
And a calculation would be even more confusing:

The L1I is virtually indexed, but physically tagged.
64KB L1I cache, 64 Bytes per Cacheline = 1024 cache lines
1024 lines / 2 way associative = 512 indexes
64 Bytes per Cacheline (6 bits) + 512 indexes (9 bits) = bits [14:0]
virtual and physical addresses are the same for bits [11:0], which 
leaves the remaining 14:12 susceptible for aliasing.

So bit 12 comes from PAGESIZE and yes, the 14 could be derived from the 
CPUID cache info, but I don't see much value in breaking this down this way.
But I agree that there should be some comment in the patch which at 
least notes that bits [14:12] are due to the L1I design, maybe we can 
copy a nicer version of the above math in the commit message for reference.

Regards,
(Continue reading)

H. Peter Anvin | 27 Jul 00:34 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/26/2011 12:42 PM, Andre Przywara wrote:
>>
>> In other words, it's completely ad hoc.
> 
> There is no need to determine it by calculating, because it caused by 
> the special design of the BD L1 cache and thus fixed.
> And a calculation would be even more confusing:
> 
> The L1I is virtually indexed, but physically tagged.
> 64KB L1I cache, 64 Bytes per Cacheline = 1024 cache lines
> 1024 lines / 2 way associative = 512 indexes
> 64 Bytes per Cacheline (6 bits) + 512 indexes (9 bits) = bits [14:0]
> virtual and physical addresses are the same for bits [11:0], which 
> leaves the remaining 14:12 susceptible for aliasing.
> 
> So bit 12 comes from PAGESIZE and yes, the 14 could be derived from the 
> CPUID cache info, but I don't see much value in breaking this down this way.
> But I agree that there should be some comment in the patch which at 
> least notes that bits [14:12] are due to the L1I design, maybe we can 
> copy a nicer version of the above math in the commit message for reference.
> 

The right thing to do this would be to have a bit in CPUID which would
indicate that the kernel has to perform the above calculation -- because
obviously not all CPUs have this issue.  From that we can calculate a
mask to insert into whatever appropriate location... depending on the
exact tack we have to apply to deal with this situation.

	-hpa

(Continue reading)

Avi Kivity | 27 Jul 06:14 2011
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/26/2011 10:42 PM, Andre Przywara wrote:
>
> There is no need to determine it by calculating, because it caused by 
> the special design of the BD L1 cache and thus fixed.
> And a calculation would be even more confusing:
>
> The L1I is virtually indexed, but physically tagged.
> 64KB L1I cache, 64 Bytes per Cacheline = 1024 cache lines
> 1024 lines / 2 way associative = 512 indexes
> 64 Bytes per Cacheline (6 bits) + 512 indexes (9 bits) = bits [14:0]
> virtual and physical addresses are the same for bits [11:0], which 
> leaves the remaining 14:12 susceptible for aliasing.
>
> So bit 12 comes from PAGESIZE and yes, the 14 could be derived from 
> the CPUID cache info, but I don't see much value in breaking this down 
> this way.
> But I agree that there should be some comment in the patch which at 
> least notes that bits [14:12] are due to the L1I design, maybe we can 
> copy a nicer version of the above math in the commit message for 
> reference.
>

If among the 12,432.8 cpuid leaves exposed by the cpu we had a bit that 
said L1I was shared, and another that said it was virtually indexed, and 
others describing the cache size, cache line size, and number of ways, 
then we could perform the arithmetic at runtime, yes?

That means that if the caches grow or increase their associativity, then 
we don't need to patch the kernel again.

(Continue reading)

Borislav Petkov | 27 Jul 08:21 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Wed, Jul 27, 2011 at 12:14:26AM -0400, Avi Kivity wrote:
> On 07/26/2011 10:42 PM, Andre Przywara wrote:
> >
> > There is no need to determine it by calculating, because it caused by 
> > the special design of the BD L1 cache and thus fixed.
> > And a calculation would be even more confusing:
> >
> > The L1I is virtually indexed, but physically tagged.
> > 64KB L1I cache, 64 Bytes per Cacheline = 1024 cache lines
> > 1024 lines / 2 way associative = 512 indexes
> > 64 Bytes per Cacheline (6 bits) + 512 indexes (9 bits) = bits [14:0]
> > virtual and physical addresses are the same for bits [11:0], which 
> > leaves the remaining 14:12 susceptible for aliasing.
> >
> > So bit 12 comes from PAGESIZE and yes, the 14 could be derived from 
> > the CPUID cache info, but I don't see much value in breaking this down 
> > this way.
> > But I agree that there should be some comment in the patch which at 
> > least notes that bits [14:12] are due to the L1I design, maybe we can 
> > copy a nicer version of the above math in the commit message for 
> > reference.
> >
> 
> If among the 12,432.8 cpuid leaves exposed by the cpu we had a bit that 
> said L1I was shared, and another that said it was virtually indexed, and 
> others describing the cache size, cache line size, and number of ways, 
> then we could perform the arithmetic at runtime, yes?
> 
> That means that if the caches grow or increase their associativity, then 
> we don't need to patch the kernel again.
(Continue reading)

Ingo Molnar | 27 Jul 08:59 2011
Picon
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue


* Borislav Petkov <bp <at> amd64.org> wrote:

> On Wed, Jul 27, 2011 at 12:14:26AM -0400, Avi Kivity wrote:
> > On 07/26/2011 10:42 PM, Andre Przywara wrote:
> > >
> > > There is no need to determine it by calculating, because it caused by 
> > > the special design of the BD L1 cache and thus fixed.
> > > And a calculation would be even more confusing:
> > >
> > > The L1I is virtually indexed, but physically tagged.
> > > 64KB L1I cache, 64 Bytes per Cacheline = 1024 cache lines
> > > 1024 lines / 2 way associative = 512 indexes
> > > 64 Bytes per Cacheline (6 bits) + 512 indexes (9 bits) = bits [14:0]
> > > virtual and physical addresses are the same for bits [11:0], which 
> > > leaves the remaining 14:12 susceptible for aliasing.
> > >
> > > So bit 12 comes from PAGESIZE and yes, the 14 could be derived from 
> > > the CPUID cache info, but I don't see much value in breaking this down 
> > > this way.
> > > But I agree that there should be some comment in the patch which at 
> > > least notes that bits [14:12] are due to the L1I design, maybe we can 
> > > copy a nicer version of the above math in the commit message for 
> > > reference.
> > >
> > 
> > If among the 12,432.8 cpuid leaves exposed by the cpu we had a bit that 
> > said L1I was shared, and another that said it was virtually indexed, and 
> > others describing the cache size, cache line size, and number of ways, 
> > then we could perform the arithmetic at runtime, yes?
(Continue reading)

Avi Kivity | 27 Jul 11:30 2011
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/27/2011 09:59 AM, Ingo Molnar wrote:
> >
> >  Yeah, actually the idea is to patch the kernel only this one time
> >  and never ever be needing to do this for future CPUs, for this
> >  matter.
>
> But the check you have added turns this workaround on for all family
> 0x15 CPUs. If a family 0x15 CPU is built with a larger or more
> associative cache, one that does not need the workaround, we would
> still turn on the workaround.

Similarly, if family 0x21 CPUs have a similar arrangement (with 
identical or different cache sizes and associativity) the hardcoded 
check will fail.

--

-- 
error compiling committee.c: too many arguments to function

Borislav Petkov | 27 Jul 17:37 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Wed, Jul 27, 2011 at 05:30:56AM -0400, Avi Kivity wrote:
> On 07/27/2011 09:59 AM, Ingo Molnar wrote:
> > >
> > >  Yeah, actually the idea is to patch the kernel only this one time
> > >  and never ever be needing to do this for future CPUs, for this
> > >  matter.
> >
> > But the check you have added turns this workaround on for all family
> > 0x15 CPUs. If a family 0x15 CPU is built with a larger or more
> > associative cache, one that does not need the workaround, we would
> > still turn on the workaround.
> 
> Similarly, if family 0x21 CPUs have a similar arrangement (with 
> identical or different cache sizes and associativity) the hardcoded 
> check will fail.

So, in short, I$ design is the same for all F15h processors so a family
0x15 check suffices here.

--

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
Avi Kivity | 27 Jul 17:45 2011
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/27/2011 06:37 PM, Borislav Petkov wrote:
> On Wed, Jul 27, 2011 at 05:30:56AM -0400, Avi Kivity wrote:
> >  On 07/27/2011 09:59 AM, Ingo Molnar wrote:
> >  >  >
> >  >  >   Yeah, actually the idea is to patch the kernel only this one time
> >  >  >   and never ever be needing to do this for future CPUs, for this
> >  >  >   matter.
> >  >
> >  >  But the check you have added turns this workaround on for all family
> >  >  0x15 CPUs. If a family 0x15 CPU is built with a larger or more
> >  >  associative cache, one that does not need the workaround, we would
> >  >  still turn on the workaround.
> >
> >  Similarly, if family 0x21 CPUs have a similar arrangement (with
> >  identical or different cache sizes and associativity) the hardcoded
> >  check will fail.
>
> So, in short, I$ design is the same for all F15h processors so a family
> 0x15 check suffices here.

What about F21h processors?  I see you're not checking them.

Is there any reason the check should not be made based on topology?

--

-- 
error compiling committee.c: too many arguments to function

Borislav Petkov | 27 Jul 17:49 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Wed, Jul 27, 2011 at 11:45:00AM -0400, Avi Kivity wrote:
> What about F21h processors?  I see you're not checking them.
> 
> Is there any reason the check should not be made based on topology?

Because this patch addresses a performance improvement for the F15h
microarchitecture - that doesn't have anything to do with cache
topology.

--

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
Avi Kivity | 27 Jul 17:57 2011
Picon

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On 07/27/2011 06:49 PM, Borislav Petkov wrote:
> On Wed, Jul 27, 2011 at 11:45:00AM -0400, Avi Kivity wrote:
> >  What about F21h processors?  I see you're not checking them.
> >
> >  Is there any reason the check should not be made based on topology?
>
> Because this patch addresses a performance improvement for the F15h
> microarchitecture - that doesn't have anything to do with cache
> topology.

Okay.

Out of curiosity, what's the performance impact if the workaround is not 
enabled?

--

-- 
error compiling committee.c: too many arguments to function

Borislav Petkov | 27 Jul 18:42 2011

Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue

On Wed, Jul 27, 2011 at 11:57:45AM -0400, Avi Kivity wrote:
> Out of curiosity, what's the performance impact if the workaround is
> not enabled?

Up to 3% for a CPU-intensive style benchmark, and it can vary highly in
a microbenchmark depending on workload and compiler.

--

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Gmane