Re: [PATCH] KVM: x86: Shove vp_bitmap handling down into sparse_set_to_vcpu_mask()

[Sean Christopherson]

On Fri, Oct 29, 2021, Sean Christopherson wrote:
> On Fri, Oct 29, 2021, Sean Christopherson wrote:
> > On Fri, Oct 29, 2021, Vitaly Kuznetsov wrote:
> > > > +	/* If vp_index == vcpu_idx for all vCPUs, fill vcpu_mask directly. */
> > > > +	if (likely(!has_mismatch))
> > > > +		bitmap = (u64 *)vcpu_mask;
> > > > +
> > > > +	memset(bitmap, 0, sizeof(vp_bitmap));
> > > 
> > > ... but in the unlikely case has_mismatch == true 'bitmap' is still
> > > uninitialized here, right? How doesn't it crash?
> > 
> > I'm sure it does crash.  I'll hack the guest to actually test this.
> 
> Crash confirmed.  But I don't feel too bad about my one-line goof because the
> existing code botches sparse VP_SET, i.e. _EX flows.  The spec requires the guest
> to explicit specify the number of QWORDS in the variable header[*], e.g. VP_SET
> in this case, but KVM ignores that and does a harebrained calculation to "count"
> the number of sparse banks.  It does this by counting the number of bits set in
> valid_bank_mask, which is comically broken because (a) the whole "sparse" thing
> should be a clue that they banks are not packed together, (b) the spec clearly
> states that "bank = VPindex / 64", (c) the sparse_bank madness makes this waaaay
> more complicated than it needs to be, and (d) the massive sparse_bank allocation
> on the stack is completely unnecessary because KVM simply ignores everything that
> wouldn't fit in vp_bitmap.
> 
> To reproduce, stuff vp_index in descending order starting from KVM_MAX_VCPUS - 1.
> 
> 	hv_vcpu->vp_index = KVM_MAX_VCPUS - vcpu->vcpu_idx - 1;
> 
> E.g. with an 8 vCPU guest, KVM will calculate sparse_banks_len=1, read zeros, and
> do nothing, hanging the guest because it never sends IPIs.
 
Ugh, I can't read.  The example[*] clarifies that the "sparse" VP_SET packs things
into BankContents.  I don't think I imagined my guest hanging though, so something
is awry.  Back to debugging...

[*] https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vp_set#processor-set-example

> So v2 will be completely different because the "fix" for the KASAN issue is to
> get rid of sparse_banks entirely.
> 
> [1] https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/hypercall-interface#variable-sized-hypercall-input-headers
> [2] https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vp_set#sparse-virtual-processor-set