Bug 235876 - ACPI related commit r330113 broke Microsoft Hyper-V passthrough device on Generation 2 FreeBSD VM
Summary: ACPI related commit r330113 broke Microsoft Hyper-V passthrough device on Gen...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.0-RELEASE
Hardware: amd64 Any
: --- Affects Some People
Assignee: freebsd-acpi (Nobody)
URL:
Keywords: needs-qa, regression
Depends on:
Blocks:
 
Reported: 2019-02-20 06:46 UTC by Wei Hu
Modified: 2019-05-01 18:18 UTC (History)
7 users (show)

See Also:


Attachments
Acpidump collected on FreeBSD 12 Guest OS running as a Gen 2 VM on Microsoft Hyper-V (78.68 KB, text/plain)
2019-02-20 06:46 UTC, Wei Hu
no flags Details
TX2 asl (13.76 KB, application/x-xz)
2019-02-27 14:50 UTC, Ed Maste
no flags Details
thunderx2_uart_workaround.patch (1.27 KB, patch)
2019-03-01 19:11 UTC, John Baldwin
no flags Details | Diff
tx2 asl from firmware 7.2 (13.75 KB, application/x-xz)
2019-03-05 21:55 UTC, Ed Maste
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wei Hu 2019-02-20 06:46:15 UTC
Created attachment 202178 [details]
Acpidump collected on FreeBSD 12 Guest OS running as a Gen 2 VM on Microsoft Hyper-V

On HyperV Generation 2 VMs, all passthrough devices are attached to pcib0 bus. Upon guest OS boot, pcib0 bus as a device asks to resource 0x2000 byte MMIO memory address space. With the Commit r330133, the request was rejected by nexus_alloc_resource(). Further debugging shows with this commit, the address range requested has been reserved by unknown device much earlier than pcib0’s request. And almost all good addresses were reserved by this ‘unknown’ device. I put some debug print in the code. Following is the dmesg output:
----
OS boot
…
nexus0: &&&in nexus_alloc_resource, check 10, child request range (0xfe0000000-0xfffffffff), count 0x20000000     A request to reserve this amount of memory address in the range.
unknown: &&&in nexus_alloc_resource, check 20, this is the child, needactivate = 0    requesting child is unknown because the name of device_t is empty.
nexus0: &&&in nexus_alloc_resource, after calling rman_reserve_resource, rv return Not NULL           memory addr successfully reserved for this unknown device
…
nexus0: &&&in nexus_alloc_resource, check 10, this is the request bus, range (0xf8000000-0xffdfffff), count 0x7e00000   once again request another chunk of memory address
unknown: &&&in nexus_alloc_resource, check 20, this is the child, needactivate = 0      requesting child is unknown because the name of device_t is empty.
nexus0: &&&in nexus_alloc_resource, after calling rman_reserve_resource, rv return Not NULL    memory addr successfully reserved again for this unknown device
…
…
nexus0: &&&in nexus_alloc_resource, check 10, this is the request bus, range (0xfe0000000-0xfffffffff), count 0x2000    A request to reserve 0x2000 byte in this address range
pcib0: &&&in nexus_alloc_resource, check 20, this is the child, needactivate = 2       the requestor is pcib0
vmbus0: ***in pcib_host_res_alloc, after calling bus_generic_alloc_resource, r = NULL      request failed
…
nexus0: &&&in nexus_alloc_resource, check 10, this is the request bus, range (0xf8000000-0xfed3ffff), count 0x2000    Pcib0 tries again in a different address range.
pcib0: &&&in nexus_alloc_resource, check 20, this is the child, needactivate = 2        the requestor is pcib0
vmbus0: ***in pcib_host_res_alloc, after calling bus_generic_alloc_resource, r = NULL      failed again because both of the address ranges were already reserved by ‘unknown’ device
…
pcib0: failed to get resource for cfg window    picb0 failed to attach, and hence all passthrough devices attached under this bus fail to attach and load.
----

