Re: Partial review of kvm arm git patches.

[Rusty Russell <rusty@rustcorp.com.au>]

On Thu, 2 Feb 2012 09:48:22 -0500, Christoffer Dall <cdall@cs.columbia.edu> wrote:
> On Thu, Feb 2, 2012 at 12:04 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > Hi all,
> >
> >        Started reading through the git tree at
> > git://github.com/virtualopensystems/linux-kvm-arm.git (kvm-a15-v6-stage
> > branch), and noticed some things.  I'm learning ARM as I go, so
> > apologies in advance for any dumb questions.
> >
> > Firstly, I want to say that reading this code was a pleasant surprise!
> > Many of my comments below are nit-picking...
> 
> thanks.
> 
> I am a little confused though, did you not already send a lot of these
> comments yesterday?

Ah, yes.  I stopped and started over a couple of days, and accidentally
sent off a draft.  I thought I killed it before it went out, but clearly
not...

> Also, I appreciate very much the review, but remember that I am not
> done with this patch series - which is why it has not been sent out
> yet but merely pushed to a staging branch.

Sure... I wanted to read the code, and I figured I might as well comment
as I go.  Doing the latest version seemed sensible, though it means some
comments are trivial.

> >> +     addr = PAGE_OFFSET;
> >> +     end = ~0;
> >> +     do {
> >> +             next = pgd_addr_end(addr, end);
> >> +             pgd = hyp_pgd + pgd_index(addr);
> >> +             pud = pud_offset(pgd, addr);
> >> +
> >> +             BUG_ON(pud_bad(*pud));
> >> +
> >> +             if (pud_none(*pud))
> >> +                     continue;
> >> +
> >> +             pmd = pmd_offset(pud, addr);
> >> +             free_ptes(pmd, addr);
> >> +             pmd_free(NULL, pmd);
> >> +     } while (addr = next, addr != end);
> >
> > All your loops are like this, but I think a for () is sufficient.
> > I don't think any of them are called start == end anyway, and it
> > wouldn't matter if they were...
> >
> 
> I don't understand this point. Why is a for() better. This scheme is
> used elsewhere in the kernel as well.

Indeed, there's a habit of doing this in the mm code, but that's very
old code, predating Hugh's introduction of pmd_addr_end() et al in 2005.

For non-mm hackers, it's just a confusing mess.  This is what that loop
is actually doing, written in normal C:

        for (addr = PAGE_OFFSET; addr != 0; addr += PGDIR_SIZE) {
                pgd = hyp_pgd + pgd_index(addr);
                pud = pud_offset(pgd, addr);

                BUG_ON(pud_bad(*pud));

                if (pud_none(*pud))
                        continue;

                pmd = pmd_offset(pud, addr);
                free_ptes(pmd, addr);
                pmd_free(NULL, pmd);
        }

This version doesn't make me wonder what I'm missing about the loop
boundaries.

> > As a general rule, I prefer not to see "inline" in C files.  If the
> > function ever becomes unused, it's nice if gcc tells you.  Though maybe
> > here that's a feature?
> >
> 
> just a hint to the compiler to inline. It will probably do that
> anyway, and I like your argument, so sure.
> 
> Is the inline hint completely ignored by the compiler in terms of
> actually inlining or not?

No, it works.  But gcc keeps getting smarter: it will inline any
called-once function anyway, and the threshold for inlining changes with
-Os vs -O2.

Thus, I consider inline the register keyword of the 90s :)

> > The accretion of loose emulation leads to QEMU-style emulation, where
> > you can't distinguish sloppiness from deliberate hacks, and you're
> > terrified to clean up anything because some weird setup will break.  I'd
> > really rather see us emulating as tightly as possible.
> >
> 
> Ok. There should be no deliberate hacks in there. So anything
> imprecise is sloppiness or at least something incomplete.
> 
> So I am not sure what you are suggesting. Something completely
> different or just fixing up the emulation code?

I'm suggesting fixing it up.  Peter has some WIP QEMU patches to change
its emulation to a table-driven approach, I'd like to try the same kind
of thing here.

> > (And unit testing our emulation code, but that's another topic).
> >
> 
> If you want to come up with a nice framework for this, be my guest ;)

OK, I'll think harder about this...

> Looking forward for more to come. I will try to release a v6 series
> real soon, so you don't have to spend time on stuff that is merged in
> the wrong patch etc.

Yeah, I'll go through that once you've posted it.

Every time I re-read this stuff, I learn a little more.  Thanks!

Cheers,
Rusty.