| Message ID | 150525bb-1c48-c331-3212-eff18bc4c13d@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: build adjustments | expand |
On Wed, 15 Jul 2020, Jan Beulich wrote: > asm/domain.h is a dependency of xen/sched.h, and hence should not itself > include xen/sched.h. Nor should any of the other #include-s used by it. > While at it, also drop two other #include-s that aren't needed by this > particular header. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -2,7 +2,7 @@ > #define __ASM_DOMAIN_H__ > > #include <xen/cache.h> > -#include <xen/sched.h> > +#include <xen/timer.h> > #include <asm/page.h> > #include <asm/p2m.h> > #include <asm/vfp.h> > @@ -11,8 +11,6 @@ > #include <asm/vgic.h> > #include <asm/vpl011.h> > #include <public/hvm/params.h> > -#include <xen/serial.h> > -#include <xen/rbtree.h> > > struct hvm_domain > { > --- a/xen/include/asm-arm/vfp.h > +++ b/xen/include/asm-arm/vfp.h > @@ -1,7 +1,7 @@ > #ifndef _ASM_VFP_H > #define _ASM_VFP_H > > -#include <xen/sched.h> > +struct vcpu; > > #if defined(CONFIG_ARM_32) > # include <asm/arm32/vfp.h> >
On 15/07/2020 11:39, Jan Beulich wrote: > asm/domain.h is a dependency of xen/sched.h, and hence should not itself > include xen/sched.h. Nor should any of the other #include-s used by it. > While at it, also drop two other #include-s that aren't needed by this > particular header. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -2,7 +2,7 @@ > #define __ASM_DOMAIN_H__ > > #include <xen/cache.h> > -#include <xen/sched.h> > +#include <xen/timer.h> > #include <asm/page.h> > #include <asm/p2m.h> > #include <asm/vfp.h> > @@ -11,8 +11,6 @@ > #include <asm/vgic.h> > #include <asm/vpl011.h> > #include <public/hvm/params.h> > -#include <xen/serial.h> While we don't need the rbtree.h, we technically need serial.h for using vuart_info. I would rather prefer if headers are not implicitly included whenever it is possible. Cheers,
On 17.07.2020 16:44, Julien Grall wrote: > On 15/07/2020 11:39, Jan Beulich wrote: >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -2,7 +2,7 @@ >> #define __ASM_DOMAIN_H__ >> >> #include <xen/cache.h> >> -#include <xen/sched.h> >> +#include <xen/timer.h> >> #include <asm/page.h> >> #include <asm/p2m.h> >> #include <asm/vfp.h> >> @@ -11,8 +11,6 @@ >> #include <asm/vgic.h> >> #include <asm/vpl011.h> >> #include <public/hvm/params.h> >> -#include <xen/serial.h> > > While we don't need the rbtree.h, we technically need serial.h for using > vuart_info. The only reference to it is const struct vuart_info *info; which doesn't require a definition nor even a forward declaration of struct vuart_info. It should just be source files instantiating a struct or de-referencing pointers to one that actually need to see the full declaration. The only source file doing so (vuart.c) already includes xen/serial.h. (In fact, it being just a single source file doing so, the struct definition could [and imo should] be moved there. The type can be entirely opaque to the rest of the code base, as - over time - we've been doing for other structs.) > I would rather prefer if headers are not implicitly included whenever it > is possible. I agree with this principle, where it applies. Jan
On 20/07/2020 09:17, Jan Beulich wrote: > On 17.07.2020 16:44, Julien Grall wrote: >> On 15/07/2020 11:39, Jan Beulich wrote: >>> --- a/xen/include/asm-arm/domain.h >>> +++ b/xen/include/asm-arm/domain.h >>> @@ -2,7 +2,7 @@ >>> #define __ASM_DOMAIN_H__ >>> >>> #include <xen/cache.h> >>> -#include <xen/sched.h> >>> +#include <xen/timer.h> >>> #include <asm/page.h> >>> #include <asm/p2m.h> >>> #include <asm/vfp.h> >>> @@ -11,8 +11,6 @@ >>> #include <asm/vgic.h> >>> #include <asm/vpl011.h> >>> #include <public/hvm/params.h> >>> -#include <xen/serial.h> >> >> While we don't need the rbtree.h, we technically need serial.h for using >> vuart_info. > > The only reference to it is > > const struct vuart_info *info; > > which doesn't require a definition nor even a forward declaration > of struct vuart_info. It should just be source files instantiating > a struct or de-referencing pointers to one that actually need to > see the full declaration. Ah yes. I got confused because you introduced a forward declaration of struct vcpu. But this is because you need it to declare the function prototype. > The only source file doing so (vuart.c) > already includes xen/serial.h. (In fact, it being just a single > source file doing so, the struct definition could [and imo should] > be moved there. The type can be entirely opaque to the rest of the > code base, as - over time - we've been doing for other structs.) There are definitely more use of vuart_info within the code base. All the UART on Arm will fill up the structure (see drivers/char/pl011.c) for instance. So the definition is in the correct place. Cheers,
On 20.07.2020 11:09, Julien Grall wrote: > > > On 20/07/2020 09:17, Jan Beulich wrote: >> On 17.07.2020 16:44, Julien Grall wrote: >>> On 15/07/2020 11:39, Jan Beulich wrote: >>>> --- a/xen/include/asm-arm/domain.h >>>> +++ b/xen/include/asm-arm/domain.h >>>> @@ -2,7 +2,7 @@ >>>> #define __ASM_DOMAIN_H__ >>>> >>>> #include <xen/cache.h> >>>> -#include <xen/sched.h> >>>> +#include <xen/timer.h> >>>> #include <asm/page.h> >>>> #include <asm/p2m.h> >>>> #include <asm/vfp.h> >>>> @@ -11,8 +11,6 @@ >>>> #include <asm/vgic.h> >>>> #include <asm/vpl011.h> >>>> #include <public/hvm/params.h> >>>> -#include <xen/serial.h> >>> >>> While we don't need the rbtree.h, we technically need serial.h for using >>> vuart_info. >> >> The only reference to it is >> >> const struct vuart_info *info; >> >> which doesn't require a definition nor even a forward declaration >> of struct vuart_info. It should just be source files instantiating >> a struct or de-referencing pointers to one that actually need to >> see the full declaration. > > Ah yes. I got confused because you introduced a forward declaration of > struct vcpu. But this is because you need it to declare the function > prototype. As a result - are you happy for the change to go in with Stefano's ack then? >> The only source file doing so (vuart.c) >> already includes xen/serial.h. (In fact, it being just a single >> source file doing so, the struct definition could [and imo should] >> be moved there. The type can be entirely opaque to the rest of the >> code base, as - over time - we've been doing for other structs.) > > There are definitely more use of vuart_info within the code base. All > the UART on Arm will fill up the structure (see drivers/char/pl011.c) > for instance. > > So the definition is in the correct place. Hmm, I will admit I judged from the uses of ->arch.vuart.info alone. Jan
Hi Jan, On 20/07/2020 12:28, Jan Beulich wrote: > On 20.07.2020 11:09, Julien Grall wrote: >> >> >> On 20/07/2020 09:17, Jan Beulich wrote: >>> On 17.07.2020 16:44, Julien Grall wrote: >>>> On 15/07/2020 11:39, Jan Beulich wrote: >>>>> --- a/xen/include/asm-arm/domain.h >>>>> +++ b/xen/include/asm-arm/domain.h >>>>> @@ -2,7 +2,7 @@ >>>>> #define __ASM_DOMAIN_H__ >>>>> >>>>> #include <xen/cache.h> >>>>> -#include <xen/sched.h> >>>>> +#include <xen/timer.h> >>>>> #include <asm/page.h> >>>>> #include <asm/p2m.h> >>>>> #include <asm/vfp.h> >>>>> @@ -11,8 +11,6 @@ >>>>> #include <asm/vgic.h> >>>>> #include <asm/vpl011.h> >>>>> #include <public/hvm/params.h> >>>>> -#include <xen/serial.h> >>>> >>>> While we don't need the rbtree.h, we technically need serial.h for using >>>> vuart_info. >>> >>> The only reference to it is >>> >>> const struct vuart_info *info; >>> >>> which doesn't require a definition nor even a forward declaration >>> of struct vuart_info. It should just be source files instantiating >>> a struct or de-referencing pointers to one that actually need to >>> see the full declaration. >> >> Ah yes. I got confused because you introduced a forward declaration of >> struct vcpu. But this is because you need it to declare the function >> prototype. > > As a result - are you happy for the change to go in with Stefano's > ack then? Yes. Sorry I should have been clearer in my previous answer. Cheers,
On 20/07/2020 16:15, Julien Grall wrote: > Hi Jan, > > On 20/07/2020 12:28, Jan Beulich wrote: >> On 20.07.2020 11:09, Julien Grall wrote: >>> >>> >>> On 20/07/2020 09:17, Jan Beulich wrote: >>>> On 17.07.2020 16:44, Julien Grall wrote: >>>>> On 15/07/2020 11:39, Jan Beulich wrote: >>>>>> --- a/xen/include/asm-arm/domain.h >>>>>> +++ b/xen/include/asm-arm/domain.h >>>>>> @@ -2,7 +2,7 @@ >>>>>> #define __ASM_DOMAIN_H__ >>>>>> #include <xen/cache.h> >>>>>> -#include <xen/sched.h> >>>>>> +#include <xen/timer.h> >>>>>> #include <asm/page.h> >>>>>> #include <asm/p2m.h> >>>>>> #include <asm/vfp.h> >>>>>> @@ -11,8 +11,6 @@ >>>>>> #include <asm/vgic.h> >>>>>> #include <asm/vpl011.h> >>>>>> #include <public/hvm/params.h> >>>>>> -#include <xen/serial.h> >>>>> >>>>> While we don't need the rbtree.h, we technically need serial.h for >>>>> using >>>>> vuart_info. >>>> >>>> The only reference to it is >>>> >>>> const struct vuart_info *info; >>>> >>>> which doesn't require a definition nor even a forward declaration >>>> of struct vuart_info. It should just be source files instantiating >>>> a struct or de-referencing pointers to one that actually need to >>>> see the full declaration. >>> >>> Ah yes. I got confused because you introduced a forward declaration of >>> struct vcpu. But this is because you need it to declare the function >>> prototype. >> >> As a result - are you happy for the change to go in with Stefano's >> ack then? > > Yes. Sorry I should have been clearer in my previous answer. I have committed now. Cheers,
--- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -2,7 +2,7 @@ #define __ASM_DOMAIN_H__ #include <xen/cache.h> -#include <xen/sched.h> +#include <xen/timer.h> #include <asm/page.h> #include <asm/p2m.h> #include <asm/vfp.h> @@ -11,8 +11,6 @@ #include <asm/vgic.h> #include <asm/vpl011.h> #include <public/hvm/params.h> -#include <xen/serial.h> -#include <xen/rbtree.h> struct hvm_domain { --- a/xen/include/asm-arm/vfp.h +++ b/xen/include/asm-arm/vfp.h @@ -1,7 +1,7 @@ #ifndef _ASM_VFP_H #define _ASM_VFP_H -#include <xen/sched.h> +struct vcpu; #if defined(CONFIG_ARM_32) # include <asm/arm32/vfp.h>
asm/domain.h is a dependency of xen/sched.h, and hence should not itself include xen/sched.h. Nor should any of the other #include-s used by it. While at it, also drop two other #include-s that aren't needed by this particular header. Signed-off-by: Jan Beulich <jbeulich@suse.com>