host: make system blob memory work on macOS - #182
Conversation
|
Friendly ping @gurchetansingh @jmacnak for reviewing. |
eba0d61 to
2b5933a
Compare
|
@jmacnak knows this code well, would be the right person to review |
293c959 to
7129900
Compare
jmacnak
left a comment
There was a problem hiding this comment.
One thread about readability but overall logic LGTM
| // so that the host can control its memory properties. This ensures that the guest | ||
| // only sees `VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT` and will not try to map the | ||
| // memory. | ||
| if (features.VulkanUseDedicatedAhbMemoryType.enabled()) { |
There was a problem hiding this comment.
Now that we are inserting 2 emulated guest memory types and the indicies and offsets are getting a little more complicated to read and reason about, I wonder if we could replace the index mappings with something a bit more readable.
struct EmulatedGuestMemoryType {
uint32_t hostMemoryTypeIndex = kInvalidMemoryTypeIndex;
// This memory type is reserved for controlling the
// memory properties of AHBs with the
// VulkanUseDedicatedAhbMemoryType feature.
bool isReservedForAhbAllocations = false;
// This memory type is reserved for optimally tiled
// images on Apple when used with system blobs where
// the host must not try to
bool isReservedForAppleSystemBlobAllocations = false;
// The memory properties reported to the guest.
VkMemoryType memoryType;
};
std::vector<EmulatedGuestMemoryType> mGuestMemoryTypes;
struct HostMemoryType {
// The corresponding guest memory index.
uint32_t guestMemoryTypeIndex = kInvalidMemoryTypeIndex;
// The unaltered host memory properties.
VkMemoryType memoryType;
}
std::vector<HostMemoryType> mHostMemoryTypes;
Then, inside EmulatedPhysicalDeviceMemoryProperties's constructor, we have something like
mHostMemoryProperties = hostMemoryProperties;
// Start with the original host properties
mGuestMemoryTypes.reserve(hostMemoryProperties. memoryTypeCount);
for ( <each host memory type> ) {
mGuestMemoryTypes.push_back(EmulatedGuestMemoryType{
.hostMemoryTypeIndex = index,
.memoryType = hostMemoryProperties.memoryType[index]
};
}
<apply transformations on mGuestMemoryTypes>
<insert a EmulatedGuestMemoryType for apple if needed>
<insert a EmulatedGuestMemoryType for AHBs if needed>
where we are just working with objects and we can use
mGuestMemoryTypes.insert(mGuestMemoryTypes.begin() + <desired index>);
with a helper function that is self documenting:
uint32_t FindIndexForNewMemoryType(const std::vector<EmulatedGuestMemoryType>& memoryTypes) {
// Per https://docs.vulkan.org/refpages/latest/refpages/source/VkPhysicalDeviceMemoryProperties.html,
// memory types must be ordered so that earlier memory
// types have a strict subset of flags of later types.
...
}
The very end of the constructor can finally generate the flattened mGuestMemoryProperties.
WDYT?
There was a problem hiding this comment.
Done. Guest memory types are now a vector of EmulatedGuestMemoryType. The Apple type is inserted with findIndexForNewMemoryType() and the vector is flattened into mGuestMemoryProperties afterwards.
3fb83c3 to
72e8d04
Compare
vkGetDeviceImageMemoryRequirements answers with the host's memory type bits unless the format is one gfxstream decompresses. The query that names a created image translates them first. Nothing notices while a guest memory type index is also a host one, which holds until a type is emulated. With one, a guest is told an image fits memory types it does not have, and asking the same question the other way answers something else. Translate on the way out on every path.
A system blob's shared memory is handed to the driver as a host pointer, and a driver states the alignment it takes in minImportedHostPointerAlignment. The size is rounded to 4096 instead, which is the page size the guest is assumed to have rather than anything the importing driver asked for. On Apple silicon a page is 16KB and kosmickrisp asks for that, so an allocation whose size is a multiple of 4096 but not of 16KB is imported against the driver's stated requirement. Round up to whichever is larger. Where the two agree, which is everywhere a page is 4096, nothing changes.
System blob memory is named shared-memory-vk-N with a per process counter and opened without O_EXCL. A server that is killed leaves its objects behind, the next one reuses the names, and on macOS an object that already has a size cannot be resized: ftruncate fails with EINVAL and vkAllocateMemory fails. Put the pid in the name, and on Apple unlink the object as soon as it exists; the descriptor keeps it alive and is what the guest is handed.
With system blobs, host visible memory is shared memory imported as a host pointer, and Metal cannot bind a tiled image to that. A device that reports one unified memory type, as kosmickrisp does, then has nowhere to put an image, and kk_image_plane_bind asserts on the first one. Add a guest only type in that case: device local, allocating from the same host type without host visible emulation. A tiled image is offered that type alone, since the host visible one cannot hold it; a linear image may still use either. MoltenVK reports a device local only type of its own, so nothing changes there. Guest memory types are now kept as a list of structs, each naming its host type and what it is reserved for, instead of index maps in both directions. The new type is inserted where the spec's ordering puts it: a type whose flags are a strict subset of another's sits at a lower index, which on a unified memory device is the front. The AHB type is still appended, as before. Metal is what makes this the common case, so it is behind __APPLE__.
72e8d04 to
f0f5808
Compare
Two host fixes for running the guest Vulkan driver through kumquat on macOS.
Both only take effect with SystemBlob enabled.
vulkan: add a guest only device local memory type when the host reports a
single host visible unified type. Metal cannot bind a tiled image to an
imported host pointer; this gives images ordinary device memory. Unit test
added.
base: put the pid in the shared memory name and unlink it on creation, so a
killed server does not leave objects that make the next ftruncate fail.
Tested with bazel test //host/vulkan:gfxstream_emulatedphysicalmemory_tests,
and vulkaninfo, vkcube and a dEQP-VK smoke run on kosmickrisp and MoltenVK.