qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Richard Henderson <richard.henderson@linaro.org>
To: cupertinomiranda@gmail.com, qemu-devel@nongnu.org
Cc: shahab@synopsys.com, linux-snps-arc@lists.infradead.org,
	claziss@synopsys.com, cmiranda@synopsys.com
Subject: Re: [PATCH 01/27] arc: Add initial core cpu files
Date: Tue, 6 Apr 2021 17:47:25 -0700	[thread overview]
Message-ID: <e94289ec-924e-1c3b-f4f2-a267f14e4510@linaro.org> (raw)
In-Reply-To: <20210405143138.17016-2-cupertinomiranda@gmail.com>

On 4/5/21 7:31 AM, cupertinomiranda@gmail.com wrote:
> +    DEFINE_PROP_BOOL("byte-order", ARCCPU, cfg.byte_order, false),

"byte-order" makes no sense as a bool.
"little-endian" or "big-endian" would.

> +    info->endian = BFD_ENDIAN_LITTLE;

Not using the setting?

> +/*-*-indent-tabs-mode:nil;tab-width:4;indent-line-function:'insert-tab'-*-*/
> +/* vim: set ts=4 sw=4 et: */

Should be redundant with .editorconfig.

> +#define CPU_GP(env)     ((env)->r[26])
> +#define CPU_FP(env)     ((env)->r[27])
> +#define CPU_SP(env)     ((env)->r[28])
> +#define CPU_ILINK(env)  ((env)->r[29])
> +#define CPU_ILINK1(env) ((env)->r[29])
> +#define CPU_ILINK2(env) ((env)->r[30])
> +#define CPU_BLINK(env)  ((env)->r[31])
> +#define CPU_LP(env)     ((env)->r[60])
> +#define CPU_IMM(env)    ((env)->r[62])
> +#define CPU_PCL(env)    ((env)->r[63])

Surely this is better as a enum of regnos?

I'm not especially keen on lvalue macros like this, especially when you can't 
reuse the enum for e.g. the tcg globals.

> +/* Flags in pstate */
> +#define Hf_b  (0)
> +#define AEf_b (5)
> +#define Uf_b  (7)
> +#define Lf_b  (12)
> +#define DZf_b (13)
> +#define SCf_b (14)
> +#define ESf_b (15)
> +#define ADf_b (19)
> +#define USf_b (20)
> +
> +/* Flags with their on fields */
> +#define IEf_b   (31)
> +#define IEf_bS  (1)
> +
> +#define Ef_b    (1)
> +#define Ef_bS   (4)
> +
> +#define DEf_b   (6)
> +#define DEf_bS  (1)
> +
> +#define Vf_b    (8)
> +#define Vf_bS   (1)
> +#define Cf_b    (9)
> +#define Cf_bS   (1)
> +#define Nf_b    (10)
> +#define Nf_bS   (1)
> +#define Zf_b    (11)
> +#define Zf_bS   (1)
> +
> +#define RBf_b   (16)
> +#define RBf_bS  (3)

We have include/hw/registerfields.h that's a bit better at defining, 
extracting, and setting fields.

