Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add QEMU pvpanic ISA device #596

Merged
merged 24 commits into from
Jan 12, 2024
Merged

add QEMU pvpanic ISA device #596

merged 24 commits into from
Jan 12, 2024

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jan 9, 2024

This branch adds support for the pvpanic virtual device implemented by
QEMU. This device allows guests to report kernel panics to the
hypervisor. In propolis-server, guest-reported kernel panics are
handled by incrementing an Oximeter metric.

The pvpanic device can be exposed to the guest either as an ISA bus I/O
port device or as a PCI bus device. This branch implements the ISA bus
device. I'd like to also add a PCI pvpanic device, but will implement
that in a subsequent pull request.

In order for the guest to detect the ISA bus pvpanic device, it's
necessary to add an entry for the panic device to the ACPI DSDT table.
This is the AML that QEMU adds to its DSDT when the ISA bus pvpanic
device is enabled:

//
// QEMU panic device
//
Device (PEVT)
{
    Name (_HID, "QEMU0001")  // _HID: Hardware ID
    Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
    {
        IO (Decode16,
            0x0505,             // Range Minimum
            0x0505,             // Range Maximum
            0x01,               // Alignment
            0x01,               // Length
            )
    })
    OperationRegion (PEOR, SystemIO, 0x0505, One)
    Field (PEOR, ByteAcc, NoLock, Preserve)
    {
        PEPT,   8
    }

    Name (_STA, 0x0F)  // _STA: Status
    Method (RDPT, 0, NotSerialized)
    {
        Local0 = PEPT /* \_SB_.PCI0.S08_.PEVT.PEPT */
        Return (Local0)
    }

    Method (WRPT, 1, NotSerialized)
    {
        PEPT = Arg0
    }
}

This means that in order for guests to use this device, we need to boot
with an ACPI table that contains this entry. For testing purposes, I
modified EDK2 OVMF to add this entry to the DSDT. In the future, though,
we'll likely want Propolis to generate ACPI tables dynamically on boot
based on the instance spec.

The EDK2 changes I used for testing this are available here.

To test this change, I ran propolis-standalone with an Alpine Linux
3.19 guest,, and the following device added to the VM config file:

[dev.pvpanic]
driver = "qemu-pvpanic"
enable_mmio = true

The guest correctly detects the panic device and loads the appropriate
kernel module. If I then trigger a panic in the guest using SysRq, like
this:

$ echo 1 > /proc/sys/kernel/sysrq
$ echo c > /proc/sysrq-trigger

The guest crashes, and propolis-standalone logs:

dev: pvpanic
 Jan 11 18:14:13.494 DEBG guest kernel panic, guest_handled: false, host_handled: true

Closes #592

Copy link
Collaborator

@pfmooney pfmooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for drawing this up.

lib/propolis/src/hw/qemu/pvpanic.rs Outdated Show resolved Hide resolved
lib/propolis/src/hw/qemu/pvpanic.rs Outdated Show resolved Hide resolved
lib/propolis/src/hw/qemu/pvpanic.rs Outdated Show resolved Hide resolved
openapi/propolis-server.json Show resolved Hide resolved
lib/propolis/src/hw/qemu/pvpanic.rs Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license-eye has totally checked 254 files.

Valid Invalid Ignored Fixed
189 1 64 0
Click to see the invalid file list
  • bin/propolis-server/src/lib/stats/pvpanic.rs

bin/propolis-server/src/lib/stats/pvpanic.rs Show resolved Hide resolved
@hawkw hawkw force-pushed the eliza/pvpanic branch 2 times, most recently from 52af7ae to c189f9a Compare January 10, 2024 20:22
@hawkw hawkw changed the title [WIP] add QEMU PVPANIC device add QEMU pvpanic-mmio device Jan 11, 2024
@hawkw hawkw marked this pull request as ready for review January 11, 2024 19:21
@hawkw hawkw requested review from pfmooney and gjcolombo January 11, 2024 19:21
@hawkw hawkw changed the title add QEMU pvpanic-mmio device add QEMU pvpanic ISA device Jan 11, 2024
Copy link
Contributor

@gjcolombo gjcolombo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good! A few comments on how we want the instance spec side of this to look:

Copy link
Contributor

@gjcolombo gjcolombo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one small tweak. (I didn't leave any additional comments on it, but the device emulation also seems fine to me, modulo any other feedback @pfmooney might have.)

Thanks for putting this together!

crates/propolis-api-types/src/instance_spec/v0/mod.rs Outdated Show resolved Hide resolved
hawkw added a commit to oxidecomputer/edk2 that referenced this pull request Jan 12, 2024
This branch adds a DSDT entry for the QEMU pvpanic virtual device to OVMF.

This is necessary for oxidecomputer/propolis#596 to actually work.
@hawkw hawkw merged commit 7828d9c into master Jan 12, 2024
10 checks passed
@hawkw hawkw deleted the eliza/pvpanic branch January 12, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

want pvpanic device
3 participants