I did a long binary search on head branch and finally narrow down to this commit r330113. The commit was not in 11 branch so passthrough on 11.2 VM still works fine. However 12 release got affected. I suspect this tiny change in ACPI causes the device address interpretation changed. 

Attached is acpidump output I did on a Hyper-V Gen 2 VM with one NIC configured to pass through to the VM.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-02-20 06:59:45 UTC
Thank you for your report Wei Hu.

base r330133 (MFC of base r328630) appears to be a minor man page change, unrelated to ACPI. Can you please confirm the revision the issue was bisected to?

Could you please also confirm the exact uname -a output of the system.
Comment 2 Conrad Meyer freebsd_committer freebsd_triage 2019-02-20 07:05:54 UTC
(In reply to Kubilay Kocak from comment #1)
https://svnweb.freebsd.org/base?view=revision&revision=r330113

(The correct number occurs below the typoed version in the report.)
Comment 3 John Baldwin freebsd_committer freebsd_triage 2019-02-21 22:49:06 UTC
Hmm, so I rechecked the spec and while it does say that bit 0 is ignored, the spec also includes QWordMemory which explicitly mentions the producer/consumer flag (e.g. 19.6.111).  Andrew, do you have the acpidump of the Cavium box that was affected by this?  It seems it was setting a reserved bit and relying on that reserved bit being ignored, whereas in this case Hyper-V and is setting the bit and relying on it being treated the same as it would be treated for other resource types?  Both are undefined behavior, but the Cavium behavior seems to be more weird (setting a reserved bit and hoping it is ignored).  Perhaps we can put '#ifdef __arm64__' around the res->Type part of the check?
Comment 4 Wei Hu 2019-02-26 04:06:02 UTC
(In reply to Kubilay Kocak from comment #1)
I am sorry for the typo. The commit which caused problem should be r330113. I just corrected the summary with the right commit number.
Comment 5 Andrew Turner freebsd_committer freebsd_triage 2019-02-27 14:40:22 UTC
I don't currently have access to that machine. Ed can you get the acpidump from the ThunderX2 box?
Comment 6 Ed Maste freebsd_committer freebsd_triage 2019-02-27 14:50:21 UTC
Created attachment 202411 [details]
TX2 asl
Comment 7 John Baldwin freebsd_committer freebsd_triage 2019-03-01 18:14:06 UTC
So this ASL is definitely using the flag explicitly.  None of them are in ExtendedAddressSpaces, and some of them appear to be correct (e.g. the Host-PCI bridge is using ResourceProducer in the way the non-arm-specific ACPI Host-PCI bridge driver expects: to list ranges it decodes that are then used by child devices).  It seems that a couple of the uses of ResourceProducer in this ASL are just wrong though.  First, numbers of times the flag is used vs not used:

> grep ResourceConsumer tx2.asl  | wc -l
      53
> grep ResourceProducer tx2.asl  | wc -l
      11

Of those 11 cases, SB.ECAM is probably a consumer, not a producer, though we already treat acpi_sysres devices special.

SB.PCI0 resources should be producers.  If the committed patch actually worked fully it would have broken booting completely since PCI0 would have claimed all the resources for all its children blocking all of them from being able to alloc bars.  We probably skip resources for PCI bridges which is why this might still be working.

The URT0 device's memory should be a consumer.  Note that the interrupt resource is a consumer, and other similar devices use consumer.  This is just firmware writer incompetence.

URT1, URT2, and URT3 are same as URT0.

PCI1 is same as PCI0.

So really, the real issue on the Cavium box is that the UART _CRS buffers have a typo.  I think the commit should be changed so that we just ignore 'ResourceProducer' on devices with the ARMH0011 _HID but otherwise leave it in place.
Comment 8 Ed Maste freebsd_committer freebsd_triage 2019-03-01 18:30:41 UTC
(In reply to John Baldwin from comment #7)

Note that the TX2 firmware is in active development (and I'm not completely certain what version is on it now). I will soon make sure it's running the latest firmware and we can see if the issue persists. If so we can submit a bug report to have it fixed.
Comment 9 John Baldwin freebsd_committer freebsd_triage 2019-03-01 19:11:20 UTC
Created attachment 202488 [details]
thunderx2_uart_workaround.patch

Here's an untested patch to restrict ignoring the Producer flag to the Cavium uarts.  I didn't do a full-blown quirk field and set of quirks since we just have one for now.  If we ever needed one in the future we could do that (maybe with a table of _HID values and quirk masks), but for now this change would be sufficient.  It's probably best to test this on the ThunderX2 first to make sure it works and then we can see if it fixes the issue on Hyper-V.
Comment 10 Ed Maste freebsd_committer freebsd_triage 2019-03-05 21:55:15 UTC
Created attachment 202621 [details]
tx2 asl from firmware 7.2
Comment 11 Ed Maste freebsd_committer freebsd_triage 2019-03-05 21:57:25 UTC
new asl from 7.2 firmware, the latest released
Comment 12 Ed Maste freebsd_committer freebsd_triage 2019-03-05 22:50:15 UTC
Unfortunately -current as of today (with this patch) is not booting on TX2 (mountroot failing), will try later if I get network boot going.
Comment 13 John Baldwin freebsd_committer freebsd_triage 2019-03-15 23:21:35 UTC
Ed, I think you told me on IRC that this patch booted ok for you on latest head?  If so, can we confirm it fixes Hyper V?
Comment 14 commit-hook freebsd_committer freebsd_triage 2019-04-09 21:18:34 UTC
A commit references this bug:

Author: jhb
Date: Tue Apr  9 21:18:03 UTC 2019
New revision: 346066
URL: https://svnweb.freebsd.org/changeset/base/346066

Log:
  Refine r330113 to honor the ProducerConsumer flag most of the time.

  While it is true that the ACPI spec says that the flag is only valid
  on Extended Address Space Descriptors, examples of other descriptors
  in the spec use the ProducerConsumer flag explicitly, and real
  hardware uses it as well.  In fact, even in the ASL of the Thunder X2
  for which r330113 was a workaround, some devices use this flag on
  non-Extended Address Space Descriptors correctly.  Instead, only
  ignore the flag for resources associated with the UART devices on the
  Thunder X2 using the "ARMH0011" HID to identify these devices.

  This should fix regressions from ignoring this flag in other contexts
  such as Hyper-V.

  PR:		235876
  Reported by:	Wei Hu <weh@microsoft.com>
  Tested by:	emaste (Thunder X2)
  MFC after:	2 weeks

Changes:
  head/sys/dev/acpica/acpi_resource.c
Comment 15 commit-hook freebsd_committer freebsd_triage 2019-05-01 18:18:37 UTC
A commit references this bug:

Author: jhb
Date: Wed May  1 18:17:43 UTC 2019
New revision: 347000
URL: https://svnweb.freebsd.org/changeset/base/347000

Log:
  MFC 346066: Refine r330113 to honor the ProducerConsumer flag most of the time.

  While it is true that the ACPI spec says that the flag is only valid
  on Extended Address Space Descriptors, examples of other descriptors
  in the spec use the ProducerConsumer flag explicitly, and real
  hardware uses it as well.  In fact, even in the ASL of the Thunder X2
  for which r330113 was a workaround, some devices use this flag on
  non-Extended Address Space Descriptors correctly.  Instead, only
  ignore the flag for resources associated with the UART devices on the
  Thunder X2 using the "ARMH0011" HID to identify these devices.

  This should fix regressions from ignoring this flag in other contexts
  such as Hyper-V.

  PR:		235876

Changes:
_U  stable/12/
  stable/12/sys/dev/acpica/acpi_resource.c