virtio_mmio: Support command-line parameters
ClosedPublic
Actions

Authored by cperciva on Aug 13 2022, 12:55 AM.

Details

Reviewers

kevans
imp
jrtc27
bryanv

Commits

rG0e1f5ab7db2c: virtio_mmio: Support command-line parameters

Summary

The Virtio MMIO bus driver was added in 2014 with support for devices
exposed via FDT; in 2018 support was added to discover Virtio MMIO
devices via ACPI tables, as in QEMU. The Firecracker VMM eschews both
FDT and ACPI, instead presenting device information via kernel command
line arguments of the form virtio_mmio.device=<parameters>.

These command line parameters get converted into kernel environment
variables; this adds support for parsing those variables and attaching
virtio_mmio children to nexus.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cperciva requested review of this revision.Aug 13 2022, 12:55 AM

cperciva created this revision.

cperciva added a parent revision: D36187: kern: Support duplicate variables in early kenv.

Harbormaster completed remote builds in B46918: Diff 109283.Aug 13 2022, 12:55 AM

cperciva added a reviewer: bryanv.Aug 13 2022, 12:58 AM

jrtc27 added inline comments.Aug 13 2022, 1:25 AM

sys/dev/virtio/mmio/virtio_mmio_cmdline.c
43	No space (same for p below)
47	Unsure if these should have better types rather than adopting the everything-is-a-long Linux-y approach (and please just call this one size)
82	ULONG_MAX is only an error if errno is ERANGE
108	child can be NULL. Also what happens if the same id is given twice?
110	This will work on x86 I believe but not on INTRNG architectures, where the IRQ IDs are virtualised and come from intr_map_irq (normally indirected via acpi/ofw_bus_map_intr). On those architectures you'd have to synthesise something and call the appropriate one for how the platform was booted. This interface is just kind of crap, there's no way to specify which interrupt controller the IRQ is for, what it's polarity is or whether it's level or edge-triggered, all things ACPI and FDT can do. I don't know what Linux does in that case, but as it stands this is horribly broken for most architectures, you'll pick up some random meaningless IRQ, if one even exists with that internal ID.
133	This is pretty sad. If they're guaranteed sequential you can avoid the upper limit. If not can you iterate over the environment instead?

I don't think you should use identify method.
I think you should just use a probe method that does the iterations and add children, then calls bus_probe_and_attach.
Identify methods have a lot of drawbacks and are generally a pita, plus the children wind up on nexus which is undesirable in most cases (since it's trivial to avoid in this case, there's no good reason to do that instead of probe/attach)

sys/dev/virtio/mmio/virtio_mmio_cmdline.c
76	Surely there's already a routine to do this stuff in the kernel....
108	No. child can't really be NULL. We're early enough in boot it will always succeed.

This revision now requires changes to proceed.Aug 13 2022, 3:20 PM

Which probe would you hook and where would you attach them other then nexus? There isn’t an acpi or ofwbus to hang them off in this case…

In D36189#821472, @jrtc27 wrote:

Which probe would you hook and where would you attach them other then nexus? There isn’t an acpi or ofwbus to hang them off in this case…

You would add this bus driver to nexus and then it would flow naturally from that.

But then how do you decide when to add it? Surely it then needs an identify method still? Admittedly you can then move some of the logic out, but still.

In D36189#821492, @jrtc27 wrote:

But then how do you decide when to add it? Surely it then needs an identify method still? Admittedly you can then move some of the logic out, but still.

Yes. The identify routine adds the bus instance to nexus0 and nothing else. Sorry if I wasn't clear about that. I'll make sure to drink more caffeine before answering.

cperciva added inline comments.Aug 13 2022, 3:52 PM

sys/dev/virtio/mmio/virtio_mmio_cmdline.c
47	I would have preferred to use `size_t` but we don't have any strtosize_t function available. Out the the types we could parse integers into, `unsigned long` seemed like the best option.
76	I thought there would be, but I couldn't find one! I think parsing stuff like this in the kernel is far less common than printing it.
82	We don't have errno in the kernel. There's no way to distinguish between an error and a "real" ULONG_MAX in libkern's strtoul... but it doesn't really matter here since a real ULONG_MAX means someone gave us nonsense anyway.
133	The upper limit is there to guard against buffer overflow in case someone gives us an insane kernel environment. Note that we stop scanning as soon as we find a name which isn't in the kernel environment; we (normally!) won't scan all the way up to 9999.

In D36189#821493, @imp wrote:

In D36189#821492, @jrtc27 wrote:

But then how do you decide when to add it? Surely it then needs an identify method still? Admittedly you can then move some of the logic out, but still.

Yes. The identify routine adds the bus instance to nexus0 and nothing else. Sorry if I wasn't clear about that. I'll make sure to drink more caffeine before answering.

If I understand correctly, you're talking about having a "vtmmio_cmdline" bus which has "virtio_mmio" devices attached to it?

I'm happy to rewrite this code, but my understanding of probe code is still quite limited; is there an example I can consult?

I believe that https://reviews.freebsd.org/D36914 will work and implements what I was suggesting.
Sorry for being so slow to put that into words / code.

In D36189#838285, @imp wrote:

I believe that https://reviews.freebsd.org/D36914 will work and implements what I was suggesting.
Sorry for being so slow to put that into words / code.

Thanks! I'll test this with all my other changes and commit assuming everything works as expected.

Note for the review history: imp's suggestion turned out to be a bit harder to implement than expected, so he agreed to having this version go into the tree with the possibility of reworking it at a later date.

what @cperciva said :)

This revision is now accepted and ready to land.Oct 14 2022, 7:35 PM

jrtc27 added inline comments.Oct 14 2022, 7:37 PM

sys/conf/files
3440	Linux has it as an off-by-default config option
sys/dev/virtio/mmio/virtio_mmio_cmdline.c
110	This still stands... ideally this would be marked broken for !INTRNG/!x86 as appropriate, otherwise you're building something that's known-broken