| Message ID | 2c9eb4fc175a1bdd21293f2e2611d8e21991636d.1691016993.git.sanastasio@raptorengineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ppc: Enable full Xen build | expand |
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@raptorengineering.com> 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 ... > --- 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? > --- 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. 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. Jan
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@raptorengineering.com> > > 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
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
diff --git a/xen/common/memory.c b/xen/common/memory.c index c206fa4808..1b185b00e4 100644 --- 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> diff --git a/xen/common/symbols.c b/xen/common/symbols.c index 9377f41424..691e617925 100644 --- a/xen/common/symbols.c +++ b/xen/common/symbols.c @@ -19,6 +19,7 @@ #include <xen/virtual_region.h> #include <public/platform.h> #include <xen/guest_access.h> +#include <xen/errno.h> #ifdef SYMBOLS_ORIGIN extern const unsigned int symbols_offsets[]; diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index c603c39bb9..349b31cb4c 100644 --- 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> #define MAX_POOL_NAME_LEN 16 diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index d35af34841..767127b440 100644 --- 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> #include <asm/domain.h> #include <asm/numa.h> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 405db59971..ef817efec9 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -24,6 +24,7 @@ #include <xen/page-defs.h> #include <xen/pci.h> #include <xen/spinlock.h> +#include <xen/errno.h> #include <public/domctl.h> #include <public/hvm/ioreq.h> #include <asm/device.h> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 854f3e32c0..6a96534a45 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -21,6 +21,7 @@ #include <xen/smp.h> #include <xen/perfc.h> #include <asm/atomic.h> +#include <asm/current.h> #include <xen/vpci.h> #include <xen/wait.h> #include <public/xen.h>
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@raptorengineering.com> --- xen/common/memory.c | 1 + xen/common/symbols.c | 1 + xen/common/xmalloc_tlsf.c | 1 + xen/include/xen/domain.h | 1 + xen/include/xen/iommu.h | 1 + xen/include/xen/sched.h | 1 + 6 files changed, 6 insertions(+)