Page MenuHomeFreeBSD
Differential D21948

Support hotplug of PCI devices on EC2.
ClosedPublic
Actions

Authored by jhb on Oct 8 2019, 9:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 4:09 PM
Unknown Object (File)
Thu, Nov 21, 10:21 AM
Unknown Object (File)
Thu, Nov 21, 9:40 AM
Unknown Object (File)
Tue, Nov 19, 10:06 PM
Unknown Object (File)
Tue, Nov 19, 5:43 AM
Unknown Object (File)
Fri, Nov 15, 3:48 AM
Unknown Object (File)
Thu, Nov 14, 6:45 AM
Unknown Object (File)
Tue, Nov 12, 11:08 AM
View All Files
Subscribers
cperciva
imp
scottl
PCI

Details

Reviewers
imp
cperciva
Commits
rS353587: Install an ACPI PCI bus notify handler.
rS353586: Support hot insertion and removal of PCI devices on EC2.
rS353585: Export pci_attach() and pci_detach().
Summary

Install notify handlers for child handles of ACPI PCI buses.
Only handles with an _EJ0 method are considered hotpluggable.

Test Plan
  • waiting for Colin to test this. I sent an earlier patch but it was incomplete as it was missing the changes in sys/dev/pci

Diff Detail

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

Event Timeline

jhb created this revision.Oct 8 2019, 9:38 PM
Herald added a subscriber: imp. · View Herald TranscriptOct 8 2019, 9:38 PM
Harbormaster completed remote builds in B26937: Diff 63059.Oct 8 2019, 9:38 PM
jhb updated this revision to Diff 63062.Oct 8 2019, 10:09 PM

Re-add the bus notify handler to see if that helps Scott's use case.

Harbormaster completed remote builds in B26938: Diff 63062.Oct 8 2019, 10:09 PM
cperciva added a comment.Oct 8 2019, 10:38 PM

Running with

sysctl debug.acpi.level="ACPI_LV_INFO"
sysctl debug.acpi.layer="ACPI_ALL_COMPONENTS ACPI_ALL_DRIVERS"

when I attach a new disk I see in /var/log/messages:

Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446
Oct 8 22:37:20 freebsd kernel: ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler:
Oct 8 22:37:20 freebsd kernel: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler:
Oct 8 22:37:20 freebsd kernel: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler:
Oct 8 22:37:20 freebsd kernel: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446
Oct 8 22:37:20 freebsd kernel: ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler:
Oct 8 22:37:20 freebsd kernel: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446
Oct 8 22:37:20 freebsd kernel: ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446
Oct 8 22:37:20 freebsd kernel: ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler:
Oct 8 22:37:20 freebsd kernel: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446
Oct 8 22:37:20 freebsd kernel: ExSystemIoSpaceHandler:
Oct 8 22:37:20 freebsd kernel: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler:
Oct 8 22:37:20 freebsd kernel: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: evmisc-0267 EvQueueNotifyRequest : No notify handler for Notify, ignoring (S1E_, 1) node 0xfffff80003f14ec0
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE00
Oct 8 22:37:20 freebsd kernel: exregion-0446 ExSystemIoSpaceHandler: System-IO (width 32) R/W 0 Address=000000000000AE04

The important part here I assume is "EvQueueNotifyRequest : No notify handler for Notify, ignoring (S1E_, 1)" -- the notify handler doesn't seem to be installed. I have the instance running; how should I debug this?

