bhyve: Move legacy PCI interrupt handling under amd64/
ClosedPublic
Actions

Authored by markj on Jun 23 2023, 10:03 PM.

Details

Reviewers

jhb
corvink

Group Reviewers

bhyve

Commits

rGa862ee32ca8b: bhyve: Move legacy PCI interrupt handling under amd64/
rG55c13f6e7a41: bhyve: Move legacy PCI interrupt handling under amd64/

Summary

Specifically, move IO-APIC, LPC and PIRQ routing code under amd64/.

Use ifdefs to conditionally compile related code in other files. In
particular, legacy PCI interrupt handling is now compiled only on amd64.
This is not too invasive, but suggestions for a more modular approach
would be appreciated.

I am not sure why qemu fwcfg handling is tied to LPC, and I suspect it
should be decoupled. In this commit I just apply an ifdef hammer, but
we will eventually want fwcfg on arm64 as well.

No functional change intended.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 52273
Build 49164: arc lint + arc unit

Event Timeline

markj created this revision.Jun 23 2023, 10:03 PM

Herald added a reviewer: bhyve. · View Herald TranscriptJun 23 2023, 10:03 PM

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

markj requested review of this revision.Jun 23 2023, 10:03 PM

markj added a parent revision: D40738: bhyve: Compile some device models only on amd64.

markj added a child revision: D40740: bhyve: Conditionally compile framebuffer-related code.

Harbormaster completed remote builds in B52273: Diff 123703.Jun 23 2023, 10:03 PM

corvink added inline comments.Jun 26 2023, 6:24 AM

usr.sbin/bhyve/pci_emul.h
129	This ifdef isn't required. Especially, as legacy interrupts could be implemented by arm64 too. In general, I'd like to avoid ifdefs as much as possible. However, I don't have a strong preference here.
usr.sbin/bhyve/qemu_fwcfg.c
431	fwcfg is mainly used for hypervisor <-> guest firmware communication. That's why it's tied to the bootrom option which is tied to the lpc. Btw: I was told to do it that way. See https://reviews.freebsd.org/D31578#712764

corvink accepted this revision.Jun 26 2023, 6:25 AM

This revision is now accepted and ready to land.Jun 26 2023, 6:25 AM

corvink added inline comments.Jun 26 2023, 6:27 AM

usr.sbin/bhyve/qemu_fwcfg.c
433	I do think that we want to have fwcfg for arm64 too. Maybe add a comment which explains that we want to have fwcfg for arm64 but that we can't enable it yet because we do not implement the mmio interface yet.

For fwcfg, it's an odd quirk of x86 that the bootrom functionality is hung off of the lpc layer. We may want to adjust the configuration of this such that fwcfg and bootroom are configured at the top-level instead of under lpc at least on arm64, but that can wait until we are a bit further along I think.

usr.sbin/bhyve/pci_emul.c
1778–1779
usr.sbin/bhyve/pci_emul.h
129	I think it's fine to #ifdef this. I would be tempted perhaps to have a helper macro that is something like 'PCI_LINTR` that is only defined on platforms that support legacy interrupts, but I don't think we are likely to support them on either arm64 or RISC-V, so making it all conditional on only amd64 is fine (unless PCI_LINTR would be more readable).

markj marked 3 inline comments as done.Jul 5 2023, 2:25 PM

markj added inline comments.

usr.sbin/bhyve/pci_emul.h
129	A more abstract PCI_LINTR symbol would be more readable, but this code is also tied to amd64-specific interfaces (e.g., from ioapic.h), so I'm on the fence here. I think I'll just leave it as is. In general, I'd like to avoid ifdefs as much as possible. Me too, at least most of the ifdefs in bhyverun.c will go away once the file is split into MD components.
usr.sbin/bhyve/qemu_fwcfg.c
431	We could also repurpose the `-l` option on arm64 to mean something other than "LPC devices". It can be used for generic platform configuration variables, e.g., `bootrom` and `fwcfg`.
433	Indeed, I'll add a comment.