Skip to content

target/riscv: add is_virtual parameter to memory access method #1241

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 2 commits into
base: riscv
Choose a base branch
from
Open

target/riscv: add is_virtual parameter to memory access method #1241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31175e5
target/riscv: fix riscv_mmu behaviour
fk-sc May 6, 2025
b078514
target/riscv: add is_virtual parameter to memory access method
fk-sc Mar 13, 2025
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
6 changes: 3 additions & 3 deletions doc/openocd.texi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11522,9 +11522,9 @@ dump_sample_buf}.
@end deffn

@deffn {Command} {riscv repeat_read} count address [size=4]
Quickly read count words of the given size from address. This can be useful
to read out a buffer that's memory-mapped to be accessed through a single
address, or to sample a changing value in a memory-mapped device.
Quickly read count words of the given size from physical address. This can
be useful to read out a buffer that's memory-mapped to be accessed through
a single address, or to sample a changing value in a memory-mapped device.
@end deffn

@deffn {Command} {riscv info}
Expand Down
3 changes: 2 additions & 1 deletion src/target/riscv/riscv-011.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2339,7 +2339,8 @@ static int write_memory(struct target *target, const riscv_mem_access_args_t arg
return ERROR_FAIL;
}

static int access_memory(struct target *target, const riscv_mem_access_args_t args)
static int access_memory(struct target *target,
const riscv_mem_access_args_t args, const bool is_virtual)
{
assert(riscv_mem_access_is_valid(args));
const bool is_write = riscv_mem_access_is_write(args);
Expand Down
16 changes: 11 additions & 5 deletions src/target/riscv/riscv-013.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ static int register_read_direct(struct target *target, riscv_reg_t *value,
enum gdb_regno number);
static int register_write_direct(struct target *target, enum gdb_regno number,
riscv_reg_t value);
static int riscv013_access_memory(struct target *target, const riscv_mem_access_args_t args);
static int riscv013_access_memory(struct target *target,
const riscv_mem_access_args_t args, const bool is_virtual);
static bool riscv013_get_impebreak(const struct target *target);
static unsigned int riscv013_get_progbufsize(const struct target *target);

Expand Down Expand Up @@ -1277,7 +1278,8 @@ static int scratch_read64(struct target *target, scratch_mem_t *scratch,
.count = 2,
.increment = 4,
};
if (riscv013_access_memory(target, args) != ERROR_OK)
if (riscv013_access_memory(target, args,
/* is_virtual */ false) != ERROR_OK)
return ERROR_FAIL;
*value = buf_get_u64(buffer,
/* first = */ 0, /* bit_num = */ 64);
Expand Down Expand Up @@ -1319,7 +1321,8 @@ static int scratch_write64(struct target *target, scratch_mem_t *scratch,
.count = 2,
.increment = 4,
};
if (riscv013_access_memory(target, args) != ERROR_OK)
if (riscv013_access_memory(target, args,
/* is_virtual */ false) != ERROR_OK)
return ERROR_FAIL;
}
break;
Expand Down Expand Up @@ -4551,8 +4554,8 @@ access_memory_abstract(struct target *target, const riscv_mem_access_args_t args
write_memory_abstract(target, args);
}

