Re: [PATCH v1 31/43] hw/loongarch: Add support loongson3 virt machine type.

[Richard Henderson]

On 4/15/22 02:40, Xiaojuan Yang wrote:
> +++ b/include/hw/loongarch/loongarch.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Definitions for loongarch board emulation.
> + *
> + * Copyright (C) 2021 Loongson Technology Corporation Limited
> + */
> +
> +#ifndef HW_LOONGARCH_H
> +#define HW_LOONGARCH_H
> +
> +#include "target/loongarch/cpu.h"
> +#include "qemu-common.h"
> +#include "hw/boards.h"
> +#include "qemu/queue.h"
> +
> +#define LOONGARCH_MAX_VCPUS     4
> +
> +#define FEATURE_REG             0x8
> +#define IOCSRF_TEMP             0
> +#define IOCSRF_NODECNT          1
> +#define IOCSRF_MSI              2
> +#define IOCSRF_EXTIOI           3
> +#define IOCSRF_CSRIPI           4
> +#define IOCSRF_FREQCSR          5
> +#define IOCSRF_FREQSCALE        6
> +#define IOCSRF_DVFSV1           7
> +#define IOCSRF_GMOD             9
> +#define IOCSRF_VM               11
> +
> +#define IOCSR_MEM_SIZE          0x428
> +
> +#define VENDOR_REG              0x10
> +#define CPUNAME_REG             0x20
> +#define MISC_FUNC_REG           0x420
> +#define IOCSRM_EXTIOI_EN        48
> +
> +#define LOONGARCH_ISA_IO_BASE   0x18000000UL
> +#define LOONGARCH_ISA_IO_SIZE   0x0004000
> +
> +struct LoongArchMachineState {
> +    /*< private >*/
> +    MachineState parent_obj;
> +
> +    MemoryRegion lowmem;
> +    MemoryRegion highmem;
> +    MemoryRegion isa_io;
> +};
> +
> +#define TYPE_LOONGARCH_MACHINE  MACHINE_TYPE_NAME("virt")
> +OBJECT_DECLARE_SIMPLE_TYPE(LoongArchMachineState, LOONGARCH_MACHINE)
> +#endif

Should this file instead be named "hw/loongarch/virt.h", since it contains definitions for 
the virt machine type?  Presumably at some point you'll be defining a machine board based 
on real hardware, at which point this will seem mis-named.


> +++ b/target/loongarch/cpu.c
> @@ -17,6 +17,8 @@
>  #include "internals.h"
>  #include "fpu/softfloat-helpers.h"
>  #include "cpu-csr.h"
> +#include "sysemu/reset.h"
> +#include "hw/loader.h"
>  
>  const char * const regnames[32] = {
>      "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
> @@ -80,6 +82,8 @@ static void loongarch_cpu_set_pc(CPUState *cs, vaddr value)
>      env->pc = value;
>  }
>  
> +#include "hw/loongarch/loongarch.h"
> +
>  void loongarch_cpu_set_irq(void *opaque, int irq, int level)
>  {
>      LoongArchCPU *cpu = opaque;
> @@ -103,6 +107,48 @@ void loongarch_cpu_set_irq(void *opaque, int irq, int 
> level)
>      }
>  }
>  
> +static void loongarch_qemu_write(void *opaque, hwaddr addr,
> +                                 uint64_t val, unsigned size)
> +{
> +}
> +
> +static uint64_t loongarch_qemu_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    switch (addr) {
> +    case FEATURE_REG:
> +        return 1UL << IOCSRF_MSI | 1UL << IOCSRF_EXTIOI |
> +               1UL << IOCSRF_CSRIPI;
> +    case VENDOR_REG:
> +        return 0x6e6f73676e6f6f4c; /* "Loongson" */
> +    case CPUNAME_REG:
> +        return 0x303030354133;     /* "3A5000" */
> +    case MISC_FUNC_REG:
> +        return 1UL << IOCSRM_EXTIOI_EN;
> +    }
> +    return 0;
> +}
> +
> +static const MemoryRegionOps loongarch_qemu_ops = {
> +    .read = loongarch_qemu_read,
> +    .write = loongarch_qemu_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
> +    },
> +    .impl = {
> +        .min_access_size = 8,
> +        .max_access_size = 8,
> +    },
> +};
> +
> +static void reset_cb(void *opaque)
> +{
> +    LoongArchCPU *cpu = opaque;
> +
> +    cpu_reset(CPU(cpu));
> +}
> +
>  static inline bool cpu_loongarch_hw_interrupts_enabled(CPULoongArchState *env)
>  {
>      bool ret = 0;
> @@ -518,11 +564,19 @@ static void loongarch_cpu_realizefn(DeviceState *dev, 
> Error **errp)
>  static void loongarch_cpu_init(Object *obj)
>  {
>      LoongArchCPU *cpu = LOONGARCH_CPU(obj);
> +    CPULoongArchState *env = &cpu->env;
>  
>      cpu_set_cpustate_pointers(cpu);
>      qdev_init_gpio_in(DEVICE(cpu), loongarch_cpu_set_irq, N_IRQS);
>      timer_init_ns(&cpu->timer, QEMU_CLOCK_VIRTUAL,
>                    &loongarch_constant_timer_cb, cpu);
> +    memory_region_init_io(&env->system_iocsr, OBJECT(cpu), NULL,
> +                      env, "iocsr", UINT64_MAX);
> +    address_space_init(&env->address_space_iocsr, &env->system_iocsr, "IOCSR");
> +    qemu_register_reset(reset_cb, cpu);
> +    memory_region_init_io(&env->iocsr_mem, OBJECT(cpu), &loongarch_qemu_ops,
> +                          NULL, "iocsr_misc", IOCSR_MEM_SIZE);
> +    memory_region_add_subregion(&env->system_iocsr, 0, &env->iocsr_mem);
>  }
>  
>  static ObjectClass *loongarch_cpu_class_by_name(const char *cpu_model)

These changes belong elsewhere, not while defining -M virt.
Some of it earlier, where you defined the iocsr address space.

Do not use qemu_register_reset, use device_class_set_parent_reset.


r~