risc-v/mpfs: ihc: cleanup DEBUGASSERTs and irq enabling

Replace DEBUGASSERTs with sanity checks. DEBUGASSERT()s are
not necessarily enabled at all, thus risking the functionality
especially in that case. Remove PANICs as well.

Don't enable the ihc irq too early. If enabled, and the master
is already up, the irq is being issued so that the system gets
stuck or is severely slowed down. Master may be already up if
this NuttX hart only is rebooted, for example.

Signed-off-by: Eero Nurkkala <eero.nurkkala@offcode.fi>

arch/risc-v/src/mpfs/mpfs_ihc.c

@@ -55,7 +55,7 @@
  * Pre-processor Definitions
  ****************************************************************************/
 
-#ifdef CONFIG_MPFS_IHC_DEBUG
+#ifdef CONFIG_DEBUG_ERROR
 #  define ihcerr  _err
 #  define ihcwarn _warn
 #  define ihcinfo _info
@@ -337,9 +337,6 @@ static uint32_t mpfs_ihc_context_to_remote_hart_id(ihc_channel_t channel)
     }
   else
     {
-      DEBUGASSERT(LIBERO_SETTING_CONTEXT_A_HART_EN > 0);
-      DEBUGASSERT(LIBERO_SETTING_CONTEXT_B_HART_EN > 0);
-
       /* Determine context we are in */
 
       if (channel == IHC_CHANNEL_TO_CONTEXTA)
@@ -350,10 +347,6 @@ static uint32_t mpfs_ihc_context_to_remote_hart_id(ihc_channel_t channel)
         {
           harts_in_context = LIBERO_SETTING_CONTEXT_B_HART_EN;
         }
-      else
-        {
-          DEBUGPANIC();
-        }
 
       hart_idx = 0;
 
@@ -369,8 +362,6 @@ static uint32_t mpfs_ihc_context_to_remote_hart_id(ihc_channel_t channel)
         }
     }
 
-  DEBUGASSERT(hart != UNDEFINED_HART_ID);
-
   return hart;
 }
 
@@ -431,8 +422,6 @@ static uint32_t mpfs_ihc_context_to_local_hart_id(ihc_channel_t channel)
         }
     }
 
-  DEBUGASSERT(hart < MPFS_NUM_HARTS);
-
   return hart;
 }
 
