From 3cc6df311648df544b6b7edf366f296430446868 Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Tue, 14 Dec 2021 15:15:40 +0100 Subject: [PATCH] base: tighten affinity handling This patch improves the robustness of the CPU-affinity handling. - The types in base/affinity.h received the accessors 'Location::within(space)' and 'Affinity::valid', which alleviates the fiddling with coordinates when sanity checking the values, in init or core. - The 'Affinity::Location::valid' method got removed because its meaning was too vague. For sanity checks of affinity configurations, the new 'within' method is approriate. In cases where only the x,y values are used for selecting a physical CPU (during thread creation), the validity check (width*height > 0) was not meaningful anyway. - The 'Affinity::Location::from_xml' requires a 'Affinity::Space' as argument because a location always relates to the bounds of a specific space. This function now implements the selection of whole rows or columns, which has previously a feature of the sandbox library only. - Whenever the sandbox library (init) encounters an invalid affinity configuration, it prints a warning message as a diagnostic aid. - A new 'Affinity::unrestricted' function constructs an affinity that covers the whole affinity space. The named functions clarifies the meaning over the previous use of the default constructor. - Core's CPU service denies session requests with an invalid affinity parameter. Previously, it would fall back to an unrestricted affinity. Issue #4300 --- repos/base-hw/src/core/platform_thread.cc | 3 +- .../base-hw/src/core/vm_session_component.cc | 2 +- repos/base-nova/src/core/platform.cc | 2 -- repos/base-nova/src/core/thread_start.cc | 7 +--- repos/base/include/base/affinity.h | 34 +++++++++++++++---- repos/base/include/base/child.h | 7 ++-- repos/base/src/core/include/cpu_root.h | 3 ++ repos/base/src/core/main.cc | 2 +- repos/gems/src/app/depot_deploy/child.h | 16 +++++---- repos/gems/src/app/depot_deploy/children.h | 5 +-- repos/gems/src/app/depot_deploy/main.cc | 3 +- repos/gems/src/app/sculpt_manager/deploy.cc | 8 +++-- repos/gems/src/app/sculpt_manager/deploy.h | 2 +- repos/gems/src/app/sculpt_manager/main.cc | 2 +- repos/os/src/lib/sandbox/utils.h | 32 +++++++---------- 15 files changed, 75 insertions(+), 53 deletions(-) diff --git a/repos/base-hw/src/core/platform_thread.cc b/repos/base-hw/src/core/platform_thread.cc index 8235f64152..fac3d8551c 100644 --- a/repos/base-hw/src/core/platform_thread.cc +++ b/repos/base-hw/src/core/platform_thread.cc @@ -174,8 +174,7 @@ int Platform_thread::start(void * const ip, void * const sp) return -1; } - unsigned const cpu = - _location.valid() ? _location.xpos() : 0; + unsigned const cpu = _location.xpos(); Native_utcb &utcb = *Thread::myself()->utcb(); diff --git a/repos/base-hw/src/core/vm_session_component.cc b/repos/base-hw/src/core/vm_session_component.cc index aaf9bc2ba8..22449aee6a 100644 --- a/repos/base-hw/src/core/vm_session_component.cc +++ b/repos/base-hw/src/core/vm_session_component.cc @@ -40,7 +40,7 @@ void Vm_session_component::Vcpu::exception_handler(Signal_context_capability han return; } - unsigned const cpu = location.valid() ? location.xpos() : 0; + unsigned const cpu = location.xpos(); if (!kobj.create(cpu, ds_addr, Capability_space::capid(handler), id)) Genode::warning("Cannot instantiate vm kernel object, ", diff --git a/repos/base-nova/src/core/platform.cc b/repos/base-nova/src/core/platform.cc index beaef3b57d..6b8354e2fd 100644 --- a/repos/base-nova/src/core/platform.cc +++ b/repos/base-nova/src/core/platform.cc @@ -977,8 +977,6 @@ unsigned Platform::kernel_cpu_id(Affinity::Location location) const unsigned Platform::pager_index(Affinity::Location location) const { - if (!location.valid()) return 0; - return (location.xpos() * _cpus.height() + location.ypos()) % (_cpus.width() * _cpus.height()); } diff --git a/repos/base-nova/src/core/thread_start.cc b/repos/base-nova/src/core/thread_start.cc index f97bc2a8db..7dc4024bbf 100644 --- a/repos/base-nova/src/core/thread_start.cc +++ b/repos/base-nova/src/core/thread_start.cc @@ -93,14 +93,9 @@ void Thread::start() addr_t sp = _stack->top(); Utcb &utcb = *reinterpret_cast(&_stack->utcb()); - Affinity::Location location = _affinity; - - if (!location.valid()) - location = Affinity::Location((int)boot_cpu(), 0); - /* create local EC */ enum { LOCAL_THREAD = false }; - unsigned const kernel_cpu_id = platform_specific().kernel_cpu_id(location); + unsigned const kernel_cpu_id = platform_specific().kernel_cpu_id(_affinity); uint8_t res = create_ec(native_thread().ec_sel, platform_specific().core_pd_sel(), kernel_cpu_id, (mword_t)&utcb, sp, native_thread().exc_pt_sel, LOCAL_THREAD); diff --git a/repos/base/include/base/affinity.h b/repos/base/include/base/affinity.h index 36eeba1af6..81d56f2f40 100644 --- a/repos/base/include/base/affinity.h +++ b/repos/base/include/base/affinity.h @@ -116,7 +116,7 @@ class Genode::Affinity /** * Constructor to express the affinity to a single CPU */ - Location(int xpos, unsigned ypos) + Location(int xpos, int ypos) : _xpos(xpos), _ypos(ypos), _width(1), _height(1) { } /** @@ -129,7 +129,6 @@ class Genode::Affinity int ypos() const { return _ypos; } unsigned width() const { return _width; } unsigned height() const { return _height; } - bool valid() const { return _width*_height > 0; } Location multiply_position(Space const &space) const { @@ -142,12 +141,29 @@ class Genode::Affinity return Location(_xpos + dx, _ypos + dy, _width, _height); } - static Location from_xml(Xml_node const &node) + /** + * Return true if the location resides within 'space' + */ + bool within(Space const &space) const { + int const x1 = _xpos, x2 = _xpos + _width - 1, + y1 = _ypos, y2 = _ypos + _height - 1; + + return x1 >= 0 && x1 <= x2 && (unsigned)x2 < space.width() + && y1 >= 0 && y1 <= y2 && (unsigned)y2 < space.height(); + } + + static Location from_xml(Space const &space, Xml_node const &node) + { + /* if no position value is specified, select the whole row/column */ + unsigned const + default_width = node.has_attribute("xpos") ? 1 : space.width(), + default_height = node.has_attribute("ypos") ? 1 : space.height(); + return Location(node.attribute_value("xpos", 0U), node.attribute_value("ypos", 0U), - node.attribute_value("width", 0U), - node.attribute_value("height", 0U)); + node.attribute_value("width", default_width), + node.attribute_value("height", default_height)); } }; @@ -166,6 +182,7 @@ class Genode::Affinity Space space() const { return _space; } Location location() const { return _location; } + bool valid() const { return _location.within(_space); } static Affinity from_xml(Xml_node const &node) { @@ -176,12 +193,17 @@ class Genode::Affinity node.with_sub_node("space", [&] (Xml_node const &node) { space = Space::from_xml(node); }); node.with_sub_node("location", [&] (Xml_node const &node) { - location = Location::from_xml(node); }); + location = Location::from_xml(space, node); }); }); return Affinity(space, location); } + static Affinity unrestricted() + { + return Affinity(Space(1, 1), Location(0, 0, 1, 1)); + } + /** * Return location scaled to specified affinity space */ diff --git a/repos/base/include/base/child.h b/repos/base/include/base/child.h index cfcf8e7844..d38a3d75bd 100644 --- a/repos/base/include/base/child.h +++ b/repos/base/include/base/child.h @@ -579,9 +579,12 @@ class Genode::Child : protected Rpc_object, label_from_args(_args.string()), session_diag_from_args(_args.string())); _env_service.construct(_child, route.service); + + Affinity const affinity = + _child._policy.filter_session_affinity(Affinity::unrestricted()); + _connection.construct(*_env_service, _child._id_space, _client_id, - _args, _child._policy.filter_session_affinity(Affinity()), - route.label, route.diag); + _args, affinity, route.label, route.diag); } catch (Service_denied) { error(_child._policy.name(), ": ", _service_name(), " " diff --git a/repos/base/src/core/include/cpu_root.h b/repos/base/src/core/include/cpu_root.h index 56e274617a..63241398af 100644 --- a/repos/base/src/core/include/cpu_root.h +++ b/repos/base/src/core/include/cpu_root.h @@ -43,6 +43,9 @@ namespace Genode { if (ram_quota < Trace::Control_area::SIZE) throw Insufficient_ram_quota(); + if (!affinity.valid()) + throw Service_denied(); + return new (md_alloc()) Cpu_session_component(*this->ep(), session_resources_from_args(args), diff --git a/repos/base/src/core/main.cc b/repos/base/src/core/main.cc index 234bc455f6..5455baac83 100644 --- a/repos/base/src/core/main.cc +++ b/repos/base/src/core/main.cc @@ -306,7 +306,7 @@ int main() {Cpu_session::CAP_QUOTA}}, "core", Session::Diag{false}, core_ram_alloc, local_rm, ep, pager_ep, Trace::sources(), "", - Affinity(), Cpu_session::QUOTA_LIMIT); + Affinity::unrestricted(), Cpu_session::QUOTA_LIMIT); Cpu_session_capability core_cpu_cap = core_cpu.cap(); diff --git a/repos/gems/src/app/depot_deploy/child.h b/repos/gems/src/app/depot_deploy/child.h index eb13eaec0f..a4da7214cf 100644 --- a/repos/gems/src/app/depot_deploy/child.h +++ b/repos/gems/src/app/depot_deploy/child.h @@ -305,8 +305,10 @@ class Depot_deploy::Child : public List_model::Element * the content of the depot user "local", which * is assumed to be mutable */ - inline void gen_start_node(Xml_generator &, Xml_node common, - Prio_levels prio_levels, + inline void gen_start_node(Xml_generator &, + Xml_node const &common, + Prio_levels prio_levels, + Affinity::Space affinity_space, Depot_rom_server const &cached_depot_rom, Depot_rom_server const &uncached_depot_rom) const; @@ -327,8 +329,10 @@ class Depot_deploy::Child : public List_model::Element }; -void Depot_deploy::Child::gen_start_node(Xml_generator &xml, Xml_node common, - Prio_levels prio_levels, +void Depot_deploy::Child::gen_start_node(Xml_generator &xml, + Xml_node const &common, + Prio_levels const prio_levels, + Affinity::Space const affinity_space, Depot_rom_server const &cached_depot_rom, Depot_rom_server const &uncached_depot_rom) const { @@ -425,11 +429,11 @@ void Depot_deploy::Child::gen_start_node(Xml_generator &xml, Xml_node common, if (affinity_from_launcher) launcher_xml.with_sub_node("affinity", [&] (Xml_node node) { - location = Affinity::Location::from_xml(node); }); + location = Affinity::Location::from_xml(affinity_space, node); }); if (affinity_from_start) start_xml.with_sub_node("affinity", [&] (Xml_node node) { - location = Affinity::Location::from_xml(node); }); + location = Affinity::Location::from_xml(affinity_space, node); }); xml.node("affinity", [&] () { xml.attribute("xpos", location.xpos()); diff --git a/repos/gems/src/app/depot_deploy/children.h b/repos/gems/src/app/depot_deploy/children.h index 971f02d3b0..69038ca958 100644 --- a/repos/gems/src/app/depot_deploy/children.h +++ b/repos/gems/src/app/depot_deploy/children.h @@ -115,12 +115,13 @@ class Depot_deploy::Children void gen_start_nodes(Xml_generator &xml, Xml_node common, Child::Prio_levels prio_levels, + Affinity::Space affinity_space, Child::Depot_rom_server const &cached_depot_rom, Child::Depot_rom_server const &uncached_depot_rom) const { _children.for_each([&] (Child const &child) { - child.gen_start_node(xml, common, prio_levels, cached_depot_rom, - uncached_depot_rom); }); + child.gen_start_node(xml, common, prio_levels, affinity_space, + cached_depot_rom, uncached_depot_rom); }); } void gen_queries(Xml_generator &xml) const diff --git a/repos/gems/src/app/depot_deploy/main.cc b/repos/gems/src/app/depot_deploy/main.cc index 21cfdfa4d3..65f51aa970 100644 --- a/repos/gems/src/app/depot_deploy/main.cc +++ b/repos/gems/src/app/depot_deploy/main.cc @@ -129,7 +129,8 @@ struct Depot_deploy::Main Child::Depot_rom_server const parent { }; _children.gen_start_nodes(xml, config.sub_node("common_routes"), - prio_levels, parent, parent); + prio_levels, Affinity::Space(1, 1), + parent, parent); }); /* update query for blueprints of all unconfigured start nodes */ diff --git a/repos/gems/src/app/sculpt_manager/deploy.cc b/repos/gems/src/app/sculpt_manager/deploy.cc index 569ba2356f..93464dfa20 100644 --- a/repos/gems/src/app/sculpt_manager/deploy.cc +++ b/repos/gems/src/app/sculpt_manager/deploy.cc @@ -148,8 +148,9 @@ void Sculpt::Deploy::handle_deploy() } -void Sculpt::Deploy::gen_runtime_start_nodes(Xml_generator &xml, - Prio_levels prio_levels) const +void Sculpt::Deploy::gen_runtime_start_nodes(Xml_generator &xml, + Prio_levels prio_levels, + Affinity::Space affinity_space) const { /* depot-ROM instance for regular (immutable) depot content */ xml.node("start", [&] () { @@ -176,5 +177,6 @@ void Sculpt::Deploy::gen_runtime_start_nodes(Xml_generator &xml, /* generate start nodes for deployed packages */ if (managed_deploy.has_sub_node("common_routes")) _children.gen_start_nodes(xml, managed_deploy.sub_node("common_routes"), - prio_levels, "depot_rom", "dynamic_depot_rom"); + prio_levels, affinity_space, + "depot_rom", "dynamic_depot_rom"); } diff --git a/repos/gems/src/app/sculpt_manager/deploy.h b/repos/gems/src/app/sculpt_manager/deploy.h index 0c7c760984..9670454c38 100644 --- a/repos/gems/src/app/sculpt_manager/deploy.h +++ b/repos/gems/src/app/sculpt_manager/deploy.h @@ -232,7 +232,7 @@ struct Sculpt::Deploy void gen_child_diagnostics(Xml_generator &xml) const; - void gen_runtime_start_nodes(Xml_generator &, Prio_levels) const; + void gen_runtime_start_nodes(Xml_generator &, Prio_levels, Affinity::Space) const; Signal_handler _managed_deploy_handler { _env.ep(), *this, &Deploy::_handle_managed_deploy }; diff --git a/repos/gems/src/app/sculpt_manager/main.cc b/repos/gems/src/app/sculpt_manager/main.cc index e5399a69cb..f82c3e2fd6 100644 --- a/repos/gems/src/app/sculpt_manager/main.cc +++ b/repos/gems/src/app/sculpt_manager/main.cc @@ -1800,7 +1800,7 @@ void Sculpt::Main::_generate_runtime_config(Xml_generator &xml) const xml.node("start", [&] () { gen_launcher_query_start_content(xml); }); - _deploy.gen_runtime_start_nodes(xml, _prio_levels); + _deploy.gen_runtime_start_nodes(xml, _prio_levels, _affinity_space); } } diff --git a/repos/os/src/lib/sandbox/utils.h b/repos/os/src/lib/sandbox/utils.h index 31a27ddf10..7db3b3a2c9 100644 --- a/repos/os/src/lib/sandbox/utils.h +++ b/repos/os/src/lib/sandbox/utils.h @@ -210,29 +210,23 @@ namespace Sandbox { affinity_location_from_xml(Affinity::Space const &space, Xml_node start_node) { typedef Affinity::Location Location; - try { - Xml_node node = start_node.sub_node("affinity"); - /* if no position value is specified, select the whole row/column */ - unsigned long const - default_width = node.has_attribute("xpos") ? 1 : space.width(), - default_height = node.has_attribute("ypos") ? 1 : space.height(); + Location result = Location(0, 0, space.width(), space.height()); - unsigned long const - width = node.attribute_value("width", default_width), - height = node.attribute_value("height", default_height); + start_node.with_sub_node("affinity", [&] (Xml_node node) { - int const x1 = (int)node.attribute_value("xpos", 0), - y1 = (int)node.attribute_value("ypos", 0), - x2 = (int)(x1 + width - 1), - y2 = (int)(y1 + height - 1); + Location const location = Location::from_xml(space, node); - /* clip location to space boundary */ - return Location(max(x1, 0), max(y1, 0), - min((unsigned)(x2 - x1 + 1), space.width()), - min((unsigned)(y2 - y1 + 1), space.height())); - } - catch (...) { return Location(0, 0, space.width(), space.height()); } + if (!location.within(space)) { + Service::Name const name = start_node.attribute_value("name", Service::Name()); + warning(name, ": affinity location exceeds affinity-space boundary"); + return; + } + + result = location; + }); + + return result; } }