Re: [PATCH v1 02/13] KVM: s390: fake memslots for ucontrol VMs

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri, 10 Jan 2025 10:31:38 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 08.01.25 um 19:14 schrieb Claudio Imbrenda:
> > Create fake memslots for ucontrol VMs. The fake memslots identity-map
> > userspace.
> > 
> > Now memslots will always be present, and ucontrol is not a special case
> > anymore.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >   arch/s390/kvm/kvm-s390.c | 42 ++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index ecbdd7d41230..797b8503c162 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -59,6 +59,7 @@
> >   #define LOCAL_IRQS 32
> >   #define VCPU_IRQS_MAX_BUF (sizeof(struct kvm_s390_irq) * \
> >   			   (KVM_MAX_VCPUS + LOCAL_IRQS))
> > +#define UCONTROL_SLOT_SIZE SZ_4T
> >   
> >   const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
> >   	KVM_GENERIC_VM_STATS(),
> > @@ -3326,6 +3327,23 @@ void kvm_arch_free_vm(struct kvm *kvm)
> >   	__kvm_arch_free_vm(kvm);
> >   }
> >   
> > +static void kvm_s390_ucontrol_ensure_memslot(struct kvm *kvm, unsigned long addr)
> > +{
> > +	struct kvm_userspace_memory_region2 region = {
> > +		.slot = addr / UCONTROL_SLOT_SIZE,
> > +		.memory_size = UCONTROL_SLOT_SIZE,
> > +		.guest_phys_addr = ALIGN_DOWN(addr, UCONTROL_SLOT_SIZE),
> > +		.userspace_addr = ALIGN_DOWN(addr, UCONTROL_SLOT_SIZE),
> > +	};
> > +	struct kvm_memory_slot *slot;
> > +
> > +	mutex_lock(&kvm->slots_lock);
> > +	slot = gfn_to_memslot(kvm, addr);
> > +	if (!slot)
> > +		__kvm_set_memory_region(kvm, &region);
> > +	mutex_unlock(&kvm->slots_lock);
> > +}
> > +  
> 
> Would simply having one slot from 0 to TASK_SIZE also work? This could avoid the
> construction of the fake slots during runtime.

unfortunately memslots are limited to 4TiB.
having bigger ones would require even more changes all across KVM (and
maybe qemu too)

> 
> >   int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> >   {
> >   	gfp_t alloc_flags = GFP_KERNEL_ACCOUNT;
> > @@ -3430,6 +3448,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> >   	if (type & KVM_VM_S390_UCONTROL) {
> >   		kvm->arch.gmap = NULL;
> >   		kvm->arch.mem_limit = KVM_S390_NO_MEM_LIMIT;
> > +		/* pre-initialize a bunch of memslots; the amount is arbitrary */
> > +		for (i = 0; i < 32; i++)
> > +			kvm_s390_ucontrol_ensure_memslot(kvm, i * UCONTROL_SLOT_SIZE);
> >   	} else {
> >   		if (sclp.hamax == U64_MAX)
> >   			kvm->arch.mem_limit = TASK_SIZE_MAX;
> > @@ -5704,6 +5725,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >   #ifdef CONFIG_KVM_S390_UCONTROL
> >   	case KVM_S390_UCAS_MAP: {
> >   		struct kvm_s390_ucas_mapping ucasmap;
> > +		unsigned long a;  
> 
> maybe addr?

yes

> 
> [...]
> 
> > @@ -5852,10 +5879,18 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
> >   				   struct kvm_memory_slot *new,
> >   				   enum kvm_mr_change change)
> >   {
> > -	gpa_t size;
> > +	gpa_t size = new->npages * PAGE_SIZE;
> >   
> > -	if (kvm_is_ucontrol(kvm))
> > -		return -EINVAL;  
> 
> Maybe add some comment here what and why we are checking those?

will do

> 
> > +	if (kvm_is_ucontrol(kvm)) {
> > +		if (change != KVM_MR_CREATE || new->flags)
> > +			return -EINVAL;
> > +		if (new->userspace_addr != new->base_gfn * PAGE_SIZE)
> > +			return -EINVAL;
> > +		if (!IS_ALIGNED(new->userspace_addr | size, UCONTROL_SLOT_SIZE))
> > +			return -EINVAL;
> > +		if (new->id != new->userspace_addr / UCONTROL_SLOT_SIZE)
> > +			return -EINVAL;
> > +	}
> >