Re: [PATCH v3 3/3] LoongArch: KVM: Reject repeated PCH-PIC CTRL_INIT

[Huacai Chen]

On Wed, Oct 14, 2026 at 4:37 PM Tao Cui <cui.tao@xxxxxxxxx> wrote:
>
> Hi, Huacai
>
> 在 2026/10/14 15:37, Huacai Chen 写道:
> > Hi, Tao,
> >
> > On Wed, Oct 14, 2026 at 1:30 PM Tao Cui <cui.tao@xxxxxxxxx> wrote:
> >>
> >> From: Tao Cui <cuitao@xxxxxxxxxx>
> >>
> >> 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, using the initialized
> >> state of the kvm_io_device as the indicator.  The MMIO 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@xxxxxxxxxx>
> >> ---
> >>  arch/loongarch/kvm/intc/pch_pic.c | 12 +++++++++---
> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
> >> index 51e4a6a39dc0..7979466de729 100644
> >> --- a/arch/loongarch/kvm/intc/pch_pic.c
> >> +++ b/arch/loongarch/kvm/intc/pch_pic.c
> >> @@ -281,16 +281,22 @@ 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;
> >> +       mutex_lock(&kvm->slots_lock);
> > Why move the mutex_lock()?
> >
>
> Thanks for the question.
>
> The intent is to keep the repeated-init check mutual-excluded with the
> bus registration.  The set_attr ioctl path holds no other lock, so with
> the check outside slots_lock, two concurrent CTRL_INIT calls could both
> see device.ops == NULL and both register the same device on the MMIO
> bus.  Holding the lock across the check, the registration and the base
> commit makes them exclusive.
>
> That said, if you prefer the check before the lock, I can respin --
> please let me know.
OK, moving the lock is reasonable, but I don't think updating
pch_pic_base conditionally is necessary. So just move it before
kvm_iodevice_init() is fine.

Huacai

>
> Thanks,
> Tao
>
> > Huacai
> >
> >> +       if (s->device.ops) {
> >> +               ret = -EEXIST;
> >> +               goto out;
> >> +       }
> >>         /* init device by pch pic writing and reading ops */
> >>         kvm_iodevice_init(device, &kvm_pch_pic_ops);
> >> -       mutex_lock(&kvm->slots_lock);
> >>         /* 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;
> >> +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
> >>
>