On 28/03/2022 21:49, Richard Henderson wrote:
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.
Strangely enough I had a similar requirement for my q800 patches, and when I tried to
implement a ROM memory region then the accesses didn't work as expected. I can't
remember the exact problem however...
+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/.
That's probably my fault; the example of splitting the non-user parts of the CPU into
a separate function was based upon SPARC64 and that code currently lives in
hw/sparc64. I do recall there were some recent discussions about moving such code
into target/* though.
ATB,
Mark.