Re: [RFC PATCH 1/3] x86: Add another set of MSR accessor functions

[Borislav Petkov <bp@alien8.de>]

On Mon, Feb 17, 2014 at 08:21:47AM -0800, H. Peter Anvin wrote:
> Good patch series overall, but I do have some issues with this one:
> 
> On 02/09/2014 05:48 AM, Borislav Petkov wrote:
> > + */
> > +int msr_read(u32 msr, struct msr *m)
> > +{
> > +	int err;
> > +	u64 val;
> > +
> > +	val = native_read_msr_safe(msr, &err);
> 
> I don't think we should use the native_ function here.

Ah, right, pv gunk, I conveniently victimized those cache lines away
from my head. :-P

rdmsrl_safe it is.

> 
> > +	if (err)
> > +		pr_warn("%s: Error reading MSR 0x%08x\n", __func__, msr);
> > +	else
> > +		m->q = val;
> 
> I also don't think we should print a message if the MSR doesn't exist.
> This will be a normal occurrence in a number of flows.

Right, I was suspecting the screaming in dmesg could upset people but
wasn't sure. Good point.

> > +static int __flip_bit(u32 msr, u8 bit, bool set)
> > +{
> > +	struct msr m;
> > +
> > +	if (bit > 63)
> > +		return -1;
> 
> Feels a bit excessive, but I'd suggest returning -EINVAL instead.

Ok.

> I would suggest explicitly making this an inline function.

Sure.

> > +	if (msr_read(msr, &m))
> > +		return -1;
> 
> Return -EIO?

Actually, msr_read already gives a retval so I can simply carry that
out. And it *is* -EIO already :-)

> How about:
> 
> 	m1 = m;
> 	if (set)
> 		m1.q |= BIT_64(bit);
> 	else
> 		m1.q &= ~BIT_64(bit);
> 
> 	if (m1.q != m.q) {
> 		if (msr_write(...))
> 			...

It's all the same to me, sure.

> Again, I'm not sure if printing a message here makes sense. In fact,
> this is the second message you print for the same thing.

Ok, I'll make them completely silent - if their users wanna say
something, they can do that based on the retval. Good.

Thanks for checking them out - I'll start playing with the revised
versions on real hw.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--