Re: [PATCH 1/9] xen/common: Add missing #includes treewide

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 8/7/23 10:39 AM, Jan Beulich wrote:
> On 03.08.2023 01:02, Shawn Anastasio wrote:
>> A few files treewide depend on defininitions in headers that they
>> don't include. This works when arch headers end up including the
>> required headers by chance, but broke on ppc64 with only minimal/stub
>> arch headers.
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> 
> I'm okay with the changes in principle, but I'd like to ask a question
> nevertheless, perhaps also for other REST maintainers (whom you should
> have Cc-ed, btw) to chime in.
> 
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -28,6 +28,7 @@
>>  #include <asm/current.h>
>>  #include <asm/hardirq.h>
>>  #include <asm/p2m.h>
>> +#include <asm/page.h>
>>  #include <public/memory.h>
>>  #include <xsm/xsm.h>
> 
> I realize there are several asm/*.h being included here already. Yet
> generally I think common .c files would better not include any of
> them directly; only xen/*.h ones should (and even there one might see
> possible restrictions on what's "legitimate"). Do you recall what it
> was that's needed from asm/page.h here ...

The references to invalidate_icache (memory.c:310), clear_page
(memory.c:1867), and copy_page (memory.c:1876) all need asm/page.h to be
included somehow. I'm not sure which file ends up including asm/page.h
for build to work on x86/arm, but with my minimal PPC headers it wasn't
getting incidentally included and build was failing.

> 
>> --- a/xen/common/xmalloc_tlsf.c
>> +++ b/xen/common/xmalloc_tlsf.c
>> @@ -27,6 +27,7 @@
>>  #include <xen/mm.h>
>>  #include <xen/pfn.h>
>>  #include <asm/time.h>
>> +#include <asm/page.h>
> 
> ... and here?

Here it's the PAGE_ALIGN used at xmalloc_tlsf.c:549

> 
>> --- a/xen/include/xen/domain.h
>> +++ b/xen/include/xen/domain.h
>> @@ -4,6 +4,7 @@
>>  
>>  #include <xen/types.h>
>>  
>> +#include <public/domctl.h>
>>  #include <public/xen.h>
> 
> While following our sorting guidelines, this still looks a little odd.
> We typically would include public/xen.h first, but then almost all other
> public headers include it anyway. So I'm inclined to suggest to replace
> (rather than amend) the existing #include here.

To be clear, you're suggesting replacing the include of <public/xen.h>
to <public/domctl.h>? I've tested this and it works fine, as expected.

> 
> Then again I wonder why this include is needed. xen/domain.h is
> effectively included everywhere, yet I would have hoped public/domctl.h
> isn't.

domctl.h is required because of the reference to `struct
xen_domctl_createdomain` on domain.h:84. Alternatively, we could get
away with a forward declaration of the struct.

> Jan

Thanks,
Shawn