[PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations

Xenia Ragiadakou posted 4 patches 3 years, 7 months ago
There is a newer version of this series
[PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations
Posted by Xenia Ragiadakou 3 years, 7 months ago
Add the function prototypes of the functions below in <asm/processor.h> header
file so that they are visible before the function definitions in traps.c.
enter_hypervisor_from_guest_preirq()
enter_hypervisor_from_guest()
do_trap_hyp_sync()
do_trap_guest_sync()
do_trap_irq()
do_trap_fiq()
leave_hypervisor_to_guest()

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/arm/include/asm/processor.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index c021160412..74cc07028f 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -576,10 +576,24 @@ void vcpu_regs_hyp_to_user(const struct vcpu *vcpu,
 void vcpu_regs_user_to_hyp(struct vcpu *vcpu,
                            const struct vcpu_guest_core_regs *regs);
 
+void enter_hypervisor_from_guest_preirq(void);
+
+void enter_hypervisor_from_guest(void);
+
+void do_trap_hyp_sync(struct cpu_user_regs *regs);
+
+void do_trap_guest_sync(struct cpu_user_regs *regs);
+
 void do_trap_hyp_serror(struct cpu_user_regs *regs);
 
 void do_trap_guest_serror(struct cpu_user_regs *regs);
 
+void do_trap_irq(struct cpu_user_regs *regs);
+
+void do_trap_fiq(struct cpu_user_regs *regs);
+
+void leave_hypervisor_to_guest(void);
+
 register_t get_default_hcr_flags(void);
 
 /*
-- 
2.34.1
Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations
Posted by Julien Grall 3 years, 7 months ago
Hi Xenia,

On 05/07/2022 22:02, Xenia Ragiadakou wrote:
> Add the function prototypes of the functions below in <asm/processor.h> header
> file so that they are visible before the function definitions in traps.c.
> enter_hypervisor_from_guest_preirq()
> enter_hypervisor_from_guest()
> do_trap_hyp_sync()
> do_trap_guest_sync()
> do_trap_irq()
> do_trap_fiq()
> leave_hypervisor_to_guest()

I have actually came across those missing prototypes in the past and I 
have refrained to add them because they are not meant to be called from C.

While I agree this is a violation of Misra C, I am still not convinced 
they should be added (there is no need to verify the prototype).

So IMHO, they should be marked as deviation.

Cheers,

-- 
Julien Grall
Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations
Posted by Xenia Ragiadakou 3 years, 7 months ago
Hi Julien,

On 7/6/22 00:28, Julien Grall wrote:
> Hi Xenia,
> 
> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>> Add the function prototypes of the functions below in 
>> <asm/processor.h> header
>> file so that they are visible before the function definitions in traps.c.
>> enter_hypervisor_from_guest_preirq()
>> enter_hypervisor_from_guest()
>> do_trap_hyp_sync()
>> do_trap_guest_sync()
>> do_trap_irq()
>> do_trap_fiq()
>> leave_hypervisor_to_guest()
> 
> I have actually came across those missing prototypes in the past and I 
> have refrained to add them because they are not meant to be called from C.
> 
> While I agree this is a violation of Misra C, I am still not convinced 
> they should be added (there is no need to verify the prototype).

Yes, there is no need. Here, I decided to follow the example of 
do_trap_hyp_serror() and do_trap_guest_serror() for consistency.

> 
> So IMHO, they should be marked as deviation.
> 
> Cheers,
> 

-- 
Xenia
Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations
Posted by Julien Grall 3 years, 7 months ago
Hi Xenia,

On 05/07/2022 22:49, Xenia Ragiadakou wrote:
> On 7/6/22 00:28, Julien Grall wrote:
>> Hi Xenia,
>>
>> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>>> Add the function prototypes of the functions below in 
>>> <asm/processor.h> header
>>> file so that they are visible before the function definitions in 
>>> traps.c.
>>> enter_hypervisor_from_guest_preirq()
>>> enter_hypervisor_from_guest()
>>> do_trap_hyp_sync()
>>> do_trap_guest_sync()
>>> do_trap_irq()
>>> do_trap_fiq()
>>> leave_hypervisor_to_guest()
>>
>> I have actually came across those missing prototypes in the past and I 
>> have refrained to add them because they are not meant to be called 
>> from C.
>>
>> While I agree this is a violation of Misra C, I am still not convinced 
>> they should be added (there is no need to verify the prototype).
> 
> Yes, there is no need. Here, I decided to follow the example of 
> do_trap_hyp_serror() and do_trap_guest_serror() for consistency.

do_trap_guest_serror() is called from C code (see arch/arm32/traps.c).

do_trap_hyp_serror() is called only from assembly. I would argue that if 
you want consistency, then you should drop the prototype rather than 
modifying 90% of the other examples.

Otherwise, this is not really consistency but more a design choice (you 
want to be fully compliant with MISRA).

Cheers,

-- 
Julien Grall
Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations
Posted by Xenia Ragiadakou 3 years, 7 months ago
Hi Julien,

On 7/6/22 00:56, Julien Grall wrote:
> On 05/07/2022 22:49, Xenia Ragiadakou wrote:
>> On 7/6/22 00:28, Julien Grall wrote:
>>> Hi Xenia,
>>>
>>> On 05/07/2022 22:02, Xenia Ragiadakou wrote:
>>>> Add the function prototypes of the functions below in 
>>>> <asm/processor.h> header
>>>> file so that they are visible before the function definitions in 
>>>> traps.c.
>>>> enter_hypervisor_from_guest_preirq()
>>>> enter_hypervisor_from_guest()
>>>> do_trap_hyp_sync()
>>>> do_trap_guest_sync()
>>>> do_trap_irq()
>>>> do_trap_fiq()
>>>> leave_hypervisor_to_guest()
>>>
>>> I have actually came across those missing prototypes in the past and 
>>> I have refrained to add them because they are not meant to be called 
>>> from C.
>>>
>>> While I agree this is a violation of Misra C, I am still not 
>>> convinced they should be added (there is no need to verify the 
>>> prototype).
>>
>> Yes, there is no need. Here, I decided to follow the example of 
>> do_trap_hyp_serror() and do_trap_guest_serror() for consistency.
> 
> do_trap_guest_serror() is called from C code (see arch/arm32/traps.c).

You are right, my mistake.

> 
> do_trap_hyp_serror() is called only from assembly. I would argue that if 
> you want consistency, then you should drop the prototype rather than 
> modifying 90% of the other examples.
> 
> Otherwise, this is not really consistency but more a design choice (you 
> want to be fully compliant with MISRA).

I agree with you that there is no need to declare the variables and 
functions that are called only from assembly, prior to their 
definitions. So, this kind of violations could be grouped together and 
provide a formal deviation. This seems to me reasonable.

-- 
Xenia
Re: [PATCH 1/4] xen/arm: traps: Fix MISRA C 2012 Rule 8.4 violations
Posted by Stefano Stabellini 3 years, 7 months ago
On Tue, 5 Jul 2022, Julien Grall wrote:
> Hi Xenia,
> 
> On 05/07/2022 22:49, Xenia Ragiadakou wrote:
> > On 7/6/22 00:28, Julien Grall wrote:
> > > Hi Xenia,
> > > 
> > > On 05/07/2022 22:02, Xenia Ragiadakou wrote:
> > > > Add the function prototypes of the functions below in <asm/processor.h>
> > > > header
> > > > file so that they are visible before the function definitions in
> > > > traps.c.
> > > > enter_hypervisor_from_guest_preirq()
> > > > enter_hypervisor_from_guest()
> > > > do_trap_hyp_sync()
> > > > do_trap_guest_sync()
> > > > do_trap_irq()
> > > > do_trap_fiq()
> > > > leave_hypervisor_to_guest()
> > > 
> > > I have actually came across those missing prototypes in the past and I
> > > have refrained to add them because they are not meant to be called from C.
> > > 
> > > While I agree this is a violation of Misra C, I am still not convinced
> > > they should be added (there is no need to verify the prototype).
> > 
> > Yes, there is no need. Here, I decided to follow the example of
> > do_trap_hyp_serror() and do_trap_guest_serror() for consistency.
> 
> do_trap_guest_serror() is called from C code (see arch/arm32/traps.c).
> 
> do_trap_hyp_serror() is called only from assembly. I would argue that if you
> want consistency, then you should drop the prototype rather than modifying 90%
> of the other examples.
> 
> Otherwise, this is not really consistency but more a design choice (you want
> to be fully compliant with MISRA).

Actually I am not sure that MISRA requires prototypes for assembly
functions only meant to be called from assembly. I think MISRA requires
detailed docs for anything assembly, but I don't think it requires C
prototypes for all assembly functions.

So I think we could skip them for now. At some point we'll discuss what
we need to do for the assembly code but we haven't tackled that yet.