From: Miaohe Lin <[email protected]>
We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
Signed-off-by: Miaohe Lin <[email protected]>
---
arch/x86/kvm/mtrr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
index 7f0059aa30e1..a98701d9f2bf 100644
--- a/arch/x86/kvm/mtrr.c
+++ b/arch/x86/kvm/mtrr.c
@@ -348,7 +348,7 @@ static void set_var_mtrr_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
int index, is_mtrr_mask;
index = (msr - 0x200) / 2;
- is_mtrr_mask = msr - 0x200 - 2 * index;
+ is_mtrr_mask = (msr - 0x200) % 2;
cur = &mtrr_state->var_ranges[index];
/* remove the entry if it's in the list. */
@@ -424,7 +424,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
int is_mtrr_mask;
index = (msr - 0x200) / 2;
- is_mtrr_mask = msr - 0x200 - 2 * index;
+ is_mtrr_mask = (msr - 0x200) % 2;
if (!is_mtrr_mask)
*pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
else
--
2.19.1
On 05/03/20 03:48, linmiaohe wrote:
> From: Miaohe Lin <[email protected]>
>
> We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> arch/x86/kvm/mtrr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> index 7f0059aa30e1..a98701d9f2bf 100644
> --- a/arch/x86/kvm/mtrr.c
> +++ b/arch/x86/kvm/mtrr.c
> @@ -348,7 +348,7 @@ static void set_var_mtrr_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> int index, is_mtrr_mask;
>
> index = (msr - 0x200) / 2;
> - is_mtrr_mask = msr - 0x200 - 2 * index;
> + is_mtrr_mask = (msr - 0x200) % 2;
> cur = &mtrr_state->var_ranges[index];
>
> /* remove the entry if it's in the list. */
> @@ -424,7 +424,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> int is_mtrr_mask;
>
> index = (msr - 0x200) / 2;
> - is_mtrr_mask = msr - 0x200 - 2 * index;
> + is_mtrr_mask = (msr - 0x200) % 2;
> if (!is_mtrr_mask)
> *pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
> else
>
If you're going to do that, might as well use ">> 1" for index instead
of "/ 2", and "msr & 1" for is_mtrr_mask.
Paolo
From: Paolo Bonzini
> Sent: 05 March 2020 14:36
>
> On 05/03/20 03:48, linmiaohe wrote:
> > From: Miaohe Lin <[email protected]>
> >
> > We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
> >
> > Signed-off-by: Miaohe Lin <[email protected]>
> > ---
> > arch/x86/kvm/mtrr.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> > index 7f0059aa30e1..a98701d9f2bf 100644
> > --- a/arch/x86/kvm/mtrr.c
> > +++ b/arch/x86/kvm/mtrr.c
> > @@ -348,7 +348,7 @@ static void set_var_mtrr_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> > int index, is_mtrr_mask;
> >
> > index = (msr - 0x200) / 2;
> > - is_mtrr_mask = msr - 0x200 - 2 * index;
> > + is_mtrr_mask = (msr - 0x200) % 2;
> > cur = &mtrr_state->var_ranges[index];
> >
> > /* remove the entry if it's in the list. */
> > @@ -424,7 +424,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> > int is_mtrr_mask;
> >
> > index = (msr - 0x200) / 2;
> > - is_mtrr_mask = msr - 0x200 - 2 * index;
> > + is_mtrr_mask = (msr - 0x200) % 2;
> > if (!is_mtrr_mask)
> > *pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
> > else
> >
>
> If you're going to do that, might as well use ">> 1" for index instead
> of "/ 2", and "msr & 1" for is_mtrr_mask.
Provided the variables are unsigned it makes little difference
whether you use / % or >> &.
At least with / % the two values are the same.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 05/03/20 16:10, David Laight wrote:
>>> index = (msr - 0x200) / 2;
>>> - is_mtrr_mask = msr - 0x200 - 2 * index;
>>> + is_mtrr_mask = (msr - 0x200) % 2;
>>> cur = &mtrr_state->var_ranges[index];
>>>
>>> /* remove the entry if it's in the list. */
>>> @@ -424,7 +424,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>>> int is_mtrr_mask;
>>>
>>> index = (msr - 0x200) / 2;
>>> - is_mtrr_mask = msr - 0x200 - 2 * index;
>>> + is_mtrr_mask = (msr - 0x200) % 2;
>>> if (!is_mtrr_mask)
>>> *pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
>>> else
>>>
>> If you're going to do that, might as well use ">> 1" for index instead
>> of "/ 2", and "msr & 1" for is_mtrr_mask.
> Provided the variables are unsigned it makes little difference
> whether you use / % or >> &.
> At least with / % the two values are the same.
Yes, I'm old-fashioned, but also I prefer ">>" and "&" for both signed
and unsigned, because if ever I need to switch from unsigned to signed I
will get floor-division instead of round-to-zero division (most likely
the code doesn't expect negative remainders if it was using unsigned).
(That perhaps also reflects on me working a lot with Smalltalk long
before switching to the kernel...).
Paolo
Hi,
Paolo Bonzini <[email protected]> wrote:
>On 05/03/20 03:48, linmiaohe wrote:
>> From: Miaohe Lin <[email protected]>
>>
>> We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
>> index = (msr - 0x200) / 2;
>> - is_mtrr_mask = msr - 0x200 - 2 * index;
>> + is_mtrr_mask = (msr - 0x200) % 2;
>> if (!is_mtrr_mask)
>> *pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
>> else
>>
>
>If you're going to do that, might as well use ">> 1" for index instead of "/ 2", and "msr & 1" for is_mtrr_mask.
>
Many thanks for suggestion. What do you mean is like this ?
index = (msr - 0x200) >> 1;
is_mtrr_mask = msr & 1;
Thanks again.
On March 5, 2020 6:05:40 PM PST, linmiaohe <[email protected]> wrote:
>Hi,
>Paolo Bonzini <[email protected]> wrote:
>>On 05/03/20 03:48, linmiaohe wrote:
>>> From: Miaohe Lin <[email protected]>
>>>
>>> We can get is_mtrr_mask by calculating (msr - 0x200) % 2 directly.
>>> index = (msr - 0x200) / 2;
>>> - is_mtrr_mask = msr - 0x200 - 2 * index;
>>> + is_mtrr_mask = (msr - 0x200) % 2;
>>> if (!is_mtrr_mask)
>>> *pdata = vcpu->arch.mtrr_state.var_ranges[index].base;
>>> else
>>>
>>
>>If you're going to do that, might as well use ">> 1" for index instead
>of "/ 2", and "msr & 1" for is_mtrr_mask.
>>
>
>Many thanks for suggestion. What do you mean is like this ?
>
> index = (msr - 0x200) >> 1;
> is_mtrr_mask = msr & 1;
>
>Thanks again.
You realize that the compiler will probably produce exactly the same code, right? As such, it is about making the code easy for the human reader.
Even if it didn't, this code is as far from performance critical as one can possibly get.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
[email protected] wrote:
>>On March 5, 2020 6:05:40 PM PST, linmiaohe <[email protected]> wrote:
>>Hi,
>>Paolo Bonzini <[email protected]> wrote:
>>Many thanks for suggestion. What do you mean is like this ?
>>
>> index = (msr - 0x200) >> 1;
>> is_mtrr_mask = msr & 1;
>>
>>Thanks again.
>
>You realize that the compiler will probably produce exactly the same code, right? As such, it is about making the code easy for the human reader.
>
>Even if it didn't, this code is as far from performance critical as one can possibly get.
Yep, it looks gain little. Thanks.
On Fri, 6 Mar 2020 at 10:23, linmiaohe <[email protected]> wrote:
>
> [email protected] wrote:
> >>On March 5, 2020 6:05:40 PM PST, linmiaohe <[email protected]> wrote:
> >>Hi,
> >>Paolo Bonzini <[email protected]> wrote:
> >>Many thanks for suggestion. What do you mean is like this ?
> >>
> >> index = (msr - 0x200) >> 1;
> >> is_mtrr_mask = msr & 1;
> >>
> >>Thanks again.
> >
> >You realize that the compiler will probably produce exactly the same code, right? As such, it is about making the code easy for the human reader.
> >
> >Even if it didn't, this code is as far from performance critical as one can possibly get.
>
> Yep, it looks gain little. Thanks.
Please post the performance number when you mention optimize XXX later.
Wanpeng
Wanpeng Li <[email protected]> wrote:
>On Fri, 6 Mar 2020 at 10:23, linmiaohe <[email protected]> wrote:
>>
>> [email protected] wrote:
>> >>On March 5, 2020 6:05:40 PM PST, linmiaohe <[email protected]> wrote:
>> >>Hi,
>> >>Paolo Bonzini <[email protected]> wrote:
>> >>Many thanks for suggestion. What do you mean is like this ?
>> >>
>> >> index = (msr - 0x200) >> 1;
>> >> is_mtrr_mask = msr & 1;
>> >>
>> >>Thanks again.
>> >
>> >You realize that the compiler will probably produce exactly the same code, right? As such, it is about making the code easy for the human reader.
>> >
>> >Even if it didn't, this code is as far from performance critical as one can possibly get.
>>
>> Yep, it looks gain little. Thanks.
>
>Please post the performance number when you mention optimize XXX later.
>
Sure, I would take care of this. Thanks for your reminder!