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

[Bibo Mao]

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.

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