Re: [PATCH v2 4/6] KVM: x86: Introduce KVM_GET_SREGS2 / KVM_SET_SREGS2

[Maxim Levitsky <mlevitsk@redhat.com>]

On Mon, 2021-04-26 at 14:32 +0200, Paolo Bonzini wrote:
> On 26/04/21 13:13, Maxim Levitsky wrote:
> > +	if (sregs2->flags & KVM_SREGS2_FLAGS_PDPTRS_VALID) {
> > +
> > +		if (!is_pae_paging(vcpu))
> > +			return -EINVAL;
> > +
> > +		for (i = 0 ; i < 4 ; i++)
> > +			kvm_pdptr_write(vcpu, i, sregs2->pdptrs[i]);
> > +
> > +		kvm_register_mark_dirty(vcpu, VCPU_EXREG_PDPTR);
> > +		mmu_reset_needed = 1;
> > +	}
> 
> I think this should also have
> 
> 	else {
> 		if (is_pae_paging(vcpu))
> 			return -EINVAL;
> 	}


What about the case when we migrate from qemu that doesn't use
this ioctl to qemu that does? 

In this case assuming that the new qemu does use SREGS2 ioctls,
the PDPTR data will not be present
in the migration stream and thus qemu will call this ioctl without this flag
set.

I think I should in this case load the pdptrs from memory,
Or I should make qemu not use this ioctl in this.
What do you prefer?

Thanks for pointing this bug out though!
I haven't thought about this case well enough.

Best regards,
	Maxim Levitsky

> 
> but perhaps even better, check it at the beginning:
> 
> 	if ((sregs->cr4 & X86_CR4_PAE) &&
>              !!(sregs->efer & EFER_LMA) == !!(sregs2->flags & KVM_SREGS2_FLAGS_PDPTRS_VALID))
> 		return -EINVAL;
> 
> which technically means the flag is redundant, but there is some value in
> having the flag and not allowing the user to shoot itself in the foot.
> 
> Paolo
>