diff mbox series

[XEN] ioreq: include arch-specific ioreq header in <xen/ioreq.h>

Message ID e5f13920dfcb9f828abb4a36dd410d342f4c0939.1692974235.git.nicola.vetrini@bugseng.com (mailing list archive)
State New, archived
Headers show
Series [XEN] ioreq: include arch-specific ioreq header in <xen/ioreq.h> | expand

Commit Message

Nicola Vetrini Aug. 25, 2023, 3:02 p.m. UTC
The common header file for ioreq should include the arch-specific one.
This also addresses violations of MISRA C:2012 Rule 8.4 caused by the missing
inclusion of <asm/ioreq.h> in the arm implementation file.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
- The deleted includes are therefore no longer necessary, since
 <xen/ioreq.h> is sufficient.
- The functions (possibly) missing a visible declaration prior to their definition
  currently are 'handle_ioserv' and 'try_fwd_ioserv'
---
 xen/arch/arm/io.c       | 1 -
 xen/common/ioreq.c      | 1 -
 xen/include/xen/ioreq.h | 1 +
 3 files changed, 1 insertion(+), 2 deletions(-)

--
2.34.1

Comments

Stefano Stabellini Aug. 25, 2023, 7:29 p.m. UTC | #1
On Fri, 25 Aug 2023, Nicola Vetrini wrote:
> The common header file for ioreq should include the arch-specific one.
> This also addresses violations of MISRA C:2012 Rule 8.4 caused by the missing
> inclusion of <asm/ioreq.h> in the arm implementation file.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> - The deleted includes are therefore no longer necessary, since
>  <xen/ioreq.h> is sufficient.
> - The functions (possibly) missing a visible declaration prior to their definition
>   currently are 'handle_ioserv' and 'try_fwd_ioserv'
> ---
>  xen/arch/arm/io.c       | 1 -
>  xen/common/ioreq.c      | 1 -
>  xen/include/xen/ioreq.h | 1 +
>  3 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 96c740d5636c..13ae1fed5718 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -14,7 +14,6 @@
>  #include <xen/sort.h>
>  #include <asm/cpuerrata.h>
>  #include <asm/current.h>
> -#include <asm/ioreq.h>
>  #include <asm/mmio.h>
>  #include <asm/traps.h>
> 
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index 7cb717f7a2a4..bde1a1f4eaa1 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -28,7 +28,6 @@
>  #include <xen/trace.h>
> 
>  #include <asm/guest_atomics.h>
> -#include <asm/ioreq.h>
> 
>  #include <public/hvm/ioreq.h>
>  #include <public/hvm/params.h>
> diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
> index a26614d331e3..d85477c665e9 100644
> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -20,6 +20,7 @@
>  #define __XEN_IOREQ_H__
> 
>  #include <xen/sched.h>
> +#include <asm/ioreq.h>
> 
>  #include <public/hvm/dm_op.h>
> 
> --
> 2.34.1
>
Jan Beulich Aug. 28, 2023, 7:59 a.m. UTC | #2
On 25.08.2023 17:02, Nicola Vetrini wrote:
> The common header file for ioreq should include the arch-specific one.

To be honest I'm not convinced of "should" here. There are two aspects
to consider: On one hand it is good practice to do what you say. Otoh it
introduces a needless dependency, and it'll require new arch-es (RISC-V,
PPC at present) to introduce another dummy header just to satisfy the
xen/ioreq.h use in common/memory.c. Therefore, _if_ we want to go this
route, besides a wording change here I think ...

> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -20,6 +20,7 @@
>  #define __XEN_IOREQ_H__
> 
>  #include <xen/sched.h>
> +#include <asm/ioreq.h>

... this #include wants to be conditional upon CONFIG_IOREQ_SERVER being
defined. (I'm actually surprised that there's no respective #ifdef in
that header, meaning types defined there are available even when the
functionality was turned off.)

Jan
Nicola Vetrini Sept. 1, 2023, 7:13 a.m. UTC | #3
On 28/08/2023 09:59, Jan Beulich wrote:
> On 25.08.2023 17:02, Nicola Vetrini wrote:
>> The common header file for ioreq should include the arch-specific one.
> 
> To be honest I'm not convinced of "should" here. There are two aspects
> to consider: On one hand it is good practice to do what you say. Otoh 
> it
> introduces a needless dependency, and it'll require new arch-es 
> (RISC-V,
> PPC at present) to introduce another dummy header just to satisfy the
> xen/ioreq.h use in common/memory.c. Therefore, _if_ we want to go this
> route, besides a wording change here I think ...
> 

