Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 2020/4/26 20:59, Thomas Huth wrote: On 23/04/2020 13.00, Christian Borntraeger wrote: On 23.04.20 12:58, Tianjia Zhang wrote: On 2020/4/23 18:39, Cornelia Huck wrote: On Thu, 23 Apr 2020 11:01:43 +0800 Tianjia Zhang wrote: On 2020/4/23 0:04, Cornelia Huck wrote: On Wed, 22 Apr 2020 17:58:04 +0200 Christian Borntraeger wrote: On 22.04.20 15:45, Cornelia Huck wrote: On Wed, 22 Apr 2020 20:58:04 +0800 Tianjia Zhang wrote: In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function s/Earlier than/For/ ? parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This patch does a unified cleanup of these remaining redundant parameters. Signed-off-by: Tianjia Zhang --- arch/s390/kvm/kvm-s390.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index e335a7e5ead7..d7bb2e7a07ff 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) return rc; } -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; struct runtime_instr_cb *riccb; struct gs_cb *gscb; @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } if (vcpu->arch.gs_enabled) { current->thread.gs_cb = (struct gs_cb *) - >run->s.regs.gscb; + _run->s.regs.gscb; Not sure if these changes (vcpu->run-> => kvm_run->) are really worth it. (It seems they amount to at least as much as the changes advertised in the patch description.) Other opinions? Agreed. It feels kind of random. Maybe just do the first line (move kvm_run from the function parameter list into the variable declaration)? Not sure if this is better. There's more in this patch that I cut... but I think just moving kvm_run from the parameter list would be much less disruptive. I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but there will be more disruptive, not less. I just fail to see the benefit; sure, kvm_run-> is convenient, but the current code is just fine, and any rework should be balanced against the cost (e.g. cluttering git annotate). cluttering git annotate ? Does it mean Fix ("comment"). Is it possible to solve this problem by splitting this patch? No its about breaking git blame (and bugfix backports) for just a cosmetic improvement. It could be slightly more than a cosmetic improvement (depending on the smartness of the compiler): vcpu->run-> are two dereferences, while kvm_run-> is only one dereference. So it could be slightly more compact and faster code. Thomas If the compiler is smart enough, this place can be automatically optimized, but we can't just rely on the compiler, if not? This requires a trade-off between code cleanliness readability and breaking git blame. In addition, I have removed the changes here and sent a v4 patch. Please also help review it. Thanks and best, Tianjia
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 23/04/2020 13.00, Christian Borntraeger wrote: > > > On 23.04.20 12:58, Tianjia Zhang wrote: >> >> >> On 2020/4/23 18:39, Cornelia Huck wrote: >>> On Thu, 23 Apr 2020 11:01:43 +0800 >>> Tianjia Zhang wrote: >>> On 2020/4/23 0:04, Cornelia Huck wrote: > On Wed, 22 Apr 2020 17:58:04 +0200 > Christian Borntraeger wrote: > >> On 22.04.20 15:45, Cornelia Huck wrote: >>> On Wed, 22 Apr 2020 20:58:04 +0800 >>> Tianjia Zhang wrote: >>> In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function >>> >>> s/Earlier than/For/ ? >>> parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This patch does a unified cleanup of these remaining redundant parameters. Signed-off-by: Tianjia Zhang --- arch/s390/kvm/kvm-s390.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index e335a7e5ead7..d7bb2e7a07ff 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) return rc; } -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; struct runtime_instr_cb *riccb; struct gs_cb *gscb; @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } if (vcpu->arch.gs_enabled) { current->thread.gs_cb = (struct gs_cb *) - >run->s.regs.gscb; + _run->s.regs.gscb; >>> >>> Not sure if these changes (vcpu->run-> => kvm_run->) are really worth >>> it. (It seems they amount to at least as much as the changes advertised >>> in the patch description.) >>> >>> Other opinions? >> >> Agreed. It feels kind of random. Maybe just do the first line (move >> kvm_run from the >> function parameter list into the variable declaration)? Not sure if this >> is better. >> > > There's more in this patch that I cut... but I think just moving > kvm_run from the parameter list would be much less disruptive. > I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but there will be more disruptive, not less. >>> >>> I just fail to see the benefit; sure, kvm_run-> is convenient, but the >>> current code is just fine, and any rework should be balanced against >>> the cost (e.g. cluttering git annotate). >>> >> >> cluttering git annotate ? Does it mean Fix ("comment"). Is it possible >> to solve this problem by splitting this patch? > > No its about breaking git blame (and bugfix backports) for just a cosmetic > improvement. It could be slightly more than a cosmetic improvement (depending on the smartness of the compiler): vcpu->run-> are two dereferences, while kvm_run-> is only one dereference. So it could be slightly more compact and faster code. Thomas
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 2020/4/23 19:00, Christian Borntraeger wrote: On 23.04.20 12:58, Tianjia Zhang wrote: On 2020/4/23 18:39, Cornelia Huck wrote: On Thu, 23 Apr 2020 11:01:43 +0800 Tianjia Zhang wrote: On 2020/4/23 0:04, Cornelia Huck wrote: On Wed, 22 Apr 2020 17:58:04 +0200 Christian Borntraeger wrote: On 22.04.20 15:45, Cornelia Huck wrote: On Wed, 22 Apr 2020 20:58:04 +0800 Tianjia Zhang wrote: In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function s/Earlier than/For/ ? parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This patch does a unified cleanup of these remaining redundant parameters. Signed-off-by: Tianjia Zhang --- arch/s390/kvm/kvm-s390.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index e335a7e5ead7..d7bb2e7a07ff 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) return rc; } -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; struct runtime_instr_cb *riccb; struct gs_cb *gscb; @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } if (vcpu->arch.gs_enabled) { current->thread.gs_cb = (struct gs_cb *) - >run->s.regs.gscb; + _run->s.regs.gscb; Not sure if these changes (vcpu->run-> => kvm_run->) are really worth it. (It seems they amount to at least as much as the changes advertised in the patch description.) Other opinions? Agreed. It feels kind of random. Maybe just do the first line (move kvm_run from the function parameter list into the variable declaration)? Not sure if this is better. There's more in this patch that I cut... but I think just moving kvm_run from the parameter list would be much less disruptive. I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but there will be more disruptive, not less. I just fail to see the benefit; sure, kvm_run-> is convenient, but the current code is just fine, and any rework should be balanced against the cost (e.g. cluttering git annotate). cluttering git annotate ? Does it mean Fix ("comment"). Is it possible to solve this problem by splitting this patch? No its about breaking git blame (and bugfix backports) for just a cosmetic improvement. And I agree with Conny: the cost is higher than the benefit. I will make a fix in the v3 version. Help to see if there are problems with the next few patches. Thanks, Tianjia
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 23.04.20 12:58, Tianjia Zhang wrote: > > > On 2020/4/23 18:39, Cornelia Huck wrote: >> On Thu, 23 Apr 2020 11:01:43 +0800 >> Tianjia Zhang wrote: >> >>> On 2020/4/23 0:04, Cornelia Huck wrote: On Wed, 22 Apr 2020 17:58:04 +0200 Christian Borntraeger wrote: > On 22.04.20 15:45, Cornelia Huck wrote: >> On Wed, 22 Apr 2020 20:58:04 +0800 >> Tianjia Zhang wrote: >> >>> In the current kvm version, 'kvm_run' has been included in the >>> 'kvm_vcpu' >>> structure. Earlier than historical reasons, many kvm-related function >> >> s/Earlier than/For/ ? >> >>> parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same >>> time. >>> This patch does a unified cleanup of these remaining redundant >>> parameters. >>> >>> Signed-off-by: Tianjia Zhang >>> --- >>> arch/s390/kvm/kvm-s390.c | 37 ++--- >>> 1 file changed, 22 insertions(+), 15 deletions(-) >>> >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index e335a7e5ead7..d7bb2e7a07ff 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) >>> return rc; >>> } >>> -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run >>> *kvm_run) >>> +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) >>> { >>> + struct kvm_run *kvm_run = vcpu->run; >>> struct runtime_instr_cb *riccb; >>> struct gs_cb *gscb; >>> @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu >>> *vcpu, struct kvm_run *kvm_run) >>> } >>> if (vcpu->arch.gs_enabled) { >>> current->thread.gs_cb = (struct gs_cb *) >>> - >run->s.regs.gscb; >>> + _run->s.regs.gscb; >> >> Not sure if these changes (vcpu->run-> => kvm_run->) are really worth >> it. (It seems they amount to at least as much as the changes advertised >> in the patch description.) >> >> Other opinions? > > Agreed. It feels kind of random. Maybe just do the first line (move > kvm_run from the > function parameter list into the variable declaration)? Not sure if this > is better. > There's more in this patch that I cut... but I think just moving kvm_run from the parameter list would be much less disruptive. >>> >>> I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but >>> there will be more disruptive, not less. >> >> I just fail to see the benefit; sure, kvm_run-> is convenient, but the >> current code is just fine, and any rework should be balanced against >> the cost (e.g. cluttering git annotate). >> > > cluttering git annotate ? Does it mean Fix ("comment"). Is it possible > to solve this problem by splitting this patch? No its about breaking git blame (and bugfix backports) for just a cosmetic improvement. And I agree with Conny: the cost is higher than the benefit.
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 2020/4/23 18:39, Cornelia Huck wrote: On Thu, 23 Apr 2020 11:01:43 +0800 Tianjia Zhang wrote: On 2020/4/23 0:04, Cornelia Huck wrote: On Wed, 22 Apr 2020 17:58:04 +0200 Christian Borntraeger wrote: On 22.04.20 15:45, Cornelia Huck wrote: On Wed, 22 Apr 2020 20:58:04 +0800 Tianjia Zhang wrote: In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function s/Earlier than/For/ ? parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This patch does a unified cleanup of these remaining redundant parameters. Signed-off-by: Tianjia Zhang --- arch/s390/kvm/kvm-s390.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index e335a7e5ead7..d7bb2e7a07ff 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) return rc; } -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; struct runtime_instr_cb *riccb; struct gs_cb *gscb; @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } if (vcpu->arch.gs_enabled) { current->thread.gs_cb = (struct gs_cb *) - >run->s.regs.gscb; + _run->s.regs.gscb; Not sure if these changes (vcpu->run-> => kvm_run->) are really worth it. (It seems they amount to at least as much as the changes advertised in the patch description.) Other opinions? Agreed. It feels kind of random. Maybe just do the first line (move kvm_run from the function parameter list into the variable declaration)? Not sure if this is better. There's more in this patch that I cut... but I think just moving kvm_run from the parameter list would be much less disruptive. I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but there will be more disruptive, not less. I just fail to see the benefit; sure, kvm_run-> is convenient, but the current code is just fine, and any rework should be balanced against the cost (e.g. cluttering git annotate). cluttering git annotate ? Does it mean Fix ("comment"). Is it possible to solve this problem by splitting this patch?
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On Thu, 23 Apr 2020 11:01:43 +0800 Tianjia Zhang wrote: > On 2020/4/23 0:04, Cornelia Huck wrote: > > On Wed, 22 Apr 2020 17:58:04 +0200 > > Christian Borntraeger wrote: > > > >> On 22.04.20 15:45, Cornelia Huck wrote: > >>> On Wed, 22 Apr 2020 20:58:04 +0800 > >>> Tianjia Zhang wrote: > >>> > In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' > structure. Earlier than historical reasons, many kvm-related function > >>> > >>> s/Earlier than/For/ ? > >>> > parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same > time. > This patch does a unified cleanup of these remaining redundant > parameters. > > Signed-off-by: Tianjia Zhang > --- > arch/s390/kvm/kvm-s390.c | 37 ++--- > 1 file changed, 22 insertions(+), 15 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index e335a7e5ead7..d7bb2e7a07ff 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) > return rc; > } > > -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run > *kvm_run) > +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) > { > +struct kvm_run *kvm_run = vcpu->run; > struct runtime_instr_cb *riccb; > struct gs_cb *gscb; > > @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, > struct kvm_run *kvm_run) > } > if (vcpu->arch.gs_enabled) { > current->thread.gs_cb = (struct gs_cb *) > ->run->s.regs.gscb; > +_run->s.regs.gscb; > >>> > >>> Not sure if these changes (vcpu->run-> => kvm_run->) are really worth > >>> it. (It seems they amount to at least as much as the changes advertised > >>> in the patch description.) > >>> > >>> Other opinions? > >> > >> Agreed. It feels kind of random. Maybe just do the first line (move > >> kvm_run from the > >> function parameter list into the variable declaration)? Not sure if this > >> is better. > >> > > > > There's more in this patch that I cut... but I think just moving > > kvm_run from the parameter list would be much less disruptive. > > > > I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but > there will be more disruptive, not less. I just fail to see the benefit; sure, kvm_run-> is convenient, but the current code is just fine, and any rework should be balanced against the cost (e.g. cluttering git annotate).
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 2020/4/22 23:58, Christian Borntraeger wrote: On 22.04.20 15:45, Cornelia Huck wrote: On Wed, 22 Apr 2020 20:58:04 +0800 Tianjia Zhang wrote: In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function s/Earlier than/For/ ? parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This patch does a unified cleanup of these remaining redundant parameters. Signed-off-by: Tianjia Zhang --- arch/s390/kvm/kvm-s390.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index e335a7e5ead7..d7bb2e7a07ff 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) return rc; } -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; struct runtime_instr_cb *riccb; struct gs_cb *gscb; @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } if (vcpu->arch.gs_enabled) { current->thread.gs_cb = (struct gs_cb *) - >run->s.regs.gscb; + _run->s.regs.gscb; Not sure if these changes (vcpu->run-> => kvm_run->) are really worth it. (It seems they amount to at least as much as the changes advertised in the patch description.) Other opinions? Agreed. It feels kind of random. Maybe just do the first line (move kvm_run from the function parameter list into the variable declaration)? Not sure if this is better. Why not, `kvm_run` is equivalent to `vcpu->run`, which is also part of the cleanup, or do you mean to put this change in another patch? Thanks, Tianjia
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 2020/4/23 0:04, Cornelia Huck wrote: On Wed, 22 Apr 2020 17:58:04 +0200 Christian Borntraeger wrote: On 22.04.20 15:45, Cornelia Huck wrote: On Wed, 22 Apr 2020 20:58:04 +0800 Tianjia Zhang wrote: In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function s/Earlier than/For/ ? parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This patch does a unified cleanup of these remaining redundant parameters. Signed-off-by: Tianjia Zhang --- arch/s390/kvm/kvm-s390.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index e335a7e5ead7..d7bb2e7a07ff 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) return rc; } -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; struct runtime_instr_cb *riccb; struct gs_cb *gscb; @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } if (vcpu->arch.gs_enabled) { current->thread.gs_cb = (struct gs_cb *) - >run->s.regs.gscb; + _run->s.regs.gscb; Not sure if these changes (vcpu->run-> => kvm_run->) are really worth it. (It seems they amount to at least as much as the changes advertised in the patch description.) Other opinions? Agreed. It feels kind of random. Maybe just do the first line (move kvm_run from the function parameter list into the variable declaration)? Not sure if this is better. There's more in this patch that I cut... but I think just moving kvm_run from the parameter list would be much less disruptive. I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but there will be more disruptive, not less. Thanks, Tianjia
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 2020/4/22 21:45, Cornelia Huck wrote: On Wed, 22 Apr 2020 20:58:04 +0800 Tianjia Zhang wrote: In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function s/Earlier than/For/ ? Yes, it should be replaced like this. parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This patch does a unified cleanup of these remaining redundant parameters. Signed-off-by: Tianjia Zhang --- arch/s390/kvm/kvm-s390.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index e335a7e5ead7..d7bb2e7a07ff 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) return rc; } -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; struct runtime_instr_cb *riccb; struct gs_cb *gscb; @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } if (vcpu->arch.gs_enabled) { current->thread.gs_cb = (struct gs_cb *) - >run->s.regs.gscb; + _run->s.regs.gscb; Not sure if these changes (vcpu->run-> => kvm_run->) are really worth it. (It seems they amount to at least as much as the changes advertised in the patch description.) Other opinions? Why not replace `vcpu->run->` to `kvm_run->` ? If not, there will be both styles of code, which is confusing. I will be confused and think that this is something different. Thanks, Tianjia restore_gs_cb(current->thread.gs_cb); } preempt_enable();
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On Wed, 22 Apr 2020 17:58:04 +0200 Christian Borntraeger wrote: > On 22.04.20 15:45, Cornelia Huck wrote: > > On Wed, 22 Apr 2020 20:58:04 +0800 > > Tianjia Zhang wrote: > > > >> In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' > >> structure. Earlier than historical reasons, many kvm-related function > > > > s/Earlier than/For/ ? > > > >> parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. > >> This patch does a unified cleanup of these remaining redundant parameters. > >> > >> Signed-off-by: Tianjia Zhang > >> --- > >> arch/s390/kvm/kvm-s390.c | 37 ++--- > >> 1 file changed, 22 insertions(+), 15 deletions(-) > >> > >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > >> index e335a7e5ead7..d7bb2e7a07ff 100644 > >> --- a/arch/s390/kvm/kvm-s390.c > >> +++ b/arch/s390/kvm/kvm-s390.c > >> @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) > >>return rc; > >> } > >> > >> -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > >> +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) > >> { > >> + struct kvm_run *kvm_run = vcpu->run; > >>struct runtime_instr_cb *riccb; > >>struct gs_cb *gscb; > >> > >> @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, > >> struct kvm_run *kvm_run) > >>} > >>if (vcpu->arch.gs_enabled) { > >>current->thread.gs_cb = (struct gs_cb *) > >> - >run->s.regs.gscb; > >> + _run->s.regs.gscb; > > > > Not sure if these changes (vcpu->run-> => kvm_run->) are really worth > > it. (It seems they amount to at least as much as the changes advertised > > in the patch description.) > > > > Other opinions? > > Agreed. It feels kind of random. Maybe just do the first line (move kvm_run > from the > function parameter list into the variable declaration)? Not sure if this is > better. > There's more in this patch that I cut... but I think just moving kvm_run from the parameter list would be much less disruptive.
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 22.04.20 15:45, Cornelia Huck wrote: > On Wed, 22 Apr 2020 20:58:04 +0800 > Tianjia Zhang wrote: > >> In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' >> structure. Earlier than historical reasons, many kvm-related function > > s/Earlier than/For/ ? > >> parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. >> This patch does a unified cleanup of these remaining redundant parameters. >> >> Signed-off-by: Tianjia Zhang >> --- >> arch/s390/kvm/kvm-s390.c | 37 ++--- >> 1 file changed, 22 insertions(+), 15 deletions(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index e335a7e5ead7..d7bb2e7a07ff 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) >> return rc; >> } >> >> -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) >> +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) >> { >> +struct kvm_run *kvm_run = vcpu->run; >> struct runtime_instr_cb *riccb; >> struct gs_cb *gscb; >> >> @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, >> struct kvm_run *kvm_run) >> } >> if (vcpu->arch.gs_enabled) { >> current->thread.gs_cb = (struct gs_cb *) >> ->run->s.regs.gscb; >> +_run->s.regs.gscb; > > Not sure if these changes (vcpu->run-> => kvm_run->) are really worth > it. (It seems they amount to at least as much as the changes advertised > in the patch description.) > > Other opinions? Agreed. It feels kind of random. Maybe just do the first line (move kvm_run from the function parameter list into the variable declaration)? Not sure if this is better.
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On Wed, 22 Apr 2020 20:58:04 +0800 Tianjia Zhang wrote: > In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' > structure. Earlier than historical reasons, many kvm-related function s/Earlier than/For/ ? > parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. > This patch does a unified cleanup of these remaining redundant parameters. > > Signed-off-by: Tianjia Zhang > --- > arch/s390/kvm/kvm-s390.c | 37 ++--- > 1 file changed, 22 insertions(+), 15 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index e335a7e5ead7..d7bb2e7a07ff 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) > return rc; > } > > -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) > { > + struct kvm_run *kvm_run = vcpu->run; > struct runtime_instr_cb *riccb; > struct gs_cb *gscb; > > @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, > struct kvm_run *kvm_run) > } > if (vcpu->arch.gs_enabled) { > current->thread.gs_cb = (struct gs_cb *) > - >run->s.regs.gscb; > + _run->s.regs.gscb; Not sure if these changes (vcpu->run-> => kvm_run->) are really worth it. (It seems they amount to at least as much as the changes advertised in the patch description.) Other opinions? > restore_gs_cb(current->thread.gs_cb); > } > preempt_enable();
[PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This patch does a unified cleanup of these remaining redundant parameters. Signed-off-by: Tianjia Zhang --- arch/s390/kvm/kvm-s390.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index e335a7e5ead7..d7bb2e7a07ff 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) return rc; } -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; struct runtime_instr_cb *riccb; struct gs_cb *gscb; @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) } if (vcpu->arch.gs_enabled) { current->thread.gs_cb = (struct gs_cb *) - >run->s.regs.gscb; + _run->s.regs.gscb; restore_gs_cb(current->thread.gs_cb); } preempt_enable(); @@ -4243,8 +4244,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) /* SIE will load etoken directly from SDNX and therefore kvm_run */ } -static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; + if (kvm_run->kvm_dirty_regs & KVM_SYNC_PREFIX) kvm_s390_set_prefix(vcpu, kvm_run->s.regs.prefix); if (kvm_run->kvm_dirty_regs & KVM_SYNC_CRS) { @@ -4257,23 +4260,23 @@ static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) vcpu->arch.sie_block->ckc = kvm_run->s.regs.ckc; } save_access_regs(vcpu->arch.host_acrs); - restore_access_regs(vcpu->run->s.regs.acrs); + restore_access_regs(kvm_run->s.regs.acrs); /* save host (userspace) fprs/vrs */ save_fpu_regs(); vcpu->arch.host_fpregs.fpc = current->thread.fpu.fpc; vcpu->arch.host_fpregs.regs = current->thread.fpu.regs; if (MACHINE_HAS_VX) - current->thread.fpu.regs = vcpu->run->s.regs.vrs; + current->thread.fpu.regs = kvm_run->s.regs.vrs; else - current->thread.fpu.regs = vcpu->run->s.regs.fprs; - current->thread.fpu.fpc = vcpu->run->s.regs.fpc; + current->thread.fpu.regs = kvm_run->s.regs.fprs; + current->thread.fpu.fpc = kvm_run->s.regs.fpc; if (test_fp_ctl(current->thread.fpu.fpc)) /* User space provided an invalid FPC, let's clear it */ current->thread.fpu.fpc = 0; /* Sync fmt2 only data */ if (likely(!kvm_s390_pv_cpu_is_protected(vcpu))) { - sync_regs_fmt2(vcpu, kvm_run); + sync_regs_fmt2(vcpu); } else { /* * In several places we have to modify our internal view to @@ -4292,8 +4295,10 @@ static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) kvm_run->kvm_dirty_regs = 0; } -static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void store_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; + kvm_run->s.regs.todpr = vcpu->arch.sie_block->todpr; kvm_run->s.regs.pp = vcpu->arch.sie_block->pp; kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea; @@ -4313,8 +4318,10 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) /* SIE will save etoken directly into SDNX and therefore kvm_run */ } -static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void store_regs(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; + kvm_run->psw_mask = vcpu->arch.sie_block->gpsw.mask; kvm_run->psw_addr = vcpu->arch.sie_block->gpsw.addr; kvm_run->s.regs.prefix = kvm_s390_get_prefix(vcpu); @@ -4324,16 +4331,16 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) kvm_run->s.regs.pft = vcpu->arch.pfault_token; kvm_run->s.regs.pfs = vcpu->arch.pfault_select; kvm_run->s.regs.pfc = vcpu->arch.pfault_compare; - save_access_regs(vcpu->run->s.regs.acrs); + save_access_regs(kvm_run->s.regs.acrs); restore_access_regs(vcpu->arch.host_acrs); /* Save guest register state */ save_fpu_regs(); - vcpu->run->s.regs.fpc = current->thread.fpu.fpc; + kvm_run->s.regs.fpc = current->thread.fpu.fpc; /* Restore will be done lazily at