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.

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

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

Conversation

hcallahan-lowrisc
Copy link
Contributor

Added more comments around the function of SpikeCosim::check_mem_access() and SpikeCosim::mmio_load()/mmio_store().
Minor refactoring for clarity and flow with the comments.

I guess it could be argued that this should be in the /doc directory, but I'm indifferent to either. Feedback welcome.

Copy link
Contributor

@ctopal ctopal left a comment

Choose a reason for hiding this comment

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

LGTM and thanks a lot for clarifying how things work! I was wondering if there would be a better way of documenting this rather than in putting it directly into the code as comments though.

Also, you might want to use clang formatter for lint failure.

dv/cosim/spike_cosim.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@GregAC GregAC left a comment

Choose a reason for hiding this comment

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

A good improvement here. Though I'd prefer not to alter the behaviour of the checking, was it intentional you altered the behaviour?

}
// case).
uint32_t aligned_addr = addr & 0xfffffffc;
if ( 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 does alter the behaviour from what it was previously. Now the address needs to the fail the 'is_probably_dmem_access' heuristic before we check if the address matches a pending iside error. Previously if it matched a pending iside error it automatically assumed it was an iside error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this to be an assertion failure if the is_probably_dmem_access and pending_iside_error are both true. I think it is probably better to be explicit about what happens when something goes wrong here. Does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup sounds good

//
// 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.

dv/cosim/spike_cosim.cc Outdated Show resolved Hide resolved
@hcallahan-lowrisc
Copy link
Contributor Author

Rebased

Copy link
Collaborator

@GregAC GregAC left a comment

Choose a reason for hiding this comment

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

Thanks for this @hcallahan-lowrisc, looks good once we've fixed up the assertion (see comment).

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!

}
// case).
uint32_t aligned_addr = addr & 0xfffffffc;
if ( 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.

Yup sounds good

@GregAC
Copy link
Collaborator

GregAC commented Oct 10, 2022

@hcallahan-lowrisc any progress on this?

@hcallahan-lowrisc
Copy link
Contributor Author

@hcallahan-lowrisc any progress on this?

Not yet. Will try and get it closed soon.

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Great work! Just a few nits, but otherwise I think it looks good.

// 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

// 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

//
// 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 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants