arm,dev: add (dummy) ISecR support to the PL390 GIC

Information
Submitter:	Bjoern A. Zeeb
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	andysan

Description

Add (dummy) support for Interrupt Security Registers to allow
software to read/write them even though we do not properly
implement checks yet. This avoids hitting panic()s and
seems to be `good enough' to get certain software running
happily.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Isn't there some code missing here?	Andreas Sandberg	April 5, 2016, 5:04 a.m.	Dropped

Thanks for looking into this!

I'd suggest that you implement this as a GICv1 without security extensions. According to the architecture spec, the ICDISRn registers should be RAZ/WI in that case, so no need to store additional state.

Bjoern A. Zeeb March 16, 2016, 6:56 a.m. (March 16, 2016, 6:56 a.m.)

Well the problem seems to be that a pfr(1) returns with & 0x000000f0 being true, which is how FreeBSD (and probably everyone else) would detect whether SecExt are present. So I assume we should drop the bits as well from ArmISA.py id_pfr1 ( id_pfr1 = Param.UInt32(0x00001011, "Processor Feature Register 1") )?
At least currently assuming they are not there is not an option.

Side note: FreeBSD code doesn't all do the right thing everywhere either depending on that flag but for as long as it's there we do and gem5 doesn't.

Andreas Sandberg May 12, 2016, 7:09 a.m. (May 12, 2016, 7:09 a.m.)

Good point. We obviously shouldn't set the TZ bits in PFR unless security extensions have been enabled. I have tasked someone with looking into this issue and I hope to have a patch shortly.

Andreas Sandberg May 12, 2016, 7:39 a.m. (May 12, 2016, 7:39 a.m.)

Ok, I have done some more digging. According to the GICv2 manual:

  * A processor that implements the ARM Virtualization Extensions must also implement the ARM Security Extensions.
  * A GIC that implements the GIC Virtualization Extensions is not required to implement the GIC Security Extensions.

My interpretation of this is that is incorrect for an operating system to assume that the GIC supports security extensions just because the CPU does. (That doesn't mean that we should do the right thing in ID_PFRx though.)

We are doing a fairly substantial refresh of the GIC code base at the moment and we will implement GICD_IGROUPr (ICDISR) as a GICv1 without security extensions, which would implement a superset of this this patch. I don't have an ETA yet, but I suspect we can post it externally within a couple of weeks.

Bjoern A. Zeeb May 16, 2016, 11:53 a.m. (May 16, 2016, 11:53 a.m.)

OK, I looked at as much bits as I could find. It seems for the GIC in gem5 the TYPER (not named that way; that's a v1 vs. v2 thing I guess) register does the right thing and not set the Security Extensions bit. I have adjusted the arm(v6/7) and the armv8 code for the GIC (v1/2) on FreeBSD to check for that bit before trying to issue writes for ICDISRn.

With rgards to the CPU I can't say much apart from that wasn't an issue so far. I am certain FreeBSD does not have proper checks for the SecExt being there in all places it needs to have them either, but that's not a gem5 issue.

I'll discard this one and upstream the OS improvement after it's been reviewed there. Thanks for pointing out the right GIC bits here.

src/dev/arm/gic_pl390.cc (Diff revision 1)

Isn't there some code missing here?

Show all issues

Bjoern A. Zeeb March 16, 2016, 6:56 a.m. (March 16, 2016, 6:56 a.m.)

Yes I know. I had not intentions to do the 100% right thing throughout the entire ARM platform. I am just working around my problem and leaving the comment for someone to finish the work who knows better and can test better than I can ;-)

You have a pending review.

Review Board 2.0.15

This change has been discarded.

Screenshots

Files

Issue Summary

Status: Discarded