From b99c31684025d4c730021330173c5829c9ca13fa Mon Sep 17 00:00:00 2001 From: "Bobby R. Bruce" Date: Tue, 21 Jul 2020 17:29:19 -0700 Subject: [PATCH] base,arch: Fixed usage of `bitfield::replaceBits` `bitfield::replaceBits` has two parameters, `first` and `last`, which relate to the position of the MSB and the LSB of the bits to be replaced respectively. Therefore `first` >= `last`. In some areas of the codebase, this assumption has been flipped with `first` <= `last`. This caused at least one known error, recorded here: https://gem5.atlassian.net/browse/GEM5-695. These inconsistencies have therefore been rectified. A note has been added to the `bitfield::replaceBits` Doxygen to make the usage of this function clearer. Change-Id: Ie75856161d9a5684066430ecbdcc52e04e1e77bf Issue-on: https://gem5.atlassian.net/browse/GEM5-696 Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/31674 Reviewed-by: Bobby R. Bruce Maintainer: Bobby R. Bruce Tested-by: kokoro --- src/arch/mips/isa.cc | 38 ++++++++++++++++++------------------ src/arch/x86/bios/intelmp.cc | 6 +++--- src/arch/x86/bios/intelmp.hh | 4 ++-- src/base/bitfield.hh | 4 +++- 4 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/arch/mips/isa.cc b/src/arch/mips/isa.cc index 616876ceb..5fbb6cf06 100644 --- a/src/arch/mips/isa.cc +++ b/src/arch/mips/isa.cc @@ -184,7 +184,7 @@ ISA::configCP() // Now, create Write Mask for ProcID register RegVal procIDMask = 0; // Read-Only register - replaceBits(procIDMask, 0, 32, 0); + replaceBits(procIDMask, 32, 0, 0); setRegMask(MISCREG_PRID, procIDMask); // Config @@ -198,7 +198,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_CONFIG, cfg); // Now, create Write Mask for Config register RegVal cfg_Mask = 0x7FFF0007; - replaceBits(cfg_Mask, 0, 32, 0); + replaceBits(cfg_Mask, 32, 0, 0); setRegMask(MISCREG_CONFIG, cfg_Mask); // Config1 @@ -220,7 +220,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_CONFIG1, cfg1); // Now, create Write Mask for Config register RegVal cfg1_Mask = 0; // Read Only Register - replaceBits(cfg1_Mask, 0, 32, 0); + replaceBits(cfg1_Mask, 32,0 , 0); setRegMask(MISCREG_CONFIG1, cfg1_Mask); // Config2 @@ -237,7 +237,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_CONFIG2, cfg2); // Now, create Write Mask for Config register RegVal cfg2_Mask = 0x7000F000; // Read Only Register - replaceBits(cfg2_Mask, 0, 32, 0); + replaceBits(cfg2_Mask, 32, 0, 0); setRegMask(MISCREG_CONFIG2, cfg2_Mask); // Config3 @@ -253,7 +253,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_CONFIG3, cfg3); // Now, create Write Mask for Config register RegVal cfg3_Mask = 0; // Read Only Register - replaceBits(cfg3_Mask, 0, 32, 0); + replaceBits(cfg3_Mask, 32,0 , 0); setRegMask(MISCREG_CONFIG3, cfg3_Mask); // EBase - CPUNum @@ -264,7 +264,7 @@ ISA::configCP() // Now, create Write Mask for Config register RegVal EB_Mask = 0x3FFFF000;// Except Exception Base, the // entire register is read only - replaceBits(EB_Mask, 0, 32, 0); + replaceBits(EB_Mask, 32, 0, 0); setRegMask(MISCREG_EBASE, EB_Mask); // SRS Control - HSS (Highest Shadow Set) @@ -273,7 +273,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_SRSCTL, scsCtl); // Now, create Write Mask for the SRS Ctl register RegVal SC_Mask = 0x0000F3C0; - replaceBits(SC_Mask, 0, 32, 0); + replaceBits(SC_Mask, 32, 0, 0); setRegMask(MISCREG_SRSCTL, SC_Mask); // IntCtl - IPTI, IPPCI @@ -283,7 +283,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_INTCTL, intCtl); // Now, create Write Mask for the IntCtl register RegVal IC_Mask = 0x000003E0; - replaceBits(IC_Mask, 0, 32, 0); + replaceBits(IC_Mask, 32, 0, 0); setRegMask(MISCREG_INTCTL, IC_Mask); // Watch Hi - M - FIXME (More than 1 Watch register) @@ -292,7 +292,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_WATCHHI0, watchHi); // Now, create Write Mask for the IntCtl register RegVal wh_Mask = 0x7FFF0FFF; - replaceBits(wh_Mask, 0, 32, 0); + replaceBits(wh_Mask, 32, 0, 0); setRegMask(MISCREG_WATCHHI0, wh_Mask); // Perf Ctr - M - FIXME (More than 1 PerfCnt Pair) @@ -302,14 +302,14 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_PERFCNT0, perfCntCtl); // Now, create Write Mask for the IntCtl register RegVal pc_Mask = 0x00007FF; - replaceBits(pc_Mask, 0, 32, 0); + replaceBits(pc_Mask, 32, 0, 0); setRegMask(MISCREG_PERFCNT0, pc_Mask); // Random setMiscRegNoEffect(MISCREG_CP0_RANDOM, 63); // Now, create Write Mask for the IntCtl register RegVal random_Mask = 0; - replaceBits(random_Mask, 0, 32, 0); + replaceBits(random_Mask, 32, 0, 0); setRegMask(MISCREG_CP0_RANDOM, random_Mask); // PageGrain @@ -318,7 +318,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_PAGEGRAIN, pageGrain); // Now, create Write Mask for the IntCtl register RegVal pg_Mask = 0x10000000; - replaceBits(pg_Mask, 0, 32, 0); + replaceBits(pg_Mask, 32, 0, 0); setRegMask(MISCREG_PAGEGRAIN, pg_Mask); // Status @@ -337,7 +337,7 @@ ISA::configCP() setMiscRegNoEffect(MISCREG_STATUS, status); // Now, create Write Mask for the Status register RegVal stat_Mask = 0xFF78FF17; - replaceBits(stat_Mask, 0, 32, 0); + replaceBits(stat_Mask, 32, 0, 0); setRegMask(MISCREG_STATUS, stat_Mask); @@ -381,29 +381,29 @@ ISA::configCP() RegVal mask = 0x7FFFFFFF; // Now, create Write Mask for the Index register - replaceBits(mask, 0, 32, 0); + replaceBits(mask, 32, 0, 0); setRegMask(MISCREG_INDEX, mask); mask = 0x3FFFFFFF; - replaceBits(mask, 0, 32, 0); + replaceBits(mask, 32, 0, 0); setRegMask(MISCREG_ENTRYLO0, mask); setRegMask(MISCREG_ENTRYLO1, mask); mask = 0xFF800000; - replaceBits(mask, 0, 32, 0); + replaceBits(mask, 32, 0, 0); setRegMask(MISCREG_CONTEXT, mask); mask = 0x1FFFF800; - replaceBits(mask, 0, 32, 0); + replaceBits(mask, 32, 0, 0); setRegMask(MISCREG_PAGEMASK, mask); mask = 0x0; - replaceBits(mask, 0, 32, 0); + replaceBits(mask, 32, 0, 0); setRegMask(MISCREG_BADVADDR, mask); setRegMask(MISCREG_LLADDR, mask); mask = 0x08C00300; - replaceBits(mask, 0, 32, 0); + replaceBits(mask, 32, 0, 0); setRegMask(MISCREG_CAUSE, mask); } diff --git a/src/arch/x86/bios/intelmp.cc b/src/arch/x86/bios/intelmp.cc index f7e99949b..55088fd80 100644 --- a/src/arch/x86/bios/intelmp.cc +++ b/src/arch/x86/bios/intelmp.cc @@ -284,9 +284,9 @@ X86ISA::IntelMP::Processor::Processor(Params * p) : BaseConfigEntry(p, 0), if (p->bootstrap) cpuFlags |= (1 << 1); - replaceBits(cpuSignature, 0, 3, p->stepping); - replaceBits(cpuSignature, 4, 7, p->model); - replaceBits(cpuSignature, 8, 11, p->family); + replaceBits(cpuSignature, 3, 0, p->stepping); + replaceBits(cpuSignature, 7, 4, p->model); + replaceBits(cpuSignature, 11, 8, p->family); } X86ISA::IntelMP::Processor * diff --git a/src/arch/x86/bios/intelmp.hh b/src/arch/x86/bios/intelmp.hh index fbc5775ee..2ee400f4c 100644 --- a/src/arch/x86/bios/intelmp.hh +++ b/src/arch/x86/bios/intelmp.hh @@ -239,8 +239,8 @@ class IntAssignment : public BaseConfigEntry sourceBusID(_sourceBusID), sourceBusIRQ(_sourceBusIRQ), destApicID(_destApicID), destApicIntIn(_destApicIntIn) { - replaceBits(flags, 0, 1, polarity); - replaceBits(flags, 2, 3, trigger); + replaceBits(flags, 1, 0, polarity); + replaceBits(flags, 3, 2, trigger); } }; diff --git a/src/base/bitfield.hh b/src/base/bitfield.hh index 82c33071b..5cde01963 100644 --- a/src/base/bitfield.hh +++ b/src/base/bitfield.hh @@ -150,7 +150,9 @@ insertBits(T val, int bit, B bit_val) /** * A convenience function to replace bits first to last of val with bit_val - * in place. + * in place. It is functionally equivalent to insertBits. + * + * \note "first" is the MSB and "last" is the LSB. "first" >= "last" */ template inline -- 2.30.2