I would name this xen_percpu_evtchn{_enabled} or some such. Given the current usage I think xen_support_evtchn_rebind is confusing, as it's not about rebinding an event channel, but being able to assign event channels to vCPUs.

Just replied to the most controversial part I think, will try to review the rest tomorrow.

Thanks and sorry for the delay! Mostly nits, I think the only major question is whether you need to use the dma system to allocate the memory for the vcpu time infos. I think it would be fine to just use plain malloc.

Thanks! For the record: the lock is not currently needed given the only caller has APs and interrupts disabled. I'm fine with adding this safeguard, but it should be noted in the commit message IMO that this is not currently an issue.

Thanks! There's also some ELFNOTES that are now stale, but will cleanup later.

I wondered about this, but since makeplist didn't suggest the prefix I didn't do it. Did I maybe overlook at poudriere output?

LGTM, just one minor nit in the MOVED file I think. Thanks for fixing the xen-tools package also!

Address comments from Elliott.

In D30228#679017, @ehem_freebsd_m5p.com wrote:

This isn't quite what I had expected, I was expecting more of cfa0b7b82fbdda56d7160569def5c6133eb045aa to be reverted. That though appears to have been done in a separate commit which isn't here.

In D29873#679072, @ehem_freebsd_m5p.com wrote:

In D29873#675696, @imp wrote:

The only reason to maybe add it is that our ttys are the same everywhere and we'd like to go to 1 ttys since the need to have it be radically different has passed away.

Given this, I'm kind of tempted to pull the trigger and merge to a single file at the same time as adding the Xen console device.

Made this a flavour of the edk2 port: https://reviews.freebsd.org/D30225

pvclock_softc_init feels slightly weird, as it's not a real softc, same with pvclock_attach or detach, as they are not called from the typical bus functions (ie: there's no identify/probe for example, and neither are part of a device_method_t struct). I would just use pvclock_private, and have a single pvclock_init/pvclock_destroy set of helpers. Attempting to make them look like DEVMETHOD is IMO confusing.

In D29873#675124, @ehem_freebsd_m5p.com wrote:

Think about this, should D29873 add the Xen virtual console device to the sbin/init/ttys.* files for all architectures? This would keep these files consistent and at least one other architecture is looking at potential Xen support. My only question would be the treatment of x86. As of FreeBSD 12, the Xen code was using the emulated serial port as console instead of the ring. Should the ring device be added for x86 even though it currently isn't used?

You will also need to change the call to intr_execute_handlers as that's x86 specific also? And the lapic_eoi call.

LGTM, just one request

If this patch has to make progress the commit message needs to be updated to reflect why a vCPU ID filed might not be needed, and that having a per-arch helper allows to abstract away whether it's a completely different CPU field or just abstracted away from existing data.

In D29873#671117, @ehem_freebsd_m5p.com wrote:

If someone has FreeBSD on a server where Domain 0 does not belong to them, then it could be undesirable to treat the Xen console as secure. Running FreeBSD as a DomU on one's own server, almost certainly you want the console considered secure.

I'll try to come up with better language.

If you only need such early detection for the console support, maybe you could consider placing it in the console probe hook instead?

I think the code is fine, I'm however having trouble parsing the commit message: "Guests on someone else's hardware may disable this, but for one's own hardware this is valuable.", could you please reword to something easier to parse? What do you mean with 'someone else's hardware'?

In D29403#670493, @ehem_freebsd_m5p.com wrote:

In D29403#663721, @royger wrote:

My point is that maybe instead of an inline function you can introduce a define instead, ie:

#define xen_disable_pv_disks false

For Arm, and leave the x86 as-is, just moving the extern declaration into the arch specific xen-os.h.

Did you misread the commit or make a mistake when reading the Phabricator file list? Whether or not that approach is used, sys/xen/xen-os.h still needs to be modified to remove xen_disable_pv_disks from there. No definition or declaration is added for ARM in this commit. The commits for sys/arm64/include/xen/xen-os.h aren't yet present.

Sorry, I didn't have any more time to look into the clock ordering stuff. Using the link doesn't work fine here because FreeBSD will try to initialise clocks that rely on link sources and then fail. IMO the best way would be to init clocks once all the dependencies are meet, but I haven't looked into how to do it, maybe try to init the clock and just postpone if the full dependency tree cannot be initialised?

I don't have a strong opinion on the rewording of the assert messages. @mhorne would you be OK if I commit this with the fixed error != 0 check?

The commit message doesn't seem to match the actual code, it says: "implement suspend as panic for ARM" yet xctrl_suspend for Arm is just a printf, not a panic.

The commit message needs to be adjusted, as it contains a question. All memory shared with Xen is of type WB (the default attribute), but I'm not sure I see the point of the question, the bus_dmamap_sync checks that the submitted segments match the requirement of the underlying device in terms of aligments, sizes and boundaries, or else bounces it to some temporary memory. This cannot be avoided AFAICT. A very good test for the dmamap stuff is newfs, which tends to do weird block writes that trigger the dmamap bouncing for blkfront IIRC.

I think the comment about pmap not being initialized for HVM needs some clarification.

In D29403#659126, @ehem_freebsd_m5p.com wrote:

That is pretty well what the series does. Introduces sys/arm64/include/xen/xen-os.h with xen_pv_disks_disabled() as a static inline which returns false. The real question is whether to adjust history by introducing all of these at the same time as the function is introduced generally. I suspect that is likely better, I think this may be better via e-mail.

LGTM, I'm however tempted to s/__inline/inline/ at commit unless there's a reason to explicitly use __inline.

I'm happy with this, will queue it.

I'm fine with this, if in the future other arches require some arch specific stuff to be executed at a similar place we could introduce an arch-specific hook (ie: xen_cpu_resumed or some such, I'm bad at naming).

I usually import the whole headers at once in order to keep them in sync with specific versions. I would be fine if you just pick the whole contents of the public/ folder after your change and dump it directly on FreeBSD.

One minor comment. I have some comments about gnttab_map, but I would make those in the relevant patch. Maybe we should just name this VM_MEMATTR_XEN and drop the _SHARED suffix.

I'm not sure why this is needed, as I would expect that just using PCPU_GET(vcpu_id) or PCPU_ID_GET(cpu, vcpu_id) if you need to fetch the vcpu id of a different CPU to be enough.

I think the commit message needs some kind of adjustments, as it seems to mix event channel rebind with having setup a vector callback for event channel notification delivery?

Forgot to ask on the previous patch, but I guess the tunable won't be available to Arm guests anyway, in which case it might be easier to just force xen_disable_pv_disks to false for Arm:

Just one minor comment.

LGTM. Thanks!

In D29042#650907, @ehem_freebsd_m5p.com wrote:

Yeah, I've got no idea what-so-ever why one might feel xenpv.c should be
in a "bus" directory. No similarity to any type of bus. No devices
attached at all. Certainly not a bus.

Also, the 'Adapted-to-HEAD-by' tag is not really a standard tag, and doesn't mention the original author either. I would just add a line saying:

LGTM, we should make this a bool type also.

Some minor comments.

LGTM.

I would consider placing it in sys/dev/xen/bus/xenpv.c

Do you have a git repo I can pick this patch from? So that it has the proper Author field. Also in FreeBSD we tend to use the 'Sponsored by:' tag to credit the company that paid for the work (if any):

Don't really have much knowledge in this area to provide a meaningful review I'm afraid.