Since including both <xen/ioreq.h> and the arch-specific one was 
requested to be changed
in previous series, I was trying to apply the same pattern here.
If you prefer not to change the current layout then the fix is even 
simpler: I'll just
include <asm/ioreq.h> in xen/arch/arm/ioreq.c, which is where it's 
missing.

>> --- a/xen/include/xen/ioreq.h
>> +++ b/xen/include/xen/ioreq.h
>> @@ -20,6 +20,7 @@
>>  #define __XEN_IOREQ_H__
>> 
>>  #include <xen/sched.h>
>> +#include <asm/ioreq.h>
> 
> ... this #include wants to be conditional upon CONFIG_IOREQ_SERVER 
> being
> defined. (I'm actually surprised that there's no respective #ifdef in
> that header, meaning types defined there are available even when the
> functionality was turned off.)
> 
> Jan

The two functions in question are actually guarded by an #ifdef 
CONFIG_IOREQ_SERVER
in arch/arm/include/asm/ioreq.h (in the #else branch some stubs are 
defined)
Jan Beulich Sept. 1, 2023, 7:36 a.m. UTC | #4
On 01.09.2023 09:13, Nicola Vetrini wrote:
> On 28/08/2023 09:59, Jan Beulich wrote:
>> On 25.08.2023 17:02, Nicola Vetrini wrote:
>>> The common header file for ioreq should include the arch-specific one.
>>
>> To be honest I'm not convinced of "should" here. There are two aspects
>> to consider: On one hand it is good practice to do what you say. Otoh 
>> it
>> introduces a needless dependency, and it'll require new arch-es 
>> (RISC-V,
>> PPC at present) to introduce another dummy header just to satisfy the
>> xen/ioreq.h use in common/memory.c. Therefore, _if_ we want to go this
>> route, besides a wording change here I think ...
>>
> 
> Since including both <xen/ioreq.h> and the arch-specific one was 
> requested to be changed
> in previous series, I was trying to apply the same pattern here.
> If you prefer not to change the current layout then the fix is even 
> simpler: I'll just
> include <asm/ioreq.h> in xen/arch/arm/ioreq.c, which is where it's 
> missing.
> 
>>> --- a/xen/include/xen/ioreq.h
>>> +++ b/xen/include/xen/ioreq.h
>>> @@ -20,6 +20,7 @@
>>>  #define __XEN_IOREQ_H__
>>>
>>>  #include <xen/sched.h>
>>> +#include <asm/ioreq.h>
>>
>> ... this #include wants to be conditional upon CONFIG_IOREQ_SERVER 
>> being
>> defined. (I'm actually surprised that there's no respective #ifdef in
>> that header, meaning types defined there are available even when the
>> functionality was turned off.)
> 
> The two functions in question are actually guarded by an #ifdef 
> CONFIG_IOREQ_SERVER
> in arch/arm/include/asm/ioreq.h (in the #else branch some stubs are 
> defined)

