Page MenuHomeFreeBSD

virtio: Use a common class name between virtio_mmio and virtio_pci
Needs ReviewPublic

Authored by jrtc27 on Jan 7 2021, 11:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 9:51 AM
Unknown Object (File)
Tue, Mar 19, 4:46 AM
Unknown Object (File)
Sun, Mar 3, 11:14 AM
Unknown Object (File)
Feb 23 2024, 11:42 AM
Unknown Object (File)
Feb 22 2024, 2:22 AM
Unknown Object (File)
Jan 12 2024, 12:16 AM
Unknown Object (File)
Dec 21 2023, 11:15 AM
Unknown Object (File)
Dec 20 2023, 5:03 AM
Subscribers

Details

Reviewers
bryanv
imp
Summary

They both provide the same bus for child devices, the only difference is
how they implement the underlying transport, but this should not be
relevant to devices.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 36009
Build 32898: arc lint + arc unit

Event Timeline

@bryanv Does this seem sensible to you? It's a cleanup/simplification I've been meaning to do for a while as the current situation is a bit silly, unless there's a reason you can think of why this is a bad idea?

newbusly it looks good.

This revision is now accepted and ready to land.Jan 8 2021, 12:02 AM
sys/dev/virtio/balloon/virtio_balloon.c
160–162

These could then be combined into a single macro as a follow-up commit.

This would change the device names in places like devinfo, devctl, vmstat, etc, correct? Frankly, I prefer to keep some indication of the transport used in the device name. I have some WIP man pages changes to add a virtio_pci(4) page for modern-specific tunables, and add missing MLINK's to improve discoverability between device and kernel module names.

This has made me rethink D27929 as one step too far, and I'll effectively drop that patch and move them both under the virtio_pci bus.

I'm fine with folding the PNP into a single macro, and perhaps adding a VIRTIO_DRIVER_MODULE as well.

This would change the device names in places like devinfo, devctl, vmstat, etc, correct?

Of the transport, yes. The leaf device attached to that doesn't include the transport in it regardless. devinfo will still print the hierarchy so vtpci_driver instances have a pcibX above/next to them. The description of the transport device still includes the transport in it.

Frankly, I prefer to keep some indication of the transport used in the device name. I have some WIP man pages changes to add a virtio_pci(4) page for modern-specific tunables, and add missing MLINK's to improve discoverability between device and kernel module names.

Yeah, I'm personally torn. I do think cosmetically it's a nice to have, but it does still seem a bit silly to have two separate buses for what is the same interface.

This has made me rethink D27929 as one step too far, and I'll effectively drop that patch and move them both under the virtio_pci bus.

Ack, that's what reminded me that I wanted to do this (and I was otherwise going to suggest you do that).

I'm fine with folding the PNP into a single macro, and perhaps adding a VIRTIO_DRIVER_MODULE as well.

Should I open a review to do that change instead (with VIRTIO_SIMPLE_PNPINFO defining the table and containing both of the MODULE_PNP_INFO calls) and then leave this one as just a cosmetic change that can be bikeshed with less importance (and then only needs to touch the transports and virtio.h, not the device drivers themselves)?

For reference, this is what an AArch64 QEMU virt machine shows in devinfo when using PCI:

root@:/ # devinfo
nexus0
  acpi0
    cpu0
    uart0
    virtio0
    virtio1
    virtio2
    virtio3
    virtio4
    virtio5
    virtio6
    virtio7
    virtio8
    virtio9
    virtio10
    virtio11
    virtio12
    virtio13
    virtio14
    virtio15
    virtio16
    virtio17
    virtio18
    virtio19
    virtio20
    virtio21
    virtio22
    virtio23
    virtio24
    virtio25
    virtio26
    virtio27
    virtio28
    virtio29
    virtio30
    virtio31
    pcib0
      pci0
        hostb0
        virtio32
          vtblk0
        virtio33
          vtnet0
        virtio34
    pci_link0
    pci_link1
    pci_link2
    pci_link3
    acpi_sysresource0
    acpi_button0
    psci0
    gic0
    generic_timer0
  cryptosoft0
  efirtc0
root@:/ # devinfo -p vtblk0
vtblk0 virtio32 pci0 pcib0 acpi0 nexus0
root@:/ # devinfo -p vtblk0 -v
vtblk0 pnpinfo vendor=0x00001af4 device=0x1001 subvendor=0x1af4 device_type=0x00000002
 virtio32 pnpinfo vendor=0x1af4 device=0x1001 subvendor=0x1af4 subdevice=0x0002 class=0x010000 at slot=1 function=0 dbsf=pci0:0:1:0
 pci0
 pcib0 pnpinfo _HID=PNP0A08 _UID=0 _CID=PNP0A03 at handle=\_SB_.PCI0
 acpi0
 nexus0 pnpinfo nexus

