Controller hardware reset by ziyifu225 · Pull Request #268 · cyberus-technology/usbvfiod

ziyifu225 · 2026-07-09T11:34:10Z

When we tried to unload and reload the xhci_pci module in Linux, the warning WARN usbvfiod::device::xhci::command_ring: received useless write to CRCR while running can be triggered.
The reason is that we didn't really implement host controller reset before. When the Linux guest unloaded and reloaded xhci_pci, the driver reset the host controller and then reprogrammed the command/event/slot state. Since usbvfiod still kept the old state, the command ring could still be considered running and CRCR writes during re-initialization produced warnings.

This PR fixes this error by

Stopping the command ring
Resetting event ring/interrupter state
Resetting slot manager state

snue

Really cool! I suspect you have tested that the controller reset works now when the guest reloads the xhci driver? This would make for a good integration test, or maybe addition to an existing test.

I left some comments. Looks pretty good already 👍

snue · 2026-07-09T16:24:34Z

Since usbvfiod still kept the old state, the command ring could still be considered running and CRCR writes during re-initialization produced warnings.

It is also broken in that state 😉 Thanks for fixing this 🎉

snue

Pretty nice now. Functionally this seems sufficient, but I did not test it yet. I left some more suggestions, and I'd like to have a test (unit or integration or both) before merge.

The event worker used to wait for ERSTBA once, configure the event ring, and then keep using that configuration forever. After a host controller reset, the guest driver writes ERSTBA/ERSTSZ/ERDP again. The worker must stop using the old event ring state and wait for the new configuration. Split the worker loop into an unconfigured phase and a configured phase. On Reset, clear the EventRing state and return to waiting for ERSTBA.

lbeierlieb

Thanks for the PR! I tested the feature with rmmod xhci_pci; modprobe xhci_pci and the controller came back to life successfully. The implement logic looks alright, but I do not fully agree with which logic is implemented where. I have to checked all commits yet.

lbeierlieb · 2026-07-13T07:49:54Z

    }

