Skip to content
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.

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

draft: hexagon system emulation initial #99

Open
wants to merge 109 commits into
base: tip
Choose a base branch
from
Open

draft: hexagon system emulation initial #99

wants to merge 109 commits into from

Conversation

androm3da
Copy link

No description provided.

androm3da and others added 30 commits January 2, 2025 19:31
The BADVA reg is referred to with the wrong identifier.  The
CAUSE reg field of SSR is not yet modeled, we will dump
the SSR in a subsequent commit.

Signed-off-by: Brian Cain <[email protected]>
These registers are defined in the Qualcomm Hexagon V71 Programmer's
Reference Manual -
https://docs.qualcomm.com/bundle/publicresource/80-N2040-51_REV_AB_Hexagon_V71_ProgrammerS_Reference_Manual.pdf
Refer to §11.9.1 SYSTEM GUEST, §11.9.2 SYSTEM MONITOR.

Signed-off-by: Brian Cain <[email protected]>
FIXME: this commit contains FIXMEs

Signed-off-by: Brian Cain <[email protected]>
This commit provides handlers to generate TCG for guest and system
register reads and writes.  They will be leveraged by a future commit.

Signed-off-by: Brian Cain <[email protected]>
FIXME: this commit contains FIXMEs

Signed-off-by: Brian Cain <[email protected]>
Define the register fields for ssr, schedcfg, stid, bestwait, ccr,
modectl, imask, ipendad.

Define the fields for TLB entries.

Signed-off-by: Brian Cain <[email protected]>
FIXME: Can we avoid this UNUSED?  Or if not, can we explain why
we generate these?

Error that we see without this change:

    In file included from ../target/hexagon/translate.c:40:
    target/hexagon/analyze_funcs_generated.c.inc: In function ‘analyze_Y2_tfrscrr’:
    target/hexagon/analyze_funcs_generated.c.inc:16748:15: error: unused variable ‘SsN’ [-Werror=unused-variable]
    16748 |     const int SsN = insn->regno[1];
          |               ^~~
    target/hexagon/analyze_funcs_generated.c.inc: In function ‘analyze_Y4_tfrscpp’:
    target/hexagon/analyze_funcs_generated.c.inc:16771:15: error: unused variable ‘SssN’ [-Werror=unused-variable]
    16771 |     const int SssN = insn->regno[1];
          |               ^~~~
    target/hexagon/analyze_funcs_generated.c.inc: In function ‘analyze_G4_tfrgcrr’:
    target/hexagon/analyze_funcs_generated.c.inc:16794:15: error: unused variable ‘GsN’ [-Werror=unused-variable]
    16794 |     const int GsN = insn->regno[1];
          |               ^~~
    target/hexagon/analyze_funcs_generated.c.inc: In function ‘analyze_G4_tfrgcpp’:
    target/hexagon/analyze_funcs_generated.c.inc:16817:15: error: unused variable ‘GssN’ [-Werror=unused-variable]
    16817 |     const int GssN = insn->regno[1];
          |               ^~~~

Signed-off-by: Brian Cain <[email protected]>
SidManning and others added 2 commits January 2, 2025 20:30
Note: QTimer was implemented before ARM SSE Timer was upstreamed, there may
be opportunity to use that device instead.

Co-authored-by: Damien Hedde <[email protected]>
Co-authored-by: Tobias Röhmel <[email protected]>
Signed-off-by: Brian Cain <[email protected]>
@androm3da androm3da self-assigned this Jan 3, 2025
Copy link

@taylorsimpson taylorsimpson left a comment

Choose a reason for hiding this comment

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

I'll start reviewing these ...
In general, 100+ patches is going to be hard for the community to review. Consider combining patches whenever possible. Can this be combined with the patch that defines HEX_SREG_BADVA?

if tag == "J4_hintjumpr":
return False
return True
return False

Choose a reason for hiding this comment

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

return ("A_JUMP" in attribdict[tag] or "A_CALL" in attribdict) and tag != "J2_hintmumpr"

