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 <[email protected]>
---
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 *)
- &vcpu->run->s.regs.gscb;
+ &kvm_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 return */
current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc;
current->thread.fpu.regs = vcpu->arch.host_fpregs.regs;
if (likely(!kvm_s390_pv_cpu_is_protected(vcpu)))
- store_regs_fmt2(vcpu, kvm_run);
+ store_regs_fmt2(vcpu);
}
int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
@@ -4371,7 +4378,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
goto out;
}
- sync_regs(vcpu, kvm_run);
+ sync_regs(vcpu);
enable_cpu_timer_accounting(vcpu);
might_fault();
@@ -4393,7 +4400,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
}
disable_cpu_timer_accounting(vcpu);
- store_regs(vcpu, kvm_run);
+ store_regs(vcpu);
kvm_sigset_deactivate(vcpu);
--
2.17.1
On Wed, 22 Apr 2020 20:58:04 +0800
Tianjia Zhang <[email protected]> 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 <[email protected]>
> ---
> 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 *)
> - &vcpu->run->s.regs.gscb;
> + &kvm_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();
On 22.04.20 15:45, Cornelia Huck wrote:
> On Wed, 22 Apr 2020 20:58:04 +0800
> Tianjia Zhang <[email protected]> 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 <[email protected]>
>> ---
>> 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 *)
>> - &vcpu->run->s.regs.gscb;
>> + &kvm_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.
On Wed, 22 Apr 2020 17:58:04 +0200
Christian Borntraeger <[email protected]> wrote:
> On 22.04.20 15:45, Cornelia Huck wrote:
> > On Wed, 22 Apr 2020 20:58:04 +0800
> > Tianjia Zhang <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 *)
> >> - &vcpu->run->s.regs.gscb;
> >> + &kvm_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.
On 2020/4/22 21:45, Cornelia Huck wrote:
> On Wed, 22 Apr 2020 20:58:04 +0800
> Tianjia Zhang <[email protected]> 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 <[email protected]>
>> ---
>> 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 *)
>> - &vcpu->run->s.regs.gscb;
>> + &kvm_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();
On 2020/4/23 0:04, Cornelia Huck wrote:
> On Wed, 22 Apr 2020 17:58:04 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> On 22.04.20 15:45, Cornelia Huck wrote:
>>> On Wed, 22 Apr 2020 20:58:04 +0800
>>> Tianjia Zhang <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 *)
>>>> - &vcpu->run->s.regs.gscb;
>>>> + &kvm_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
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 *)
>>> - &vcpu->run->s.regs.gscb;
>>> + &kvm_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
On Thu, 23 Apr 2020 11:01:43 +0800
Tianjia Zhang <[email protected]> wrote:
> On 2020/4/23 0:04, Cornelia Huck wrote:
> > On Wed, 22 Apr 2020 17:58:04 +0200
> > Christian Borntraeger <[email protected]> wrote:
> >
> >> On 22.04.20 15:45, Cornelia Huck wrote:
> >>> On Wed, 22 Apr 2020 20:58:04 +0800
> >>> Tianjia Zhang <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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 *)
> >>>> - &vcpu->run->s.regs.gscb;
> >>>> + &kvm_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).
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 <[email protected]> wrote:
>>
>>> On 2020/4/23 0:04, Cornelia Huck wrote:
>>>> On Wed, 22 Apr 2020 17:58:04 +0200
>>>> Christian Borntraeger <[email protected]> wrote:
>>>>
>>>>> On 22.04.20 15:45, Cornelia Huck wrote:
>>>>>> On Wed, 22 Apr 2020 20:58:04 +0800
>>>>>> Tianjia Zhang <[email protected]> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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 *)
>>>>>>> - &vcpu->run->s.regs.gscb;
>>>>>>> + &kvm_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 xxxx ("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.
On 2020/4/23 18:39, Cornelia Huck wrote:
> On Thu, 23 Apr 2020 11:01:43 +0800
> Tianjia Zhang <[email protected]> wrote:
>
>> On 2020/4/23 0:04, Cornelia Huck wrote:
>>> On Wed, 22 Apr 2020 17:58:04 +0200
>>> Christian Borntraeger <[email protected]> wrote:
>>>
>>>> On 22.04.20 15:45, Cornelia Huck wrote:
>>>>> On Wed, 22 Apr 2020 20:58:04 +0800
>>>>> Tianjia Zhang <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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 *)
>>>>>> - &vcpu->run->s.regs.gscb;
>>>>>> + &kvm_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 xxxx ("comment"). Is it
possible to solve this problem by splitting this patch?
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 <[email protected]> wrote:
>>>
>>>> On 2020/4/23 0:04, Cornelia Huck wrote:
>>>>> On Wed, 22 Apr 2020 17:58:04 +0200
>>>>> Christian Borntraeger <[email protected]> wrote:
>>>>>
>>>>>> On 22.04.20 15:45, Cornelia Huck wrote:
>>>>>>> On Wed, 22 Apr 2020 20:58:04 +0800
>>>>>>> Tianjia Zhang <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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 *)
>>>>>>>> - &vcpu->run->s.regs.gscb;
>>>>>>>> + &kvm_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 xxxx ("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
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 <[email protected]> wrote:
>>>
>>>> On 2020/4/23 0:04, Cornelia Huck wrote:
>>>>> On Wed, 22 Apr 2020 17:58:04 +0200
>>>>> Christian Borntraeger <[email protected]> wrote:
>>>>>
>>>>>> On 22.04.20 15:45, Cornelia Huck wrote:
>>>>>>> On Wed, 22 Apr 2020 20:58:04 +0800
>>>>>>> Tianjia Zhang <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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 *)
>>>>>>>> - &vcpu->run->s.regs.gscb;
>>>>>>>> + &kvm_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 xxxx ("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
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 <[email protected]> wrote:
>>>>
>>>>> On 2020/4/23 0:04, Cornelia Huck wrote:
>>>>>> On Wed, 22 Apr 2020 17:58:04 +0200
>>>>>> Christian Borntraeger <[email protected]> wrote:
>>>>>>
>>>>>>> On 22.04.20 15:45, Cornelia Huck wrote:
>>>>>>>> On Wed, 22 Apr 2020 20:58:04 +0800
>>>>>>>> Tianjia Zhang <[email protected]> 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 <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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 *)
>>>>>>>>> - &vcpu->run->s.regs.gscb;
>>>>>>>>> + &kvm_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 xxxx ("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