@@ -454,10 +443,20 @@ static uint32_t mpfs_ihc_context_to_local_hart_id(ihc_channel_t channel)
 
 static void mpfs_ihc_rx_handler(uint32_t *message)
 {
-  g_vq_idx = message[0];
+  uint32_t msg = message[0];
 
-  DEBUGASSERT((g_vq_idx == VRING0_NOTIFYID) ||
+  /* After a warm reboot, the message may be initially corrupt as the renote
-              (g_vq_idx == VRING1_NOTIFYID));
+   * doesn't know we restarted and reinitialized the registers.
+   */
+
+  if ((msg == VRING0_NOTIFYID) || (msg == VRING1_NOTIFYID))
+    {
+      g_vq_idx = msg;
+    }
+  else
+    {
+      return;
+    }
 
 #ifdef MPFS_RPTUN_USE_THREAD
   nxsem_post(&g_mpfs_rx_sig);
@@ -495,7 +494,10 @@ static void mpfs_ihc_worker(void *arg)
     }
   while ((ctrl_reg & RMP_MESSAGE_PRESENT) && --retries);
 
-  DEBUGASSERT(retries != 0);
+  if (retries == 0)
+    {
+      ihcerr("Could not send the message!\n");
+    }
 
   modifyreg32(MPFS_IHC_CTRL(g_work_arg.mhartid,
               g_work_arg.rhartid), 0, ACK_INT);
@@ -526,19 +528,25 @@ static void mpfs_ihc_rx_message(ihc_channel_t channel, uint32_t mhartid,
   uint32_t rhartid  = mpfs_ihc_context_to_remote_hart_id(channel);
   uint32_t ctrl_reg = getreg32(MPFS_IHC_CTRL(mhartid, rhartid));
 
+  if (rhartid == UNDEFINED_HART_ID)
+    {
+      ihcerr("Remote hart not identified!\n");
+      return;
+    }
+
   /* Check if we have a message */
 
   if (mhartid == CONTEXTB_HARTID)
     {
       uintptr_t msg_in = MPFS_IHC_MSG_IN(mhartid, rhartid);
-      DEBUGASSERT(msg == NULL);
       mpfs_ihc_rx_handler((uint32_t *)msg_in);
     }
   else
     {
       /* This path is meant for the OpenSBI vendor extension only */
 
-      DEBUGPANIC();
+      ihcerr("Wrong recipient!\n");
+      return;
     }
 
   /* Set MP to 0. Note this generates an interrupt on the other hart
@@ -673,15 +681,19 @@ static int mpfs_ihc_interrupt(int irq, void *context, void *arg)
  *   hart_to_configure   - Hart to be configured
  *
  * Returned Value:
- *   None
+ *   OK if all is good, a negated error code otherwise
  *
  ****************************************************************************/
 
-static void mpfs_ihc_local_context_init(uint32_t hart_to_configure)
+static int mpfs_ihc_local_context_init(uint32_t hart_to_configure)
 {
   uint32_t rhartid = 0;
 
-  DEBUGASSERT(hart_to_configure < MPFS_NUM_HARTS);
+  if (hart_to_configure >= MPFS_NUM_HARTS)
+    {
+      ihcerr("Configuring too many harts!\n");
+      return -EINVAL;
+    }
 
   while (rhartid < MPFS_NUM_HARTS)
     {
@@ -691,6 +703,8 @@ static void mpfs_ihc_local_context_init(uint32_t hart_to_configure)
 
   g_connected_harts     = ihcia_remote_harts[hart_to_configure];
   g_connected_hart_ints = ihcia_remote_hart_ints[hart_to_configure];
+
+  return OK;
 }
 
 /****************************************************************************
@@ -744,7 +758,24 @@ static int mpfs_ihc_tx_message(ihc_channel_t channel, uint32_t *message)
   uint32_t ctrl_reg;
   uint32_t retries      = 10000;
 
-  DEBUGASSERT(message_size <= IHC_MAX_MESSAGE_SIZE);
+  if (mhartid >= MPFS_NUM_HARTS)
+    {
+      ihcerr("Problem finding proper mhartid\n");
+      return -EINVAL;
+    }
+
+  if (rhartid == UNDEFINED_HART_ID)
+    {
+      /* Something went wrong */
+
+      ihcerr("Remote hart not found!\n");
+      return -EINVAL;
+    }
+  else if (message_size > IHC_MAX_MESSAGE_SIZE)
+    {
+      ihcerr("Sent message too large!\n");
+      return -EINVAL;
+    }
 
   /* Check if the system is busy.  All we can try is wait. */
 
@@ -871,8 +902,6 @@ mpfs_rptun_get_resource(struct rptun_dev_s *dev)
 
   /* Only slave supported so far */
 
-  DEBUGASSERT(!priv->master);
-
   if (priv->shmem != NULL)
     {
       return &priv->shmem->rsc;
@@ -901,9 +930,12 @@ mpfs_rptun_get_resource(struct rptun_dev_s *dev)
       rsc->rpmsg_vdev.gfeatures     = 1 << VIRTIO_RPMSG_F_NS  |
                                       1 << VIRTIO_RPMSG_F_ACK;
 
-      /* Set to VIRTIO_CONFIG_STATUS_DRIVER_OK when master is up */
+      /* If the master is up already, don't clear the status here */
 
-      rsc->rpmsg_vdev.status        = 0;
+      if (!g_shmem.master_up)
+        {
+          rsc->rpmsg_vdev.status    = 0;
+        }
 
       rsc->rpmsg_vdev.config_len    = sizeof(struct fw_rsc_config);
       rsc->rpmsg_vdev.num_of_vrings = VRINGS;
@@ -925,6 +957,12 @@ mpfs_rptun_get_resource(struct rptun_dev_s *dev)
 
   g_rptun_initialized = true;
 
+  /* Don't enable this too early; if the master is already up, irqs will
+   * likely hang the system as no ACKs may be sent yet.
+   */
+
+  up_enable_irq(g_plic_irq);
+
   return &priv->shmem->rsc;
 }
 
@@ -1046,7 +1084,10 @@ static int mpfs_rptun_notify(struct rptun_dev_s *dev, uint32_t notifyid)
         }
       while ((ret != OK) && --retries);
 
-      DEBUGASSERT(ret == OK);
+      if (retries == 0)
+        {
+          return -EIO;
+        }
     }
 
   return ret;
@@ -1232,8 +1273,19 @@ static void mpfs_rptun_worker(void *arg)
 {
   struct mpfs_queue_table_s *info;
 
-  DEBUGASSERT((g_vq_idx - VRING0_NOTIFYID) < VRINGS);
+  /* Check whether the struct is initialized yet */
-  info = &g_mpfs_virtqueue_table[g_vq_idx - VRING0_NOTIFYID];
+
+  if (*(uintptr_t *)&g_mpfs_virtqueue_table[0] == 0)
+    {
+      return;
+    }
+
+  if (g_vq_idx >= VRINGS)
+    {
+      return;
+    }
+
+  info = &g_mpfs_virtqueue_table[g_vq_idx];
   virtqueue_notification((struct virtqueue *)info->data);
 }
 #endif
