Re: [PATCH v3 4/6] target/riscv: Create an KVM AIA irqchip

[Daniel Henrique Barboza]

On 5/26/23 03:25, Yong-Xuan Wang wrote:
> implement a function to create an KVM AIA chip
>
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> ---
>   target/riscv/kvm.c       | 83 ++++++++++++++++++++++++++++++++++++++++
>   target/riscv/kvm_riscv.h |  3 ++
>   2 files changed, 86 insertions(+)
>
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index eb469e8ca5..ead121154f 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -34,6 +34,7 @@
>   #include "exec/address-spaces.h"
>   #include "hw/boards.h"
>   #include "hw/irq.h"
> +#include "hw/intc/riscv_imsic.h"
>   #include "qemu/log.h"
>   #include "hw/loader.h"
>   #include "kvm_riscv.h"
> @@ -548,3 +549,85 @@ bool kvm_arch_cpu_check_are_resettable(void)
>   void kvm_arch_accel_class_init(ObjectClass *oc)
>   {
>   }
> +
> +void kvm_riscv_aia_create(DeviceState *aplic_s, bool msimode, int socket,
> +                          uint64_t aia_irq_num, uint64_t hart_count,
> +                          uint64_t aplic_base, uint64_t imsic_base)
> +{
> +    int ret;
> +    int aia_fd = -1;
> +    uint64_t aia_mode;
> +    uint64_t aia_nr_ids;
> +    uint64_t aia_hart_bits = find_last_bit(&hart_count, BITS_PER_LONG) + 1;
> +
> +    if (!msimode) {
> +        error_report("Currently KVM AIA only supports aplic_imsic mode");
> +        exit(1);
> +    }
> +
> +    aia_fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_RISCV_AIA, false);
> +
> +    if (aia_fd < 0) {
> +        error_report("Unable to create in-kernel irqchip");
> +        exit(1);
> +    }
> +
> +    ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
> +                            KVM_DEV_RISCV_AIA_CONFIG_MODE,
> +                            &aia_mode, false, NULL);
> +
> +    ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
> +                            KVM_DEV_RISCV_AIA_CONFIG_IDS,
> +                            &aia_nr_ids, false, NULL);
> +
> +    ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
> +                            KVM_DEV_RISCV_AIA_CONFIG_SRCS,
> +                            &aia_irq_num, true, NULL);
> +    if (ret < 0) {
> +        error_report("KVM AIA: fail to set number input irq lines");
> +        exit(1);
> +    }

I see that you didn't check 'ret' for the first 2 calls of kvm_device_access().
Is it intentional?

Since you're setting customized error messages for every step I think it's worth
also handling the case where we fail to set aia_mode and aia_nr_ids.


Thanks,


Daniel


> +
> +    ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CONFIG,
> +                            KVM_DEV_RISCV_AIA_CONFIG_HART_BITS,
> +                            &aia_hart_bits, true, NULL);
> +    if (ret < 0) {
> +        error_report("KVM AIA: fail to set number of harts");
> +        exit(1);
> +    }
> +
> +    ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_ADDR,
> +                            KVM_DEV_RISCV_AIA_ADDR_APLIC,
> +                            &aplic_base, true, NULL);
> +    if (ret < 0) {
> +        error_report("KVM AIA: fail to set the base address of APLIC");
> +        exit(1);
> +    }
> +
> +    for (int i = 0; i < hart_count; i++) {
> +        uint64_t imsic_addr = imsic_base + i * IMSIC_HART_SIZE(0);
> +        ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_ADDR,
> +                                KVM_DEV_RISCV_AIA_ADDR_IMSIC(i),
> +                                &imsic_addr, true, NULL);
> +        if (ret < 0) {
> +            error_report("KVM AIA: fail to set the base address of IMSICs");
> +            exit(1);
> +        }
> +    }
> +
> +    if (kvm_has_gsi_routing()) {
> +        for (uint64_t idx = 0; idx < aia_irq_num + 1; ++idx) {
> +            kvm_irqchip_add_irq_route(kvm_state, idx, socket, idx);
> +        }
> +        kvm_gsi_routing_allowed = true;
> +        kvm_irqchip_commit_routes(kvm_state);
> +    }
> +
> +    ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_CTRL,
> +                            KVM_DEV_RISCV_AIA_CTRL_INIT,
> +                            NULL, true, NULL);
> +    if (ret < 0) {
> +        error_report("KVM AIA: initialized fail");
> +        exit(1);
> +    }
> +}
> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
> index 606968a4b7..6067adff51 100644
> --- a/target/riscv/kvm_riscv.h
> +++ b/target/riscv/kvm_riscv.h
> @@ -21,6 +21,9 @@
>   
>   void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
>   void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
> +void kvm_riscv_aia_create(DeviceState *aplic_s, bool msimode, int socket,
> +                          uint64_t aia_irq_num, uint64_t hart_count,
> +                          uint64_t aplic_base, uint64_t imsic_base);
>   
>   #define KVM_DEV_RISCV_AIA_GRP_CONFIG            0
>   #define KVM_DEV_RISCV_AIA_CONFIG_MODE           0