Page MenuHomeFreeBSD
Differential D29084

bhyve: virtio shares definitions between sys/dev/virtio
ClosedPublic
Actions

Authored by khng on Mar 5 2021, 3:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 6:50 PM
Unknown Object (File)
Mon, Nov 18, 12:32 AM
Unknown Object (File)
Sun, Nov 17, 10:35 PM
Unknown Object (File)
Fri, Nov 15, 1:41 AM
Unknown Object (File)
Sun, Nov 10, 1:46 PM
Unknown Object (File)
Sun, Nov 3, 11:19 AM
Unknown Object (File)
Oct 22 2024, 5:50 PM
Unknown Object (File)
Oct 22 2024, 4:11 AM
View All Files
Subscribers
afedorov
bcran
grehan
imp
rew
rgrimes
Contributor Reviews (src)

Details

Reviewers
lwhsu
ygy
grehan
philip
Group Reviewers
bhyve
Commits
rGddf1b5ac761e: bhyve: virtio shares definitions between sys/dev/virtio
rG54ac6f721efc: bhyve: virtio shares definitions between sys/dev/virtio
Summary

Definitions inside usr.sbin/bhyve/virtio.h are thrown away.
Definitions in sys/dev/virtio are used instead.

This reduces code duplication.

Submitted by: Ka Ho Ng <khng@freebsdfoundation.org>
Sponsored by: The FreeBSD Foundation

Diff Detail

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

Event Timeline

khng created this revision.Mar 5 2021, 3:33 PM
Herald added a reviewer: bhyve. · View Herald TranscriptMar 5 2021, 3:34 PM
Herald added subscribers: Contributor Reviews (src), bcran, rgrimes, imp. · View Herald Transcript
khng requested review of this revision.Mar 5 2021, 3:34 PM
Harbormaster completed remote builds in B37601: Diff 85204.Mar 5 2021, 3:34 PM
khng added reviewers: lwhsu, ygy.Mar 5 2021, 3:34 PM
khng updated this revision to Diff 85207.Mar 5 2021, 3:40 PM

VRING_AVAIL_F_NO_INTERRUPT and VRING_USED_F_NO_NOTIFY can also be removed

Harbormaster completed remote builds in B37604: Diff 85207.Mar 5 2021, 3:40 PM
khng updated this revision to Diff 85238.Mar 5 2021, 6:44 PM

Replace VTCFG_R with VIRTIO_PCI_/VIRTIO_MSI_ from sys/dev/virtio/pci/virtio_pci_legacy_var.h

Harbormaster completed remote builds in B37625: Diff 85238.Mar 5 2021, 6:44 PM
khng updated this revision to Diff 85240.Mar 5 2021, 6:51 PM

Comment fix

Harbormaster completed remote builds in B37627: Diff 85240.Mar 5 2021, 6:52 PM
afedorov added a subscriber: afedorov.Mar 5 2021, 8:28 PM
rew added a subscriber: rew.Mar 6 2021, 3:15 AM

take my suggestions/questions with a grain of salt as I'm not familiar with this area of the code..

fundamentally though, this looks like a good change.

sys/dev/virtio/virtio.h
33–34

I got this error when compiling bhyve:

...[ clipped ] ...
-Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter -Qunused-arguments -c /usr/src/usr.sbin/bhyve/pci_virtio_rnd.c -o pci_virtio_rnd.o
In file included from /usr/src/usr.sbin/bhyve/pci_virtio_rnd.c:63:
In file included from /usr/src/usr.sbin/bhyve/virtio.h:36:
In file included from /usr/src/sys/dev/virtio/virtio.h:34:
/usr/src/sys/dev/virtio/virtio_endian.h:42:15: error: unknown type name 'bool'
static inline bool
[... clipped ...]
(9 errors generated)

fixed the error: https://reviews.freebsd.org/differential/diff/85257/

I provided the diff for context since virtio_endian.h isn't in the review

usr.sbin/bhyve/virtio.h
132–159

I wonder why these were __packed, but their corresponding structs in virtio_ring.h aren't?

Only a question, not a suggestion for change.

