Details

Reviewers

royger
mhorne
julien_xen.org

Commits

rGc89f1f12b07b: xen/xen-os: move inclusion of machine/xen-os.h later

Summary

Several of x86 enable/disable functions depend upon the xen*domain()
functions. As such the xen*domain() functions need to be declared
before machine/xen-os.h.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ehem_freebsd_m5p.com created this revision.Apr 17 2021, 5:42 PM

Herald added a subscriber: imp. · View Herald TranscriptApr 17 2021, 5:42 PM

ehem_freebsd_m5p.com requested review of this revision.Apr 17 2021, 5:42 PM

Harbormaster completed remote builds in B38653: Diff 87625.Apr 17 2021, 5:42 PM

ehem_freebsd_m5p.com added child revisions: D29405: xen/netfront: introduce xen_pv_nics_disabled(), D29406: xen/control: introduce xen_pv_shutdown_handler(), D29403: xen: introduce xen_pv_disks_disabled(), D29402: xen: introduce xen_support_evtchn_bind().Apr 17 2021, 5:45 PM

I had been thinking @royger was going to be checking for x86, but since latest needed checking, I tried and found this was an issue. Seems rather a number of kernel headers fail to include headers they depend upon as well.

This came about due to a WiP which needed a bit of checking on x86. Then found there hadn't been much checking to ensure things didn't get broken on x86.

This though brings up an issue: What is the FreeBSD stance on headers which require other headers?

For instance <sys/bus.h> requires multiple other headers to be #include'd, otherwise you get build errors. Notably uint64_t is needed, yet <sys/bus.h> does not #include any definition itself.

In D29811#668814, @ehem_freebsd_m5p.com wrote:

This came about due to a WiP which needed a bit of checking on x86. Then found there hadn't been much checking to ensure things didn't get broken on x86.

This though brings up an issue: What is the FreeBSD stance on headers which require other headers?

For instance <sys/bus.h> requires multiple other headers to be #include'd, otherwise you get build errors. Notably uint64_t is needed, yet <sys/bus.h> does not #include any definition itself.

Generally, users of the headers are expected to include the right prerequisite headers. sys/types.h is rarely included which is how uint64_t comes in. This is done to avoid quadratic expansion of the headers one needs as recursion happens.

However, there's several exceptions to this rule when the definitions are in _foo.h files that are shared between foo.h and other headers.

ehem_freebsd_m5p.com mentioned this in D29406: xen/control: introduce xen_pv_shutdown_handler().Apr 21 2021, 2:16 AM

D29811 is a notable concern. I discovered it had been quite some time since I had last tried building x86 and upon trying I discovered things had gotten broken. D29811 is marked as the parent for several since I believe those need D29811 first.

Fix a lurking gotcha. sys/x86/xen/xen_intr.c had been directly including
machine/xen/xen-os.h, which it shouldn't have been doing. This is closely
related so I think it is okay to include with this commit.

Harbormaster completed remote builds in B38786: Diff 87926.Apr 22 2021, 6:17 AM

D29811 could use some attention. It is marked as parent for several since those break on x86 without D29811 (wrong order of function declaration, several call xen_*_domain()).