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

[Yong-Xuan Wang]

Hi  Daniel,

Thanks for your suggestions! I'll fix it in patch v4.

Regards,
Yong-Xuan

On Tue, Jun 6, 2023 at 9:45 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> 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