Page MenuHomeFreeBSD
Differential D35800

x86: Add support for PVH version 1 memmap
ClosedPublic
Actions

Authored by cperciva on Jul 13 2022, 12:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 27, 1:19 PM
Unknown Object (File)
Fri, Dec 27, 1:18 PM
Unknown Object (File)
Fri, Dec 27, 1:09 PM
Unknown Object (File)
Fri, Dec 27, 12:33 PM
Unknown Object (File)
Fri, Dec 27, 12:28 PM
Unknown Object (File)
Thu, Dec 26, 8:45 PM
Unknown Object (File)
Sat, Dec 21, 2:28 PM
Unknown Object (File)
Nov 26 2024, 10:03 AM
View All Files
Subscribers
ehem_freebsd_m5p.com
emaste
imp

Details

Reviewers
royger
imp
Commits
rG023a025b5cb1: x86: Add support for PVH version 1 memmap
Summary

Version 0 of PVH booting uses a Xen hypercall to retrieve the system
memory map; in version 1 the memory map can be provided via the
start_info structure.

Using the memory map from the version 1 start_info structure allows
FreeBSD to use PVH booting on systems other than Xen, e.g. on the
Firecracker VM.

Sponsored by: https://www.patreon.com/cperciva

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 46499
Build 43388: arc lint + arc unit

Event Timeline

cperciva created this revision.Jul 13 2022, 12:47 AM
Herald added subscribers: ehem_freebsd_m5p.com, imp. · View Herald TranscriptJul 13 2022, 12:47 AM
cperciva requested review of this revision.Jul 13 2022, 12:47 AM
Harbormaster completed remote builds in B46400: Diff 108086.Jul 13 2022, 12:47 AM
cperciva added a parent revision: D35799: x86: Add COMPAT_LINUX_MPTABLE_BUGS option.Jul 13 2022, 12:47 AM
cperciva added a child revision: D35801: x86: Distinguish Xen from non-Xen PVH boots.
cperciva added a reviewer: royger.Jul 13 2022, 12:50 AM
imp accepted this revision.Jul 13 2022, 12:58 AM

Code looks sane, but I don't know the diff btn v0 and v1 to know if that's implemented correctly.

This revision is now accepted and ready to land.Jul 13 2022, 12:58 AM
royger added inline comments.Jul 14 2022, 2:10 PM
sys/x86/xen/pv.c
402

This should be const.

405

It's fine to use unsigned int here for both nentries and i, as the size of those fields is 32bits (in fact I don't think you need a local variable for nentries, as you could just use start_info->memmap_entries IMO.

408

For safety you need to add KERNBASE here to the physical address, or you otherwise risk this only working with the initial page tables that have identity mappings.

412

You could limit the scope of entry and define it inside the if:

struct bios_smap entry = {
    .base = entries[i].addr,
    .length = entries[i].size,
    .type = entries[i].type,
};

Or just cast the entry pointer into a struct bios_smap pointer, as the layouts are the same (at least for the 3 fields we care about).

461

Might be worth adding a KASSERT that we only get here when isxen() == true.

emaste added a subscriber: emaste.Jul 17 2022, 6:08 PM
cperciva updated this revision to Diff 108382.Jul 20 2022, 11:56 PM

Add missing const

This revision now requires review to proceed.Jul 20 2022, 11:56 PM
Harbormaster completed remote builds in B46498: Diff 108382.Jul 20 2022, 11:56 PM
cperciva updated this revision to Diff 108383.Jul 21 2022, 12:34 AM

+ KERNBASE.

Harbormaster completed remote builds in B46499: Diff 108383.Jul 21 2022, 12:34 AM
cperciva added inline comments.Jul 21 2022, 12:36 AM
sys/x86/xen/pv.c
402

Thanks, fixed.

405

In practice I'm sure it would be fine, but as a matter of style I prefer to use size_t for sizes of and indexes into arrays.

408

Thanks, fixed.

412

I considered that, but I figured this way was clear and gave me a convenient place to include the comment
explaining that XEN_HVM_MEMMAP_TYPE_* values match SMAP_TYPE_* values and that copying them
directly isn't a bug.

ehem_freebsd_m5p.com added a comment.Aug 13 2022, 3:34 AM

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)

cperciva added a comment.Aug 17 2022, 12:10 AM
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'm looking at sys/contrib/xen/arch-x86/hvm/start_info.h. PVH was invented by Xen with a "version 0" which requires a Xen hypercall to obtain the memory map; for non-Xen platforms (which don't have the Xen hypercall) "version 1" is effectively where things start.

cperciva added a comment.Aug 17 2022, 12:11 AM

@royger Are you ok with this version?

royger added a comment.Aug 18 2022, 12:46 PM
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)

This is slightly different, PVHv1 didn't use the hvm start info structure, and was completely different from what's current PVH. The version of the start info strutructe is decoupled from the PVHvX suffix that you will see in some documentation.

royger accepted this revision.EditedAug 18 2022, 12:54 PM

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

sys/x86/xen/pv.c
412

Could you at least define entry inside the for loop to limit the scope? It's only used there AFAICT.

This revision is now accepted and ready to land.Aug 18 2022, 12:54 PM
cperciva updated this revision to Diff 109541.Aug 19 2022, 1:21 AM

Move struct bios_smap entry per royger's suggestion.

This revision now requires review to proceed.Aug 19 2022, 1:21 AM
Harbormaster completed remote builds in B47016: Diff 109541.Aug 19 2022, 1:21 AM
cperciva marked an inline comment as done.Aug 19 2022, 1:22 AM
cperciva added inline comments.
sys/x86/xen/pv.c
412

Done.

royger accepted this revision.Sep 23 2022, 1:34 PM
This revision is now accepted and ready to land.Sep 23 2022, 1:34 PM
Closed by commit rG023a025b5cb1: x86: Add support for PVH version 1 memmap (authored by cperciva). · Explain WhyOct 18 2022, 6:03 AM
This revision was automatically updated to reflect the committed changes.
cperciva marked an inline comment as done.
cperciva added a commit: rG023a025b5cb1: x86: Add support for PVH version 1 memmap.

Revision Contents
Changeset List

PathSize
sys/
x86/
xen/
48 lines

Diff 108383

View Options

sys/x86/xen/pv.c

Loading...