src/arch/arm/ArmSystem.py (Diff revision 1)

Could you create a new intermediate class for both FreeBSD and Linux? Right now this is all duplicated and I'd really prefer to not have the duplication

Show all issues

1. Ruslan Bukin April 20, 2015, 5:13 a.m. (April 20, 2015, 5:13 a.m.)

   python is easy, but it involves some mess in C++. Not sure how to do that

2. Ruslan Bukin April 23, 2015, 8:05 a.m. (April 23, 2015, 8:05 a.m.)

   I have added GenericArmSystem. thanks

src/arch/arm/freebsd/freebsd.hh (Diff revision 1)

I'd be surprised if these these are the same for Linux and FreeBSD. Similarly for the data structures below. Have you had a chance to compare them with the FreeBSD versions to check. They can be the source of odd and very difficult to find bugs in SE mode

Show all issues

1. Ruslan Bukin April 22, 2015, 1:15 p.m. (April 22, 2015, 1:15 p.m.)

   thanks I've fixed this and some other constants I found, else removed.

src/arch/arm/freebsd/process.cc (Diff revision 1)

no tabs please

Show all issues

src/arch/arm/freebsd/process.cc (Diff revision 1)

Done FreeBSD use these "private" syscalls like ARM AArch32 Linux does?

Show all issues

1. Ruslan Bukin April 20, 2015, 6:13 a.m. (April 20, 2015, 6:13 a.m.)

   we dont have such on FreeBSD. So I removed this

src/arch/arm/freebsd/process.cc (Diff revision 1)

Is there any way some of this code could be put in the base class and shared between Linux and FreeBSD. 

Is it purely a subset of the Linux setup? If so, prehaps a function in the base class with an additional parameter specifying Linux (in which case nothing is done) or FreeBSD which if(!FreeBSD) is added to a few of the lines?

Show all issues

Thanks Ruslan. This is getting a lot closer, and it will be great to have FreeBSD support in gem5. I have a couple questions above about code duplication between FreeBSD/Linux.

Thanks again,
Ali

Thanks a lot for the changes. I have a few other minor comments and then I think it's good to go.

src/arch/arm/freebsd/system.cc (Diff revision 8)

I'm guessing that FreeBSD doesn't have these? This code is used to not cache certain structures that will be cacheable on one cpu, but uncachebale on another during boot. I think it's probably fine to retrun false here and be done with it (along with deleting the member variables).

Show all issues

1. Ruslan Bukin April 24, 2015, 3:47 a.m. (April 24, 2015, 3:47 a.m.)

   thanks. Fixed

src/arch/arm/freebsd/system.cc (Diff revision 8)

Does FreeBSD use a flattened devicetree for hardware identification or something else?

If it doesn't then this could can be removed.

Show all issues

1. Ruslan Bukin April 24, 2015, 3:47 a.m. (April 24, 2015, 3:47 a.m.)

   Yes FreeBSD use FDT. fdt_get_range() is one of the FDT functions freebsd has.

src/arch/generic/freebsd/threadinfo.hh (Diff revision 8)

is this called the same as it is in linux? (for all the functions above). This code isn't needed and it can all just panic() with unimplmented if there isn't a good corallary in FreeBSD

Show all issues

1. Ruslan Bukin April 24, 2015, 3:47 a.m. (April 24, 2015, 3:47 a.m.)

   I removed unused/unimplemented functions. This was just copied from linux and it does not apply to FreeBSD. Thanks

Some minor bits. For the rest it looks great. Thanks for the contribution!

src/arch/arm/freebsd/freebsd.hh (Diff revision 9)

Should we stick to the "normal" format, or is there a good reason to keep this as is?

Show all issues

1. Ruslan Bukin April 24, 2015, 5:55 a.m. (April 24, 2015, 5:55 a.m.)

   what the normal format is ? we use this format for all the external stuff related to The FreeBSD Project (we have a bunch of license headers for different languages, different sponsors, but the format is the same)

2. Andreas Hansson April 24, 2015, 6:01 a.m. (April 24, 2015, 6:01 a.m.)

   All files in the gem5 src/ tree should have the same license text you find in the LICENSE file in the root. Note that there is an extra paragraph in the files with an ARM copyright.
   The text should be exactly the same as what you have, it's merely the formatting that differs. I have no objection to either, I am merely suggesting to keep it the same to avoid confusion.

3. Ruslan Bukin April 24, 2015, 8:10 a.m. (April 24, 2015, 8:10 a.m.)

   should I keep 3rd clause as LICENSE file has, or can it be removed ?

4. Andreas Hansson April 24, 2015, 9:31 a.m. (April 24, 2015, 9:31 a.m.)

   3rd? There is the first copyright block, and then two paragraphs.

5. Ruslan Bukin April 24, 2015, 9:34 a.m. (April 24, 2015, 9:34 a.m.)

   3rd BSD clause:
   
   neither the name of the copyright holders nor the names of its
   
   contributors may be used to endorse or promote products derived from
   
   this software without specific prior written permission.
   we (in Computer Laboratory) prefer 2-clause BSD license

6. Andreas Hansson April 24, 2015, 9:36 a.m. (April 24, 2015, 9:36 a.m.)

   Ah, I see.
   I think we should stick to the same license throughout.

7. Ruslan Bukin April 24, 2015, 9:47 a.m. (April 24, 2015, 9:47 a.m.)

   http://dpaste.com/0PAW27Y
   
   do this works ?

8. Andreas Hansson April 24, 2015, 9:59 a.m. (April 24, 2015, 9:59 a.m.)

   I would think so. Is there anyone else with input on this matter?

9. Ruslan Bukin April 24, 2015, 10:47 a.m. (April 24, 2015, 10:47 a.m.)

   patch updated

10. Andreas Hansson April 27, 2015, 12:29 a.m. (April 27, 2015, 12:29 a.m.)

    Excellent. Thanks!

src/arch/arm/freebsd/process.hh (Diff revision 9)

I guess these should technically be __ARCH_ARM...

Show all issues

src/arch/arm/freebsd/system.cc (Diff revision 9)

whitespace

Show all issues

src/arch/arm/process.cc (Diff revision 9)

I guess there is no way or merging the "if linux" blocks?

Show all issues

src/arch/arm/system.hh (Diff revision 9)

virtual

Show all issues

src/arch/arm/system.cc (Diff revision 9)

I'd even suggest to leave these in the header for now and make them one-liners.

Show all issues

,

Ship It!

Ship It!