Re: [PATCH 2/5] xen/arm: Add asm/domain.h include to kernel.h

[Julien Grall <julien@xxxxxxx>]

Hi,

On 11/08/2023 15:22, Luca Fancellu wrote:
> > On 11 Aug 2023, at 15:13, Julien Grall <julien@xxxxxxx> wrote:
> > On 11/08/2023 14:40, Luca Fancellu wrote:
> > > > On 11 Aug 2023, at 13:56, Julien Grall <julien@xxxxxxx> 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,

--
Julien Grall