| Message ID | 20230808080010.3858575-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fine granular configuration | expand |
Hi Luca, On 08/08/2023 09:00, Luca Fancellu wrote: > Add asm/domain.h that is defining the type 'enum domain_type', it > is needed on arm64 build where this type is used for a member of > the structure kernel_info. I read "needed" as in it Xen build is broken. But AFAIK, this is more a latent issue if someone else want to include the header. Is that correct? If so, how about: The 'enum domain_type' is defined by 'asm/domain.h' which is not included (directly or indirectly) by 'asm/kernel.h'. This currently doesn't break the compilation because asm/domain.h will included by the user of 'kernel.h'. But it would be better to avoid relying on it. So add the include in 'asm/domain.h'. > Fixes: 66e994a5e74f ("xen: arm64: add guest type to domain field.") While we aim to have header self-contained, this has never been a guarantee in Xen. So I would argue this is not a fix in the sense it someone would want to ingest it in there tree. Cheers, > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > xen/arch/arm/include/asm/kernel.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h > index 4617cdc83bac..0a23e86c2d37 100644 > --- a/xen/arch/arm/include/asm/kernel.h > +++ b/xen/arch/arm/include/asm/kernel.h > @@ -7,6 +7,7 @@ > #define __ARCH_ARM_KERNEL_H__ > > #include <xen/device_tree.h> > +#include <asm/domain.h> > #include <asm/setup.h> > > /*
> On 11 Aug 2023, at 13:56, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 08/08/2023 09:00, Luca Fancellu wrote: >> Add asm/domain.h that is defining the type 'enum domain_type', it >> is needed on arm64 build where this type is used for a member of >> the structure kernel_info. > > I read "needed" as in it Xen build is broken. But AFAIK, this is more a latent issue if someone else want to include the header. Is that correct? Yes correct > > If so, how about: > > The 'enum domain_type' is defined by 'asm/domain.h' which is not included (directly or indirectly) by 'asm/kernel.h'. > > This currently doesn't break the compilation because asm/domain.h will included by the user of 'kernel.h'. But it would be better to avoid relying on it. So add the include in 'asm/domain.h'. Yeah much better, should I push a v2? > >> Fixes: 66e994a5e74f ("xen: arm64: add guest type to domain field.") > > While we aim to have header self-contained, this has never been a guarantee in Xen. So I would argue this is not a fix in the sense it someone would want to ingest it in there tree. Ok I see, I thought it could be linked to the issue about sorting headers that led to build breakage, but I’ve not investigated further so I would be ok to drop the Fixes: > > Cheers, > >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> xen/arch/arm/include/asm/kernel.h | 1 + >> 1 file changed, 1 insertion(+) >> diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h >> index 4617cdc83bac..0a23e86c2d37 100644 >> --- a/xen/arch/arm/include/asm/kernel.h >> +++ b/xen/arch/arm/include/asm/kernel.h >> @@ -7,6 +7,7 @@ >> #define __ARCH_ARM_KERNEL_H__ >> #include <xen/device_tree.h> >> +#include <asm/domain.h> >> #include <asm/setup.h> >> /* > > -- > Julien Grall
Hi Luca, On 11/08/2023 14:40, Luca Fancellu wrote: > > >> On 11 Aug 2023, at 13:56, Julien Grall <julien@xen.org> wrote: >> >> Hi Luca, >> >> On 08/08/2023 09:00, Luca Fancellu wrote: >>> Add asm/domain.h that is defining the type 'enum domain_type', it >>> is needed on arm64 build where this type is used for a member of >>> the structure kernel_info. >> >> I read "needed" as in it Xen build is broken. But AFAIK, this is more a latent issue if someone else want to include the header. Is that correct? > > Yes correct > >> >> If so, how about: >> >> The 'enum domain_type' is defined by 'asm/domain.h' which is not included (directly or indirectly) by 'asm/kernel.h'. >> >> This currently doesn't break the compilation because asm/domain.h will included by the user of 'kernel.h'. But it would be better to avoid relying on it. So add the include in 'asm/domain.h'. > > Yeah much better, should I push a v2? No. I can deal with it on commit. > >> >>> Fixes: 66e994a5e74f ("xen: arm64: add guest type to domain field.") >> >> While we aim to have header self-contained, this has never been a guarantee in Xen. So I would argue this is not a fix in the sense it someone would want to ingest it in there tree. > > Ok I see, I thought it could be linked to the issue about sorting headers that led to build breakage, but I’ve I am probably missing something here. Which issue are you referring to? Is it a follow-up patch that will sort headers? > not investigated further so I would be ok to drop the Fixes: Cheers,
> On 11 Aug 2023, at 15:13, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 11/08/2023 14:40, Luca Fancellu wrote: >>> On 11 Aug 2023, at 13:56, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Luca, >>> >>> On 08/08/2023 09:00, Luca Fancellu wrote: >>>> Add asm/domain.h that is defining the type 'enum domain_type', it >>>> is needed on arm64 build where this type is used for a member of >>>> the structure kernel_info. >>> >>> I read "needed" as in it Xen build is broken. But AFAIK, this is more a latent issue if someone else want to include the header. Is that correct? >> Yes correct >>> >>> If so, how about: >>> >>> The 'enum domain_type' is defined by 'asm/domain.h' which is not included (directly or indirectly) by 'asm/kernel.h'. >>> >>> This currently doesn't break the compilation because asm/domain.h will included by the user of 'kernel.h'. But it would be better to avoid relying on it. So add the include in 'asm/domain.h'. >> Yeah much better, should I push a v2? > > No. I can deal with it on commit. Ok thank you for doing that > >>> >>>> Fixes: 66e994a5e74f ("xen: arm64: add guest type to domain field.") >>> >>> While we aim to have header self-contained, this has never been a guarantee in Xen. So I would argue this is not a fix in the sense it someone would want to ingest it in there tree. >> Ok I see, I thought it could be linked to the issue about sorting headers that led to build breakage, but I’ve > > I am probably missing something here. Which issue are you referring to? Is it a follow-up patch that will sort headers? It’s an issue I’ve faced when trying to sort automatically the include using clang-format, I’ve seen issues building domain_build.c after sorting the headers in the way we expect from coding style, I thought was related to some headers not being self-contained. > >> not investigated further so I would be ok to drop the Fixes: > > Cheers, > > -- > Julien Grall
Hi, On 11/08/2023 15:22, Luca Fancellu wrote: >> On 11 Aug 2023, at 15:13, Julien Grall <julien@xen.org> wrote: >> On 11/08/2023 14:40, Luca Fancellu wrote: >>>> On 11 Aug 2023, at 13:56, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Luca, >>>> >>>> On 08/08/2023 09:00, Luca Fancellu wrote: >>>>> Add asm/domain.h that is defining the type 'enum domain_type', it >>>>> is needed on arm64 build where this type is used for a member of >>>>> the structure kernel_info. >>>> >>>> I read "needed" as in it Xen build is broken. But AFAIK, this is more a latent issue if someone else want to include the header. Is that correct? >>> Yes correct >>>> >>>> If so, how about: >>>> >>>> The 'enum domain_type' is defined by 'asm/domain.h' which is not included (directly or indirectly) by 'asm/kernel.h'. >>>> >>>> This currently doesn't break the compilation because asm/domain.h will included by the user of 'kernel.h'. But it would be better to avoid relying on it. So add the include in 'asm/domain.h'. >>> Yeah much better, should I push a v2? >> >> No. I can deal with it on commit. > > Ok thank you for doing that > >> >>>> >>>>> Fixes: 66e994a5e74f ("xen: arm64: add guest type to domain field.") >>>> >>>> While we aim to have header self-contained, this has never been a guarantee in Xen. So I would argue this is not a fix in the sense it someone would want to ingest it in there tree. >>> Ok I see, I thought it could be linked to the issue about sorting headers that led to build breakage, but I’ve >> >> I am probably missing something here. Which issue are you referring to? Is it a follow-up patch that will sort headers? > > It’s an issue I’ve faced when trying to sort automatically the include using clang-format, I’ve seen issues building domain_build.c after sorting the headers in the way we expect from coding style, I thought was related to some headers not being self-contained. Ah I understand now. Usually, we would batch the re-ordering and the additional include in the same patch because they are tightly coupled together and it is easier to confirm it is necessary. Anyway, this one is easy, so I am happy to commit this one in advance. Cheers,
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index 4617cdc83bac..0a23e86c2d37 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -7,6 +7,7 @@ #define __ARCH_ARM_KERNEL_H__ #include <xen/device_tree.h> +#include <asm/domain.h> #include <asm/setup.h> /*
Add asm/domain.h that is defining the type 'enum domain_type', it is needed on arm64 build where this type is used for a member of the structure kernel_info. Fixes: 66e994a5e74f ("xen: arm64: add guest type to domain field.") Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/include/asm/kernel.h | 1 + 1 file changed, 1 insertion(+)