static int
riscv013_access_memory(struct target *target, const riscv_mem_access_args_t args)
static int riscv013_access_memory(struct target *target,
const riscv_mem_access_args_t args, const bool is_virtual)
{
assert(riscv_mem_access_is_valid(args));

Expand Down Expand Up @@ -4580,12 +4583,15 @@ riscv013_access_memory(struct target *target, const riscv_mem_access_args_t args
riscv_mem_access_method_t method = r->mem_access_methods[i];
switch (method) {
case RISCV_MEM_ACCESS_PROGBUF:
// TODO: pass is_virtual here in future commits
skip_reason[method] = access_memory_progbuf(target, args);
break;
case RISCV_MEM_ACCESS_SYSBUS:
// TODO: pass is_virtual here in future commits
skip_reason[method] = access_memory_sysbus(target, args);
break;
case RISCV_MEM_ACCESS_ABSTRACT:
// TODO: pass is_virtual here in future commits
skip_reason[method] = access_memory_abstract(target, args);
break;
default:
Expand Down
57 changes: 34 additions & 23 deletions src/target/riscv/riscv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,13 @@ bool riscv_virt2phys_mode_is_sw(const struct target *target)
return r->virt2phys_mode == RISCV_VIRT2PHYS_MODE_SW;
}

bool riscv_virt2phys_mode_is_off(const struct target *target)
{
assert(target);
RISCV_INFO(r);
return r->virt2phys_mode == RISCV_VIRT2PHYS_MODE_OFF;
}

const char *riscv_virt2phys_mode_to_str(riscv_virt2phys_mode_t mode)
{
assert(mode == RISCV_VIRT2PHYS_MODE_OFF
Expand Down Expand Up @@ -3017,9 +3024,6 @@ static int riscv_mmu(struct target *target, int *enabled)
{
*enabled = 0;

if (!riscv_virt2phys_mode_is_sw(target))
return ERROR_OK;

/* 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 @@ -3152,7 +3156,7 @@ static int riscv_address_translate(struct target *target,
.increment = 4,
.count = (1 << info->pte_shift) / 4,
};
int retval = r->access_memory(target, args);
int retval = r->access_memory(target, args, /* is_virtual */ false);
if (retval != ERROR_OK)
return ERROR_FAIL;

Expand Down Expand Up @@ -3385,6 +3389,14 @@ static int check_virt_memory_access(struct target *target, target_addr_t address
return ERROR_OK;
}

static int riscv_access_phys_memory(struct target *target,
const riscv_mem_access_args_t args)
{
RISCV_INFO(r);
return r->access_memory(target, args, /* is_virtual */ false);
}


static int riscv_read_phys_memory(struct target *target, target_addr_t phys_address,
uint32_t size, uint32_t count, uint8_t *buffer)
{
Expand All @@ -3395,8 +3407,7 @@ static int riscv_read_phys_memory(struct target *target, target_addr_t phys_addr
.count = count,
.increment = size,
};
RISCV_INFO(r);
return r->access_memory(target, args);
return riscv_access_phys_memory(target, args);
}

static int riscv_write_phys_memory(struct target *target, target_addr_t phys_address,
Expand All @@ -3409,12 +3420,11 @@ static int riscv_write_phys_memory(struct target *target, target_addr_t phys_add
.count = count,
.increment = size,
};

RISCV_INFO(r);
return r->access_memory(target, args);
return riscv_access_phys_memory(target, args);
}

static int riscv_rw_memory(struct target *target, const riscv_mem_access_args_t args)
static int riscv_access_virt_memory(struct target *target,
const riscv_mem_access_args_t args)
{
assert(riscv_mem_access_is_valid(args));

Expand All @@ -3425,16 +3435,14 @@ 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)
return r->access_memory(target, args);
if (riscv_virt2phys_mode_is_off(target))
return r->access_memory(target, args, /* is_virtual */ false);

if (riscv_virt2phys_mode_is_hw(target))
return r->access_memory(target, args, /* is_virtual */ true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please double-check this line.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

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 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)

Copy link
Collaborator

@JanMatCodasip JanMatCodasip Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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;


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 Expand Up @@ -3464,7 +3472,8 @@ static int riscv_rw_memory(struct target *target, const riscv_mem_access_args_t
else
current_access.read_buffer += current_count * args.size;

result = r->access_memory(target, current_access);
result = r->access_memory(target,
current_access, /* is_virtual */ false);
if (result != ERROR_OK)
return result;

Expand All @@ -3485,7 +3494,7 @@ static int riscv_read_memory(struct target *target, target_addr_t address,
.increment = size,
};

return riscv_rw_memory(target, args);
return riscv_access_virt_memory(target, args);
}

static int riscv_write_memory(struct target *target, target_addr_t address,
Expand All @@ -3499,7 +3508,7 @@ static int riscv_write_memory(struct target *target, target_addr_t address,
.increment = size,
};

return riscv_rw_memory(target, args);
return riscv_access_virt_memory(target, args);
}

static const char *riscv_get_gdb_arch(const struct target *target)
Expand Down Expand Up @@ -5232,7 +5241,9 @@ COMMAND_HANDLER(handle_repeat_read)
.count = count,
.increment = 0,
};
int result = r->access_memory(target, args);
/* TODO: Add a command parameter that enables
* choosing between virtual and physical access */
int result = r->access_memory(target, args, /* is_virtual */ false);
if (result == ERROR_OK) {
target_handle_md_output(cmd, target, address, size, count, buffer,
false);
Expand Down Expand Up @@ -5615,7 +5626,7 @@ static const struct command_registration riscv_exec_command_handlers[] = {
.handler = handle_repeat_read,
.mode = COMMAND_ANY,
.usage = "count address [size=4]",
.help = "Repeatedly read the value at address."
.help = "Repeatedly read the value at physical address."
},
{
.name = "set_command_timeout_sec",
Expand Down
4 changes: 3 additions & 1 deletion src/target/riscv/riscv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ struct riscv_info {
riscv_sample_config_t *config,
int64_t until_ms);

int (*access_memory)(struct target *target, const riscv_mem_access_args_t args);
int (*access_memory)(struct target *target,
const riscv_mem_access_args_t args, const bool is_virtual);

unsigned int (*data_bits)(struct target *target);

Expand Down Expand Up @@ -411,6 +412,7 @@ typedef struct {

bool riscv_virt2phys_mode_is_hw(const struct target *target);
bool riscv_virt2phys_mode_is_sw(const struct target *target);
bool riscv_virt2phys_mode_is_off(const struct target *target);

/* Wall-clock timeout for a command/access. Settable via RISC-V Target commands.*/
int riscv_get_command_timeout_sec(void);
Expand Down
Loading