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
