Looks pretty good.  I'll admit that I wasn't very thorough once it got into src/dev and there was a lot of diff.  One comment is that there's a lot of random whitespace in these diffs.

src/base/bitmap.hh (Diff revision 1)

Don't forget include guards.

#ifdef __BASE_VNC_...

1. Ali Saidi Jan. 27, 2011, 8:42 a.m. (Jan. 27, 2011, 8:42 a.m.)

   done

src/base/bitmap.cc (Diff revision 1)

I'm guessing that this exceeds 80 columns.  There are also several magic numbers here.  Can you explain what they are, or at least where they come from?

1. Ali Saidi Jan. 27, 2011, 7:45 a.m. (Jan. 27, 2011, 7:45 a.m.)

   none do, added a comment on where to find info about bmp image format

src/base/vnc/VncServer.py (Diff revision 1)

Should we pick something other than 5900 as the default since many people may be running VNC?

1. Ali Saidi Jan. 27, 2011, 7:45 a.m. (Jan. 27, 2011, 7:45 a.m.)

   Might as well let m5 pick a higher port. Some vnc clients don't like arbitrary ports.

src/base/vnc/convert.hh (Diff revision 1)

newline before {

1. Ali Saidi Jan. 27, 2011, 7:45 a.m. (Jan. 27, 2011, 7:45 a.m.)

   done

src/base/vnc/convert.cc (Diff revision 1)

I'd put the int on the next line.

1. Ali Saidi Jan. 27, 2011, 7:45 a.m. (Jan. 27, 2011, 7:45 a.m.)

   done.

src/base/vnc/convert.cc (Diff revision 1)

fatal?

1. Ali Saidi Jan. 27, 2011, 7:46 a.m. (Jan. 27, 2011, 7:46 a.m.)

   done

src/base/vnc/convert.cc (Diff revision 1)

fatal?

Also, is the indent wrong here?

1. Ali Saidi Jan. 27, 2011, 7:46 a.m. (Jan. 27, 2011, 7:46 a.m.)

   done/fixed

src/base/vnc/convert.cc (Diff revision 1)

I'd say:
assert(0 < height && height < 4000);
assert(0 < width && width < 4000);

Two reasons.  It's a little clearer that you're setting bounds, and it also prevents the values from being negative.  Seems like this should even not be an assert, but something that ends in a fatal.

Finally, should it be <= 4000?

1. Ali Saidi Jan. 27, 2011, 7:46 a.m. (Jan. 27, 2011, 7:46 a.m.)

   changed.

src/base/vnc/convert.cc (Diff revision 1)

whack.

1. Ali Saidi Jan. 27, 2011, 7:46 a.m. (Jan. 27, 2011, 7:46 a.m.)

   done

src/base/vnc/convert.cc (Diff revision 1)

Brace on new line.

1. Ali Saidi Jan. 27, 2011, 7:46 a.m. (Jan. 27, 2011, 7:46 a.m.)

   done

src/base/vnc/convert.cc (Diff revision 1)

you do width * height often enough that it seems that you should create an area() or pixelCount() function (or even a const variable).

1. Ali Saidi Jan. 27, 2011, 7:46 a.m. (Jan. 27, 2011, 7:46 a.m.)

   added area()

src/base/vnc/convert.cc (Diff revision 1)

Remove random whitespace.

1. Ali Saidi Jan. 27, 2011, 7:46 a.m. (Jan. 27, 2011, 7:46 a.m.)

   done

src/base/vnc/vncserver.hh (Diff revision 1)

const function?

1. Ali Saidi Jan. 27, 2011, 7:48 a.m. (Jan. 27, 2011, 7:48 a.m.)

   done

src/base/vnc/vncserver.hh (Diff revision 1)

Brace is usually on new line for struct, but in this case, perhaps we don't care.

1. Ali Saidi Jan. 27, 2011, 7:49 a.m. (Jan. 27, 2011, 7:49 a.m.)

   There are so many of these i think it could go against Steve's desire for dense code 

2. Nathan Binkert Jan. 27, 2011, 8:55 a.m. (Jan. 27, 2011, 8:55 a.m.)

   Yeah, that's fine with me.  As long as there is no code inside, I'm cool.

3. Steve Reinhardt Jan. 27, 2011, 9:05 a.m. (Jan. 27, 2011, 9:05 a.m.)

   Thanks for thinking of me :-)

src/base/vnc/vncserver.hh (Diff revision 1)

