lkml.org 
[lkml]   [2014]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 1/3] x86: Add another set of MSR accessor functions
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.
--


\
 
 \ /
  Last update: 2014-02-17 22:22    [W:0.159 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site