Re: [PATCH v6 03/14] KVM: X86: Don't track dirty for KVM_SET_[TSS_ADDR|IDENTITY_MAP_ADDR]

[Peter Xu]

On Tue, Mar 10, 2020 at 08:06:37AM -0700, Sean Christopherson wrote:
> On Mon, Mar 09, 2020 at 05:44:13PM -0400, Peter Xu wrote:
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 40b1e6138cd5..fc638a164e03 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -3467,34 +3467,26 @@ static bool guest_state_valid(struct kvm_vcpu *vcpu)
> >  	return true;
> >  }
> >  
> > -static int init_rmode_tss(struct kvm *kvm)
> > +static int init_rmode_tss(struct kvm *kvm, void __user *ua)
> >  {
> > -	gfn_t fn;
> > +	const void *zero_page = (const void *) __va(page_to_phys(ZERO_PAGE(0)));
> >  	u16 data = 0;
> 
> "data" doesn't need to be intialized to zero, it's set below before it's used.

Yeah I didn't touch it because this change is irrelevant to the rest.
But I can remove it altogether.

> 
> >  	int idx, r;
> 
> nit: I'd prefer to rename "idx" to "i" to make it more obvious it's a plain
> ole loop counter.  Reusing the srcu index made me do a double take :-)

Another irrelevant change, but ok.

> 
> >  
> > -	idx = srcu_read_lock(&kvm->srcu);
> > -	fn = to_kvm_vmx(kvm)->tss_addr >> PAGE_SHIFT;
> > -	r = kvm_clear_guest_page(kvm, fn, 0, PAGE_SIZE);
> > -	if (r < 0)
> > -		goto out;
> > +	for (idx = 0; idx < 3; idx++) {
> > +		r = __copy_to_user(ua + PAGE_SIZE * idx, zero_page, PAGE_SIZE);
> > +		if (r)
> > +			return -EFAULT;
> > +	}
> 
> Can this be done in a single __copy_to_user(), or do those helpers not like
> crossing page boundaries?

Maybe because the zero_page is only PAGE_SIZE long? :)

[...]

> > -int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
> > +/**
> > + * __x86_set_memory_region: Setup KVM internal memory slot
> > + *
> > + * @kvm: the kvm pointer to the VM.
> > + * @id: the slot ID to setup.
> > + * @gpa: the GPA to install the slot (unused when @size == 0).
> > + * @size: the size of the slot. Set to zero to uninstall a slot.
> > + *
> > + * This function helps to setup a KVM internal memory slot.  Specify
> > + * @size > 0 to install a new slot, while @size == 0 to uninstall a
> > + * slot.  The return code can be one of the following:
> > + *
> > + *   - An error number if error happened, or,
> > + *   - For installation: the HVA of the newly mapped memory slot, or,
> > + *   - For uninstallation: zero if we successfully uninstall a slot.
> 
> Maybe tweak this so the return it stands out?  And returning zero on
> uninstallation is no longer true in kvm/queue, at least not without further
> modifications (as is it'll return 0xdead000000000000 on 64-bit).  The
> 0xdead shenanigans won't trigger IS_ERR(), so I think this can simply be:
> 
>  * Returns:
>  *   hva:    on success
>  *   -errno: on error
> 
> With the blurb below calling out that hva is bogus uninstallation.

Sure, I'll rebase to kvm/queue for the next version with the
suggestion.

Thanks,

-- 
Peter Xu