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

[Fuad Tabba]

Hi Sascha,

...

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

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

Pretty much, yes. Folding the LPI IST in the same way is better than I
expected. All __u64 also avoids implicit padding.

The LPI size is the one that matters. The SPI size is nr_spis * 4, and
nr_spis is something the VMM set itself through GRP_NR_IRQS, so the
two sides cannot disagree. The LPI size is derived from IST_CFGR
instead, so that is the buffer where the kernel computing the expected
size and rejecting a mismatch earns its keep. The entry is a fixed 4
bytes either way, since only the architected 32-bit ISTE is exposed,
so sizeof(__u32) is right.

The main thing I would ask is that the checks be strict both ways,
since turning silent acceptance into -EINVAL is really the point of
the size field. Two cases: reject a non-zero lpi_ist_addr when the
kernel has no LPI IST, not just allow 0 when it does; and on restore,
reject rather than silently skip when the LPI IST is not configured
yet, which as far as I can see makes an out-of-order restore, the IST
buffer before IST_CFGR and IST_BASER, a no-op.

Another thing your proposal cleanly resolves is the ordering point
from my earlier mail. Once the LPI IST save writes the userspace
buffer instead of guest memory, the requirement to serialise guest
memory after saving the ISTs goes away, since the save no longer
touches guest memory.

Cheers,
/fuad