Page MenuHomeFreeBSD

x86: Add support for PVH version 1 memmap
ClosedPublic

Authored by cperciva on Jul 13 2022, 12:47 AM.
Tags
None
Referenced Files
F80132686: D35800.id108383.diff
Thu, Mar 28, 8:35 AM
Unknown Object (File)
Mon, Mar 4, 5:57 AM
Unknown Object (File)
Mon, Mar 4, 5:57 AM
Unknown Object (File)
Mon, Mar 4, 5:56 AM
Unknown Object (File)
Mon, Mar 4, 5:56 AM
Unknown Object (File)
Mon, Mar 4, 5:52 AM
Unknown Object (File)
Mon, Mar 4, 5:52 AM
Unknown Object (File)
Sun, Mar 3, 3:38 AM

Details

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 47016
Build 43905: arc lint + arc unit

Event Timeline

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
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).

462

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

This revision now requires review to proceed.Jul 20 2022, 11:56 PM
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.

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 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.

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.

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

Move struct bios_smap entry per royger's suggestion.

This revision now requires review to proceed.Aug 19 2022, 1:21 AM
cperciva added inline comments.
sys/x86/xen/pv.c
412

Done.

This revision is now accepted and ready to land.Sep 23 2022, 1:34 PM
This revision was automatically updated to reflect the committed changes.
cperciva marked an inline comment as done.