linux-kernel - Re: [PATCH 11/24] x86/oprofile: Add IBS support for AMD CPUs, model specific code

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]

Message-ID: <20080726100341.GF25890@elte.hu>
Date:	Sat, 26 Jul 2008 12:03:41 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Robert Richter <robert.richter@....com>
Cc:	Barry Kasindorf <barry.kasindorf@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	oprofile-list <oprofile-list@...ts.sourceforge.net>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 11/24] x86/oprofile: Add IBS support for AMD CPUs,
	model specific code


* Robert Richter <robert.richter@....com> wrote:

> @@ -132,6 +214,65 @@ static int op_amd_check_ctrs(struct pt_regs * const regs,
>  		}
>  	}
>  
> +	/*If AMD and IBS is available */
> +	if (ibs_allowed && ibs_config.fetch_enabled) {
> +		rdmsr(MSR_AMD64_IBSFETCHCTL, low, high);
> +		if (high & IBS_FETCH_VALID_BIT) {
> +			ibs_fetch.ibs_fetch_ctl_high = high;
> +			ibs_fetch.ibs_fetch_ctl_low = low;
> +			rdmsr(MSR_AMD64_IBSFETCHLINAD, low, high);
> +			ibs_fetch.ibs_fetch_lin_addr_high = high;
> +			ibs_fetch.ibs_fetch_lin_addr_low = low;
> +			rdmsr(MSR_AMD64_IBSFETCHPHYSAD, low, high);
> +			ibs_fetch.ibs_fetch_phys_addr_high = high;
> +			ibs_fetch.ibs_fetch_phys_addr_low = low;
> +
> +			oprofile_add_ibs_sample(regs,
> +						(unsigned int *)&ibs_fetch,
> +						IBS_FETCH_BEGIN);

please move this innermost code into a helper function. That will help 
eliminate line 80 artifacts as well.

> +	if (ibs_allowed && ibs_config.op_enabled) {
> +		rdmsr(MSR_AMD64_IBSOPCTL, low, high);
> +		if (low & IBS_OP_VALID_BIT) {
> +			rdmsr(MSR_AMD64_IBSOPRIP, low, high);
> +			ibs_op.ibs_op_rip_low = low;
> +			ibs_op.ibs_op_rip_high = high;
> +			rdmsr(MSR_AMD64_IBSOPDATA, low, high);
> +			ibs_op.ibs_op_data1_low = low;
> +			ibs_op.ibs_op_data1_high = high;
> +			rdmsr(MSR_AMD64_IBSOPDATA2, low, high);
> +			ibs_op.ibs_op_data2_low = low;
> +			ibs_op.ibs_op_data2_high = high;
> +			rdmsr(MSR_AMD64_IBSOPDATA3, low, high);
> +			ibs_op.ibs_op_data3_low = low;
> +			ibs_op.ibs_op_data3_high = high;
> +			rdmsr(MSR_AMD64_IBSDCLINAD, low, high);
> +			ibs_op.ibs_dc_linear_low = low;
> +			ibs_op.ibs_dc_linear_high = high;
> +			rdmsr(MSR_AMD64_IBSDCPHYSAD, low, high);
> +			ibs_op.ibs_dc_phys_low = low;
> +			ibs_op.ibs_dc_phys_high = high;
> +
> +			/* reenable the IRQ */
> +			oprofile_add_ibs_sample(regs,
> +						(unsigned int *)&ibs_op,
> +						IBS_OP_BEGIN);
> +			rdmsr(MSR_AMD64_IBSOPCTL, low, high);
> +			low &= ~(IBS_OP_VALID_BIT);
> +			low |= IBS_OP_ENABLE;
> +			wrmsr(MSR_AMD64_IBSOPCTL, low, high);
> +		}
> +	}

ditto.

> +		/**** Be sure to run loop until NULL is returned to
> +		decrement reference count on any pci_dev structures
> +		returned ****/

please use standard comment style:

 /*
  * Multi-line ..........
  * ....................... comment:
  */

> +		while ((gh_device = pci_get_device(PCI_VENDOR_ID_AMD,
> +			PCI_DEVICE_ID_AMD_10H_NB_MISC, gh_device))
> +			!= NULL) {
> +			/* This code may change if we can find a proper
> +			* way to get at the PCI extended config space */

ditto.

> +			pci_write_config_dword(
> +				gh_device, IBS_LVT_OFFSET_PCI,
> +				(vector | IBS_CTL_LVT_OFFSET_VALID_BIT));

perhaps use a helper function to reduce col 80 artifacts.

> +/*
> + * unitialize the APIC for the IBS interrupts if needed on AMD Family10h
> + * rev B0 and later */

comment style.

> +static void setup_ibs_files(struct super_block *sb, struct dentry *root)
> +{
> +	char buf[12];

magic limit.

> +	struct dentry *dir;
> +
> +	if (!ibs_allowed)
> +		return;
> +
> +	/* setup some reasonable defaults */
> +	ibs_config.max_cnt_fetch = 250000;
> +	ibs_config.fetch_enabled = 0;
> +	ibs_config.max_cnt_op = 250000;
> +	ibs_config.op_enabled = 0;
> +	ibs_config.dispatched_ops = 1;
> +	snprintf(buf,  sizeof(buf), "ibs_fetch");
> +	dir = oprofilefs_mkdir(sb, root, buf);

you could make use of a small style trick here: use a newline before 
this statement, to make blocks of code stand out in a clearer way. The 
construction of 'buf' is finished after the snprintf above, then comes 
the next step, to create the oprofilefs entries.

	Ingo
--
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/