Page MenuHomeFreeBSD

xen: create VM_MEMATTR_XEN for Xen memory mappings
AcceptedPublic

Authored by ehem_freebsd_m5p.com on Mar 20 2021, 3:24 PM.

Details

Reviewers
royger
mhorne
Summary

The requirements for pages shared with Xen/other VMs may vary from
architecture to architecture. As such create a macro which various
architectures can use.

Remove a use of PAT_WRITE_BACK in xenstore.c. This is a x86-ism which
shouldn't have been present in a common area.

Original idea: Julien Grall <julien@xen.org>, 2014-01-14 06:44:08
Approach proposed by: Roger Pau Monné <royger@FreeBSD.org>

Diff Detail

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

Event Timeline

Hmm, really need to give credit to others since the original idea came from Julien Grall and Roger Pau Monne.

This interacts with D29305, since that might prefer VM_MEMATTR_XEN_SHARED. Issue is whether D29305 is going to get approval first, versus D29351. There is also D29042 where xenpv.c also has a use of VM_MEMATTR_DEFAULT which likely should be addressed by this.

There is a lot of potential interaction with these Xen patches. I'm trying to hold the ones with the extreme interactions, but there is a good chance I may lose track.

Including the extra spots in this diff/patch. The spot in grant_table.c
could be taken care of by properly fixing the grant table code. Depending
upon whether D29042 is committed before versus after this, the file could
be in either spot.

I'm unsure of how to properly handle situations where ordering is
*crucial*. Many subsequent commits also depend upon this one.

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.

sys/x86/include/xen/xen-os.h
33

Better use VM_MEMATTR_WRITE_BACK explicitly here. Not that I think the default will ever change, but better be safe than sorry.

I think you're actually pointing at a significant issue with how Julien Grall's commit series was originally constructed. The series was arranged as a bunch of infrastructure adjustments (adding xen_pmap()/xen_*()) and then most of the ARM-specifics bits were done in larger commits near the end.

Now that I think about it, this seems a poor choice. Seems a better choice may be to create sys/arm64/include/xen/xen-os.h in one early commit, then add the ARM versions of these functions as the functions are added. This though means substantial adjustment to commit history.

sys/x86/include/xen/xen-os.h
33

Notice where this is, this is sys/x86/include/xen/xen-os.h. This is preserving existing behavior for x86. Though you could want to change the behavior for x86. The commit for sys/arm64/include/xen/xen-os.h was separate.

Doing the rename to VM_MEMATTR_XEN. Without objections it is due.

Only issue is surrounding ideas...

This patch interacts with D29498. The theory behind D29498 is if D29498 is committed first, then D29351 would include adding

#define VM_MEMATTR_XEN VM_MEMATTR_WRITE_BACK

to sys/arm64/include/xen/xen-os.h. This might make it clearer what is happening.

Trick is requires D29498 to be committed in a timeframe of now.

ehem_freebsd_m5p.com retitled this revision from xen: create VM_MEMATTR_XEN_SHARED for Xen memory mappings to xen: create VM_MEMATTR_XEN for Xen memory mappings.Mar 30 2021, 10:48 PM
sys/x86/include/xen/xen-os.h
33

VM_MEMATTR_DEFAULT is defined as VM_MEMATTR_WRITE_BACK so there is no behaviour change by using one or the other.

Hmm, just realized I misread what Roger Pau Monné in the comment. I was
reading it as Roger Pau Monné thought this was
sys/arm64/include/xen/xen-os.h and needed ARM's value. Instead was
Roger Pau Monné thinking x86 should also be using this value? I'm fine
with that.

Sorry language is an awful easily confused tool. That goes double for
natural languages.

An awful lot of the subsequent project depends upon this commit. Could review for D29351 get accelerated? (in particular an extra hunk could be added to D29305 if this gets in soon, plus I've got another one queued, but waiting for D29351 to be committed)

Looks good to me, I'm happy to commit this but will wait a day or two in case @royger wants to sign off.

This revision is now accepted and ready to land.Apr 5 2021, 11:53 PM

An awful lot of the subsequent project depends upon this commit. Could review for D29351 get accelerated? (in particular an extra hunk could be added to D29305 if this gets in soon, plus I've got another one queued, but waiting for D29351 to be committed)

By the way, you can set parent/child relationships between reviews if you go to Edit Related Revisions. This would help clarify the intended order.

I'm happy with this, will queue it.