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

[Tao Cui <cui.tao@linux.dev>]

Hi, Huacai

在 2026/10/14 15:37, Huacai Chen 写道:
> Hi, Tao,
> 
> On Wed, Oct 14, 2026 at 1:30 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, 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@kylinos.cn>
>> ---
>>  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.

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