Brad, Yes it does break older checkpoints, but it does also fix bugs. The parameters you mention can trivially be added to older checkpoint files either by hand or by a small script. Locked addresses should have been serialized, and surprisingly enough we never had a problem with it, or more likely we actually did and never figured out the root cause. _size exists now because if you mmap a file, the file size can be smaller than the memory you're trying to put it in, in which case you need to know the real size of the file. Unfortunately, we don't have a really good way to deal with different versions of checkpoint files, and I believe that attempting to create some sort of auto-migrator would be very challenging and of little benefit. I think this is the first time in well over a year that the checkpoints have been altered, and I don't expect any churn on the files.
It would probably be good to explicitly mention this in the future, or we could try to maintain a script that migrated versions in the case that there was a trivial default available. Adding this to the [system.physical] section should do the trick: lal_cid = lal_addr = _size = 134217728 (assuming 128MB) Ali On Dec 9, 2010, at 11:02 PM, Gabriel Michael Black wrote: > Right, I don't want to sound like those sort of changes should just be done > cavalierly. I'm just saying we shouldn't be afraid to make them when we need > to. > > Gabe > > Quoting "Beckmann, Brad" <[email protected]>: > >> Thanks Gabe. >> >> Yeah, I don't think conditioning on the ARM ISA for this change is >> necessarily a good idea. I just suggested it to see if there was some sort >> of conditional we could add. As far as the "_size" parameter goes, it is >> not clear to me why that needs to be in the checkpoint. Isn't the memory >> size passed in from the configuration? Maybe I'm not fully understanding >> Ali's change. >> >> Overall, I think we need to be careful by just saying that these sort of >> changes are just something people need to deal with. It is really hard for >> someone to easily fix these sort of problems because it often requires >> understanding someone else's code in a good amount of detail. I agree that >> any sort of versioning system would add a decent amount of complexity. But >> continually dealing with these sorts of problems as well as helping others >> deal with them, will not be easy either. >> >> Brad >> >> >> >>> -----Original Message----- >>> From: [email protected] [mailto:[email protected]] On >>> Behalf Of Gabriel Michael Black >>> Sent: Thursday, December 09, 2010 9:04 PM >>> To: [email protected] >>> Subject: Re: [m5-dev] changeset in m5: ARM: Add checkpointing support >>> >>> In reference to this and the next email, I don't think these should be >>> conditioned on the ARM ISA because it looks like those values are >>> valid and need to be serialized for all ISAs. It's just that the other >>> ISAs happen to have checkpoints that are old enough that they break, >>> and that's not a good enough reason to introduce a superficial ISA >>> incompatibility. >>> >>> The core problem, which is a real issue, is that checkpoints created >>> with one version of M5 aren't necessarily compatible with other >>> versions, although they tend to be. That's unfortunate, but I think to >>> avoid really binding our hands as far as how we can change things, >>> that's just one of those things we'll need to live with. We could try >>> to implement some sort of versioning system maybe just to warn that >>> something is incompatible, or maybe have a default value if something >>> isn't found. I'm not sure whether that would solve the problem, >>> though, and it would add a decent amount of complexity. >>> >>> As far as regression testing, that would be great. There have been a >>> lot of times where I've wished we had that since I accidentally broke >>> something :-). I remember hearing that would be hard to do for some >>> reason, though. >>> >>> Gabe >>> >>> Quoting "Beckmann, Brad" <[email protected]>: >>> >>> > Hi Ali, >>> > >>> > I just synced with this changeset 7733, as well as changeset 7730, >>> > and I now notice that the modifications to physical.cc break all >>> > previous checkpoints. Can we put the lal_addr and lal_cid >>> > serialization and unserialization in a conditional that tests for >>> > the ARM ISA? I welcome other suggestions as well. >>> > >>> > In general, I would be interested to hear other people's thoughts on >>> > adding a checkpoint test to the regression tester. It would be >>> > great if we can at least identify ahead of time what changesets >>> > break older checkpoints. >>> > >>> > Brad >>> > >>> > >>> > >>> >> -----Original Message----- >>> >> From: [email protected] [mailto:[email protected]] On >>> >> Behalf Of Ali Saidi >>> >> Sent: Monday, November 08, 2010 11:59 AM >>> >> To: [email protected] >>> >> Subject: [m5-dev] changeset in m5: ARM: Add checkpointing support >>> >> >>> >> changeset 08d6a773d1b6 in /z/repo/m5 >>> >> details: http://repo.m5sim.org/m5?cmd=changeset;node=08d6a773d1b6 >>> >> description: >>> >> ARM: Add checkpointing support >>> >> >>> >> diffstat: >>> >> >>> >> src/arch/arm/isa.hh | 12 +++++- >>> >> src/arch/arm/linux/system.cc | 5 +- >>> >> src/arch/arm/linux/system.hh | 4 +- >>> >> src/arch/arm/pagetable.hh | 87 +++++++++++++++++++++++--------- >>> --- >>> >> -------- >>> >> src/arch/arm/table_walker.cc | 16 ++++++- >>> >> src/arch/arm/table_walker.hh | 2 +- >>> >> src/arch/arm/tlb.cc | 14 ++++++- >>> >> src/arch/arm/tlb.hh | 2 - >>> >> src/dev/arm/gic.cc | 44 +++++++++++++++++++++- >>> >> src/dev/arm/pl011.cc | 42 ++++++++++++++++++++- >>> >> src/dev/arm/rv_ctrl.cc | 2 - >>> >> src/dev/arm/timer_sp804.cc | 59 ++++++++++++++++++++++++++++- >>> >> src/dev/arm/timer_sp804.hh | 4 ++ >>> >> src/mem/physical.cc | 30 +++++++++++++++ >>> >> src/mem/physical.hh | 5 ++ >>> >> src/sim/SConscript | 1 + >>> >> src/sim/system.cc | 2 +- >>> >> src/sim/system.hh | 2 +- >>> >> 18 files changed, 268 insertions(+), 65 deletions(-) >>> >> >>> >> diffs (truncated from 587 to 300 lines): >>> >> >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/arch/arm/isa.hh >>> >> --- a/src/arch/arm/isa.hh Mon Nov 08 13:58:24 2010 -0600 >>> >> +++ b/src/arch/arm/isa.hh Mon Nov 08 13:58:25 2010 -0600 >>> >> @@ -178,10 +178,18 @@ >>> >> } >>> >> >>> >> void serialize(EventManager *em, std::ostream &os) >>> >> - {} >>> >> + { >>> >> + DPRINTF(Checkpoint, "Serializing Arm Misc >>> Registers\n"); >>> >> + SERIALIZE_ARRAY(miscRegs, NumMiscRegs); >>> >> + } >>> >> void unserialize(EventManager *em, Checkpoint *cp, >>> >> const std::string §ion) >>> >> - {} >>> >> + { >>> >> + DPRINTF(Checkpoint, "Unserializing Arm Misc >>> Registers\n"); >>> >> + UNSERIALIZE_ARRAY(miscRegs, NumMiscRegs); >>> >> + CPSR tmp_cpsr = miscRegs[MISCREG_CPSR]; >>> >> + updateRegMap(tmp_cpsr); >>> >> + } >>> >> >>> >> ISA() >>> >> { >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/arch/arm/linux/system.cc >>> >> --- a/src/arch/arm/linux/system.cc Mon Nov 08 13:58:24 2010 - >>> 0600 >>> >> +++ b/src/arch/arm/linux/system.cc Mon Nov 08 13:58:25 2010 - >>> 0600 >>> >> @@ -99,9 +99,9 @@ >>> >> } >>> >> >>> >> void >>> >> -LinuxArmSystem::startup() >>> >> +LinuxArmSystem::initState() >>> >> { >>> >> - ArmSystem::startup(); >>> >> + ArmSystem::initState(); >>> >> ThreadContext *tc = threadContexts[0]; >>> >> >>> >> // Set the initial PC to be at start of the kernel code >>> >> @@ -117,7 +117,6 @@ >>> >> { >>> >> } >>> >> >>> >> - >>> >> LinuxArmSystem * >>> >> LinuxArmSystemParams::create() >>> >> { >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/arch/arm/linux/system.hh >>> >> --- a/src/arch/arm/linux/system.hh Mon Nov 08 13:58:24 2010 - >>> 0600 >>> >> +++ b/src/arch/arm/linux/system.hh Mon Nov 08 13:58:25 2010 - >>> 0600 >>> >> @@ -67,8 +67,8 @@ >>> >> LinuxArmSystem(Params *p); >>> >> ~LinuxArmSystem(); >>> >> >>> >> - /** Initialize the CPU for booting */ >>> >> - void startup(); >>> >> + void initState(); >>> >> + >>> >> private: >>> >> #ifndef NDEBUG >>> >> /** Event to halt the simulator if the kernel calls panic() */ >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/arch/arm/pagetable.hh >>> >> --- a/src/arch/arm/pagetable.hh Mon Nov 08 13:58:24 2010 -0600 >>> >> +++ b/src/arch/arm/pagetable.hh Mon Nov 08 13:58:25 2010 -0600 >>> >> @@ -48,6 +48,8 @@ >>> >> #include "arch/arm/vtophys.hh" >>> >> #include "config/full_system.hh" >>> >> >>> >> +#include "sim/serialize.hh" >>> >> + >>> >> namespace ArmISA { >>> >> >>> >> struct VAddr >>> >> @@ -71,39 +73,6 @@ >>> >> >>> >> }; >>> >> >>> >> -struct TlbRange >>> >> -{ >>> >> - Addr va; >>> >> - Addr size; >>> >> - int contextId; >>> >> - bool global; >>> >> - >>> >> - inline bool >>> >> - operator<(const TlbRange &r2) const >>> >> - { >>> >> - if (!(global || r2.global)) { >>> >> - if (contextId < r2.contextId) >>> >> - return true; >>> >> - else if (contextId > r2.contextId) >>> >> - return false; >>> >> - } >>> >> - >>> >> - if (va < r2.va) >>> >> - return true; >>> >> - return false; >>> >> - } >>> >> - >>> >> - inline bool >>> >> - operator==(const TlbRange &r2) const >>> >> - { >>> >> - return va == r2.va && >>> >> - size == r2.size && >>> >> - contextId == r2.contextId && >>> >> - global == r2.global; >>> >> - } >>> >> -}; >>> >> - >>> >> - >>> >> // ITB/DTB table entry >>> >> struct TlbEntry >>> >> { >>> >> @@ -143,10 +112,8 @@ >>> >> >>> >> // Access permissions >>> >> bool xn; // Execute Never >>> >> - uint8_t ap:3; // Access permissions bits >>> >> - uint8_t domain:4; // Access Domain >>> >> - >>> >> - TlbRange range; // For fast TLB searching >>> >> + uint8_t ap; // Access permissions bits >>> >> + uint8_t domain; // Access Domain >>> >> >>> >> //Construct an entry that maps to physical address addr for SE >>> >> mode >>> >> TlbEntry(Addr _asn, Addr _vaddr, Addr _paddr) >>> >> @@ -196,9 +163,49 @@ >>> >> return (pfn << N) | (va & size); >>> >> } >>> >> >>> >> - void serialize(std::ostream &os) { panic("Need to >>> Implement\n"); } >>> >> - void unserialize(Checkpoint *cp, const std::string §ion) >>> >> - { panic("Need to Implement\n");} >>> >> + void >>> >> + serialize(std::ostream &os) >>> >> + { >>> >> + SERIALIZE_SCALAR(pfn); >>> >> + SERIALIZE_SCALAR(size); >>> >> + SERIALIZE_SCALAR(vpn); >>> >> + SERIALIZE_SCALAR(asid); >>> >> + SERIALIZE_SCALAR(N); >>> >> + SERIALIZE_SCALAR(global); >>> >> + SERIALIZE_SCALAR(valid); >>> >> + SERIALIZE_SCALAR(nonCacheable); >>> >> + SERIALIZE_SCALAR(sNp); >>> >> + SERIALIZE_ENUM(mtype); >>> >> + SERIALIZE_SCALAR(innerAttrs); >>> >> + SERIALIZE_SCALAR(outerAttrs); >>> >> + SERIALIZE_SCALAR(shareable); >>> >> + SERIALIZE_SCALAR(attributes); >>> >> + SERIALIZE_SCALAR(xn); >>> >> + SERIALIZE_SCALAR(ap); >>> >> + SERIALIZE_SCALAR(domain); >>> >> + } >>> >> + void >>> >> + unserialize(Checkpoint *cp, const std::string §ion) >>> >> + { >>> >> + UNSERIALIZE_SCALAR(pfn); >>> >> + UNSERIALIZE_SCALAR(size); >>> >> + UNSERIALIZE_SCALAR(vpn); >>> >> + UNSERIALIZE_SCALAR(asid); >>> >> + UNSERIALIZE_SCALAR(N); >>> >> + UNSERIALIZE_SCALAR(global); >>> >> + UNSERIALIZE_SCALAR(valid); >>> >> + UNSERIALIZE_SCALAR(nonCacheable); >>> >> + UNSERIALIZE_SCALAR(sNp); >>> >> + UNSERIALIZE_ENUM(mtype); >>> >> + UNSERIALIZE_SCALAR(innerAttrs); >>> >> + UNSERIALIZE_SCALAR(outerAttrs); >>> >> + UNSERIALIZE_SCALAR(shareable); >>> >> + UNSERIALIZE_SCALAR(attributes); >>> >> + UNSERIALIZE_SCALAR(xn); >>> >> + UNSERIALIZE_SCALAR(ap); >>> >> + UNSERIALIZE_SCALAR(domain); >>> >> + } >>> >> + >>> >> }; >>> >> >>> >> >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/arch/arm/table_walker.cc >>> >> --- a/src/arch/arm/table_walker.cc Mon Nov 08 13:58:24 2010 - >>> 0600 >>> >> +++ b/src/arch/arm/table_walker.cc Mon Nov 08 13:58:25 2010 - >>> 0600 >>> >> @@ -59,10 +59,20 @@ >>> >> } >>> >> >>> >> >>> >> -unsigned int >>> >> -drain(Event *de) >>> >> +unsigned int TableWalker::drain(Event *de) >>> >> { >>> >> - panic("Not implemented\n"); >>> >> + if (stateQueueL1.size() != 0 || stateQueueL2.size() != 0) >>> >> + { >>> >> + changeState(Draining); >>> >> + DPRINTF(Checkpoint, "TableWalker busy, wait to drain\n"); >>> >> + return 1; >>> >> + } >>> >> + else >>> >> + { >>> >> + changeState(Drained); >>> >> + DPRINTF(Checkpoint, "TableWalker free, no need to >>> drain\n"); >>> >> + return 0; >>> >> + } >>> >> } >>> >> >>> >> Port* >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/arch/arm/table_walker.hh >>> >> --- a/src/arch/arm/table_walker.hh Mon Nov 08 13:58:24 2010 - >>> 0600 >>> >> +++ b/src/arch/arm/table_walker.hh Mon Nov 08 13:58:25 2010 - >>> 0600 >>> >> @@ -350,7 +350,7 @@ >>> >> return dynamic_cast<const Params *>(_params); >>> >> } >>> >> >>> >> - virtual unsigned int drain(Event *de) { panic("write me\n"); } >>> >> + virtual unsigned int drain(Event *de); >>> >> virtual Port *getPort(const std::string &if_name, int idx = - >>> 1); >>> >> >>> >> Fault walk(RequestPtr req, ThreadContext *tc, uint8_t cid, >>> >> TLB::Mode mode, >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/arch/arm/tlb.cc >>> >> --- a/src/arch/arm/tlb.cc Mon Nov 08 13:58:24 2010 -0600 >>> >> +++ b/src/arch/arm/tlb.cc Mon Nov 08 13:58:25 2010 -0600 >>> >> @@ -245,14 +245,24 @@ >>> >> void >>> >> TLB::serialize(ostream &os) >>> >> { >>> >> - panic("Implement Serialize\n"); >>> >> + DPRINTF(Checkpoint, "Serializing Arm TLB\n"); >>> >> + >>> >> + SERIALIZE_SCALAR(_attr); >>> >> + for(int i = 0; i < size; i++){ >>> >> + nameOut(os, csprintf("%s.TlbEntry%d", name(), i)); >>> >> + table[i].serialize(os); >>> >> + } >>> >> } >>> >> >>> >> void >>> >> TLB::unserialize(Checkpoint *cp, const string §ion) >>> >> { >>> >> + DPRINTF(Checkpoint, "Unserializing Arm TLB\n"); >>> >> >>> >> - panic("Need to properly unserialize TLB\n"); >>> >> + UNSERIALIZE_SCALAR(_attr); >>> >> + for(int i = 0; i < size; i++){ >>> >> + table[i].unserialize(cp, csprintf("%s.TlbEntry%d", section, >>> >> i)); >>> >> + } >>> >> } >>> >> >>> >> void >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/arch/arm/tlb.hh >>> >> --- a/src/arch/arm/tlb.hh Mon Nov 08 13:58:24 2010 -0600 >>> >> +++ b/src/arch/arm/tlb.hh Mon Nov 08 13:58:25 2010 -0600 >>> >> @@ -83,8 +83,6 @@ >>> >> MustBeOne = 0x80 >>> >> }; >>> >> protected: >>> >> - typedef std::multimap<Addr, int> PageTable; >>> >> - PageTable lookupTable; // Quick lookup into page table >>> >> >>> >> TlbEntry *table; // the Page Table >>> >> int size; // TLB Size >>> >> diff -r a2c660de7787 -r 08d6a773d1b6 src/dev/arm/gic.cc >>> >> --- a/src/dev/arm/gic.cc Mon Nov 08 13:58:24 2010 -0600 >>> >> +++ b/src/dev/arm/gic.cc Mon Nov 08 13:58:25 2010 -0600 >>> >> @@ -495,13 +495,53 @@ >>> >> void >>> >> Gic::serialize(std::ostream &os) >>> >> { >>> >> - panic("Need to implement serialization\n"); >>> >> + DPRINTF(Checkpoint, "Serializing Arm GIC\n"); >>> >> + >>> >> + SERIALIZE_SCALAR(distAddr); >>> >> + SERIALIZE_SCALAR(cpuAddr); >>> >> + SERIALIZE_SCALAR(distPioDelay); >>> >> + SERIALIZE_SCALAR(cpuPioDelay); >>> >> + SERIALIZE_SCALAR(enabled); >>> >> + SERIALIZE_SCALAR(itLines); >>> >> + SERIALIZE_SCALAR(itLinesLog2); >>> >> + SERIALIZE_ARRAY(intEnabled, 32); >>> >> + SERIALIZE_ARRAY(pendingInt, 32); >>> >> + SERIALIZE_ARRAY(activeInt, 32); >>> >> + SERIALIZE_ARRAY(iccrpr, 8); >>> >> + SERIALIZE_ARRAY(intPriority, 1020); >>> >> + SERIALIZE_ARRAY(cpuTarget, 1020); >>> >> + SERIALIZE_ARRAY(intConfig, 64); >>> >> + SERIALIZE_ARRAY(cpuEnabled, 8); >>> >> + SERIALIZE_ARRAY(cpuPriority, 8); >>> >> + SERIALIZE_ARRAY(cpuBpr, 8); >>> >> + SERIALIZE_ARRAY(cpuHighestInt, 8); >>> >> + SERIALIZE_SCALAR(irqEnable); >>> >> } >>> >> >>> >> void >>> >> Gic::unserialize(Checkpoint *cp, const std::string §ion) >>> >> { >>> >> - panic("Need to implement serialization\n"); >>> >> + DPRINTF(Checkpoint, "Unserializing Arm GIC\n"); >>> >> + >>> >> + UNSERIALIZE_SCALAR(distAddr); >>> >> + UNSERIALIZE_SCALAR(cpuAddr); >>> >> + UNSERIALIZE_SCALAR(distPioDelay); >>> >> _______________________________________________ >>> >> m5-dev mailing list >>> >> [email protected] >>> >> http://m5sim.org/mailman/listinfo/m5-dev >>> > >>> > >>> > _______________________________________________ >>> > m5-dev mailing list >>> > [email protected] >>> > http://m5sim.org/mailman/listinfo/m5-dev >>> > >>> >>> >>> _______________________________________________ >>> m5-dev mailing list >>> [email protected] >>> http://m5sim.org/mailman/listinfo/m5-dev >> >> >> _______________________________________________ >> m5-dev mailing list >> [email protected] >> http://m5sim.org/mailman/listinfo/m5-dev >> > > > _______________________________________________ > m5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/m5-dev > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
