Re: [PATCH v1 2/2] s390/kvm: handle diagnose 318 instruction call

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx>
>> Reviewed-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> ---
>>  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).

-- 
Respectfully,
- Collin Walling




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux