Re: [RFCv2 05/37] s390/mm: provide memory management functions for protected KVM guests

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 04.02.20 11:57, Cornelia Huck wrote:

>>  
>>  /* Generic bits for GMAP notification on DAT table entry changes. */
>> @@ -61,6 +62,7 @@ struct gmap {
>>  	spinlock_t shadow_lock;
>>  	struct gmap *parent;
>>  	unsigned long orig_asce;
>> +	unsigned long se_handle;
> 
> This is a deviation from the "protected virtualization" naming scheme
> used in the previous patches, but I understand that the naming of this
> whole feature is still in flux :) (Would still be nice to have it
> consistent, though.)
> 
> However, I think I'd prefer something named based on protected
> virtualization: the se_* stuff here just tends to make me think of
> SELinux...

I will just use guest_handle as this guests assigned to and from such:

arch/s390/include/asm/gmap.h:   unsigned long se_handle;
arch/s390/include/asm/uv.h:             .guest_handle = gmap->se_handle,
arch/s390/kvm/pv.c:     WRITE_ONCE(kvm->arch.gmap->se_handle, 0);
arch/s390/kvm/pv.c:     kvm->arch.gmap->se_handle = uvcb.guest_handle;
arch/s390/mm/fault.c:           .guest_handle = gmap->se_handle,


> 
>>  	int edat_level;
>>  	bool removed;
>>  	bool initialized;
>> diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h
>> index bcfb6371086f..984026cb3608 100644
>> --- a/arch/s390/include/asm/mmu.h
>> +++ b/arch/s390/include/asm/mmu.h
>> @@ -16,6 +16,8 @@ typedef struct {
>>  	unsigned long asce;
>>  	unsigned long asce_limit;
>>  	unsigned long vdso_base;
>> +	/* The mmu context belongs to a secure guest. */
>> +	atomic_t is_se;
> 
> Here as well.

I will use is "is_protected"
> 
>>  	/*
>>  	 * The following bitfields need a down_write on the mm
>>  	 * semaphore when they are written to. As they are only
> 
> (...)
> 
>> @@ -520,6 +521,15 @@ static inline int mm_has_pgste(struct mm_struct *mm)
>>  	return 0;
>>  }
>>  
>> +static inline int mm_is_se(struct mm_struct *mm)
>> +{
>> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
>> +	if (unlikely(atomic_read(&mm->context.is_se)))
>> +		return 1;
>> +#endif
>> +	return 0;
>> +}
> 
> I'm wondering how big of an overhead we actually have because we need
> to check the flag here if the feature is enabled. We have an extra
> check in a few functions anyway, even if protected virt is not
> configured in. Given that distributions would likely want to enable the
> feature in their kernels, I'm currently tending towards dropping the
> extra config option.

Makes sense. I will wait a bit more but if you do not disagree I will remove 
the extra host config and bind things to CONFIG_KVM or CONFIG_PGSTE depending
on context

> 
>> +
>>  static inline int mm_alloc_pgste(struct mm_struct *mm)
>>  {
>>  #ifdef CONFIG_PGSTE
> 
> (...)
> 
>> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
>> index f7778493e829..136c60a8e3ca 100644
>> --- a/arch/s390/kernel/uv.c
>> +++ b/arch/s390/kernel/uv.c
>> @@ -9,6 +9,8 @@
>>  #include <linux/sizes.h>
>>  #include <linux/bitmap.h>
>>  #include <linux/memblock.h>
>> +#include <linux/pagemap.h>
>> +#include <linux/swap.h>
>>  #include <asm/facility.h>
>>  #include <asm/sections.h>
>>  #include <asm/uv.h>
>> @@ -98,4 +100,172 @@ void adjust_to_uv_max(unsigned long *vmax)
>>  	if (prot_virt_host && *vmax > uv_info.max_sec_stor_addr)
>>  		*vmax = uv_info.max_sec_stor_addr;
>>  }
>> +
>> +static int __uv_pin_shared(unsigned long paddr)
>> +{
>> +	struct uv_cb_cfs uvcb = {
>> +		.header.cmd	= UVC_CMD_PIN_PAGE_SHARED,
>> +		.header.len	= sizeof(uvcb),
>> +		.paddr		= paddr,
>> +	};
>> +
>> +	if (uv_call(0, (u64)&uvcb))
>> +		return -EINVAL;
>> +	return 0;
> 
> I guess this call won't set an error rc in the control block, if it
> does not set a condition code != 0?

Right. RC is only set for CC!=0