Patchwork [v1,2/2] s390/kvm: handle diagnose 318 instruction call

login
register
mail settings
Submitter Collin Walling
Date Dec. 4, 2018, 10:06 p.m.
Message ID <1543961190-31521-3-git-send-email-walling@linux.ibm.com>
Download mbox | patch
Permalink /patch/672415/
State New
Headers show

Comments

Collin Walling - Dec. 4, 2018, 10:06 p.m.
Diagnose 318 is a privileged instruction that must be interpreted by 
SIE and handled via KVM.

The control program name and version codes (CPNC and CPVC) set by this
instruction are saved to the kvm->arch struct. The CPNC is also set in
the SIE control block of all VCPUs. The new kvm_s390_set_misc interface
is introduced for migration.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 13 +++++-
 arch/s390/include/uapi/asm/kvm.h |  5 +++
 arch/s390/kvm/diag.c             | 12 ++++++
 arch/s390/kvm/kvm-s390.c         | 88 ++++++++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h         |  1 +
 5 files changed, 118 insertions(+), 1 deletion(-)
David Hildenbrand - Dec. 5, 2018, 9:02 a.m.
On 04.12.18 23:06, Collin Walling wrote:
> Diagnose 318 is a privileged instruction that must be interpreted by 
> SIE and handled via KVM.
> 
> The control program name and version codes (CPNC and CPVC) set by this
> instruction are saved to the kvm->arch struct. The CPNC is also set in
> the SIE control block of all VCPUs. The new kvm_s390_set_misc interface
> is introduced for migration.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 13 +++++-
>  arch/s390/include/uapi/asm/kvm.h |  5 +++
>  arch/s390/kvm/diag.c             | 12 ++++++
>  arch/s390/kvm/kvm-s390.c         | 88 ++++++++++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.h         |  1 +
>  5 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index d5d2488..ad42949 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -229,7 +229,8 @@ struct kvm_s390_sie_block {
>  	__u32	scaol;			/* 0x0064 */
>  	__u8	reserved68;		/* 0x0068 */
>  	__u8    epdx;			/* 0x0069 */
> -	__u8    reserved6a[2];		/* 0x006a */
> +	__u8    cpnc;			/* 0x006a */
> +	__u8	reserved6b;		/* 0x006b */
>  	__u32	todpr;			/* 0x006c */
>  #define GISA_FORMAT1 0x00000001
>  	__u32	gd;			/* 0x0070 */
> @@ -391,6 +392,7 @@ struct kvm_vcpu_stat {
>  	u64 diagnose_9c;
>  	u64 diagnose_258;
>  	u64 diagnose_308;
> +	u64 diagnose_318;
>  	u64 diagnose_500;
>  	u64 diagnose_other;
>  };
> @@ -804,6 +806,14 @@ struct kvm_s390_vsie {
>  	struct page *pages[KVM_MAX_VCPUS];
>  };
>  
> +union kvm_s390_diag318_info {
> +	u64 cpc;
> +	struct {
> +		u64 cpnc : 8;
> +		u64 cpvc : 56;
> +	};
> +};
> +
>  struct kvm_arch{
>  	void *sca;
>  	int use_esca;
> @@ -838,6 +848,7 @@ struct kvm_arch{
>  	/* subset of available cpu features enabled by user space */
>  	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>  	struct kvm_s390_gisa *gisa;
> +	union kvm_s390_diag318_info diag318_info;
>  };
>  
>  #define KVM_HVA_ERR_BAD		(-1UL)
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 16511d9..bdbf4d8 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
>  #define KVM_S390_VM_CRYPTO		2
>  #define KVM_S390_VM_CPU_MODEL		3
>  #define KVM_S390_VM_MIGRATION		4
> +#define KVM_S390_VM_MISC		5

I somewhat dislike the category MISC. But I don't know of anything
better. Maybe "MACHINE", hmmm

>  
>  /* kvm attributes for mem_ctrl */
>  #define KVM_S390_VM_MEM_ENABLE_CMMA	0
> @@ -130,6 +131,7 @@ struct kvm_s390_vm_cpu_machine {
>  #define KVM_S390_VM_CPU_FEAT_PFMFI	11
>  #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
>  #define KVM_S390_VM_CPU_FEAT_KSS	13
> +#define KVM_S390_VM_CPU_FEAT_DIAG318	14
>  struct kvm_s390_vm_cpu_feat {
>  	__u64 feat[16];
>  };
> @@ -168,6 +170,9 @@ struct kvm_s390_vm_cpu_subfunc {
>  #define KVM_S390_VM_MIGRATION_START	1
>  #define KVM_S390_VM_MIGRATION_STATUS	2
>  
> +/* kvm attributes for KVM_S390_VM_MISC */
> +#define KVM_S390_VM_MISC_CPC		0
> +
>  /* for KVM_GET_REGS and KVM_SET_REGS */
>  struct kvm_regs {
>  	/* general purpose regs for s390 */
> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
> index 45634b3d..b61ffd2 100644
> --- a/arch/s390/kvm/diag.c
> +++ b/arch/s390/kvm/diag.c
> @@ -235,6 +235,16 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
>  	return ret < 0 ? ret : 0;
>  }
>  
> +static int __diag_set_control_prog_name(struct kvm_vcpu *vcpu)
> +{
> +	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
> +	unsigned long cpc = vcpu->run->s.regs.gprs[reg];
> +
> +	vcpu->stat.diagnose_318++;
> +	kvm_s390_set_cpc(vcpu->kvm, cpc);
> +	return 0;
> +}
> +
>  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
>  {
>  	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;
> @@ -254,6 +264,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
>  		return __diag_page_ref_service(vcpu);
>  	case 0x308:
>  		return __diag_ipl_functions(vcpu);
> +	case 0x318:
> +		return __diag_set_control_prog_name(vcpu);
>  	case 0x500:
>  		return __diag_virtio_hypercall(vcpu);
>  	default:
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fe24150..19d061b 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -157,6 +157,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  	{ "instruction_diag_9c", VCPU_STAT(diagnose_9c) },
>  	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
>  	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
> +	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
>  	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
>  	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
>  	{ NULL }
> @@ -371,6 +372,10 @@ static void kvm_s390_cpu_feat_init(void)
>  
>  	if (MACHINE_HAS_ESOP)
>  		allow_cpu_feat(KVM_S390_VM_CPU_FEAT_ESOP);
> +
> +	/* Enable DIAG318 guest support unconditionally */
> +	allow_cpu_feat(KVM_S390_VM_CPU_FEAT_DIAG318);
> +
>  	/*
>  	 * We need SIE support, ESOP (PROT_READ protection for gmap_shadow),
>  	 * 64bit SCAO (SCA passthrough) and IDTE (for gmap_shadow unshadowing).
> @@ -1173,6 +1178,71 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
>  	return ret;
>  }
>  
> +void kvm_s390_set_cpc(struct kvm *kvm, u64 cpc)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int i;
> +
> +	mutex_lock(&kvm->lock);
> +	kvm->arch.diag318_info.cpc = cpc;
> +
> +	VM_EVENT(kvm, 3, "SET: cpnc: 0x%x cpvc: 0x%llx",
> +		 (u8)kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;

No, that does not look completely right. The other VCPUs could be
running in the SIE. Can you update that via a sync request instead?

(after a VCPU updated the CPNC, other VCPUS could read for some time the
old value, as values in this part of the sie_block might be cached while
a CPU is in the SIE)

> +	}
> +	mutex_unlock(&kvm->lock);
> +}
> +
> +static int kvm_s390_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	int ret;
> +	u64 cpc;
> +
> +	switch (attr->attr) {
> +	case KVM_S390_VM_MISC_CPC:
> +		ret = -EFAULT;
> +		if (get_user(cpc, (u64 __user *)attr->addr))
> +			break;
> +		kvm_s390_set_cpc(kvm, cpc);
> +		ret = 0;
> +		break;
> +	default:
> +		ret = -ENXIO;
> +		break;
> +	}
> +	return ret;
> +}
> +
> +static int kvm_s390_get_cpc(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	u64 cpc = kvm->arch.diag318_info.cpc;

We could have a possible race with a guest VCPU here, but I guess we
don't care.

> +
> +	if (put_user(cpc, (u64 __user *)attr->addr))
> +		return -EFAULT;
> +
> +	VM_EVENT(kvm, 3, "QUERY: cpnc: 0x%x cpvc: 0x%llx",
> +		 (u8)kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
> +
> +	return 0;
> +}
> +
> +static int kvm_s390_get_misc(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	int ret;
> +
> +	switch (attr->attr) {
> +	case KVM_S390_VM_MISC_CPC:
> +		ret = kvm_s390_get_cpc(kvm, attr);
> +		break;
> +	default:
> +		ret = -ENXIO;
> +		break;
> +	}
> +	return ret;
> +}
> +
>  static int kvm_s390_set_processor(struct kvm *kvm, struct kvm_device_attr *attr)
>  {
>  	struct kvm_s390_vm_cpu_processor *proc;
> @@ -1435,6 +1505,9 @@ static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = kvm_s390_vm_set_migration(kvm, attr);
>  		break;
> +	case KVM_S390_VM_MISC:
> +		ret = kvm_s390_set_misc(kvm, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -1460,6 +1533,9 @@ static int kvm_s390_vm_get_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = kvm_s390_vm_get_migration(kvm, attr);
>  		break;
> +	case KVM_S390_VM_MISC:
> +		ret = kvm_s390_get_misc(kvm, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -1534,6 +1610,16 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = 0;
>  		break;
> +	case KVM_S390_VM_MISC:
> +		switch (attr->attr) {
> +		case KVM_S390_VM_MISC_CPC:
> +			ret = 0;
> +			break;
> +		default:
> +			ret = -ENXIO;
> +			break;
> +		}
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -2650,7 +2736,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  	preempt_disable();
>  	vcpu->arch.sie_block->epoch = vcpu->kvm->arch.epoch;
>  	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
> +	vcpu->arch.sie_block->cpnc = vcpu->kvm->arch.diag318_info.cpnc;
>  	preempt_enable();
> +
>  	mutex_unlock(&vcpu->kvm->lock);
>  	if (!kvm_is_ucontrol(vcpu->kvm)) {
>  		vcpu->arch.gmap = vcpu->kvm->arch.gmap;
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 1f6e36c..8137f15 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -281,6 +281,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
>  int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
>  
>  /* implemented in kvm-s390.c */
> +void kvm_s390_set_cpc(struct kvm *kvm, u64 cpc);
>  void kvm_s390_set_tod_clock(struct kvm *kvm,
>  			    const struct kvm_s390_vm_tod_clock *gtod);
>  long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
> 

Related to vSIE, don't we also have to forward the cpc value into the
shadow SCB?
Collin Walling - Dec. 5, 2018, 4:07 p.m.
On 12/5/18 4:02 AM, David Hildenbrand wrote:
> On 04.12.18 23:06, Collin Walling wrote:
>> Diagnose 318 is a privileged instruction that must be interpreted by 
>> SIE and handled via KVM.
>>
>> The control program name and version codes (CPNC and CPVC) set by this
>> instruction are saved to the kvm->arch struct. The CPNC is also set in
>> the SIE control block of all VCPUs. The new kvm_s390_set_misc interface
>> is introduced for migration.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  arch/s390/include/asm/kvm_host.h | 13 +++++-
>>  arch/s390/include/uapi/asm/kvm.h |  5 +++
>>  arch/s390/kvm/diag.c             | 12 ++++++
>>  arch/s390/kvm/kvm-s390.c         | 88 ++++++++++++++++++++++++++++++++++++++++
>>  arch/s390/kvm/kvm-s390.h         |  1 +
>>  5 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index d5d2488..ad42949 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -229,7 +229,8 @@ struct kvm_s390_sie_block {
>>  	__u32	scaol;			/* 0x0064 */
>>  	__u8	reserved68;		/* 0x0068 */
>>  	__u8    epdx;			/* 0x0069 */
>> -	__u8    reserved6a[2];		/* 0x006a */
>> +	__u8    cpnc;			/* 0x006a */
>> +	__u8	reserved6b;		/* 0x006b */
>>  	__u32	todpr;			/* 0x006c */
>>  #define GISA_FORMAT1 0x00000001
>>  	__u32	gd;			/* 0x0070 */
>> @@ -391,6 +392,7 @@ struct kvm_vcpu_stat {
>>  	u64 diagnose_9c;
>>  	u64 diagnose_258;
>>  	u64 diagnose_308;
>> +	u64 diagnose_318;
>>  	u64 diagnose_500;
>>  	u64 diagnose_other;
>>  };
>> @@ -804,6 +806,14 @@ struct kvm_s390_vsie {
>>  	struct page *pages[KVM_MAX_VCPUS];
>>  };
>>  
>> +union kvm_s390_diag318_info {
>> +	u64 cpc;
>> +	struct {
>> +		u64 cpnc : 8;
>> +		u64 cpvc : 56;
>> +	};
>> +};
>> +
>>  struct kvm_arch{
>>  	void *sca;
>>  	int use_esca;
>> @@ -838,6 +848,7 @@ struct kvm_arch{
>>  	/* subset of available cpu features enabled by user space */
>>  	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>>  	struct kvm_s390_gisa *gisa;
>> +	union kvm_s390_diag318_info diag318_info;
>>  };
>>  
>>  #define KVM_HVA_ERR_BAD		(-1UL)
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 16511d9..bdbf4d8 100644
>> --- a/arch/s390/include/uapi/asm/kvm.h
>> +++ b/arch/s390/include/uapi/asm/kvm.h
>> @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
>>  #define KVM_S390_VM_CRYPTO		2
>>  #define KVM_S390_VM_CPU_MODEL		3
>>  #define KVM_S390_VM_MIGRATION		4
>> +#define KVM_S390_VM_MISC		5
> 
> I somewhat dislike the category MISC. But I don't know of anything
> better. Maybe "MACHINE", hmmm
> 

Hard to say without being able to foresee into the future what other
data we might want to categorize...

"KVM_S390_VM_MACHINE" does sound better, but there's already a
"KVM_S390_VM_CPU_MACHINE" attribute. Hopefully that doesn't look confusing.

Let's see how it looks in v2?

>>  
>>  /* kvm attributes for mem_ctrl */
>>  #define KVM_S390_VM_MEM_ENABLE_CMMA	0
>> @@ -130,6 +131,7 @@ struct kvm_s390_vm_cpu_machine {
>>  #define KVM_S390_VM_CPU_FEAT_PFMFI	11
>>  #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
>>  #define KVM_S390_VM_CPU_FEAT_KSS	13
>> +#define KVM_S390_VM_CPU_FEAT_DIAG318	14
>>  struct kvm_s390_vm_cpu_feat {
>>  	__u64 feat[16];
>>  };
>> @@ -168,6 +170,9 @@ struct kvm_s390_vm_cpu_subfunc {
>>  #define KVM_S390_VM_MIGRATION_START	1
>>  #define KVM_S390_VM_MIGRATION_STATUS	2
>>  
>> +/* kvm attributes for KVM_S390_VM_MISC */
>> +#define KVM_S390_VM_MISC_CPC		0
>> +
>>  /* for KVM_GET_REGS and KVM_SET_REGS */
>>  struct kvm_regs {
>>  	/* general purpose regs for s390 */
>> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
>> index 45634b3d..b61ffd2 100644
>> --- a/arch/s390/kvm/diag.c
>> +++ b/arch/s390/kvm/diag.c
>> @@ -235,6 +235,16 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
>>  	return ret < 0 ? ret : 0;
>>  }
>>  
>> +static int __diag_set_control_prog_name(struct kvm_vcpu *vcpu)
>> +{
>> +	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
>> +	unsigned long cpc = vcpu->run->s.regs.gprs[reg];
>> +
>> +	vcpu->stat.diagnose_318++;
>> +	kvm_s390_set_cpc(vcpu->kvm, cpc);
>> +	return 0;
>> +}
>> +
>>  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
>>  {
>>  	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;
>> @@ -254,6 +264,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
>>  		return __diag_page_ref_service(vcpu);
>>  	case 0x308:
>>  		return __diag_ipl_functions(vcpu);
>> +	case 0x318:
>> +		return __diag_set_control_prog_name(vcpu);
>>  	case 0x500:
>>  		return __diag_virtio_hypercall(vcpu);
>>  	default:
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index fe24150..19d061b 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -157,6 +157,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>>  	{ "instruction_diag_9c", VCPU_STAT(diagnose_9c) },
>>  	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
>>  	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
>> +	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
>>  	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
>>  	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
>>  	{ NULL }
>> @@ -371,6 +372,10 @@ static void kvm_s390_cpu_feat_init(void)
>>  
>>  	if (MACHINE_HAS_ESOP)
>>  		allow_cpu_feat(KVM_S390_VM_CPU_FEAT_ESOP);
>> +
>> +	/* Enable DIAG318 guest support unconditionally */
>> +	allow_cpu_feat(KVM_S390_VM_CPU_FEAT_DIAG318);
>> +
>>  	/*
>>  	 * We need SIE support, ESOP (PROT_READ protection for gmap_shadow),
>>  	 * 64bit SCAO (SCA passthrough) and IDTE (for gmap_shadow unshadowing).
>> @@ -1173,6 +1178,71 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
>>  	return ret;
>>  }
>>  
>> +void kvm_s390_set_cpc(struct kvm *kvm, u64 cpc)
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	int i;
>> +
>> +	mutex_lock(&kvm->lock);
>> +	kvm->arch.diag318_info.cpc = cpc;
>> +
>> +	VM_EVENT(kvm, 3, "SET: cpnc: 0x%x cpvc: 0x%llx",
>> +		 (u8)kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
> 
> No, that does not look completely right. The other VCPUs could be
> running in the SIE. Can you update that via a sync request instead?
> 
> (after a VCPU updated the CPNC, other VCPUS could read for some time the
> old value, as values in this part of the sie_block might be cached while
> a CPU is in the SIE)
> 

True. There are different name codes for other control programs (v/ZM, z/OS, etc).
We should make sure that the name code is properly updated for all VCPUs if we are
running a non-linux environment on top of KVM.

Thanks for the heads up! 

>> +	}
>> +	mutex_unlock(&kvm->lock);
>> +}
>> +
>> +static int kvm_s390_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	int ret;
>> +	u64 cpc;
>> +
>> +	switch (attr->attr) {
>> +	case KVM_S390_VM_MISC_CPC:
>> +		ret = -EFAULT;
>> +		if (get_user(cpc, (u64 __user *)attr->addr))
>> +			break;
>> +		kvm_s390_set_cpc(kvm, cpc);
>> +		ret = 0;
>> +		break;
>> +	default:
>> +		ret = -ENXIO;
>> +		break;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int kvm_s390_get_cpc(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	u64 cpc = kvm->arch.diag318_info.cpc;
> 
> We could have a possible race with a guest VCPU here, but I guess we
> don't care.
> 

Better safe than sorry? I'll see how another sync request looks here.

>> +
>> +	if (put_user(cpc, (u64 __user *)attr->addr))
>> +		return -EFAULT;
>> +
>> +	VM_EVENT(kvm, 3, "QUERY: cpnc: 0x%x cpvc: 0x%llx",
>> +		 (u8)kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
>> +
>> +	return 0;
>> +}
>> +
>> +static int kvm_s390_get_misc(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	int ret;
>> +
>> +	switch (attr->attr) {
>> +	case KVM_S390_VM_MISC_CPC:
>> +		ret = kvm_s390_get_cpc(kvm, attr);
>> +		break;
>> +	default:
>> +		ret = -ENXIO;
>> +		break;
>> +	}
>> +	return ret;
>> +}
>> +
>>  static int kvm_s390_set_processor(struct kvm *kvm, struct kvm_device_attr *attr)
>>  {
>>  	struct kvm_s390_vm_cpu_processor *proc;
>> @@ -1435,6 +1505,9 @@ static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>  	case KVM_S390_VM_MIGRATION:
>>  		ret = kvm_s390_vm_set_migration(kvm, attr);
>>  		break;
>> +	case KVM_S390_VM_MISC:
>> +		ret = kvm_s390_set_misc(kvm, attr);
>> +		break;
>>  	default:
>>  		ret = -ENXIO;
>>  		break;
>> @@ -1460,6 +1533,9 @@ static int kvm_s390_vm_get_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>  	case KVM_S390_VM_MIGRATION:
>>  		ret = kvm_s390_vm_get_migration(kvm, attr);
>>  		break;
>> +	case KVM_S390_VM_MISC:
>> +		ret = kvm_s390_get_misc(kvm, attr);
>> +		break;
>>  	default:
>>  		ret = -ENXIO;
>>  		break;
>> @@ -1534,6 +1610,16 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>  	case KVM_S390_VM_MIGRATION:
>>  		ret = 0;
>>  		break;
>> +	case KVM_S390_VM_MISC:
>> +		switch (attr->attr) {
>> +		case KVM_S390_VM_MISC_CPC:
>> +			ret = 0;
>> +			break;
>> +		default:
>> +			ret = -ENXIO;
>> +			break;
>> +		}
>> +		break;
>>  	default:
>>  		ret = -ENXIO;
>>  		break;
>> @@ -2650,7 +2736,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>>  	preempt_disable();
>>  	vcpu->arch.sie_block->epoch = vcpu->kvm->arch.epoch;
>>  	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
>> +	vcpu->arch.sie_block->cpnc = vcpu->kvm->arch.diag318_info.cpnc;
>>  	preempt_enable();
>> +
>>  	mutex_unlock(&vcpu->kvm->lock);
>>  	if (!kvm_is_ucontrol(vcpu->kvm)) {
>>  		vcpu->arch.gmap = vcpu->kvm->arch.gmap;
>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>> index 1f6e36c..8137f15 100644
>> --- a/arch/s390/kvm/kvm-s390.h
>> +++ b/arch/s390/kvm/kvm-s390.h
>> @@ -281,6 +281,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
>>  int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
>>  
>>  /* implemented in kvm-s390.c */
>> +void kvm_s390_set_cpc(struct kvm *kvm, u64 cpc);
>>  void kvm_s390_set_tod_clock(struct kvm *kvm,
>>  			    const struct kvm_s390_vm_tod_clock *gtod);
>>  long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
>>
> 
> Related to vSIE, don't we also have to forward the cpc value into the
> shadow SCB?
> 

Yes, the name code should certainly be forwarded into the shadow SCB. (The version
code is stored in the vcpu, but SIE doesn't care about that value anyway).
David Hildenbrand - Dec. 5, 2018, 4:37 p.m.
>>> +	}
>>> +	mutex_unlock(&kvm->lock);
>>> +}
>>> +
>>> +static int kvm_s390_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
>>> +{
>>> +	int ret;
>>> +	u64 cpc;
>>> +
>>> +	switch (attr->attr) {
>>> +	case KVM_S390_VM_MISC_CPC:
>>> +		ret = -EFAULT;
>>> +		if (get_user(cpc, (u64 __user *)attr->addr))
>>> +			break;
>>> +		kvm_s390_set_cpc(kvm, cpc);
>>> +		ret = 0;
>>> +		break;
>>> +	default:
>>> +		ret = -ENXIO;
>>> +		break;
>>> +	}
>>> +	return ret;
>>> +}
>>> +
>>> +static int kvm_s390_get_cpc(struct kvm *kvm, struct kvm_device_attr *attr)
>>> +{
>>> +	u64 cpc = kvm->arch.diag318_info.cpc;
>>
>> We could have a possible race with a guest VCPU here, but I guess we
>> don't care.
>>
> 
> Better safe than sorry? I'll see how another sync request looks here.
> 

Sync requests most probably don't make sense here.

I guess we can leave the code as is. User space won't be able to
recognize the difference either way (as long as the cpc is updated in
one shot). A VCPU could update just before or just after the call.
Collin Walling - Dec. 5, 2018, 10:46 p.m.
On 12/5/18 11:07 AM, Collin Walling wrote:
> On 12/5/18 4:02 AM, David Hildenbrand wrote:
>> On 04.12.18 23:06, Collin Walling wrote:
>>> Diagnose 318 is a privileged instruction that must be interpreted by 
>>> SIE and handled via KVM.
>>>
>>> The control program name and version codes (CPNC and CPVC) set by this
>>> instruction are saved to the kvm->arch struct. The CPNC is also set in
>>> the SIE control block of all VCPUs. The new kvm_s390_set_misc interface
>>> is introduced for migration.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---

[...]

>>>  
>>> +void kvm_s390_set_cpc(struct kvm *kvm, u64 cpc)
>>> +{
>>> +	struct kvm_vcpu *vcpu;
>>> +	int i;
>>> +
>>> +	mutex_lock(&kvm->lock);
>>> +	kvm->arch.diag318_info.cpc = cpc;
>>> +
>>> +	VM_EVENT(kvm, 3, "SET: cpnc: 0x%x cpvc: 0x%llx",
>>> +		 (u8)kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
>>> +
>>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>>> +		vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
>>
>> No, that does not look completely right. The other VCPUs could be
>> running in the SIE. Can you update that via a sync request instead?
>>
>> (after a VCPU updated the CPNC, other VCPUS could read for some time the
>> old value, as values in this part of the sie_block might be cached while
>> a CPU is in the SIE)
>>
> 
> True. There are different name codes for other control programs (v/ZM, z/OS, etc).
> We should make sure that the name code is properly updated for all VCPUs if we are
> running a non-linux environment on top of KVM.
> 
> Thanks for the heads up! 
> 

I've been reading up on what the other requests (KVM_REQ_*) are used for, and none of them
seem to make sense for what we want to do here (update a field in the vcpu). So let's see 
if I fully understand how this should be implemented: 

1) create a new s390 specific request that will handle updating the cpc
2) call a sync_broadcast using the new request within set_cpc

Alternatively, I think a vcpu block would work as well (you mentioned this in the RFC back 
at the end of August / beginning of September).

Am I headed in the right direction?

[...]
David Hildenbrand - Dec. 6, 2018, 8:25 a.m.
On 05.12.18 23:46, Collin Walling wrote:
> On 12/5/18 11:07 AM, Collin Walling wrote:
>> On 12/5/18 4:02 AM, David Hildenbrand wrote:
>>> On 04.12.18 23:06, Collin Walling wrote:
>>>> Diagnose 318 is a privileged instruction that must be interpreted by 
>>>> SIE and handled via KVM.
>>>>
>>>> The control program name and version codes (CPNC and CPVC) set by this
>>>> instruction are saved to the kvm->arch struct. The CPNC is also set in
>>>> the SIE control block of all VCPUs. The new kvm_s390_set_misc interface
>>>> is introduced for migration.
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
> 
> [...]
> 
>>>>  
>>>> +void kvm_s390_set_cpc(struct kvm *kvm, u64 cpc)
>>>> +{
>>>> +	struct kvm_vcpu *vcpu;
>>>> +	int i;
>>>> +
>>>> +	mutex_lock(&kvm->lock);
>>>> +	kvm->arch.diag318_info.cpc = cpc;
>>>> +
>>>> +	VM_EVENT(kvm, 3, "SET: cpnc: 0x%x cpvc: 0x%llx",
>>>> +		 (u8)kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
>>>> +
>>>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>>>> +		vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
>>>
>>> No, that does not look completely right. The other VCPUs could be
>>> running in the SIE. Can you update that via a sync request instead?
>>>
>>> (after a VCPU updated the CPNC, other VCPUS could read for some time the
>>> old value, as values in this part of the sie_block might be cached while
>>> a CPU is in the SIE)
>>>
>>
>> True. There are different name codes for other control programs (v/ZM, z/OS, etc).
>> We should make sure that the name code is properly updated for all VCPUs if we are
>> running a non-linux environment on top of KVM.
>>
>> Thanks for the heads up! 
>>
> 
> I've been reading up on what the other requests (KVM_REQ_*) are used for, and none of them
> seem to make sense for what we want to do here (update a field in the vcpu). So let's see 
> if I fully understand how this should be implemented: 
> 
> 1) create a new s390 specific request that will handle updating the cpc
> 2) call a sync_broadcast using the new request within set_cpc
> 
> Alternatively, I think a vcpu block would work as well (you mentioned this in the RFC back 
> at the end of August / beginning of September).

Yes it would as well. E.g. see kvm_arch_crypto_clear_masks()

mutex_lock(&kvm->lock);
kvm_s390_vcpu_block_all(kvm);

/* do stuff to other VCPUs */

kvm_s390_vcpu_unblock_all(kvm);
mutex_unlock(&kvm->lock);

> 
> Am I headed in the right direction?
> 
> [...]
>

Patch

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index d5d2488..ad42949 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -229,7 +229,8 @@  struct kvm_s390_sie_block {
 	__u32	scaol;			/* 0x0064 */
 	__u8	reserved68;		/* 0x0068 */
 	__u8    epdx;			/* 0x0069 */
-	__u8    reserved6a[2];		/* 0x006a */
+	__u8    cpnc;			/* 0x006a */
+	__u8	reserved6b;		/* 0x006b */
 	__u32	todpr;			/* 0x006c */
 #define GISA_FORMAT1 0x00000001
 	__u32	gd;			/* 0x0070 */
@@ -391,6 +392,7 @@  struct kvm_vcpu_stat {
 	u64 diagnose_9c;
 	u64 diagnose_258;
 	u64 diagnose_308;
+	u64 diagnose_318;
 	u64 diagnose_500;
 	u64 diagnose_other;
 };
@@ -804,6 +806,14 @@  struct kvm_s390_vsie {
 	struct page *pages[KVM_MAX_VCPUS];
 };
 
+union kvm_s390_diag318_info {
+	u64 cpc;
+	struct {
+		u64 cpnc : 8;
+		u64 cpvc : 56;
+	};
+};
+
 struct kvm_arch{
 	void *sca;
 	int use_esca;
@@ -838,6 +848,7 @@  struct kvm_arch{
 	/* subset of available cpu features enabled by user space */
 	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	struct kvm_s390_gisa *gisa;
+	union kvm_s390_diag318_info diag318_info;
 };
 
 #define KVM_HVA_ERR_BAD		(-1UL)
diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 16511d9..bdbf4d8 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -74,6 +74,7 @@  struct kvm_s390_io_adapter_req {
 #define KVM_S390_VM_CRYPTO		2
 #define KVM_S390_VM_CPU_MODEL		3
 #define KVM_S390_VM_MIGRATION		4
+#define KVM_S390_VM_MISC		5
 
 /* kvm attributes for mem_ctrl */
 #define KVM_S390_VM_MEM_ENABLE_CMMA	0
@@ -130,6 +131,7 @@  struct kvm_s390_vm_cpu_machine {
 #define KVM_S390_VM_CPU_FEAT_PFMFI	11
 #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
 #define KVM_S390_VM_CPU_FEAT_KSS	13
+#define KVM_S390_VM_CPU_FEAT_DIAG318	14
 struct kvm_s390_vm_cpu_feat {
 	__u64 feat[16];
 };
@@ -168,6 +170,9 @@  struct kvm_s390_vm_cpu_subfunc {
 #define KVM_S390_VM_MIGRATION_START	1
 #define KVM_S390_VM_MIGRATION_STATUS	2
 
+/* kvm attributes for KVM_S390_VM_MISC */
+#define KVM_S390_VM_MISC_CPC		0
+
 /* for KVM_GET_REGS and KVM_SET_REGS */
 struct kvm_regs {
 	/* general purpose regs for s390 */
diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
index 45634b3d..b61ffd2 100644
--- a/arch/s390/kvm/diag.c
+++ b/arch/s390/kvm/diag.c
@@ -235,6 +235,16 @@  static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 
+static int __diag_set_control_prog_name(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	unsigned long cpc = vcpu->run->s.regs.gprs[reg];
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_cpc(vcpu->kvm, cpc);
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;
@@ -254,6 +264,8 @@  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_control_prog_name(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);
 	default:
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index fe24150..19d061b 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -157,6 +157,7 @@  struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "instruction_diag_9c", VCPU_STAT(diagnose_9c) },
 	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
 	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
+	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
 	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
 	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
 	{ NULL }
@@ -371,6 +372,10 @@  static void kvm_s390_cpu_feat_init(void)
 
 	if (MACHINE_HAS_ESOP)
 		allow_cpu_feat(KVM_S390_VM_CPU_FEAT_ESOP);
+
+	/* Enable DIAG318 guest support unconditionally */
+	allow_cpu_feat(KVM_S390_VM_CPU_FEAT_DIAG318);
+
 	/*
 	 * We need SIE support, ESOP (PROT_READ protection for gmap_shadow),
 	 * 64bit SCAO (SCA passthrough) and IDTE (for gmap_shadow unshadowing).
@@ -1173,6 +1178,71 @@  static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+void kvm_s390_set_cpc(struct kvm *kvm, u64 cpc)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	mutex_lock(&kvm->lock);
+	kvm->arch.diag318_info.cpc = cpc;
+
+	VM_EVENT(kvm, 3, "SET: cpnc: 0x%x cpvc: 0x%llx",
+		 (u8)kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
+
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
+	}
+	mutex_unlock(&kvm->lock);
+}
+
+static int kvm_s390_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+	u64 cpc;
+
+	switch (attr->attr) {
+	case KVM_S390_VM_MISC_CPC:
+		ret = -EFAULT;
+		if (get_user(cpc, (u64 __user *)attr->addr))
+			break;
+		kvm_s390_set_cpc(kvm, cpc);
+		ret = 0;
+		break;
+	default:
+		ret = -ENXIO;
+		break;
+	}
+	return ret;
+}
+
+static int kvm_s390_get_cpc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	u64 cpc = kvm->arch.diag318_info.cpc;
+
+	if (put_user(cpc, (u64 __user *)attr->addr))
+		return -EFAULT;
+
+	VM_EVENT(kvm, 3, "QUERY: cpnc: 0x%x cpvc: 0x%llx",
+		 (u8)kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
+
+	return 0;
+}
+
+static int kvm_s390_get_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+
+	switch (attr->attr) {
+	case KVM_S390_VM_MISC_CPC:
+		ret = kvm_s390_get_cpc(kvm, attr);
+		break;
+	default:
+		ret = -ENXIO;
+		break;
+	}
+	return ret;
+}
+
 static int kvm_s390_set_processor(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	struct kvm_s390_vm_cpu_processor *proc;
@@ -1435,6 +1505,9 @@  static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = kvm_s390_vm_set_migration(kvm, attr);
 		break;
+	case KVM_S390_VM_MISC:
+		ret = kvm_s390_set_misc(kvm, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -1460,6 +1533,9 @@  static int kvm_s390_vm_get_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = kvm_s390_vm_get_migration(kvm, attr);
 		break;
+	case KVM_S390_VM_MISC:
+		ret = kvm_s390_get_misc(kvm, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -1534,6 +1610,16 @@  static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = 0;
 		break;
+	case KVM_S390_VM_MISC:
+		switch (attr->attr) {
+		case KVM_S390_VM_MISC_CPC:
+			ret = 0;
+			break;
+		default:
+			ret = -ENXIO;
+			break;
+		}
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -2650,7 +2736,9 @@  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 	preempt_disable();
 	vcpu->arch.sie_block->epoch = vcpu->kvm->arch.epoch;
 	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
+	vcpu->arch.sie_block->cpnc = vcpu->kvm->arch.diag318_info.cpnc;
 	preempt_enable();
+
 	mutex_unlock(&vcpu->kvm->lock);
 	if (!kvm_is_ucontrol(vcpu->kvm)) {
 		vcpu->arch.gmap = vcpu->kvm->arch.gmap;
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 1f6e36c..8137f15 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -281,6 +281,7 @@  int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
 
 /* implemented in kvm-s390.c */
+void kvm_s390_set_cpc(struct kvm *kvm, u64 cpc);
 void kvm_s390_set_tod_clock(struct kvm *kvm,
 			    const struct kvm_s390_vm_tod_clock *gtod);
 long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);