You will likely need to introduce a wrapper for xen_intr_assign_cpu() in order to keep the second parameter as an apic_id.

In D37682#857040, @ehem_freebsd_m5p.com wrote:

Updating to newer variation of Xen's interface here. I'd noticed NR_EVENT_CHANNELS and EVTCHN_2L_NR_CHANNELS before, but hadn't known which one was the more recent/proper one.

There is substantial overlap with D30743. Notably D30743 adds a bunch of uses due to _Static_assert()s with the constant. Looking at the number of uses, I would suggest xen_intr.c should use a #define NR_EVENT_CHANNELS, which then moves to sys/dev/xen/bus/intr-internal.h with D30648. The reason being there are so many uses, both the architecture and independent portions would need simultaneous update if there was a move to the more recent types of event channel.

Sorry, I'm on parental leave and so far didn't have time to look into this. I will be back on the 8th of March, is there any rush to get this in now? If there's a rush I would be fine with this going in as long as someone can give a look and test it.

LGTM. Thanks for doing this.

IMO would be better to do this probing inside of an Arm specific early init routine, but if the current approach works and it's the prefered way for Arm people that's fine with me.

In D37067#843235, @emaste wrote:

It looks like MAX_APIC_ID is used only in madt_parse_cpu() as a limit, so this change should have no functional impact on any currently working system.
I agree that we should address allocations sized by max_apic_id; we need to do something similar for arrays sized by MAXCPU and that's going to be a lot more work.

In D37067#841727, @emaste wrote:

Could you do this in the same patch where MAXCPU is increased?

I'd like to do it independently, so that setting MAXCPU via the kernel config works again. Ideally also MFC this (but no MAXCPU bump) to stable/13. Alternatively, could we just replace MAX_APIC_ID with MAXCPU * 2?

Could you do this in the same patch where MAXCPU is increased? I think it would be clearer that there's a dependency between MAXCPU and MAX_APIC_ID and that both need to be increased in lockstep.

No, MAX_APIC_ID is also relevant for amd64, as it's used by madt_parse_cpu(), it needs to be bumped to twice the value of MAXCPU I think.

But you need to also bump MAX_APIC_ID, or else the MADT parse code will start dropping CPUs that have IDs > 512. Note that APIC IDs don't need to be continuous, so it's possible to have a system only with even APIC IDs (ie: 0, 2, 4..) at which point in order to support 512 CPUs you might need to be able to parse APIC IDs up to 1024 or more.

LGTM, the commit/revision title needs updating AFAICT.

LGTM, just one nit. Fell free to commit when that's done, no need for another round of review.

FWIW xen_time.c might be slightly better, because it contains the code for the event timer, but also the time counter and the system clock.

I was expecting this would happen sooner or later, TBH I was waiting for a real clash to happen before doing the renaming. I assume you now need to use parts of this file on Arm?

LGTM. I would use unsigned int as array index, but that's a question of taste.

In D35800#821423, @ehem_freebsd_m5p.com wrote:

This mostly doesn't effect my goals so I've been quiet. One issue here though, what is your source for PVH version numbers?

Previous work has referred them using one-based versions, whereas you appear to be using zero-based versions. Of note ac3ede5371af/D30228. Perhaps Firecracker uses zero-based while Xen uses one-based? (I'm unsure which is "official", just prior usage is one-based)

I think I would prefer setting it to PVH in both cases, we have other ways to expose the underlying hypervisor, and the boot entry point is the same for Xen or KVM PVH.

I wonder whether we might want to move part of the x86/xen/pv.c contents (IOW: those used by firecracker also) into a more generic file? Maybe sys/x86/x86/pvh.c?

• Use MAX() instead of max().
• Remove extra newline.

Nah, I guess there's no need to move it, so I will keep as-is to avoid code churn. Sorry for the alias mess :).

Thanks!

In D35001#793469, @jhb wrote:

I'm axing the devclass argument to DRIVER_MODULE since it is unused the vast majority of the time. If you'd rather not hardcode the string we could instead use driver->name instead of the hardcoded string.

Note however that we hardcode the driver name a few lines below in the call to BUS_ADD_CHILD, so if we are really worried about that then we need to change BUS_ADD_CHILD to also use driver->name.

Hm, isn't this more fragile, as we need to keep the string in driver_t in sync with the call to devclass_find()?