x86/xen: use UNUSABLE e820 regions for foreign mappings

[royger]

Summary

The current approach is to create foreign mappings in any unpopulated address
on the memory map. This however can cause issues, as late-loaded drivers could
then found their MMIO region has been stolen to be used as foreign mapping
scratch space (due to the Xen drivers having started first).

Workaround this on dom0 by using UNUSABLE e820 ranges as scratch space for
foreign mappings. The e820 memory map provided to dom0 is based on the native
one, but since PVH dom0 uses second stage translation, the UNUSABLE ranges on
the host memory map doesn't affect it, and we can also guarantee no device
MMIO uses those.

Additionally, any RAM in the e820 not available to dom0 because dom0
memory has been limited on the command line, or because those are in use by
Xen, are converted to UNUSABLE in the dom0 memory map.

Sponsored by: Cloud Software Group

[ehem_freebsd_m5p.com]

This is oddly similar to what Julien Grall did in 2015. For aarch64 the suggested range for the shared information page is available to the platform device. Perhaps the architecture should pass the struct resource * to xenpv?

[royger]

In D44251#1009698, @ehem_freebsd_m5p.com wrote:

This is oddly similar to what Julien Grall did in 2015. For aarch64 the suggested range for the shared information page is available to the platform device. Perhaps the architecture should pass the struct resource * to xenpv?

If that works for Arm, I don't see why not. Just to be clear, you are suggesting that the only arch-specific bit is the initialization of mem_rman if I understand correctly?

[ehem_freebsd_m5p.com]

I think so (suddenly worried about famous last words).

[royger]

Move part of the logic to be common between all possible arches, just leaving the initialization of the resource manager arch-specific.

[markj]

I just left some comments about style. I'm not too familiar with rman but this looks reasonable to me.

sys/dev/xen/bus/xenpv.c

104

sys/x86/xen/hvm.c

553

584

[ehem_freebsd_m5p.com]

Right now mostly looks okay, but my initial test failed. Could be my implementation failed, but right now I'm hunting an issue.

sys/dev/xen/bus/xenpv.c

100–105

I'm inclined to simply omit this. I imagine every architecture will want an implementation, so this placeholder seems dubious value.

110–118

May not violate the official style, but I dislike declaring and setting at the same time. This also makes this one look quite different from every other use of error.

162

This needs a message to aid someone trying to figure out why an allocation failed.

174–175

I suspect this will rarely be used as I suspect all architectures will implement the resource manager. The only exception might be i386 where it might be possible for the e820 regions to be omitted. I would also be tempted to get rid of LOW_MEM_LIMIT.

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

101–102

In the case of the Xen interrupt controller, I thought it appropriate to declare the functions in the architecture header since those functions were hot and many were trivial enough for the compiler to inline. Here, the function is quite cold and I imagine most (likely all) architectures will want to implement it. As such I think this should be in sys/xen/xen-os.h instead.

[ehem_freebsd_m5p.com]

sys/dev/xen/bus/xenpv.c

152

I haven't yet confirmed this, but I'm starting to wonder whether RF_ACTIVE | RF_UNMAPPED is right. Could well be my attempt at interfacing is wrong though.

[ehem_freebsd_m5p.com]

I don't know what the commits look like behind the scenes. I think implementing the use of &unpopulated_mem should be a separate commit from adding the use of the e820 region.

In case you haven't guessed, I'm trying to get this to work on ARM64 and I haven't gotten it to work yet. As a result, some comments may later turn out to be noise. In particular my earlier comment about line 167 looks to have been wrong.

sys/dev/xen/bus/xenpv.c

133–155

Something is wrong here. The rman_get_flags(res) & RF_ACTIVE looks like you're trying to distinguish whether a resource belongs to &unpopulated_mem versus being allocated by bus_alloc_resource(). Is there actually any need to deactivate the resource before releasing it?

151–152

Likely this should be used instead of requiring the architecture to page-align the range(s).

156–161

RF_ACTIVE is supposed to tell the parent/owner to activate the resource before handing it to us. Won't this be a nop?

186

