linux-kernel - RE: [PATCH] [v4] x86, suspend: Save/restore extra MSR registers for suspend

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]

Date:	Sun, 11 Oct 2015 02:43:11 +0000
From:	"Chen, Yu C" <yu.c.chen@...el.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
CC:	"pavel@....cz" <pavel@....cz>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"hpa@...or.com" <hpa@...or.com>, "bp@...en8.de" <bp@...en8.de>,
	"Zhang, Rui" <rui.zhang@...el.com>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] [v4] x86, suspend: Save/restore extra MSR registers for
 suspend

Hi Rafael, thanks for your comment,

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@...ysocki.net]
> Sent: Saturday, October 10, 2015 5:50 AM
> To: Chen, Yu C
> Cc: pavel@....cz; tglx@...utronix.de; mingo@...hat.com; hpa@...or.com;
> bp@...en8.de; Zhang, Rui; linux-pm@...r.kernel.org; x86@...nel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH] [v4] x86, suspend: Save/restore extra MSR registers for
> suspend
> 
> On Thursday, August 27, 2015 11:18:27 AM Chen Yu wrote:
> >
> > +struct msr_type {
> 
> I'd call this msr_data.
> 
OK, will change its name. But msr_data is already defined
in kvm_host.h , so I changed it to msr_save_data in the new patch version.

> > +	bool msr_saved;
> > +	struct msr_info rv;
> > +};
> > +
> > +struct saved_msr {
> 
> And this msr_context.
> 
OK, will do.
> > +	unsigned short num;
> > +	struct msr_type *msr_array;
> > +};
> > +
> >  /* image of the saved processor state */  struct saved_context {
> >  	u16 es, fs, gs, ss;
> >  	unsigned long cr0, cr2, cr3, cr4;
> >  	u64 misc_enable;
> >  	bool misc_enable_saved;
> > +	struct saved_msr msr_for_save;
> 
> "msr_to_save"?
> 
OK, will change.

> > +struct msr_type {
> > +	bool msr_saved;
> > +	struct msr_info rv;
> > +};
> > +
> > +struct saved_msr {
> > +	unsigned short num;
> > +	struct msr_type *msr_array;
> > +};
> 
> The definitions look the same as the previous ones.
> 
> Can we share them somehow?
> 
Yes, I've moved the common code to arch/x86/include/asm/msr.h.
> > +static void msr_save_context(struct saved_context *ctxt) {
> > +	int i = 0;
> > +
> > +	for (i = 0; i < ctxt->msr_for_save.num; i++) {
> > +		struct msr_type *msr = &ctxt->msr_for_save.msr_array[i];
> > +
> > +		msr->msr_saved = !rdmsrl_safe(msr->rv.msr_no,
> > +			&msr->rv.reg.q);
> > +	}
> 
> If you did something like
> 
> 	struct msr_type *msr = ctxt->msr_for_save.msr_array;
> 	struct msr_type *end = msr + ctxt->msr_for_save.num;
> 
> 	while (msr < end) {
> 		msr->msr_saved = !rdmsrl_safe(msr->rv.msr_no, &msr-
> >rv.reg.q);
> 		msr++;
> 	}
> 
> here (and analogously below), it would be somewhat easier to follow IMO.
> 
OK, changed the code.

> > +/* We constrain the number of MSRs to 64. */
> 
> Why 64 in particular?
> 
Once I looked up the SDM and estimate that 64 should be enough for some important
MSR registers. 
> > +#define MAX_MSR_SAVED	64
> > +
> > +static struct msr_type msr_context_array[MAX_MSR_SAVED];
> 
> I wonder if this array may be allocated dynamically?
> 
> We'll waste memory here in the majority of cases.
> 
OK, I've changed it to use kmalloc_array for storing.

> > +
> > +/*
> > + * Following section is a quirk framework for problematic BIOS:
> 
> "The following ..."
> 
OK.
> > + * Sometimes MSRs are modified by BIOS after suspended to
> > + * ram, this might cause unexpected behavior after resumed.
> 
> "RAM" (in capitals) and "during resume" or "after wakeup".
> 
OK. will do.

> > + * Thus we save/restore these specified MSRs during suspending
> > + * in order to work around it.
> > + * A typical bug is reported at:
> > + * https://bugzilla.redhat.com/show_bug.cgi?id=1227208
> > + */
> > +static int msr_set_info(const u32 *msr_id, const int total_num)
> 
> I'd call it "msr_init_context" or something like that.
> 
OK, changed it to msr_init_context.
> Thanks,
> Rafael

Best Regards,
Yu