qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Mark Cave-Ayland
Subject: Re: [RFC PATCH v7 14/29] hw/loongarch: Add support loongson3 virt machine type.
Date: Mon, 28 Mar 2022 22:02:05 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]