Bug 250924 - Bhyve AHCI disk controller regression due to r364334
Summary: Bhyve AHCI disk controller regression due to r364334
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bhyve (show other bugs)
Version: 12.2-RELEASE
Hardware: amd64 Any
: --- Affects Some People
Assignee: Peter Grehan
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2020-11-07 14:07 UTC by Rolf Stalder
Modified: 2021-12-18 14:39 UTC (History)
3 users (show)

See Also:

Attachments
diff patch against r364334 (272 bytes, patch)
2020-11-07 14:07 UTC, Rolf Stalder 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Rolf Stalder 2020-11-07 14:07:38 UTC 
Created attachment 219426 [details]
diff patch against r364334

Since https://svnweb.freebsd.org/base?view=revision&revision=364334 Solaris and Illumos guests are no longer able to detect the Bhyve AHCI disk controller and the following error is reported:

---
SATA disk device at port X does not support UDMA
---

https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/io/sata/impl/sata.c#L12752

The culprit seems to be the missing ATA flag 88 at offset 53 in the parameter buffer and this flag is indeed no longer set in 12.2-p0:

RELEASE-12.1:

buf[53] = (1 << 1 | 1 << 2);

RELEASE-12.2:

ata_ident->atavalid = (ATA_FLAG_54_58 | ATA_FLAG_64_70)

sys/sys/ata.h:
#define ATA_FLAG_54_58                  0x0001  /* words 54-58 valid */
#define ATA_FLAG_64_70                  0x0002  /* words 64-70 valid */
#define ATA_FLAG_88                     0x0004  /* word 88 valid */

---

Setting the old parameters solves the problem (at least for Solaris/Illumos):

ata_ident->atavalid = (ATA_FLAG_64_70 | ATA_FLAG_88)

The attached patch reverts these parameters and thus restores the old behaviour for both controllers (disk and cdrom).

The question is whether this change is intentional or simply a bug.
Comment 1 Peter Grehan freebsd_committer freebsd_triage 2020-11-09 03:37:22 UTC 
That's a bug :( I'll do a before/after check of other fields and see if there are some additional unintended deletions.
Comment 2 Rolf Stalder 2020-11-09 14:34:32 UTC 
Thanks a lot. Apart from that I've found no differing fields.
Comment 3 commit-hook freebsd_committer freebsd_triage 2020-11-15 12:59:33 UTC 
A commit references this bug:

Author: grehan
Date: Sun Nov 15 12:59:25 UTC 2020
New revision: 367709
URL: https://svnweb.freebsd.org/changeset/base/367709

Log:
  Fix regression in AHCI controller settings.

  When the AHCI code was reworked to use FreeBSD struct
  definitions, the valid element was mis-transcribed resulting
  in the UMDA capability being hidden. This prevented Illumos
  from using AHCI disk/cdrom drives.

  Fix by using definitions that match the code pre-rework.

  PR:	250924
  Submitted by:	Rolf Stalder
  Reported by:	Rolf Stalder
  MFC after:	3 days

Changes:
  head/usr.sbin/bhyve/pci_ahci.c
Comment 4 commit-hook freebsd_committer freebsd_triage 2020-11-20 03:34:12 UTC 
A commit references this bug:

Author: grehan
Date: Fri Nov 20 03:33:31 UTC 2020
New revision: 367859
URL: https://svnweb.freebsd.org/changeset/base/367859

Log:
  MFC r367709
      Fix regression in AHCI controller settings.

  PR:	250924
  Submitted by:	Rolf Stalder
  Reported by:	Rolf Stalder
  Relnotes:	Yes

Changes:
_U  stable/12/
  stable/12/usr.sbin/bhyve/pci_ahci.c
Comment 5 Rolf Stalder 2021-12-18 14:39:46 UTC 
This patch still works with both subsequent releases (12.3-RELEASE and 13.0-RELEASE) and the status of this bug report can therefore be set to "closed".