Bug 250924 - Bhyve AHCI disk controller regression due to r364334
Summary: Bhyve AHCI disk controller regression due to r364334
Status: In Progress
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: 2020-11-20 03:34 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

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