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

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.

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.

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?

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.

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.

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

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

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.

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.

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

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

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.

hvm_common.c

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.

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.

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

You can drop this blank line.

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.

See below.

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).

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.