Re: [PATCH] KVM: ARM: updtae the VMID generation logic

[Marc Zyngier <marc.zyngier@arm.com>]

On Thu, 29 Mar 2018 16:27:58 +0100,
Mark Rutland wrote:
> 
> On Thu, Mar 29, 2018 at 11:00:24PM +0800, Shannon Zhao wrote:
> > From: zhaoshenglong <zhaoshenglong@huawei.com>
> > 
> > Currently the VMID for some VM is allocated during VCPU entry/exit
> > context and will be updated when kvm_next_vmid inversion. So this will
> > cause the existing VMs exiting from guest and flush the tlb and icache.
> > 
> > Also, while a platform with 8 bit VMID supports 255 VMs, it can create
> > more than 255 VMs and if we create e.g. 256 VMs, some VMs will occur
> > page fault since at some moment two VMs have same VMID.
> 
> Have you seen this happen?
> 
> I beleive that update_vttbr() should prevent this. We intialize
> kvm_vmid_gen to 1, and when we init a VM, we set its vmid_gen to 0. So
> the first time a VM is scheduled, update_vttbr() will allocate a VMID,
> and by construction we shouldn't be able to allocate the same VMID to
> multiple active VMs, regardless of whether we overflow several
> times.

I remember testing that exact scenario when we implemented the VMID
rollover a (long) while back. Maybe we've introduced a regression, but
we're supposed to support 255 VMs running at the same time (which is
not the same as having 255 VMs in total).

Shannon: if you have observed such regression, please let us know.

> 
> > This patch uses the bitmap to record which VMID used and available.
> > Initialize the VMID and vttbr during creating the VM instead of VCPU
> > entry/exit context. Also it will return error to user space if it wants
> > to create VMs more than the supporting number.
> 
> This creates a functional regression for anyone creating a large number
> of VMs.

Indeed, and I'm not buys that approach at all. As I said above, the
intent is that we can have up to 2^VMID_SIZE-1 VMs running at the same
time, and *any* number of VMs in the system.

> If VMID overflow is a real bottleneck, it would be vastly better to
> improve the VMID allocator along the lines of the arm64 ASID allocator,
> so that upon overflow we reserve the set of active VMIDs (and therefore
> avoid expensive TLB + icache maintenance). That does not require a
> global limit on the number of VMs.

+1.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.