Skip to content

target/riscv: fix riscv_mmu behaviour #1256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from
Open

target/riscv: fix riscv_mmu behaviour #1256

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions src/target/riscv/riscv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3017,9 +3017,6 @@ static int riscv_mmu(struct target *target, int *enabled)
{
*enabled = 0;

if (!riscv_virt2phys_mode_is_sw(target))
return ERROR_OK;

Comment on lines -3020 to -3022
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@lz-bro lz-bro May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@fk-sc fk-sc May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@lz-bro lz-bro May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU, this is exactly the purpose of #1241

You are right, yeah

/* Don't use MMU in explicit or effective M (machine) mode */
riscv_reg_t priv;
if (riscv_reg_get(target, &priv, GDB_REGNO_PRIV) != ERROR_OK) {
Expand Down Expand Up @@ -3425,16 +3422,11 @@ static int riscv_rw_memory(struct target *target, const riscv_mem_access_args_t
return ERROR_OK;
}

int mmu_enabled;
int result = riscv_mmu(target, &mmu_enabled);
if (result != ERROR_OK)
return result;

RISCV_INFO(r);
if (!mmu_enabled)
if (riscv_virt2phys_mode_is_hw(target))
return r->access_memory(target, args);
Comment on lines +3426 to 3427
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
*/
...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


result = check_virt_memory_access(target, args.address,
int result = check_virt_memory_access(target, args.address,
args.size, args.count, is_write);
if (result != ERROR_OK)
return result;
Expand Down
Loading