Re: [PATCH 05/11] LoongArch: KVM: Add EXTIOI device support

[maobibo]

On 2024/7/5 上午10:38, Xianglai Li wrote:
> Added device model for EXTIOI interrupt controller,
> implemented basic create destroy interface,
> and registered device model to kvm device table.
>
> Signed-off-by: Tianrui Zhao <zhaotianrui@xxxxxxxxxxx>
> Signed-off-by: Xianglai Li <lixianglai@xxxxxxxxxxx>
> ---
> Cc: Bibo Mao <maobibo@xxxxxxxxxxx>
> Cc: Huacai Chen <chenhuacai@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: loongarch@xxxxxxxxxxxxxxx
> Cc: Min Zhou <zhoumin@xxxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Tianrui Zhao <zhaotianrui@xxxxxxxxxxx>
> Cc: WANG Xuerui <kernel@xxxxxxxxxx>
> Cc: Xianglai li <lixianglai@xxxxxxxxxxx>
>
>   arch/loongarch/include/asm/kvm_extioi.h |  78 +++++++++++++++++
>   arch/loongarch/include/asm/kvm_host.h   |   2 +
>   arch/loongarch/kvm/Makefile             |   1 +
>   arch/loongarch/kvm/intc/extioi.c        | 111 ++++++++++++++++++++++++
>   arch/loongarch/kvm/main.c               |   6 +-
>   include/uapi/linux/kvm.h                |   2 +
>   6 files changed, 199 insertions(+), 1 deletion(-)
>   create mode 100644 arch/loongarch/include/asm/kvm_extioi.h
>   create mode 100644 arch/loongarch/kvm/intc/extioi.c
>
> diff --git a/arch/loongarch/include/asm/kvm_extioi.h b/arch/loongarch/include/asm/kvm_extioi.h
> new file mode 100644
> index 000000000000..48a117b2be5d
> --- /dev/null
> +++ b/arch/loongarch/include/asm/kvm_extioi.h
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 Loongson Technology Corporation Limited
> + */
> +
> +#ifndef LOONGARCH_EXTIOI_H
> +#define LOONGARCH_EXTIOI_H
> +
> +#include <kvm/iodev.h>
> +
> +#define EXTIOI_IRQS			256
> +#define EXTIOI_ROUTE_MAX_VCPUS		256
> +#define EXTIOI_IRQS_U8_NUMS		(EXTIOI_IRQS / 8)
> +#define EXTIOI_IRQS_U32_NUMS		(EXTIOI_IRQS_U8_NUMS / 4)
> +#define EXTIOI_IRQS_U64_NUMS		(EXTIOI_IRQS_U32_NUMS / 2)
> +/* map to ipnum per 32 irqs */
> +#define EXTIOI_IRQS_NODETYPE_COUNT	16
> +
> +#define EXTIOI_BASE			0x1400
> +#define EXTIOI_SIZE			0x900
> +
> +#define LS3A_INTC_IP			8
How about rename LS3A_INTC_IP with something like CPU_IP_NUM?