@@ -1262,8 +1314,19 @@ static int mpfs_rptun_thread(int argc, char *argv[])
 
   while (1)
     {
-      DEBUGASSERT((g_vq_idx - VRING0_NOTIFYID) < VRINGS);
+      /* Check whether the struct is initialized yet */
-      info = &g_mpfs_virtqueue_table[g_vq_idx - VRING0_NOTIFYID];
+
+      if (*(uintptr_t *)&g_mpfs_virtqueue_table[0] == 0)
+        {
+          return 0;
+        }
+
+      if (g_vq_idx >= VRINGS)
+        {
+          return -EINVAL;
+        }
+
+      info = &g_mpfs_virtqueue_table[g_vq_idx];
       virtqueue_notification((struct virtqueue *)info->data);
 
       nxsem_wait(&g_mpfs_rx_sig);
@@ -1326,17 +1389,18 @@ int mpfs_ihc_init(void)
 
   /* Initialize IHC FPGA module registers to a known state */
 
-  mpfs_ihc_local_context_init(mhartid);
+  ret = mpfs_ihc_local_context_init(mhartid);
+  if (ret != OK)
+    {
+      return ret;
+    }
+
   mpfs_ihc_local_remote_config(mhartid, rhartid);
 
   /* Attach and enable the applicable irq */
 
   ret = irq_attach(g_plic_irq, mpfs_ihc_interrupt, NULL);
-  if (ret == OK)
+  if (ret != OK)
-    {
-      up_enable_irq(g_plic_irq);
-    }
-  else
     {
       ihcerr("ERROR: Not able to attach irq\n");
       return ret;
@@ -1345,6 +1409,18 @@ int mpfs_ihc_init(void)
   /* Initialize and wait for the master. This will block until. */
 
   ihcinfo("Waiting for the master online...\n");
+
+  /* Check if the remote is already up.  This is the case after reboot of
+   * this particular hart only.
+   */
+
+  if ((getreg32(MPFS_IHC_CTRL(CONTEXTA_HARTID, CONTEXTB_HARTID)) & (MPIE_EN
+      | ACKIE_EN)) != 0)
+    {
+      g_shmem.master_up = true;
+      g_shmem.rsc.rpmsg_vdev.status |= VIRTIO_CONFIG_STATUS_DRIVER_OK;
+    }
+
   ret = mpfs_rptun_init(MPFS_RPTUN_SHMEM_NAME, MPFS_RPTUN_CPU_NAME);
   if (ret < 0)
     {

arch/risc-v/src/mpfs/mpfs_ihc_sbi.c

@@ -73,8 +73,7 @@ static uint32_t g_connected_harts_c;
  * Description:
  *   This is a copy of modifyreg32() without spinlock.  That function is a
  *   real danger here as it is likely located in eNVM, thus being a real
- *   bottleneck.  All functions called from this file should be located in
+ *   bottleneck.
- *   the zero device.
  *
  * Input Parameters:
  *   addr       - Address to perform the operation
@@ -219,8 +218,6 @@ static uint32_t mpfs_ihc_sbi_context_to_remote_hart_id(ihc_channel_t channel)
       hart_idx++;
     }
 
-  DEBUGASSERT(hart != UNDEFINED_HART_ID);
-
   return hart;
 }
 
@@ -245,8 +242,6 @@ static uint32_t mpfs_ihc_sbi_context_to_local_hart_id(ihc_channel_t channel)
   uint32_t harts_in_context = LIBERO_SETTING_CONTEXT_A_HART_EN;
   uint32_t hart_next        = 0;
 
-  DEBUGASSERT(channel > IHC_CHANNEL_TO_HART4);
-
   hart_idx = 0;
   while (hart_idx < MPFS_NUM_HARTS)
     {
@@ -272,7 +267,6 @@ static uint32_t mpfs_ihc_sbi_context_to_local_hart_id(ihc_channel_t channel)
       hart_idx++;
     }
 
-  DEBUGASSERT(hart < MPFS_NUM_HARTS);
   return hart;
 }
 
