[PATCH v2 1/9] hw/intc: Allow gaps in hartids for aclint and aplic

Djordje Todorovic posted 9 patches 5 months, 2 weeks ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Paolo Bonzini <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
[PATCH v2 1/9] hw/intc: Allow gaps in hartids for aclint and aplic
Posted by Djordje Todorovic 5 months, 2 weeks ago
This is needed for riscv based CPUs by MIPS.

Signed-off-by: Chao-ying Fu <cfu@mips.com>
Signed-off-by: Djordje Todorovic <djordje.todorovic@htecgroup.com>
---
 hw/intc/riscv_aclint.c | 33 +++++++++++++++++++++++++++++++--
 hw/intc/riscv_aplic.c  | 10 +++++++---
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
index b0139f03f5..5bda02a179 100644
--- a/hw/intc/riscv_aclint.c
+++ b/hw/intc/riscv_aclint.c
@@ -131,6 +131,9 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, hwaddr addr,
         size_t hartid = mtimer->hartid_base +
                         ((addr - mtimer->timecmp_base) >> 3);
         CPUState *cpu = cpu_by_arch_id(hartid);
+        if (cpu == NULL) {
+            return 0;
+        }
         CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
         if (!env) {
             qemu_log_mask(LOG_GUEST_ERROR,
@@ -174,6 +177,9 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
         size_t hartid = mtimer->hartid_base +
                         ((addr - mtimer->timecmp_base) >> 3);
         CPUState *cpu = cpu_by_arch_id(hartid);
+        if (cpu == NULL) {
+            return;
+        }
         CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
         if (!env) {
             qemu_log_mask(LOG_GUEST_ERROR,
@@ -233,6 +239,9 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
         /* Check if timer interrupt is triggered for each hart. */
         for (i = 0; i < mtimer->num_harts; i++) {
             CPUState *cpu = cpu_by_arch_id(mtimer->hartid_base + i);
+            if (cpu == NULL) {
+                continue;
+            }
             CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
             if (!env) {
                 continue;
@@ -292,7 +301,11 @@ static void riscv_aclint_mtimer_realize(DeviceState *dev, Error **errp)
     s->timecmp = g_new0(uint64_t, s->num_harts);
     /* Claim timer interrupt bits */
     for (i = 0; i < s->num_harts; i++) {
-        RISCVCPU *cpu = RISCV_CPU(cpu_by_arch_id(s->hartid_base + i));
+        CPUState *temp = cpu_by_arch_id(s->hartid_base + i);
+        if (temp == NULL) {
+            continue;
+        }
+        RISCVCPU *cpu = RISCV_CPU(temp);
         if (riscv_cpu_claim_interrupts(cpu, MIP_MTIP) < 0) {
             error_report("MTIP already claimed");
             exit(1);
@@ -373,6 +386,9 @@ DeviceState *riscv_aclint_mtimer_create(hwaddr addr, hwaddr size,
 
     for (i = 0; i < num_harts; i++) {
         CPUState *cpu = cpu_by_arch_id(hartid_base + i);
+        if (cpu == NULL) {
+            continue;
+        }
         RISCVCPU *rvcpu = RISCV_CPU(cpu);
         CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
         riscv_aclint_mtimer_callback *cb =
@@ -408,6 +424,9 @@ static uint64_t riscv_aclint_swi_read(void *opaque, hwaddr addr,
     if (addr < (swi->num_harts << 2)) {
         size_t hartid = swi->hartid_base + (addr >> 2);
         CPUState *cpu = cpu_by_arch_id(hartid);
+        if (cpu == NULL) {
+            return 0;
+        }
         CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
         if (!env) {
             qemu_log_mask(LOG_GUEST_ERROR,
@@ -431,6 +450,9 @@ static void riscv_aclint_swi_write(void *opaque, hwaddr addr, uint64_t value,
     if (addr < (swi->num_harts << 2)) {
         size_t hartid = swi->hartid_base + (addr >> 2);
         CPUState *cpu = cpu_by_arch_id(hartid);
+        if (cpu == NULL) {
+            return;
+        }
         CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
         if (!env) {
             qemu_log_mask(LOG_GUEST_ERROR,
@@ -481,7 +503,11 @@ static void riscv_aclint_swi_realize(DeviceState *dev, Error **errp)
 
     /* Claim software interrupt bits */
     for (i = 0; i < swi->num_harts; i++) {
-        RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(swi->hartid_base + i));
+        CPUState *temp = cpu_by_arch_id(swi->hartid_base + i);
+        if (temp == NULL) {
+            continue;
+        }
+        RISCVCPU *cpu = RISCV_CPU(temp);
         /* We don't claim mip.SSIP because it is writable by software */
         if (riscv_cpu_claim_interrupts(cpu, swi->sswi ? 0 : MIP_MSIP) < 0) {
             error_report("MSIP already claimed");
@@ -545,6 +571,9 @@ DeviceState *riscv_aclint_swi_create(hwaddr addr, uint32_t hartid_base,
 
     for (i = 0; i < num_harts; i++) {
         CPUState *cpu = cpu_by_arch_id(hartid_base + i);
+        if (cpu == NULL) {
+            continue;
+        }
         RISCVCPU *rvcpu = RISCV_CPU(cpu);
 
         qdev_connect_gpio_out(dev, i,
diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
index 8bcd9f4697..360a3dc117 100644
--- a/hw/intc/riscv_aplic.c
+++ b/hw/intc/riscv_aplic.c
@@ -899,9 +899,11 @@ static void riscv_aplic_realize(DeviceState *dev, Error **errp)
         if (!aplic->msimode) {
             /* Claim the CPU interrupt to be triggered by this APLIC */
             for (i = 0; i < aplic->num_harts; i++) {
-                RISCVCPU *cpu;
-
-                cpu = RISCV_CPU(cpu_by_arch_id(aplic->hartid_base + i));
+                CPUState *temp = cpu_by_arch_id(aplic->hartid_base + i);
+                if (temp == NULL) {
+                    continue;
+                }
+                RISCVCPU *cpu = RISCV_CPU(temp);
                 if (riscv_cpu_claim_interrupts(cpu,
                     (aplic->mmode) ? MIP_MEIP : MIP_SEIP) < 0) {
                     error_report("%s already claimed",
@@ -1076,6 +1078,8 @@ DeviceState *riscv_aplic_create(hwaddr addr, hwaddr size,
         if (!msimode) {
             for (i = 0; i < num_harts; i++) {
                 CPUState *cpu = cpu_by_arch_id(hartid_base + i);
+                if (cpu == NULL)
+                    continue;
 
                 qdev_connect_gpio_out_named(dev, NULL, i,
                                             qdev_get_gpio_in(DEVICE(cpu),
-- 
2.34.1
Re: [PATCH v2 1/9] hw/intc: Allow gaps in hartids for aclint and aplic
Posted by Philippe Mathieu-Daudé 5 months, 1 week ago
Hi,

On 2/6/25 15:12, Djordje Todorovic wrote:
> This is needed for riscv based CPUs by MIPS.

This justification is not really convincing.

> 
> Signed-off-by: Chao-ying Fu <cfu@mips.com>
> Signed-off-by: Djordje Todorovic <djordje.todorovic@htecgroup.com>
> ---
>   hw/intc/riscv_aclint.c | 33 +++++++++++++++++++++++++++++++--
>   hw/intc/riscv_aplic.c  | 10 +++++++---
>   2 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index b0139f03f5..5bda02a179 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -131,6 +131,9 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, hwaddr addr,
>           size_t hartid = mtimer->hartid_base +
>                           ((addr - mtimer->timecmp_base) >> 3);
>           CPUState *cpu = cpu_by_arch_id(hartid);
> +        if (cpu == NULL) {
> +            return 0;

It looks like some misconfiguration. How do you reach that?
Otherwise please log as GUEST_ERROR.

> +        }
>           CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
>           if (!env) {
>               qemu_log_mask(LOG_GUEST_ERROR,
> @@ -174,6 +177,9 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
>           size_t hartid = mtimer->hartid_base +
>                           ((addr - mtimer->timecmp_base) >> 3);
>           CPUState *cpu = cpu_by_arch_id(hartid);
> +        if (cpu == NULL) {

If so, please log:

             qemu_log_mask(LOG_GUEST_ERROR,
                           "aclint-mtimer: invalid hartid: %zu", hartid);

> +            return;
> +        }
>           CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;

Then env is valid, so this line needs update.
Re: [PATCH v2 1/9] hw/intc: Allow gaps in hartids for aclint and aplic
Posted by Djordje Todorovic 5 months ago
On 10. 6. 25. 09:34, Philippe Mathieu-Daudé wrote:
> CAUTION: This email originated from outside of the organization. Do 
> not click links or open attachments unless you recognize the sender 
> and know the content is safe.
>
>
> Hi,
>
> On 2/6/25 15:12, Djordje Todorovic wrote:
>> This is needed for riscv based CPUs by MIPS.
>
> This justification is not really convincing.
>
>>
>> Signed-off-by: Chao-ying Fu <cfu@mips.com>
>> Signed-off-by: Djordje Todorovic <djordje.todorovic@htecgroup.com>
>> ---
>>   hw/intc/riscv_aclint.c | 33 +++++++++++++++++++++++++++++++--
>>   hw/intc/riscv_aplic.c  | 10 +++++++---
>>   2 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
>> index b0139f03f5..5bda02a179 100644
>> --- a/hw/intc/riscv_aclint.c
>> +++ b/hw/intc/riscv_aclint.c
>> @@ -131,6 +131,9 @@ static uint64_t riscv_aclint_mtimer_read(void 
>> *opaque, hwaddr addr,
>>           size_t hartid = mtimer->hartid_base +
>>                           ((addr - mtimer->timecmp_base) >> 3);
>>           CPUState *cpu = cpu_by_arch_id(hartid);
>> +        if (cpu == NULL) {
>> +            return 0;
>
> It looks like some misconfiguration. How do you reach that?
> Otherwise please log as GUEST_ERROR.
>
You are right, at the latest, it does not trigger those.

I will remove it in v3.

Thanks!

>> +        }
>>           CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
>>           if (!env) {
>>               qemu_log_mask(LOG_GUEST_ERROR,
>> @@ -174,6 +177,9 @@ static void riscv_aclint_mtimer_write(void 
>> *opaque, hwaddr addr,
>>           size_t hartid = mtimer->hartid_base +
>>                           ((addr - mtimer->timecmp_base) >> 3);
>>           CPUState *cpu = cpu_by_arch_id(hartid);
>> +        if (cpu == NULL) {
>
> If so, please log:
>
>             qemu_log_mask(LOG_GUEST_ERROR,
>                           "aclint-mtimer: invalid hartid: %zu", hartid);
>
>> +            return;
>> +        }
>>           CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
>
> Then env is valid, so this line needs update.