RE: [PATCH] [v2] x86, suspend: Save/restore extra MSR registers for suspend

[Chen, Yu C]

Hi, Boris,thanks for your review

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@xxxxxxxxx]
> Sent: Monday, August 24, 2015 4:50 PM
> To: Chen, Yu C
> Cc: rjw@xxxxxxxxxxxxx; pavel@xxxxxx; tglx@xxxxxxxxxxxxx;
> mingo@xxxxxxxxxx; hpa@xxxxxxxxx; Zhang, Rui; lenb@xxxxxxxxxx;
> x86@xxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] [v2] x86, suspend: Save/restore extra MSR registers for
> suspend
> 
> On Fri, Aug 21, 2015 at 07:53:34PM +0800, Chen Yu wrote:
> > +struct msr_type {
> > +	unsigned int msr_id;
> > +	bool msr_saved;
> > +	u64 msr_value;
> > +};
> 
> These definitions look awfully close to struct msr_info in include/asm/msr.h
> 
> Maybe reuse them instead of growing yet another type...
> 
OK,  I'll use msr_info instead of  msr_id and msr_value:
struct msr_type {
	bool msr_saved;
	struct msr_info rv;
};

> > +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];
> 
> No need for the line breaks here, let them stick out for better readability.
> 
OK, will do.
> > +		msr->msr_saved = !rdmsrl_safe(msr->msr_id,
> > +			&msr->msr_value);
> > +	}
> > +		struct msr_type *msr =
> > +			&ctxt->msr_for_save.msr_array[i];
> > +		if (msr->msr_saved)
> > +			wrmsrl(msr->msr_id, msr->msr_value);
> 
> Ditto.
> 
OK, will do.


Best Regards,
Yu