Messages in this thread | | | Date | Tue, 26 Apr 2022 15:39:14 +0200 | Subject | Re: [PATCH v2 1/2] KVM: s390: Don't indicate suppression on dirtying, failing memop | From | Janosch Frank <> |
| |
On 4/26/22 15:25, Janis Schoetterl-Glausch wrote: > On 4/26/22 09:18, Janosch Frank wrote: >> On 4/25/22 12:01, Janis Schoetterl-Glausch wrote: >>> If user space uses a memop to emulate an instruction and that >>> memop fails, the execution of the instruction ends. >>> Instruction execution can end in different ways, one of which is >>> suppression, which requires that the instruction execute like a no-op. >> >> >> >>> A writing memop that spans multiple pages and fails due to key >>> protection can modified guest memory, as a result, the likely >>> correct ending is termination. Therefore do not indicate a >>> suppressing instruction ending in this case. >> >> Check grammar. >> >>> >>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> >>> --- >>> arch/s390/kvm/gaccess.c | 47 ++++++++++++++++++++++++----------------- >>> 1 file changed, 28 insertions(+), 19 deletions(-) >>> >>> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c >>> index d53a183c2005..3b1fbef82288 100644 >>> --- a/arch/s390/kvm/gaccess.c >>> +++ b/arch/s390/kvm/gaccess.c >>> @@ -491,8 +491,8 @@ enum prot_type { >>> PROT_TYPE_IEP = 4, >>> }; >>> -static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, >>> - u8 ar, enum gacc_mode mode, enum prot_type prot) >>> +static int trans_exc_ending(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar, >>> + enum gacc_mode mode, enum prot_type prot, bool suppress) >>> { >>> struct kvm_s390_pgm_info *pgm = &vcpu->arch.pgm; >>> struct trans_exc_code_bits *tec; >>> @@ -503,22 +503,24 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, >>> switch (code) { >>> case PGM_PROTECTION: >>> - switch (prot) { >>> - case PROT_TYPE_IEP: >>> - tec->b61 = 1; >>> - fallthrough; >>> - case PROT_TYPE_LA: >>> - tec->b56 = 1; >>> - break; >>> - case PROT_TYPE_KEYC: >>> - tec->b60 = 1; >>> - break; >>> - case PROT_TYPE_ALC: >>> - tec->b60 = 1; >>> - fallthrough; >>> - case PROT_TYPE_DAT: >>> - tec->b61 = 1; >>> - break; >>> + if (suppress) { >>> + switch (prot) { >>> + case PROT_TYPE_IEP: >>> + tec->b61 = 1; >>> + fallthrough; >>> + case PROT_TYPE_LA: >>> + tec->b56 = 1; >>> + break; >>> + case PROT_TYPE_KEYC: >>> + tec->b60 = 1; >>> + break; >>> + case PROT_TYPE_ALC: >>> + tec->b60 = 1; >>> + fallthrough; >>> + case PROT_TYPE_DAT: >>> + tec->b61 = 1; >>> + break; >>> + } >>> } >> >> How about switching this around and masking those bits on termination. > > I did initially have if (!terminate) { ... }, but it seemed more straight forward > to me without the negation. Or are you suggesting explicitly resetting the > bits to zero when terminating?
Yes
>> >>> fallthrough; >>> case PGM_ASCE_TYPE: >>> @@ -552,6 +554,12 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, >>> return code; >>> } >>> +static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar, >>> + enum gacc_mode mode, enum prot_type prot) >>> +{ >>> + return trans_exc_ending(vcpu, code, gva, ar, mode, prot, true); >>> +} >>> + >>> static int get_vcpu_asce(struct kvm_vcpu *vcpu, union asce *asce, >>> unsigned long ga, u8 ar, enum gacc_mode mode) >>> { >>> @@ -1110,7 +1118,8 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, >>> ga = kvm_s390_logical_to_effective(vcpu, ga + fragment_len); >>> } >>> if (rc > 0) >>> - rc = trans_exc(vcpu, rc, ga, ar, mode, prot); >>> + rc = trans_exc_ending(vcpu, rc, ga, ar, mode, prot, >>> + (mode != GACC_STORE) || (idx == 0)); >> >> Add a boolean variable named terminating, calculate the value before passing the boolean on. > > Ok. I'll scope it to the body of the if. >> >>> out_unlock: >>> if (need_ipte_lock) >>> ipte_unlock(vcpu); >> >> >
| |