We generally try not to have __attribute__ stuff in our code, right?  Don't we generally have a #define in src/base/compiler.hh and then use that here?

Also, since you put the padding in, I'm not even sure that you need the __packed__

1. Ali Saidi Jan. 27, 2011, 7:52 a.m. (Jan. 27, 2011, 7:52 a.m.)

   I added a define for it. We used to have defines that created ints with certain alignments, but we got rid of them at some point. 
   
   I'd rather not try to debug an issue with the compiler randomly decides to put some padding in the struck. 

2. Nathan Binkert Jan. 27, 2011, 8:57 a.m. (Jan. 27, 2011, 8:57 a.m.)

   With a #define, I'm fine.  I don't know of any C compiler that doesn't align variables in a C struct according to their size (with a max alignment of 4 bytes on 32-bit machines and 8bytes on 64-bit machines).  So, if we're not gcc, it will likely just work.  (I'd really like to use llvm some day).

src/base/vnc/vncserver.hh (Diff revision 1)

Please use two lines.  This just hides a variable.

1. Ali Saidi Jan. 27, 2011, 7:52 a.m. (Jan. 27, 2011, 7:52 a.m.)

   done

src/base/vnc/vncserver.cc (Diff revision 1)

atomic_write already ignores EINTR (and that's the whole point of the atomic functions.  seems that the whole do/while is unnecessary.

1. Ali Saidi Jan. 27, 2011, 7:54 a.m. (Jan. 27, 2011, 7:54 a.m.)

   done

src/base/vnc/vncserver.cc (Diff revision 1)

Random whitespace

1. Ali Saidi Jan. 27, 2011, 7:54 a.m. (Jan. 27, 2011, 7:54 a.m.)

   done

src/base/vnc/vncserver.cc (Diff revision 1)

random indent

1. Ali Saidi Jan. 27, 2011, 7:55 a.m. (Jan. 27, 2011, 7:55 a.m.)

   fixed

src/base/vnc/vncserver.cc (Diff revision 1)

Does this do what you expect it to do?  Should you do str[len + 1] = 0?
(I really don't know, it's just suspect)

1. Ali Saidi Jan. 27, 2011, 7:56 a.m. (Jan. 27, 2011, 7:56 a.m.)

   cleaned up to null terminate in the right place.

src/dev/arm/RealView.py (Diff revision 1)

line too long

1. Ali Saidi Jan. 27, 2011, 7:58 a.m. (Jan. 27, 2011, 7:58 a.m.)

   fixed

src/dev/arm/kmi.hh (Diff revision 1)

comment? (while you're at it)

1. Ali Saidi Jan. 27, 2011, 7:59 a.m. (Jan. 27, 2011, 7:59 a.m.)

   done

src/dev/arm/kmi.cc (Diff revision 1)

sort :)

1. Ali Saidi Jan. 27, 2011, 8:05 a.m. (Jan. 27, 2011, 8:05 a.m.)

   done

src/dev/ps2.cc (Diff revision 1)

hmmm.  Should we put this in ext?  Otherwise, will we have a dependency on libx11-dev or something like that?

1. Ali Saidi Jan. 27, 2011, 8:04 a.m. (Jan. 27, 2011, 8:04 a.m.)

   if you want. It's already been installed in every machine I've looked.

2. Nathan Binkert Jan. 27, 2011, 8:59 a.m. (Jan. 27, 2011, 8:59 a.m.)

   I do want.  ubuntu-server doesn't have any of them by default and I use that on most server machines.  Might be worthwhile to copy keysym.h into ext/, though I could live with the dependency if you want to use VNC.

src/dev/ps2.cc (Diff revision 1)

void on its own line.

1. Ali Saidi Jan. 27, 2011, 8:05 a.m. (Jan. 27, 2011, 8:05 a.m.)

   done

src/dev/ps2.cc (Diff revision 1)

no semicolon, and /* namespace ... */

Right?

1. Ali Saidi Jan. 27, 2011, 8:05 a.m. (Jan. 27, 2011, 8:05 a.m.)

   done

It would be a good idea to split this into a change that adds the VNC stuff, a change that takes advantage of it in ARM, and the serialization change. You could even split out the bitmap stuff. This change is quite large, but it looks like it would split pretty cleanly into smaller chunks.

Overall this looks like a pretty nice set of changes. It seems to be pretty well written, and it's doing some really useful stuff. There are some things that need polishing up a bit and some moderate tweaks that would be good to make (memmap for the bitmap update, delay update instead of spamming empty updates, not polling for connections (unless I misunderstand what that's doing), inheritance instead of switch statement for conversion function), but in general some good stuff.

