* [PATCH v2 0/4] LoongArch: KVM: irqchip fixes
@ 2026-09-29 10:28 Tao Cui
2026-09-29 10:28 ` [PATCH v2 1/4] LoongArch: KVM: Clear device pointer in irqchip destroy callbacks Tao Cui
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Tao Cui @ 2026-09-29 10:28 UTC (permalink / raw)
To: maobibo, gaosong, zhaotianrui
Cc: loongarch, kvm, linux-kernel, chenhuacai, kernel,
nagachaithanya9911, cui.tao, 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
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 1/4] LoongArch: KVM: Clear device pointer in irqchip destroy callbacks 2026-09-29 10:28 [PATCH v2 0/4] LoongArch: KVM: irqchip fixes Tao Cui @ 2026-09-29 10:28 ` Tao Cui 2026-09-29 10:28 ` [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq Tao Cui ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: Tao Cui @ 2026-09-29 10:28 UTC (permalink / raw) To: maobibo, gaosong, zhaotianrui Cc: loongarch, kvm, linux-kernel, chenhuacai, kernel, nagachaithanya9911, cui.tao, 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq 2026-09-29 10:28 [PATCH v2 0/4] LoongArch: KVM: irqchip fixes Tao Cui 2026-09-29 10:28 ` [PATCH v2 1/4] LoongArch: KVM: Clear device pointer in irqchip destroy callbacks Tao Cui @ 2026-09-29 10:28 ` Tao Cui 2026-09-30 1:42 ` Bibo Mao 2026-09-29 10:28 ` [PATCH v2 3/4] LoongArch: KVM: Propagate real error code in kvm_pch_pic_create Tao Cui 2026-09-29 10:28 ` [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT Tao Cui 3 siblings, 1 reply; 16+ messages in thread From: Tao Cui @ 2026-09-29 10:28 UTC (permalink / raw) To: maobibo, gaosong, zhaotianrui Cc: loongarch, kvm, linux-kernel, chenhuacai, kernel, nagachaithanya9911, cui.tao, 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq 2026-09-29 10:28 ` [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq Tao Cui @ 2026-09-30 1:42 ` Bibo Mao 2026-09-30 2:08 ` Huacai Chen 0 siblings, 1 reply; 16+ messages in thread From: Bibo Mao @ 2026-09-30 1:42 UTC (permalink / raw) To: Tao Cui, gaosong, zhaotianrui Cc: loongarch, kvm, linux-kernel, chenhuacai, kernel, nagachaithanya9911, Tao Cui 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); > } > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq 2026-09-30 1:42 ` Bibo Mao @ 2026-09-30 2:08 ` Huacai Chen 2026-09-30 2:15 ` Bibo Mao 0 siblings, 1 reply; 16+ messages in thread From: Huacai Chen @ 2026-09-30 2:08 UTC (permalink / raw) To: Bibo Mao Cc: Tao Cui, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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); > > } > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq 2026-09-30 2:08 ` Huacai Chen @ 2026-09-30 2:15 ` Bibo Mao 0 siblings, 0 replies; 16+ messages in thread From: Bibo Mao @ 2026-09-30 2:15 UTC (permalink / raw) To: Huacai Chen Cc: Tao Cui, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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); >>> } >>> >>> >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/4] LoongArch: KVM: Propagate real error code in kvm_pch_pic_create 2026-09-29 10:28 [PATCH v2 0/4] LoongArch: KVM: irqchip fixes Tao Cui 2026-09-29 10:28 ` [PATCH v2 1/4] LoongArch: KVM: Clear device pointer in irqchip destroy callbacks Tao Cui 2026-09-29 10:28 ` [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq Tao Cui @ 2026-09-29 10:28 ` Tao Cui 2026-09-29 10:28 ` [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT Tao Cui 3 siblings, 0 replies; 16+ messages in thread From: Tao Cui @ 2026-09-29 10:28 UTC (permalink / raw) To: maobibo, gaosong, zhaotianrui Cc: loongarch, kvm, linux-kernel, chenhuacai, kernel, nagachaithanya9911, cui.tao, 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-29 10:28 [PATCH v2 0/4] LoongArch: KVM: irqchip fixes Tao Cui ` (2 preceding siblings ...) 2026-09-29 10:28 ` [PATCH v2 3/4] LoongArch: KVM: Propagate real error code in kvm_pch_pic_create Tao Cui @ 2026-09-29 10:28 ` Tao Cui 2026-09-29 12:43 ` Huacai Chen 3 siblings, 1 reply; 16+ messages in thread From: Tao Cui @ 2026-09-29 10:28 UTC (permalink / raw) To: maobibo, gaosong, zhaotianrui Cc: loongarch, kvm, linux-kernel, chenhuacai, kernel, nagachaithanya9911, cui.tao, 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-29 10:28 ` [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT Tao Cui @ 2026-09-29 12:43 ` Huacai Chen 2026-09-30 1:15 ` Tao Cui 2026-09-30 1:54 ` Bibo Mao 0 siblings, 2 replies; 16+ messages in thread From: Huacai Chen @ 2026-09-29 12:43 UTC (permalink / raw) To: Tao Cui Cc: maobibo, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-29 12:43 ` Huacai Chen @ 2026-09-30 1:15 ` Tao Cui 2026-09-30 2:25 ` Huacai Chen 2026-09-30 1:54 ` Bibo Mao 1 sibling, 1 reply; 16+ messages in thread From: Tao Cui @ 2026-09-30 1:15 UTC (permalink / raw) To: Huacai Chen Cc: cui.tao, maobibo, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, 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 >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-30 1:15 ` Tao Cui @ 2026-09-30 2:25 ` Huacai Chen 0 siblings, 0 replies; 16+ messages in thread From: Huacai Chen @ 2026-09-30 2:25 UTC (permalink / raw) To: Tao Cui Cc: maobibo, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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 > >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-29 12:43 ` Huacai Chen 2026-09-30 1:15 ` Tao Cui @ 2026-09-30 1:54 ` Bibo Mao 2026-09-30 2:24 ` Huacai Chen 1 sibling, 1 reply; 16+ messages in thread From: Bibo Mao @ 2026-09-30 1:54 UTC (permalink / raw) To: Huacai Chen, Tao Cui Cc: gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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 >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-30 1:54 ` Bibo Mao @ 2026-09-30 2:24 ` Huacai Chen 2026-09-30 2:34 ` Bibo Mao 0 siblings, 1 reply; 16+ messages in thread From: Huacai Chen @ 2026-09-30 2:24 UTC (permalink / raw) To: Bibo Mao Cc: Tao Cui, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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 > >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-30 2:24 ` Huacai Chen @ 2026-09-30 2:34 ` Bibo Mao 2026-09-30 8:52 ` Huacai Chen 0 siblings, 1 reply; 16+ messages in thread From: Bibo Mao @ 2026-09-30 2:34 UTC (permalink / raw) To: Huacai Chen Cc: Tao Cui, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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 >>>> >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-30 2:34 ` Bibo Mao @ 2026-09-30 8:52 ` Huacai Chen 2026-09-30 9:05 ` Bibo Mao 0 siblings, 1 reply; 16+ messages in thread From: Huacai Chen @ 2026-09-30 8:52 UTC (permalink / raw) To: Bibo Mao Cc: Tao Cui, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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 > >>>> > >> > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT 2026-09-30 8:52 ` Huacai Chen @ 2026-09-30 9:05 ` Bibo Mao 0 siblings, 0 replies; 16+ messages in thread From: Bibo Mao @ 2026-09-30 9:05 UTC (permalink / raw) To: Huacai Chen Cc: Tao Cui, gaosong, zhaotianrui, loongarch, kvm, linux-kernel, kernel, nagachaithanya9911, Tao Cui 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 >>>>>> >>>> >> >> ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2026-09-30 9:03 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-29 10:28 [PATCH v2 0/4] LoongArch: KVM: irqchip fixes Tao Cui 2026-09-29 10:28 ` [PATCH v2 1/4] LoongArch: KVM: Clear device pointer in irqchip destroy callbacks Tao Cui 2026-09-29 10:28 ` [PATCH v2 2/4] LoongArch: KVM: Load dmsintc pointer once in pch_msi_set_irq Tao Cui 2026-09-30 1:42 ` Bibo Mao 2026-09-30 2:08 ` Huacai Chen 2026-09-30 2:15 ` Bibo Mao 2026-09-29 10:28 ` [PATCH v2 3/4] LoongArch: KVM: Propagate real error code in kvm_pch_pic_create Tao Cui 2026-09-29 10:28 ` [PATCH v2 4/4] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT Tao Cui 2026-09-29 12:43 ` Huacai Chen 2026-09-30 1:15 ` Tao Cui 2026-09-30 2:25 ` Huacai Chen 2026-09-30 1:54 ` Bibo Mao 2026-09-30 2:24 ` Huacai Chen 2026-09-30 2:34 ` Bibo Mao 2026-09-30 8:52 ` Huacai Chen 2026-09-30 9:05 ` Bibo Mao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®