Re: [PATCH] arm64: KVM: debug: Remove spurious inline attributes
Marc Zyngier writes:
> On 17/12/15 16:28, Alex Bennée wrote:
>>
>> Marc Zyngier writes:
>>
>>> The debug trapping code is pretty heavy on the "inline" attribute,
>>> but most functions are actually referenced in the sysreg tables,
>>> making the inlining imposible.
>>>
>>> Removing the useless inline qualifier seems the right thing to do,
>>> having verified that the output code is similar.
>>>
>>> Cc: Alex Bennée
>>> Signed-off-by: Marc Zyngier
>>> ---
>>> arch/arm64/kvm/sys_regs.c | 58
>>> +++
>>> 1 file changed, 29 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 88adebf..eec3598 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -220,9 +220,9 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
>>> * All writes will set the KVM_ARM64_DEBUG_DIRTY flag to ensure the
>>> * hyp.S code switches between host and guest values in future.
>>> */
>>> -static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
>>> - struct sys_reg_params *p,
>>> - u64 *dbg_reg)
>>> +static void reg_to_dbg(struct kvm_vcpu *vcpu,
>>> + struct sys_reg_params *p,
>>> + u64 *dbg_reg)
>>> {
>>> u64 val = p->regval;
>>>
>>> @@ -235,18 +235,18 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
>>> vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>>> }
>>>
>>> -static inline void dbg_to_reg(struct kvm_vcpu *vcpu,
>>> - struct sys_reg_params *p,
>>> - u64 *dbg_reg)
>>> +static void dbg_to_reg(struct kvm_vcpu *vcpu,
>>> + struct sys_reg_params *p,
>>> + u64 *dbg_reg)
>>> {
>>> p->regval = *dbg_reg;
>>> if (p->is_32bit)
>>> p->regval &= 0xUL;
>>> }
>>
>> Christoffer's "register keyword" comments not-withstanding I'd prefer to
>> keep the reg_to_dbg/dbg_to_reg functions as inline because they really
>> are just boilerplate helpers I didn't want to repeat in the actual
>> access functions - although if you've looked at the code I assume that
>> means GCC has been smart about it.
>
> Indeed, GCC is smart enough to directly inline it. In general, GCC is
> doing a pretty good job at inlining static functions that are small
> enough not to be worth jumping to. These days, "static inline" only
> really makes sense in an include file.
Fair enough.
>
>> The rest all make sense. I wonder why I was being so inline happy?
>>
>> Reviewed-by: Alex Bennée
>
> Thanks,
>
> M.
--
Alex Bennée
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm64: KVM: debug: Remove spurious inline attributes
On 17/12/15 16:28, Alex Bennée wrote:
>
> Marc Zyngier writes:
>
>> The debug trapping code is pretty heavy on the "inline" attribute,
>> but most functions are actually referenced in the sysreg tables,
>> making the inlining imposible.
>>
>> Removing the useless inline qualifier seems the right thing to do,
>> having verified that the output code is similar.
>>
>> Cc: Alex Bennée
>> Signed-off-by: Marc Zyngier
>> ---
>> arch/arm64/kvm/sys_regs.c | 58
>> +++
>> 1 file changed, 29 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 88adebf..eec3598 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -220,9 +220,9 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
>> * All writes will set the KVM_ARM64_DEBUG_DIRTY flag to ensure the
>> * hyp.S code switches between host and guest values in future.
>> */
>> -static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
>> - struct sys_reg_params *p,
>> - u64 *dbg_reg)
>> +static void reg_to_dbg(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *p,
>> + u64 *dbg_reg)
>> {
>> u64 val = p->regval;
>>
>> @@ -235,18 +235,18 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
>> vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>> }
>>
>> -static inline void dbg_to_reg(struct kvm_vcpu *vcpu,
>> - struct sys_reg_params *p,
>> - u64 *dbg_reg)
>> +static void dbg_to_reg(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *p,
>> + u64 *dbg_reg)
>> {
>> p->regval = *dbg_reg;
>> if (p->is_32bit)
>> p->regval &= 0xUL;
>> }
>
> Christoffer's "register keyword" comments not-withstanding I'd prefer to
> keep the reg_to_dbg/dbg_to_reg functions as inline because they really
> are just boilerplate helpers I didn't want to repeat in the actual
> access functions - although if you've looked at the code I assume that
> means GCC has been smart about it.
Indeed, GCC is smart enough to directly inline it. In general, GCC is
doing a pretty good job at inlining static functions that are small
enough not to be worth jumping to. These days, "static inline" only
really makes sense in an include file.
> The rest all make sense. I wonder why I was being so inline happy?
>
> Reviewed-by: Alex Bennée
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm64: KVM: debug: Remove spurious inline attributes
Marc Zyngier writes:
> The debug trapping code is pretty heavy on the "inline" attribute,
> but most functions are actually referenced in the sysreg tables,
> making the inlining imposible.
>
> Removing the useless inline qualifier seems the right thing to do,
> having verified that the output code is similar.
>
> Cc: Alex Bennée
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/kvm/sys_regs.c | 58
> +++
> 1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 88adebf..eec3598 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -220,9 +220,9 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
> * All writes will set the KVM_ARM64_DEBUG_DIRTY flag to ensure the
> * hyp.S code switches between host and guest values in future.
> */
> -static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
> - struct sys_reg_params *p,
> - u64 *dbg_reg)
> +static void reg_to_dbg(struct kvm_vcpu *vcpu,
> +struct sys_reg_params *p,
> +u64 *dbg_reg)
> {
> u64 val = p->regval;
>
> @@ -235,18 +235,18 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
> vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> }
>
> -static inline void dbg_to_reg(struct kvm_vcpu *vcpu,
> - struct sys_reg_params *p,
> - u64 *dbg_reg)
> +static void dbg_to_reg(struct kvm_vcpu *vcpu,
> +struct sys_reg_params *p,
> +u64 *dbg_reg)
> {
> p->regval = *dbg_reg;
> if (p->is_32bit)
> p->regval &= 0xUL;
> }
Christoffer's "register keyword" comments not-withstanding I'd prefer to
keep the reg_to_dbg/dbg_to_reg functions as inline because they really
are just boilerplate helpers I didn't want to repeat in the actual
access functions - although if you've looked at the code I assume that
means GCC has been smart about it.
>
> -static inline bool trap_bvr(struct kvm_vcpu *vcpu,
> - struct sys_reg_params *p,
> - const struct sys_reg_desc *rd)
> +static bool trap_bvr(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *p,
> + const struct sys_reg_desc *rd)
> {
> u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
>
> @@ -280,15 +280,15 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct
> sys_reg_desc *rd,
> return 0;
> }
>
> -static inline void reset_bvr(struct kvm_vcpu *vcpu,
> - const struct sys_reg_desc *rd)
> +static void reset_bvr(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd)
> {
> vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg] = rd->val;
> }
>
> -static inline bool trap_bcr(struct kvm_vcpu *vcpu,
> - struct sys_reg_params *p,
> - const struct sys_reg_desc *rd)
> +static bool trap_bcr(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *p,
> + const struct sys_reg_desc *rd)
> {
> u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
>
> @@ -323,15 +323,15 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct
> sys_reg_desc *rd,
> return 0;
> }
>
> -static inline void reset_bcr(struct kvm_vcpu *vcpu,
> - const struct sys_reg_desc *rd)
> +static void reset_bcr(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd)
> {
> vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg] = rd->val;
> }
>
> -static inline bool trap_wvr(struct kvm_vcpu *vcpu,
> - struct sys_reg_params *p,
> - const struct sys_reg_desc *rd)
> +static bool trap_wvr(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *p,
> + const struct sys_reg_desc *rd)
> {
> u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
>
> @@ -366,15 +366,15 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct
> sys_reg_desc *rd,
> return 0;
> }
>
> -static inline void reset_wvr(struct kvm_vcpu *vcpu,
> - const struct sys_reg_desc *rd)
> +static void reset_wvr(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd)
> {
> vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg] = rd->val;
> }
>
> -static inline bool trap_wcr(struct kvm_vcpu *vcpu,
> - struct sys_reg_params *p,
> - const struct sys_reg_desc *rd)
> +static bool trap_wcr(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *p,
> + const struct sys_reg_desc *rd)
> {
> u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
>
> @@ -408,8 +408,8 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct
> sys_reg_desc *rd,
>
Re: [PATCH] arm64: KVM: debug: Remove spurious inline attributes
On Wed, Dec 16, 2015 at 03:49:23PM +, Marc Zyngier wrote: > The debug trapping code is pretty heavy on the "inline" attribute, > but most functions are actually referenced in the sysreg tables, > making the inlining imposible. > > Removing the useless inline qualifier seems the right thing to do, > having verified that the output code is similar. To quote Rusty when he reviewd some of my first KVM patches: "I consider inline the register keyword of the 90s" You can take that as an: Acked-by: Christoffer Dall -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
