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;

/* 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);

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