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

[Huacai Chen]

On Wed, Sep 30, 2026 at 10:33 AM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
>
>
>
> On 2026/9/30 上午10:24, Huacai Chen wrote:
> > On Wed, Sep 30, 2026 at 9:53 AM Bibo Mao <maobibo@xxxxxxxxxxx> 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@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, 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@xxxxxxxxxx>
> >>>> ---
> >>>>    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
> >>>>
> >>
>
>