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

@@ -174,8 +174,7 @@ int Platform_thread::start(void * const ip, void * const sp)
 		return -1;
 	}
 
-	unsigned const cpu =
+	unsigned const cpu = _location.xpos();
-		_location.valid() ? _location.xpos() : 0;
 
 	Native_utcb &utcb = *Thread::myself()->utcb();
 

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, ",

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());
 }

repos/base-nova/src/core/thread_start.cc

@@ -93,14 +93,9 @@ void Thread::start()
 	addr_t sp   = _stack->top();
 	Utcb  &utcb = *reinterpret_cast<Utcb *>(&_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);

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("width",  default_width),
-					                node.attribute_value("height", 0U));
+					                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
 		 */

repos/base/include/base/child.h

@@ -579,9 +579,12 @@ class Genode::Child : protected Rpc_object<Parent>,
 						                                       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()),
+					                      _args, affinity, route.label, route.diag);
-					                      route.label, route.diag);
 				}
 				catch (Service_denied) {
 					error(_child._policy.name(), ": ", _service_name(), " "

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),

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();
 

repos/gems/src/app/depot_deploy/child.h

@@ -305,8 +305,10 @@ class Depot_deploy::Child : public List_model<Child>::Element
 		 *                            the content of the depot user "local", which
 		 *                            is assumed to be mutable
 		 */
-		inline void gen_start_node(Xml_generator &, Xml_node common,
+		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<Child>::Element
 };
 
 
-void Depot_deploy::Child::gen_start_node(Xml_generator &xml, Xml_node common,
+void Depot_deploy::Child::gen_start_node(Xml_generator          &xml,
-                                         Prio_levels prio_levels,
+                                         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());

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,
+				child.gen_start_node(xml, common, prio_levels, affinity_space,
-				                     uncached_depot_rom); });
+				                     cached_depot_rom, uncached_depot_rom); });
 		}
 
 		void gen_queries(Xml_generator &xml) const

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 */

repos/gems/src/app/sculpt_manager/deploy.cc

@@ -149,7 +149,8 @@ void Sculpt::Deploy::handle_deploy()
 
 
 void Sculpt::Deploy::gen_runtime_start_nodes(Xml_generator  &xml,
-                                             Prio_levels prio_levels) const
+                                             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");
 }

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<Deploy> _managed_deploy_handler {
 		_env.ep(), *this, &Deploy::_handle_managed_deploy };

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);
 	}
 }
 

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 */
+		Location result = Location(0, 0, space.width(), space.height());
-			unsigned long const
-				default_width  = node.has_attribute("xpos") ? 1 : space.width(),
-				default_height = node.has_attribute("ypos") ? 1 : space.height();
 
-			unsigned long const
+		start_node.with_sub_node("affinity", [&] (Xml_node node) {
-				width  = node.attribute_value<unsigned long>("width",  default_width),
-				height = node.attribute_value<unsigned long>("height", default_height);
 
-			int const x1 = (int)node.attribute_value<long>("xpos", 0),
+			Location const location = Location::from_xml(space, node);
-			          y1 = (int)node.attribute_value<long>("ypos", 0),
-			          x2 = (int)(x1 + width  - 1),
-			          y2 = (int)(y1 + height - 1);
 
-			/* clip location to space boundary */
+			if (!location.within(space)) {
-			return Location(max(x1, 0), max(y1, 0),
+				Service::Name const name = start_node.attribute_value("name", Service::Name());
-			                min((unsigned)(x2 - x1 + 1), space.width()),
+				warning(name, ": affinity location exceeds affinity-space boundary");
-			                min((unsigned)(y2 - y1 + 1), space.height()));
+				return;
 			}
-		catch (...) { return Location(0, 0, space.width(), space.height()); }
+
+			result = location;
+		});
+
+		return result;
 	}
 }