| 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 |
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 >
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
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)
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
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.
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
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 --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>
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