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

[ehem_freebsd_m5p.com]

Summary

The structure hypervisor_info and associated source, introduced in cfa0b7b82fbdda56d7160569def5c6133eb045aa, is needed on all Xen architectures and should not be in the x86 area. As ARM (likely RISC-V) only has the PVv2/PVH version, leave the PVv1 version as x86-only.

While the mechanism for setting xen_domain_type/HYPERVISOR_shared_info may differ, the presence of the variables will not. All architectures need these.

[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

5139

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.