Both here and line 234 rman_is_region_manager() is used to test whether the resource was allocated from &unpopulated_mem versus bus_alloc_resource(). Perhaps release_unpopulated_mem() should be modified to handle both cases?

213

See comment for line 202. I suspect it may be best to replace release_unpopulated_mem() with a function which handles both of these.

sys/x86/xen/hvm.c

580–581

I don't think the interface should depend on this behavior. I think instead rman_reserve_resource_bound() should be used instead if alignment is required.

[ehem_freebsd_m5p.com]

Got it working on arm64 and this does indeed seem a better approach to one issue.

Seems my comment about line 167 was wrong. I'm still rather unsure about lines 133–155 though.

sys/dev/xen/bus/xenpv.c

71

I don't know about x86, but on arm64 this appears to overflow the length of the buffer. devinfo -u, "Xen unpopulated memory addresse:". Though that could also be a devinfo bug too.

[ehem_freebsd_m5p.com]

sys/dev/xen/bus/xenpv.c

71

sys/sys/rman.h => rm_descr[RM_TEXTLEN] => #define RM_TEXTLEN 32

I should also note similar things would usually be called "available", rather than "unpopulated". Guess it doesn't make a huge difference, but 2 characters shorter does help.

[ehem_freebsd_m5p.com]

sys/dev/xen/bus/xenpv.c

151–152

After some checking of rman_reserve_resource{_bound}(), seems this is the correct invocation instead. Sorry about the noise, but I've had an interesting time looking into this mess.

[royger]

Thanks, I've incorporated the feedback.

sys/dev/xen/bus/xenpv.c

100–105

It's IMO easier for bringup purposes to fill this hook later, specially taking into account there's a fallback implementation in case the arch doesn't provide any preferred ranges.

133–155

I'm unsure whether rman_release_resource() could choke now (or in the future) if the to be release resource is still active. From an strict view the resource is not mapped, so the deactivation is a formality. That would be different if we didn't use the RF_UNMAPPED flag.

151–152

Seeing this, we also want to round up size to PAGE_SIZE. All callers should already request multiple of PAGE_SIZE allocations, but since you want to have the alignment explicit we should also do for the size. Note we should also do the same for the call to bus_alloc_resource() then.

162

This will get way to verbose if unpopulated_mem is not filled by the architecture, as every allocation will result in an error printed and fallback to generic bus_alloc_resource(). There's already a message printed once that warns the user when falling back to bus_alloc_resource().

174–175

At least on x86 we need this as a fallback in case resources in unpopulated_mem are exhausted, at which point the limit is still useful on amd64 to prevent allocating memory from the low 4GB which is way more likely to be used by device MMIO.

186

No strong opinion really, is just moving a bus_release_resource() into release_unpopulated_mem().

sys/x86/xen/hvm.c

580–581

For the purposes of the unpopulated memory we simply cannot use pages that are not fully unpopulated, the page must fully be of type SMAP_TYPE_ACPI_ERROR so that we can use it to map random stuff, otherwise we risk removing an existing map.

[royger]

Incorporate feedback.

[ehem_freebsd_m5p.com]

Definitely want this, though still got some nitpicks.

During commit I prefer having this split in 2 commits. One for the MI implementation, one for the MD implementation. On architectures besides x86, Xen is likely to suggest a fair amount of available address space which has nothing to do with e820. For arm64 this is fairly valuable as it gives a strong suggestion for large amounts of completely usable address space.

sys/dev/xen/bus/xenpv.c

69–71

Style item, I suspect these should be tabs, not 4 spaces.

71

I still don't like "Xen unpopulated memory". Perhaps "Xen I/O addresses"? I'm not 100% sure of x86, but for arm64 on Xen 4.17, this ends up with 552MB of address space available for many things besides the grant table.

I'm also unsure about "unpopulated_mem".

100–105

True, though I found it such high value that it very quickly got an implementation. This will likely cover RISC-V, so only PowerPC is currently without one.

213

Alas, developing an easy to fix style issue.

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

42

This line also needs to move to sys/xen/xen-os.h. Though declaring struct rman should be sufficient for the header.