On Tue, Nov 24, 2015 at 7:19 AM, Barret Rhoden <[email protected]> wrote:
> Roughly. That also needs to unlock if we don't take the branch. I
> think this would work:
>
> void completion_wait(struct completion *comp)
> {
> int8_t state;
>
> cv_lock_irqsave(&comp->cv, &state);
> if (comp->count > 0) {
> cv_wait_and_unlock(&comp->cv);
> enable_irqsave(&state);
> } else {
> cv_unlock_irqsave(&comp->cv, &state);
> }
> }
>
Right, done.
>
> > > > +static inline void cpu_set_setcpu(struct cpu_set *cset, unsigned
> > > > int
> > > cpuno)
> > > > +{
> > > > + cset->cpus[cpuno / CPUSET_INT_BITS] |= 1 << (cpuno %
> > > CPUSET_INT_BITS);
> > > > +}
> > >
> > > This stuff all looks correct. But we should use bitops for it
> > > instead of rolling our own. We brought in many of Linux's bitops
> > > (k/i/bitops.h, k/a/x/bitops.h). If there's a general-purpose bitop
> > > we're missing that you need, then we should add it to bitops.h
> > >
> >
> > It is missing everything. get/set/clear
>
> I think we were missing get_bit, but we have __set_bit and __clear_bit
> (the __ are the non-atomic versions).
>
Oh, I missed the *arch* bitops.h file 😀
Undoing the mem_ ... adding get_bit()
> > > > + n = pwrite(fd, &tmp, sizeof(uint64_t), 0x309);
> > > > + UT_ASSERT_M("Failed to write MSR values to 0x309",
> > > > + n == sizeof(uint64_t));
> > >
> > > Will these tests always be passing? What are those MSRs, and why
> > > do we allow
> > > the user to read / write them?
> > >
> >
> > The 0x309 is a counter value register. Well, if we need to test
> > write(), we need to write somewhere.
>
> Sorry if I was unclear, I was mostly looking for a #define naming the
> register.
>
Oh, OK.
> > +/* White list entries needs to be ordered by start address, and
> > > never overlap.
> > > > + */
> > > > +static const struct address_range msr_rd_wlist[] = {
> > > > + ADDRESS_RANGE(0x00000000, 0xffffffff),
> > > > +};
> > >
> > > I like the use of a whitelist, but we need to actually pick what we
> > > want userspace to be able to read. They could give us a garbage
> > > MSR and probably
> > > trigger a GPF or something nasty.
> > >
> >
> > Basically, MSR varies from CPU family to CPU family.
>
> Do the values vary for a particular MSR, or just that the availability
> of MSRs vary based on the family? I was under the impression that it's
> the latter (availability varies, not the value). In that case, we can
> be explicit about what MSRs we're giving access to.
>
The number of MSRs depends on the number of counters.
I will try to add #define based ranges there, and we can defer the MSR
device access policy to when we have a policy.
>
> > In Linux, if you have permissions, you can read/write whatever you
> > want.
>
> I guess this means if you're root? I'd like for us to limit the
> whitelist to things we need, if we're going to open this up for any
> use. (But it sounds like you don't need it for perf, so maybe we can
> put this in something for "admin only" in the future).
>
OK.
>
> > Linux read msr code uses exception tables to catch errors.
> > Not sure what to do here.
>
> Ah nice. I guess we'd need something similar. Even if we whitelist
> MSR_FOOBAR in the code, knowing that it is okay for the user to
> read/write it, but then it turns out we run on a machine that doesn't
> have that MSR, we'd still GPF. So we'd want something like the
> exception fixups.
>
Added.
>
> > > > +static const struct address_range msr_wr_wlist[] = {
> > > > + ADDRESS_RANGE(0x000000c1, 0x000000c8),
> > > > + ADDRESS_RANGE(0x00000186, 0x00000189),
> > > > + ADDRESS_RANGE(0x00000199, 0x00000199),
> > > > + ADDRESS_RANGE(0x00000309, 0x0000030b),
> > > > + ADDRESS_RANGE(0x0000038d, 0x0000038d),
> > > > + ADDRESS_RANGE(0x0000038f, 0x00000391),
> > The ranges also, depends on the number of cores.
>
> Why is that?
>
Sorry, number of *counters*, which is ARCH specific.
Branch updated.
>
--
You received this message because you are subscribed to the Google Groups
"Akaros" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
For more options, visit https://groups.google.com/d/optout.
