From c0e5d58a96d950fb308647097cd4af02d37c34e5 Mon Sep 17 00:00:00 2001 From: Giacomo Travaglini Date: Fri, 1 Mar 2024 15:32:40 +0000 Subject: [PATCH] dev: RegisterBank addRegistersAt for fragmented reg banks (#902) One of the limitations of the RegBank class is that it does not allow you to pass a non-contiguous set of registers. Its simplest form will just accept an initializer list of registers and it will store them in sequence. A more refined version [1] will optionally accept an offset value to be passed alongside the register reference. This is not meant to be used by the register bank to store the register at the provided offset. It is rather used by the bank to sanity check the register sits exactly at the provided range. The way to work around this for a fragemented register space is to explicitly allocate RAZ/RAO blocks as registers and to pass them to addRegisters together with the others. (See the SysSecCtrl [2] as an example) This makes it a bit tedious to model a register bank with gaps between its registers. First, the exact number and position of the gaps needs to be extraced from a spec. These sometimes report only implemented registers and their offset, and omit to document gaps/reserved space. So a developer needs to manually add register offset and size to check if all registers are contiguous. Second, these reserved register blocks need to be instantiated in the bank adding boilerplate code and affecting readibility. For these reasons we add a new registration method, called addRegisters*At*. It reuses the RegisterAdder class but this time the offset field is really used to instruct the bank where the register should be mapped. The method is templated and the template parameter tells the bank which register type should be used to fill the remaining space. We make the RegBank the owner of this filler space (registers are generated internally within addRegistersAt). [1]: https://github.com/gem5/gem5/blob/stable/src/dev/reg_bank.hh#L106 [2]: https://github.com/gem5/gem5/blob/stable/src/dev/arm/ssc.cc#L48 Change-Id: I614ae6e9eeb40b365ac9b6dd8b75abbfdb9cb687 Signed-off-by: Giacomo Travaglini --- src/dev/reg_bank.hh | 84 ++++++++++++++++++++++++++++++++----- src/dev/reg_bank.test.cc | 89 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 162 insertions(+), 11 deletions(-) diff --git a/src/dev/reg_bank.hh b/src/dev/reg_bank.hh index 9f53c44e38..fd67f7e4d7 100644 --- a/src/dev/reg_bank.hh +++ b/src/dev/reg_bank.hh @@ -1,4 +1,16 @@ /* + * Copyright (c) 2024 Arm Limited + * All rights reserved + * + * The license below extends only to copyright in the software and shall + * not be construed as granting a license to any other intellectual + * property including but not limited to intellectual property relating + * to a hardware implementation of the functionality of the software + * licensed hereunder. You may use the software subject to the license + * terms below provided that you ensure that this notice is replicated + * unmodified and in its entirety in all distributions of the software, + * modified or unmodified, in source code or in binary form. + * * Copyright 2020 Google, Inc. * * Redistribution and use in source and binary forms, with or without @@ -41,6 +53,7 @@ #include #include +#include "base/addr_range.hh" #include "base/bitfield.hh" #include "base/debug.hh" #include "base/logging.hh" @@ -91,17 +104,26 @@ * read only properties of the RegisterBank instance. * * To add actual registers to the RegisterBank (discussed below), you can use - * either the addRegister method which adds a single register, or addRegisters - * which adds an initializer list of them all at once. The register will be - * appended to the end of the bank as they're added, contiguous to the - * existing registers. The size of the bank is automatically accumulated as - * registers are added. + * either the addRegister method which adds a single register, or + * addRegisters/addRegistersAt which add an initializer list of them all at + * once. * - * When adding a lot of registers, you might accidentally add an extra, - * or accidentally skip one in a long list. Because the offset is handled - * automatically, some of your registers might end up shifted higher or lower - * than you expect. To help mitigate this, you can set what offset you expect - * a register to have by specifying it as an offset, register pair. + * For addRegister and addRegisters, the registers will be appended to + * the end of the bank as they're added, contiguous to the existing registers. + * The size of the bank is automatically accumulated as registers are added. + * + * For addRegistersAt, an offset field is used to instruct the bank where the + * register should be mapped. So the entries of the initializer list will be a + * set of offset-register pair. The method is templated and the template + * parameter tells the bank which register type should be used to fill the + * remaining space. We make the RegBank the owner of this filler space + * (registers are generated internally within addRegistersAt). + * + * When adding a lot of registers with addRegisters, you might accidentally add + * an extra, or accidentally skip one in a long list. Because the offset is + * handled automatically, some of your registers might end up shifted higher or + * lower than you expect. To help mitigate this, you can set what offset you + * expect a register to have by specifying it as an offset, register pair. * * addRegisters({{0x1000, reg0}, reg1, reg2}); * @@ -887,6 +909,7 @@ class RegisterBank : public RegisterBankBase Addr _base = 0; Addr _size = 0; const std::string _name; + std::vector> owned; public: @@ -955,6 +978,47 @@ class RegisterBank : public RegisterBankBase } } + template + void + addRegistersAt(std::initializer_list adders) + { + panic_if(std::empty(adders), + "Adding an empty list of registers to %s?", name()); + + std::vector vec{adders}; + std::sort(vec.begin(), vec.end(), + [] (const auto& first, const auto& second) { + return first.offset.value() < second.offset.value(); + } + ); + + for (auto &adder: vec) { + assert(adder.offset && adder.reg); + const Addr offset = _base + _size; + + // Here we check if there is a hole (gap) between the start of the + // new register and the end of the current register bank. A positive + // gap means we need to fill the hole with the provided filler. + // If gap is negative, it means previous register is overlapping + // with the start address of the current one, and we should panic + if (int gap = adder.offset.value() - offset; gap != 0) { + panic_if(gap < 0, "Overlapping register added to the bank: %s\n", + adder.reg.value()->name()); + + // Use the filler register to fill the address range gap + AddrRange hole_range(offset, offset + gap); + owned.push_back(std::make_unique(hole_range.to_string(), gap)); + _offsetMap.emplace(offset, *owned.back().get()); + _size += gap; + } + + // Now insert the register at the specified offset. + auto *reg = adder.reg.value(); + _offsetMap.emplace(adder.offset.value(), *reg); + _size += reg->size(); + } + } + void addRegister(RegisterAdder reg) { addRegisters({reg}); } Addr base() const { return _base; } diff --git a/src/dev/reg_bank.test.cc b/src/dev/reg_bank.test.cc index c618ef16d4..c799fd5b03 100644 --- a/src/dev/reg_bank.test.cc +++ b/src/dev/reg_bank.test.cc @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020 ARM Limited + * Copyright (c) 2020, 2024 Arm Limited * All rights reserved * * The license below extends only to copyright in the software and shall @@ -1100,6 +1100,93 @@ TEST_F(RegisterBankTest, AddRegistersWithOffsetChecks) EXPECT_EQ(emptyBank.size(), 12); } +/** + * This test is using addRegistersAt method to store + * overlapping registers to the empty bank. This should not + * be permitted and the method should panic + * + * [ reg2 ] + * [ reg1 ] | + * [ reg0 ] | | + * | | | | + * 0x0 0x4 0x6 0x8 + */ +TEST_F(RegisterBankTest, AddRegistersAtOffsetDeath) +{ + gtestLogOutput.str(""); + + auto base = emptyBank.base(); + EXPECT_ANY_THROW( + emptyBank.addRegistersAt( + {{base + 0x0, reg0}, + {base + 0x2, reg1}, + {base + 0x4, reg2}})); + + std::string actual = gtestLogOutput.str(); + EXPECT_THAT(actual, HasSubstr("Overlapping register")); + EXPECT_THAT(actual, HasSubstr("reg1")); +} + +/** + * This test is using addRegistersAt method to store + * contiguous registers to the empty bank, similarly + * to what we would do when relying on addRegisters. + * The test will check size is updated consistently + * with the latter method + * + * [ reg0 ][ reg1 ][ reg2 ] + * | | | | + * 0x0 0x4 0x8 0xc + */ +TEST_F(RegisterBankTest, AddRegistersAtOffsetContiguous) +{ + auto base = emptyBank.base(); + EXPECT_EQ(emptyBank.size(), 0); + emptyBank.addRegistersAt( + {{base + 0x0, reg0}, + {base + 0x4, reg1}, + {base + 0x8, reg2}}); + EXPECT_EQ(emptyBank.size(), 0xc); +} + +/** + * This test is using addRegistersAt method to store + * non-contiguous registers to the empty bank. + * As the RegisterRao data type is passed as a template + * argument, the gaps between the registers are filled + * with rao registers. + * We check raos are correctly inserted + * + * [reg0][rao0][reg1][rao1][reg2] + * | | | | + * 0x0 0x8 0x10 0x14 + */ +TEST_F(RegisterBankTest, AddRegistersAtOffsetSparse) +{ + auto base = emptyBank.base(); + EXPECT_EQ(emptyBank.size(), 0); + emptyBank.addRegistersAt( + {{base + 0x0, reg0}, + {base + 0x8, reg1}, + {base + 0x10, reg2}}); + EXPECT_EQ(emptyBank.size(), 0x14); + + emptyBank.read(base + 0x0, buf.data(), 4); + EXPECT_EQ(reg0.get(), *reinterpret_cast(buf.data())); + + emptyBank.read(base + 0x4, buf.data(), 4); + EXPECT_EQ(0xffffffff, *reinterpret_cast(buf.data())); + + emptyBank.read(base + 0x8, buf.data(), 4); + EXPECT_EQ(reg1.get(), *reinterpret_cast(buf.data())); + + emptyBank.read(base + 0xc, buf.data(), 4); + EXPECT_EQ(0xffffffff, *reinterpret_cast(buf.data())); + + emptyBank.read(base + 0x10, buf.data(), 4); + EXPECT_EQ(reg2.get(), *reinterpret_cast(buf.data())); +} + TEST_F(RegisterBankTest, BadRegisterOffsetDeath) { gtestLogOutput.str("");