jhb added inline comments.Oct 8 2019, 10:58 PM
sys/dev/acpica/acpi_pci.c
378 ↗(On Diff #63062)

Maybe add a printf here, something like:

printf("%s: installing notify handler on %s\n", __func__,
    __func__, acpi_name(handle));

If it never prints, then maybe trace up above where we fail if _EJ0 isn't found to see if that check is always failing.

cperciva added a comment.Oct 8 2019, 11:21 PM

With an added printf I see:

Oct 8 23:14:20 freebsd kernel: acpi_pci_install_device_notify_handler: installing handler on \_SB_.PCI0.S1E_
...
Oct 8 23:18:59 freebsd kernel: evmisc-0267 EvQueueNotifyRequest : No notify handler for Notify, ignoring (S1E_, 1) node 0xfffff80003d15ec0

So... it looks like we're at least trying to install a handler here...

jhb updated this revision to Diff 63065.Oct 8 2019, 11:35 PM

Use ACPI_SYSTEM_NOTIFY instead of ACPI_DEVICE_NOTIFY.

Harbormaster completed remote builds in B26941: Diff 63065.Oct 8 2019, 11:35 PM
cperciva added a comment.Oct 8 2019, 11:54 PM

Thanks, with that fix the Notify handler is getting called:

evmisc-0290 EvQueueNotifyRequest  : Dispatching Notify on [S1E_] (Device) Value 0x01 (Device Check) Node 0xfffff80003e42ec0

panic: mutex Giant not owned at /usr/src/sys/kern/subr_bus.c:2919
cpuid = 3
time = 1570578746
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe001afe80b0
vpanic() at vpanic+0x19d/frame 0xfffffe001afe8100
panic() at panic+0x43/frame 0xfffffe001afe8160
mtx_assert() at mtx_assert+0xb4/frame 0xfffffe001afe8170
device_probe_and_attach() at device_probe_and_attach+0x2a/frame 0xfffffe001afe81a0
pci_rescan_method() at pci_rescan_method+0x4f5/frame 0xfffffe001afe8220
AcpiEvNotifyDispatch() at AcpiEvNotifyDispatch+0x50/frame 0xfffffe001afe8240
acpi_task_execute() at acpi_task_execute+0x10/frame 0xfffffe001afe8260
taskqueue_run_locked() at taskqueue_run_locked+0x10c/frame 0xfffffe001afe82c0
taskqueue_thread_loop() at taskqueue_thread_loop+0x88/frame 0xfffffe001afe82f0
fork_exit() at fork_exit+0x84/frame 0xfffffe001afe8330
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe001afe8330

  • trap 0, rip = 0, rsp = 0, rbp = 0 ---

... maybe not exactly the right way, though.

jhb updated this revision to Diff 63067.Oct 9 2019, 12:21 AM

Lock Giant around BUS_RESCAN.

Harbormaster completed remote builds in B26943: Diff 63067.Oct 9 2019, 12:21 AM
imp added a comment.Oct 9 2019, 12:32 AM

Looking giant around bus scan is, sadly, still required. Yea asserts I added :)

imp accepted this revision.Oct 9 2019, 12:36 AM

The newbus side of this looks good

This revision is now accepted and ready to land.Oct 9 2019, 12:36 AM
cperciva added a comment.Oct 9 2019, 12:54 AM

I can now attach a disk:

Oct 9 00:51:33 freebsd kernel: evmisc-0290 EvQueueNotifyRequest : Dispatching Notify on [S1E_] (Device) Value 0x01 (Device Check) Node 0xfffff80003d04ec0
Oct 9 00:51:33 freebsd kernel: nvme2: <Generic NVMe Device> at device 30.0 on pci0
Oct 9 00:51:34 freebsd kernel: nvme2: temperature threshold not supported
Oct 9 00:51:34 freebsd kernel: nvd2: <Amazon Elastic Block Store> NVMe namespace
Oct 9 00:51:34 freebsd kernel: nvd2: 102400MB (209715200 512 byte sectors)

But when I try to detach it:
Oct 9 00:52:46 freebsd kernel: EvQueueNotifyRequest : Dispatching Notify on [S1E_] (Device) Value 0x01 (Device Check) Node 0xfffff80003d04ec0
Oct 9 00:52:46 freebsd kernel: EvQueueNotifyRequest : Dispatching Notify on [S1E_] (Device) Value 0x03 (Eject Request) Node 0xfffff80003d04ec0
Oct 9 00:52:46 freebsd kernel: pci0: unknown notify 0x3

Which suggests that (a) we need to handle 0x03 (as expected), and also that maybe we need to do something to acknowledge the notification, since the disk *attachment* notification seems to be arriving a second time here.

scottl added a comment.EditedOct 9 2019, 4:06 PM

The patch works as advertised right now. A couple of observations for my case:

  • Alpine Ridge TBT3 adapter, USB-A dongle with USB memory stick attached
  • acpi_pci_bus_notify_handler() gets the DEVICE_BUS_CHECK notify events from the hotplug SMI for both insertion and removal, as expected.
  • acpi_pci_device_notify_handler() gets no notify events
  • No "EJECT_REQUEST" events are delivered