> +
> +struct loongarch_extioi {
> +	spinlock_t lock;
> +	struct kvm *kvm;
> +	struct kvm_io_device device;
> +	/* hardware state */
> +	union nodetype {
> +		u64 reg_u64[EXTIOI_IRQS_NODETYPE_COUNT / 4];
> +		u32 reg_u32[EXTIOI_IRQS_NODETYPE_COUNT / 2];
> +		uint16_t reg_u16[EXTIOI_IRQS_NODETYPE_COUNT];
> +		u8 reg_u8[EXTIOI_IRQS_NODETYPE_COUNT * 2];
> +	} nodetype;
> +
> +	/* one bit shows the state of one irq */
> +	union bounce {
> +		u64 reg_u64[EXTIOI_IRQS_U64_NUMS];
> +		u32 reg_u32[EXTIOI_IRQS_U32_NUMS];
> +		u8 reg_u8[EXTIOI_IRQS_U8_NUMS];
> +	} bounce;
> +
> +	union isr {
> +		u64 reg_u64[EXTIOI_IRQS_U64_NUMS];
> +		u32 reg_u32[EXTIOI_IRQS_U32_NUMS];
> +		u8 reg_u8[EXTIOI_IRQS_U8_NUMS];
> +	} isr;
> +	union coreisr {
> +		u64 reg_u64[EXTIOI_ROUTE_MAX_VCPUS][EXTIOI_IRQS_U64_NUMS];
> +		u32 reg_u32[EXTIOI_ROUTE_MAX_VCPUS][EXTIOI_IRQS_U32_NUMS];
> +		u8 reg_u8[EXTIOI_ROUTE_MAX_VCPUS][EXTIOI_IRQS_U8_NUMS];
> +	} coreisr;
> +	union enable {
> +		u64 reg_u64[EXTIOI_IRQS_U64_NUMS];
> +		u32 reg_u32[EXTIOI_IRQS_U32_NUMS];
> +		u8 reg_u8[EXTIOI_IRQS_U8_NUMS];
> +	} enable;
> +
> +	/* use one byte to config ipmap for 32 irqs at once */
> +	union ipmap {
> +		u64 reg_u64;
> +		u32 reg_u32[EXTIOI_IRQS_U32_NUMS / 4];
> +		u8 reg_u8[EXTIOI_IRQS_U8_NUMS / 4];
> +	} ipmap;
> +	/* use one byte to config coremap for one irq */
> +	union coremap {
> +		u64 reg_u64[EXTIOI_IRQS / 8];
> +		u32 reg_u32[EXTIOI_IRQS / 4];
> +		u8 reg_u8[EXTIOI_IRQS];
> +	} coremap;
> +
> +	DECLARE_BITMAP(sw_coreisr[EXTIOI_ROUTE_MAX_VCPUS][LS3A_INTC_IP], EXTIOI_IRQS);
> +	uint8_t  sw_coremap[EXTIOI_IRQS];
> +};
> +
> +void extioi_set_irq(struct loongarch_extioi *s, int irq, int level);
> +int kvm_loongarch_register_extioi_device(void);
> +#endif /* LOONGARCH_EXTIOI_H */
> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
> index b28487975336..0e4e46e06420 100644
> --- a/arch/loongarch/include/asm/kvm_host.h
> +++ b/arch/loongarch/include/asm/kvm_host.h
> @@ -20,6 +20,7 @@
>   #include <asm/kvm_mmu.h>
>   #include <asm/loongarch.h>
>   #include <asm/kvm_ipi.h>
> +#include <asm/kvm_extioi.h>
>   
>   /* Loongarch KVM register ids */
>   #define KVM_GET_IOC_CSR_IDX(id)		((id & KVM_CSR_IDX_MASK) >> LOONGARCH_REG_SHIFT)
> @@ -111,6 +112,7 @@ struct kvm_arch {
>   	s64 time_offset;
>   	struct kvm_context __percpu *vmcs;
>   	struct loongarch_ipi *ipi;
> +	struct loongarch_extioi *extioi;
>   };
>   
>   #define CSR_MAX_NUMS		0x800
> diff --git a/arch/loongarch/kvm/Makefile b/arch/loongarch/kvm/Makefile
> index 36c3009fe89c..a481952e3855 100644
> --- a/arch/loongarch/kvm/Makefile
> +++ b/arch/loongarch/kvm/Makefile
> @@ -19,5 +19,6 @@ kvm-y += tlb.o
>   kvm-y += vcpu.o
>   kvm-y += vm.o
>   kvm-y += intc/ipi.o
> +kvm-y += intc/extioi.o
>   
>   CFLAGS_exit.o	+= $(call cc-option,-Wno-override-init,)
> diff --git a/arch/loongarch/kvm/intc/extioi.c b/arch/loongarch/kvm/intc/extioi.c
> new file mode 100644
> index 000000000000..2f1b93e95f97
> --- /dev/null
> +++ b/arch/loongarch/kvm/intc/extioi.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Loongson Technology Corporation Limited
> + */
> +
> +#include <asm/kvm_extioi.h>
> +#include <asm/kvm_vcpu.h>
> +#include <linux/count_zeros.h>
> +
> +static int kvm_loongarch_extioi_write(struct kvm_vcpu *vcpu,
> +				struct kvm_io_device *dev,
> +				gpa_t addr, int len, const void *val)
> +{
> +	return 0;
> +}
> +
> +static int kvm_loongarch_extioi_read(struct kvm_vcpu *vcpu,
> +				struct kvm_io_device *dev,
> +				gpa_t addr, int len, void *val)
> +{
> +	return 0;
> +}
> +
> +static const struct kvm_io_device_ops kvm_loongarch_extioi_ops = {
> +	.read	= kvm_loongarch_extioi_read,
> +	.write	= kvm_loongarch_extioi_write,
> +};
> +
> +static int kvm_loongarch_extioi_get_attr(struct kvm_device *dev,
> +				struct kvm_device_attr *attr)
> +{
> +	return 0;
> +}
> +
> +static int kvm_loongarch_extioi_set_attr(struct kvm_device *dev,
> +				struct kvm_device_attr *attr)
> +{
> +	return 0;
> +}
> +
> +static void kvm_loongarch_extioi_destroy(struct kvm_device *dev)
> +{
> +	struct kvm *kvm;
> +	struct loongarch_extioi *extioi;
> +	struct kvm_io_device *device;
> +
> +	if (!dev)
> +		return;
> +
> +	kvm = dev->kvm;
> +	if (!kvm)
> +		return;
> +
> +	extioi = kvm->arch.extioi;
> +	if (!extioi)
> +		return;
How about the sentence like this?
	if (!dev || !dev->kvm || !dev->kvm->arch.extioi)
		return;
> +
> +	device = &extioi->device;
> +	kvm_io_bus_unregister_dev(kvm, KVM_IOCSR_BUS, device);
> +	kfree(extioi);
> +}
> +
> +static int kvm_loongarch_extioi_create(struct kvm_device *dev, u32 type)
> +{
> +	int ret;
> +	struct loongarch_extioi *s;
> +	struct kvm_io_device *device;
> +	struct kvm *kvm = dev->kvm;
> +
> +	/* extioi has been created */
> +	if (kvm->arch.extioi)
> +		return -EINVAL;
> +
> +	s = kzalloc(sizeof(struct loongarch_extioi), GFP_KERNEL);
> +	if (!s)
> +		return -ENOMEM;
> +	spin_lock_init(&s->lock);
> +	s->kvm = kvm;
> +
> +	/*
> +	 * Initialize IOCSR device
> +	 */
> +	device = &s->device;
> +	kvm_iodevice_init(device, &kvm_loongarch_extioi_ops);
> +	mutex_lock(&kvm->slots_lock);
> +	ret = kvm_io_bus_register_dev(kvm, KVM_IOCSR_BUS, EXTIOI_BASE, EXTIOI_SIZE, device);
> +	mutex_unlock(&kvm->slots_lock);
> +	if (ret < 0) {
> +		kfree(s);
> +		return -EFAULT;
Return value ret rather than -EFAULT?

> +	}
> +
> +	kvm->arch.extioi = s;
> +
Empty line can be removed
> +	kvm_info("create extioi device successfully\n");
kvm_info() need be removed.
> +	return 0;
> +}
> +
> +static struct kvm_device_ops kvm_loongarch_extioi_dev_ops = {
> +	.name = "kvm-loongarch-extioi",
> +	.create = kvm_loongarch_extioi_create,
> +	.destroy = kvm_loongarch_extioi_destroy,
> +	.set_attr = kvm_loongarch_extioi_set_attr,
> +	.get_attr = kvm_loongarch_extioi_get_attr,
For simplity, it can be it kvm_extioi_xxx if you like.

> +};
> +
> +int kvm_loongarch_register_extioi_device(void)
> +{
> +	return kvm_register_device_ops(&kvm_loongarch_extioi_dev_ops,
> +					KVM_DEV_TYPE_LA_EXTIOI);
> +}
> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
> index 36efc7b38f83..b5da4341006a 100644
> --- a/arch/loongarch/kvm/main.c
> +++ b/arch/loongarch/kvm/main.c
> @@ -9,6 +9,7 @@
>   #include <asm/cacheflush.h>
>   #include <asm/cpufeature.h>
>   #include <asm/kvm_csr.h>
> +#include <asm/kvm_extioi.h>
>   #include "trace.h"
>   
>   unsigned long vpid_mask;
> @@ -372,7 +373,10 @@ static int kvm_loongarch_env_init(void)
>   	if (ret)
>   		return ret;
>   
> -	return 0;
> +	/* Register loongarch extioi interrupt controller interface. */
> +	ret = kvm_loongarch_register_extioi_device();
> +
empty line can be removed here.

Regards
Bibo Mao
> +	return ret;
>   }
>   
>   static void kvm_loongarch_env_exit(void)
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 341fc9d5f3ec..607895ea450f 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1144,6 +1144,8 @@ enum kvm_device_type {
>   #define KVM_DEV_TYPE_RISCV_AIA		KVM_DEV_TYPE_RISCV_AIA
>   	KVM_DEV_TYPE_LA_IPI,
>   #define KVM_DEV_TYPE_LA_IPI		KVM_DEV_TYPE_LA_IPI
> +	KVM_DEV_TYPE_LA_EXTIOI,
> +#define KVM_DEV_TYPE_LA_EXTIOI		KVM_DEV_TYPE_LA_EXTIOI
>   
>   	KVM_DEV_TYPE_MAX,
>