linux-kernel - RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <37D7C6CF3E00A74B8858931C1DB2F0770588B1D0@shsmsx102.ccr.corp.intel.com>
Date:	Wed, 17 Feb 2016 21:57:35 +0000
From:	"Liang, Kan" <kan.liang@...el.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>,
	Stephane Eranian <eranian@...gle.com>,
	"Chegondi, Harish" <harish.chegondi@...el.com>,
	"Kleen, Andi" <andi.kleen@...el.com>
Subject: RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit


> 
> > > @@ -201,6 +201,11 @@ static void nhmex_uncore_msr_init_box(st
> > >  	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL,
> > > NHMEX_U_PMON_GLOBAL_EN_ALL);  }
> 
> > > +static void nhmex_uncore_msr_exit_box(struct intel_uncore_box
> *box)
> > > {
> > > +	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL, 0); }
> 
> The reset value for this register is 0. So how is that wrong?
> 
> > > +static void snb_uncore_msr_exit_box(struct intel_uncore_box *box)
> {
> > > +	if (box->pmu->pmu_idx == 0)
> > > +		wrmsrl(SNB_UNC_PERF_GLOBAL_CTL, 0); }
> 
> Ditto.
> 
> > > +static void snb_uncore_imc_exit_box(struct intel_uncore_box *box) {
> > > +	iounmap(box->io_addr);
> 
> That's definitely required, because it would leak a mapping.
> 
> I know Intel folks do not care about error handling and a few reference
> leaks, but I care very much.
> 
> If there is a single instance of exit_box() in that patch which flips the wrong
> bits, then please point it out with the proper reference in the manual and
> not with such half baken statements as above.
> 

Sorry, I didn't make it clear.
For the older server platforms like nhmex and client platforms like snb, I agree
with you on nhmex_uncore_msr_exit_box and snb_uncore_imc_exit_box.

However, for newer server platforms (start from IVB server), we cannot write 0
to rsv bit of BOX control registers. The behavior is undefined.
The following codes may have issues.
It looks we also write 0 to rsv bit in box_init. We may need to fix it.

You can find all the server uncore documents here.
https://software.intel.com/en-us/blogs/2014/07/11/documentation-for-uncore-performance-monitoring-units

> +static void snbep_uncore_pci_exit_box(struct intel_uncore_box *box) {
> +	struct pci_dev *pdev = box->pci_dev;
> +	int box_ctl = uncore_pci_box_ctl(box);
> +
> +	pci_write_config_dword(pdev, box_ctl, 0); }
> +

> +static void snbep_uncore_msr_exit_box(struct intel_uncore_box *box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}

> +static void ivbep_uncore_msr_exit_box(struct intel_uncore_box *box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}

> +static void ivbep_uncore_pci_exit_box(struct intel_uncore_box *box) {
> +	struct pci_dev *pdev = box->pci_dev;
> +
> +	pci_write_config_dword(pdev, SNBEP_PCI_PMON_BOX_CTL, 0); }
> +

> +static void hswep_uncore_sbox_msr_exit_box(struct intel_uncore_box
> +*box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	/* CHECKME: Does this need the bit dance like init() ? */
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}
> +

Thanks,

Kan