Re: [Qemu-devel] [PATCH 06/12 v9] target-tilegx: Add cpu basic features for linux-user

[Peter Maydell]

On 27 March 2015 at 10:54, Chen Gang <address@hidden> wrote:
> It implements minimized cpu features for linux-user.
>
> Signed-off-by: Chen Gang <address@hidden>
> ---
>  target-tilegx/cpu-qom.h |  73 ++++++++++++++++++++++++
>  target-tilegx/cpu.c     | 149 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  target-tilegx/cpu.h     |  94 ++++++++++++++++++++++++++++++

You don't really need a separate cpu-qom.h and cpu.h -- that's
just the way we've ended up with for the older targets which
got converted to QOM for legacy reasons. See target-moxie/
for an example which combines the two headers.


> +static const VMStateDescription vmstate_tilegx_cpu = {
> +    .name = "cpu",
> +    .unmigratable = 1,
> +};

I'd prefer to see a correct VMState from the start -- it's
not very difficult. Migration/snapshotting is much easier
to enforce at the point where we let code in to the tree
than if we let in non-migratable devices and CPUs and then
have to fix them up later...


> +++ b/target-tilegx/cpu.h
> @@ -0,0 +1,94 @@
> +/*
> + *  TILE-Gx virtual CPU header
> + *
> + *  Copyright (c) 2015 Chen Gang
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +#ifndef CPU_TILEGX_H
> +#define CPU_TILEGX_H
> +
> +#include "config.h"
> +#include "qemu-common.h"
> +
> +#define TARGET_LONG_BITS 64
> +
> +#define CPUArchState struct CPUTLGState
> +
> +#include "exec/cpu-defs.h"
> +#include "fpu/softfloat.h"

What's the softfloat include for?

> +
> +/* TILE-Gx register alias */
> +#define TILEGX_R_RE    0   /*  0 register, for function/syscall return value 
> */
> +#define TILEGX_R_NR    10  /* 10 register, for syscall number */
> +#define TILEGX_R_BP    52  /* 52 register, optional frame pointer */
> +#define TILEGX_R_TP    53  /* TP register, thread local storage data */
> +#define TILEGX_R_SP    54  /* SP register, stack pointer */
> +#define TILEGX_R_LR    55  /* LR register, may save pc, but it is not pc */
> +#define TILEGX_R_ZERO  63  /* Zero register, always zero */
> +#define TILEGX_R_COUNT 56  /* Only 56 registers are really useful */
> +#define TILEGX_R_NOREG 255 /* Invalid register value */
> +
> +
> +typedef struct CPUTLGState {
> +    uint64_t regs[TILEGX_R_COUNT]; /* Common used registers by outside */
> +    uint64_t pc;                   /* Current pc */
> +
> +    CPU_COMMON
> +} CPUTLGState;
> +
> +#include "cpu-qom.h"
> +
> +/* TILE-Gx memory attributes */
> +#define TARGET_PAGE_BITS 16  /* TILE-Gx uses 64KB page size */
> +#define MMAP_SHIFT TARGET_PAGE_BITS
> +#define TARGET_PHYS_ADDR_SPACE_BITS 42 /* TILE-Gx is 42 bit physical address 
> */
> +#define TARGET_VIRT_ADDR_SPACE_BITS 64 /* TILE-Gx has 64 bit virtual address 
> */

nitpick: "has [...] addresses" is the correct grammar in both these comments.

> +#define MMU_USER_IDX    0  /* independent from both qemu and architecture */

Not sure what you mean with this comment?

thanks
-- PMM