- Download Diff

Summary:

virtio: multiple fixes

Review Request #86 — Created Aug. 25, 2015 and submitted Oct. 20, 2015, 2:59 p.m.

Information
Owner:	avgapon
Repository:	illumos-gate
Branch:
Bugs:	6166
Depends On:
Commit:	dffde8c...
Reviewers
Groups:	general
People:

Description



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.

Testing Done

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

Issues

0
0
1
1
All Issues: 2

Description	From	Last Updated
On a non-DEBUG build, you get an empty-else, which some compilers or lint whine about. I'd change this to be ...	danmcd	Oct. 15, 2015, 4:17 p.m.
s/iterrupts/interrupts/	jeffpc	Oct. 15, 2015, 4:19 p.m.

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.

avgapon Aug. 26, 2015, 10:43 a.m.

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.

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

Show all issues

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;

avgapon Aug. 26, 2015, 10:42 a.m.

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.

jeffpc Aug. 31, 2015, 12:42 p.m.

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.

Description:

		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. Also, do not treat ddi_intr_get_cap failure as fatal. Not sure if this is correct, though.
	~	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.

Commit:

-	9d795529daee94ac4d59c61d19b1052ae416b7dc
+	918f3abd5eac99b32e33eec344bf5af801df1356

Diff:

Revision 2 (+48 -17)

Show changes

	usr/src/uts/common/io/virtio/virtio.c
	usr/src/uts/common/io/virtio/virtioreg.h
	usr/src/uts/common/io/virtio/virtiovar.h

Testing Done:

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

Testing Done:

		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

Bugs:

+	6166

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:

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?
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.)

avgapon Oct. 16, 2015, 12:02 p.m.

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).

jeffpc Oct. 16, 2015, 12:47 p.m.

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?

avgapon Oct. 16, 2015, 12:50 p.m.

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

jeffpc Oct. 16, 2015, 1:59 p.m.

Ok, I filed:

https://www.illumos.org/issues/6340 & https://www.illumos.org/issues/6341

usr/src/uts/common/io/virtio/virtio.c (Diff revision 2)
Show all issues
```
s/iterrupts/interrupts/
```

Ship it!

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

Change Summary:

address comments

Commit:

-	918f3abd5eac99b32e33eec344bf5af801df1356
+	dffde8c7f17120bf438035d7d23c2c6a10ceee8e

Diff:

Revision 3 (+50 -19)

Show changes

	usr/src/uts/common/io/virtio/virtio.c
	usr/src/uts/common/io/virtio/virtioreg.h
	usr/src/uts/common/io/virtio/virtiovar.h

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

virtio: multiple fixes

Description:

Commit:

Diff:

Testing Done:

Testing Done:

Bugs:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)