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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
