Re: [RFC PATCH 2/8] KVM: Add KVM_MAP_MEMORY vcpu ioctl to pre-populate guest memory

From: Isaku Yamahata <isaku.yamahata@linux.intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"federico.parola@polito.it" <federico.parola@polito.it>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"dmatlack@google.com" <dmatlack@google.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	"michael.roth@amd.com" <michael.roth@amd.com>,
	"seanjc@google.com" <seanjc@google.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [RFC PATCH 2/8] KVM: Add KVM_MAP_MEMORY vcpu ioctl to pre-populate guest memory
Date: Thu, 7 Mar 2024 12:41:49 -0800	[thread overview]
Message-ID: <20240307204149.GJ368614@ls.amr.corp.intel.com> (raw)
In-Reply-To: <85ad9d17fc50ff0784f5bcaefccdade53d2c18a9.camel@intel.com>

On Thu, Mar 07, 2024 at 12:45:16PM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> 
> >  
> > +int kvm_arch_vcpu_pre_map_memory(struct kvm_vcpu *vcpu);
> 
> No explanation of why this is needed, and why it only takes @vcpu as input w/o
> having the @mapping.
>  
> > +int kvm_arch_vcpu_map_memory(struct kvm_vcpu *vcpu,
> > +			     struct kvm_memory_mapping *mapping);
> > +
> > 
> 
> [...]
> 
> > +static int kvm_vcpu_map_memory(struct kvm_vcpu *vcpu,
> > +			       struct kvm_memory_mapping *mapping)
> > +{
> > +	bool added = false;
> > +	int idx, r = 0;
> > +
> > +	if (mapping->flags & ~(KVM_MEMORY_MAPPING_FLAG_WRITE |
> > +			       KVM_MEMORY_MAPPING_FLAG_EXEC |
> > +			       KVM_MEMORY_MAPPING_FLAG_USER |
> > +			       KVM_MEMORY_MAPPING_FLAG_PRIVATE))
> > +		return -EINVAL;
> > +	if ((mapping->flags & KVM_MEMORY_MAPPING_FLAG_PRIVATE) &&
> > +	    !kvm_arch_has_private_mem(vcpu->kvm))
> > +		return -EINVAL;
> > +
> > +	/* Sanity check */
> > +	if (!IS_ALIGNED(mapping->source, PAGE_SIZE) ||
> > +	    !mapping->nr_pages ||
> > +	    mapping->base_gfn + mapping->nr_pages <= mapping->base_gfn)
> > +		return -EINVAL;
> > +
> > +	vcpu_load(vcpu);
> > +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> > +	r = kvm_arch_vcpu_pre_map_memory(vcpu);
> > +	if (r)
> > +		return r;
> 
> Returning w/o unloading the vcpu and releasing the SRCU.

Oos, Will fix.

> > +
> > +	while (mapping->nr_pages) {
> > +		if (signal_pending(current)) {
> > +			r = -ERESTARTSYS;
> > +			break;
> > +		}
> > +
> > +		if (need_resched())
> > +			cond_resched();
> 
> need_resched() is not needed.
> 
> And normally I think we just put it at the end of the loop.

Ok, will move it.

> > +
> > +		r = kvm_arch_vcpu_map_memory(vcpu, mapping);
> > +		if (r)
> > +			break;
> > +
> > +		added = true;
> > +	}
> > +
> > +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> > +	vcpu_put(vcpu);
> > +
> > +	if (added && mapping->nr_pages > 0)
> > +		r = -EAGAIN;
> 
> Why do we need @added?
> 
> I assume the kvm_arch_vcpu_map_memory() can internally update the mapping-
> >nr_pages but still return -E<WHATEVER>.  So when that happens in the first call
> of kvm_arch_vcpu_map_memory(), @added won't get chance to turn to true.

I intend to tell the caller if the range is partially processed or not.
Anyway this seems moot. Let's drop this if clause.  Then it's caller's
responsibility to check error and partial conversion and to optionally loop
with the remaining region.
-- 
Isaku Yamahata <isaku.yamahata@linux.intel.com>