The relevant ASL (I think) is here:

Method (HPME, 0, Serialized)
{
    If (((VDID != 0xFFFFFFFF) && (PMSX == 0x01)))
    {
        Notify (PXSX, 0x02) // Device Wake
        PMSX = 0x01
        PSPX = 0x01
    }
}
jhb updated this revision to Diff 63094.Oct 9 2019, 7:50 PM

Add an in-kernel eject handler. We might replace this with a devctl_notify
that triggers events in userland instead, but this might work sufficiently
for now.

This revision now requires review to proceed.Oct 9 2019, 7:50 PM
Harbormaster completed remote builds in B26963: Diff 63094.Oct 9 2019, 7:50 PM
cperciva added a comment.Oct 9 2019, 8:35 PM

Patch does not compile.

sys/dev/acpica/acpi_pci.c
367 ↗(On Diff #63094)

This should be ACPI_NOTIFY_EJECT_REQUEST.

368 ↗(On Diff #63094)

There is no "handle" in scope. Should it be "h" instead?

cperciva added a comment.Oct 9 2019, 8:48 PM

With the above changes (s/ACPI_NOTIFY_DEVICE_EJECT_REQUEST/ACPI_NOTIFY_EJECT_REQUEST/ and s/handle/h/) I can attach a disk and detach it, but I get

Oct 9 20:44:48 freebsd kernel: ACPI Warning: \_SB.PCI0.S1E._EJ0: Insufficient arguments - Caller passed 0, ACPI requires 1 (20190703/nsarguments-414)

and after detaching a disk I can't reattach it (even devctl rescan pci0 doesn't work) -- not sure if this is because the failed call to _EJ0 broke something, or if the way we're detaching the device is preventing it from appearing in rescan after it is reattached.

jhb updated this revision to Diff 63100.Oct 9 2019, 9:16 PM
  • Fix compile issues
  • Pass the value 1 to _EJ0 after checking the ACPI 6.2 spec. Also, the spec says that for devices with _ADR (PCI), the bus driver should verify the device is removed after _EJ0 returns, so just trigger a BUS_RESCAN to delete the child device after _EJ0.
Harbormaster completed remote builds in B26967: Diff 63100.Oct 9 2019, 9:16 PM
cperciva accepted this revision.Oct 9 2019, 10:14 PM

This is now working perfectly in my tests on EC2. When I attach a disk it shows up immediately; when I detach a disk it goes away from FreeBSD and EC2 recognizes that it has successfully detached (and without needing a "force detach"); and I can cycle the same disk attached/detached many times without issues.

As far as I'm concerned, this is ready for commit. My only regret is that it might be too late for FreeBSD 12.1.

This revision is now accepted and ready to land.Oct 9 2019, 10:14 PM
cperciva added a comment.Oct 9 2019, 10:21 PM

I wonder... if we add a "hw.pci.acpi_hotplug" boot tunable and check it in acpi_pci_{attach,detach} before doing anything, could we get this merged for 12.1? I would turn that tunable on in EC2 AMIs (since I've tested it there) but we could leave it turned off by default elsewhere.

jhb added a comment.Oct 9 2019, 11:39 PM

For committing purposes I will probably split this up into a few commits:

  1. expose pci_attach/pci_detach
  2. add the "bus" notifications that work for Scott's use case
  3. add the "device" notifications using _EJ0 for EC2
Closed by commit rS353585: Export pci_attach() and pci_detach(). (authored by jhb). · Explain WhyOct 15 2019, 6:58 PM
This revision was automatically updated to reflect the committed changes.
jhb added a commit: rS353585: Export pci_attach() and pci_detach()..
jhb marked 2 inline comments as done.Oct 15 2019, 7:00 PM
jhb added a commit: rS353586: Support hot insertion and removal of PCI devices on EC2..Oct 15 2019, 7:04 PM
jhb added a commit: rS353587: Install an ACPI PCI bus notify handler..Oct 15 2019, 7:12 PM

Revision Contents
Changeset List

PathSize
head/
sys/
dev/
pci/
6 lines
2 lines

Diff 63316

View Options

head/sys/dev/pci/pci.c

Loading...
View Options

head/sys/dev/pci/pci_private.h

Loading...