| Message ID | 20201030140141.106641-1-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: s390: pv: Mark mm as protected after the set secure parameters and improve cleanup | expand |
On 30.10.20 15:01, Janosch Frank wrote: > We can only have protected guest pages after a successful set secure > parameters call as only then the UV allows imports and unpacks. > > By moving the test we can now also check for it in s390_reset_acc() > and do an early return if it is 0. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Can we check this into devel to give it some test coverage? > --- > arch/s390/kvm/kvm-s390.c | 2 +- > arch/s390/kvm/pv.c | 3 ++- > arch/s390/mm/gmap.c | 2 ++ > 3 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 6b74b92c1a58..08ea6c4735cd 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2312,7 +2312,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) > struct kvm_s390_pv_unp unp = {}; > > r = -EINVAL; > - if (!kvm_s390_pv_is_protected(kvm)) > + if (!kvm_s390_pv_is_protected(kvm) || !mm_is_protected(kvm->mm)) > break; > > r = -EFAULT; > diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c > index eb99e2f95ebe..f5847f9dec7c 100644 > --- a/arch/s390/kvm/pv.c > +++ b/arch/s390/kvm/pv.c > @@ -208,7 +208,6 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) > return -EIO; > } > kvm->arch.gmap->guest_handle = uvcb.guest_handle; > - atomic_set(&kvm->mm->context.is_protected, 1); > return 0; > } > > @@ -228,6 +227,8 @@ int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc, > *rrc = uvcb.header.rrc; > KVM_UV_EVENT(kvm, 3, "PROTVIRT VM SET PARMS: rc %x rrc %x", > *rc, *rrc); > + if (!cc) > + atomic_set(&kvm->mm->context.is_protected, 1); > return cc ? -EINVAL : 0; > } > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index cfb0017f33a7..64795d034926 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -2690,6 +2690,8 @@ static const struct mm_walk_ops reset_acc_walk_ops = { > #include <linux/sched/mm.h> > void s390_reset_acc(struct mm_struct *mm) > { > + if (!mm_is_protected(mm)) > + return; > /* > * we might be called during > * reset: we walk the pages and clear >
On 30.10.20 15:01, Janosch Frank wrote: > We can only have protected guest pages after a successful set secure > parameters call as only then the UV allows imports and unpacks. > > By moving the test we can now also check for it in s390_reset_acc() > and do an early return if it is 0. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Looks sane. Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> As said in my other mail lets give it some days for the CI to test this. > --- > arch/s390/kvm/kvm-s390.c | 2 +- > arch/s390/kvm/pv.c | 3 ++- > arch/s390/mm/gmap.c | 2 ++ > 3 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 6b74b92c1a58..08ea6c4735cd 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2312,7 +2312,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) > struct kvm_s390_pv_unp unp = {}; > > r = -EINVAL; > - if (!kvm_s390_pv_is_protected(kvm)) > + if (!kvm_s390_pv_is_protected(kvm) || !mm_is_protected(kvm->mm)) > break; > > r = -EFAULT; > diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c > index eb99e2f95ebe..f5847f9dec7c 100644 > --- a/arch/s390/kvm/pv.c > +++ b/arch/s390/kvm/pv.c > @@ -208,7 +208,6 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) > return -EIO; > } > kvm->arch.gmap->guest_handle = uvcb.guest_handle; > - atomic_set(&kvm->mm->context.is_protected, 1); > return 0; > } > > @@ -228,6 +227,8 @@ int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc, > *rrc = uvcb.header.rrc; > KVM_UV_EVENT(kvm, 3, "PROTVIRT VM SET PARMS: rc %x rrc %x", > *rc, *rrc); > + if (!cc) > + atomic_set(&kvm->mm->context.is_protected, 1); > return cc ? -EINVAL : 0; > } > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index cfb0017f33a7..64795d034926 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -2690,6 +2690,8 @@ static const struct mm_walk_ops reset_acc_walk_ops = { > #include <linux/sched/mm.h> > void s390_reset_acc(struct mm_struct *mm) > { > + if (!mm_is_protected(mm)) > + return; > /* > * we might be called during > * reset: we walk the pages and clear >
On 10/30/20 3:25 PM, Christian Borntraeger wrote: > On 30.10.20 15:01, Janosch Frank wrote: >> We can only have protected guest pages after a successful set secure >> parameters call as only then the UV allows imports and unpacks. >> >> By moving the test we can now also check for it in s390_reset_acc() >> and do an early return if it is 0. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > Looks sane. > > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> Thanks > > As said in my other mail lets give it some days for the CI to test this. Sure, I'll push it in a minute > >> --- >> arch/s390/kvm/kvm-s390.c | 2 +- >> arch/s390/kvm/pv.c | 3 ++- >> arch/s390/mm/gmap.c | 2 ++ >> 3 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 6b74b92c1a58..08ea6c4735cd 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -2312,7 +2312,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) >> struct kvm_s390_pv_unp unp = {}; >> >> r = -EINVAL; >> - if (!kvm_s390_pv_is_protected(kvm)) >> + if (!kvm_s390_pv_is_protected(kvm) || !mm_is_protected(kvm->mm)) >> break; >> >> r = -EFAULT; >> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c >> index eb99e2f95ebe..f5847f9dec7c 100644 >> --- a/arch/s390/kvm/pv.c >> +++ b/arch/s390/kvm/pv.c >> @@ -208,7 +208,6 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) >> return -EIO; >> } >> kvm->arch.gmap->guest_handle = uvcb.guest_handle; >> - atomic_set(&kvm->mm->context.is_protected, 1); >> return 0; >> } >> >> @@ -228,6 +227,8 @@ int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc, >> *rrc = uvcb.header.rrc; >> KVM_UV_EVENT(kvm, 3, "PROTVIRT VM SET PARMS: rc %x rrc %x", >> *rc, *rrc); >> + if (!cc) >> + atomic_set(&kvm->mm->context.is_protected, 1); >> return cc ? -EINVAL : 0; >> } >> >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >> index cfb0017f33a7..64795d034926 100644 >> --- a/arch/s390/mm/gmap.c >> +++ b/arch/s390/mm/gmap.c >> @@ -2690,6 +2690,8 @@ static const struct mm_walk_ops reset_acc_walk_ops = { >> #include <linux/sched/mm.h> >> void s390_reset_acc(struct mm_struct *mm) >> { >> + if (!mm_is_protected(mm)) >> + return; >> /* >> * we might be called during >> * reset: we walk the pages and clear >>
On Fri, 30 Oct 2020 10:01:41 -0400 Janosch Frank <frankja@linux.ibm.com> wrote: > We can only have protected guest pages after a successful set secure > parameters call as only then the UV allows imports and unpacks. > > By moving the test we can now also check for it in s390_reset_acc() > and do an early return if it is 0. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 2 +- > arch/s390/kvm/pv.c | 3 ++- > arch/s390/mm/gmap.c | 2 ++ > 3 files changed, 5 insertions(+), 2 deletions(-) Seems reasonable to me. I assume it has been given some coverage by now? Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 10/30/20 3:23 PM, Christian Borntraeger wrote: > On 30.10.20 15:01, Janosch Frank wrote: >> We can only have protected guest pages after a successful set secure >> parameters call as only then the UV allows imports and unpacks. >> >> By moving the test we can now also check for it in s390_reset_acc() >> and do an early return if it is 0. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > Can we check this into devel to give it some test coverage? I think this also lacks: Fixes: 29b40f105ec8 ("KVM: s390: protvirt: Add initial vm and cpu lifecycle handling") > >> --- >> arch/s390/kvm/kvm-s390.c | 2 +- >> arch/s390/kvm/pv.c | 3 ++- >> arch/s390/mm/gmap.c | 2 ++ >> 3 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 6b74b92c1a58..08ea6c4735cd 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -2312,7 +2312,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) >> struct kvm_s390_pv_unp unp = {}; >> >> r = -EINVAL; >> - if (!kvm_s390_pv_is_protected(kvm)) >> + if (!kvm_s390_pv_is_protected(kvm) || !mm_is_protected(kvm->mm)) >> break; >> >> r = -EFAULT; >> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c >> index eb99e2f95ebe..f5847f9dec7c 100644 >> --- a/arch/s390/kvm/pv.c >> +++ b/arch/s390/kvm/pv.c >> @@ -208,7 +208,6 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) >> return -EIO; >> } >> kvm->arch.gmap->guest_handle = uvcb.guest_handle; >> - atomic_set(&kvm->mm->context.is_protected, 1); >> return 0; >> } >> >> @@ -228,6 +227,8 @@ int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc, >> *rrc = uvcb.header.rrc; >> KVM_UV_EVENT(kvm, 3, "PROTVIRT VM SET PARMS: rc %x rrc %x", >> *rc, *rrc); >> + if (!cc) >> + atomic_set(&kvm->mm->context.is_protected, 1); >> return cc ? -EINVAL : 0; >> } >> >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >> index cfb0017f33a7..64795d034926 100644 >> --- a/arch/s390/mm/gmap.c >> +++ b/arch/s390/mm/gmap.c >> @@ -2690,6 +2690,8 @@ static const struct mm_walk_ops reset_acc_walk_ops = { >> #include <linux/sched/mm.h> >> void s390_reset_acc(struct mm_struct *mm) >> { >> + if (!mm_is_protected(mm)) >> + return; >> /* >> * we might be called during >> * reset: we walk the pages and clear >>
On 11.11.20 09:17, Janosch Frank wrote: > On 10/30/20 3:23 PM, Christian Borntraeger wrote: >> On 30.10.20 15:01, Janosch Frank wrote: >>> We can only have protected guest pages after a successful set secure >>> parameters call as only then the UV allows imports and unpacks. >>> >>> By moving the test we can now also check for it in s390_reset_acc() >>> and do an early return if it is 0. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> >> Can we check this into devel to give it some test coverage? > > I think this also lacks: > Fixes: 29b40f105ec8 ("KVM: s390: protvirt: Add initial vm and cpu > lifecycle handling") Yes, it does. I will schedule for kvm/master.
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 6b74b92c1a58..08ea6c4735cd 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2312,7 +2312,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) struct kvm_s390_pv_unp unp = {}; r = -EINVAL; - if (!kvm_s390_pv_is_protected(kvm)) + if (!kvm_s390_pv_is_protected(kvm) || !mm_is_protected(kvm->mm)) break; r = -EFAULT; diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c index eb99e2f95ebe..f5847f9dec7c 100644 --- a/arch/s390/kvm/pv.c +++ b/arch/s390/kvm/pv.c @@ -208,7 +208,6 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) return -EIO; } kvm->arch.gmap->guest_handle = uvcb.guest_handle; - atomic_set(&kvm->mm->context.is_protected, 1); return 0; } @@ -228,6 +227,8 @@ int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc, *rrc = uvcb.header.rrc; KVM_UV_EVENT(kvm, 3, "PROTVIRT VM SET PARMS: rc %x rrc %x", *rc, *rrc); + if (!cc) + atomic_set(&kvm->mm->context.is_protected, 1); return cc ? -EINVAL : 0; } diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index cfb0017f33a7..64795d034926 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -2690,6 +2690,8 @@ static const struct mm_walk_ops reset_acc_walk_ops = { #include <linux/sched/mm.h> void s390_reset_acc(struct mm_struct *mm) { + if (!mm_is_protected(mm)) + return; /* * we might be called during * reset: we walk the pages and clear
We can only have protected guest pages after a successful set secure parameters call as only then the UV allows imports and unpacks. By moving the test we can now also check for it in s390_reset_acc() and do an early return if it is 0. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- arch/s390/kvm/kvm-s390.c | 2 +- arch/s390/kvm/pv.c | 3 ++- arch/s390/mm/gmap.c | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-)