Drive protection user bits on TL #2383

jsmithsf · 2020-03-30T11:12:59Z

Related issue:

Type of change: other enhancement
Drive some relevant AMBA protection field information from Rocket.

Impact: API modification
When Rocket is configured to not use virtual memory, it will drive extra information on the TL user bits that can be converted into the AMBA protection fields.

Development Phase: implementation

Release Notes

The instruction vs data, cacheable vs. non-cacheable, and privileged vs. non-privileged information should be driven now.

Now we will mark an access as privileged if the core is in machine mode, or if the access is to a cacheable region.

jsmithsf · 2020-03-31T17:27:55Z

The changes are pretty minor, just looking for an approval from someone

mwachs5 · 2020-03-31T17:30:36Z

src/main/scala/rocket/DCache.scala

+  tl_out_a.bits.user.lift(AMBAProt).foreach { x =>
+    val user_bit_cacheable = edge.manager.supportsAcquireTFast(access_address, a_size)
+
+    x.privileged  := s2_req.dprv === PRV.M || user_bit_cacheable


so all cacheable accesses are also by definition privileged?

The relevant point from the AMBA spec is that if a master cannot generate accurate protection information, it is recommended that we mark the transaction as privileged. Because caches like the L2 can merge requests from multiple different privilege modes, we can't accurately provide the protection information.

mwachs5 · 2020-03-31T17:31:52Z

src/main/scala/rocket/ICache.scala

+  tl_out.a.bits.user.lift(AMBAProt).foreach { x =>
+    val user_bit_cacheable = edge_out.manager.supportsAcquireTFast(refill_paddr, lgCacheBlockBytes.U)
+
+    x.privileged  := io.privileged || user_bit_cacheable    // privileged if machine mode or memory port


oh i see the comment here. so we are assuming everythign cacheable is going to memory port? If not this comment is a bit misleading

Yes, currently we are assuming any cacheable thing is going to the memory port. During a short discussion that I had with @hcook he mentioned that it's possible this may change in the future if we add other types of ports.

It seems it should be the opposite for safety, i.e. if you cannot figure out the exact privilege level, you should assume the lowest level of privilege.

I don't understand ARM's reasoning behind their decision, but we are making this change to be more compatible with ARM behavior. So I think for that reason we should follow the same thing.

I am confused. Since we cache everything on fetch, why is privileged not just tied to true?

@terpstra, @aswaterman posed a similar question but was unsure.

mwachs5 · 2020-03-31T17:58:50Z

If @daveparry can review i can rubber stamp (assuming his review is not strong enough)

terpstra · 2020-03-31T18:53:49Z

src/main/scala/rocket/DCache.scala

+    x.fetch       := false.B
+    x.secure      := true.B
+    x.bufferable  := false.B
+    x.modifiable  := false.B


I wonder whether bufferable/modifiable should be !user_bit_cacheable ?

terpstra · 2020-03-31T18:55:32Z

src/main/scala/rocket/ICache.scala

+  tl_out.a.bits.user.lift(AMBAProt).foreach { x =>
+    val user_bit_cacheable = edge_out.manager.supportsAcquireTFast(refill_paddr, lgCacheBlockBytes.U)
+
+    x.privileged  := io.privileged || user_bit_cacheable    // privileged if machine mode or memory port


I am confused. Since we cache everything on fetch, why is privileged not just tied to true?

DecodeTheEncoded · 2023-05-17T01:36:40Z

@jsmithsf Hi, Thanks for your work. I am rookie to rocketchip, and want to understand why AMBAProtField is needed when virtual memory is disabled.

  val masterNode = TLClientNode(Seq(TLMasterPortParameters.v1(
    clients = Seq(TLMasterParameters.v1(
      sourceId = IdRange(0, 1 + icacheParams.prefetch.toInt), // 0=refill, 1=hint
      name = s"Core ${staticIdForMetadataUseOnly} ICache")),
    requestFields = useVM.option(Seq()).getOrElse(Seq(AMBAProtField())))))

I am working on a chipyard configuration(rv32imac without os support), and when I port the deign to vcu118 fpga prototyping, the firrtl will complain some amba signal is not initialized.
I do some debugging, and found the problem is icache masterNode will have amba related userbits if useVM is false, through diplomacy negotiation, these requestFields will go downsteam and reach an end of fpga signal while there is no corresponding amba signal. So, I want to know why you add this in the first place. Thanks

jsmithsf requested a review from aswaterman March 30, 2020 11:12

Drive protection user bits on TL

d90652a

jsmithsf force-pushed the amba_prot branch from f120ad5 to d90652a Compare March 30, 2020 21:45

Add new dprv bits to RoccExample

6962cac

jsmithsf requested review from hcook and terpstra March 31, 2020 17:27

mwachs5 reviewed Mar 31, 2020

View reviewed changes

mwachs5 requested a review from daveparry March 31, 2020 17:58

terpstra approved these changes Mar 31, 2020

View reviewed changes

jsmithsf merged commit 468befd into master Mar 31, 2020

michael-etzkorn deleted the amba_prot branch May 14, 2022 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drive protection user bits on TL #2383

Drive protection user bits on TL #2383

jsmithsf commented Mar 30, 2020 •

edited

Loading

jsmithsf commented Mar 31, 2020

mwachs5 Mar 31, 2020

jsmithsf Mar 31, 2020

mwachs5 Mar 31, 2020

jsmithsf Mar 31, 2020

solomatnikov Mar 31, 2020

jsmithsf Mar 31, 2020

terpstra Mar 31, 2020

jsmithsf Mar 31, 2020

mwachs5 commented Mar 31, 2020

terpstra Mar 31, 2020

terpstra Mar 31, 2020

DecodeTheEncoded commented May 17, 2023 •

edited

Loading

Drive protection user bits on TL #2383

Drive protection user bits on TL #2383

Conversation

jsmithsf commented Mar 30, 2020 • edited Loading

jsmithsf commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwachs5 commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DecodeTheEncoded commented May 17, 2023 • edited Loading

jsmithsf commented Mar 30, 2020 •

edited

Loading

DecodeTheEncoded commented May 17, 2023 •

edited

Loading