----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/633/#review1109 -----------------------------------------------------------
I don't really know what this is supposed to do functionally so I mostly ignored correctness. I mentioned the style issues I saw and possible minor improvements here and there, but I may have missed things or only pointed out one of several instances of a particular problem. src/dev/arm/gic.hh <http://reviews.m5sim.org/r/633/#comment1481> I'm tempted to suggest making these SgiMax, etc., since they aren't macros, but what you have looks like it's consistent with the rest of the file. I wouldn't mind if you changed these in a later change, though :-). src/dev/arm/gic.hh <http://reviews.m5sim.org/r/633/#comment1482> Should these be ack_id or ackId? This is again consistent with existing code, and again something you could change in a later changeset. Also you should have a space after the comma. src/dev/arm/gic.hh <http://reviews.m5sim.org/r/633/#comment1480> This parameter description is no longer accurate. src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1483> Using named constants instead of magic numbers here is a nice change. src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1484> spaces around the operators here: 7+8*x => 7 + 8 * x src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1485> I know operator precedence probably takes care of things, but it would be slightly easier to parse this visually if you had parenthesis around the 1 << blah. That would be consistent with the negated version below which would be a nice coincidence. src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1486> There shouldn't be a space after the !. Also, there are a lot of parenthesis and nested operations here. Some temporary variables and/or Demorgan's to distribute the ! would make this easier to read and understand. src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1487> Ditto. Also, the fairly complex (1 << (ctx_id + 8 * iar.cpu_id)) value is used in the previous if. That's another reason it would make sense in a temporary variable. src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1488> space around *. Order of operations with << can be surprising, so maybe parenthesis around the 8 * ctx_id as well? src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1489> How about: uint64_t Gic::genSwiMask(unsigned cpu) { if (cpu > 7) panic("Invalid CPU ID\n"); return ULL(0x0101010101010101) << cpu; } I think you need the ULL one way or the other so this will build on 32 bit systems. src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1490> If it's done then it's not a todo. Please either get rid of the comment or reword it so it just describes what it does now without the historical baggage. src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1491> If these are constants, why do they need to be serialized and unserialized? So they can be made variable in the future? src/dev/arm/gic.cc <http://reviews.m5sim.org/r/633/#comment1492> space around the * - Gabe On 2011-04-10 16:52:32, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/633/ > ----------------------------------------------------------- > > (Updated 2011-04-10 16:52:32) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > ARM: Make GIC handle IPIs and multiple processors. > > > Diffs > ----- > > src/dev/arm/gic.hh 02cb69e5cfeb > src/dev/arm/gic.cc 02cb69e5cfeb > > Diff: http://reviews.m5sim.org/r/633/diff > > > Testing > ------- > > > Thanks, > > Ali > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
