Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752826AbeAQLja (ORCPT + 1 other); Wed, 17 Jan 2018 06:39:30 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:50966 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752625AbeAQLj3 (ORCPT ); Wed, 17 Jan 2018 06:39:29 -0500 Subject: Re: [PATCH 5/6] KVM: s390: wire up seb feature To: David Hildenbrand , Martin Schwidefsky , linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org, kvm@vger.kernel.org Cc: Heiko Carstens , Paolo Bonzini , Cornelia Huck , Greg Kroah-Hartman , Jon Masters , Marcus Meissner , Jiri Kosina References: <1516182519-10623-1-git-send-email-schwidefsky@de.ibm.com> <1516182519-10623-6-git-send-email-schwidefsky@de.ibm.com> <2fa06836-75f8-9c3f-a5e3-217763f6ff4e@redhat.com> From: Christian Borntraeger Date: Wed, 17 Jan 2018 12:39:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <2fa06836-75f8-9c3f-a5e3-217763f6ff4e@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 18011711-0040-0000-0000-00000405BCF5 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18011711-0041-0000-0000-000026092D4C Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-17_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1801170169 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 01/17/2018 12:33 PM, David Hildenbrand wrote: > >> #define ECB_GS 0x40 >> #define ECB_TE 0x10 >> #define ECB_SRSI 0x04 >> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h >> index 38535a57..20b9e9f 100644 >> --- a/arch/s390/include/uapi/asm/kvm.h >> +++ b/arch/s390/include/uapi/asm/kvm.h >> @@ -224,6 +224,7 @@ struct kvm_guest_debug_arch { >> #define KVM_SYNC_RICCB (1UL << 7) >> #define KVM_SYNC_FPRS (1UL << 8) >> #define KVM_SYNC_GSCB (1UL << 9) >> +#define KVM_SYNC_SEBC (1UL << 10) >> /* length and alignment of the sdnx as a power of two */ >> #define SDNXC 8 >> #define SDNXL (1UL << SDNXC) >> @@ -247,7 +248,8 @@ struct kvm_sync_regs { >> }; >> __u8 reserved[512]; /* for future vector expansion */ >> __u32 fpc; /* valid on KVM_SYNC_VRS or KVM_SYNC_FPRS */ >> - __u8 padding1[52]; /* riccb needs to be 64byte aligned */ >> + __u8 sebc:1; /* spec blocking */ > > do you want to define the unused bits as reserved? Nicer to read IMHO I certainly want to have these bits for future use. So maybe a __u8 reserved : 7; after that makes a lot of sense. Also the sebc : 1; (spaces) (FWIW, I will rename that to bpbc for other reasons). > > (especially also using spaces "sebc : 1") > >> + __u8 padding1[51]; /* riccb needs to be 64byte aligned */ >> __u8 riccb[64]; /* runtime instrumentation controls block */ >> __u8 padding2[192]; /* sdnx needs to be 256byte aligned */ >> union { >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 2c93cbb..0c18f73 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -421,6 +421,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) >> case KVM_CAP_S390_GS: >> r = test_facility(133); >> break; >> + case KVM_CAP_S390_SEB: >> + r = test_facility(82); >> + break; >> default: >> r = 0; >> } >> @@ -2198,6 +2201,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) >> kvm_s390_set_prefix(vcpu, 0); >> if (test_kvm_facility(vcpu->kvm, 64)) >> vcpu->run->kvm_valid_regs |= KVM_SYNC_RICCB; >> + if (test_kvm_facility(vcpu->kvm, 82)) >> + vcpu->run->kvm_valid_regs |= KVM_SYNC_SEBC; >> if (test_kvm_facility(vcpu->kvm, 133)) >> vcpu->run->kvm_valid_regs |= KVM_SYNC_GSCB; >> /* fprs can be synchronized via vrs, even if the guest has no vx. With >> @@ -2339,6 +2344,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) >> current->thread.fpu.fpc = 0; >> vcpu->arch.sie_block->gbea = 1; >> vcpu->arch.sie_block->pp = 0; >> + vcpu->arch.sie_block->fpf &= ~FPF_SEBC; >> vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID; >> kvm_clear_async_pf_completion_queue(vcpu); >> if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm)) >> @@ -3298,6 +3304,10 @@ static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) >> vcpu->arch.sie_block->ecd |= ECD_HOSTREGMGMT; >> vcpu->arch.gs_enabled = 1; >> } >> + if (kvm_run->kvm_dirty_regs & KVM_SYNC_SEBC) { > > We should test for test_facility(82). Otherwise user space can enable > undefined bits in the SCB on machines with !facility 82. Agreed, will fix. > >> + vcpu->arch.sie_block->fpf &= ~FPF_SEBC; >> + vcpu->arch.sie_block->fpf |= kvm_run->s.regs.sebc ? FPF_SEBC : 0; >> + } >> save_access_regs(vcpu->arch.host_acrs); >> restore_access_regs(vcpu->run->s.regs.acrs); >> /* save host (userspace) fprs/vrs */ >> @@ -3344,6 +3354,7 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) >> kvm_run->s.regs.pft = vcpu->arch.pfault_token; >> kvm_run->s.regs.pfs = vcpu->arch.pfault_select; >> kvm_run->s.regs.pfc = vcpu->arch.pfault_compare; >> + kvm_run->s.regs.sebc = (vcpu->arch.sie_block->fpf & FPF_SEBC) == FPF_SEBC; >> save_access_regs(vcpu->run->s.regs.acrs); >> restore_access_regs(vcpu->arch.host_acrs); >> /* Save guest register state */ >> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c >> index 5d6ae03..10ea208 100644 >> --- a/arch/s390/kvm/vsie.c >> +++ b/arch/s390/kvm/vsie.c >> @@ -223,6 +223,10 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >> memcpy(scb_o->gcr, scb_s->gcr, 128); >> scb_o->pp = scb_s->pp; >> >> + /* speculative blocking */ > > This field should only be written back with test_kvm_facility(vcpu->kvm, 82) Agreed. > > (no public documentation, this looks like the SIE can modify this field? > Triggered by which instruction?) The instruction from patch 3. > >> + scb_o->fpf &= ~FPF_SEBC; >> + scb_o->fpf |= scb_s->fpf & FPF_SEBC; >> + >> /* interrupt intercept */ >> switch (scb_s->icptcode) { >> case ICPT_PROGI: >> @@ -265,6 +269,7 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >> scb_s->ecb3 = 0; >> scb_s->ecd = 0; >> scb_s->fac = 0; >> + scb_s->fpf = 0; >> >> rc = prepare_cpuflags(vcpu, vsie_page); >> if (rc) >> @@ -324,6 +329,9 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) >> prefix_unmapped(vsie_page); >> scb_s->ecb |= scb_o->ecb & ECB_TE; >> } >> + /* speculative blocking */ >> + if (test_kvm_facility(vcpu->kvm, 82)) >> + scb_s->fpf |= scb_o->fpf & FPF_SEBC; >> /* SIMD */ >> if (test_kvm_facility(vcpu->kvm, 129)) { >> scb_s->eca |= scb_o->eca & ECA_VX; > Thanks for the quick review.