@@ -304,8 +298,7 @@ static void mpfs_ihc_sbi_message_present_handler(uint32_t *message,
                                                  bool is_mp)
 {
   struct ihc_sbi_rx_msg_s *msg;
-  uintptr_t message_ihc     = (uintptr_t)MPFS_IHC_MSG_IN(mhartid, rhartid);
+  uintptr_t message_ihc = (uintptr_t)MPFS_IHC_MSG_IN(mhartid, rhartid);
-  uint32_t message_size_ihc = getreg32(MPFS_IHC_MSG_SIZE(mhartid, rhartid));
 
   msg = (struct ihc_sbi_rx_msg_s *)message;
 
@@ -325,8 +318,6 @@ static void mpfs_ihc_sbi_message_present_handler(uint32_t *message,
       msg->irq_type = ACK_IRQ | MP_IRQ;
       msg->ihc_msg = *(struct mpfs_ihc_msg_s *)message_ihc;
     }
-
-    DEBUGASSERT(sizeof(msg->ihc_msg) >= message_size_ihc);
 }
 
 /****************************************************************************
@@ -351,17 +342,23 @@ static void mpfs_ihc_sbi_message_present_handler(uint32_t *message,
 static void mpfs_ihc_sbi_rx_message(uint32_t rhartid, uint32_t mhartid,
                                     bool is_ack, bool is_mp, uint32_t *msg)
 {
+  /* Msg must be always present */
+
+  if (msg == NULL)
+    {
+      return;
+    }
+
   if (is_ack && !is_mp)
     {
       if (mhartid == CONTEXTB_HARTID)
         {
-          DEBUGPANIC();
+          return;
         }
       else
         {
           /* This path is meant for the OpenSBI vendor extension only */
 
-          DEBUGASSERT(msg != NULL);
           mpfs_ihc_sbi_message_present_handler(msg, mhartid, rhartid,
                                                is_ack, is_mp);
 
@@ -376,13 +373,12 @@ static void mpfs_ihc_sbi_rx_message(uint32_t rhartid, uint32_t mhartid,
 
       if (mhartid == CONTEXTB_HARTID)
         {
-          DEBUGPANIC();
+          return;
         }
       else
         {
           /* This path is meant for the OpenSBI vendor extension only */
 
-          DEBUGASSERT(msg != NULL);
           mpfs_ihc_sbi_message_present_handler(msg, mhartid, rhartid,
                                                is_ack, is_mp);
         }
@@ -395,7 +391,6 @@ static void mpfs_ihc_sbi_rx_message(uint32_t rhartid, uint32_t mhartid,
     }
   else if (is_ack && is_mp)
     {
-      DEBUGASSERT(msg != NULL);
       mpfs_ihc_sbi_message_present_handler(msg, mhartid, rhartid,
                                            is_ack, is_mp);
 
@@ -432,7 +427,8 @@ void mpfs_ihc_sbi_message_present_indirect_isr(ihc_channel_t channel,
   uint32_t origin_hart = mpfs_ihc_sbi_parse_incoming_hartid(mhartid,
                                                             &is_ack,
                                                             &is_mp);
-  if (origin_hart != UNDEFINED_HART_ID)
+
+  if ((origin_hart != UNDEFINED_HART_ID) && (mhartid < MPFS_NUM_HARTS))
     {
       /* Process incoming packet */
 
@@ -461,8 +457,6 @@ static void mpfs_ihc_sbi_local_context_init(uint32_t hart_to_configure)
 {
   uint32_t rhartid = 0;
 
-  DEBUGASSERT(hart_to_configure < MPFS_NUM_HARTS);
-
   while (rhartid < MPFS_NUM_HARTS)
     {
       putreg32(0, MPFS_IHC_CTRL(hart_to_configure, rhartid));
@@ -552,7 +546,18 @@ static int mpfs_ihc_sbi_tx_message(ihc_channel_t channel, uint32_t *message)
   uint32_t ctrl_reg;
   uint32_t retries      = 10000;
 
-  DEBUGASSERT(message_size <= IHC_MAX_MESSAGE_SIZE);
+  if (message_size > IHC_MAX_MESSAGE_SIZE)
+    {
+      return -EINVAL;
+    }
+  else if (rhartid == UNDEFINED_HART_ID)
+    {
+      return -EINVAL;
+    }
+  else if (mhartid >= MPFS_NUM_HARTS)
+    {
+      return -EINVAL;
+    }
 
   /* Check if the system is busy.  All we can try is wait. */
 
@@ -644,7 +649,19 @@ int mpfs_ihc_sbi_ecall_handler(unsigned long funcid, uint32_t remote_channel,
         /* mhartid = Linux hart id, rhartid = NuttX hart id */
 
         mhartid = mpfs_ihc_sbi_context_to_local_hart_id(remote_channel);
+
+        if (mhartid >= MPFS_NUM_HARTS)
+          {
+            return -EINVAL;
+          }
+
         rhartid = mpfs_ihc_sbi_context_to_remote_hart_id(remote_channel);
+
+        if (rhartid == UNDEFINED_HART_ID)
+          {
+            return -EINVAL;
+          }
+
         if (remote_channel == IHC_CHANNEL_TO_CONTEXTB)
           {
             mpfs_ihc_sbi_local_context_init(mhartid);