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

[Chen, Yu C]

Hi Rafael, thanks for your comment,

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx]
> Sent: Saturday, October 10, 2015 5:50 AM
> To: Chen, Yu C
> Cc: pavel@xxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx;
> bp@xxxxxxxxx; Zhang, Rui; linux-pm@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå