diff --git a/Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp b/Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp index c1fc61fccb..12a3e01d05 100644 --- a/Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp +++ b/Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp @@ -15,9 +15,10 @@ namespace Kernel { -VirtIOGPU3DDevice::PerContextState::PerContextState(Graphics::VirtIOGPU::ContextID context_id, OwnPtr transfer_buffer_region) +VirtIOGPU3DDevice::PerContextState::PerContextState(OpenFileDescription& description, Graphics::VirtIOGPU::ContextID context_id, OwnPtr transfer_buffer_region) : m_context_id(context_id) , m_transfer_buffer_region(move(transfer_buffer_region)) + , m_attached_file_description(description) { } @@ -43,34 +44,45 @@ VirtIOGPU3DDevice::VirtIOGPU3DDevice(VirtIOGraphicsAdapter const& graphics_adapt void VirtIOGPU3DDevice::detach(OpenFileDescription& description) { - m_context_state_lookup.remove(&description); + m_context_state_list.with([&description](auto& list) { + for (auto& context : list) { + if (&context.description() == &description) { + context.m_list_node.remove(); + // NOTE: We break here because there shouldn't be another context + // being attached to this OpenFileDescription. + break; + } + } + }); CharacterDevice::detach(description); } -ErrorOr> VirtIOGPU3DDevice::get_context_for_description(OpenFileDescription& description) -{ - auto res = m_context_state_lookup.get(&description); - if (!res.has_value()) - return EBADF; - return res.value(); -} - ErrorOr VirtIOGPU3DDevice::ioctl(OpenFileDescription& description, unsigned request, Userspace arg) { + + auto get_context_for_description = [](ContextList& list, OpenFileDescription& description) -> ErrorOr> { + for (auto& context : list) { + if (&context.description() == &description) + return context; + } + return EBADF; + }; + // TODO: We really should have ioctls for destroying resources as well switch (request) { case VIRGL_IOCTL_CREATE_CONTEXT: { - if (m_context_state_lookup.contains(&description)) - return EEXIST; - SpinlockLocker locker(m_graphics_adapter->operation_lock()); - // TODO: Delete the context if it fails to be set in m_context_state_lookup - auto context_id = TRY(m_graphics_adapter->create_context()); - LockRefPtr per_context_state = TRY(PerContextState::try_create(context_id)); - TRY(m_context_state_lookup.try_set(&description, per_context_state)); - return {}; + return m_context_state_list.with([get_context_for_description, &description, this](auto& list) -> ErrorOr { + if (auto result = get_context_for_description(list, description); !result.is_error()) + return EEXIST; + SpinlockLocker locker(m_graphics_adapter->operation_lock()); + // TODO: Delete the context if it fails to be set in m_context_state_lookup + auto context_id = TRY(m_graphics_adapter->create_context()); + auto per_context_state = TRY(PerContextState::try_create(description, context_id)); + list.append(per_context_state); + return {}; + }); } case VIRGL_IOCTL_TRANSFER_DATA: { - auto& transfer_buffer_region = TRY(get_context_for_description(description))->transfer_buffer_region(); auto user_transfer_descriptor = static_ptr_cast(arg); auto transfer_descriptor = TRY(copy_typed_from_user(user_transfer_descriptor)); if (Checked::addition_would_overflow(transfer_descriptor.offset_in_region, transfer_descriptor.num_bytes)) { @@ -79,34 +91,40 @@ ErrorOr VirtIOGPU3DDevice::ioctl(OpenFileDescription& description, unsigne if (transfer_descriptor.offset_in_region + transfer_descriptor.num_bytes > NUM_TRANSFER_REGION_PAGES * PAGE_SIZE) { return EOVERFLOW; } - if (transfer_descriptor.direction == VIRGL_DATA_DIR_GUEST_TO_HOST) { - auto target = transfer_buffer_region.vaddr().offset(transfer_descriptor.offset_in_region).as_ptr(); - return copy_from_user(target, transfer_descriptor.data, transfer_descriptor.num_bytes); - } else if (transfer_descriptor.direction == VIRGL_DATA_DIR_HOST_TO_GUEST) { - auto source = transfer_buffer_region.vaddr().offset(transfer_descriptor.offset_in_region).as_ptr(); - return copy_to_user(transfer_descriptor.data, source, transfer_descriptor.num_bytes); - } else { - return EINVAL; - } + + return m_context_state_list.with([get_context_for_description, &description, &transfer_descriptor](auto& list) -> ErrorOr { + auto context = TRY(get_context_for_description(list, description)); + auto& transfer_buffer_region = context->transfer_buffer_region(); + if (transfer_descriptor.direction == VIRGL_DATA_DIR_GUEST_TO_HOST) { + auto target = transfer_buffer_region.vaddr().offset(transfer_descriptor.offset_in_region).as_ptr(); + return copy_from_user(target, transfer_descriptor.data, transfer_descriptor.num_bytes); + } else if (transfer_descriptor.direction == VIRGL_DATA_DIR_HOST_TO_GUEST) { + auto source = transfer_buffer_region.vaddr().offset(transfer_descriptor.offset_in_region).as_ptr(); + return copy_to_user(transfer_descriptor.data, source, transfer_descriptor.num_bytes); + } else { + return EINVAL; + } + }); } case VIRGL_IOCTL_SUBMIT_CMD: { - auto context_id = TRY(get_context_for_description(description))->context_id(); - SpinlockLocker locker(m_graphics_adapter->operation_lock()); auto user_command_buffer = static_ptr_cast(arg); auto command_buffer = TRY(copy_typed_from_user(user_command_buffer)); - TRY(m_graphics_adapter->submit_command_buffer(context_id, [&](Bytes buffer) { - auto num_bytes = command_buffer.num_elems * sizeof(u32); - VERIFY(num_bytes <= buffer.size()); - MUST(copy_from_user(buffer.data(), command_buffer.data, num_bytes)); - return num_bytes; - })); - return {}; + return m_context_state_list.with([get_context_for_description, &description, &command_buffer, this](auto& list) -> ErrorOr { + auto context = TRY(get_context_for_description(list, description)); + auto context_id = context->context_id(); + SpinlockLocker locker(m_graphics_adapter->operation_lock()); + TRY(m_graphics_adapter->submit_command_buffer(context_id, [&](Bytes buffer) { + auto num_bytes = command_buffer.num_elems * sizeof(u32); + VERIFY(num_bytes <= buffer.size()); + MUST(copy_from_user(buffer.data(), command_buffer.data, num_bytes)); + return num_bytes; + })); + return {}; + }); } case VIRGL_IOCTL_CREATE_RESOURCE: { - auto per_context_state = TRY(get_context_for_description(description)); auto user_spec = static_ptr_cast(arg); VirGL3DResourceSpec spec = TRY(copy_typed_from_user(user_spec)); - Graphics::VirtIOGPU::Protocol::Resource3DSpecification const resource_spec = { .target = static_cast(spec.target), .format = spec.format, @@ -120,15 +138,18 @@ ErrorOr VirtIOGPU3DDevice::ioctl(OpenFileDescription& description, unsigne .flags = spec.flags, .padding = 0, }; - SpinlockLocker locker(m_graphics_adapter->operation_lock()); - // FIXME: What would be an appropriate resource free-ing mechanism to use in case anything - // after this fails? - auto resource_id = TRY(m_graphics_adapter->create_3d_resource(resource_spec)); - TRY(m_graphics_adapter->attach_resource_to_context(resource_id, per_context_state->context_id())); - TRY(m_graphics_adapter->ensure_backing_storage(resource_id, per_context_state->transfer_buffer_region(), 0, NUM_TRANSFER_REGION_PAGES * PAGE_SIZE)); - spec.created_resource_id = resource_id.value(); - // FIXME: We should delete the resource we just created if we fail to copy the resource id out - return copy_to_user(static_ptr_cast(arg), &spec); + return m_context_state_list.with([get_context_for_description, &description, &resource_spec, &spec, &arg, this](auto& list) -> ErrorOr { + auto context = TRY(get_context_for_description(list, description)); + SpinlockLocker locker(m_graphics_adapter->operation_lock()); + // FIXME: What would be an appropriate resource free-ing mechanism to use in case anything + // after this fails? + auto resource_id = TRY(m_graphics_adapter->create_3d_resource(resource_spec)); + TRY(m_graphics_adapter->attach_resource_to_context(resource_id, context->context_id())); + TRY(m_graphics_adapter->ensure_backing_storage(resource_id, context->transfer_buffer_region(), 0, NUM_TRANSFER_REGION_PAGES * PAGE_SIZE)); + spec.created_resource_id = resource_id.value(); + // FIXME: We should delete the resource we just created if we fail to copy the resource id out + return copy_to_user(static_ptr_cast(arg), &spec); + }); } } return EINVAL; diff --git a/Kernel/Graphics/VirtIOGPU/GPU3DDevice.h b/Kernel/Graphics/VirtIOGPU/GPU3DDevice.h index 15b8613154..168896709b 100644 --- a/Kernel/Graphics/VirtIOGPU/GPU3DDevice.h +++ b/Kernel/Graphics/VirtIOGPU/GPU3DDevice.h @@ -7,9 +7,11 @@ #pragma once #include +#include #include #include #include +#include namespace Kernel::Graphics::VirtIOGPU { @@ -101,24 +103,35 @@ private: VirtIOGPU3DDevice(VirtIOGraphicsAdapter const& graphics_adapter, NonnullOwnPtr transfer_buffer_region, Graphics::VirtIOGPU::ContextID kernel_context_id); class PerContextState final : public AtomicRefCounted { + friend class VirtIOGPU3DDevice; + public: - static ErrorOr> try_create(Graphics::VirtIOGPU::ContextID context_id) + static ErrorOr> try_create(OpenFileDescription& description, Graphics::VirtIOGPU::ContextID context_id) { auto region_result = TRY(MM.allocate_kernel_region( NUM_TRANSFER_REGION_PAGES * PAGE_SIZE, "VIRGL3D userspace upload buffer"sv, Memory::Region::Access::ReadWrite, AllocationStrategy::AllocateNow)); - return TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) PerContextState(context_id, move(region_result)))); + return TRY(adopt_nonnull_ref_or_enomem(new (nothrow) PerContextState(description, context_id, move(region_result)))); } Graphics::VirtIOGPU::ContextID context_id() { return m_context_id; } Memory::Region& transfer_buffer_region() { return *m_transfer_buffer_region; } + OpenFileDescription& description() { return m_attached_file_description; } + private: PerContextState() = delete; - explicit PerContextState(Graphics::VirtIOGPU::ContextID context_id, OwnPtr transfer_buffer_region); + PerContextState(OpenFileDescription&, Graphics::VirtIOGPU::ContextID context_id, OwnPtr transfer_buffer_region); Graphics::VirtIOGPU::ContextID m_context_id; OwnPtr m_transfer_buffer_region; + + // NOTE: We clean this whole object when the file description is closed, therefore we need to hold + // a raw reference here instead of a strong reference pointer (e.g. RefPtr, which will make it + // possible to leak the attached OpenFileDescription for a context in this device). + OpenFileDescription& m_attached_file_description; + + IntrusiveListNode> m_list_node; }; virtual bool can_read(OpenFileDescription const&, u64) const override { return true; } @@ -130,13 +143,13 @@ private: virtual ErrorOr ioctl(OpenFileDescription&, unsigned request, Userspace arg) override; virtual void detach(OpenFileDescription&) override; -private: - ErrorOr> get_context_for_description(OpenFileDescription&); + using ContextList = IntrusiveListRelaxedConst<&PerContextState::m_list_node>; +private: NonnullLockRefPtr m_graphics_adapter; // Context used for kernel operations (e.g. flushing resources to scanout) Graphics::VirtIOGPU::ContextID m_kernel_context_id; - HashMap> m_context_state_lookup; + SpinlockProtected m_context_state_list; // Memory management for backing buffers NonnullOwnPtr m_transfer_buffer_region; constexpr static size_t NUM_TRANSFER_REGION_PAGES = 1024;