target/riscv: fix riscv_mmu behaviour

[fkhaidari]

Fixed riscv_mmu behaviour: buggy check was removed.
As a result virt2phys command behaviour was fixed: now it
returns translated address even while virt2phys_mode is off.

Change-Id: Ie2e6d1057024ab794038d5ed3662ef49a4d71e70

[en-sc]

LGTM (reviewed internally).

[fkhaidari]

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

[JanMatCodasip]

LGTM. Thank you!

[lz-bro]

Access memory would fail while virt2phys_mode is hw.

Suggested change

	if (!riscv_virt2phys_mode_is_sw(target))
	return ERROR_OK;
	if (riscv_virt2phys_mode_is_hw(target))
	return ERROR_OK;

[fkhaidari]

With this check virt2phys_mode would return incorrect value: virtual address instead of physical

[lz-bro]

Access memory would fail without this check virt2phys_mode while virt2phys_mode is hw. It is correct to return a virtual address while virt2phys_mode is hw. Address translation should be done by hardware when set dcsr.mprven, I have tested it. When virt2phys_mode is hw, the virt2phys command returns the physical address, at least the current modification is incorrect, and both the virt2phys command and memory access will report errors.

[fkhaidari]

Sorry, I've mistaken. I meant that virt2phys would return incorrect value

[fkhaidari]

I think I understand you now. You are right, here we have double translation. But I think it has to be fixed in riscv_rw_memory function. Something like:

if (riscv_virt2phys_mode_is_hw(target))
	return r->access_memory(target, args);

before translation.

Sorry for the mess. This change was a part of #1241. Looks like I have extracted this part incorrectly

[lz-bro]

When virt2phys_mode is hw, the virt2phys command still report errors. Translate page table accesses failed due to setting dcsr.mprven. I think maybe it could be fixed in riscv_virt2phys function like this:

        RISCV_INFO(r);
        int mode = r->virt2phys_mode;
        if (riscv_virt2phys_mode_is_hw(target))
                r->virt2phys_mode = RISCV_VIRT2PHYS_MODE_SW;

        int result = riscv_address_translate(target,
                        satp_info, get_field(satp_value, RISCV_SATP_PPN(xlen)),
                        NULL, 0,
                        virtual, physical);

        r->virt2phys_mode = mode;
        return result;

[JanMatCodasip]

IIUC, riscv_address_translate() function should always perform physical accesses - it should work exclusively with physical addresses. @lz-bro, is this the issue that you are trying to point out?

If that's correct understanding, then we should probably create function r->access_memory_phys that will always perform physical accesses, regardless of r->virt2phys_mode. It looks to me as a cleaner solution than temporarily changing the virt2phys_mode flag.

[lz-bro]

@JanMatCodasip Yes, that's exactly what I meant.

[en-sc]

@lz-bro, great catch!

If that's correct understanding, then we should probably create function r->access_memory_phys that will always perform physical accesses, regardless of r->virt2phys_mode. It looks to me as a cleaner solution than temporarily changing the virt2phys_mode flag.

@JanMatCodasip, this will not be enough. In case of 2-stage translation, the first PTE is addressed using a virtual address. I'd suggest bailing out with a message that this is not supported for now (AFAIU, mstatus.MPP will need to be modified/restored for proper support).

... create function r->access_memory_phys that will always perform physical accesses.

AFAIU, this is exactly the purpose of #1241.

[fkhaidari]

AFAIU, this is exactly the purpose of #1241

You are right, yeah

[JanMatCodasip]

Do I understand correctly that when virt2phys is off, we should also directly call r->access_memory?

Suggested change

	if (riscv_virt2phys_mode_is_hw(target))
	return r->access_memory(target, args);
	if (riscv_virt2phys_mode_is_off(target))
	/* No address translation is performed. Virtual == physical is assumed. */
	return r->access_memory(target, args);
	if (riscv_virt2phys_mode_is_hw(target))
	/* Address translation virtual -> physical will be performed by the hardware. */
	return r->access_memory(target, args);
	/* Otherwise address translation is performed by OpenOCD - for each 4 KiB page separately.
	* The memory operation must be split to page-sized chunks.
	*/
	...

[JanMatCodasip]

@fk-sc It looks like you have independently came to the same conclusion in this comment.

[JanMatCodasip]

IIUC, riscv_address_translate() function should always perform physical accesses - it should work exclusively with physical addresses. @lz-bro, is this the issue that you are trying to point out?

If that's correct understanding, then we should probably create function r->access_memory_phys that will always perform physical accesses, regardless of r->virt2phys_mode. It looks to me as a cleaner solution than temporarily changing the virt2phys_mode flag.

[en-sc]

@JanMatCodasip, @lz-bro, I'd suggest merging this one as-is. It makes the virt2phys work in some cases, so it's a step in the right direction. To finish this we will need #1241, and a follow-up that will actually consider is_virtual parameter when accessing the memory, however IMHO it's easier to review them one-by-one.

[fkhaidari]

@JanMatCodasip, @lz-bro, can we continue merging? Maybe with some extra TODO's

[fkhaidari]

Ping

[fkhaidari]

@JanMatCodasip, @lz-bro, ping

[fkhaidari]

@JanMatCodasip, @lz-bro, can we continue merging? Maybe with some extra TODO's