I'm not convinced this should be tied to the usage of device-tree (as it's in the same file). An Arm guest booting off ACPI would also require the registration of the shared_info page, and hence this routine should be in a separate file.

Same here, the reigstration of the vcpu_info shouldn't be done here, the more that this will only be executed for the BSP, and you need the vcpu_info to be registered for each vCPU on the system

That likely means merging this portion with D29874. I noticed xen_early_init() didn't fit in xen-dt.c, but hadn't noticed xen_init() might also need to be broken off. Hmm.

Maybe. Depends upon whether this can reasonably be shared by multiple architectures. This would likely share with implementation attempts for aarch32 or other ARM on aarch64 Xen. I'm less sure about how well it will share with RISC-V.

The device-tree node is pretty generic. So I would hope that any architecture using Device-Tree would end up to use/improve it rather than reinventing the wheel.

This doesn't seem to be used by this file. Software contexts are not typically tracked in this way, but instead obtained from the device_t structure via device_get_softc(9).

xen_init() is too general a name for this function. This looks to be equivalent to xen_hvm_init_shared_page_info() from x86/xen/hvm.c. Since arm64 will be HVM only (yes?), I guess maybe xen_init_shared_page().

I don't see anything in this file that couldn't be shared, it's all pretty standard fdt/newbus stuff. I'm excluding xen_init(), which I think we agree will go elsewhere under sys/arm64/.

So yes, this should go in the generic code under sys/dev/xen. Please also make this line optional on fdt.

This could be due to having been written in 2015. I'll take a look at this.

I suppose it could be renamed. I can't help noting this is a static function so as long as the name doesn't collide with another function in the file it should be okay.

I'm kind of doubtful Xen is likely to support anything besides device-tree on ARM. The boot method I've been using has been to use a Tianocore build as something akin to a first-stage bootloader (domain configuration file: kernel = "/usr/share/xen/XEN_EFI.fd"). In this situation Tianocore preserves the device-tree as an ACPI table, so this does function in presence of ACPI/UEFI support.

That though is not an argument against breaking this function off and into a separate file.

Other issue is why isn't HYPERVISOR_shared_info in a common file somewhere...

Preferences between sys/dev/xen/dt/xen-dt.c, sys/dev/xen/devicetree/xen-dt.c or other?

I was going to argue for the first due to being shorter, but thanks to filename completion I'm thinking the second.

So yes, this should go in the generic code under sys/dev/xen. Please also make this line optional on fdt.

On the queue.

I don't see anything in this file that couldn't be shared, it's all pretty standard fdt/newbus stuff. I'm excluding xen_init(), which I think we agree will go elsewhere under sys/arm64/.

So yes, this should go in the generic code under sys/dev/xen. Please also make this line optional on fdt.

Xen *does* provide both ACPI and Device-Tree. Although, the support for ACPI in Dom0 is less mature than the guest support.

If an admin wants to use Xen + FreeBSD on server, then he/she will have to use ACPI because quite a few servers don't provide DT (or doesn't guarantee to be fully functional).

Therefore, I think we should avoid to rely too much on DT in port for FreeBSD. Note this is not a request to add support for ACPI as part of this work.

Thing is, this really is highly similar to xen_hvm_init_shared_info_page() in sys/x86/xen/hvm.c. So similar they could fairly readily be merged.

@royger does xen_hvm_init_shared_info_page() actually need to be called on resume on x86? Can it instead simply be called once during sysinit? (if yes, then this could be a static function in common.c; if no, then this could be a global in common.c)

@royger and @julien_xen.org opinion on the differences between xen_hvm_init_shared_info_page() versus what is currently xen_init()? xen_hvm_init_shared_info_page() instead does a malloc(PAGE_SIZE, M_XENHVM, M_NOWAIT), versus the vm_page_alloc() here.

Now part of D28982 and the function is xen_handle_shared_info(). This implementation appears compatible with xen_hvm_init_shared_info_page(), so the two are combined in D28982.

As above, now merged into the function xen_handle_shared_info() in D28982. Hopefully the implementation used in D28982 is acceptable.

Depending on fdt is now done.

If you've written a significant amount of this file you should add your copyright.

@julien_xen.org Can we drop All rights reserved.? We've been removing it from other files (with copyright holders permission) as it's no longer needed.

Missing a newline after int. The function should also have a FDT specific name.

Extra newline after xen_softc

What's this comment for?

Does this depend on 4k pages?

This looks like it's indented too far (although phabricator makes it hard to tell)

This should be indented 1 tab followed by 4 spaces and move the { to the end of the if statement.

Yes this can be dropped.

I'll be looking through the source trying to figure out what you mean by "FDT specific name". My first guess is FDT files are supposed to include some sort of setting to tell OFW code to load them if device X is seen. Mainly searching for a "/hypervisor" which is compatible with "xen,xen" isn't the way it is supposed to be done (this is based on something originally written in 2014, so I wouldn't be surprised at yet more changes).

I don't believe so. I haven't noticed such being hard-coded anywhere, but there could be a lurking gotcha.