[PATCH v3 36/40] KVM: arm64: gic-v5: Implement save/restore mechanisms for ISTs

[Sascha Bischoff]

On Tue, 2026-07-21 at 16:42 +0100, Fuad Tabba wrote:
> Hi Sascha,
> 
> Two things about the new KVM_DEV_ARM_VGIC_GRP_IST interface. Both are
> about the userspace ABI rather than the mechanism.
> 
> ...
> 
> > +int vgic_v5_save_spi_ist(struct kvm *kvm, struct kvm_device_attr
> > *attr)
> > +{
> > +    u32 __user *uaddr = (u32 __user *)(unsigned long)attr->addr;
> ...
> > +    for (unsigned int i = 0; i < kvm->arch.vgic.nr_spis; ++i) {
> ...
> > +        __le32 *h_iste_addr = ist.base + i * ist.iste_size;
> > +
> > +        h_iste = READ_ONCE(*h_iste_addr);
> > +        ret = put_user(h_iste, uaddr);
> > +        if (ret)
> > +            return ret;
> > +
> > +        uaddr++;
> > +    }
> 
> The kernel writes nr_spis * 4 bytes into attr->addr, but struct
> kvm_device_attr has no length field, so userspace has no way to
> declare how large its buffer is and the kernel has no way to check
> it.
> The size is implied by nr_spis, which userspace set earlier through
> KVM_DEV_ARM_VGIC_GRP_NR_IRQS, or never set, in which case it
> defaulted
> to 32.
> 
> put_user() has no idea how large the caller's buffer is. It only
> fails
> if the address itself is not writable, so writes past the end of a
> short buffer land in whatever happens to be next, with no error
> reported. The documentation states the required size, but nothing
> enforces it.
> 
> It is also the only variable length buffer in the VGICv5 interface.
> GRP_IRS_REGS is a __u64, USERSPACE_PPIS is a __u64[2], GRP_NR_IRQS is
> a __u32, and none of those can be got wrong.
> 
> kvm_device_attr itself is fixed, so no length can be added there.
> Changing what addr points at is still possible while this is
> unmerged,
> but would not be afterwards. Would it make sense for addr to point at
> a small structure carrying an explicit size, so that the kernel can
> reject a mismatch?

Hi Fuad,

I think that this is a great idea. It makes a lot more sense than
blindly trusting that the VMM has done the correct thing. With this,
KVM can validate that the VMM at least claims to have done what we'd
expect.

I guess you had something like this in mind?

    struct kvm_vgic_v5_ist {
        __u64 spi_ist_addr;
        __u64 spi_ist_size;
    };

Which then means that we can do:

    struct kvm_vgic_v5_ist ist_attr;                                  
    size_t expected = kvm->arch.vgic.nr_spis * sizeof(__u32);         

    if (copy_from_user(&ist_attr, uaddr, sizeof(ist_attr)))           
        return -EFAULT;
                                                           
    if (ist_attr.spi_ist_size != expected)
        return -EINVAL;                                               

    if (!ist_attr.spi_ist_addr)                                       
        return -EINVAL;

> 
> The second point is the LPI IST going into guest memory. I understand
> from the commit message that the shadowing is deliberate and that the
> guest has already allocated the storage, so this is about the
> interface rather than that decision.
> 
> Saving the ISTs is a KVM_GET_DEVICE_ATTR, and it writes into guest
> memory. Several of the documented ordering constraints are enforced,
> with -EBUSY when the VGIC is not initialised or when a restore is
> attempted after the VM has run. This one cannot be, because it is
> about when the VMM serialises guest memory:
> 
> > These two steps may be performed in either order. However, the
> > guest memory
> > must be serialised after the ISTs have been saved, as saving the
> > LPI IST writes
> > the IST state back into guest memory.
> 
> A VMM that snapshots guest memory before saving the ISTs gets no
> error, and the IST contents simply never reach the destination. Live
> migration looks fine to me, since kvm_write_guest() marks the pages
> dirty and a later pass picks them up, but a stop and copy save done
> in
> the natural order would lose them silently.

Yeah, these sorts of issues are exactly the concern with the current
API.

> 
> Was a userspace buffer considered for the LPI IST as well, the way
> the SPI IST already works?

When I originally designed this API, I was keen to re-use the guest
memory here so had quickly dismissed this approach. However, looking at
it again now, I think it makes a lot of sense, especially as we are
limiting the guest to 16-bits of ID space - this wasn't the case
originally which allowed the size of the ISTs to balloon significantly,
and hence it made more sense to re-use the guest's memory.

> That would remove the ordering requirement
> entirely, and by my reading the worst case is 65536 entries at 32
> bits, so 256 KiB, given the 16 bit clamp on the guest's LPI ID bits.

I am actually in favour of this approach. It removes the asymmetry
between the LPI and SPI ISTs on the save/restore path, and avoids the
explicit iteration over guest memory to save or restore the LPI IST.

With this, the proposed struct would become the following.

    struct kvm_vgic_v5_ist {
        __u64 spi_ist_addr;
        __u64 spi_ist_size;
        __u64 lpi_ist_addr;
        __u64 lpi_ist_size;
    };

The VMM would need to read the guest's IRS_IST_CFGR and IST_IST_BASER
to determine if the guest is using LPIs or not, and how many it needs
to provide storage for. If no LPIs are in use, then allow
lpi_ist_addr/lpi_ist_size to be 0, but otherwise the size must match.

Does this align with what you had in mind?

> 
> Cheers,
> /fuad

Thanks,
Sascha