amd64_edac making improper assumptions?

amd64_edac making improper assumptions?

[Jan Beulich]

Doug, Borislav,

the sizing of pvt_lookup[] and mci_lookup[] is done based on a config
setting (CONFIG_NODES_SHIFT), and the indexing happens using the
raw value read from hardware without any bounds checking. Hence,
running a kernel with e.g. !CONFIG_NUMA on a multi-socket system
would happily access other than the first and only array element.

Likewise, the use of rdmsr_on_cpu() doesn't seem to do what is
intended when !CONFIG_NUMA.

Also, assuming you can use cpumask_of_node() directly on the
node ID read from hardware seems bogus (even in general, but
namely again when MAX_NUMNODES is less than the actual number
of nodes).

Thanks for your explanation,
Jan

Re: amd64_edac making improper assumptions?

[Borislav Petkov]

Hi Jan,

On Fri, Sep 18, 2009 at 02:02:28PM +0100, Jan Beulich wrote:
> Doug, Borislav,
> 
> the sizing of pvt_lookup[] and mci_lookup[] is done based on a config
> setting (CONFIG_NODES_SHIFT), and the indexing happens using the
> raw value read from hardware without any bounds checking. Hence,
> running a kernel with e.g. !CONFIG_NUMA on a multi-socket system
> would happily access other than the first and only array element.

Good catch, will fix, thanks. See below for longer explanation.

> Likewise, the use of rdmsr_on_cpu() doesn't seem to do what is
> intended when !CONFIG_NUMA.
> 
> Also, assuming you can use cpumask_of_node() directly on the
> node ID read from hardware seems bogus (even in general, but
> namely again when MAX_NUMNODES is less than the actual number
> of nodes).

originally, the driver was compiling in NUMA for facilities like
cpumask_of_node() et al for all per-node stuff but Ingo raised a concern
that the NUMA dependency shouldn't be needed. Therefore, we needed some
alternative facilities which, for example, can give you all the cores
on a node so that you can init a driver instance per node and thus per
memory controller.

Well, Andreas did prepare a bunch of generic cpu topology patches which
handle also MCM processors (e.g. having more than one internal node) and
currently the per-CPU cpu_llc_id var contains the node id of a cpu. They
went upstream last week and I'm waiting for another patch in -tip to go
in before I send my local queue here which removes the cpumask_of_node()
usage and all the rest of NUMA stuff completely.

Hope that makes it more clear and thanks for letting us know, we're on
it :).

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632