[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <85ad9d17fc50ff0784f5bcaefccdade53d2c18a9.camel@intel.com>
Date: Thu, 7 Mar 2024 12:45:16 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "Yamahata, Isaku"
<isaku.yamahata@...el.com>
CC: "federico.parola@...ito.it" <federico.parola@...ito.it>,
"pbonzini@...hat.com" <pbonzini@...hat.com>, "dmatlack@...gle.com"
<dmatlack@...gle.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "isaku.yamahata@...il.com"
<isaku.yamahata@...il.com>, "michael.roth@....com" <michael.roth@....com>,
"seanjc@...gle.com" <seanjc@...gle.com>
Subject: Re: [RFC PATCH 2/8] KVM: Add KVM_MAP_MEMORY vcpu ioctl to
pre-populate guest memory
>
> +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.
> +
> + 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.
> +
> + 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.
> +
> + return r;
> +}
> +
> static long kvm_vcpu_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg)
> {
> @@ -4620,6 +4683,17 @@ static long kvm_vcpu_ioctl(struct file *filp,
> r = kvm_vcpu_ioctl_get_stats_fd(vcpu);
> break;
> }
> + case KVM_MAP_MEMORY: {
> + struct kvm_memory_mapping mapping;
> +
> + r = -EFAULT;
> + if (copy_from_user(&mapping, argp, sizeof(mapping)))
> + break;
> + r = kvm_vcpu_map_memory(vcpu, &mapping);
> + if (copy_to_user(argp, &mapping, sizeof(mapping)))
> + r = -EFAULT;
> + break;
> + }
> default:
> r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
> }
Powered by blists - more mailing lists