src/base/vnc/convert.hh (Diff revision 1)

A bit union is a data structure (a class, if I remember right) so these bitfield lines should be indented.

src/base/vnc/convert.cc (Diff revision 1)

Instead of using a switch statement, why don't you make VideoConvert a base class with a virtual "convert" function?

src/base/vnc/vncserver.hh (Diff revision 1)

Should these be enums instead of constant ints.

src/base/vnc/vncserver.hh (Diff revision 1)

Should this poll? That seems inefficient.

src/base/vnc/vncserver.hh (Diff revision 1)

The function name should be on a new line.

src/base/vnc/vncserver.hh (Diff revision 1)

The function name should be on a new line.

src/base/vnc/vncserver.cc (Diff revision 1)

There should be spaces around +, - and after ,

src/base/vnc/vncserver.cc (Diff revision 1)

htobe should be ok here, but you might want to use the system provided htonl. The later might be optimized for the host arch and use specialized assembly instructions. This applies to the other places this is used and betoh.

src/base/vnc/vncserver.cc (Diff revision 1)

You could put sizeof(... into a temp variable. Maybe messageLen? Also, this pattern seems pretty common, specifically reading one byte off from the start, and asserting you got the right amount of data. Could that be a helper function? You have a templated version of read. Maybe you could have a version that does this off by one read?

src/base/vnc/vncserver.cc (Diff revision 1)

Maybe sizeof(encoding)? That way you don't have to worry about the type here being the same as the type for encoding.

src/base/vnc/vncserver.cc (Diff revision 1)

Instead of using the magic numbers 1025 and 1024 in this block, you should probably either define a constant or use sizeof. A constant seems like the better of the two to me.

src/base/vnc/vncserver.cc (Diff revision 1)

I have read the VNC spec before, but I never used it and that was a while ago. Maybe the client isn't saying give me an update -now-. Maybe it's saying give me an update once you have one? So rather than just replying "no" right away, could you reply with an update after waiting for a while? I can imagine the code in the client processing an update and immediately requesting a new one, not knowing when a new one will be available.

src/dev/arm/kmi.hh (Diff revision 1)

Indent. Same for below bit unions.

src/dev/arm/kmi.cc (Diff revision 1)

Does this fall through on purpose? If so, a comment would be nice so people don't wonder.

src/dev/arm/pl111.hh (Diff revision 1)

Indent.

src/dev/arm/pl111.hh (Diff revision 1)

lookcs -> looks.

src/dev/arm/pl111.cc (Diff revision 1)

I think you're indented an extra space here.

src/dev/arm/pl111.cc (Diff revision 1)

You might want to put {}s around these since they're more than one line, but it's fairly obvious what's going on so it doesn't make much of a difference to me.

src/dev/arm/pl111.cc (Diff revision 1)

"it won't be please"? Did you mean to type "pleased"?

src/dev/arm/pl111.cc (Diff revision 1)

Rather than writing out to the bitmap all the time which might be expensive, would it make sense to memory map it and then just work on it in place? You might have to flush to disk to make sure it's all out there and you may see partial updates, though. Just a thought.

src/dev/arm/rv_ctrl.hh (Diff revision 1)

Indent.

src/dev/arm/rv_ctrl.cc (Diff revision 1)

This might have come up in previous reviews of this file (I remember something like that) but what's the significance of these values? Are they made up of parts? Do they mean something specific? Are they just always what you have here? Some named constants might help, or at least a comment explaining what's going on.

src/dev/arm/rv_ctrl.cc (Diff revision 1)

You could shorten this up a bit by letting these all fall through to one break. That makes it easier to accidentally leave things falling through when you want to make them do something, though.

src/dev/ps2.hh (Diff revision 1)

I already have a PS2 controller partially implemented for x86, so these constants/data types may be duplicated between there and here. It makes sense to include yours in my actual device model.

My code is in dev/x86/i8042.hh and .cc, where i8042 is the chip name for the PC keyboard controller.

src/dev/ps2.hh (Diff revision 1)

These should be indented.

src/dev/ps2.cc (Diff revision 1)

local->locale

src/sim/serialize.hh (Diff revision 1)

These serialization changes could be another separate change set.

src/base/vnc/vncserver.hh (Diff revision 1)

They're constant ints because their size is important to the protocol.