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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2023 18:49, Shawn Anastasio wrote:
> On 8/7/23 10:39 AM, Jan Beulich wrote:
>> On 03.08.2023 01:02, Shawn Anastasio wrote:
>>> --- 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.

Okay, that's unavoidable then.

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

Hmm, PAGE_ALIGN() really shouldn't be a per-arch #define.

>>> --- 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>?

Yes (but see below).

> I've tested this and it works fine, as expected.

Good.

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

This is always the preferred solution, when available.

Jan