chipsalliance / riscv-dv

Random instruction generator for RISC-V processor verification
Apache License 2.0
1k stars 322 forks source link

Add PMP support #459

Closed taoliug closed 4 years ago

taoliug commented 4 years ago

File this issue to track the PMP verification support. @udinator, please add your implementation plan to this bug.

udinator commented 4 years ago

First main step of implementation will be to create a new riscv_pmp_cfg configuration object that will be responsible for handling all pmp configuration randomization as well as any user-defined configurations for specific pmp regions/fields. This configuration object will then be provided to riscv_asm_program_gen, which will then generate appropriate configuration instructions for the pmp_cfg and pmp_addr CSRs.
Some initial ideas for the user-facing configuration options are as follows (both assume that 4 pmp regions will be enabled but is easily extensible):

# Option 1
gen_opts: >
  +pmp_num_regions=4
  +pmp_granularity=0
  +pmp_randomize=0
  +pmp_region_0=00101
  +pmp_addr_0=0x......
  +pmp_region_1=02111
  +pmp_addr_1=0x......
  +pmp_region_2=01111
  +pmp_addr_2=0x......
  +pmp_region_3=11001
  +pmp_addr_3=0x......

# Option 2
gen_opts: >
  +pmp_num_regions=4
  +pmp_granularity=0
  +pmp_randomize=0
  +pmp_region_0=0,OFF,1,0,1,0x.......
  +pmp_region_1=0,NA4,1,1,1,0x.......
  +pmp_region_2=0,TOR,1,1,1,0x.......
  +pmp_region_3=1,TOR,0,0,1,0x.......

# Option 3
gen_opts: >
  +pmp_num_regions=4
  +pmp_granularity=0
  +pmp_randomize=0
  +pmp_region_0=L:0,A:OFF,X:1,W:0,R:1,ADDR=0x........
  +pmp_region_1=L:0,A:NA4,X:1,W:1,R:1,ADDR=0x........
  +pmp_region_2=L:0,A:TOR,X:1,W:1,R:1,ADDR=0x........
  +pmp_region_3=L:1,A:TOR,X:0,W:0,R:1,ADDR=0x........

In option 1, the PMP L field is the leftmost digit of the pmp_region value, the A field is the next digit, and so on from left to right. A second corresponding variable represents the address corresponding to that region as well. This makes parsing from the SV side much more straightforward, but is more confusing to configure and debug from the user's perspective, and could result in a very long yaml configuration if specifications for all 16 regions are necessary.

Option 2 is a much more human-readable format, where each field is easily visible and easy to modify, but parsing from the SV side will be more complex, since it's necessary to split these strings by the comma, or whatever delimiter is used to separate the field values.

Option 3 is very similar to Option 2, but allows much more fine-grained configuration by the user, tagging each value by the PMP field name makes it not necessary to specify all 6 PMP values for a PMP region. For example, if +pmp_region_0=W:0 is specified as a region configuration, the remaining 4 L/X/W/R fields will either be randomized or set to defaults as specified by the value of pmp_randomize.

I am leaning more towards option 3, because is easily readable, and easily configurable by the user.

It is also not necessary to specify all configurations for all active pmp regions, if it is desired to have 4 PMP regions but only configure 1 region, the configuration for the remaining unspecified regions will be either randomized or set to some defaults, depending on the value of pmp_randomize.

taoliug commented 4 years ago

option 3 LGTM, thanks

vandanaprabhu commented 4 years ago

Option 3 looks good. One thing to consider for you, if possible , is adding a conversion utility function from a physical address to PMPaddr value, based on

  1. size of the physical address bits as more likely than not fewer rather than max allowable physical address bits will be implemented.
  2. Granularity G as defined in the spec
  3. Mode (OFF, Permissible NAPOTs, TOR)

Thank you!

udinator commented 4 years ago

Hi @vandanaprabhu , thanks for the input! Yes, some functionality like this would be necessary to get the correctly formatted address to write to pmp_addr CSRs, and I plan to provide a function similar to what you describe.

agrobman commented 4 years ago

How will the test generator use this configuration object besides programming the PMP CSRs? How will the test code/data sections created in the test match/mismatch PMP CSRs programming? To test the feature CPU should be able access protected regions with different access types - data/instructions/read/write/user/machine mode .

What is the plan for this? So far I see that basically there is one .text section and multiple data sections.

The test should be able to reach the test end after all protection related exceptions and to check that the taken exceptions are legitimate.

What will be pass/fail criteria for these tests?

