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

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

Hi, Huacai

在 2026/10/12 14:37, Huacai Chen 写道:
> On Wed, Sep 30, 2026 at 5:03 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>>
>>
>> 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.
> OK, Tao, please update your code, drop the 2nd patch and simplify the
> 4th one. Thanks.
> 

I'll send v3 shortly.  It drops the dmsintc single-load patch, and the
CTRL_INIT patch now checks s->device.ops for repeated initialization
instead of the separate has_init flag.  pch_pic_base is still updated
only after successful bus registration, and the original registration
error is returned instead of -EFAULT.

Thanks for the review.
Tao

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