As a separate thing I would like to update virtio_mmio to return ENXIO when probing instances with no device rather than creating pointless empty device instances as it's not particularly useful (and the AArch64 QEMU virt machine has a particularly large number of MMIO devices available for whatever reason which leads to rather silly device numbers).

This would change the device names in places like devinfo, devctl, vmstat, etc, correct?

Of the transport, yes. The leaf device attached to that doesn't include the transport in it regardless. devinfo will still print the hierarchy so vtpci_driver instances have a pcibX above/next to them. The description of the transport device still includes the transport in it.

Not all commands have the heirachy at hand, and I'd like to keep/improve alignment between man pages and device/module names.

Frankly, I prefer to keep some indication of the transport used in the device name. I have some WIP man pages changes to add a virtio_pci(4) page for modern-specific tunables, and add missing MLINK's to improve discoverability between device and kernel module names.

Yeah, I'm personally torn. I do think cosmetically it's a nice to have, but it does still seem a bit silly to have two separate buses for what is the same interface.

It has been this way for quite awhile and I think there is enough inertia to keep it. After more than a decade, the spec has 3 transports, and I don't forsee S/390 coming to FreeBSD anytime soon.

This has made me rethink D27929 as one step too far, and I'll effectively drop that patch and move them both under the virtio_pci bus.

Ack, that's what reminded me that I wanted to do this (and I was otherwise going to suggest you do that).

I'm fine with folding the PNP into a single macro, and perhaps adding a VIRTIO_DRIVER_MODULE as well.

Should I open a review to do that change instead (with VIRTIO_SIMPLE_PNPINFO defining the table and containing both of the MODULE_PNP_INFO calls) and then leave this one as just a cosmetic change that can be bikeshed with less importance (and then only needs to touch the transports and virtio.h, not the device drivers themselves)?

A new review for the PNP changes is fine

For reference, this is what an AArch64 QEMU virt machine shows in devinfo when using PCI:

root@:/ # devinfo
nexus0
  acpi0
    cpu0
    uart0
    virtio0
    virtio1
    virtio2
    virtio3
    virtio4
    virtio5
    virtio6
    virtio7
    virtio8
    virtio9
    virtio10
    virtio11
    virtio12
    virtio13
    virtio14
    virtio15
    virtio16
    virtio17
    virtio18
    virtio19
    virtio20
    virtio21
    virtio22
    virtio23
    virtio24
    virtio25
    virtio26
    virtio27
    virtio28
    virtio29
    virtio30
    virtio31
    pcib0
      pci0
        hostb0
        virtio32
          vtblk0
        virtio33
          vtnet0
        virtio34
    pci_link0
    pci_link1
    pci_link2
    pci_link3
    acpi_sysresource0
    acpi_button0
    psci0
    gic0
    generic_timer0
  cryptosoft0
  efirtc0
root@:/ # devinfo -p vtblk0
vtblk0 virtio32 pci0 pcib0 acpi0 nexus0
root@:/ # devinfo -p vtblk0 -v
vtblk0 pnpinfo vendor=0x00001af4 device=0x1001 subvendor=0x1af4 device_type=0x00000002
 virtio32 pnpinfo vendor=0x1af4 device=0x1001 subvendor=0x1af4 subdevice=0x0002 class=0x010000 at slot=1 function=0 dbsf=pci0:0:1:0
 pci0
 pcib0 pnpinfo _HID=PNP0A08 _UID=0 _CID=PNP0A03 at handle=\_SB_.PCI0
 acpi0
 nexus0 pnpinfo nexus

As a separate thing I would like to update virtio_mmio to return ENXIO when probing instances with no device rather than creating pointless empty device instances as it's not particularly useful (and the AArch64 QEMU virt machine has a particularly large number of MMIO devices available for whatever reason which leads to rather silly device numbers).

I think it is useful that a transport driver has been attached. Also my recollection when I original did it this way for PCI, is that this is needed for if the device (network, block, etc) module is later loaded. In the ensuing decade, devctl has appeared, but for driver development it is nice to just be able to load/unload the module for hack/test/hack.

For reference, this is what an AArch64 QEMU virt machine shows in devinfo when using PCI:

