xen: move common variables and code off of sys/x86/xen/hvm.c

[ehem_freebsd_m5p.com]

Summary

The xen_domain_type and HYPERVISOR_shared_info variables are shared by
all Xen architectures, so they should be in common rather than
reimplemented by each architecture.

Likewise with the setup of HYPERVISOR_shared_info. The implementation of
xen_handle_shared_info() was inspired by one from elsewhere which also
meets the need for Xen/ARM.

[ehem_freebsd_m5p.com]

Another chunk from my FreeBSD/ARM/Xen efforts. As the difference is entirely my work (though working with files which others own copyright on), I can push it without okay from anyone else.

One concern is whether I got the copyright notice right. info.c originates from sys/x86/xen/hvm.c which was updated at Citrix's behest (@royger) in 2018, but the notice there didn't appear current.

Another concern is the file was named sys/xen/info.c due to "struct hypervisor_info". Does someone desire hyp_info.c?

I'm tempted to modify xen-os.h to #include sys/param.h and machine/atomic.h, since xen-os.h needs constructs from those files. info.c doesn't need either of those headers itself, it strictly needs them due to xen-os.h not #including them itself.

[ehem_freebsd_m5p.com]

Switch to detecting x86, rather than ARM. Work is under way to get Xen
operational on RISC-V, I suspect they will also use something akin to
PVH. As such I suspect RISC-V would also want the const, so only
disable the const for x86.

[mhorne]

sys/xen/info.c

89 ↗