What is preliminary completion date for the PMP feature implementation? We may need to test PMP pretty soon. Can we rely on riscv-dv for this?

agrobman commented 4 years ago

As the tool user, I prefer the test generator creates random PMP configurations with some constraints, set in the processor configuration file, and random test rather than providing a big number of plus arguments - one can write directed tests without riscv-dv support ...

One more moment - the PMP testing should be available in bare setup too, but without address translations (MMU tables).

udinator commented 4 years ago

Hi @agrobman , the configuration object is primarily to program the PMP CSRs.

Also, as far as separating the code/data sections with different PMP csr programming, there is currently no way of being able to do this, as since generated program is of random size, it is hard to see where the instruction section ends/data section begins, etc... however I agree that I feel this would be a good feature to add, and this is a todo item to add on.

Regarding the question about pass/fail criteria for these tests, the ISS simulators are also able to exactly model PMP logic as well, so the log comparison between RTL log and ISS log is a good metric to determine passing or failure, as all PMP exceptions in RTL will be reflected in the ISS.

It is actually not necessary to have to completely specify all desired PMP regions from the command line, this is just some user-level configurability that can be used if desired; for example, to specify configuration for a single PMP region, etc... There is also a +pmp_randomize option that, when set, will fully randomize every specified PMP region with some constraints as per the spec. Did you have any other constraints in mind that you would like to be added?

Finally, the goal is to also have PMP configurations available in bare program setup as well, but this has not been addressed yet.

agrobman commented 4 years ago

@udinator , We have a CPU with PMP, we would like to verify, but it doesn't have MMU. ISS are written by people - we found that about 1/4 bugs are in the ISS we are maintaining. So ISSs need tools to verify them too.

Regardless, when should we expect the PMP tests generation to be done?

What is about MMU tests ( address translation and protection together, MMU translation tables inside the test code/data) ?

udinator commented 4 years ago

Hi @agrobman , re your point about the ISS, I have currently been using the Spike and OVPsim ISS simulators with the new PMP settings, and the results seem fairly consistent across both of these, so for the time being, this is what I'll stick to these for the verification flow.

The PMP test generation itself is fairly mature at this point, as you noted before it essentially randomizes the various pmpcfg and pmpaddr CSRs based on any constraints or overrides user can define from the YAML test target, feel free to try it out and let me know any feedback you may have. I am currently working on a rather large change that deals with the implementation of the exception handlers for PMP faults, but since you mentioned in another issue that you and your team will insert your own trap handler implementations, these shouldn't apply to your use-case.

You're right, MMU integration is important, I'll be making sure generation with both page tables and PMP protection is functioning correctly as well.

agrobman commented 4 years ago

What is the PMP test flow, besides the PMP registers setup? What are the basic constraints for the registers setup? How are these constraints related to the test code layout and data sections addresses/sizes? How do I define the number of the PMP registers?

What are the plus args to generate a PMP test?

Also I didn’t understand your question about ISS ..

Alex

udinator commented 4 years ago

The PMP registers setup is most of the PMP test flow. The existing basic constraints are mostly dealing with constraints given by the privileged spec, and don't relate to the test code layout except for <main>, which is loaded into the pmpaddr0 CSR, since it has been pretty difficult to allow relative addressing from the generator's perspective when randomly creating addresses for pmpaddr CSRs. You can define the number of PMP registers with the +pmp_num_regions=... plusarg. All of the existing plusargs for PMP generation can be found in the new() constructor of riscv_pmp_cfg.sv - currently there are 5 plusargs: +pmp_randomize, +pmp_allow_addr_overlap, +pmp_granularity, +pmp_num_regions, +pmp_max_offset; and you can additionally configure each implemented pmp region as specified near the top of this thread.

A TODO item on my end is to add a small document about PMP generation to make this more clear.

udinator commented 4 years ago

Regarding ISS, my point was that since both Spike/OVPsim implement PMP logic, I've been successfully using these for log comparison with PMP generation tests.

agrobman commented 4 years ago

So what is the purpose of this test? Just to check that the registers are writable?

udinator commented 4 years ago

It will actually configure the pmpcfg and pmpaddr CSRs to enable memory protection in the CPU. Functional checking of this will have to be done from the RTL simulation side of things, with the ISS/RTL log comparisons.

agrobman commented 4 years ago

If the registers are programmed to be outside address range of the test code – nothing won’t run in user mode Or if the permissions are set to enable accesses for used address ranges there won’t be any effects of protection seen ..

agrobman commented 4 years ago

Hi

Why is PMP guarded by BARE_MODE?

