Page MenuHomeFreeBSD

xen/xenpv: remove low memory limit for non-x86
ClosedPublic

Authored by ehem_freebsd_m5p.com on Mar 17 2021, 12:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 7:02 PM
Unknown Object (File)
Sun, Nov 24, 10:49 PM
Unknown Object (File)
Sun, Nov 24, 8:31 PM
Unknown Object (File)
Fri, Nov 22, 5:20 AM
Unknown Object (File)
Thu, Nov 21, 8:50 AM
Unknown Object (File)
Tue, Nov 12, 6:05 PM
Unknown Object (File)
Tue, Nov 12, 6:02 PM
Unknown Object (File)
Tue, Nov 12, 5:58 PM

Details

Summary

For embedded devices reserved addresses will be known in advance. More
recently added devices will also likely be correctly updated. As a
result using any available address is reasonable on non-x86.

Note, page zero is reserved and triggers special behavior by
nexus_alloc_resource(). As such PAGE_SIZE must be used instead of zero.

Diff Detail

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

Event Timeline

sys/dev/xen/bus/xenpv.c
64

I think plain 0 is just fine here, no need to define it in hex or set the type suffix.

You should expand the comment above with something along the lines of:

"Other architectures are fine allocate unpopulated memory from any available regions."

Or something similar. I would still consider that you might want to only set this for aarch64 and arm32, as I have no idea what other architectures will require.

sys/dev/xen/bus/xenpv.c
64

I was thinking include the prefix and suffix to make it look similar to the other two lines, though this may be overkill.

I disagree with only setting for arm/aarch64. Other architectures (RISC-V since that is the one on the horizon) are likely to be in situations similar to arm/aarch64 where drivers take better care in correctly reserving ranges. If an architecture gets popular enough to need such a hack, then it should be added for that architecture.

sys/dev/xen/bus/xenpv.c
64

I can confirm that this case will also be applicable to RISC-V.

Add commentary, hopefully better now.

This revision is now accepted and ready to land.Apr 6 2021, 9:33 AM

My, my; look what I found. Seems nexus_alloc_resource() has
interesting behavior if you give it the full address space.
Changing like this causes bus_alloc_resource() to stop returning NULL on
ARM.

This revision now requires review to proceed.Apr 15 2021, 5:29 AM
This revision is now accepted and ready to land.Apr 15 2021, 5:55 AM

Unfortunately D29818 would subsume this by providing further functionality. I'll be testing to confirm that functionality is useful once the build is complete (I'll first examine potential for further instrumentation during the test).

A combination of D29602 and D29818 moves this elsewhere. An additional commit on top of D29818 managed to reach a shell on FreeBSD/Xen/ARM. As such, abandon D29304 as the approach has changed.

This approach to the problem is still potentially viable and still qualifies as an alternative to D29818. Problem is there is some sort of bug causing this not to work and this needs diagnosis before being committed. I suspect D29694 might have fixed the bug, but this requires a build and test to check current status.

This revision is now accepted and ready to land.May 15 2021, 1:59 AM

The change planned this isn't ready for commit until it passes tests. I'm pretty sure the problem is elsewhere, but until that is resolved this isn't ready.

Looking at sys/arm64/arm64/nexus.c I see rather major issues and I'm inclined to think the merits of the approach used for x86 is right whereas what is present for aarch64 is wrong.

Depending on generation, ARM cores may have several physical address sizes. For arm-v8 there are 48-bit physical addresses and for v8.2 52-bit physical addresses may be available. As such initializing mem_rman with a constant range doesn't work. In fact for something approximating a GENERIC kernel BUS_SPACE_MAXADDR would need to be variable, not a constant. A device-specific kernel could use a constant, but this means devices need to override BUS_SPACE_MAXADDR.

Then there is the issue of address range(s) backed by actual memory. If you can avoid it generally you don't want to have DMA overlapping actual physical memory cells. Due to bus/electrical/device/design limitations this may be unavoidable, but you do want to avoid it if possible. As such I'm initially inclined to think x86's approach of removing physical memory from its mem_rman equivalent makes sense and the arm64 approach of having it cover all memory doesn't make sense.

The DMA-like operation of VMs means the Xen code needs to allocate unused address space which is not backed by actual memory cells (actually it is, but the memory cells belong to a foreign VM). As such we need to know about addresses which can be validly addressed, but are invalid in the normal memory map.

This is an interesting situation.

Now to get this out of "Changes Planned" since changes elsewhere reduce the concerns I had for a bit with this (accept my own change? @imp had already marked acceptable).

My one concern is this doesn't feel like the right place for code featuring a construct like this. I would suggest nexus_alloc_resource() should really handle the general case as part of the RMAN_IS_DEFAULT_RANGE() section.

This revision is now accepted and ready to land.May 30 2021, 4:47 PM

One note, seems the issue I was running into was elsewhere. The value of 0 works fine with that issue addressed.

This revision was automatically updated to reflect the committed changes.