target/riscv: add is_virtual parameter to memory access method

[fkhaidari]

Added is_virtual parameter to memory access method. This change is required to support hw translation appropriately.

Updated repeat_read documentation according to new behaviour. It is provide physical access now

[fkhaidari]

@JanMatCodasip, @MarekVCodasip could you please take a look?

[fkhaidari]

In this comment, @JanMatCodasip suggested moving the is_virtual field out of the riscv_mem_access_args_t structure. However, while implementing is_virtual across progbuf, sysbus, and abstract memory access types, I’ve noticed the flag is becoming pervasive and required multiple places. Given this, should we reconsider keeping it in riscv_mem_access_args_t to avoid redundancy and centralize its handling?

[JanMatCodasip]

fk-sc: In this comment, JanMatCodasip suggested moving the is_virtual field out of the riscv_mem_access_args_t structure. [...]

I do not feel strongly about this one. Please feel free to use and submit whatever variant in your opinion leads to cleaner, more understandable code.

My concern was that is_virtual would be passed (as part of the structure) to functions for which it is not relevant. If those functions are in minority, maybe it is an acceptable compromise.

[fkhaidari]

@JanMatCodasip, I’ve decided to keep is_virtual as a separate parameter for now. I can revisit integrating it into the riscv_mem_access_args_t structure once the progbuf, sysbus, and abstract memory subsystems are finalized

[JanMatCodasip]

Could you please double-check this line.

If mmu_enabled is false, shouldn't is_virtual be false as well?

[fkhaidari]

Addressed

[aap-sc]

@fk-sc can we discuss this design once again?

1. I don't like the suggested approach because riscv_rw_memory should always operate on virtual addresses by design. The decision on how the actual address translation is performed should be decided by lower level functions - that is I would expect for r->access_memory() to be called with is_virutual = true. The fact that in some execution context virtual address is equal to physical is irrelevant at this level of abstraction.

2. if there is a necessity to provide additional layer to simplify page-crossing translations - we can do it here and establish a contract that r->access_memory should access non-page crossing memory regions.

3. please rename mmu_enabled to satp_translation_required. mmu_enabled is too broad term that can confuse the reader (it definitely confuses me)

@JanMatCodasip sorry for the mess, most likely we @fk-sc , @en-sc and me should have yet another round of internal discussion related to the above bulllets. Please consider this MR as a draft for now. You inputs are as always are very welcome, so please do share concerns/questions/suggestions regarding the matter (if you have any)

[JanMatCodasip]

No problem at all. Thank you for letting me know.

aap-sc: [...] riscv_rw_memory should always operate on virtual addresses by design.

Sounds good to me.

In that case, if you like, you can also make the code more clear by doing this:

• rename riscv_rw_memory() to riscv_rw_virt_memory()
• create riscv_rw_phys_memory() (a thin wrapper over r->access_memory(args, /* is_virtual = */ false))

aap-sc: if there is a necessity to provide additional layer to simplify page-crossing translations - we can do it here and establish a contract that r->access_memory should access non-page crossing memory regions.

• The algorithm for breaking the memory access into the page-sized chunks, which is already in riscv_rw_memory(), looks all right to me. Or do you see any gap or need for change there, which I might have missed?
• Do I understand correctly that the only case when we need to manually break the memory transfer to page-sized pieces is when virt2phys_mode is set to SW?

aap-sc: please rename mmu_enabled to satp_translation_required. mmu_enabled is too broad term that can confuse the reader

I agree that mmu_enabled is not a clear name. It could also be called manual_satp_translation_needed. The word "manual" is IMO important here as it means that OpenOCD must do the translation work (as opposed to the hardware).

Furthermore, I am concerned about the riscv_mmu() itself. If virt2phys mode is set to HW, then riscv_mmu() returns False, which looks incorrect. As a result:

• The riscv_virt2phys() function (and the TCL command virt2phys) will return 1:1 mapping instead of correctly translated addresses.
• The target_alloc_working_area_try() will allocate the work area in the physical address range (-work-area-phys) instead of virtual (-work-area-virt).

Should the initial check in riscv_mmu() be fixed this way?

	if (riscv_virt2phys_mode_is_off(target))
		return ERROR_OK;

[fkhaidari]

@JanMatCodasip, @aap-sc, @en-sc, I've revised the implementation of this PR. At first we need to merge #1256 (this commit fixes riscv_mmu). This PR includes mentioned one. Changes in updated version of PR:

• Renamed riscv_rw_memory() to riscv_access_virt_memory()
• Created riscv_access_phys_memory() (a thin wrapper over r->access_memory(args, /* is_virtual = */ false))

[JanMatCodasip]

@fk-sc If you don't mind, I will get back to this review at a later point - after we finish the discussion under #1256.