src/cpu/simple_thread.cc (Diff revision 2)

This seems like a bad plan. Probably best to juts leave it around. 

src/cpu/simple_thread.cc (Diff revision 2)

I also believe the SimpleThread should have nothing to do with deleting the port proxies. 

I have moved this to the ThreadState destructor since that class is responsible for the creation of the proxies.

Looks pretty good... several comments below (some minor, a couple more significant ones), and there's also the pointer declaration style issue to iron out, but it looks pretty close.

I think this is my last patch to review of your series, Andreas.  I think there were only a couple (including this one) I didn't mark as "ship it"... those are the only ones I care to see again before you commit.

Thanks for all your hard work!

src/mem/fs_translating_port_proxy.hh (Diff revision 7)

Typo in "boundaries" in this and the next comment.  Unless it's some variant British spelling I'm unaware of :).

1. Andreas Hansson Jan. 11, 2012, 4:31 p.m. (Jan. 11, 2012, 4:31 p.m.)

   Fixed :)

src/mem/fs_translating_port_proxy.hh (Diff revision 7)

Are these read/write/readGtoH/writeHtoG methods used anywhere?  I didn't see them on VirtualPort and I don't recall any uses elsewhere in this patch.

I'm concerned because we could get into cases where these are unaligned and cross page boundaries, and the current code doesn't deal with that.

1. Andreas Hansson Jan. 11, 2012, 4:33 p.m. (Jan. 11, 2012, 4:33 p.m.)

   I haven't tidied up the code at all, but I can of course do so. I tried to keep the changes to a minimum and only do the file rename and the absolutely necessary edits. Obviously that doesn't seem to have worked very well.

src/mem/fs_translating_port_proxy.hh (Diff revision 7)

If you add a private inline method like:

Addr
translate(Addr va)
{
  if (_tc == NULL) {
    return TheISA::vtophys(address);
  } else {
    return TheISA::vtophys(_tc, address);
  }
}

then you could turn all of these functions into one-liners, and simplify a lot of the functions in the .cc file too.

1. Andreas Hansson Jan. 11, 2012, 4:34 p.m. (Jan. 11, 2012, 4:34 p.m.)

   Once again, I haven't tidied up the code at all to minimise the changes. I can perform this change, but would rather make it a second "cleanup" patch.

src/mem/fs_translating_port_proxy.cc (Diff revision 7)

I'd really like to see these done in mercurial as file renames rather than a combination of new and deleted files... e.g., this should be a rename of vport.cc not a new file.  Similarly for translating_port.{cc,hh} -> se_translating_proxy.{cc,hh}

1. Andreas Hansson Jan. 11, 2012, 4:35 p.m. (Jan. 11, 2012, 4:35 p.m.)

   I did do them as file renames, just as you have suggested. I did not do that for the first iteration of these changes (as I am sure you remember), and the very first step of the second iteration was exactly the renames you suggest. I don't know why the review board does not like this or gets it wrong.

src/mem/fs_translating_port_proxy.cc (Diff revision 7)

need space after the comma here (and several similar places)

1. Andreas Hansson Jan. 11, 2012, 4:36 p.m. (Jan. 11, 2012, 4:36 p.m.)

   Once again, I've tried to not change anything unless I have to, but would be very happy to do so. I'm itching to make things look decent, and really had to try hard not to fix it.

src/mem/fs_translating_port_proxy.cc (Diff revision 7)

This comment is totally outside the scope of this patch, but seeing this code again reminds me how much it annoys me, mainly because these are global functions that start with a capital (predating, but way in violation of, the style guide).  It seems to me that these might be best as methods on ThreadContext, and then they could eventually be usable in SE mode too.

1. Andreas Hansson Jan. 11, 2012, 4:37 p.m. (Jan. 11, 2012, 4:37 p.m.)

   I think that is a brilliant suggestion and would be keen to clean it up as well. I would imagine we should wait until Gabe is done with the SE/FS merge.

src/mem/port_proxy.cc (Diff revision 7)

I'd be fine with inlining these into port_proxy.hh and getting rid of this file... seems like overkill to have a separate .cc, and they're short enough it's probably worth inlining them anyway.

1. Andreas Hansson Jan. 11, 2012, 4:38 p.m. (Jan. 11, 2012, 4:38 p.m.)

   Will do so!

Sorry about nagging you on the file renames; it didn't occur to me that perhaps you had fixed that and reviewboard just wasn't capable of showing it.  Also I agree on saving the little things that are carryovers from the old code for a separate patch (the comma spacing and the translate() helper function); since reviewboard wasn't showing the rename properly it wasn't obvious that these issues were also there in the old code.

The one thing I am still concerned about is the read*() and write*() methods on FSTranslatingPortProxy.  It seems like those aren't necessary (since VirtualPort doesn't have them) and the current implementations aren't correct (since they don't deal with page crossings), so I'd prefer to see those just left out until we put in working ones.  In the meantime, the even-more-broken PortProxy methods will get inherited and not overridden; if we care about that, maybe we should temporarily override them with methods that panic.  (If we had C++11 we could delete them...)