protocol: Rework publication of events over DBus transport

A set of races was discovered around the propagation of HIOMAP protocol
BMC status events during BMC shutdown. In particular the change impacts
the design of the DBus transport defined in the protocol specification,
as signalling of both acknowledgeable and non-acknowledgeable events
could not be made atomic.

A particular case where this matters is when the daemon is terminated,
at which point it should simultaneously clear BMC_EVENT_DAEMON_READY and
set BMC_EVENT_PROTOCOL_RESET. The DBus interface as designed required
this be done as two separate messages, which lead to races propagating
the complete state update to the host during shutdown of ipmid.

Change-Id: Iaf38f77c28b8e4e4dd092b0de97dc7e777bfac65
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Documentation/protocol.md

@@ -375,8 +375,7 @@ The wire command framing is as follows:
 DBus defines its own wire-format for messages, and so support for this
 transport concentrates on mapping commands and events onto DBus concepts.
 Specifically, commands are represented by DBus methods and events are
-represented by properties for level-triggered (persistent) events and signals
-for edge-triggered (acknowledgeable) events. DBus will automatically generate a
+represented by properties. DBus will automatically generate a
 `PropertiesChanged` signal for changes to properties allowing consumers to
 learn of updates as they happen.
 
@@ -1397,8 +1396,11 @@ dirty or erased then this command must fail.
 The M, I, and D columns represent the Mailbox, IPMI and DBus transports
 respectively. The values in the M, I or D columns represent the events' bit
 index in the status byte, or in the case of the DBus transport the name of the
-relevant property or signal.
+relevant property.
 
+For the DBus interface, properties are used for all event types regardless of
+whether they should be acknowledged or not as part of the protocol. This
+ensures that state changes can be published atomically.
 
 ## `PROTOCOL_RESET` Event
 

mboxd.c

@@ -193,13 +193,8 @@ static int poll_loop(struct mbox_context *context)
 
 	/* Best to reset windows and the lpc mapping for safety */
 	/* Host didn't request reset -> Notify it */
-	if (windows_reset_all(context)) {
-	       rc = protocol_events_set(context, BMC_EVENT_WINDOW_RESET);
-	       if (rc < 0) {
-		      MSG_ERR("Failed to notify host of reset, expect host-side corruption\n");
-		      return rc;
-	       }
-	}
+	windows_reset_all(context);
+
 	rc = lpc_reset(context);
 	/* Not much we can do if this fails */
 	if (rc < 0) {

transport_dbus.c

@@ -17,7 +17,7 @@ static int transport_dbus_property_update(struct mbox_context *context,
 					  uint8_t events)
 {
 	/* Two properties plus a terminating NULL */
-	char *props[3] = { 0 };
+	char *props[5] = { 0 };
 	int i = 0;
 	int rc;
 
@@ -29,6 +29,14 @@ static int transport_dbus_property_update(struct mbox_context *context,
 		props[i++] = "DaemonReady";
 	}
 
+	if (events & BMC_EVENT_WINDOW_RESET) {
+		props[i++] = "WindowReset";
+	}
+
+	if (events & BMC_EVENT_PROTOCOL_RESET) {
+		props[i++] = "ProtocolReset";
+	}
+
 	rc = sd_bus_emit_properties_changed_strv(context->bus,
 						 MBOX_DBUS_OBJECT,
 						 /* FIXME: Hard-coding v2 */
@@ -482,6 +490,10 @@ static int transport_dbus_get_property(sd_bus *bus,
 		value = context->bmc_events & BMC_EVENT_FLASH_CTRL_LOST;
 	} else if (!strcmp("DaemonReady", property)) {
 		value = context->bmc_events & BMC_EVENT_DAEMON_READY;
+	} else if (!strcmp("WindowReset", property)) {
+		value = context->bmc_events & BMC_EVENT_WINDOW_RESET;
+	} else if (!strcmp("ProtocolReset", property)) {
+		value = context->bmc_events & BMC_EVENT_PROTOCOL_RESET;
 	} else {
 		MSG_ERR("Unknown DBus property: %s\n", property);
 		return -EINVAL;
@@ -534,6 +546,14 @@ static const sd_bus_vtable protocol_v2_vtable[] = {
 			SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
 	SD_BUS_SIGNAL("ProtocolReset", NULL, 0),
 	SD_BUS_SIGNAL("WindowReset", NULL, 0),
+	SD_BUS_PROPERTY("ProtocolReset",  "b",
+			transport_dbus_get_property,
+			0, /* Just a pointer to struct mbox_context */
+			SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
+	SD_BUS_PROPERTY("WindowReset", "b",
+			transport_dbus_get_property,
+			0, /* Just a pointer to struct mbox_context */
+			SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
 	SD_BUS_VTABLE_END
 };