(On Diff #84856)

Why is the const required, or useful, at all?

[ehem_freebsd_m5p.com]

sys/xen/info.c

89 ↗

(On Diff #84856)

Since on ARM (and likely RISC-V) it is constant. Compiler gets to do more optimizations around access to the data. If at some future point a bug caused memory corruption, this structure would be safe; if a future security vulnerability evolved which involved this data, making it constant could mitigate it. Mostly it is constant, so mark it so.

[mhorne]

sys/xen/info.c

89 ↗

(On Diff #84856)

Sure, I take your point that optimization could save the indirection, however, a smart compiler could deduce this regardless of the const qualifier. In my opinion, it is best to limit ifdef checks for CPU architecture in machine-independent code whenever it is non-trivial to do so.

In either case, my main feeling is that it does not make sense to make specific provisions for !x86 support until that support is finalized/reviewable somewhere. It can always be added in a later commit.

Otherwise, this patch seems like a useful first step towards the arm64 support.

[ehem_freebsd_m5p.com]

sys/xen/info.c

89 ↗

(On Diff #84856)

Sure, I take your point that optimization could save the indirection, however, a smart compiler could deduce this regardless of the const qualifier. In my opinion, it is best to limit ifdef checks for CPU architecture in machine-independent code whenever it is non-trivial to do so.

Can also be valuable during development. If I've got a data structure which I need to preserve, if I look up a function declaration and some incoming data is marked const I know I can pass it in and not need to keep a duplicate copy. I hold the view far too little is declared const.

In either case, my main feeling is that it does not make sense to make specific provisions for !x86 support until that support is finalized/reviewable somewhere. It can always be added in a later commit.

It is out there, but isn't ready for review. I'm waiting for an official okay on FreeBSD's license. Alas Xen is in release mode, so the original author is busy; I believe the author was okay with FreeBSD's license, just I want formal approval. So far a FreeBSD aarch64 DomU has been observed starting its boot process (guess how D28988 was observed in real life). vCPU and memory probing work, but at some point near/during xenpv0 probing things go crunch.

You want to trying a build?

[royger]

sys/xen/info.c

90 ↗

(On Diff #84856)

I would be fine if you dropped the legacy_info (and related helpers), and instead made the helpers in xen-os.h just use hvm_get_parameter (like the default hypervisor_info implementation). Then we could drop the indirection.

There's no reason to carry the legacy stuff any longer.

[ehem_freebsd_m5p.com]

Renaming the file to "common.c". Grabbing a few more variables which
x86 had in hvm.c and placing them in common.c (ARM had them in a different
file, this lets things converge).

[ehem_freebsd_m5p.com]

Thing is my goal is to get Xen/ARM working. If removing PVHv1 support made that noticeably easier, I would be for that. Yet here it makes minimal difference to simply leave it alone and removing it would require me to check I hadn't broken anything.

[ehem_freebsd_m5p.com]

sys/conf/files

5140

And naturally when I look here instead of my local repository, I realize alphabetic sorting and this is out of order. Ugh. This could be fixed on commit, though I suspect a hunk from D29875 will be merged in first.

[ehem_freebsd_m5p.com]

Update creating xen_handle_shared_info(). Idea is to share between x86 and
other architectures. This uses an approach similar to the one proposed for
ARM, but pieces have been inspired by the x86 implementation.

[ehem_freebsd_m5p.com]

Switching to SI_ORDER_SECOND from SI_ORDER_FIRST. With hypervisors, SI_ORDER_FIRST should be reserved for probing, whereas this is merely very early setup.

[mhorne]

In D28982#673130, @ehem_freebsd_m5p.com wrote:

Thing is my goal is to get Xen/ARM working. If removing PVHv1 support made that noticeably easier, I would be for that. Yet here it makes minimal difference to simply leave it alone and removing it would require me to check I hadn't broken anything.

I would be willing to take a stab at removing the PVHv1 support, if that is helpful to you. I think doing that first would make this patch cleaner.

[ehem_freebsd_m5p.com]

Updating with the latest from my current successful build. The PVHv1 portion may end up removed due to discontinuing support for PVHv1, but the handling of the shared_info page should be here. Plus hvm_start_flags and xen_domain_type become common.

[ehem_freebsd_m5p.com]

Update due to removal of PVHv1, much of the commit is gone, but some
valuable portions remain. xen_handle_shared_info() is still there, the
architecture-independent variables are still here.

[ehem_freebsd_m5p.com]

Any word on D28982? Closing in on a month since it reached its present state and no review.

[mhorne]

sys/conf/files

5138

Is there a more appropriate name for the file? The contents seem generally specific to hvm. Maybe hvm_common.c?

sys/xen/common.c

98

This function will now be called twice; as part of the SI_SUB_HYPERVISOR sysinit and in xen_hvm_init(). Is this intentional?

[ehem_freebsd_m5p.com]

sys/conf/files

5138

The variable hvm_start_flags is specific to HVM mode and xen_handle_shared_info() only does something in HVM mode.

The variables xen_domain_type and HYPERVISOR_shared_info are very general though. Few files of the Xen implementation don't access xen_domain_type (generally via xen_domain()) so that is very common.

Perhaps hvm_common.c might be better.

sys/xen/common.c

98

I'm aware. This is less than optimal, but it felt like optimizing things would degrade the implementation. In particular this is likely to be required for all !x86, but as you've noticed it is redundant for x86. xen_hvm_init() needs to function even if called multiple times anyway, so removing the redundant call didn't seem worthwhile.

Otherwise the C_SYSINIT() is going to need to be dumped into another !x86 file somewhere and that may mean it is hard to find (right now the candidate which comes to mind is xen-dt.c). Though on second thought this might well be the way to go since the detection of Xen in xen-dt.c is what means this needs to be SI_ORDER_SECOND.

[ehem_freebsd_m5p.com]

sys/xen/common.c

98

Having spent some more time thinking, I'm finding my argument rather compelling. Mainly perhaps xen_dt_probe() really should be calling this.

[mhorne]

sys/xen/common.c

98

I agree, best if some other initialization function calls this, rather than the SYSINIT.

[ehem_freebsd_m5p.com]

Removing SYSINIT call of xen_handle_shared_info(). Convinced the other approach was the way to go.

[ehem_freebsd_m5p.com]

xen_handle_shared_info() is meant to work if called multiple times, but I'll concede and opt to have it called from elsewhere. D29875 has been adjusted to handle this.

While this did compile just fine, it hasn't been tested on hardware yet. I'm trying to decide when I'll have done enough to do a full test.

[ehem_freebsd_m5p.com]

sys/xen/common.c

98

Problem with doing the call during xen_dt_probe() is it ends up looking quite ugly. For aarch64 SI_SUB_HYPERVISOR/SI_ORDER_SECOND really looks like the best approach to me.

There is a decent argument for the SYSINIT being in xen-dt.c as that avoids a redundant call on x86, but this means the invocation ends up in an almost completely unrelated place strictly for this reason. I'm up for this, but I dislike the separation.

[mhorne]

sys/conf/files

5138

hvm_common.c

sys/xen/common.c

7

If you are moving code around, you need to retain the original copyright holders' names. It is dubious whether you can claim new copyright here, there is almost no new code.

32

FYI, there's not much reason to add these for new files that won't be merged back to stable/12. It is a relic of SVN.

34–35

The include of sys/types.h is not needed when you include sys/param.h, described in style(9).

36

You can drop this blank line.

81–85

So, why has this allocation changed? Before it was just a malloc(). Please make semantic changes as separate commits from code moves, when possible. I almost did not see it.

[ehem_freebsd_m5p.com]

sys/xen/common.c

7

See below.

81–85

While it might look at first glance like a move, it is not a move. This is a reimplementation more based on one done by @julien_xen.org, but adjusted to function on x86 (the original was part of the xen-dt.c file and ARM64-only). Most error messages come from @julien_xen.org's implementation, though I did retain the comment from xen_hvm_init_shared_info_page().

Also note xen_pv_domain() was replaced with !xen_hvm_domain() since on ARM64 this is always called during SI_SUB_HYPERVISOR/SI_ORDER_SECOND. As such it needs to cope with being called when Xen is absent (hmm, should modify the comment).

[ehem_freebsd_m5p.com]

sys/conf/files

5138

Thing is neither xen_domain_type nor HYPERVISOR_shared_info is strictly HVM. While the PV implementations are deprecated, those two have a longer history.

[ehem_freebsd_m5p.com]

Taking care of style issues, adjusting the one comment.

[ehem_freebsd_m5p.com]

Updating in light of D31986.

[ehem_freebsd_m5p.com]

Adjusting filename slightly.