| Message ID | 1580374500-31247-1-git-send-email-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH/FIXUP,FOR,STABLE,BEFORE,THIS,SERIES] KVM: s390: do not clobber user space fpc during guest reset | expand |
On 30.01.20 09:55, Christian Borntraeger wrote: > The initial CPU reset currently clobbers the userspace fpc. This was an > oversight during a fixup for the lazy fpu reloading rework. The reset > calls are only done from userspace ioctls. No CPU context is loaded, so > we can (and must) act directly on the sync regs, not on the thread > context. Otherwise the fpu restore call will restore the zeroes fpc to > userspace. > > Cc: stable@kernel.org > Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index c059b86..eb789cd 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) > vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | > CR14_UNUSED_33 | > CR14_EXTERNAL_DAMAGE_SUBMASK; > - /* make sure the new fpc will be lazily loaded */ > - save_fpu_regs(); > + vcpu->run->s.regs.fpc = 0; > current->thread.fpu.fpc = 0; > vcpu->arch.sie_block->gbea = 1; > vcpu->arch.sie_block->pp = 0; > kvm_arch_vcpu_ioctl() does a vcpu_load(vcpu), followed by the call to kvm_arch_vcpu_ioctl_initial_reset(), followed by a vcpu_put(). What am I missing? (we could get rid of the kvm_arch_vcpu_ioctl_initial_reset() wrapper)
On Thu, 30 Jan 2020 10:49:35 +0100 David Hildenbrand <david@redhat.com> wrote: > On 30.01.20 09:55, Christian Borntraeger wrote: > > The initial CPU reset currently clobbers the userspace fpc. This was an > > oversight during a fixup for the lazy fpu reloading rework. The reset > > calls are only done from userspace ioctls. No CPU context is loaded, so > > we can (and must) act directly on the sync regs, not on the thread > > context. Otherwise the fpu restore call will restore the zeroes fpc to > > userspace. > > > > Cc: stable@kernel.org > > Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > > --- > > arch/s390/kvm/kvm-s390.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > index c059b86..eb789cd 100644 > > --- a/arch/s390/kvm/kvm-s390.c > > +++ b/arch/s390/kvm/kvm-s390.c > > @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) > > vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | > > CR14_UNUSED_33 | > > CR14_EXTERNAL_DAMAGE_SUBMASK; > > - /* make sure the new fpc will be lazily loaded */ > > - save_fpu_regs(); > > + vcpu->run->s.regs.fpc = 0; > > current->thread.fpu.fpc = 0; > > vcpu->arch.sie_block->gbea = 1; > > vcpu->arch.sie_block->pp = 0; > > > > kvm_arch_vcpu_ioctl() does a vcpu_load(vcpu), followed by the call to > kvm_arch_vcpu_ioctl_initial_reset(), followed by a vcpu_put(). > > What am I missing? I have been staring at this patch for some time now, and I fear I'm missing something as well. Can we please get more explanation? > > (we could get rid of the kvm_arch_vcpu_ioctl_initial_reset() wrapper) >
On 30/01/2020 11.39, Cornelia Huck wrote: > On Thu, 30 Jan 2020 10:49:35 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> On 30.01.20 09:55, Christian Borntraeger wrote: >>> The initial CPU reset currently clobbers the userspace fpc. This was an >>> oversight during a fixup for the lazy fpu reloading rework. The reset >>> calls are only done from userspace ioctls. No CPU context is loaded, so >>> we can (and must) act directly on the sync regs, not on the thread >>> context. Otherwise the fpu restore call will restore the zeroes fpc to >>> userspace. >>> >>> Cc: stable@kernel.org >>> Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> arch/s390/kvm/kvm-s390.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index c059b86..eb789cd 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) >>> vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | >>> CR14_UNUSED_33 | >>> CR14_EXTERNAL_DAMAGE_SUBMASK; >>> - /* make sure the new fpc will be lazily loaded */ >>> - save_fpu_regs(); >>> + vcpu->run->s.regs.fpc = 0; >>> current->thread.fpu.fpc = 0; >>> vcpu->arch.sie_block->gbea = 1; >>> vcpu->arch.sie_block->pp = 0; >>> >> >> kvm_arch_vcpu_ioctl() does a vcpu_load(vcpu), followed by the call to >> kvm_arch_vcpu_ioctl_initial_reset(), followed by a vcpu_put(). >> >> What am I missing? > > I have been staring at this patch for some time now, and I fear I'm > missing something as well. Can we please get more explanation? Could we please get a test for this issue in the kvm selftests, too? I.e. host sets a value in its FPC, then calls the INITIAL_RESET ioctl and then checks that the value in its FPC is still there? Thomas
On 30.01.20 10:49, David Hildenbrand wrote: > On 30.01.20 09:55, Christian Borntraeger wrote: >> The initial CPU reset currently clobbers the userspace fpc. This was an >> oversight during a fixup for the lazy fpu reloading rework. The reset >> calls are only done from userspace ioctls. No CPU context is loaded, so >> we can (and must) act directly on the sync regs, not on the thread >> context. Otherwise the fpu restore call will restore the zeroes fpc to >> userspace. >> >> Cc: stable@kernel.org >> Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index c059b86..eb789cd 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) >> vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | >> CR14_UNUSED_33 | >> CR14_EXTERNAL_DAMAGE_SUBMASK; >> - /* make sure the new fpc will be lazily loaded */ >> - save_fpu_regs(); >> + vcpu->run->s.regs.fpc = 0; >> current->thread.fpu.fpc = 0; >> vcpu->arch.sie_block->gbea = 1; >> vcpu->arch.sie_block->pp = 0; >> > > kvm_arch_vcpu_ioctl() does a vcpu_load(vcpu), followed by the call to > kvm_arch_vcpu_ioctl_initial_reset(), followed by a vcpu_put(). > > What am I missing? vcpu_load/put does no longer reload the registers lazily. We moved that out into the vcpu_run ioctl itself. (this avoids register reloading during schedule).
On 30.01.20 11:56, Thomas Huth wrote: > On 30/01/2020 11.39, Cornelia Huck wrote: >> On Thu, 30 Jan 2020 10:49:35 +0100 >> David Hildenbrand <david@redhat.com> wrote: >> >>> On 30.01.20 09:55, Christian Borntraeger wrote: >>>> The initial CPU reset currently clobbers the userspace fpc. This was an >>>> oversight during a fixup for the lazy fpu reloading rework. The reset >>>> calls are only done from userspace ioctls. No CPU context is loaded, so >>>> we can (and must) act directly on the sync regs, not on the thread >>>> context. Otherwise the fpu restore call will restore the zeroes fpc to >>>> userspace. >>>> >>>> Cc: stable@kernel.org >>>> Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") >>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> --- >>>> arch/s390/kvm/kvm-s390.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index c059b86..eb789cd 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) >>>> vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | >>>> CR14_UNUSED_33 | >>>> CR14_EXTERNAL_DAMAGE_SUBMASK; >>>> - /* make sure the new fpc will be lazily loaded */ >>>> - save_fpu_regs(); >>>> + vcpu->run->s.regs.fpc = 0; >>>> current->thread.fpu.fpc = 0; >>>> vcpu->arch.sie_block->gbea = 1; >>>> vcpu->arch.sie_block->pp = 0; >>>> >>> >>> kvm_arch_vcpu_ioctl() does a vcpu_load(vcpu), followed by the call to >>> kvm_arch_vcpu_ioctl_initial_reset(), followed by a vcpu_put(). >>> >>> What am I missing? >> >> I have been staring at this patch for some time now, and I fear I'm >> missing something as well. Can we please get more explanation? > > Could we please get a test for this issue in the kvm selftests, too? > I.e. host sets a value in its FPC, then calls the INITIAL_RESET ioctl > and then checks that the value in its FPC is still there? Yes, that will come as a later addon patch. (But I am still going to apply this series soon and do not wait for that. I have a private hack in qemu that does this checking, but this test code is too ugly to see the world).
On 30.01.20 12:01, Christian Borntraeger wrote: > > > On 30.01.20 10:49, David Hildenbrand wrote: >> On 30.01.20 09:55, Christian Borntraeger wrote: >>> The initial CPU reset currently clobbers the userspace fpc. This was an >>> oversight during a fixup for the lazy fpu reloading rework. The reset >>> calls are only done from userspace ioctls. No CPU context is loaded, so >>> we can (and must) act directly on the sync regs, not on the thread >>> context. Otherwise the fpu restore call will restore the zeroes fpc to >>> userspace. >>> >>> Cc: stable@kernel.org >>> Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> arch/s390/kvm/kvm-s390.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index c059b86..eb789cd 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) >>> vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | >>> CR14_UNUSED_33 | >>> CR14_EXTERNAL_DAMAGE_SUBMASK; >>> - /* make sure the new fpc will be lazily loaded */ >>> - save_fpu_regs(); >>> + vcpu->run->s.regs.fpc = 0; >>> current->thread.fpu.fpc = 0; >>> vcpu->arch.sie_block->gbea = 1; >>> vcpu->arch.sie_block->pp = 0; >>> >> >> kvm_arch_vcpu_ioctl() does a vcpu_load(vcpu), followed by the call to >> kvm_arch_vcpu_ioctl_initial_reset(), followed by a vcpu_put(). >> >> What am I missing? > > vcpu_load/put does no longer reload the registers lazily. We moved that out into the > vcpu_run ioctl itself. (this avoids register reloading during schedule). see e1788bb KVM: s390: handle floating point registers in the run ioctl not in vcpu_put/load 31d8b8d KVM: s390: handle access registers in the run ioctl not in vcpu_put/load so maybe we want to change the Fixes tag to this patch.
On 30.01.20 12:14, Christian Borntraeger wrote: > > > On 30.01.20 12:01, Christian Borntraeger wrote: >> >> >> On 30.01.20 10:49, David Hildenbrand wrote: >>> On 30.01.20 09:55, Christian Borntraeger wrote: >>>> The initial CPU reset currently clobbers the userspace fpc. This was an >>>> oversight during a fixup for the lazy fpu reloading rework. The reset >>>> calls are only done from userspace ioctls. No CPU context is loaded, so >>>> we can (and must) act directly on the sync regs, not on the thread >>>> context. Otherwise the fpu restore call will restore the zeroes fpc to >>>> userspace. >>>> >>>> Cc: stable@kernel.org >>>> Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") >>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> --- >>>> arch/s390/kvm/kvm-s390.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index c059b86..eb789cd 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) >>>> vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | >>>> CR14_UNUSED_33 | >>>> CR14_EXTERNAL_DAMAGE_SUBMASK; >>>> - /* make sure the new fpc will be lazily loaded */ >>>> - save_fpu_regs(); >>>> + vcpu->run->s.regs.fpc = 0; >>>> current->thread.fpu.fpc = 0; >>>> vcpu->arch.sie_block->gbea = 1; >>>> vcpu->arch.sie_block->pp = 0; >>>> >>> >>> kvm_arch_vcpu_ioctl() does a vcpu_load(vcpu), followed by the call to >>> kvm_arch_vcpu_ioctl_initial_reset(), followed by a vcpu_put(). >>> >>> What am I missing? >> >> vcpu_load/put does no longer reload the registers lazily. We moved that out into the >> vcpu_run ioctl itself. (this avoids register reloading during schedule). > > see > e1788bb KVM: s390: handle floating point registers in the run ioctl not in vcpu_put/load > 31d8b8d KVM: s390: handle access registers in the run ioctl not in vcpu_put/load > > so maybe we want to change the Fixes tag to this patch. > Yes, because e1788bb KVM: s390: handle floating point registers in the run ioctl not in vcpu_put/load broke it. We should audit all users of save_fpu_regs().
On 30.01.20 12:20, David Hildenbrand wrote: > On 30.01.20 12:14, Christian Borntraeger wrote: >> >> >> On 30.01.20 12:01, Christian Borntraeger wrote: >>> >>> >>> On 30.01.20 10:49, David Hildenbrand wrote: >>>> On 30.01.20 09:55, Christian Borntraeger wrote: >>>>> The initial CPU reset currently clobbers the userspace fpc. This was an >>>>> oversight during a fixup for the lazy fpu reloading rework. The reset >>>>> calls are only done from userspace ioctls. No CPU context is loaded, so >>>>> we can (and must) act directly on the sync regs, not on the thread >>>>> context. Otherwise the fpu restore call will restore the zeroes fpc to >>>>> userspace. >>>>> >>>>> Cc: stable@kernel.org >>>>> Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") >>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>> --- >>>>> arch/s390/kvm/kvm-s390.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>>> index c059b86..eb789cd 100644 >>>>> --- a/arch/s390/kvm/kvm-s390.c >>>>> +++ b/arch/s390/kvm/kvm-s390.c >>>>> @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) >>>>> vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | >>>>> CR14_UNUSED_33 | >>>>> CR14_EXTERNAL_DAMAGE_SUBMASK; >>>>> - /* make sure the new fpc will be lazily loaded */ >>>>> - save_fpu_regs(); >>>>> + vcpu->run->s.regs.fpc = 0; >>>>> current->thread.fpu.fpc = 0; >>>>> vcpu->arch.sie_block->gbea = 1; >>>>> vcpu->arch.sie_block->pp = 0; >>>>> >>>> >>>> kvm_arch_vcpu_ioctl() does a vcpu_load(vcpu), followed by the call to >>>> kvm_arch_vcpu_ioctl_initial_reset(), followed by a vcpu_put(). >>>> >>>> What am I missing? >>> >>> vcpu_load/put does no longer reload the registers lazily. We moved that out into the >>> vcpu_run ioctl itself. (this avoids register reloading during schedule). >> >> see >> e1788bb KVM: s390: handle floating point registers in the run ioctl not in vcpu_put/load >> 31d8b8d KVM: s390: handle access registers in the run ioctl not in vcpu_put/load >> >> so maybe we want to change the Fixes tag to this patch. >> > > Yes, because > > e1788bb KVM: s390: handle floating point registers in the run ioctl not > in vcpu_put/load > > broke it. > > We should audit all users of save_fpu_regs(). I think the store status ioctl is also broken. Everything else looks sane.
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index c059b86..eb789cd 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2824,8 +2824,7 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu) vcpu->arch.sie_block->gcr[14] = CR14_UNUSED_32 | CR14_UNUSED_33 | CR14_EXTERNAL_DAMAGE_SUBMASK; - /* make sure the new fpc will be lazily loaded */ - save_fpu_regs(); + vcpu->run->s.regs.fpc = 0; current->thread.fpu.fpc = 0; vcpu->arch.sie_block->gbea = 1; vcpu->arch.sie_block->pp = 0;
The initial CPU reset currently clobbers the userspace fpc. This was an oversight during a fixup for the lazy fpu reloading rework. The reset calls are only done from userspace ioctls. No CPU context is loaded, so we can (and must) act directly on the sync regs, not on the thread context. Otherwise the fpu restore call will restore the zeroes fpc to userspace. Cc: stable@kernel.org Fixes: 9abc2a08a7d6 ("KVM: s390: fix memory overwrites when vx is disabled") Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- arch/s390/kvm/kvm-s390.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)