linux-kernel - Re: [patch 3/3] x86: cache_info: Kill the atomic allocation in amd_init_l3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 24 Jul 2011 12:27:34 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
	Hans Rosenfeld <hans.rosenfeld@....com>,
	Borislav Petkov <borislav.petkov@....com>,
	Andreas Herrmann <andreas.herrmann3@....com>,
	Mike Travis <travis@....com>
Subject: Re: [patch 3/3] x86: cache_info: Kill the atomic allocation in
 amd_init_l3_cache()

On Sun, Jul 24, 2011 at 09:46:09AM -0000, Thomas Gleixner wrote:
> It's not a good reason to allocate memory in the smp function call
> just because someone thought it's the most conveniant place.
> 
> The AMD L3 data is coupled to the northbridge info by a pointer to the
> corresponding north bridge data. So allocating it with the northbridge
> data and referencing the northbridge in the cache_info code instead
> uses less memory and gets rid of that atomic allocation hack in the
> smp function call.

Nice, much better.

See for a minor nitpick below.

> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  arch/x86/include/asm/amd_nb.h         |    6 ++
>  arch/x86/kernel/cpu/intel_cacheinfo.c |   74 +++++++++++-----------------------
>  2 files changed, 32 insertions(+), 48 deletions(-)
> 
> Index: linux-2.6/arch/x86/include/asm/amd_nb.h
> ===================================================================
> --- linux-2.6.orig/arch/x86/include/asm/amd_nb.h
> +++ linux-2.6/arch/x86/include/asm/amd_nb.h
> @@ -19,9 +19,15 @@ extern int amd_numa_init(void);
>  extern int amd_get_subcaches(int);
>  extern int amd_set_subcaches(int, int);
>  
> +struct amd_l3_cache {
> +	unsigned indices;
> +	u8	 subcaches[4];
> +};
> +
>  struct amd_northbridge {
>  	struct pci_dev *misc;
>  	struct pci_dev *link;
> +	struct amd_l3_cache l3_cache;
>  };
>  
>  struct amd_northbridge_info {
> Index: linux-2.6/arch/x86/kernel/cpu/intel_cacheinfo.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ linux-2.6/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -151,18 +151,12 @@ union _cpuid4_leaf_ecx {
>  	u32 full;
>  };
>  
> -struct amd_l3_cache {
> -	struct	 amd_northbridge *nb;
> -	unsigned indices;
> -	u8	 subcaches[4];
> -};
> -
>  struct _cpuid4_info_regs {
>  	union _cpuid4_leaf_eax eax;
>  	union _cpuid4_leaf_ebx ebx;
>  	union _cpuid4_leaf_ecx ecx;
>  	unsigned long size;
> -	struct amd_l3_cache *l3;
> +	struct amd_northbridge *nb;
>  };
>  
>  struct _cpuid4_info {
> @@ -309,12 +303,13 @@ struct _cache_attr {
>  /*
>   * L3 cache descriptors
>   */
> -static void __cpuinit amd_calc_l3_indices(struct amd_l3_cache *l3)
> +static void __cpuinit amd_calc_l3_indices(struct amd_northbridge *nb)
>  {
> +	struct amd_l3_cache *l3 = &nb->l3_cache;
>  	unsigned int sc0, sc1, sc2, sc3;
>  	u32 val = 0;
>  
> -	pci_read_config_dword(l3->nb->misc, 0x1C4, &val);
> +	pci_read_config_dword(nb->misc, 0x1C4, &val);
>  
>  	/* calculate subcache sizes */
>  	l3->subcaches[0] = sc0 = !(val & BIT(0));
> @@ -328,33 +323,16 @@ static void __cpuinit amd_calc_l3_indice
>  static void __cpuinit amd_init_l3_cache(struct _cpuid4_info_regs *this_leaf,
>  					int index)
>  {
> -	static struct amd_l3_cache *__cpuinitdata l3_caches;
>  	int node;
>  
>  	/* only for L3, and not in virtualized environments */
> -	if (index < 3 || amd_nb_num() == 0)
> +	if (index < 3)
>  		return;

AFAICT, we still need the "amd_nb_num() == 0" check for xen because it
doesn't export NB PCI devices to the guest, see f2b20e41...

>  
> -	/*
> -	 * Strictly speaking, the amount in @size below is leaked since it is
> -	 * never freed but this is done only on shutdown so it doesn't matter.
> -	 */
> -	if (!l3_caches) {
> -		int size = amd_nb_num() * sizeof(struct amd_l3_cache);
> -
> -		l3_caches = kzalloc(size, GFP_ATOMIC);
> -		if (!l3_caches)
> -			return;
> -	}
> -
>  	node = amd_get_nb_id(smp_processor_id());
> -
> -	if (!l3_caches[node].nb) {
> -		l3_caches[node].nb = node_to_amd_nb(node);
> -		amd_calc_l3_indices(&l3_caches[node]);
> -	}
> -
> -	this_leaf->l3 = &l3_caches[node];
> +	this_leaf->nb = node_to_amd_nb(node);

although, on a second thought, node_to_amd_nb(node) should return NULL
since the AMD NB caching code shouldnt've enumerated any NB devices and

> +	if (this_leaf->nb && !this_leaf->nb->l3_cache.indices)
> +		amd_calc_l3_indices(this_leaf->nb);

this check should fail. Hm.

Let me test the patchset on Monday to verify there are no other subtle
interactions I haven't thought of right now.

Thanks.

-- 
Regards/Gruss,
    Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/