virtio: multiple fixes

Review Request #86 — Created Aug. 25, 2015 and submitted

  • virtio_register_msi: handle alloc/free size mismatch.

  • virtio_register_msi: do not destroy sc_intr_htable if everything goes well.
    Explicitly return success instead of falling through to the error handling code.

  • virtio_enable_msi: disable interrupts in out_bind error handling block.

  • virtio: do not switch to 24-byte offset until MSI-X mode is enabled:

    • be explicit that it is MSI-X mode that requires 24-byte offset, not MSI;
    • set 20-byte offset as early as possible;
    • remember the interrupt type in virtion_register_*, use it in virtio_enable_msi.

Tested in a bhyve VM configured to provide virtio-net and virtio-block devices.

$ echo '::interrupts -d' | mdb -k
IRQ  Vect IPL Bus    Trg Type   CPU Share APIC/INT# Driver Name(s)
1    0x42 5   ISA    Edg Fixed  0   1     0x0/0x1   i8042#1
4    0xb0 12  ISA    Edg Fixed  0   1     0x0/0x4   asy#1
9    0x80 9   PCI    Lvl Fixed  0   1     0x0/0x9   acpi_wrapper_isr
12   0x43 5   ISA    Edg Fixed  0   1     0x0/0xc   i8042#1
24   0x40 5   PCI    Edg MSI-X  0   1     -         vioblk#0
25   0x41 5   PCI    Edg MSI-X  0   1     -         vioblk#0
26   0x60 6   PCI    Edg MSI-X  0   1     -         vioif#2
27   0x61 6   PCI    Edg MSI-X  0   1     -         vioif#2
208  0xd0 14         Edg IPI    all 1     -         kcpc_hw_overflow_intr
209  0xd1 14         Edg IPI    all 1     -         cbe_fire
240  0xe0 15         Edg IPI    all 1     -         apic_error_intr
  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
  2. usr/src/uts/common/io/virtio/virtio.c (Diff revision 1)

    I'd follow the principle of least surprise and return the error that ddi_intr_get_cap() returns if it fails (including a goto out_add_handlers).

    This is my opinion, but I'm happy to be overruled by others.

    1. Better safe than sorry. Also, from a sample of a few other drivers that I looked at all of them treat the error from ddi_intr_get_cap() as fatal.
      So, I am updating the patch.

  3. usr/src/uts/common/io/virtio/virtio.c (Diff revision 1)

    On a non-DEBUG build, you get an empty-else, which some compilers or lint whine about.

    I'd change this to be less-linty:

    ASSERT(sc->sc_int_type == DDI_INTR_TYPE_MSI ||
    sc->sc_int_type == DDI_INTR_TYPE_MSIX);
    if (sc->sc_int_type == DDI_INTR_TYPE_MSIX)
    sc->sc_config_offset = VIRTIO_CONFIG_DEVICE_CONFIG_MSIX;

    1. I believe that non-DEBUG ASSERT expands to (void)0 and so the else branch is not empty.
      I think that I've encountered such a code pattern in the illumos code before.
      Lastly, I do not get any compilation warnings in either DEBUG or non-DEBUG build in my environment.

    2. Andriy is right, the branch will not be empty. If lint doesn't complain, great - I much prefer the current version because the condition is clearer. BTW, feel free to use ASSERT3U instead to make potential panic messages include the expected and actual values.

  1. All in all, good changes. Since you looked at this code long enough to figure out all these bugs, I have a couple of questions not related to your changes:

    1. virtio_enable_msi() seems to be lying in the error messages about falling back to INTx. It seems like it'll not try to fall back, but rather just errors out. Am I missing something here?

    2. virtio_register_msi() doesn't seem to try hard enough with MSI. Specifically, suppose the system supports MSIX, MSI, and FIXED. virtio_register_msi() will try to register MSIX interrupts. Suppose that fails. Instead of moving onto the next preferred one (MSI), it bails. Then virtio_register_ints() tries to salvage the situation by calling virtio_register_intx() to register FIXED interrupts. This seems suboptimal. Shouldn't it try to get MSI before totally giving up on MSI-type interrupt? (Note: I just know that MSIX is better than MSI which is better than FIXED. I am not a driver expert.)

    1. I completely agree on both points. I think that those issues could be addressed as follow-up changes (just trying to avoid "while you are there..." situation).

    2. Yeah, sorry. I didn't mean to imply you should address those here. Are you going to attack those two points "soon" (as a separate review) or should I file bugs for those?

    3. I might not get time to work on those soon-ish, so it's probably better to open tickets.
      Thank you for the review!

    4. Ok, I filed: &

  2. usr/src/uts/common/io/virtio/virtio.c (Diff revision 2)


  1. Other than Josef's comments, this change looks great to me.

  1. Ship It!
Review request changed

Status: Closed (submitted)