Page MenuHomeFreeBSD

bhyve: fix NVMe emulation update of SQHD
ClosedPublic

Authored by chuck on Mar 15 2020, 11:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 1, 10:10 PM
Unknown Object (File)
Wed, May 1, 9:44 PM
Unknown Object (File)
Wed, May 1, 9:41 PM
Unknown Object (File)
Wed, May 1, 3:58 PM
Unknown Object (File)
Thu, Apr 18, 6:33 PM
Unknown Object (File)
Feb 29 2024, 6:10 AM
Unknown Object (File)
Jan 18 2024, 5:40 AM
Unknown Object (File)
Jan 13 2024, 3:21 AM
Subscribers

Details

Summary

The SQHD field of a Completion Queue entry indicates the current
Submission Queue head pointer value. The head pointer represents the
next entry to be consumed and is updated after consuming the current
entry.

In the Admin queue processing, the current code updates the head pointer
after reporting the value to the host via the SQHD. This gives the
impression that the Controller is perpetually one command behind in its
processing of the Admin SQ. And while this doesn't appear to bother some
initiators, it is wrong.

Fix is to update the SQ head pointer prior to writing the SQHD value in
the completion.

Reported by khng300

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This seems legit, but I've not done a deep dive...

This revision is now accepted and ready to land.Mar 16 2020, 12:18 AM
usr.sbin/bhyve/pci_nvme.c
1261 ↗(On Diff #69559)

Writing cdw0 seems unrelated to this change? Also, the acquire memory barrier seems odd. Normally for writing to a descriptor entry what you want is a release barrier on the write to set the word in the descriptor entry that the host reads to determine if an entry is valid.

usr.sbin/bhyve/pci_nvme.c
1261 ↗(On Diff #69559)
  1. Yes, the cdw0 change is unrelated but it's a bug I noticed while fixing the OP's issue
  2. Agreed that the acquire is odd, but it's original code. I have some WIP that may remove this

I guess at least add a note in the commit log explaining why you are setting cdw0 (e.g. While here, set the cdw0 field for foo case which was missing).

The barrier in its new location (halfway through the update) makes no real sense. It probably just wants to be atomic_load_short() which I think didn't exist when this was first written.

This revision was automatically updated to reflect the committed changes.