Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve comments around check_mem_access and mmio_load/store #1724

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve comments around check_mem_access and mmio_load/store #1724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions dv/cosim/cosim.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

// Information about a dside transaction observed on the DUT memory interface
struct DSideAccessInfo {
// set when the access is a store, in which case `data` is the store data from
// Set when the access is a store, in which case `data` is the store data from
// the DUT. Otherwise `data` is the load data provided to the DUT.
bool store;
// `addr` is the address and must be 32-bit aligned. `data` and `be` are
Expand All @@ -20,7 +20,7 @@ struct DSideAccessInfo {
uint32_t addr;
uint32_t be;

// set if an error response to the transaction is seen.
// Set to indicate an error response to the attempted DUT memory access.
bool error;

// `misaligned_first` and `misaligned_second` are set when the transaction is
Expand All @@ -30,9 +30,9 @@ struct DSideAccessInfo {
//
// For example an unaligned 32-bit load to 0x3 produces a transaction with
// `addr` as 0x0 and `misaligned_first` set to true, then a transaction with
// `addr` as 0x4 and `misaligned_second` set to true. An unaligned 16-bit load
// to 0x01 only produces a transaction with `addr` as 0x0 and
// `misaligned_first` set to true, there is no second half.
// `addr` as 0x4 and `misaligned_second` set to true.
// An unaligned 16-bit load to 0x01 only produces a transaction with `addr`
// as 0x0 and `misaligned_first` set to true, there is no second half.
bool misaligned_first;
bool misaligned_second;
};
Expand Down
168 changes: 134 additions & 34 deletions dv/cosim/spike_cosim.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,40 +71,56 @@ SpikeCosim::SpikeCosim(const std::string &isa_string, uint32_t start_pc,
char *SpikeCosim::addr_to_mem(reg_t addr) { return nullptr; }

bool SpikeCosim::mmio_load(reg_t addr, size_t len, uint8_t *bytes) {
// Load from the ISS memory model, while checking if the access is
// consistent with the observed DUT memory acceses.
// Return false if any errors occur, or the checking fails.
// Within Spike (mmu_t::load_slow_path), this causes a load_access_fault.

// Spike does not differentiate between iside and dside memory accesses
// (as ibex does) so we make an educated guess which is which, and
// then after fetching the data, check against the recorded DUT access
// that should be queued as pending.
//
// Spike may attempt to access up to 8-bytes from the PC when fetching, so
// assume as a dside access when it falls outside that range.
// N.B. Heuristic
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth mentioning that is the heuristic doesn't get it right it will produce false failures. In general we don't expect to see the scenarios that generate such false failures in practise and if they prove to be an issue we'll rethink this mechanism.

uint32_t pc = processor->get_state()->pc;
bool is_probably_dmem_access = (addr < pc || addr >= (pc + 8));
// Check we don't have a heuristic mismatch (this may be a false-positive but
// worth failing explicity if it is seen)
assert(!(is_probably_dmem_access && pending_iside_error));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to check if the pending iside error address matches the incoming address (pending_iside_err_addr == aligned_addr check you can see below).

Maybe add a new bool that combines the two as it'll be used in a couple of places?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes good point. Will do!


// Attempt to make an identical load from the bus memory device.
// This can fail e.g. if accessing an unmapped address (see SpikeCosim::add_memory)
bool bus_error = !bus.load(addr, len, bytes);

bool dut_error = false;
// If the DUT encountered a bus error for its access, or the checking failed,
// this is a dut_error.
bool dut_error = is_probably_dmem_access ? (check_mem_access(false, addr, len, bytes) != kCheckMemOk) : false;

// Incoming access may be an iside or dside access. Use PC to help determine
// which.
uint32_t pc = processor->get_state()->pc;
uint32_t aligned_addr = addr & 0xfffffffc;

if (pending_iside_error && (aligned_addr == pending_iside_err_addr)) {
// Check if the incoming access is subject to an iside error, in which case
// assume it's an iside access and produce an error.
if ( pending_iside_error &&
(pending_iside_err_addr == aligned_addr)) {
// Check if the aligned PC matches with the aligned address or an
// incremented aligned PC (to capture the unaligned 4-byte instruction
// case).
// If the pending_iside_error has been set, produce a dut_error.
pending_iside_error = false;
dut_error = true;
} else if (addr < pc || addr >= (pc + 8)) {
// Spike may attempt to access up to 8-bytes from the PC when fetching, so
// only check as a dside access when it falls outside that range.

// Otherwise check if the aligned PC matches with the aligned address or an
// incremented aligned PC (to capture the unaligned 4-byte instruction
// case). Assume a successful iside access if either of these checks are
// true, otherwise assume a dside access and check against DUT dside
// accesses. If the RTL produced a bus error for the access, or the
// checking failed produce a memory fault in spike.
dut_error = (check_mem_access(false, addr, len, bytes) != kCheckMemOk);
}
};

return !(bus_error || dut_error);
}

bool SpikeCosim::mmio_store(reg_t addr, size_t len, const uint8_t *bytes) {
// Store to the ISS memory model, while checking if the access is
// consistent with the observed DUT memory acceses.
// Return false if any errors occur, or the checking fails.
// Within Spike (mmu_t::store_slow_path), this causes a store_access_fault.

bool bus_error = !bus.store(addr, len, bytes);
// If the RTL produced a bus error for the access, or the checking failed
// produce a memory fault in spike.
// If the DUT encountered a bus error for its access, or the checking failed,
// this is a dut_error.
bool dut_error = (check_mem_access(true, addr, len, bytes) != kCheckMemOk);

return !(bus_error || dut_error);
Expand Down Expand Up @@ -519,12 +535,76 @@ void SpikeCosim::fixup_csr(int csr_num, uint32_t csr_val) {

SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
bool store, uint32_t addr, size_t len, const uint8_t *bytes) {
// Because Spike is only fed instructions that have been made available
// on the RVFI by IBEX (retired and synchronously trapping), spike will
// always run slightly-behind the ibex, and hence we need to store memory
// accesses and errors when they are seen by the testbench, and then pop
// them from the queues when spike actually executes them.
// This delay may be a few cycles in general, but may never happen if
// the access was iside-speculative.
//
// Whenever a load/store is attempted by spike, we check the following
// conditions....
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: 4 dots instead of 3

//
// 0) First, if an iside error has been detected and
// pending_iside_error=True, a dut_error is generated within mmio_load(), and
// this function is not called. This function is called if the heuristics
// believe the access is a dmem access. (dmem may be accessed with stores or
// loads, while imem is only loads.)
//
// As well as checking for correspondence between the spike access and the
// queued DUT dmem access, we also want to pass along the error if the dmem
// access response came back with an error from the DUT memory system bus. So,
// if all the below checks pass, the final step is to check for a marked error
// in the queued dmem access, and pass it along to Spike if so.
//
// The correspondence checks are as follows:
//
// 1) Check there is actually a dmem access in the queue at all.
// Pop the top dmem access from the queue, and then...
// 2) Check the addr of spike access matches that of the queued dmem access.
// 3) Check the type of spike access (load/store) matches that of the queued
// dmem access.
//
// If the dut dmem access was misaligned :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space before colon

// (Because ibex dmem can make misaligned and byte-enabled accesses, and spike
// will not make accesses in this way, we need to check the final result of
// the two systems is the same. Spike will make multiple single-byte accesses
// for the same end-result). Therefore, this function (and mmio_load/store)
// may be called multiple times for a single dut dmem access.
// By checking the byte-enables of the memory accesses, we can check that..
// 4) The spike-accesses do not repeat accessing any bytes the dut accesses.
// 5) The spike-access does not touch any bytes not accessed by the dut.
// ..and while doing this, record the bytes accessed so next time around we
// can make the checks 4) and 5).
// When all of the bytes have been accessed matching the queued access,
// discard this recorded data in preparation for the next access.
//
// If the dut dmem access is aligned :
// 6) Check that the spike-accessed bytes match the dut dmem byte-enables in
// one go.
//
// Until this point, we have not even checked the data of the accesses.
// For accesses not tagged with a dut error:
// 7) Check the data of the spike access matches that of the ibex dmem access,
// for all stores and for loads without errors.
//
// For dut dmem accesses with errors, we can't check the data, but one more
// possible sanity test is that for misaligned accesses...
// 8) Check the second access exists in the pending queue
// 9) Check the second (next) access has the correct address
// After 9), pop both dmem accesses from pending. Only return a single error if needed.
//
// Finally, pass along the error to spike if the dut access at the top of the
// queue was marked with one (DSideAccessInfo.error)

assert(len >= 1 && len <= 4);
// Expect that no spike memory accesses cross a 32-bit boundary
assert(((addr + (len - 1)) & 0xfffffffc) == (addr & 0xfffffffc));

std::string iss_action = store ? "store" : "load";

// 1)
// Check if there are any pending DUT accesses to check against
if (pending_dside_accesses.size() == 0) {
std::stringstream err_str;
Expand All @@ -540,6 +620,7 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(

std::string dut_action = top_pending_access_info.store ? "store" : "load";

// 2)
// Check for an address match
uint32_t aligned_addr = addr & 0xfffffffc;
if (aligned_addr != top_pending_access_info.addr) {
Expand All @@ -552,6 +633,7 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
return kCheckMemCheckFailed;
}

// 3)
// Check access type match
if (store != top_pending_access_info.store) {
std::stringstream err_str;
Expand All @@ -575,6 +657,7 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
// accesses where the DUT will generate an access covering all bytes within
// an aligned 32-bit word.

// 4)
// Check bytes accessed this time haven't already been been seen for the DUT
// access we are trying to match against
if ((expected_be & top_pending_access.be_spike) != 0) {
Expand All @@ -590,6 +673,7 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
return kCheckMemCheckFailed;
}

// 5)
// Check expected access isn't trying to access bytes that the DUT access
// didn't access.
if ((expected_be & ~top_pending_access_info.be) != 0) {
Expand All @@ -610,6 +694,7 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
pending_access_done = true;
}
} else {
// 6)
// For aligned accesses bytes from spike access must precisely match bytes
// from DUT access in one go
if (expected_be != top_pending_access_info.be) {
Expand All @@ -626,9 +711,10 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
pending_access_done = true;
}

// 7)
// Check data from expected access matches pending DUT access.
// Data is ignored on error responses to loads so don't check it. Always check
// store data.
// Always check store data.
// (Data is ignored on error responses to loads so don't check it.)
if (store || !top_pending_access_info.error) {
// Combine bytes into a single word
uint32_t expected_data = 0;
Expand All @@ -647,10 +733,10 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(

if (expected_data != masked_dut_data) {
std::stringstream err_str;
err_str << "DUT generated " << iss_action << " at address " << std::hex
<< top_pending_access_info.addr << " with data "
<< masked_dut_data << " but data " << expected_data
<< " was expected with byte mask " << expected_be;
err_str << "DUT generated " << iss_action << " dmem access at address "
<< std::hex << top_pending_access_info.addr << " with data "
<< masked_dut_data << " but ISS access(es) got data " << std::hex << expected_data
<< " with equivalent byte mask of " << std::hex << expected_be;

errors.emplace_back(err_str.str());

Expand All @@ -659,15 +745,24 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
}

bool pending_access_error = top_pending_access_info.error;
// If the pending dside access has an error, then we don't need to check to data
// of the accesses. However, we can still check if the correct number of accesses
// took place, and then immediately pop the erroneous dmem access(es) off the top
// of the queue.

if (pending_access_error && misaligned) {
// When misaligned accesses see an error, if they have crossed a 32-bit
// boundary DUT will generate two accesses. If the top pending access from
// the DUT was the first half of a misaligned access which accesses the top
// byte, it must have crossed the 32-bit boundary and generated a second
// access
if (top_pending_access_info.misaligned_first &&
// If requested to make a misaligned access that crosses a 32-bit
// boundary (this could be a word or halfword access), the DUT
// LSU generates two word-aligned accesses with appropriate
// byte-enables set.
//
// If the top pending access from the DUT was the first half of a misaligned
// access which accesses the top byte, it must have crossed the 32-bit
// boundary and generated a second access
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: missing .

if ( top_pending_access_info.misaligned_first &&
((top_pending_access_info.be & 0x8) != 0)) {

// 8)
// Check the second access DUT exists
if ((pending_dside_accesses.size() < 2) ||
!pending_dside_accesses[1].dut_access_info.misaligned_second) {
Expand All @@ -681,6 +776,7 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
return kCheckMemCheckFailed;
}

// 9)
// Check the second access had the expected address
if (pending_dside_accesses[1].dut_access_info.addr !=
(top_pending_access_info.addr + 4)) {
Expand Down Expand Up @@ -712,6 +808,10 @@ SpikeCosim::check_mem_result_e SpikeCosim::check_mem_access(
pending_dside_accesses.erase(pending_dside_accesses.begin());
}

// If we got this far, just check if the DUT access was marked as
// an error, and pass this on to Spike if so as a dut_error.
// (If we just erased the top dmem access from the pending queue, this
// returned error refers to that.)
return pending_access_error ? kCheckMemBusError : kCheckMemOk;
}

Expand Down