self.gen_check_impl(f, regno)
f.write(code_fmt(f"""\
TCGv {self.reg_tcg()} = tcg_temp_new();
gen_read_greg({self.reg_tcg()}, {self.reg_num});

Choose a reason for hiding this comment

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

If this is a Dest, why do we need a read?

"""))
def analyze_read(self, f, regno):
f.write(code_fmt(f"""\
// const int {self.reg_num} = insn->regno[{regno}];

Choose a reason for hiding this comment

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

Looks incomplete - add a FIXME??

self.gen_check_impl(f, regno)
f.write(code_fmt(f"""\
TCGv_i64 {self.reg_tcg()} = tcg_temp_new_i64();
gen_read_greg_pair({self.reg_tcg()}, {self.reg_num});

Choose a reason for hiding this comment

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

Is read needed?

"""))
def analyze_read(self, f, regno):
f.write(code_fmt(f"""\
// const int {self.reg_num} = insn->regno[{regno}];

Choose a reason for hiding this comment

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

FIXME?

self.decl_reg_num(f, regno)
f.write(code_fmt(f"""\
TCGv {self.reg_tcg()} = tcg_temp_new();
gen_read_sreg({self.reg_tcg()}, {self.reg_num});

Choose a reason for hiding this comment

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

Read needed?

"""))
def analyze_read(self, f, regno):
f.write(code_fmt(f"""\
// const int {self.reg_num} = insn->regno[{regno}];

Choose a reason for hiding this comment

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

FIXME??

self.decl_reg_num(f, regno)
f.write(code_fmt(f"""\
TCGv_i64 {self.reg_tcg()} = tcg_temp_new_i64();
gen_read_sreg_pair({self.reg_tcg()}, {self.reg_num});

Choose a reason for hiding this comment

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

Read needed??

"""))
def analyze_read(self, f, regno):
f.write(code_fmt(f"""\
// const int {self.reg_num} = insn->regno[{regno}];

Choose a reason for hiding this comment

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

FIXME??

@@ -42,6 +42,14 @@ def gen_analyze_func(f, tag, regs, imms):
f.write(f"static void analyze_{tag}(DisasContext *ctx)\n")
f.write("{\n")

if hex_common.tag_ignore(tag):
f.write("}\n\n")
return

Choose a reason for hiding this comment

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

For tag_ignore's, we should bail early and never need to generate the analyze function.


if ("A_PRIV" in hex_common.attribdict[tag] or
"A_GUEST" in hex_common.attribdict[tag]):
f.write("#ifndef CONFIG_USER_ONLY\n")

Choose a reason for hiding this comment

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

Should this be outside the function definition?

#ifndef CONFIG_USER_ONLY static void analyze_{tag}(...) { ... } #endif /* !CONFIG_USER_ONLY */

## Skip the priv instructions
if "A_PRIV" in hex_common.attribdict[tag]:
for tag in hex_common.get_user_tags():
if hex_common.tag_ignore(tag):

Choose a reason for hiding this comment

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

Suggest combining this patch with the one that defines get_user_tags

Have get_user_tags/get_sys_tags/get_all_tags remove that tag_ignore ones, so you don't have to check it here.

@@ -60,6 +60,8 @@ def main():
f.write('#include "macros.h.inc"\n\n')

for tag in hex_common.tags:

Choose a reason for hiding this comment

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

hex_common.get_user_tags()

Pretty sure idef parser doesn't deal with system instructions.

Choose a reason for hiding this comment

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

Pretty sure idef parser doesn't deal with system instructions.

Yeah, and we actually skip those below:

            ## Skip the priv instructions
            if "A_PRIV" in hex_common.attribdict[tag]:
                continue
            ## Skip the guest instructions
            if "A_GUEST" in hex_common.attribdict[tag]:
                continue

So we can probably remove these if's and just go with get_user_tags()

for tag in hex_common.get_user_tags():
f.write(f"OPCODE({tag}),\n")

for tag in hex_common.get_sys_tags():

Choose a reason for hiding this comment

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

Wrap these with #ifndef CONFIG_USER_ONLY

if tag == "Y6_diag0":
continue
if tag == "Y6_diag1":
if hex_common.tag_ignore(tag):

Choose a reason for hiding this comment

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

If you modify the get_*_tags functions to remove the tag_ignore functions, this could be

`for tag in hex_common.get_all_tags():
...

It seems like your intent is to remove all uses of hex_common.tags, correct?`

@@ -121,18 +138,7 @@ def main():
f.write('#include "idef-generated-emitter.h.inc"\n\n')

for tag in hex_common.tags:

Choose a reason for hiding this comment

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

See comments on prior patch regarding hex_common.get_all_tags() and tag_ignore.

Copy link

@taylorsimpson taylorsimpson left a comment

Choose a reason for hiding this comment

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

Suggest to combine this with the patch that uses these

Copy link

@taylorsimpson taylorsimpson left a comment

Choose a reason for hiding this comment

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

Suggest to move this later in the series - at least until after these have been added to CPUHexagonState.

@@ -20,6 +20,11 @@

#include "fpu/softfloat-types.h"

#define NUM_GREGS 32
#define GREG_WRITES_MAX 32

Choose a reason for hiding this comment

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

What's a realistic number here? Probably 1 or 2 is the max writes in a packet.

Copy link
Author

Choose a reason for hiding this comment

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

Can't you fill each packet with guest reg transfers? So more like 4 or 5?

I'll double check that this is a per-packet and not per-TB allocation.

#define NUM_GREGS 32
#define GREG_WRITES_MAX 32
#define NUM_SREGS 64
#define SREG_WRITES_MAX 64

Choose a reason for hiding this comment

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

Ditto

Looks like qdev_realize_and_unref must be called first otherwise the
associated irqs functions are null.

Signed-off-by: Sid Manning <[email protected]>
Copy link

@taylorsimpson taylorsimpson left a comment

Choose a reason for hiding this comment

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

Run the following command in your qemu repo
git config diff.orderFile scripts/git.orderfile
It will put files in the order preferred by the community. In particular, the .h files will be at the beginning.

#ifndef CONFIG_USER_ONLY
/* Some system registers are per thread and some are global. */
target_ulong t_sreg[NUM_SREGS];
target_ulong t_sreg_written[NUM_SREGS];

Choose a reason for hiding this comment

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

Not needed since we have removed HEX_DEBUG

target_ulong *g_sreg;

target_ulong greg[NUM_GREGS];
target_ulong greg_written[NUM_GREGS];

Choose a reason for hiding this comment

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

Ditto

@androm3da
Copy link
Author

I'll start reviewing these ... In general, 100+ patches is going to be hard for the community to review. Consider combining patches whenever possible. Can this be combined with the patch that defines HEX_SREG_BADVA?

Combining patches seems to be contrary to the goal to keeping the patches concise.

How about a compromise where I divide this review up into multiple parts?

Copy link

@taylorsimpson taylorsimpson left a comment

Choose a reason for hiding this comment

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

Are all of these needed? Many are not related to system mode.

## Skip the priv instructions
if "A_PRIV" in hex_common.attribdict[tag]:
for tag in hex_common.get_user_tags():
if hex_common.tag_ignore(tag):

Choose a reason for hiding this comment

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

Perhaps the tag_ignore function could be added in this commit instead of target/hexagon: Add some utility functions for sysemu?

I think it makes it easier to understand the motivation behind this helper function if added together with its use here.

@@ -278,11 +278,13 @@ def need_PC(tag):


def need_next_PC(tag):
return "A_CALL" in attribdict[tag]
return "A_CALL" in attribdict[tag] or tag == "J2_trap0" or tag == "J2_trap1"

Choose a reason for hiding this comment

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

nitpick: the title of the commit sounds a bit misleading to me. I thought it was gonna add new instruction semantics. Perhaps it could be "update need_next_PC, multi-cof for sysemu instructions" ?

/*
* Hexagon processors have a strong memory model.
*/
#define TCG_GUEST_DEFAULT_MO (TCG_MO_ALL)

Choose a reason for hiding this comment

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

Just curious: isn't this needed for linux-user mode too? I wonder why it wasn't at upstream already

int sreg_log_idx;
TCGv t_sreg_new_value[NUM_SREGS];
TCGv greg_new_value[NUM_GREGS];
#endif

Choose a reason for hiding this comment

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

target/hexagon: Add guest/sys reg writes to DC

Nit: maybe we could spell out the full DisasContext name to avoid confusions with "Data Cache" (specially since we have another commit that says "target/hexagon: Define DC states ")

* Direct-to-guest is not implemented yet, continuing would cause unexpected
* behavior, so we abort.
*/
#define ASSERT_DIRECT_TO_GUEST_UNSET(ENV, EXCP) \

Choose a reason for hiding this comment

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

I think this would be easier to understand if it is added at the "target/hexagon: Implement do_raise_exception()" commit, which is the first caller of this macro.

@@ -604,6 +604,11 @@ static bool hexagon_tlb_fill(CPUState *cs, vaddr address, int size,
}


#include "hw/core/sysemu-cpu-ops.h"

Choose a reason for hiding this comment

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

I think this commit ( "target/hexagon: Add sysemu_ops" ) could be squashed into the next one ( "target/hexagon: Add cpu_get_phys_page_debug() " )

@@ -489,7 +489,6 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t encoding)
insn->iclass = iclass_bits(encoding);
return 1;
}
g_assert_not_reached();

Choose a reason for hiding this comment

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

FIXME: why remove this unreachable?

I think the explanation could be because we want this invalid packet to raise an exception, which is more realistic regarding the real hardware, instead of assert()-ing here

@@ -33,7 +33,7 @@
# Since: 3.0
##
{ 'enum' : 'SysEmuTarget',
'data' : [ 'aarch64', 'alpha', 'arm', 'avr', 'hppa', 'i386',
'data' : [ 'aarch64', 'alpha', 'arm', 'avr', 'hexagon', 'hppa', 'i386',

Choose a reason for hiding this comment

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

Can this commit ( " qapi: Add hexagon machine to QAPI " ) be squashed into "hw/hexagon: Add machine configs for sysemu" ?

@@ -0,0 +1,85 @@
/*

Choose a reason for hiding this comment

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

I think the previous commit ( "target/hexagon: Add a QTimer address prop" ) could be squashed into this one.

Comment on lines 1319 to 1350
void HELPER(sreg_write)(CPUHexagonState *env, uint32_t reg, uint32_t val)
{
g_assert_not_reached();
}

void HELPER(sreg_write_pair)(CPUHexagonState *env, uint32_t reg, uint64_t val)

{
g_assert_not_reached();
}

uint32_t HELPER(sreg_read)(CPUHexagonState *env, uint32_t reg)
{
g_assert_not_reached();
}

uint64_t HELPER(sreg_read_pair)(CPUHexagonState *env, uint32_t reg)
{
g_assert_not_reached();
}

uint32_t HELPER(greg_read)(CPUHexagonState *env, uint32_t reg)
{
g_assert_not_reached();
}

uint64_t HELPER(greg_read_pair)(CPUHexagonState *env, uint32_t reg)
{
g_assert_not_reached();
}
#endif

Choose a reason for hiding this comment

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

It doesn't look like we "use" these helpers before the commit that actually implements them. If that is correct, I think the separation between adding the stubs and the actual implementation is not necessary, and it might make reviewing harder.

So I'd suggest squashing these two:

  • target/hexagon: Add placeholder greg/sreg r/w helpers
  • target/hexagon: Add sreg_{read,write} helpers

@quic-mathbern
Copy link

Combining patches seems to be contrary to the goal to keeping the patches concise.

I think we might be able to combine a few of the smaller ones without compromising the size of the patches. I've left some comments on the few I think might be "combinable".

How about a compromise where I divide this review up into multiple parts?

I think this is a great idea. Perhaps 3 parts of 35 commits. Since all of them are buildable on their own, we could even merge one part/section at a time, allowing the first patches to brew upstream while we are working on reviews/re-runs for the next parts.

return true;
}
qemu_log_mask(LOG_UNIMP,
"Warning: ignoring write to guest register pair G%d:%d\n",

Choose a reason for hiding this comment

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

Use PRId32 instead of %d - several instances of this.

hex_t_sreg[reg_num + 1]);
}
} else {
gen_helper_sreg_read_pair(dst, tcg_env, tcg_constant_tl(reg_num));

Choose a reason for hiding this comment

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

Suggest to combine this patch with the helper implementation

Copy link

@taylorsimpson taylorsimpson left a comment

Choose a reason for hiding this comment

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

Put this patch first in the series. It helps give context to the other patches.


uint32_t arch_get_system_reg(CPUHexagonState *env, uint32_t reg)
{
g_assert_not_reached();

Choose a reason for hiding this comment

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

Why so many g_assert_not_reached? Go ahead and provide the implementation here.

Copy link
Author

Choose a reason for hiding this comment

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

This was in order to cut down on the size of the patches. Provide a call target so that callers of arch_get_system_reg() will compile and make some sense to read, but postpone filling that in to a subsequent patch.

I guess I've got things upside-down? I deliberately did this kind of thing several times in this series with the thought that it would make things easier for reviewers. But - does it make it harder instead?


uint32_t arch_get_system_reg(CPUHexagonState *env, uint32_t reg);

#define ARCH_GET_THREAD_REG(ENV, REG) \

Choose a reason for hiding this comment

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

I'm confused why these macros exist. If we want to keep them, let's make sure they are used everywhere. Otherwise, just call the functions directly.

@@ -1433,6 +1433,10 @@ void HELPER(setprio)(CPUHexagonState *env, uint32_t thread, uint32_t prio)
g_assert_not_reached();
}

void HELPER(nmi)(CPUHexagonState *env, uint32_t thread_mask)
{
g_assert_not_reached();

Choose a reason for hiding this comment

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

Why not implemented here?

target_ulong threadId;
hex_lock_state_t tlb_lock_state;
hex_lock_state_t k0_lock_state;
target_ulong next_PC;

Choose a reason for hiding this comment

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

Is next_PC needed in the runtime state? Use the value in DisasContext.

* SPDX-License-Identifier: BSD-3-Clause
*/

#ifndef _MAX_H

Choose a reason for hiding this comment

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

#ifndef HEXAGON_MAX_H

#ifndef CONFIG_USER_ONLY
gdb_register_coprocessor(cs, hexagon_sys_gdb_read_register,
hexagon_sys_gdb_write_register,
gdb_find_static_feature("hexagon-sys.xml"), 0);

Choose a reason for hiding this comment

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

Combine this patch with the one that creates hexagon-sys.xml

@androm3da
Copy link
Author

Are all of these needed? Many are not related to system mode.

I'm not 100% certain that all are needed, no. But I thought that the vast majority of them were.

I tried to omit unnecessary changes on a per-feature basis. So that's why things like semihosting and HVX system emulation are omitted/incomplete.

It was my goal to have the minimal set of changes to run the minivm test suite. But if we have some trivial "hello world" sysemu test case that does a system reg write and stop() instruction, then we could likely cut down on the number of changes. However I'm thinking that would help more w/omitting the later patches in this series than the earlier ones.

Choose a reason for hiding this comment

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

Also add hex_mmu.c to the list of system mode files to build

Copy link

@taylorsimpson taylorsimpson left a comment

Choose a reason for hiding this comment

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

For the Co-authored-by's

  • Does Sid have an oss.qualcomm.com email address?
  • Should we put Mike's personal email address?

@androm3da
Copy link
Author

For the Co-authored-by's

  • Does Sid have an oss.qualcomm.com email address?

no.

  • Should we put Mike's personal email address?

no.

BQL_LOCK_GUARD();
CPU_FOREACH(cs) {
HexagonCPU *found_cpu = HEXAGON_CPU(cs);
CPUHexagonState *found_env = &found_cpu->env;

Choose a reason for hiding this comment

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

Use cpu_env
CPUHexagonState *found_env = cpu_env(found_cpu);

Lots of instances of "&cpu->env" need to be changed.

return;
}
}
hex_interrupt_update(env);

Choose a reason for hiding this comment

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

If we get to here without finding the vCPU we are looking for, do we need to call hex_interrupt_update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants