Re: [RFC PATCH v7 14/29] hw/loongarch: Add support loongson3 virt machine type.

[Richard Henderson]

On 3/28/22 06:57, Xiaojuan Yang wrote:
> +static uint64_t loongarch_qemu_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    uint64_t feature = 0UL;
> +
> +    switch (addr) {
> +    case FEATURE_REG:
> +        feature |= 1UL << IOCSRF_MSI | 1UL << IOCSRF_EXTIOI |
> +                   1UL << IOCSRF_CSRIPI;
> +        return feature ;

What's the point of the feature variable?

> +    case VENDOR_REG:
> +        return *(uint64_t *)"Loongson";
> +    case CPUNAME_REG:
> +        return *(uint64_t *)"3A5000";

This is definitely wrong, as (1) it depends on host endianness, and (2) you're reading 8 
bytes from a 7 byte string.

> +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 = 4,
> +        .max_access_size = 8,
> +    },

The implementation above doesn't actually support access size 4; it only 
supports 8.
It doesn't seem like this should be a io region at all, but a ROM.


> +static void loongarch_cpu_init(LoongArchCPU *la_cpu, int cpu_num)
> +{
> +    CPULoongArchState *env;
> +    env = &la_cpu->env;
> +
> +    memory_region_init_io(&env->system_iocsr, OBJECT(la_cpu), NULL,
> +                      env, "iocsr", UINT64_MAX);
> +    address_space_init(&env->address_space_iocsr, &env->system_iocsr, "IOCSR");
> +
> +    timer_init_ns(&la_cpu->timer, QEMU_CLOCK_VIRTUAL,
> +                  &loongarch_constant_timer_cb, la_cpu);

This timer belongs to the cpu, not the board model.
This init belongs over in target/loongarch/.


r~