VirtIO 1.0 and multi-queue support #998
Conversation
dancrossnyc
requested review from
citrus-it,
iximeow and
papertigers
and removed request for
papertigers
dancrossnyc
force-pushed
the
virtio-modern
branch
from
3a48da6 to
e7b1927
Compare
dancrossnyc
force-pushed
the
virtio-modern
branch
from
e7b1927 to
59f9711
Compare
citrus-it
left a comment
citrus-it
left a comment
There was a problem hiding this comment.
I've reviewed this mostly from a VirtIO/viona perspective.
| #[derive(Default)] | ||
| pub struct vioc_intr_poll { | ||
| pub vip_nrings: u16, | ||
| pub vip_status: [u32; howmany(VIONA_MAX_QPAIRS, 32)], |
There was a problem hiding this comment.
Since you have the padding explicitly shown in other structures here do you want to do the same for the hole here?
|
|
||
| #[repr(C)] | ||
| #[derive(Default)] | ||
| pub struct vioc_nofity_mmio { |
There was a problem hiding this comment.
| pub struct vioc_nofity_mmio { | |
| pub struct vioc_notify_mmio { |
| } | ||
|
|
||
| pub const VIONA_MIN_QPAIRS: usize = 1; | ||
| pub const VIONA_MAX_QPAIRS: usize = 0x100; |
There was a problem hiding this comment.
Perhaps a comment here to say that this limit comes from viona and is deliberately far lower than that allowed by the spec (0x8000)? 256 queue pairs should be enough for anyone.. or something
| | VNA_IOC_GET_MTU | ||
| | VNA_IOC_SET_MTU | ||
| | VNA_IOC_SET_PAIRS | ||
| | VNA_IOC_SET_USEPAIRS, |
There was a problem hiding this comment.
Both GET_PAIRS and GET_USEPAIRS should also be safe in terms of not requiring copyin/copyout.
I appreciate they are probably not used.
| /// polling. | ||
| V6 = 6, | ||
|
|
||
| /// Adds support for VirtIO 1.2 (modern) virtqueues. |
There was a problem hiding this comment.
Technically modern queues came in with VirtIO 1.0
| CommonConfigReg::QueueSize => { | ||
| let state = self.state.lock().unwrap(); | ||
| let offered = | ||
| VqSize::try_from(wo.read_u16()).expect("bad queue size"); |
There was a problem hiding this comment.
If a guest sends a nonsensical queue size here (such as 0 or something which is not a power of two) we probably ought not panic.
| CommonConfigReg::QueueNotifyData => { | ||
| // Read-only for driver | ||
| } | ||
| CommonConfigReg::QueueReset => self.set_needs_reset(dev), |
There was a problem hiding this comment.
This is a register for per-queue reset, not entire device. Since we aren't advertising VIRTIO_F_RING_RESET we never expect to see it and setting the reset-required bit is probably not an unreasonable response!
| // logic) is kept well aware | ||
| // interested in the placement of those BARs (such as the notify | ||
| // logic) is configured properly. | ||
| self.bar_update(ps.bar(pci::BarN::BAR0).unwrap()); |
There was a problem hiding this comment.
Although we aren't using it yet, the BAR creation logic only creates BAR0 if "Legacy" or "Transitional" mode was selected. We probably need the same existence check as for BAR2 here.
| used.used_idx = Wrapping(info.used_idx); | ||
| } | ||
|
|
||
| /// Accummulates a sequence of available descrptors into a `Chain`. |
There was a problem hiding this comment.
| /// Accummulates a sequence of available descrptors into a `Chain`. | |
| /// Accummulates a sequence of available descriptors into a `Chain`. |
| } | ||
| } | ||
|
|
||
| impl MigrateMulti for PciVirtioViona { |
There was a problem hiding this comment.
It seems we'll need to push at least the number of selected queue pairs into the migration state here.
No description provided.