| Message ID | 20220425100147.1755340-2-scgl@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Dirtying, failing memop: don't indicate suppression | expand |
On Mon, 25 Apr 2022 12:01:46 +0200 Janis Schoetterl-Glausch <scgl@linux.ibm.com> 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. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> Reviewed-by: Claudio Imbrenda <imbrenda@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; > + } > } > 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)); > out_unlock: > if (need_ipte_lock) > ipte_unlock(vcpu);
Am 25.04.22 um 12:01 schrieb Janis Schoetterl-Glausch: > 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. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > arch/s390/kvm/gaccess.c | 47 ++++++++++++++++++++++++----------------- > 1 file changed, 28 insertions(+), 19 deletions(-) Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
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. > 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. > out_unlock: > if (need_ipte_lock) > ipte_unlock(vcpu);
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? > >> 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); > >
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); >> >> >
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; + } } 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)); out_unlock: if (need_ipte_lock) ipte_unlock(vcpu);
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. Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> --- arch/s390/kvm/gaccess.c | 47 ++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 19 deletions(-)