[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 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
>>>>
>>