virtio_mmio: Support command-line parameters

[cperciva]

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.

[jrtc27]

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?

[imp]

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.

[jrtc27]

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…

[imp]

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.

[jrtc27]

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.

[imp]

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]

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.

[cperciva]

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?

[imp]

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.

[cperciva]

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.

[cperciva]

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.

[imp]

what @cperciva said :)

[jrtc27]

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

[cperciva]

Maybe I should put this into files.x86 instead of files? If it's not likely to work on !x86 systems... @jrtc27 what do you think?

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