| Message ID | 20191024114059.102802-24-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Add support for protected VMs | expand |
On Thu, 24 Oct 2019 07:40:45 -0400 Janosch Frank <frankja@linux.ibm.com> wrote: Add at least a short sentence here? > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index eddc9508c1b1..17a78774c617 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) > rc = gmap_mprotect_notify(vcpu->arch.gmap, > kvm_s390_get_prefix(vcpu), > PAGE_SIZE * 2, PROT_WRITE); > + if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) { > + rc = uv_convert_to_secure(vcpu->arch.gmap, > + kvm_s390_get_prefix(vcpu)); > + WARN_ON_ONCE(rc && rc != -EEXIST); > + rc = uv_convert_to_secure(vcpu->arch.gmap, > + kvm_s390_get_prefix(vcpu) + PAGE_SIZE); > + WARN_ON_ONCE(rc && rc != -EEXIST); > + rc = 0; So, what happens if we have an error other than -EEXIST (which presumably means that we already protected it) here? The page is not protected, and further accesses will get an error? (Another question: what can actually go wrong here?) > + } > if (rc) { > kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); > return rc;
On 11/18/19 5:39 PM, Cornelia Huck wrote: > On Thu, 24 Oct 2019 07:40:45 -0400 > Janosch Frank <frankja@linux.ibm.com> wrote: > > Add at least a short sentence here? For protected VMs the lowcore does not only need to be mapped, but also needs to be protected memory, if not we'll get a validity interception when trying to run it. > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index eddc9508c1b1..17a78774c617 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) >> rc = gmap_mprotect_notify(vcpu->arch.gmap, >> kvm_s390_get_prefix(vcpu), >> PAGE_SIZE * 2, PROT_WRITE); >> + if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) { >> + rc = uv_convert_to_secure(vcpu->arch.gmap, >> + kvm_s390_get_prefix(vcpu)); >> + WARN_ON_ONCE(rc && rc != -EEXIST); >> + rc = uv_convert_to_secure(vcpu->arch.gmap, >> + kvm_s390_get_prefix(vcpu) + PAGE_SIZE); >> + WARN_ON_ONCE(rc && rc != -EEXIST); >> + rc = 0; > > So, what happens if we have an error other than -EEXIST (which > presumably means that we already protected it) here? The page is not > protected, and further accesses will get an error? (Another question: > what can actually go wrong here?) If KVM or QEMU write into a lowcore, we will fail the integrity check on import and this cpu will never run again. In retrospect a warn_on_once might be the wrong error handling here. > >> + } >> if (rc) { >> kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); >> return rc; >
On Tue, 19 Nov 2019 09:11:11 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 11/18/19 5:39 PM, Cornelia Huck wrote: > > On Thu, 24 Oct 2019 07:40:45 -0400 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > > Add at least a short sentence here? > > For protected VMs the lowcore does not only need to be mapped, but also > needs to be protected memory, if not we'll get a validity interception > when trying to run it. Much better, thanks! > > > > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> --- > >> arch/s390/kvm/kvm-s390.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > >> index eddc9508c1b1..17a78774c617 100644 > >> --- a/arch/s390/kvm/kvm-s390.c > >> +++ b/arch/s390/kvm/kvm-s390.c > >> @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) > >> rc = gmap_mprotect_notify(vcpu->arch.gmap, > >> kvm_s390_get_prefix(vcpu), > >> PAGE_SIZE * 2, PROT_WRITE); > >> + if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) { > >> + rc = uv_convert_to_secure(vcpu->arch.gmap, > >> + kvm_s390_get_prefix(vcpu)); > >> + WARN_ON_ONCE(rc && rc != -EEXIST); > >> + rc = uv_convert_to_secure(vcpu->arch.gmap, > >> + kvm_s390_get_prefix(vcpu) + PAGE_SIZE); > >> + WARN_ON_ONCE(rc && rc != -EEXIST); > >> + rc = 0; > > > > So, what happens if we have an error other than -EEXIST (which > > presumably means that we already protected it) here? The page is not > > protected, and further accesses will get an error? (Another question: > > what can actually go wrong here?) > > If KVM or QEMU write into a lowcore, we will fail the integrity check on > import and this cpu will never run again. From the guest's POV, is that similar to a cpu going into xstop? > In retrospect a warn_on_once might be the wrong error handling here. > > > > >> + } > >> if (rc) { > >> kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); > >> return rc; > > > >
On 11/19/19 10:45 AM, Cornelia Huck wrote: > On Tue, 19 Nov 2019 09:11:11 +0100 [...] >>> >>> So, what happens if we have an error other than -EEXIST (which >>> presumably means that we already protected it) here? The page is not >>> protected, and further accesses will get an error? (Another question: >>> what can actually go wrong here?) >> >> If KVM or QEMU write into a lowcore, we will fail the integrity check on >> import and this cpu will never run again. > > From the guest's POV, is that similar to a cpu going into xstop? Not really, I'm wondering what happens, if we try to send a restart to such a cpu. > >> In retrospect a warn_on_once might be the wrong error handling here. >> >>> >>>> + } >>>> if (rc) { >>>> kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); >>>> return rc; >>> >> >> >
On 24.10.19 13:40, Janosch Frank wrote: > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index eddc9508c1b1..17a78774c617 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) > rc = gmap_mprotect_notify(vcpu->arch.gmap, > kvm_s390_get_prefix(vcpu), > PAGE_SIZE * 2, PROT_WRITE); > + if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) { > + rc = uv_convert_to_secure(vcpu->arch.gmap, > + kvm_s390_get_prefix(vcpu)); > + WARN_ON_ONCE(rc && rc != -EEXIST); > + rc = uv_convert_to_secure(vcpu->arch.gmap, > + kvm_s390_get_prefix(vcpu) + PAGE_SIZE); > + WARN_ON_ONCE(rc && rc != -EEXIST); > + rc = 0; > + } ... what if userspace reads the prefix pages just after these calls? validity? :/ > if (rc) { > kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); > return rc; >
On 11/19/19 11:18 AM, David Hildenbrand wrote: > On 24.10.19 13:40, Janosch Frank wrote: >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index eddc9508c1b1..17a78774c617 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) >> rc = gmap_mprotect_notify(vcpu->arch.gmap, >> kvm_s390_get_prefix(vcpu), >> PAGE_SIZE * 2, PROT_WRITE); >> + if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) { >> + rc = uv_convert_to_secure(vcpu->arch.gmap, >> + kvm_s390_get_prefix(vcpu)); >> + WARN_ON_ONCE(rc && rc != -EEXIST); >> + rc = uv_convert_to_secure(vcpu->arch.gmap, >> + kvm_s390_get_prefix(vcpu) + PAGE_SIZE); >> + WARN_ON_ONCE(rc && rc != -EEXIST); >> + rc = 0; >> + } > > ... what if userspace reads the prefix pages just after these calls? > validity? :/ Currently yes, we're working with firmware to fix this. > >> if (rc) { >> kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); >> return rc; >> >
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index eddc9508c1b1..17a78774c617 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) rc = gmap_mprotect_notify(vcpu->arch.gmap, kvm_s390_get_prefix(vcpu), PAGE_SIZE * 2, PROT_WRITE); + if (!rc && kvm_s390_pv_is_protected(vcpu->kvm)) { + rc = uv_convert_to_secure(vcpu->arch.gmap, + kvm_s390_get_prefix(vcpu)); + WARN_ON_ONCE(rc && rc != -EEXIST); + rc = uv_convert_to_secure(vcpu->arch.gmap, + kvm_s390_get_prefix(vcpu) + PAGE_SIZE); + WARN_ON_ONCE(rc && rc != -EEXIST); + rc = 0; + } if (rc) { kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); return rc;
Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- arch/s390/kvm/kvm-s390.c | 9 +++++++++ 1 file changed, 9 insertions(+)