We don't have MMU, why we can't program PMP?

udinator commented 4 years ago

If the registers are programmed to be outside address range of the test code – nothing won’t run in user mode Or if the permissions are set to enable accesses for used address ranges there won’t be any effects of protection seen ..

Yes this is true. By default the PMP settings are set to enable all accesses to the 0x0-0xFFFFFFFF memory region, but you can configure these manually from the command line if a full randomization is not entirely desirable, or you can override some configuration CSRs and randomize the rest, etc... Something that I want to investigate further down the road is what you mentioned earlier, about using labels for the text, data sections, etc... to load into pmpaddr CSRs to easily split the program into protected regions.

Can you elaborate what you mean by PMP being guarded by BARE_MODE? I check that bare program mode is disabled before I generate the PMP-specific trap handlers in the beginning of the program, you should still be able to program the PMP CSRs if bare mode is enabled. An MMU shouldn't be a requirement to program PMP, this feature should be independent of page tables and so on.

agrobman commented 4 years ago

one example of guarding: ( if the SATP_MODE = BARE PMP code is not generated)

if (!cfg.bare_program_mode) begin setup_misa(); // Create all page tables create_page_table(hart); // Setup privileged mode registers and enter target privileged mode pre_enter_privileged_mode(hart); end // Init section BTW, what's setup_misa? misa is read only register, why do you write it? why do you "init_machine_mode" ? The CPU should start in machine mode ..

Our expectations about PMP testing were : programming PMP registers constraint randomly ( somewhat based on generated test sections/code ) then the code should try to get access violations: fetch side - to get to the non-executable address 1) by a branch/change of flow; 2) sequentially cross the region; Data: read and write regions with all possible permissions

Randomization of the PMP regs supposed to check all kinds of regions overlaps.

there should be exception handlers/test code hooks to finish the test gracefully..

Think how to achieve DUT 100% code/toggle/functional coverage with your tests .. BTW, we have 16 PMP registers in the CPU.

agrobman commented 4 years ago

we are using random test generator in 24/7 runs. We are run 100s simulations simultaneously. We can't setup test parameters manually, everything is collected automatically - failures, coverage etc.

udinator commented 4 years ago

Hmm, I'm able to generate the PMP code with satp_mode=BARE on my end. I'll look into what's going on when generating with bare_program_mode=1, this might be the issue you are referring to.

The setup_misa function is there to manually set up the misa CSR with the supported XLEN and any enabled extensions. misa is a read-write register, so writing to it is correct.

Yes you're correct that the CPU will automatically start in machine mode, the init_machine_mode section is generated because the random privilege mode that the random instructions should execute in was set to MACHINE_MODE, it just sets the intended privilege mode to mret into inside of mstatus. If we wanted to execute in User-mode, this section would be named init_user_mode and would write the U-mode value into mstatus.

Currently I am only looking at the generated main section to start setting up the pmp CSRs, something that I want to investigate further down the road is what you mentioned about using labels for the text, data sections, etc... to load into pmpaddr CSRs. The randomization of the PMP CSRs themselves should provide enough access violations from both the imem and dmem side, depending on the randomization, but enough iterations of the test generation should cover a lot of these scenarios. Yes, the randomization supports creating region overlaps as well. I have a PR out for a custom PMP exception handler that will allow forward progress to be made instead of just spinning forever on the failing i-fetch/store/load. We also support a parameterizable number of PMP regions as well. With regards to your comment about manually setting up test parameters, all PMP test parameters are defined through the YAML test entry format.

agrobman commented 4 years ago

Do you have a overview of the generated test structure, cpu config and plusargs reference, describing how these parameters effect generated test? such reference manual should help greatly to understand the tool capabilities.

regarding PMP parameters : 1) We don't use python script/yaml descriptions - we have no latest python installed on our servers. we had to write our own perl script to run riscv-dv. Besides running generator it also patches generated code to fit our simulation environment and DUT. 2) We need these PMP parameters be randomized per test; 3) based on the generated code/data sizes most likely or nothing will get permissions or everything will get access violation . probability of toggling permissions in the test is near the zero if the randomization of the PMP regs does not take in account the test layout.

udinator commented 4 years ago

Currently there isn't a document of the generated test structure, but the available set of plusargs should be available in one of the documentations.

Yes, currently the PMP parameters will be randomized per test (unless specified in test configuration); a TODO item will be to add support for taking into account the various test sections when mapping to the PMP registers.

taoliug commented 4 years ago

Udi, can you close this bug, and start a new bug for the feature request from agrobman@ ?