[PATCH v2 0/4] LoongArch: KVM: irqchip fixes

[PATCH v2 0/4] LoongArch: KVM: irqchip fixes

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

Hi,

Four fixes for the LoongArch KVM irqchip code:

- Patch 1 clears the device pointer in the destroy callbacks: when
  KVM_CREATE_DEVICE succeeds but the following fd allocation fails
  (e.g. under RLIMIT_NOFILE), ops->destroy() frees the irqchip while
  kvm->arch.* still points to it.

- Patch 2 loads kvm->arch.dmsintc once in the MSI injection path:
  pch_msi_set_irq() re-reads the pointer between the non-NULL check
  and the address-window comparison, so a concurrent device removal
  can be observed between them.

- Patch 3 aligns kvm_pch_pic_create() with kvm_eiointc_create() by
  propagating the real error code; the kvm_ipi_create() counterpart is
  being fixed separately (Chaithanya Lagisetty).

- Patch 4 rejects repeated PCH-PIC CTRL_INIT with -EEXIST, tracking
  the state with a has_init flag so the check and the MMIO base update
  are atomic under slots_lock.

All patches carry Fixes tags.

Changes in v2:

- Drop "Guard against NULL irqchip in irq injection" (v1 patch 2) and
  "Rebase steal time counter in vcpu context" (v1 patch 4) after
  review discussion: their trigger scenarios are not reachable
  through real VMM behaviour.

- Use the one-line destroy style in dmsintc suggested by Bibo.

- Track repeated PCH-PIC CTRL_INIT with a has_init flag and return
  -EEXIST instead of -EBUSY, also suggested by Bibo; move the check
  and base update under slots_lock so they are atomic.

- Add READ_ONCE() to the dmsintc pointer loads so the compiler
  keeps each of them a single load.

Tao Cui (4):
  LoongArch: KVM: Clear device pointer in irqchip destroy callbacks
  LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq
  LoongArch: KVM: Propagate real error code in kvm_pch_pic_create
  LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

 arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
 arch/loongarch/kvm/intc/dmsintc.c        |  7 ++++++-
 arch/loongarch/kvm/intc/eiointc.c        |  1 +
 arch/loongarch/kvm/intc/ipi.c            |  1 +
 arch/loongarch/kvm/intc/pch_pic.c        | 22 ++++++++++++++++------
 5 files changed, 25 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] LoongArch: KVM: Clear device pointer in irqchip destroy callbacks

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

The destroy callbacks of the four irqchip devices free the device
structure without clearing kvm->arch.{ipi,eiointc,pch_pic,dmsintc},
leaving a dangling pointer.  ops->destroy() is not only called at VM
teardown but also when KVM_CREATE_DEVICE succeeds and the following
anon_inode_getfd() fails (e.g. under RLIMIT_NOFILE); the VM stays
alive, kvm_arch_irqchip_in_kernel() still reports true, and interrupt
injection dereferences the freed device.

Clear the pointer when destroying.

Fixes: c532de5a67a7 ("LoongArch: KVM: Add IPI device support")
Fixes: 2e8b9df82631 ("LoongArch: KVM: Add EIOINTC device support")
Fixes: e785dfacf7e7 ("LoongArch: KVM: Add PCHPIC device support")
Fixes: 229132c309d6 ("LoongArch: KVM: Add DMSINTC device support")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 arch/loongarch/kvm/intc/dmsintc.c | 1 +
 arch/loongarch/kvm/intc/eiointc.c | 1 +
 arch/loongarch/kvm/intc/ipi.c     | 1 +
 arch/loongarch/kvm/intc/pch_pic.c | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
index 89f980d867be..41c8b597b5bd 100644
--- a/arch/loongarch/kvm/intc/dmsintc.c
+++ b/arch/loongarch/kvm/intc/dmsintc.c
@@ -166,6 +166,7 @@ static void kvm_dmsintc_destroy(struct kvm_device *dev)
 		return;
 
 	kfree(dev->kvm->arch.dmsintc);
+	dev->kvm->arch.dmsintc = NULL;
 	kfree(dev);
 }
 
diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
index 80f78e07c74a..fe0a1918f26f 100644
--- a/arch/loongarch/kvm/intc/eiointc.c
+++ b/arch/loongarch/kvm/intc/eiointc.c
@@ -675,6 +675,7 @@ static void kvm_eiointc_destroy(struct kvm_device *dev)
 
 	kvm = dev->kvm;
 	eiointc = kvm->arch.eiointc;
+	kvm->arch.eiointc = NULL;
 	mutex_lock(&kvm->slots_lock);
 	kvm_io_bus_unregister_dev(kvm, KVM_IOCSR_BUS, &eiointc->device);
 	kvm_io_bus_unregister_dev(kvm, KVM_IOCSR_BUS, &eiointc->device_vext);
diff --git a/arch/loongarch/kvm/intc/ipi.c b/arch/loongarch/kvm/intc/ipi.c
index 7b333a4a0430..6ee90c10827a 100644
--- a/arch/loongarch/kvm/intc/ipi.c
+++ b/arch/loongarch/kvm/intc/ipi.c
@@ -444,6 +444,7 @@ static void kvm_ipi_destroy(struct kvm_device *dev)
 
 	kvm = dev->kvm;
 	ipi = kvm->arch.ipi;
+	kvm->arch.ipi = NULL;
 	mutex_lock(&kvm->slots_lock);
 	kvm_io_bus_unregister_dev(kvm, KVM_IOCSR_BUS, &ipi->device);
 	mutex_unlock(&kvm->slots_lock);
diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
index 2b63b0c2c7ce..62c09b5f3937 100644
--- a/arch/loongarch/kvm/intc/pch_pic.c
+++ b/arch/loongarch/kvm/intc/pch_pic.c
@@ -483,6 +483,7 @@ static void kvm_pch_pic_destroy(struct kvm_device *dev)
 
 	kvm = dev->kvm;
 	s = kvm->arch.pch_pic;
+	kvm->arch.pch_pic = NULL;
 	/* unregister pch pic device and free it's memory */
 	mutex_lock(&kvm->slots_lock);
 	kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &s->device);
-- 
2.43.0

[PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

pch_msi_set_irq() reads kvm->arch.dmsintc several times: the non-NULL
check and the address-window comparison each reload the pointer, so a
concurrent device removal can be observed between them and the
following dereference hits a freed object.

Load the pointer once at the top of pch_msi_set_irq() and add a NULL
check with a local snapshot in dmsintc_set_irq().  Both loads use
READ_ONCE() so the compiler keeps them single.  This closes the
reload race; a narrower window where removal happens right after the
load remains, as the injection path takes no lock against destroy.

Fixes: 03de5eecb0f0 ("LoongArch: KVM: Add DMSINTC inject msi to vCPU")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 arch/loongarch/kvm/intc/dmsintc.c | 6 +++++-
 arch/loongarch/kvm/intc/pch_pic.c | 7 ++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
index 41c8b597b5bd..91a163698c3c 100644
--- a/arch/loongarch/kvm/intc/dmsintc.c
+++ b/arch/loongarch/kvm/intc/dmsintc.c
@@ -69,9 +69,13 @@ int dmsintc_set_irq(struct kvm *kvm, u64 addr, int data, int level)
 {
 	unsigned int irq, cpu;
 	struct kvm_vcpu *vcpu;
+	struct loongarch_dmsintc *s = READ_ONCE(kvm->arch.dmsintc);
+
+	if (!s)
+		return -EINVAL;
 
 	irq = (addr >> AVEC_IRQ_SHIFT) & AVEC_IRQ_MASK;
-	cpu = (addr >> AVEC_CPU_SHIFT) & kvm->arch.dmsintc->cpu_mask;
+	cpu = (addr >> AVEC_CPU_SHIFT) & s->cpu_mask;
 	if (cpu >= KVM_MAX_VCPUS)
 		return -EINVAL;
 	vcpu = kvm_get_vcpu_by_cpuid(kvm, cpu);
diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
index 62c09b5f3937..30a8c65c7609 100644
--- a/arch/loongarch/kvm/intc/pch_pic.c
+++ b/arch/loongarch/kvm/intc/pch_pic.c
@@ -71,10 +71,11 @@ void pch_pic_set_irq(struct loongarch_pch_pic *s, int irq, int level)
 int pch_msi_set_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, int level)
 {
 	u64 msg_addr = (((u64)e->msi.address_hi) << 32) | e->msi.address_lo;
+	struct loongarch_dmsintc *dmsintc = READ_ONCE(kvm->arch.dmsintc);
 
-	if (cpu_has_msgint && kvm->arch.dmsintc &&
-		msg_addr >= kvm->arch.dmsintc->msg_addr_base &&
-		msg_addr < (kvm->arch.dmsintc->msg_addr_base + kvm->arch.dmsintc->msg_addr_size)) {
+	if (cpu_has_msgint && dmsintc &&
+		msg_addr >= dmsintc->msg_addr_base &&
+		msg_addr < (dmsintc->msg_addr_base + dmsintc->msg_addr_size)) {
 		return dmsintc_set_irq(kvm, msg_addr, e->msi.data, level);
 	}
 
-- 
2.43.0

[PATCH v2 3/4] LoongArch: KVM: Propagate real error code in kvm_pch_pic_create

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

kvm_pch_pic_create() replaces the real error of
kvm_setup_default_irq_routing() with a fixed -ENOMEM.  Return the
actual code, as kvm_eiointc_create() already does; the kvm_ipi_create()
case is fixed separately by "Return the actual error code in
kvm_ipi_create()" (Chaithanya Lagisetty).

Fixes: e785dfacf7e7 ("LoongArch: KVM: Add PCHPIC device support")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
Reviewed-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/kvm/intc/pch_pic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
index 30a8c65c7609..7a704f18880d 100644
--- a/arch/loongarch/kvm/intc/pch_pic.c
+++ b/arch/loongarch/kvm/intc/pch_pic.c
@@ -445,7 +445,7 @@ static int kvm_pch_pic_create(struct kvm_device *dev, u32 type)
 
 	ret = kvm_setup_default_irq_routing(kvm);
 	if (ret)
-		return -ENOMEM;
+		return ret;
 
 	s = kzalloc_obj(struct loongarch_pch_pic);
 	if (!s)
-- 
2.43.0

[PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
invocation: every call overwrites pch_pic_base and registers the same
kvm_io_device on the MMIO bus at the new address, while
kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
init, MMIO to the stale ranges computes its register offset against the
new base and silently reads 0 / drops writes, and the leftover bus
entries persist until the VM is destroyed.

Reject repeated initialization with -EEXIST, tracking the state with
a has_init flag so the check and the MMIO base update are atomic
under slots_lock.  The base is only committed after a successful bus
registration, and the real registration error is propagated instead
of being replaced with -EFAULT.

Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
 arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
index 887b0431fd20..679132d840e6 100644
--- a/arch/loongarch/include/asm/kvm_pch_pic.h
+++ b/arch/loongarch/include/asm/kvm_pch_pic.h
@@ -53,6 +53,7 @@ struct loongarch_pch_pic {
 	spinlock_t lock;
 	struct kvm *kvm;
 	struct kvm_io_device device;
+	bool has_init;
 	union pch_pic_id id;
 	uint64_t mask; /* 1:disable irq, 0:enable irq */
 	uint64_t htmsi_en; /* 1:msi */
diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
index 7a704f18880d..a884a043feef 100644
--- a/arch/loongarch/kvm/intc/pch_pic.c
+++ b/arch/loongarch/kvm/intc/pch_pic.c
@@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
 	struct kvm_io_device *device;
 	struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
 
-	s->pch_pic_base = addr;
 	device = &s->device;
 	/* init device by pch pic writing and reading ops */
 	kvm_iodevice_init(device, &kvm_pch_pic_ops);
 	mutex_lock(&kvm->slots_lock);
+	if (s->has_init) {
+		ret = -EEXIST;
+		goto out;
+	}
 	/* register pch pic device */
 	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
+	if (!ret) {
+		s->pch_pic_base = addr;
+		s->has_init = true;
+	}
+out:
 	mutex_unlock(&kvm->slots_lock);
 
-	return (ret < 0) ? -EFAULT : 0;
+	return ret;
 }
 
 /* used by user space to get or set pch pic registers */
-- 
2.43.0

Re: [PATCH v2 1/4] LoongArch: KVM: Clear device pointer in irqchip destroy callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] kvm: loongarch: NULL deref and UAF via lockless irqchip destruction
--

commit 1d2bf4dc9e1262efde3462bede6b473d0a4a01e5
Author: Tao Cui <cuitao@kylinos.cn>

LoongArch: KVM: Clear device pointer in irqchip destroy callbacks

The commit clears device pointers in the destroy callbacks for the four
LoongArch irqchip devices to prevent dangling pointers. This addresses an
issue where anon_inode_getfd() failures during KVM_CREATE_DEVICE can
leave the VM alive with a freed irqchip device.

> diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> index bb7285c49df3a..4bd98b888e3f1 100644
> --- a/arch/loongarch/kvm/intc/dmsintc.c
> +++ b/arch/loongarch/kvm/intc/dmsintc.c
> @@ -166,6 +166,7 @@ static void kvm_dmsintc_destroy(struct kvm_device *dev)
>  		return;
>  
>  	kfree(dev->kvm->arch.dmsintc);
> +	dev->kvm->arch.dmsintc = NULL;

[Severity: Critical]
Does setting this pointer to NULL after freeing it leave a window open for
a use-after-free?

If anon_inode_getfd() fails during KVM_CREATE_DEVICE, the error path
calls ops->destroy() locklessly. At the same time, concurrent KVM_IRQ_LINE
requests can locklessly invoke kvm_set_irq(), which drops kvm->irq_srcu
before calling the routing callbacks.

If those callbacks read the pointer between the kfree() and this NULL
assignment, won't they dereference freed memory? Does this ordering defeat
any READ_ONCE() fixes intended for concurrent lockless readers?

[ ... ]

> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> index e7b77705c516a..5d046a1cab4e2 100644
> --- a/arch/loongarch/kvm/intc/pch_pic.c
> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> @@ -480,6 +480,7 @@ static void kvm_pch_pic_destroy(struct kvm_device *dev)
>  
>  	kvm = dev->kvm;
>  	s = kvm->arch.pch_pic;
> +	kvm->arch.pch_pic = NULL;

[Severity: Critical]
Can assigning NULL here without synchronization lead to a deterministic NULL
pointer dereference and host panic?

If kvm->arch.pch_pic is set to NULL locklessly here, and a concurrent
KVM_IRQ_LINE ioctl calls into kvm_set_pic_irq(), the routing table
execution happens after kvm_set_irq() drops irq_srcu:

virt/kvm/irqchip.c:kvm_set_irq() {
    ...
    idx = srcu_read_lock(&kvm->irq_srcu);
    i = kvm_irq_map_gsi(kvm, irq_set, irq);
    srcu_read_unlock(&kvm->irq_srcu, idx);

    while (i--) {
        int r;
        r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level,
                           line_status);
    ...
}

The callback kvm_set_pic_irq() then blindly passes the newly-NULL pointer
to pch_pic_set_irq() without checking it:

arch/loongarch/kvm/irqfd.c:kvm_set_pic_irq() {
    ...
    pch_pic_set_irq(kvm->arch.pch_pic, e->irqchip.pin, level);
    ...
}

Since pch_pic_set_irq() unconditionally dereferences the pointer:

arch/loongarch/kvm/intc/pch_pic.c:pch_pic_set_irq() {
    ...
    spin_lock(&s->lock);
    ...
}

Doesn't this mean an unprivileged userspace process exhausting file
descriptors and concurrently injecting interrupts will trigger a host panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929102821.36112-1-cui.tao@linux.dev?part=1

Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Huacai Chen]

Hi, Tao,

On Tue, Sep 29, 2026 at 6:29 PM Tao Cui <cui.tao@linux.dev> wrote:
>
> From: Tao Cui <cuitao@kylinos.cn>
>
> KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
> invocation: every call overwrites pch_pic_base and registers the same
> kvm_io_device on the MMIO bus at the new address, while
> kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
> init, MMIO to the stale ranges computes its register offset against the
> new base and silently reads 0 / drops writes, and the leftover bus
> entries persist until the VM is destroyed.
>
> Reject repeated initialization with -EEXIST, tracking the state with
> a has_init flag so the check and the MMIO base update are atomic
> under slots_lock.  The base is only committed after a successful bus
> registration, and the real registration error is propagated instead
> of being replaced with -EFAULT.
>
> Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> ---
>  arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
>  arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
> index 887b0431fd20..679132d840e6 100644
> --- a/arch/loongarch/include/asm/kvm_pch_pic.h
> +++ b/arch/loongarch/include/asm/kvm_pch_pic.h
> @@ -53,6 +53,7 @@ struct loongarch_pch_pic {
>         spinlock_t lock;
>         struct kvm *kvm;
>         struct kvm_io_device device;
> +       bool has_init;
>         union pch_pic_id id;
>         uint64_t mask; /* 1:disable irq, 0:enable irq */
>         uint64_t htmsi_en; /* 1:msi */
> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> index 7a704f18880d..a884a043feef 100644
> --- a/arch/loongarch/kvm/intc/pch_pic.c
> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> @@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
>         struct kvm_io_device *device;
>         struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
Why so complicated? The below is enough, no?

diff --git a/arch/loongarch/kvm/intc/pch_pic.c
b/arch/loongarch/kvm/intc/pch_pic.c
index 2b63b0c2c7ce..7855d78304b7 100644
--- a/arch/loongarch/kvm/intc/pch_pic.c
+++ b/arch/loongarch/kvm/intc/pch_pic.c
@@ -281,6 +281,9 @@ static int kvm_pch_pic_init(struct kvm_device
*dev, u64 addr)
        struct kvm_io_device *device;
        struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;

+       if (s->device->ops)
+               return -EEXIST;
+
        s->pch_pic_base = addr;
        device = &s->device;
        /* init device by pch pic writing and reading ops */

>
> -       s->pch_pic_base = addr;
>         device = &s->device;
>         /* init device by pch pic writing and reading ops */
>         kvm_iodevice_init(device, &kvm_pch_pic_ops);
>         mutex_lock(&kvm->slots_lock);
> +       if (s->has_init) {
> +               ret = -EEXIST;
> +               goto out;
> +       }
>         /* register pch pic device */
>         ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
> +       if (!ret) {
> +               s->pch_pic_base = addr;
> +               s->has_init = true;
> +       }
> +out:
>         mutex_unlock(&kvm->slots_lock);
>
> -       return (ret < 0) ? -EFAULT : 0;
> +       return ret;
>  }
>
>  /* used by user space to get or set pch pic registers */
> --
> 2.43.0
>

Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Tao Cui]

Hi, Huacai.

在 2026/9/29 20:43, Huacai Chen 写道:
> Hi, Tao,
> 
> On Tue, Sep 29, 2026 at 6:29 PM Tao Cui <cui.tao@linux.dev> wrote:
>>
>> From: Tao Cui <cuitao@kylinos.cn>
>>
>> KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
>> invocation: every call overwrites pch_pic_base and registers the same
>> kvm_io_device on the MMIO bus at the new address, while
>> kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
>> init, MMIO to the stale ranges computes its register offset against the
>> new base and silently reads 0 / drops writes, and the leftover bus
>> entries persist until the VM is destroyed.
>>
>> Reject repeated initialization with -EEXIST, tracking the state with
>> a has_init flag so the check and the MMIO base update are atomic
>> under slots_lock.  The base is only committed after a successful bus
>> registration, and the real registration error is propagated instead
>> of being replaced with -EFAULT.
>>
>> Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>> ---
>>  arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
>>  arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
>> index 887b0431fd20..679132d840e6 100644
>> --- a/arch/loongarch/include/asm/kvm_pch_pic.h
>> +++ b/arch/loongarch/include/asm/kvm_pch_pic.h
>> @@ -53,6 +53,7 @@ struct loongarch_pch_pic {
>>         spinlock_t lock;
>>         struct kvm *kvm;
>>         struct kvm_io_device device;
>> +       bool has_init;
>>         union pch_pic_id id;
>>         uint64_t mask; /* 1:disable irq, 0:enable irq */
>>         uint64_t htmsi_en; /* 1:msi */
>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
>> index 7a704f18880d..a884a043feef 100644
>> --- a/arch/loongarch/kvm/intc/pch_pic.c
>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
>> @@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
>>         struct kvm_io_device *device;
>>         struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> Why so complicated? The below is enough, no?
> 
Thanks for the suggestion.

I agree that a separate has_init flag can be unnecessary here. v3 uses
s->device.ops to track whether the device has already been initialized.

The other changes are kept: pch_pic_base is updated only after
successful bus registration, and the original registration error is
returned instead of -EFAULT.

Thanks,
Tao

> diff --git a/arch/loongarch/kvm/intc/pch_pic.c
> b/arch/loongarch/kvm/intc/pch_pic.c
> index 2b63b0c2c7ce..7855d78304b7 100644
> --- a/arch/loongarch/kvm/intc/pch_pic.c
> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> @@ -281,6 +281,9 @@ static int kvm_pch_pic_init(struct kvm_device
> *dev, u64 addr)
>         struct kvm_io_device *device;
>         struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> 
> +       if (s->device->ops)
> +               return -EEXIST;
> +
>         s->pch_pic_base = addr;
>         device = &s->device;
>         /* init device by pch pic writing and reading ops */
> 
>>
>> -       s->pch_pic_base = addr;
>>         device = &s->device;
>>         /* init device by pch pic writing and reading ops */
>>         kvm_iodevice_init(device, &kvm_pch_pic_ops);
>>         mutex_lock(&kvm->slots_lock);
>> +       if (s->has_init) {
>> +               ret = -EEXIST;
>> +               goto out;
>> +       }
>>         /* register pch pic device */
>>         ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
>> +       if (!ret) {
>> +               s->pch_pic_base = addr;
>> +               s->has_init = true;
>> +       }
>> +out:
>>         mutex_unlock(&kvm->slots_lock);
>>
>> -       return (ret < 0) ? -EFAULT : 0;
>> +       return ret;
>>  }
>>
>>  /* used by user space to get or set pch pic registers */
>> --
>> 2.43.0
>>

Re: [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq

[Bibo Mao]

On 2026/9/29 下午6:28, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> pch_msi_set_irq() reads kvm->arch.dmsintc several times: the non-NULL
> check and the address-window comparison each reload the pointer, so a
> concurrent device removal can be observed between them and the
> following dereference hits a freed object.
> 
> Load the pointer once at the top of pch_msi_set_irq() and add a NULL
> check with a local snapshot in dmsintc_set_irq().  Both loads use
> READ_ONCE() so the compiler keeps them single.  This closes the
> reload race; a narrower window where removal happens right after the
> load remains, as the injection path takes no lock against destroy.
> 
> Fixes: 03de5eecb0f0 ("LoongArch: KVM: Add DMSINTC inject msi to vCPU")
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> ---
>   arch/loongarch/kvm/intc/dmsintc.c | 6 +++++-
>   arch/loongarch/kvm/intc/pch_pic.c | 7 ++++---
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> index 41c8b597b5bd..91a163698c3c 100644
> --- a/arch/loongarch/kvm/intc/dmsintc.c
> +++ b/arch/loongarch/kvm/intc/dmsintc.c
> @@ -69,9 +69,13 @@ int dmsintc_set_irq(struct kvm *kvm, u64 addr, int data, int level)
>   {
>   	unsigned int irq, cpu;
>   	struct kvm_vcpu *vcpu;
> +	struct loongarch_dmsintc *s = READ_ONCE(kvm->arch.dmsintc);
> +
> +	if (!s)
> +		return -EINVAL;
NULL check with kvm->arch.dmsintc is already done in its caller function 
pch_msi_set_irq(). It is not necessary here.
>   
>   	irq = (addr >> AVEC_IRQ_SHIFT) & AVEC_IRQ_MASK;
> -	cpu = (addr >> AVEC_CPU_SHIFT) & kvm->arch.dmsintc->cpu_mask;
> +	cpu = (addr >> AVEC_CPU_SHIFT) & s->cpu_mask;
>   	if (cpu >= KVM_MAX_VCPUS)
>   		return -EINVAL;
>   	vcpu = kvm_get_vcpu_by_cpuid(kvm, cpu);
> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> index 62c09b5f3937..30a8c65c7609 100644
> --- a/arch/loongarch/kvm/intc/pch_pic.c
> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> @@ -71,10 +71,11 @@ void pch_pic_set_irq(struct loongarch_pch_pic *s, int irq, int level)
>   int pch_msi_set_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, int level)
>   {
>   	u64 msg_addr = (((u64)e->msi.address_hi) << 32) | e->msi.address_lo;
> +	struct loongarch_dmsintc *dmsintc = READ_ONCE(kvm->arch.dmsintc);
what is usage of READ_ONCE() here?

If you want to simple the usage of kvm->arch.dmsintc in multiple places, 
just *struct loongarch_dmsintc *dmsintc = kvm->arch.dmsintc* is enough. 
And it is not fixup patch, it is code cleanup.

Regards
Bibo Mao
>   
> -	if (cpu_has_msgint && kvm->arch.dmsintc &&
> -		msg_addr >= kvm->arch.dmsintc->msg_addr_base &&
> -		msg_addr < (kvm->arch.dmsintc->msg_addr_base + kvm->arch.dmsintc->msg_addr_size)) {
> +	if (cpu_has_msgint && dmsintc &&
> +		msg_addr >= dmsintc->msg_addr_base &&
> +		msg_addr < (dmsintc->msg_addr_base + dmsintc->msg_addr_size)) {
>   		return dmsintc_set_irq(kvm, msg_addr, e->msi.data, level);
>   	}
>   
> 

Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Bibo Mao]

On 2026/9/29 下午8:43, Huacai Chen wrote:
> Hi, Tao,
> 
> On Tue, Sep 29, 2026 at 6:29 PM Tao Cui <cui.tao@linux.dev> wrote:
>>
>> From: Tao Cui <cuitao@kylinos.cn>
>>
>> KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
>> invocation: every call overwrites pch_pic_base and registers the same
>> kvm_io_device on the MMIO bus at the new address, while
>> kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
>> init, MMIO to the stale ranges computes its register offset against the
>> new base and silently reads 0 / drops writes, and the leftover bus
>> entries persist until the VM is destroyed.
>>
>> Reject repeated initialization with -EEXIST, tracking the state with
>> a has_init flag so the check and the MMIO base update are atomic
>> under slots_lock.  The base is only committed after a successful bus
>> registration, and the real registration error is propagated instead
>> of being replaced with -EFAULT.
>>
>> Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>> ---
>>   arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
>>   arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
>> index 887b0431fd20..679132d840e6 100644
>> --- a/arch/loongarch/include/asm/kvm_pch_pic.h
>> +++ b/arch/loongarch/include/asm/kvm_pch_pic.h
>> @@ -53,6 +53,7 @@ struct loongarch_pch_pic {
>>          spinlock_t lock;
>>          struct kvm *kvm;
>>          struct kvm_io_device device;
>> +       bool has_init;
>>          union pch_pic_id id;
>>          uint64_t mask; /* 1:disable irq, 0:enable irq */
>>          uint64_t htmsi_en; /* 1:msi */
>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
>> index 7a704f18880d..a884a043feef 100644
>> --- a/arch/loongarch/kvm/intc/pch_pic.c
>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
>> @@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
>>          struct kvm_io_device *device;
>>          struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> Why so complicated? The below is enough, no?
> 
> diff --git a/arch/loongarch/kvm/intc/pch_pic.c
> b/arch/loongarch/kvm/intc/pch_pic.c
> index 2b63b0c2c7ce..7855d78304b7 100644
> --- a/arch/loongarch/kvm/intc/pch_pic.c
> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> @@ -281,6 +281,9 @@ static int kvm_pch_pic_init(struct kvm_device
> *dev, u64 addr)
>          struct kvm_io_device *device;
>          struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> 
> +       if (s->device->ops)
> +               return -EEXIST;
This can work, however I think that it is not a good idea to access 
internal structure field about kvm_io_device. If so, there is no use 
about API kvm_iodevice_init(), just s->device->ops = &kvm_pch_pic_ops is ok.

If adding has_init is redundant, maybe we can set s->pch_pic_base with 
INVALID_GPA in kvm_pch_pic_create() or some other methods. However I 
think directly accessing kvm_io_device::ops is not a good method, no 
other architectures do in such way.

Regards
Bibo Mao
> +
>          s->pch_pic_base = addr;
>          device = &s->device;
>          /* init device by pch pic writing and reading ops */
> 
>>
>> -       s->pch_pic_base = addr;
>>          device = &s->device;
>>          /* init device by pch pic writing and reading ops */
>>          kvm_iodevice_init(device, &kvm_pch_pic_ops);
>>          mutex_lock(&kvm->slots_lock);
>> +       if (s->has_init) {
>> +               ret = -EEXIST;
>> +               goto out;
>> +       }
>>          /* register pch pic device */
>>          ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
>> +       if (!ret) {
>> +               s->pch_pic_base = addr;
>> +               s->has_init = true;
>> +       }
>> +out:
>>          mutex_unlock(&kvm->slots_lock);
>>
>> -       return (ret < 0) ? -EFAULT : 0;
>> +       return ret;
>>   }
>>
>>   /* used by user space to get or set pch pic registers */
>> --
>> 2.43.0
>>

Re: [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq

[Huacai Chen]

On Wed, Sep 30, 2026 at 9:41 AM Bibo Mao <maobibo@loongson.cn> wrote:
>
>
>
> On 2026/9/29 下午6:28, Tao Cui wrote:
> > From: Tao Cui <cuitao@kylinos.cn>
> >
> > pch_msi_set_irq() reads kvm->arch.dmsintc several times: the non-NULL
> > check and the address-window comparison each reload the pointer, so a
> > concurrent device removal can be observed between them and the
> > following dereference hits a freed object.
> >
> > Load the pointer once at the top of pch_msi_set_irq() and add a NULL
> > check with a local snapshot in dmsintc_set_irq().  Both loads use
> > READ_ONCE() so the compiler keeps them single.  This closes the
> > reload race; a narrower window where removal happens right after the
> > load remains, as the injection path takes no lock against destroy.
> >
> > Fixes: 03de5eecb0f0 ("LoongArch: KVM: Add DMSINTC inject msi to vCPU")
> > Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> > ---
> >   arch/loongarch/kvm/intc/dmsintc.c | 6 +++++-
> >   arch/loongarch/kvm/intc/pch_pic.c | 7 ++++---
> >   2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
> > index 41c8b597b5bd..91a163698c3c 100644
> > --- a/arch/loongarch/kvm/intc/dmsintc.c
> > +++ b/arch/loongarch/kvm/intc/dmsintc.c
> > @@ -69,9 +69,13 @@ int dmsintc_set_irq(struct kvm *kvm, u64 addr, int data, int level)
> >   {
> >       unsigned int irq, cpu;
> >       struct kvm_vcpu *vcpu;
> > +     struct loongarch_dmsintc *s = READ_ONCE(kvm->arch.dmsintc);
> > +
> > +     if (!s)
> > +             return -EINVAL;
> NULL check with kvm->arch.dmsintc is already done in its caller function
> pch_msi_set_irq(). It is not necessary here.
> >
> >       irq = (addr >> AVEC_IRQ_SHIFT) & AVEC_IRQ_MASK;
> > -     cpu = (addr >> AVEC_CPU_SHIFT) & kvm->arch.dmsintc->cpu_mask;
> > +     cpu = (addr >> AVEC_CPU_SHIFT) & s->cpu_mask;
> >       if (cpu >= KVM_MAX_VCPUS)
> >               return -EINVAL;
> >       vcpu = kvm_get_vcpu_by_cpuid(kvm, cpu);
> > diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> > index 62c09b5f3937..30a8c65c7609 100644
> > --- a/arch/loongarch/kvm/intc/pch_pic.c
> > +++ b/arch/loongarch/kvm/intc/pch_pic.c
> > @@ -71,10 +71,11 @@ void pch_pic_set_irq(struct loongarch_pch_pic *s, int irq, int level)
> >   int pch_msi_set_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, int level)
> >   {
> >       u64 msg_addr = (((u64)e->msi.address_hi) << 32) | e->msi.address_lo;
> > +     struct loongarch_dmsintc *dmsintc = READ_ONCE(kvm->arch.dmsintc);
> what is usage of READ_ONCE() here?
>
> If you want to simple the usage of kvm->arch.dmsintc in multiple places,
> just *struct loongarch_dmsintc *dmsintc = kvm->arch.dmsintc* is enough.
> And it is not fixup patch, it is code cleanup.
I completely don't think this patch is necessary.

Huacai

>
> Regards
> Bibo Mao
> >
> > -     if (cpu_has_msgint && kvm->arch.dmsintc &&
> > -             msg_addr >= kvm->arch.dmsintc->msg_addr_base &&
> > -             msg_addr < (kvm->arch.dmsintc->msg_addr_base + kvm->arch.dmsintc->msg_addr_size)) {
> > +     if (cpu_has_msgint && dmsintc &&
> > +             msg_addr >= dmsintc->msg_addr_base &&
> > +             msg_addr < (dmsintc->msg_addr_base + dmsintc->msg_addr_size)) {
> >               return dmsintc_set_irq(kvm, msg_addr, e->msi.data, level);
> >       }
> >
> >
>

Re: [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq

[Bibo Mao]

On 2026/9/30 上午10:08, Huacai Chen wrote:
> On Wed, Sep 30, 2026 at 9:41 AM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2026/9/29 下午6:28, Tao Cui wrote:
>>> From: Tao Cui <cuitao@kylinos.cn>
>>>
>>> pch_msi_set_irq() reads kvm->arch.dmsintc several times: the non-NULL
>>> check and the address-window comparison each reload the pointer, so a
>>> concurrent device removal can be observed between them and the
>>> following dereference hits a freed object.
>>>
>>> Load the pointer once at the top of pch_msi_set_irq() and add a NULL
>>> check with a local snapshot in dmsintc_set_irq().  Both loads use
>>> READ_ONCE() so the compiler keeps them single.  This closes the
>>> reload race; a narrower window where removal happens right after the
>>> load remains, as the injection path takes no lock against destroy.
>>>
>>> Fixes: 03de5eecb0f0 ("LoongArch: KVM: Add DMSINTC inject msi to vCPU")
>>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>>> ---
>>>    arch/loongarch/kvm/intc/dmsintc.c | 6 +++++-
>>>    arch/loongarch/kvm/intc/pch_pic.c | 7 ++++---
>>>    2 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/loongarch/kvm/intc/dmsintc.c b/arch/loongarch/kvm/intc/dmsintc.c
>>> index 41c8b597b5bd..91a163698c3c 100644
>>> --- a/arch/loongarch/kvm/intc/dmsintc.c
>>> +++ b/arch/loongarch/kvm/intc/dmsintc.c
>>> @@ -69,9 +69,13 @@ int dmsintc_set_irq(struct kvm *kvm, u64 addr, int data, int level)
>>>    {
>>>        unsigned int irq, cpu;
>>>        struct kvm_vcpu *vcpu;
>>> +     struct loongarch_dmsintc *s = READ_ONCE(kvm->arch.dmsintc);
>>> +
>>> +     if (!s)
>>> +             return -EINVAL;
>> NULL check with kvm->arch.dmsintc is already done in its caller function
>> pch_msi_set_irq(). It is not necessary here.
>>>
>>>        irq = (addr >> AVEC_IRQ_SHIFT) & AVEC_IRQ_MASK;
>>> -     cpu = (addr >> AVEC_CPU_SHIFT) & kvm->arch.dmsintc->cpu_mask;
>>> +     cpu = (addr >> AVEC_CPU_SHIFT) & s->cpu_mask;
>>>        if (cpu >= KVM_MAX_VCPUS)
>>>                return -EINVAL;
>>>        vcpu = kvm_get_vcpu_by_cpuid(kvm, cpu);
>>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
>>> index 62c09b5f3937..30a8c65c7609 100644
>>> --- a/arch/loongarch/kvm/intc/pch_pic.c
>>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
>>> @@ -71,10 +71,11 @@ void pch_pic_set_irq(struct loongarch_pch_pic *s, int irq, int level)
>>>    int pch_msi_set_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, int level)
>>>    {
>>>        u64 msg_addr = (((u64)e->msi.address_hi) << 32) | e->msi.address_lo;
>>> +     struct loongarch_dmsintc *dmsintc = READ_ONCE(kvm->arch.dmsintc);
>> what is usage of READ_ONCE() here?
>>
>> If you want to simple the usage of kvm->arch.dmsintc in multiple places,
>> just *struct loongarch_dmsintc *dmsintc = kvm->arch.dmsintc* is enough.
>> And it is not fixup patch, it is code cleanup.
> I completely don't think this patch is necessary.
yeap, I have the same feeling about this :)

> 
> Huacai
> 
>>
>> Regards
>> Bibo Mao
>>>
>>> -     if (cpu_has_msgint && kvm->arch.dmsintc &&
>>> -             msg_addr >= kvm->arch.dmsintc->msg_addr_base &&
>>> -             msg_addr < (kvm->arch.dmsintc->msg_addr_base + kvm->arch.dmsintc->msg_addr_size)) {
>>> +     if (cpu_has_msgint && dmsintc &&
>>> +             msg_addr >= dmsintc->msg_addr_base &&
>>> +             msg_addr < (dmsintc->msg_addr_base + dmsintc->msg_addr_size)) {
>>>                return dmsintc_set_irq(kvm, msg_addr, e->msi.data, level);
>>>        }
>>>
>>>
>>

Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Huacai Chen]

On Wed, Sep 30, 2026 at 9:53 AM Bibo Mao <maobibo@loongson.cn> wrote:
>
>
>
> On 2026/9/29 下午8:43, Huacai Chen wrote:
> > Hi, Tao,
> >
> > On Tue, Sep 29, 2026 at 6:29 PM Tao Cui <cui.tao@linux.dev> wrote:
> >>
> >> From: Tao Cui <cuitao@kylinos.cn>
> >>
> >> KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
> >> invocation: every call overwrites pch_pic_base and registers the same
> >> kvm_io_device on the MMIO bus at the new address, while
> >> kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
> >> init, MMIO to the stale ranges computes its register offset against the
> >> new base and silently reads 0 / drops writes, and the leftover bus
> >> entries persist until the VM is destroyed.
> >>
> >> Reject repeated initialization with -EEXIST, tracking the state with
> >> a has_init flag so the check and the MMIO base update are atomic
> >> under slots_lock.  The base is only committed after a successful bus
> >> registration, and the real registration error is propagated instead
> >> of being replaced with -EFAULT.
> >>
> >> Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
> >> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> >> ---
> >>   arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
> >>   arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
> >>   2 files changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
> >> index 887b0431fd20..679132d840e6 100644
> >> --- a/arch/loongarch/include/asm/kvm_pch_pic.h
> >> +++ b/arch/loongarch/include/asm/kvm_pch_pic.h
> >> @@ -53,6 +53,7 @@ struct loongarch_pch_pic {
> >>          spinlock_t lock;
> >>          struct kvm *kvm;
> >>          struct kvm_io_device device;
> >> +       bool has_init;
> >>          union pch_pic_id id;
> >>          uint64_t mask; /* 1:disable irq, 0:enable irq */
> >>          uint64_t htmsi_en; /* 1:msi */
> >> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> >> index 7a704f18880d..a884a043feef 100644
> >> --- a/arch/loongarch/kvm/intc/pch_pic.c
> >> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> >> @@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
> >>          struct kvm_io_device *device;
> >>          struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> > Why so complicated? The below is enough, no?
> >
> > diff --git a/arch/loongarch/kvm/intc/pch_pic.c
> > b/arch/loongarch/kvm/intc/pch_pic.c
> > index 2b63b0c2c7ce..7855d78304b7 100644
> > --- a/arch/loongarch/kvm/intc/pch_pic.c
> > +++ b/arch/loongarch/kvm/intc/pch_pic.c
> > @@ -281,6 +281,9 @@ static int kvm_pch_pic_init(struct kvm_device
> > *dev, u64 addr)
> >          struct kvm_io_device *device;
> >          struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> >
> > +       if (s->device->ops)
> > +               return -EEXIST;
> This can work, however I think that it is not a good idea to access
> internal structure field about kvm_io_device. If so, there is no use
> about API kvm_iodevice_init(), just s->device->ops = &kvm_pch_pic_ops is ok.
I'm a little not agree. :)

I think kvm_iodevice_init() is designed to do more work rather than
just set the ops (though it just set the ops now), otherwise its name
should be kvm_iodevice_set_ops().

In addition, even if kvm_iodevice_init() is really a setter, there is
no getter for the ops, so when we need to access ops, we can only
open-code it.


Huacai

>
> If adding has_init is redundant, maybe we can set s->pch_pic_base with
> INVALID_GPA in kvm_pch_pic_create() or some other methods. However I
> think directly accessing kvm_io_device::ops is not a good method, no
> other architectures do in such way.
>
> Regards
> Bibo Mao
> > +
> >          s->pch_pic_base = addr;
> >          device = &s->device;
> >          /* init device by pch pic writing and reading ops */
> >
> >>
> >> -       s->pch_pic_base = addr;
> >>          device = &s->device;
> >>          /* init device by pch pic writing and reading ops */
> >>          kvm_iodevice_init(device, &kvm_pch_pic_ops);
> >>          mutex_lock(&kvm->slots_lock);
> >> +       if (s->has_init) {
> >> +               ret = -EEXIST;
> >> +               goto out;
> >> +       }
> >>          /* register pch pic device */
> >>          ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
> >> +       if (!ret) {
> >> +               s->pch_pic_base = addr;
> >> +               s->has_init = true;
> >> +       }
> >> +out:
> >>          mutex_unlock(&kvm->slots_lock);
> >>
> >> -       return (ret < 0) ? -EFAULT : 0;
> >> +       return ret;
> >>   }
> >>
> >>   /* used by user space to get or set pch pic registers */
> >> --
> >> 2.43.0
> >>
>

Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Huacai Chen]

On Wed, Sep 30, 2026 at 9:15 AM Tao Cui <cui.tao@linux.dev> wrote:
>
> Hi, Huacai.
>
> 在 2026/9/29 20:43, Huacai Chen 写道:
> > Hi, Tao,
> >
> > On Tue, Sep 29, 2026 at 6:29 PM Tao Cui <cui.tao@linux.dev> wrote:
> >>
> >> From: Tao Cui <cuitao@kylinos.cn>
> >>
> >> KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
> >> invocation: every call overwrites pch_pic_base and registers the same
> >> kvm_io_device on the MMIO bus at the new address, while
> >> kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
> >> init, MMIO to the stale ranges computes its register offset against the
> >> new base and silently reads 0 / drops writes, and the leftover bus
> >> entries persist until the VM is destroyed.
> >>
> >> Reject repeated initialization with -EEXIST, tracking the state with
> >> a has_init flag so the check and the MMIO base update are atomic
> >> under slots_lock.  The base is only committed after a successful bus
> >> registration, and the real registration error is propagated instead
> >> of being replaced with -EFAULT.
> >>
> >> Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
> >> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> >> ---
> >>  arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
> >>  arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
> >>  2 files changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
> >> index 887b0431fd20..679132d840e6 100644
> >> --- a/arch/loongarch/include/asm/kvm_pch_pic.h
> >> +++ b/arch/loongarch/include/asm/kvm_pch_pic.h
> >> @@ -53,6 +53,7 @@ struct loongarch_pch_pic {
> >>         spinlock_t lock;
> >>         struct kvm *kvm;
> >>         struct kvm_io_device device;
> >> +       bool has_init;
> >>         union pch_pic_id id;
> >>         uint64_t mask; /* 1:disable irq, 0:enable irq */
> >>         uint64_t htmsi_en; /* 1:msi */
> >> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> >> index 7a704f18880d..a884a043feef 100644
> >> --- a/arch/loongarch/kvm/intc/pch_pic.c
> >> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> >> @@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
> >>         struct kvm_io_device *device;
> >>         struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> > Why so complicated? The below is enough, no?
> >
> Thanks for the suggestion.
>
> I agree that a separate has_init flag can be unnecessary here. v3 uses
> s->device.ops to track whether the device has already been initialized.
>
> The other changes are kept: pch_pic_base is updated only after
> successful bus registration, and the original registration error is
> returned instead of -EFAULT.
The return value changes can be kept, but why keep the pch_pic_base updating?


Huacai

>
> Thanks,
> Tao
>
> > diff --git a/arch/loongarch/kvm/intc/pch_pic.c
> > b/arch/loongarch/kvm/intc/pch_pic.c
> > index 2b63b0c2c7ce..7855d78304b7 100644
> > --- a/arch/loongarch/kvm/intc/pch_pic.c
> > +++ b/arch/loongarch/kvm/intc/pch_pic.c
> > @@ -281,6 +281,9 @@ static int kvm_pch_pic_init(struct kvm_device
> > *dev, u64 addr)
> >         struct kvm_io_device *device;
> >         struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> >
> > +       if (s->device->ops)
> > +               return -EEXIST;
> > +
> >         s->pch_pic_base = addr;
> >         device = &s->device;
> >         /* init device by pch pic writing and reading ops */
> >
> >>
> >> -       s->pch_pic_base = addr;
> >>         device = &s->device;
> >>         /* init device by pch pic writing and reading ops */
> >>         kvm_iodevice_init(device, &kvm_pch_pic_ops);
> >>         mutex_lock(&kvm->slots_lock);
> >> +       if (s->has_init) {
> >> +               ret = -EEXIST;
> >> +               goto out;
> >> +       }
> >>         /* register pch pic device */
> >>         ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
> >> +       if (!ret) {
> >> +               s->pch_pic_base = addr;
> >> +               s->has_init = true;
> >> +       }
> >> +out:
> >>         mutex_unlock(&kvm->slots_lock);
> >>
> >> -       return (ret < 0) ? -EFAULT : 0;
> >> +       return ret;
> >>  }
> >>
> >>  /* used by user space to get or set pch pic registers */
> >> --
> >> 2.43.0
> >>
>

Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Bibo Mao]

On 2026/9/30 上午10:24, Huacai Chen wrote:
> On Wed, Sep 30, 2026 at 9:53 AM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2026/9/29 下午8:43, Huacai Chen wrote:
>>> Hi, Tao,
>>>
>>> On Tue, Sep 29, 2026 at 6:29 PM Tao Cui <cui.tao@linux.dev> wrote:
>>>>
>>>> From: Tao Cui <cuitao@kylinos.cn>
>>>>
>>>> KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
>>>> invocation: every call overwrites pch_pic_base and registers the same
>>>> kvm_io_device on the MMIO bus at the new address, while
>>>> kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
>>>> init, MMIO to the stale ranges computes its register offset against the
>>>> new base and silently reads 0 / drops writes, and the leftover bus
>>>> entries persist until the VM is destroyed.
>>>>
>>>> Reject repeated initialization with -EEXIST, tracking the state with
>>>> a has_init flag so the check and the MMIO base update are atomic
>>>> under slots_lock.  The base is only committed after a successful bus
>>>> registration, and the real registration error is propagated instead
>>>> of being replaced with -EFAULT.
>>>>
>>>> Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
>>>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>>>> ---
>>>>    arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
>>>>    arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
>>>>    2 files changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
>>>> index 887b0431fd20..679132d840e6 100644
>>>> --- a/arch/loongarch/include/asm/kvm_pch_pic.h
>>>> +++ b/arch/loongarch/include/asm/kvm_pch_pic.h
>>>> @@ -53,6 +53,7 @@ struct loongarch_pch_pic {
>>>>           spinlock_t lock;
>>>>           struct kvm *kvm;
>>>>           struct kvm_io_device device;
>>>> +       bool has_init;
>>>>           union pch_pic_id id;
>>>>           uint64_t mask; /* 1:disable irq, 0:enable irq */
>>>>           uint64_t htmsi_en; /* 1:msi */
>>>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
>>>> index 7a704f18880d..a884a043feef 100644
>>>> --- a/arch/loongarch/kvm/intc/pch_pic.c
>>>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
>>>> @@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
>>>>           struct kvm_io_device *device;
>>>>           struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
>>> Why so complicated? The below is enough, no?
>>>
>>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c
>>> b/arch/loongarch/kvm/intc/pch_pic.c
>>> index 2b63b0c2c7ce..7855d78304b7 100644
>>> --- a/arch/loongarch/kvm/intc/pch_pic.c
>>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
>>> @@ -281,6 +281,9 @@ static int kvm_pch_pic_init(struct kvm_device
>>> *dev, u64 addr)
>>>           struct kvm_io_device *device;
>>>           struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
>>>
>>> +       if (s->device->ops)
>>> +               return -EEXIST;
>> This can work, however I think that it is not a good idea to access
>> internal structure field about kvm_io_device. If so, there is no use
>> about API kvm_iodevice_init(), just s->device->ops = &kvm_pch_pic_ops is ok.
> I'm a little not agree. :)
> 
> I think kvm_iodevice_init() is designed to do more work rather than
> just set the ops (though it just set the ops now), otherwise its name
> should be kvm_iodevice_set_ops().
> 
> In addition, even if kvm_iodevice_init() is really a setter, there is
> no getter for the ops, so when we need to access ops, we can only
> open-code it.
if so, you can try to add kvm_iodevice_get_ops API and check the 
response of KVM community.

Regards
Bibo Mao
> 
> 
> Huacai
> 
>>
>> If adding has_init is redundant, maybe we can set s->pch_pic_base with
>> INVALID_GPA in kvm_pch_pic_create() or some other methods. However I
>> think directly accessing kvm_io_device::ops is not a good method, no
>> other architectures do in such way.
>>
>> Regards
>> Bibo Mao
>>> +
>>>           s->pch_pic_base = addr;
>>>           device = &s->device;
>>>           /* init device by pch pic writing and reading ops */
>>>
>>>>
>>>> -       s->pch_pic_base = addr;
>>>>           device = &s->device;
>>>>           /* init device by pch pic writing and reading ops */
>>>>           kvm_iodevice_init(device, &kvm_pch_pic_ops);
>>>>           mutex_lock(&kvm->slots_lock);
>>>> +       if (s->has_init) {
>>>> +               ret = -EEXIST;
>>>> +               goto out;
>>>> +       }
>>>>           /* register pch pic device */
>>>>           ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
>>>> +       if (!ret) {
>>>> +               s->pch_pic_base = addr;
>>>> +               s->has_init = true;
>>>> +       }
>>>> +out:
>>>>           mutex_unlock(&kvm->slots_lock);
>>>>
>>>> -       return (ret < 0) ? -EFAULT : 0;
>>>> +       return ret;
>>>>    }
>>>>
>>>>    /* used by user space to get or set pch pic registers */
>>>> --
>>>> 2.43.0
>>>>
>>

Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Huacai Chen]

On Wed, Sep 30, 2026 at 10:33 AM Bibo Mao <maobibo@loongson.cn> wrote:
>
>
>
> On 2026/9/30 上午10:24, Huacai Chen wrote:
> > On Wed, Sep 30, 2026 at 9:53 AM Bibo Mao <maobibo@loongson.cn> wrote:
> >>
> >>
> >>
> >> On 2026/9/29 下午8:43, Huacai Chen wrote:
> >>> Hi, Tao,
> >>>
> >>> On Tue, Sep 29, 2026 at 6:29 PM Tao Cui <cui.tao@linux.dev> wrote:
> >>>>
> >>>> From: Tao Cui <cuitao@kylinos.cn>
> >>>>
> >>>> KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
> >>>> invocation: every call overwrites pch_pic_base and registers the same
> >>>> kvm_io_device on the MMIO bus at the new address, while
> >>>> kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
> >>>> init, MMIO to the stale ranges computes its register offset against the
> >>>> new base and silently reads 0 / drops writes, and the leftover bus
> >>>> entries persist until the VM is destroyed.
> >>>>
> >>>> Reject repeated initialization with -EEXIST, tracking the state with
> >>>> a has_init flag so the check and the MMIO base update are atomic
> >>>> under slots_lock.  The base is only committed after a successful bus
> >>>> registration, and the real registration error is propagated instead
> >>>> of being replaced with -EFAULT.
> >>>>
> >>>> Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
> >>>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> >>>> ---
> >>>>    arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
> >>>>    arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
> >>>>    2 files changed, 11 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
> >>>> index 887b0431fd20..679132d840e6 100644
> >>>> --- a/arch/loongarch/include/asm/kvm_pch_pic.h
> >>>> +++ b/arch/loongarch/include/asm/kvm_pch_pic.h
> >>>> @@ -53,6 +53,7 @@ struct loongarch_pch_pic {
> >>>>           spinlock_t lock;
> >>>>           struct kvm *kvm;
> >>>>           struct kvm_io_device device;
> >>>> +       bool has_init;
> >>>>           union pch_pic_id id;
> >>>>           uint64_t mask; /* 1:disable irq, 0:enable irq */
> >>>>           uint64_t htmsi_en; /* 1:msi */
> >>>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> >>>> index 7a704f18880d..a884a043feef 100644
> >>>> --- a/arch/loongarch/kvm/intc/pch_pic.c
> >>>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> >>>> @@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
> >>>>           struct kvm_io_device *device;
> >>>>           struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> >>> Why so complicated? The below is enough, no?
> >>>
> >>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c
> >>> b/arch/loongarch/kvm/intc/pch_pic.c
> >>> index 2b63b0c2c7ce..7855d78304b7 100644
> >>> --- a/arch/loongarch/kvm/intc/pch_pic.c
> >>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> >>> @@ -281,6 +281,9 @@ static int kvm_pch_pic_init(struct kvm_device
> >>> *dev, u64 addr)
> >>>           struct kvm_io_device *device;
> >>>           struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
> >>>
> >>> +       if (s->device->ops)
> >>> +               return -EEXIST;
> >> This can work, however I think that it is not a good idea to access
> >> internal structure field about kvm_io_device. If so, there is no use
> >> about API kvm_iodevice_init(), just s->device->ops = &kvm_pch_pic_ops is ok.
> > I'm a little not agree. :)
> >
> > I think kvm_iodevice_init() is designed to do more work rather than
> > just set the ops (though it just set the ops now), otherwise its name
> > should be kvm_iodevice_set_ops().
> >
> > In addition, even if kvm_iodevice_init() is really a setter, there is
> > no getter for the ops, so when we need to access ops, we can only
> > open-code it.
> if so, you can try to add kvm_iodevice_get_ops API and check the
> response of KVM community.
There is not a setter, so I don't think a getter is necessary.

Moreover, you said "no other architectures directly access ops", but
in fact, __vgic_doorbell_to_its() from arch/arm64/kvm/vgic/vgic-its.c
directly accesses ops.


Huacai

>
> Regards
> Bibo Mao
> >
> >
> > Huacai
> >
> >>
> >> If adding has_init is redundant, maybe we can set s->pch_pic_base with
> >> INVALID_GPA in kvm_pch_pic_create() or some other methods. However I
> >> think directly accessing kvm_io_device::ops is not a good method, no
> >> other architectures do in such way.
> >>
> >> Regards
> >> Bibo Mao
> >>> +
> >>>           s->pch_pic_base = addr;
> >>>           device = &s->device;
> >>>           /* init device by pch pic writing and reading ops */
> >>>
> >>>>
> >>>> -       s->pch_pic_base = addr;
> >>>>           device = &s->device;
> >>>>           /* init device by pch pic writing and reading ops */
> >>>>           kvm_iodevice_init(device, &kvm_pch_pic_ops);
> >>>>           mutex_lock(&kvm->slots_lock);
> >>>> +       if (s->has_init) {
> >>>> +               ret = -EEXIST;
> >>>> +               goto out;
> >>>> +       }
> >>>>           /* register pch pic device */
> >>>>           ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
> >>>> +       if (!ret) {
> >>>> +               s->pch_pic_base = addr;
> >>>> +               s->has_init = true;
> >>>> +       }
> >>>> +out:
> >>>>           mutex_unlock(&kvm->slots_lock);
> >>>>
> >>>> -       return (ret < 0) ? -EFAULT : 0;
> >>>> +       return ret;
> >>>>    }
> >>>>
> >>>>    /* used by user space to get or set pch pic registers */
> >>>> --
> >>>> 2.43.0
> >>>>
> >>
>
>

Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Bibo Mao]

On 2026/9/30 下午4:52, Huacai Chen wrote:
> On Wed, Sep 30, 2026 at 10:33 AM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2026/9/30 上午10:24, Huacai Chen wrote:
>>> On Wed, Sep 30, 2026 at 9:53 AM Bibo Mao <maobibo@loongson.cn> wrote:
>>>>
>>>>
>>>>
>>>> On 2026/9/29 下午8:43, Huacai Chen wrote:
>>>>> Hi, Tao,
>>>>>
>>>>> On Tue, Sep 29, 2026 at 6:29 PM Tao Cui <cui.tao@linux.dev> wrote:
>>>>>>
>>>>>> From: Tao Cui <cuitao@kylinos.cn>
>>>>>>
>>>>>> KVM_DEV_LOONGARCH_PCH_PIC_CTRL_INIT has no guard against repeated
>>>>>> invocation: every call overwrites pch_pic_base and registers the same
>>>>>> kvm_io_device on the MMIO bus at the new address, while
>>>>>> kvm_pch_pic_destroy() unregisters only one bus range.  After a repeated
>>>>>> init, MMIO to the stale ranges computes its register offset against the
>>>>>> new base and silently reads 0 / drops writes, and the leftover bus
>>>>>> entries persist until the VM is destroyed.
>>>>>>
>>>>>> Reject repeated initialization with -EEXIST, tracking the state with
>>>>>> a has_init flag so the check and the MMIO base update are atomic
>>>>>> under slots_lock.  The base is only committed after a successful bus
>>>>>> registration, and the real registration error is propagated instead
>>>>>> of being replaced with -EFAULT.
>>>>>>
>>>>>> Fixes: d206d9514873 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
>>>>>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>>>>>> ---
>>>>>>     arch/loongarch/include/asm/kvm_pch_pic.h |  1 +
>>>>>>     arch/loongarch/kvm/intc/pch_pic.c        | 12 ++++++++++--
>>>>>>     2 files changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/loongarch/include/asm/kvm_pch_pic.h b/arch/loongarch/include/asm/kvm_pch_pic.h
>>>>>> index 887b0431fd20..679132d840e6 100644
>>>>>> --- a/arch/loongarch/include/asm/kvm_pch_pic.h
>>>>>> +++ b/arch/loongarch/include/asm/kvm_pch_pic.h
>>>>>> @@ -53,6 +53,7 @@ struct loongarch_pch_pic {
>>>>>>            spinlock_t lock;
>>>>>>            struct kvm *kvm;
>>>>>>            struct kvm_io_device device;
>>>>>> +       bool has_init;
>>>>>>            union pch_pic_id id;
>>>>>>            uint64_t mask; /* 1:disable irq, 0:enable irq */
>>>>>>            uint64_t htmsi_en; /* 1:msi */
>>>>>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
>>>>>> index 7a704f18880d..a884a043feef 100644
>>>>>> --- a/arch/loongarch/kvm/intc/pch_pic.c
>>>>>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
>>>>>> @@ -282,16 +282,24 @@ static int kvm_pch_pic_init(struct kvm_device *dev, u64 addr)
>>>>>>            struct kvm_io_device *device;
>>>>>>            struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
>>>>> Why so complicated? The below is enough, no?
>>>>>
>>>>> diff --git a/arch/loongarch/kvm/intc/pch_pic.c
>>>>> b/arch/loongarch/kvm/intc/pch_pic.c
>>>>> index 2b63b0c2c7ce..7855d78304b7 100644
>>>>> --- a/arch/loongarch/kvm/intc/pch_pic.c
>>>>> +++ b/arch/loongarch/kvm/intc/pch_pic.c
>>>>> @@ -281,6 +281,9 @@ static int kvm_pch_pic_init(struct kvm_device
>>>>> *dev, u64 addr)
>>>>>            struct kvm_io_device *device;
>>>>>            struct loongarch_pch_pic *s = dev->kvm->arch.pch_pic;
>>>>>
>>>>> +       if (s->device->ops)
>>>>> +               return -EEXIST;
>>>> This can work, however I think that it is not a good idea to access
>>>> internal structure field about kvm_io_device. If so, there is no use
>>>> about API kvm_iodevice_init(), just s->device->ops = &kvm_pch_pic_ops is ok.
>>> I'm a little not agree. :)
>>>
>>> I think kvm_iodevice_init() is designed to do more work rather than
>>> just set the ops (though it just set the ops now), otherwise its name
>>> should be kvm_iodevice_set_ops().
>>>
>>> In addition, even if kvm_iodevice_init() is really a setter, there is
>>> no getter for the ops, so when we need to access ops, we can only
>>> open-code it.
>> if so, you can try to add kvm_iodevice_get_ops API and check the
>> response of KVM community.
> There is not a setter, so I don't think a getter is necessary.
why setter is necessary, getter is not necessary.
> 
> Moreover, you said "no other architectures directly access ops", but
> in fact, __vgic_doorbell_to_its() from arch/arm64/kvm/vgic/vgic-its.c
> directly accesses ops.
If it is used by others, I have no objection any more. But for me I 
never write such code.

> 
> 
> Huacai
> 
>>
>> Regards
>> Bibo Mao
>>>
>>>
>>> Huacai
>>>
>>>>
>>>> If adding has_init is redundant, maybe we can set s->pch_pic_base with
>>>> INVALID_GPA in kvm_pch_pic_create() or some other methods. However I
>>>> think directly accessing kvm_io_device::ops is not a good method, no
>>>> other architectures do in such way.
>>>>
>>>> Regards
>>>> Bibo Mao
>>>>> +
>>>>>            s->pch_pic_base = addr;
>>>>>            device = &s->device;
>>>>>            /* init device by pch pic writing and reading ops */
>>>>>
>>>>>>
>>>>>> -       s->pch_pic_base = addr;
>>>>>>            device = &s->device;
>>>>>>            /* init device by pch pic writing and reading ops */
>>>>>>            kvm_iodevice_init(device, &kvm_pch_pic_ops);
>>>>>>            mutex_lock(&kvm->slots_lock);
>>>>>> +       if (s->has_init) {
>>>>>> +               ret = -EEXIST;
>>>>>> +               goto out;
>>>>>> +       }
>>>>>>            /* register pch pic device */
>>>>>>            ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, addr, PCH_PIC_SIZE, device);
>>>>>> +       if (!ret) {
>>>>>> +               s->pch_pic_base = addr;
>>>>>> +               s->has_init = true;
>>>>>> +       }
>>>>>> +out:
>>>>>>            mutex_unlock(&kvm->slots_lock);
>>>>>>
>>>>>> -       return (ret < 0) ? -EFAULT : 0;
>>>>>> +       return ret;
>>>>>>     }
>>>>>>
>>>>>>     /* used by user space to get or set pch pic registers */
>>>>>> --
>>>>>> 2.43.0
>>>>>>
>>>>
>>
>>