| Message ID | 1543961190-31521-3-git-send-email-walling@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use DIAG318 to set Control Program Name & Version Codes | expand |
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?
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).
>>> + } >>> + 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.
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? [...]
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? > > [...] >
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);