Page MenuHomeFreeBSD

xen/arm64: introduce sys/arm64/include/xen/xen-os.h
Needs ReviewPublic

Authored by ehem_freebsd_m5p.com on Mar 30 2021, 6:52 PM.

Details

Reviewers
royger
mhorne
manu
Summary

Based on a 2014-01-13 17:40:58 commit by Julien Grall <julien@xen.org>
titled "xen/arm64: add xen platform". This is one of many fragments
resulting from that commit.

These are the values appropriate for ARM64 on Xen. Since the ARM64
implementation is more recent, there are fewer changeable settings and
most are constants (unlike x86).

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 <ehem+freebsd@m5p.com>
Original implementation: Julien Grall <julien@xen.org>, 2021-06-28 15:31:57

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 40479
Build 37368: arc lint + arc unit

Event Timeline

My thinking is, if this is brought in now then various commits could include both the x86 and ARM settings. Thus illustrating why those needed to be added. Notably, D29351, D29402, D29403, D29404, D29405, and D29406 all use distinct implementations for ARM.

The downside is this leaves a mysterious header hanging out in ARM64 land apparently doing nothing until the rest of Xen/ARM is operational.

mhorne requested changes to this revision.EditedMar 30 2021, 8:05 PM

My thinking is, if this is brought in now then various commits could include both the x86 and ARM settings. Thus illustrating why those needed to be added. Notably, D29351, D29402, D29403, D29404, D29405, and D29406 all use distinct implementations for ARM.

The downside is this leaves a mysterious header hanging out in ARM64 land apparently doing nothing until the rest of Xen/ARM is operational.

Although I understand your reasoning, I do not think this is the right way to handle this.

The reviews you linked are useful in their own right: they do the preliminary cleanups required to accommodate support for a new architecture. Adding the arm64 versions of those definitions is outside the scope of this, since that support doesn't exist yet in FreeBSD.

Introducing this header in such a piecemeal way will complicate the history. I would rather see it committed in its entirety alongside other relevant pieces of the arm64 xen support.

This revision now requires changes to proceed.Mar 30 2021, 8:05 PM

Now realizing I misunderstood Roger Pau Monné's comment, I basically agree with Mitchell Horne on his view. Even had I understood correctly on the first try, this approach was worthy of considering anyway.

I keep coming back and wondering if this really is the way things should be done. There are a bunch of commits which potentially merely add a line or two to this file and don't really own it. This would also serve as the marker for where the real implementation of Xen/ARM support starts in history.

While the new version will include rather more in the file during introduction, I'm still finding this file seems to function best being introduced by itself. The new revision though will include a bunch of the constants which x86 includes in its version of the file.

This revision now requires changes to proceed.Jun 29 2021, 3:45 AM

Mostly going to be redoing most of this, but this is likely to be a single version.

Updating to current version. This is nearly everything which ends up here.

Guess this really does need review at this stage.

Doing some later testing and notice for D29498, FreeBSD's style is parentheses around return values. I'll be updating that, but this other item showed up.

sys/arm64/include/xen/xen-os.h
54–55

@julien_xen.org I'm in the middle of testing related to this. The code and comment disagree here. If it is always possible to rebind event channels on ARM, this should be returning true instead of false.

Simply a goof during development and your project never got to the point of testing this?

Adjusting return of xen_support_evtchn_rebind(). This is expected to be renamed later (possibly during commit), but for now simply update Phabricator so wrong thing doesn't get committed. Correct value is "true" since this is available on aarch64.