Minimal collection for running on Xen/ARM64 #1358
Conversation
|
Commit cecb5a1 was tested and confirmed to work. |
|
Before being rebased, 54bf574 was tested and confirmed to work. The delta is simply removing the stray clock interrupt fix and pulling 2 of the intermediates. Mostly there are 3 problems in need of fixes. First, since Xen interrupts are very dynamic, release needs to work #1281. Second, some sort of per-processor hook is needed; Lastly, INTRNG's interface is appropriate for many PICs. Problem is due to being an unusual bus, this is well suited to a lower-level interface, #1285. Trying to work with the existing interface would be quite expensive. Then simply becomes an issue of figuring out how much squashing should be done (while @jgrall did the original implementation, this has rather a lot of changes to get it into shape). |
|
So instead of #1280, #1363 also functions to provide access to the appropriate hooks. For In After the |
sys/kern/subr_intr.c
Outdated
| return (NULL); | ||
|
|
||
| KASSERT((pic->pic_flags & FLAG_TYPE_MASK) == (flags & FLAG_TYPE_MASK), | ||
| ("%s: Found wrong type of controler: %s (flags = %u, type = %u)", |
sys/kern/subr_intr.c
Outdated
| @@ -815,7 +815,7 @@ pic_create(device_t dev, intptr_t xref, u_int flags) | |||
| pic->pic_dev = dev; | |||
| pic->pic_flags = flags; | |||
| mtx_init(&pic->pic_child_lock, "pic child lock", NULL, MTX_SPIN); | |||
| SLIST_INSERT_HEAD(&pic_list, pic, pic_next); | |||
| STAILQ_INSERT_TAIL(&pic_list, pic, pic_next); | |||
There was a problem hiding this comment.
You should at the very least have a comment mentioning that the insertion order is important, since it preserves an invariant that you apparently depend on.
There was a problem hiding this comment.
I don't depend on it. There was a statement the proposed IPI PIC for RISC-V would depend on this though. In truth there are so few uses of pic_init_secondary() that doing anything is simply a guessing game.
Since this seems so controversial I'll likely be switching to declaring the Xen entry as a separate root PIC. This is mildly abusing that functionality, but does seem a good choice as a backup solution.
There was a problem hiding this comment.
If you don't require this, then it doesn't belong in minimal set of patches for running Xen on arm64. I don't have strong opinions on the issue at hand, but since it's difficult to get consensus on intrng changes, your goal is easier to achieve if we can avoid those that aren't strictly needed.
There was a problem hiding this comment.
I don't depend on any invariant. There are claims of there being invariants, but I can't verify any of those. Notably the call I need doesn't care about ordering.
Some sort of hook into processor startup is needed. Otherwise we end up with all Xen events on a single processor. There is also an alternative approach which showed up after this had last been updated.
sys/kern/subr_intr.c
Outdated
| if (pic != NULL) { | ||
| pic->pic_flags |= flags & FLAG_TYPE_MASK; |
There was a problem hiding this comment.
Which PIC driver requires this? I can't see any uses of intr_msi_register() in Xen.
There was a problem hiding this comment.
This was simply trying to preserve behavior of the pic_init_secondary() hooks. By merging them controllers only have this called once.
There was a problem hiding this comment.
"Trying" meaning that it's supposed to work but isn't tested? It's not clear to me.
There was a problem hiding this comment.
Meaning the case I have the GICv3 doesn't have MSI capability, so this isn't well-tested. That is where this comes into play.
sys/kern/subr_intr.c
Outdated
| //mtx_unlock(&isrc_table_lock); | ||
| //mtx_lock(&pic_list_lock); | ||
| STAILQ_FOREACH(pic, &pic_list, pic_next) | ||
| PIC_INIT_SECONDARY(pic->pic_dev); |
There was a problem hiding this comment.
This isn't minimally required for arm64/Xen, as it changes the behaviour of other PIC drivers which implement PIC_INIT_SECONDARY.
There was a problem hiding this comment.
Have you noticed how many implementations of that there are? The hook is barely used. Most PICs leave the empty default. As a result the hook is effectively broken for anything which isn't a root PIC.
Turning it into a root PIC mildly abuses the functionality, but does sort of match what the device-tree looks like.
There was a problem hiding this comment.
Of course I noticed, otherwise I wouldn't have commented. The point is that there is more than zero, and I don't see any explanation in the commit logs of how they were tested, nor any explanation for why it is unlikely to cause problems.
There was a problem hiding this comment.
Being rarely used means it is simple to examine the uses and take actions believed to preserve functionality. I lack any systems to fully test this, but I can confirm it provides an appropriate hook to get Xen fully functional.
The default hook is a NOP. Only GICv3 tries to propagate the call, but that requires a device-tree which conforms to the preconceptions of someone. Since the one provide via Xen differs, there is a problem.
Anyway I was planning to abandon this approach as while the issue in the comment remains "QQQ: Only root PIC is aware of other CPUs ???". Hooking in as a root also works.
|
I'm expecting the per-processor startup hook to change in light of other recent commits. Two issues with INTRNG remain though:
|
intr_describe_irq() was mixing two operations together. There was the mapping step and the underlying intr_event_describe_handler() call. Split these two steps apart to match other portions of the interface. Differential Revision: https://reviews.freebsd.org/D39333
Specialized peripheral PIC drivers may need to handle most interrupt setup steps themselves without touching newbus. Two cases are intr_add_handler() and intr_describe() which are internally used by `intr_setup_irq()`/`intr_describe_irq()`. Exposing these allow for alternative use case. In fact the BUS interface for INTRNG is arguably a layering violation. Those 6 functions could just as well be in nexus.c or a kernel library. Differential Revision: https://reviews.freebsd.org/D39333
There is potential for non-root PICs to need per-processor initialization. Few root PICs try to propogate the call. As such add a pass of calling pic_init_secondary() on all children with a non-root value. Differential Revision: https://reviews.freebsd.org/D40474
There is minimal benefit in delaying event allocation. Worse, this reduces opportunities for merging architectural interrupt structures together. Since the event is now created before being added to the interrupt table, there is no longer any need for locking. A few spots also no longer need to check for ->isrc_event being NULL, clean those up. Differential Revision: https://reviews.freebsd.org/D40166
SSIA. If the event is still valid when deregister is called, it needs to be destroyed. Differential Revision: https://reviews.freebsd.org/D38599
Match the i386/AMD64 headers in doing inline functions for the Xen hypervisor calls. This has been heavily inspired by work done by Julien Grall and Stefano Stabellini. Differential Revision: https://reviews.freebsd.org/D30996
There is minimal variation between architectures for the hypercall wrappers. Most common is word size, which effects a few wrappers, for these check for ILP32 (which identifies all 32-bit architectures). Also notable is HYPERVISOR_set_trap_table() which appears x86-only. Due to the minimal difference, move most of the upper level wrappers to a common header, leaving only the lower level wrappers with the architecture. SPDX tags were added. Add comment documenting _hypercall#() interface with an eye towards future development. The amd64 definition of HYPERVISOR_set_callbacks() was wrong and the i386 was used instead (last argument is ignored by Xen on amd64, but the definition was still wrong).
xencons_cnprobe() is an ideal point to probe for the presence of the Xen hypervisor. This is the first place which MUST know whether Xen is present (unless the console is disabled). For x86 there are other earlier spots where Xen's presence is currently probed, but for other architectures this is ideal. No functional changes intended. Reviewed by: royger Differential Revision: https://reviews.freebsd.org/D30816
The main source file originated as the files "sys/arm64/xen/xen-dt.c" and "sys/arm64/xen/xen_arch_intr.c". They're being merged. These originally resided in sys/arm64/, but they've been moved to sys/dev/xen/bus/intrng-* since they can likely be shared between INTRNG architectures. Julien Grall's original commits lacked the copyright notice, but Julien Grall has agreed to BSD license.
Many settings in xen-os.h differ from x86 to ARM64. Due to Xen's much longer history on x86, several features are optional on x86, but guaranteed on ARM64. Based on a 2014-01-13 17:40:58 commit by Julien Grall <[email protected]> titled "arm64: Add xen platform". This is one of several fragments resulting from that commit. xen_pmap()/xen_unmap() were replaced by VM_MEMATTR_XEN. ARM specifically needs these mappings cacheable, as such prefer VM_MEMATTR_WRITE_BACK over VM_MEMATTR_DEFAULT. Submitted by: Elliott Mitchell <[email protected]>, 2021-06-28 15:31:57 Original implementation: Julien Grall <[email protected]>, 2014-01-13 17:40:58 Differential Revision: https://reviews.freebsd.org/D29498
Add the earliest stage of Xen guest initialization. This checks for the presence of Xen and sets flags to indicate the need to initialize other pieces (or not). This retrieves Xen's recommended address range for grant table and interrupt for event channel. The interrupt is forwarded to the Xen interrupt handling code. Originally the device was "xen0", but "xen-dt0" should hopefully be clearer in boot messages. The address range is for grant table and the interrupt is used for the event channel. The test in xen_dt_probe() was adjusted to match how such probes are done. The location where presence is probed was also adjusted. This was originally implemented by Julien Grall in 2014. This is was broken off of a much larger commit. Numerous adjustments have been done by Elliott Mitchell in 2021. Submitted by: Elliott Mitchell <[email protected]> Original implementation: Julien Grall <[email protected]>, 2014-01-13 17:40:58 Differential Revision: https://reviews.freebsd.org/D29875
This is the core of the architecture-dependent portion of the Xen event channel/interrupt handling. Originally implemented by Julien Grall in 2015, but heavily updated for submission by Elliott Mitchell in 2021 and 2022. This is based on the core FreeBSD interrupt code. While the additional functionality provided by intrng could be nice, only the core is actually required. The original file had been sys/arm64/xen/xen_arch_intr.c, but this has been merged into a common INTRNG file. Reason being this portion can likely be shared with other INTRNG architectures (notably RISC-V). Submitted by: Elliott Mitchell <[email protected]> Original implementation: Julien Grall <[email protected]>, 2015-10-21 07:18:56 Original implementation: Julien Grall <[email protected]>, 2015-11-02 04:31:56 Differential Revision: https://reviews.freebsd.org/D31062
Updates to the Xen event channel core made the port unsigned. As such the format string should specify unsigned.
This is closer to the original commit: arm64: Add xen platform This platform code allow FreeBSD to boot as Xen on ARMv8 guest. I'm not 100% sure of the implementation of synch_* on ARM64. I choose to use the atomic_* builtin. Is it SMP-safe?
Unsurprisingly >5 years was more than enough to require updates to this function. Luckily not too complicated of a fix. Changing to constants is also a Good Thing.
This distributes the event channels among processors instead of placing all of them on vCPU#0. Normal interrupt sources are balanced once, at the end of the boot process. Since new event channels can be created any time, they need to be dynamically balanced. Differential Revision: https://reviews.freebsd.org/D31690
The isrc allocation has been moved to architecture code where the decision to allocate or not can be made. This means setting of several fields needs to be handled by architecture code. Modify the prototype of xen_arch_intr_alloc(), start passing the variables needed, and finally set them on aarch64. This also means releasing of isrcs also needs to be moved to architecture code. Differential Revision: https://reviews.freebsd.org/D31063
Fallout from abc7a4a. Apparently nowhere else in FreeBSD was the PAGE_MASK_4K macro used. Though perhaps few places were using the explicitly sized PAGE_* macros.
Simple cleanup.
New implementation of the check. While Julien Grall's implementation had the check, it was completely removed during bring-up for expedience. This has been rebased on top to recognize the original also had the test even though its action no longer works.
Nominally the function should work for this case too, just it hasn't been tested in this situation.
With the variables now in xen_common.c, this file no longer needs to define them. Nuke this remnant.
Some previous builds appeared to potentially need the simplebus binding, but this has been shown to be unnecessary. Only use ofwbus since that is how Xen/ARM passes the hypervisor information. This is known to work once based on top of INTRNG, does this actually work when on top of the kernel events?
Initial implementation. Best to replace the existing file Julien Grall originally created. I /think/ this is what these are supposed to do...
Almost certainly needed.
This matches the timing for setting up the vcpu_info tables. Just after secondary processors have been started and called on each one individually.
Alas, ARM declared xen_ulong_t to be 64-bits long, unlike i386 where it matches the word size. As a result, compatibility wrappers are needed for Xen atomic operations.
The step of enabling the setup in general is better a separate commit. This was broken off of "xen/arm64: add xen platform". Submitted by: Elliott Mitchell <[email protected]> Original implementation: Julien Grall <[email protected]>, 2014-01-13 17:40:58 Differential Revision: https://reviews.freebsd.org/D30950
Presently the probing mechanism for Xen requires device-trees, but no other portion requires FDT. As a result marking everything as depending upon FDT would be wrong, but this requires excluding Xen from LINT-ACPI. Add a similar section to LINT-FDT since it will likely be needed in the future.
Initial implementation. Best to replace the existing file Julien Grall originally created. I /think/ this is what these are supposed to do...
Should handle the interface for rebinding interrupts. May well produce error returns though. Based on GICv3.
This /looks/ plausible based on examples.
Seems using the proper interface adds a colon for us. Adjust to match what we should look like.
When this is called there /should/ be a source, right? (hmm, that may be backwards...)
The interface considers was previously willing to call bind without the event having been set. This is now impossible, but keep this as an assert. This fixes the previous commit. Appears subsequent adjustments omitted handling this.
Several functions have turned back into extern, now onto implementation.
Rather than looking at the event, INTRNG chooses to duplicate this data. As such need to update the information here too.
The use of these has completely disappeared, so nuke them.
Hand regions provided by Xen for grant-table and other allocations. This was originally intended for grant-table mappings, but Xen allows them to be used for anything. Xen guarantees these regions will be empty, and the device-tree code prevents them from being used for other purposes.
Despite the documentation being unclear, this *does* need to be initialized.
Now that we can add the handler and describe via INTRNG, do so. This might also fix balancing during start.
Appears INTRNG wants the PIC to choose the processor, not suggesting a processor? Seems odd. Try this.
Many thanks to Julien Grall who did the initial work in 2014-2015. Now we can have FreeBSD on Xen on ARM64 machines. Want accelerated graphics on a Raspberry PI? Differential Revision: https://reviews.freebsd.org/D31956
ARM64 has been completed. A great deal of the recent work was done by royger.
For people who didn't figure out the end goal of all the pull requests, here is a proof of concept with the pieces gathered together. Large amounts of other cleanup was found during implementation, hopefully those get in too.
The presently tested and known working bootloader is the ArmVirtXen build of Tianocore/EDK2. Due to a change in Xen at present one patch is required. Hopefully a similar patch will be integrated soon, but presently that extra is needed.
Configure the VM with
kernel = "XEN_EFI.fi"and a FreeBSD mini-memstick build as a disk. Then everything will boot as expected.Likely a bunch of squashing is needed, this though is a bit tricky to ensure Julien Grall gets appropriate credit. Though the Xen implementation was heavily rewritten.