+    pub fn reset(&self) -> anyhow::Result<()> {
+        self.running.store(false, Ordering::Relaxed);


The worker sets running to false when processing the message, so this assignment seems superfluous. This change also improves separation of concerns--pub fn reset() is only responsible for relaying the reset request to the worker, and the worker implements the logic.

lbeierlieb · 2026-07-13T08:55:13Z

                            WorkerMessage::Stop => {
                                self.state = WorkerState::Stopping;
                                break;
                            }


Not directly related to the PR, but we should probably also set the commandring_running to false when stopping.
"It [the CRR bit] is cleared to ‘0’ when the Command Ring is “stopped” after writing a ‘1’
to the Command Stop (CS) or Command Abort (CA) flags, or if the R/S bit is cleared to ‘0’" (XHCI spec)

Sorry, please remove again 🙈 we set the state to Stopping, in which we set CRR to false and then transition to Stopped.

lbeierlieb · 2026-07-13T09:10:01Z

-                    self.interrupt_line = interrupt_line;
-                    debug!("Updated interrupt line");
-                }
+    async fn process_configured_message(&mut self) -> anyhow::Result<bool> {


I would appreciate a short doc comment about the bool return value.

lbeierlieb · 2026-07-13T09:12:10Z

-                    self.interrupt_line = interrupt_line;
-                    debug!("Updated interrupt line");
-                }
+    async fn process_configured_message(&mut self) -> anyhow::Result<bool> {


process_configured_message sounds like the method processes a configured message. However, the method is the method for processing messages while the event ring is configured.

Right. Maybe just process_message()? And document the fact that this is called in configured state in the comment together with explaining the return value?

on second thought: Would it not be more straightforward to loop in process_messages/run_configured, with a break when we detect a reset message? Getting rid of the bool return value and the while loop in the run_loop.

lbeierlieb · 2026-07-13T09:25:54Z

+                if value & usbcmd::HCRST == usbcmd::HCRST {
+                    info!("Host Controller Reset requested");
+                    self.command_ring
+                        .reset()
+                        .map_err(|err| error!("failed to reset command ring: {err}"))
+                        .ok();
+                    self.interrupter
+                        .reset()
+                        .map_err(|err| error!("failed to reset event ring: {err}"))
+                        .ok();
+                    self.slot_manager
+                        .reset()
+                        .map_err(|err| error!("failed to reset slots: {err}"))
+                        .ok();
+                }


The XhciController is supposed to only connect the components, not implement any logic itself. The usbcmd.write should recognize the reset bit and then either send instruct command ring, interrupter, and slot manager to reset itself, or we introduce a small dedicated reset component, which the usbcmd knows and which is responsible for informing all relevant other components.

Yes, this sounds like a clean design, but also a bit overengineered IMHO. So far I am fine with the controller implementing the logic that concerns orchestrating multiple components and leaving any register access side effects and everything that is internal to a component to these. Our register abstractions cannot really deal with causing side effects currently, and I would like to keep it that way for now.

We have

#[derive(Debug, Default, Clone)] pub struct ErstbaRegister { value: Arc<AtomicU64>, notify: Arc<Notify>, } impl ErstbaRegister { pub fn read(&self) -> u64 { self.value.load(Ordering::Relaxed) } pub fn erstba(&self) -> u64 { self.value.load(Ordering::Relaxed) & !0x1f } pub fn write(&self, new_value: u64) { self.value.store(new_value, Ordering::Relaxed); self.notify.notify_waiters(); } ...

so that the interrupter can notice writes.

A reset component could listen the same way for usbcmd writes. The majority of notifications would not result in an action, but the total number of usbcmd writes is low, so overall no performance issue.

I would really like to keep any logic out of the XhciController. Controller reset is probably the case that would be the most acceptable to handle directly, but I fear it's a slippery slope and will cause more logic creeping into the top-level controller.

lbeierlieb · 2026-07-13T09:30:25Z

-            offset::USBCMD => self.usbcmd.write(value),
+            offset::USBCMD => {
+                self.usbcmd.write(value);
+                if value & usbcmd::HCRST == usbcmd::HCRST {


We recognize that the bit is set here, but I did not see setting the HCRST bit to fulfill the spec's HCRST description: "This bit is cleared to ‘0’ by the Host Controller when the reset process is complete".
Did I miss the HCRST zero write?
Even then, the timing must be off. We should wait until all components that we instruct to reset report that they have finished resetting (might take a while, we are passing messages around), and then update HCRST to zero.

We only check the written value and never actually store the bit, as the guest has no guarantee it will be able to see the bit flip (as far as I interpret the specification). When this was a noop, we were just "instantly done". We should probably document this behavior here, but I would like to see a stress test hitting register writes when we are not ready for them before actually implementing the observable set/clear on this bit.

If we want to make this bit observable, the special Reset component owned by usbcmd you proposed above suddenly becomes a more appealing option 😉

suggestion:

usbcmd::write notices HCRST set, notifies reset component; write also preserves HCRST

reset component triggers reset in other components

reset component waits for all components to report that they finished the reset operation

reset component sets HCRST bit to zero

* `usbcmd::write` notices HCRST set, notifies reset component; `write` also preserves HCRST

Yes.

* reset component triggers reset in other components

Through shared references to the relevant components I presume? Or just message passing?

* reset component waits for all components to report that they finished the reset operation

Yes. Just await a bunch of Futures with the reset component being a separate task. Do we need extra cleanup for it?

* reset component sets HCRST bit to zero

It also has access to the Arc<AtomicU64> value of usbcmd for that and take care only to handle HCRST? Or do we go for separate AtomicBool and assemble them in usbcmd? We could revert the recent refactor of the running bit from bool, as we have different "audiences" for these bits.

Through shared references to the relevant components I presume? Or just message passing?

The same way as currently the XhciController does it. All components are split in split in half--the "front end" is what the controller stores, storing atomics and message senders, the "back end" is a worker that owns state and typically processes messages coming from the front end. The reset component could take the front ends directly and call reset on them, which will send reset messages to the back-end worker. Alternatively, the front ends offer a create_reset_sender function, a thin wrapper around the sender to the respective back end.

Yes. Just await a bunch of Futures with the reset component being a separate task. Do we need extra cleanup for it?

cleanup for what? 🙈

It also has access to the Arc<AtomicU64> value of usbcmd for that and take care only to handle HCRST? Or do we go for separate AtomicBool and assemble them in usbcmd? We could revert the recent refactor of the running bit from bool, as we have different "audiences" for these bits.

Either direct access to the Arc<AtomicU32> with fetch_and or implement UsbcmdRegister::unsetHcrst and share the UsbcmdRegister between controller and reset component with an Arc.

cleanup for what? 🙈

All good 😁 the design checks out. Just a fetch_and on the Atomic sounds right as well.

I drafted a bit.

trait ResetSender { fn send_reset(&self, completion_notifier: oneshot::Sender<()>) -> anyhow::Result<()>; fn reset(&self) -> oneshot::Receiver<()> { let (send, recv) = oneshot::channel(); self.send_reset(send); recv } }

Each resetable component has to implement a sender and a function that returns such a sender. Command-ring example:

struct CommandRingResetSender { sender_to_worker: mpsc::UnboundedSender<WorkerMessage>, } impl ResetSender for CommandRingResetSender { fn send_reset(&self, completion_notifier: oneshot::Sender<()>) -> anyhow::Result<()> { self.sender_to_worker.send(WorkerMessage::Reset(completion_notifier)).map_err(|e| e.into()) } } impl CommandRing { pub fn reset_sender(&self) -> CommandRingResetSender { CommandRingResetSender { sender_to_worker: self.sender_to_worker.clone(), } } }

The reset component:

struct ResetCoordinator<const N: usize> { hcrst_notify: Notify, reset_senders: [Box<dyn ResetSender>; N], usbcmd: Arc<AtomicU32>, } impl<const N: usize> ResetCoordinator<N> { async fn run_loop(&self) -> anyhow::Result<()> { loop { self.hcrst_notify.notified().await; for reset_sender in &self.reset_senders { reset_sender.reset().await?; } self.usbcmd .fetch_and(!(usbcmd::HCRST as u32), Ordering::Relaxed); } } }

Ok, that's a nice separation of concerns. And the only reason for the reset().await to fail is when a channel is closed, in which case we don't need to bother sending the other resets or ever clearing the bit?

yep, exactly; reset() return value is directly the oneshot::Receiver, whose Future returns RecvError when the sender is dropped.

USBCMD no longer needs to log HCRST itself. The reset side effects are handled in the xHCI MMIO write path, so log the reset request there instead.

clean the extra running setting

set the commandring_running to false when stopping

Move configured-state message processing into run_configured() and break on reset instead of returning a bool to control the outer loop.

Rework host controller reset based on the PR design discussion. Previously, XhciController detected USBCMD.HCRST and directly reset the command ring, interrupter, and slot manager. Now USBCMD preserves HCRST and notifies a ResetCoordinator, which resets components through thin ResetSender handles and waits for completion before clearing HCRST. This keeps reset orchestration out of the top-level controller, gives each component ownership of its reset side effects, and makes HCRST observable until the reset process has completed.

lbeierlieb

Thank you for integrating the suggested changes! We are getting there, but there is some cleanup necessary to properly divide responsibilities.

lbeierlieb · 2026-07-17T13:20:04Z

                            WorkerMessage::Stop => {
                                self.state = WorkerState::Stopping;
                                break;
                            }


Sorry, please remove again 🙈 we set the state to Stopping, in which we set CRR to false and then transition to Stopped.

lbeierlieb · 2026-07-17T13:26:34Z

 use std::sync::Arc;

-#[derive(Debug)]
+#[derive(Debug, Clone)]


Why this change? You never use the clone functionality, the code works perfectly without Clone.

lbeierlieb · 2026-07-17T13:28:55Z

    msg_sender: mpsc::UnboundedSender<InterrupterMessage>,
 }

+#[derive(Debug, Clone)]


The idea is to create an InterrupterResetSender once and give it to the ResetCoordinator. There is no need for cloning intended, so I would rather not remove the Clone from here.

lbeierlieb · 2026-07-17T13:30:49Z

+        self.registers.interrupt_management.write(0);
+        self.registers
+            .interrupt_moderation_interval
+            .write(IMOD_DEFAULT);
+        self.registers.erst_base_address.reset();
+        self.registers.erst_size.write(0);
+        self.registers.eventring_dequeue_pointer.write(0);


The interrupter worker should execute all this code once it receives the message. send_reset should really only do sending the reset message to the worker.

lbeierlieb · 2026-07-17T13:40:44Z

-                    InterrupterMessage::Reset => self.event_ring.reset(),
+                    InterrupterMessage::Reset(completion) => {
+                        self.event_ring.reset();
+                        completion.send(()).ok();


Please use completion.send_anyhow(())?; (needs use crate::oneshot_anyhow::SendWithAnyhowError;)

lbeierlieb · 2026-07-17T13:46:10Z

+#[derive(Debug, Clone)]
 pub struct UsbcmdRegister {
    value: Arc<AtomicU32>,
+    notify: Arc<Notify>,


This notify is used only for reset notify, so reset_notify or similar would be a better name, then one look at the struct is enough to understand the purpose; otherwise, you have to look through the code and the usages of notify to understand the purpose.

lbeierlieb · 2026-07-17T13:47:00Z

 };

-#[derive(Debug)]
+#[derive(Debug, Clone)]


another unnecessary Clone

lbeierlieb · 2026-07-17T13:47:24Z

+        self.config_reg.reset();
+        self.dcbaap.reset();


should be in the worker

lbeierlieb · 2026-07-17T13:48:01Z

+    config_reg: ConfigureRegister,
+    dcbaap: DcbaapRegister,


not necessary when all the reset logic is in the worker

lbeierlieb · 2026-07-17T13:49:53Z

+                    let result = self.reset().await;
+                    completion.send(()).ok();
+                    result?;


if the reset failed, then we don't necessarily want to signal successful completion of the reset.
Suggestion:

self.reset().await?; completion.send_anyhow(())?;

snue · 2026-07-17T16:15:19Z

+        for slot in self.slots.iter_mut().filter_map(Option::as_mut) {
+            result = result.and(slot.pre_drop().await);
+        }
+        self.slots = [const { None }; MAX_SLOTS as usize].into();


We can get rid of this by also just using Option::take like below, right?

snue · 2026-07-17T16:21:58Z

-            .store(value.try_into().unwrap(), Ordering::Relaxed);
+            .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |current| {
+                let hcrst = current as u64 & usbcmd::HCRST;
+                let value = (value & BITMASK_PRESERVED) | (value & usbcmd::HCRST) | hcrst;


Suggested change

let value = (value & BITMASK_PRESERVED) | (value & usbcmd::HCRST) | hcrst;

let value = (value & BITMASK_WRITABLE) | hcrst;

snue · 2026-07-17T16:27:29Z

+        // Preserve an ongoing reset until the reset coordinator clears HCRST.
        self.value
-            .store(value.try_into().unwrap(), Ordering::Relaxed);
+            .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |current| {


Please double-check with @lbeierlieb whether Relaxed is really sufficient here.

ziyifu225 requested review from Noi0103, lbeierlieb and snue July 9, 2026 11:34

command ring: implement reset state

cfa3d50

ziyifu225 force-pushed the ziyifu/controller-reset branch 2 times, most recently from e5dd689 to d7f95a0 Compare July 9, 2026 15:34

snue reviewed Jul 9, 2026

View reviewed changes

ziyifu225 force-pushed the ziyifu/controller-reset branch from d7f95a0 to 61df7dd Compare July 10, 2026 08:39

snue reviewed Jul 10, 2026

View reviewed changes

Comment thread src/device/pci/xhci.rs Outdated

Comment thread src/device/xhci/registers.rs Outdated

Comment thread src/device/xhci/slot_manager.rs Outdated

Comment thread src/device/xhci/slot_manager.rs Outdated

Comment thread src/device/xhci/slot_manager.rs Outdated

ziyifu225 added 3 commits July 10, 2026 17:36

register: Add reset helpers

86bb12e

event ring: add event ring reset helper

23f886c

ziyifu225 force-pushed the ziyifu/controller-reset branch from a083bba to 1167813 Compare July 10, 2026 15:42

lbeierlieb requested changes Jul 13, 2026

View reviewed changes

ziyifu225 added 3 commits July 16, 2026 10:32

slot manager: implement reset all slots

f44698a

xhci: invoke reset from HCRST

2f0c330

registers: update host controller reset log message

f96fc07

USBCMD no longer needs to log HCRST itself. The reset side effects are handled in the xHCI MMIO write path, so log the reset request there instead.

ziyifu225 force-pushed the ziyifu/controller-reset branch from 813c2f9 to f96fc07 Compare July 16, 2026 08:33

ziyifu225 added 4 commits July 16, 2026 10:37

fixup! command ring: implement reset state

3987355

clean the extra running setting

fixup! command ring: implement reset state

337d7a0

set the commandring_running to false when stopping

fixup! interrupt: event worker reconfigure event ring after reset

9f23810

Move configured-state message processing into run_configured() and break on reset instead of returning a bool to control the outer loop.

lbeierlieb requested changes Jul 17, 2026

View reviewed changes

snue reviewed Jul 17, 2026

View reviewed changes

	let value = (value & BITMASK_PRESERVED) \| (value & usbcmd::HCRST) \| hcrst;
	let value = (value & BITMASK_WRITABLE) \| hcrst;

Uh oh!

Conversation

ziyifu225 commented Jul 9, 2026

Uh oh!

snue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snue commented Jul 9, 2026

Uh oh!

snue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lbeierlieb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbeierlieb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!