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

Huang, Kai
2024-03-07T12:45:22Z

>
> +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);
> }