root@:/ # devinfo
nexus0
  acpi0
    cpu0
    uart0
    virtio0
    virtio1
    virtio2
    virtio3
    virtio4
    virtio5
    virtio6
    virtio7
    virtio8
    virtio9
    virtio10
    virtio11
    virtio12
    virtio13
    virtio14
    virtio15
    virtio16
    virtio17
    virtio18
    virtio19
    virtio20
    virtio21
    virtio22
    virtio23
    virtio24
    virtio25
    virtio26
    virtio27
    virtio28
    virtio29
    virtio30
    virtio31
    pcib0
      pci0
        hostb0
        virtio32
          vtblk0
        virtio33
          vtnet0
        virtio34
    pci_link0
    pci_link1
    pci_link2
    pci_link3
    acpi_sysresource0
    acpi_button0
    psci0
    gic0
    generic_timer0
  cryptosoft0
  efirtc0
root@:/ # devinfo -p vtblk0
vtblk0 virtio32 pci0 pcib0 acpi0 nexus0
root@:/ # devinfo -p vtblk0 -v
vtblk0 pnpinfo vendor=0x00001af4 device=0x1001 subvendor=0x1af4 device_type=0x00000002
 virtio32 pnpinfo vendor=0x1af4 device=0x1001 subvendor=0x1af4 subdevice=0x0002 class=0x010000 at slot=1 function=0 dbsf=pci0:0:1:0
 pci0
 pcib0 pnpinfo _HID=PNP0A08 _UID=0 _CID=PNP0A03 at handle=\_SB_.PCI0
 acpi0
 nexus0 pnpinfo nexus

As a separate thing I would like to update virtio_mmio to return ENXIO when probing instances with no device rather than creating pointless empty device instances as it's not particularly useful (and the AArch64 QEMU virt machine has a particularly large number of MMIO devices available for whatever reason which leads to rather silly device numbers).

I think it is useful that a transport driver has been attached. Also my recollection when I original did it this way for PCI, is that this is needed for if the device (network, block, etc) module is later loaded. In the ensuing decade, devctl has appeared, but for driver development it is nice to just be able to load/unload the module for hack/test/hack.

Oh I didn't mean if a driver wasn't available, I meant if the device ID is 0. If it's non-zero but no driver's available that would still attach ready for if the driver gets loaded later, either automatically by devd or manually by the user.

<snip>

As a separate thing I would like to update virtio_mmio to return ENXIO when probing instances with no device rather than creating pointless empty device instances as it's not particularly useful (and the AArch64 QEMU virt machine has a particularly large number of MMIO devices available for whatever reason which leads to rather silly device numbers).

I think it is useful that a transport driver has been attached. Also my recollection when I original did it this way for PCI, is that this is needed for if the device (network, block, etc) module is later loaded. In the ensuing decade, devctl has appeared, but for driver development it is nice to just be able to load/unload the module for hack/test/hack.

Oh I didn't mean if a driver wasn't available, I meant if the device ID is 0. If it's non-zero but no driver's available that would still attach ready for if the driver gets loaded later, either automatically by devd or manually by the user.

ENXIO is fine for MMIO DeviceId of 0x0. Currently the MMIO driver fails to conform to section 4.2.3.1.1 w.r.t. to this.

<snip>

As a separate thing I would like to update virtio_mmio to return ENXIO when probing instances with no device rather than creating pointless empty device instances as it's not particularly useful (and the AArch64 QEMU virt machine has a particularly large number of MMIO devices available for whatever reason which leads to rather silly device numbers).

I think it is useful that a transport driver has been attached. Also my recollection when I original did it this way for PCI, is that this is needed for if the device (network, block, etc) module is later loaded. In the ensuing decade, devctl has appeared, but for driver development it is nice to just be able to load/unload the module for hack/test/hack.

Oh I didn't mean if a driver wasn't available, I meant if the device ID is 0. If it's non-zero but no driver's available that would still attach ready for if the driver gets loaded later, either automatically by devd or manually by the user.

ENXIO is fine for MMIO DeviceId of 0x0. Currently the MMIO driver fails to conform to section 4.2.3.1.1 w.r.t. to this.

Yeah, and I can do magic at the same time (and move the version check to probe).

This revision now requires review to proceed.Jan 9 2021, 10:13 PM

I'd like to keep the current distinction - virito_pci and virtio_mmio - so it is clear as to what is being attached. After https://reviews.freebsd.org/D28073 there is much less boilerplate, and not likely another transport is forthcoming.