* [PATCH] arm64: KVM: debug: Remove spurious inline attributes
@ 2015-12-16 15:49 Marc Zyngier
2015-12-16 20:45 ` Christoffer Dall
2015-12-17 16:28 ` Alex Bennée
0 siblings, 2 replies; 5+ messages in thread
From: Marc Zyngier @ 2015-12-16 15:49 UTC (permalink / raw)
To: Christoffer Dall; +Cc: kvm, kvmarm, alex.bennee
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 <alex.bennee@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
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 &= 0xffffffffUL;
}
-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,
return 0;
}
-static inline void reset_wcr(struct kvm_vcpu *vcpu,
- const struct sys_reg_desc *rd)
+static void reset_wcr(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd)
{
vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg] = rd->val;
}
@@ -723,9 +723,9 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
* system is in.
*/
-static inline bool trap_xvr(struct kvm_vcpu *vcpu,
- struct sys_reg_params *p,
- const struct sys_reg_desc *rd)
+static bool trap_xvr(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];
--
2.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: KVM: debug: Remove spurious inline attributes
2015-12-16 15:49 [PATCH] arm64: KVM: debug: Remove spurious inline attributes Marc Zyngier
@ 2015-12-16 20:45 ` Christoffer Dall
2015-12-17 16:28 ` Alex Bennée
1 sibling, 0 replies; 5+ messages in thread
From: Christoffer Dall @ 2015-12-16 20:45 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, kvmarm, alex.bennee
On Wed, Dec 16, 2015 at 03:49:23PM +0000, 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 <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: KVM: debug: Remove spurious inline attributes
2015-12-16 15:49 [PATCH] arm64: KVM: debug: Remove spurious inline attributes Marc Zyngier
2015-12-16 20:45 ` Christoffer Dall
@ 2015-12-17 16:28 ` Alex Bennée
2015-12-17 16:51 ` Marc Zyngier
1 sibling, 1 reply; 5+ messages in thread
From: Alex Bennée @ 2015-12-17 16:28 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Christoffer Dall, kvm, kvmarm
Marc Zyngier <marc.zyngier@arm.com> 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 <alex.bennee@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> 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 &= 0xffffffffUL;
> }
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,
> return 0;
> }
>
> -static inline void reset_wcr(struct kvm_vcpu *vcpu,
> - const struct sys_reg_desc *rd)
> +static void reset_wcr(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd)
> {
> vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg] = rd->val;
> }
> @@ -723,9 +723,9 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
> * system is in.
> */
>
> -static inline bool trap_xvr(struct kvm_vcpu *vcpu,
> - struct sys_reg_params *p,
> - const struct sys_reg_desc *rd)
> +static bool trap_xvr(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];
The rest all make sense. I wonder why I was being so inline happy?
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: KVM: debug: Remove spurious inline attributes
2015-12-17 16:28 ` Alex Bennée
@ 2015-12-17 16:51 ` Marc Zyngier
2015-12-17 20:21 ` Alex Bennée
0 siblings, 1 reply; 5+ messages in thread
From: Marc Zyngier @ 2015-12-17 16:51 UTC (permalink / raw)
To: Alex Bennée; +Cc: kvm, kvmarm
On 17/12/15 16:28, Alex Bennée wrote:
>
> Marc Zyngier <marc.zyngier@arm.com> 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 <alex.bennee@linaro.org>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> 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 &= 0xffffffffUL;
>> }
>
> 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 <alex.bennee@linaro.org>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: KVM: debug: Remove spurious inline attributes
2015-12-17 16:51 ` Marc Zyngier
@ 2015-12-17 20:21 ` Alex Bennée
0 siblings, 0 replies; 5+ messages in thread
From: Alex Bennée @ 2015-12-17 20:21 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Christoffer Dall, kvm, kvmarm
Marc Zyngier <marc.zyngier@arm.com> writes:
> On 17/12/15 16:28, Alex Bennée wrote:
>>
>> Marc Zyngier <marc.zyngier@arm.com> 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 <alex.bennee@linaro.org>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>> 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 &= 0xffffffffUL;
>>> }
>>
>> 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 <alex.bennee@linaro.org>
>
> Thanks,
>
> M.
--
Alex Bennée
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-17 20:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-16 15:49 [PATCH] arm64: KVM: debug: Remove spurious inline attributes Marc Zyngier
2015-12-16 20:45 ` Christoffer Dall
2015-12-17 16:28 ` Alex Bennée
2015-12-17 16:51 ` Marc Zyngier
2015-12-17 20:21 ` Alex Bennée
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.