Well, I don't see how an #ifdef there helps with the aspect mentioned
earlier (new arch-es needing to needlessly provide such a header as long
as the #include here is unconditional).

Jan
Nicola Vetrini Sept. 1, 2023, 9:13 a.m. UTC | #5
On 01/09/2023 09:36, Jan Beulich wrote:
> On 01.09.2023 09:13, Nicola Vetrini wrote:
>> On 28/08/2023 09:59, Jan Beulich wrote:
>>> On 25.08.2023 17:02, Nicola Vetrini wrote:
>>>> The common header file for ioreq should include the arch-specific 
>>>> one.
>>> 
>>> To be honest I'm not convinced of "should" here. There are two 
>>> aspects
>>> to consider: On one hand it is good practice to do what you say. Otoh
>>> it
>>> introduces a needless dependency, and it'll require new arch-es
>>> (RISC-V,
>>> PPC at present) to introduce another dummy header just to satisfy the
>>> xen/ioreq.h use in common/memory.c. Therefore, _if_ we want to go 
>>> this
>>> route, besides a wording change here I think ...
>>> 
>> 
>> Since including both <xen/ioreq.h> and the arch-specific one was
>> requested to be changed
>> in previous series, I was trying to apply the same pattern here.
>> If you prefer not to change the current layout then the fix is even
>> simpler: I'll just
>> include <asm/ioreq.h> in xen/arch/arm/ioreq.c, which is where it's
>> missing.
>> 
>>>> --- a/xen/include/xen/ioreq.h
>>>> +++ b/xen/include/xen/ioreq.h
>>>> @@ -20,6 +20,7 @@
>>>>  #define __XEN_IOREQ_H__
>>>> 
>>>>  #include <xen/sched.h>
>>>> +#include <asm/ioreq.h>
>>> 
>>> ... this #include wants to be conditional upon CONFIG_IOREQ_SERVER
>>> being
>>> defined. (I'm actually surprised that there's no respective #ifdef in
>>> that header, meaning types defined there are available even when the
>>> functionality was turned off.)
>> 
>> The two functions in question are actually guarded by an #ifdef
>> CONFIG_IOREQ_SERVER
>> in arch/arm/include/asm/ioreq.h (in the #else branch some stubs are
>> defined)
> 
> Well, I don't see how an #ifdef there helps with the aspect mentioned
> earlier (new arch-es needing to needlessly provide such a header as 
> long
> as the #include here is unconditional).
> 

As far as I can tell, including the asm header in the arm implementation 
file does not imply
that new archs will need such an header. Of course, if the solution 
proposed in the patch is
chosen then I agree with you.
Jan Beulich Sept. 1, 2023, 9:44 a.m. UTC | #6
On 01.09.2023 11:13, Nicola Vetrini wrote:
> On 01/09/2023 09:36, Jan Beulich wrote:
>> On 01.09.2023 09:13, Nicola Vetrini wrote:
>>> On 28/08/2023 09:59, Jan Beulich wrote:
>>>> On 25.08.2023 17:02, Nicola Vetrini wrote:
>>>>> The common header file for ioreq should include the arch-specific 
>>>>> one.
>>>>
>>>> To be honest I'm not convinced of "should" here. There are two 
>>>> aspects
>>>> to consider: On one hand it is good practice to do what you say. Otoh
>>>> it
>>>> introduces a needless dependency, and it'll require new arch-es
>>>> (RISC-V,
>>>> PPC at present) to introduce another dummy header just to satisfy the
>>>> xen/ioreq.h use in common/memory.c. Therefore, _if_ we want to go 
>>>> this
>>>> route, besides a wording change here I think ...
>>>>
>>>
>>> Since including both <xen/ioreq.h> and the arch-specific one was
>>> requested to be changed
>>> in previous series, I was trying to apply the same pattern here.
>>> If you prefer not to change the current layout then the fix is even
>>> simpler: I'll just
>>> include <asm/ioreq.h> in xen/arch/arm/ioreq.c, which is where it's
>>> missing.
>>>
>>>>> --- a/xen/include/xen/ioreq.h
>>>>> +++ b/xen/include/xen/ioreq.h
>>>>> @@ -20,6 +20,7 @@
>>>>>  #define __XEN_IOREQ_H__
>>>>>
>>>>>  #include <xen/sched.h>
>>>>> +#include <asm/ioreq.h>
>>>>
>>>> ... this #include wants to be conditional upon CONFIG_IOREQ_SERVER
>>>> being
>>>> defined. (I'm actually surprised that there's no respective #ifdef in
>>>> that header, meaning types defined there are available even when the
>>>> functionality was turned off.)
>>>
>>> The two functions in question are actually guarded by an #ifdef
>>> CONFIG_IOREQ_SERVER
>>> in arch/arm/include/asm/ioreq.h (in the #else branch some stubs are
>>> defined)
>>
>> Well, I don't see how an #ifdef there helps with the aspect mentioned
>> earlier (new arch-es needing to needlessly provide such a header as 
>> long
>> as the #include here is unconditional).
> 
> As far as I can tell, including the asm header in the arm implementation 
> file does not imply
> that new archs will need such an header. Of course, if the solution 
> proposed in the patch is
> chosen then I agree with you.

Hmm, maybe then I misunderstood your earlier reply: I thought you were
arguing against the change I had suggested.

Jan
Nicola Vetrini Sept. 1, 2023, 9:52 a.m. UTC | #7
On 01/09/2023 11:44, Jan Beulich wrote:
> On 01.09.2023 11:13, Nicola Vetrini wrote:
>> On 01/09/2023 09:36, Jan Beulich wrote:
>>> On 01.09.2023 09:13, Nicola Vetrini wrote:
>>>> On 28/08/2023 09:59, Jan Beulich wrote:
>>>>> On 25.08.2023 17:02, Nicola Vetrini wrote:
>>>>>> The common header file for ioreq should include the arch-specific
>>>>>> one.
>>>>> 
>>>>> To be honest I'm not convinced of "should" here. There are two
>>>>> aspects
>>>>> to consider: On one hand it is good practice to do what you say. 
>>>>> Otoh
>>>>> it
>>>>> introduces a needless dependency, and it'll require new arch-es
>>>>> (RISC-V,
>>>>> PPC at present) to introduce another dummy header just to satisfy 
>>>>> the
>>>>> xen/ioreq.h use in common/memory.c. Therefore, _if_ we want to go
>>>>> this
>>>>> route, besides a wording change here I think ...
>>>>> 
>>>> 
>>>> Since including both <xen/ioreq.h> and the arch-specific one was
>>>> requested to be changed
>>>> in previous series, I was trying to apply the same pattern here.
>>>> If you prefer not to change the current layout then the fix is even
>>>> simpler: I'll just
>>>> include <asm/ioreq.h> in xen/arch/arm/ioreq.c, which is where it's
>>>> missing.
>>>> 
>>>>>> --- a/xen/include/xen/ioreq.h
>>>>>> +++ b/xen/include/xen/ioreq.h
>>>>>> @@ -20,6 +20,7 @@
>>>>>>  #define __XEN_IOREQ_H__
>>>>>> 
>>>>>>  #include <xen/sched.h>
>>>>>> +#include <asm/ioreq.h>
>>>>> 
>>>>> ... this #include wants to be conditional upon CONFIG_IOREQ_SERVER
>>>>> being
>>>>> defined. (I'm actually surprised that there's no respective #ifdef 
>>>>> in
>>>>> that header, meaning types defined there are available even when 
>>>>> the
>>>>> functionality was turned off.)
>>>> 
>>>> The two functions in question are actually guarded by an #ifdef
>>>> CONFIG_IOREQ_SERVER
>>>> in arch/arm/include/asm/ioreq.h (in the #else branch some stubs are
>>>> defined)
>>> 
>>> Well, I don't see how an #ifdef there helps with the aspect mentioned
>>> earlier (new arch-es needing to needlessly provide such a header as
>>> long
>>> as the #include here is unconditional).
>> 
>> As far as I can tell, including the asm header in the arm 
>> implementation
>> file does not imply
>> that new archs will need such an header. Of course, if the solution
>> proposed in the patch is
>> chosen then I agree with you.
> 
> Hmm, maybe then I misunderstood your earlier reply: I thought you were
> arguing against the change I had suggested.
> 

I'm fine with either way, but since the one proposed in the patch may 
result in an inconvenience
for other architectures, then perhaps the other is better.
diff mbox series

Patch

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 96c740d5636c..13ae1fed5718 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -14,7 +14,6 @@ 
 #include <xen/sort.h>
 #include <asm/cpuerrata.h>
 #include <asm/current.h>
-#include <asm/ioreq.h>
 #include <asm/mmio.h>
 #include <asm/traps.h>

diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 7cb717f7a2a4..bde1a1f4eaa1 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -28,7 +28,6 @@ 
 #include <xen/trace.h>

 #include <asm/guest_atomics.h>
-#include <asm/ioreq.h>

 #include <public/hvm/ioreq.h>
 #include <public/hvm/params.h>
diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
index a26614d331e3..d85477c665e9 100644
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -20,6 +20,7 @@ 
 #define __XEN_IOREQ_H__

 #include <xen/sched.h>
+#include <asm/ioreq.h>

 #include <public/hvm/dm_op.h>