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
