[PATCH] KVM: arm64: Remove spurious semicolon in reg_to_encoding()

[PATCH] KVM: arm64: Remove spurious semicolon in reg_to_encoding()

[Marc Zyngier]

Although not a problem right now, it flared up while working
on some other aspects of the code-base. Remove the useless
semicolon.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d46e7f706cb0..42ccc27fb684 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -923,7 +923,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 
 #define reg_to_encoding(x)						\
 	sys_reg((u32)(x)->Op0, (u32)(x)->Op1,				\
-		(u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2);
+		(u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
 
 /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
 #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Remove spurious semicolon in reg_to_encoding()

[Alexandru Elisei]

Hi Marc,

On 12/31/20 3:09 PM, Marc Zyngier wrote:
> Although not a problem right now, it flared up while working
> on some other aspects of the code-base. Remove the useless
> semicolon.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d46e7f706cb0..42ccc27fb684 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -923,7 +923,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  
>  #define reg_to_encoding(x)						\
>  	sys_reg((u32)(x)->Op0, (u32)(x)->Op1,				\
> -		(u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2);
> +		(u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)

Macro's shouldn't really end with a semicolon, otherwise code might break
unexpectedly:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

I skimmed over the code and the macro is used like a function. Do you think it's
worth turning it into a function to gain type checking for the argument (which is
expected to be sys_reg_desc) and for the return value?

Thanks,
Alex
>  
>  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Remove spurious semicolon in reg_to_encoding()

[Marc Zyngier]

On 2021-01-05 13:54, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 12/31/20 3:09 PM, Marc Zyngier wrote:
>> Although not a problem right now, it flared up while working
>> on some other aspects of the code-base. Remove the useless
>> semicolon.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index d46e7f706cb0..42ccc27fb684 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -923,7 +923,7 @@ static bool access_pmuserenr(struct kvm_vcpu 
>> *vcpu, struct sys_reg_params *p,
>> 
>>  #define reg_to_encoding(x)						\
>>  	sys_reg((u32)(x)->Op0, (u32)(x)->Op1,				\
>> -		(u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2);
>> +		(u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
> 
> Macro's shouldn't really end with a semicolon, otherwise code might 
> break
> unexpectedly:
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> I skimmed over the code and the macro is used like a function. Do you 
> think it's
> worth turning it into a function to gain type checking for the
> argument (which is
> expected to be sys_reg_desc) and for the return value?

Not quite.

This macro takes indifferently a sys_reg_desc or a sys_reg_params,
making it impossible to turn into a function. Is it disgusting and
makes you want to pull your eyes out? Yes.

The whole params/desc duality dates back to the early 32bit code,
and I blame Rusty Russell for it. Just saying... ;-)

That's one of things I always wanted to refactor...

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Remove spurious semicolon in reg_to_encoding()

[Alexandru Elisei]

Hi Marc,

On 1/5/21 2:04 PM, Marc Zyngier wrote:
> On 2021-01-05 13:54, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> On 12/31/20 3:09 PM, Marc Zyngier wrote:
>>> Although not a problem right now, it flared up while working
>>> on some other aspects of the code-base. Remove the useless
>>> semicolon.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/kvm/sys_regs.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index d46e7f706cb0..42ccc27fb684 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -923,7 +923,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct
>>> sys_reg_params *p,
>>>
>>>  #define reg_to_encoding(x)                        \
>>>      sys_reg((u32)(x)->Op0, (u32)(x)->Op1,                \
>>> -        (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2);
>>> +        (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
>>
>> Macro's shouldn't really end with a semicolon, otherwise code might break
>> unexpectedly:
>>
>> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>
>> I skimmed over the code and the macro is used like a function. Do you think it's
>> worth turning it into a function to gain type checking for the
>> argument (which is
>> expected to be sys_reg_desc) and for the return value?
>
> Not quite.
>
> This macro takes indifferently a sys_reg_desc or a sys_reg_params,

Right, find_reg() calls reg_to_encoding() with a struct sys_reg_params argument, I
missed that.

Found a few instances where sys_reg() calls can be replaced with
reg_to_encoding(), will send a trivial patch for that.

Thanks,
Alex
> making it impossible to turn into a function. Is it disgusting and
> makes you want to pull your eyes out? Yes.
>
> The whole params/desc duality dates back to the early 32bit code,
> and I blame Rusty Russell for it. Just saying... ;-)
>
> That's one of things I always wanted to refactor...
>
> Thanks,
>
>         M.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel