Added bus blocker to deny requests to dmInner when dmactive=0

[mwachs5]

Related issue:

Type of change: feature request

Impact:API modification

Development Phase: implementation

Release Notes

dmactive is now used to deny transactions (return error) that would pass across the DMI interface when dmactive=0. This is to prevent accesses to the interface when it may be in reset and/or have no running clock which might cause it to hang.

This will result in an error response and will prevent reading the DMSTATUS fields (e.g. version) before dmactive is set to 1.

This also changes the interface to make the TLError device more flexible to allow a reduced-size TLError device to be instantiated in the Debug Module.

This also changes the TLBusBypass to report its minLatency as the min of both the original and bypassed paths minLatency.

[mwachs5]

It occurs to me that the JTAG DTM has an assertion that error will never be returned. This code needs to be revisited before this PR can be merged (Note: I did that)

[terpstra]

unsound

[terpstra]

That is a lie. The implementation clearly has minLatency=0.

[mwachs5]

then it has always been lying.

[mwachs5]

previously DevNull device always reported that it had minLatency=1. So if this is actually 0 then it should report as 0.

[mwachs5]

This is the line I am referring to:

rocket-chip/src/main/scala/devices/tilelink/DevNull.scala

Line 55 in b0ed2be

minLatency = 1))) // no bypass needed for this device

[ernie-sifive]

Other than minLatency, the rest looks good.

[mwachs5]

@terpstra can you please re-review how I handled passing the bufferError parameter and communicating the minLatency appropriately.

@ernie-sifive can you please review the change I made to turn assertion into cover property for the JTAG Debug Transport

[ernie-sifive]

The assert -> cover change looks good to me.

[mwachs5]

This is failing regressions because of unhandled errors. I am not sure what is not handling this, presumably OpenOCD?

Warn : Adapter driver 'remote_bitbang' did not declare which transports it allows; assuming legacy JTAG-only
Info : only one transport option; autoselect 'jtag'
Info : Initializing remote_bitbang driver
Info : Connecting to localhost:39085
Info : remote_bitbang driver initialized
Info : This adapter doesn't support configurable speed
Info : JTAG tap: riscv.cpu tap/device found: 0x00000001 (mfg: 0x000 (<invalid>), part: 0x0000, ver: 0x0)
Error: fflush: Broken pipe
Error: read: count=-1, error=Broken pipe
Error: dmi_scan failed jtag scan
Error: failed read (NOP) at 0x11, status=2
Info : Listening on port 40231 for gdb connections
Error: Target not examined yet

[ernie-sifive]

@mwachs5 The OpenOCD failure is from reading dmstatus before setting dmactive to 1. This was fixed many months ago in riscv-openocd. We need to update rocket-tools with the latest riscv-openocd. I tried to do a PR to do this but don't have permission for rocket-tools.

[aswaterman]

@ernie-sifive It looks like the permissions for that repository were misconfigured (the intent was that people who have write access to this repo also have it over there). It should be fixed now.

[ernie-sifive]

@mwachs5 rocket-chip commit 177b5b8 fixes the OpenOCD issue. (Edit: Well, not really...the resulting tools are missing riscv-unknown-elf-objcopy).

There is another case of reading dmstatus while dmactive=0 that is not handling an error return and is also causing CI failures in rocket-chip. Once again I don't have permission to push a branch to the repo (@aswaterman?). In riscv-isa-sim/fesvr/dtm.cc, function dtm_t::resume:

if (running) {
  write(DMI_DMCONTROL, 0);
  // Read dmstatus to avoid back-to-back writes to dmcontrol.                                                                               
  read(DMI_STATUS);   
 }

I would propose changing the write to dmcontrol to write dmactive=1.

@mwachs5 You should also be aware that an assertion fires when the bus blocker error device responds. Something is still wrong with the minlatency settings in the blocker.

Assertion failed: 'A' and 'D' concurrent, despite minlatency 3 (connected at BusBypass.scala:32:12)
    at Monitor.scala:49 assert(cond, message)

[mwachs5]

Thanks Ernie. I will take a look at the minlatency issue early this week.

[mwachs5]

What needs riscv-unknown-elf-objcopy? This is required by riscv-tests/debug or the rest of rocket-chip code?

I can look into just building a specific commit of riscv-openocd instead of relying on the version in riscv-tools...

[ernie-sifive]

objcopy is needed by sifive closed-source configs that refer to the riscv-tools under rocket-chip. My attempt at updating riscv-tools (chipsalliance/rocket-tools@7f46e7e) changed only openocd and only as far as the commit that fixed the dmstatus access with dmactive=0 (riscv-collab/riscv-openocd@906635c).

[mwachs5]

@ernie-sifive do you still need help with the necessary fesvr change? Or are you just looking for my help (on this PR) for the minLatency issue?

