Re: [PATCH v2] iommu/riscv: Replace illegal command with dummy IOFENCE to prevent hardware lockup

On Fri, Jul 17, 2026 at 9:46 PM Tomasz Jeznach <[email protected]> wrote:
>
> Hi,
>
> On 6/29/26 7:30 PM, Zong Li wrote:
> > When the RISC-V IOMMU encounters an illegal command, the hardware
> > stops processing and the HEAD register remains pointing at the
> > illegal command. If software does not handle this properly, the
> > hardware will be stuck at this index indefinitely, preventing any
> > further command queue operations.
>
> The definition of proper handling was included in the spec. This patch
> simply implements logic to ignore real hardware errors and move forward,
> leaving the system in an unspecified state. The driver should not allow
> the system to pretend that everything is fine.
>
> > This patch implements a recovery mechanism by replacing the illegal
> > command with a dummy IOFENCE instruction (all operands are zero):
> >
> > 1. Prevents hardware lockup: By overwriting the illegal command with
> >     a valid instruction, the hardware can continue processing from the
> >     current position instead of being stuck.
> Currently, there is no clear way of communicating mapping
> synchronization failures in the IOMMU/DMA path. The only reasonable
> fault handler is either to panic or to make the fault visible to the
> user with a very long timeout. The timeout approach has been chosen in
> this driver; there are pros and cons to this, but let's not get into
> that discussion here.
>
> By overwriting an illegal command without any notification to the user,
> we are simply hiding e.g. TLB invalidation faults and likely causing
> use-after-free errors for DMA transfers. I would prefer not to go that way.
>
> > 2. Enables user recovery: After replacing the illegal command, the
> >     user/driver has an opportunity to retry the original failed
> >     operation rather than losing all queued work.
> Unfortunately user/driver has no way to know if any retry is required.
>
> > 3. Minimal hardware impact: A dummy IOFENCE behaves as a NOP, it
> >     it performs no cache invalidation operations and has no side
> >     effects on the system state. This is the safest replacement
> >     instruction.
> In the first place, an illegal command should not be enqueued into the
> command queue. If there is one, it's a driver/software bug or a hardware
> incompatibility with the specification. We should address this problem
> first. If a command has been interpreted as illegal, there might be
> other possible reasons for the fault - e.g., reading commands from an
> incorrect memory location or some other memory corruption related
> issues. Overwriting it with a "NOP" might not resolve this at all.
>
> I agree there is error handling needed for cmd_ill / timeout / fault
> errors, but so far the only reasonable way to handle those is to make
> the fault loud and annoying to the user (applications), with the IOMMU
> driver reporting errors via the kernel log.
>
> Is this patch trying to solve a real problem of a hardware faulting on
> illegal instructions, or is this a theoretical scenario with the
> implementation mimicking the SMMU implementation?
>
> > Signed-off-by: Zong Li <[email protected]>
> > ---
> >
> > The main goal is to at least prevent the hardware from getting stuck
> > and crashing the entire system.
> >
> > Here are the main issues we would need to solve if we fully follow the spec:
> >
> > 1. IOFENCE index shift: After an illegal command, if another thread is
> > waiting for subsequent IOFENCE to finish, resubmitting commands will
> > change that IOFENCE's index in the queue. This means the waiting
> > thread in 'riscv_iommu_cmd_sync' might finish too early because the
> > 'prod' value should be changed as well. We could fix this by making
> > IOFENCE write a sequence number to a specific address, and having the
> > thread wait for that data instead.
>
> Unfortunately, there is already a 'fix' added in the driver to exit the
> sync wait loop if a fault is reported on the command queue (line #381).
> There is a race with the fault interrupt handler that should be
> addressed and fixed.
>
> > 2. Timeout errors: If an illegal command happens while another thread
> > is trying to write a command, that thread might be waiting for the
> > tail to move (in 'riscv_iommu_queue_send'), and exit the wait due to a
> > timeout. This leads to errors in the caller subsystem (like DMA). So
> > it seems even if the resubmit finishes later, it might not help much.
> >
> > 3. Queue tail mismatch: Similar to point 2 situation, a thread waiting
> > in 'riscv_iommu_queue_send' expects prod == queue->tail. If we
> > resubmit commands quickly, queue->tail is updated asynchronously to a
> > farther value. The waiting thread might never see the condition met
> > and time out.
> >
> > 4. Shadow queue overhead: Inside the threading IRQ handler, we cannot
> > easily know what the illegal command was just by checking the current
> > command queue. We would need to create a "shadow command queue" to
> > keep a history. This would break the current driver design. We would
> > also need to add locks to prevent race conditions on shadow command
> > queue, which would reduce the driver's performance.
> >
> > Considering these trade-offs, I prefer not to make the driver much
> > more complex and slower just to handle rare hardware errors.
> > Treating this hardware fault as a fatal error without trying to
> > recover it in software might be too extreme and would require a
> > hardware reset. Therefore, this patch might be a good middle ground.
>
> As an alternative trade-off, maybe consider reporting and storing the
> fault for the IOMMU instance and failing all subsequent page map
> requests. Instead of a long timeout, this would fail quickly, giving
> users a clear signal that the DMA subsystem using this IOMMU device is
> no longer usable.
>
> An alternative error-handling mechanism I've considered in the past was
> to shut down the command queue, quiesce all IOMMU interfaces (PRI/ATS),
> and completely reprogram the IOMMU from scratch (setting up the DDTP and
> re-enabling the command queue). As per the specification, restarting the
> command queue clears cmd_ill/timeout/fault errors. Once the IOMMU
> configuration is restarted, we can unblock any pending
> riscv_iommu_queue_wait() calls with a simpler sequence lock.
>
> > It prevents the hardware lockup. Even though we might lose some commands
> > and cause incorrect results for the user, it at least keeps the system
> > alive and gives the user a chance to retry their operation again.
> I think this patch only hides hardware errors, potentially masking
> not-so-rare real faults.
>

Hi Tomasz,
I totally agree with your prespective. This patch tries to recover the
IOMMU hardware from lockup, but it doesn't recover the command. It
might introduce potential errors. I would not continue with this patch
until we can solve it properly. Thanks

>
> Best,
> - Tomasz
>
> >
> > Changed in v1:
> > - Added more comments
> > - Rebased on v7.2-rc1
> >
> >   drivers/iommu/riscv/iommu.c | 32 +++++++++++++++++++++++++++++++-
> >   1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> > index cec3ddd7ab10..c009c1906e23 100644
> > --- a/drivers/iommu/riscv/iommu.c
> > +++ b/drivers/iommu/riscv/iommu.c
> > @@ -464,13 +464,43 @@ static unsigned int riscv_iommu_queue_send(struct riscv_iommu_queue *queue,
> >   static irqreturn_t riscv_iommu_cmdq_process(int irq, void *data)
> >   {
> >       const struct riscv_iommu_queue *queue = (struct riscv_iommu_queue *)data;
> > -     unsigned int ctrl;
> > +     struct riscv_iommu_command cmd;
> > +     unsigned int ctrl, head;
> >
> >       /* Clear MF/CQ errors, complete error recovery to be implemented. */
> >       ctrl = riscv_iommu_readl(queue->iommu, queue->qcr);
> >       if (ctrl & (RISCV_IOMMU_CQCSR_CQMF | RISCV_IOMMU_CQCSR_CMD_TO |
> >                   RISCV_IOMMU_CQCSR_CMD_ILL | RISCV_IOMMU_CQCSR_FENCE_W_IP)) {
> > +
> > +             /*
> > +              * Resubmitting all commands submitted since the last IOFENCE that
> > +              * successfully completed will introduce various race conditions.
> > +              * Use a dummy IOFENCE instead of the illegal command to prevent
> > +              * hardware lockup.
> > +              * Please note that some commands might be lost, including:
> > +              *  - The task from the illegal command itself
> > +              *  - The commands submitted between the last IOFENCE and illegal one
> > +              * However, this gives the user or driver a chance to retry the
> > +              * failed operation without resetting the enitre system
> > +              */
> > +             if (ctrl & RISCV_IOMMU_CQCSR_CMD_ILL) {
> > +                     /*
> > +                      * The head pointer is not updated by the hardware, it
> > +                      * still points to the index of illegal command
> > +                      */
> > +                     riscv_iommu_readl_timeout(queue->iommu, Q_HEAD(queue), head,
> > +                                               !(head & ~queue->mask), 0,
> > +                                               RISCV_IOMMU_QUEUE_TIMEOUT);
> > +
> > +                     memset(&cmd, 0, sizeof(cmd));
> > +                     cmd.dword0 = FIELD_PREP(RISCV_IOMMU_CMD0_OPCODE,
> > +                                             RISCV_IOMMU_CMD_IOFENCE_OPCODE);
> > +                     memcpy(queue->base + head * sizeof(cmd), &cmd, sizeof(cmd));
> > +                     dma_wmb();
> > +             }
> > +
> >               riscv_iommu_writel(queue->iommu, queue->qcr, ctrl);
> > +
> >               dev_warn(queue->iommu->dev,
> >                        "Queue #%u error; fault:%d timeout:%d illegal:%d fence_w_ip:%d\n",
> >                        queue->qid,
>