Bug 250924

Summary: Bhyve AHCI disk controller regression due to r364334
Product: Base System Reporter: Rolf Stalder <freebsd>
Component: bhyveAssignee: Peter Grehan <grehan>
Status: Closed FIXED    
Severity: Affects Some People CC: grehan, lwhsu, markj
Priority: --- Keywords: regression
Version: 12.2-RELEASE   
Hardware: amd64   
OS: Any   
Attachments:
Description Flags
diff patch against r364334 none

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".