[ernie-sifive]

@mwachs5 I don't have permission to push branches to any of the rocket-tools submodules. I think the most expedient way to get this PR to pass CI tests is to branch riscv-openocd and riscv-fesvr from the commits currently pointed to by rocket-tools, make the necessary change to each, and then update rocket-tools to point to these branch commits.

[aswaterman]

@ernie-sifive you don’t need write permissions to create pull requests. You can fork the repo to your own github account then make PRs back to the original.

[mwachs5]

@aswaterman I attempted to do as suggested and just bump the riscv-openocd version and then add the fix to riscv-fesvr, but seems riscv/riscv-fesvr is locked down and can't be modified anymore due to being deprecated...? Am I understanding this correctly?

[mwachs5]

Attempting to move this forwards with riscv-software-src/riscv-isa-sim#392. I am still unclear why the lack of objcopy is a problem, since it seems like the current version of rocket-tools pointed to by rocket-chip also doesn't include objcopy.

This will require us to bump rocket-tools past the point where riscv-fesvr was merged with riscv-isa-sim. I don't know why that was not done before or what complications it will bring. @ernie-sifive it looks like you attempted to do this in your Debug reset PR

[aswaterman]

Hopefully there should be no problem with moving Spike up to that point.

…

On Mon, Feb 10, 2020 at 11:43 AM Megan Wachs ***@***.***> wrote: Attempting to move this forwards with riscv-software-src/riscv-isa-sim#392 <riscv-software-src/riscv-isa-sim#392>. I am still unclear why the lack of objcopy is a problem, since it seems like the current version of rocket-tools pointed to by rocket-chip also doesn't include objcopy. This will require us to bump rocket-tools past the point where riscv-fesvr was merged with riscv-isa-sim. I don't know why that was not done before or what complications it will bring. @ernie-sifive <https://github.com/ernie-sifive> it looks like you attempted to do this in your Debug reset PR — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2205?email_source=notifications&email_token=AAH3XQSKFHFZUKIBWLDLDLTRCGG4LA5CNFSM4JRO3WZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELJN3EQ#issuecomment-584244626>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH3XQUZVV2OCHD7B2UADH3RCGG4LANCNFSM4JRO3WZQ> .

[ernie-sifive]

@mwachs5 Yes, I attempted this but was never able to achieve a consistent set of tools. The objcopy problem was seen in closed-source that depends on rocket-tools.

[mwachs5]

I figured out that the minLatency issue is because TLBypassNode does not have the same behavior as TLXbar in its handling of min latency, it just grabs the parameters from the "real" (non bypassed) device vs combining information from the real vs bypassed device. At least for minLatency I am not sure why you would not want to take the actual minimum.

[mwachs5]

This is still failing on tests loading with DTM with an assertion failure that dtm is not supposed to return error. Will look into it.

[mwachs5]

This is working, but the version of rocket-tools is not passing its own regressions, so will need to resolve that before merging.

[mwachs5]

The current version of rocket-tools is now passing its own regressions. This is ready to merge once more downstream testing is complete and once @terpstra removes his blocking review.

[terpstra]

I'm not sure about this comment. AFAICT, this device never responds. So minLatency=infinity would be more accurate. This device (TLDeadlock) also flagrantly violates the TL forward progress requirements, but I assume based on the name that this is intentional. What is this device for?

[mwachs5]

I don't know I assume you had created it? Maybe @hcook knows?

[mwachs5]

This is now failing due to missing pexpect. Not sure why this suddenly was a problem...

[mwachs5]

@richardxia i think the tool which required python-pexpect package to be installed in .travis is now python3. Does that mean I need to install a different pexpect package?

[richardxia]

@richardxia i think the tool which required python-pexpect package to be installed in .travis is now python3. Does that mean I need to install a different pexpect package?

If you switched from Python 2 to Python 3, and if you installed the Python package pexpect via Ubuntu's official apt repository rather than from the Python Package Index, then yes, you need to install python3-pexpect, which is Ubuntu's packaging of Python 3's pexpect library.

Also, just for the record, I have never touched the Travis CI tests for rocket-chip ever (which you can confirm by checking the commit history of .travis.yml here :P), so I don't really know much about how they are configured, but if you want me to dig into them and figure out how they work, I'm happy to do so. Just wanted to make it clear here that I don't have any pre-existing expertise of rocket-chip's CI, only pre-existing expertise of the intersection of Travis CI, Ubuntu, and Python.

[mwachs5]

This is failing due to pexpect.exceptions.ExceptionPexpect: The command was not found or was not executable: riscv64-unknown-elf-gdb.. Something still off with my tools install.

[mwachs5]

hooray. this is finally passing. Will merge with squash-and-merge once some downstream tests pass