x86: Add support for PVH version 1 memmap

[cperciva]

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

[imp]

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

[royger]

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.

[cperciva]

Add missing const

[cperciva]

+ KERNBASE.

[cperciva]

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]

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]

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]

@royger Are you ok with this version?

[royger]

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]

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.

[cperciva]

Move struct bios_smap entry per royger's suggestion.

[cperciva]

sys/x86/xen/pv.c

412

Done.