187–188

vring_size() returns an int

khng updated this revision to Diff 85260.Mar 6 2021, 6:45 AM
khng marked 2 inline comments as done.
  • Fix rew@'s compilation error
  • vring_size_aligned()'s return type should match vring_size()'s
Harbormaster completed remote builds in B37636: Diff 85260.Mar 6 2021, 6:45 AM
khng added inline comments.Mar 6 2021, 6:48 AM
usr.sbin/bhyve/virtio.h
132–159

These __packed does seem unnecessary to me. They belonged to 366f60834ff8e which was the first commit introducing usr.sbin/bhyve/virtio.h.

khng updated this revision to Diff 85264.Mar 6 2021, 11:01 AM

Reuse ISR status flags from sys/dev/virtio

Harbormaster completed remote builds in B37637: Diff 85264.Mar 6 2021, 11:01 AM
grehan accepted this revision.Mar 6 2021, 11:10 AM
grehan added a subscriber: grehan.

Thanks for doing this; a long overdue change.

This revision is now accepted and ready to land.Mar 6 2021, 11:10 AM
khng updated this revision to Diff 85274.Mar 6 2021, 4:40 PM

D29084?id=85260 was accidentally reverted.

This revision now requires review to proceed.Mar 6 2021, 4:40 PM
Harbormaster completed remote builds in B37640: Diff 85274.Mar 6 2021, 4:40 PM
grehan accepted this revision.Mar 6 2021, 8:57 PM
This revision is now accepted and ready to land.Mar 6 2021, 8:57 PM
khng planned changes to this revision.Mar 7 2021, 7:04 PM
khng updated this revision to Diff 85303.Mar 7 2021, 7:09 PM

vring_size has to be further aligned. Do it in vring_size_aligned now.

See 2.6.2 Legacy Interfaces: A Note on Virtqueue Layout

Harbormaster completed remote builds in B37660: Diff 85303.Mar 7 2021, 7:09 PM
khng added a comment.Mar 7 2021, 7:13 PM
In D29084#652080, @khng300_gmail.com wrote:

vring_size has to be further aligned. Do it in vring_size_aligned now.

See 2.6.2 Legacy Interfaces: A Note on Virtqueue Layout

Thanks rew@ for pointing that out.

grehan accepted this revision.Mar 7 2021, 11:05 PM

Good catch rew :)

This revision is now accepted and ready to land.Mar 7 2021, 11:05 PM
khng added a reviewer: philip.Mar 16 2021, 11:11 AM
philip accepted this revision.Mar 16 2021, 11:15 AM

Approved by: philip (mentor)

Closed by commit rG54ac6f721efc: bhyve: virtio shares definitions between sys/dev/virtio (authored by khng). · Explain WhyMar 16 2021, 11:31 AM
This revision was automatically updated to reflect the committed changes.
khng added a commit: rG54ac6f721efc: bhyve: virtio shares definitions between sys/dev/virtio.
khng added a commit: rGddf1b5ac761e: bhyve: virtio shares definitions between sys/dev/virtio.Aug 12 2021, 6:46 AM

Revision Contents
Changeset List

PathSize
sys/
dev/
virtio/
4 lines
3 lines
usr.sbin/
bhyve/
2 lines
2 lines
2 lines
2 lines
3 lines
2 lines
137 lines
147 lines

Diff 85260

View Options

sys/dev/virtio/virtio.h

Loading...
View Options

sys/dev/virtio/virtio_endian.h

Loading...
View Options

usr.sbin/bhyve/pci_virtio_9p.c

Loading...
View Options

usr.sbin/bhyve/pci_virtio_block.c

Loading...
View Options

usr.sbin/bhyve/pci_virtio_console.c

Loading...
View Options

usr.sbin/bhyve/pci_virtio_net.c

Loading...
View Options

usr.sbin/bhyve/pci_virtio_rnd.c

Loading...
View Options

usr.sbin/bhyve/pci_virtio_scsi.c

Loading...
View Options

usr.sbin/bhyve/virtio.h

Loading...
View Options

usr.sbin/bhyve/virtio.c

Loading...