> +#define SET_STATUS_BIT(STAT, BIT, VALUE) { \
> +    STAT.pstate &= ~(1 << BIT##_b); \
> +    STAT.pstate |= (VALUE << BIT##_b); \
> +}

do {
     (STAT).pstate = deposit32((STAT).pstate, BIT, 1, VALUE);
} while (0)

> +typedef struct {
> +    target_ulong pstate;
> +
> +    target_ulong RBf;
> +    target_ulong Ef;     /* irq priority treshold. */
> +    target_ulong Vf;     /*  overflow                */
> +    target_ulong Cf;     /*  carry                   */
> +    target_ulong Nf;     /*  negative                */
> +    target_ulong Zf;     /*  zero                    */
> +    target_ulong DEf;
> +    target_ulong IEf;

I understand why the 4 arithmetic flags are split out, but why are the others? 
  Surely they are not nearly so performance sensitive.

> +/* ARC PIC interrupt bancked regs. */
> +typedef struct {
> +    target_ulong priority;
> +    target_ulong trigger;
> +    target_ulong pulse_cancel;
> +    target_ulong enable;
> +    target_ulong pending;
> +    target_ulong status;
> +} ARCIrq;

This is cpu.h.  The PIC is not the cpu, so this should be elsewhere.

> +typedef struct CPUARCState {
> +    target_ulong        r[64];
> +
> +    ARCStatus stat, stat_l1, stat_er;
> +
> +    struct {
> +        target_ulong    S2;
> +        target_ulong    S1;
> +        target_ulong    CS;
> +    } macmod;
> +
> +    target_ulong intvec;
> +
> +    target_ulong eret;
> +    target_ulong erbta;
> +    target_ulong ecr;
> +    target_ulong efa;
> +    target_ulong bta;
> +    target_ulong bta_l1;
> +    target_ulong bta_l2;
> +
> +    target_ulong pc;     /*  program counter         */

What is this and why is it different from PCL, aka r[63]?

> +    /* used for propagatinng "hostpc/return address" to sub-functions */
> +    uintptr_t host_pc;

Not a fan.  Subfunctions should have the retaddr passed down directly, so that 
it's obvious that the value is not stale.

> +static inline int cpu_mmu_index(const CPUARCState *env, bool ifetch)
> +{
> +    return GET_STATUS_BIT(env->stat, Uf) != 0 ? 1 : 0;
> +}

A very complicated way to just return GET_STATUS_BIT.
Or even GET_STATUS_BIT != 0, come to that.

> +
> +static inline void cpu_get_tb_cpu_state(CPUARCState *env, target_ulong *pc,
> +                                        target_ulong *cs_base,
> +                                        uint32_t *pflags)
> +{
> +    *pc = env->pc;
> +    *cs_base = 0;
> +#ifdef CONFIG_USER_ONLY
> +    assert(0); /* Not really supported at the moment. */

g_assert_not_reached() is our canonical "can't get here".
Though #error would be better in this instance.


r~


  reply	other threads:[~2021-04-07  0:48 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-05 14:31 *** ARC port for review *** cupertinomiranda
2021-04-05 14:31 ` [PATCH 01/27] arc: Add initial core cpu files cupertinomiranda
2021-04-07  0:47   ` Richard Henderson [this message]
2021-04-05 14:31 ` [PATCH 02/27] arc: Decoder code cupertinomiranda
2021-04-07  1:25   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 03/27] arc: Opcode definitions table cupertinomiranda
2021-04-05 14:31 ` [PATCH 04/27] arc: TCG and decoder glue code and helpers cupertinomiranda
2021-04-07  2:37   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 05/27] arc: TCG instruction generator and hand-definitions cupertinomiranda
2021-04-07  3:52   ` Richard Henderson
2021-04-07 16:47   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 06/27] arc: semfunc.c tcg code generator cupertinomiranda
2021-04-07 17:14   ` Richard Henderson
2021-04-07 18:33     ` Peter Maydell
2021-04-05 14:31 ` [PATCH 07/27] arc: TCG instruction definitions cupertinomiranda
2021-04-07 19:38   ` Richard Henderson
2021-04-08  0:20   ` Richard Henderson
2021-04-12 14:27     ` Cupertino Miranda
2021-04-05 14:31 ` [PATCH 08/27] arc: Add BCR and AUX registers implementation cupertinomiranda
2021-04-05 14:31 ` [PATCH 09/27] arc: Add IRQ and timer subsystem support cupertinomiranda
2021-04-05 14:31 ` [PATCH 10/27] arc: Add memory management unit (MMU) support cupertinomiranda
2021-04-05 14:31 ` [PATCH 11/27] arc: Add memory protection unit (MPU) support cupertinomiranda
2021-04-05 14:31 ` [PATCH 12/27] arc: Add gdbstub and XML for debugging support cupertinomiranda
2021-04-05 14:31 ` [PATCH 13/27] arc: Add Synopsys ARC emulation boards cupertinomiranda
2021-04-05 14:31 ` [PATCH 14/27] arc: Add support for ARCv2 cupertinomiranda
2021-04-07 20:30   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 15/27] tests/tcg: ARC: Add TCG instruction definition tests cupertinomiranda
2021-04-07 20:38   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 16/27] tests/acceptance: ARC: Add linux boot testing cupertinomiranda
2021-04-07 20:40   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 17/27] arcv3: Core cpu file changes cupertinomiranda
2021-04-05 14:31 ` [PATCH 18/27] arcv3: Decoder code cupertinomiranda
2021-04-07 23:07   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 19/27] arcv3: Opcode definition table cupertinomiranda
2021-04-05 14:31 ` [PATCH 20/27] arcv3: TCG, decoder glue code and helper changes cupertinomiranda
2021-04-07 23:36   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 21/27] arcv3: TCG instruction generator changes cupertinomiranda
2021-04-07 23:43   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 22/27] arcv3: TCG instruction definitions cupertinomiranda
2021-04-07 23:48   ` Richard Henderson
2021-04-05 14:31 ` [PATCH 23/27] arcv3: BCR and AUX register changes cupertinomiranda
2021-04-05 14:31 ` [PATCH 24/27] arcv3: IRQ changes and new MMUv6 WIP cupertinomiranda
2021-04-05 14:31 ` [PATCH 25/27] arcv3: gdbstub changes and new XML files cupertinomiranda
2021-04-05 14:31 ` [PATCH 26/27] arcv3: board changes cupertinomiranda
2021-04-05 14:31 ` [PATCH 27/27] arcv3: Add support for ARCv3 cupertinomiranda
2021-04-06 23:47 ` *** ARC port for review *** Richard Henderson
2021-04-12 14:25   ` Cupertino Miranda

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e94289ec-924e-1c3b-f4f2-a267f14e4510@linaro.org \
    --to=richard.henderson@linaro.org \
    --cc=claziss@synopsys.com \
    --cc=cmiranda@synopsys.com \
    --cc=cupertinomiranda@gmail.com \
    --cc=linux-snps-arc@lists.infradead.org \
    --cc=qemu-devel@nongnu.org \
    --cc=shahab@synopsys.com \
    --subject='Re: [PATCH 01/27] arc: Add initial core cpu files' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).