diff --git a/crates/viona-api/src/ffi.rs b/crates/viona-api/src/ffi.rs index e67f97d11..d20cfdcf7 100644 --- a/crates/viona-api/src/ffi.rs +++ b/crates/viona-api/src/ffi.rs @@ -127,6 +127,7 @@ pub struct vioc_ring_state { pub vrs_qaddr_desc: u64, pub vrs_qaddr_avail: u64, pub vrs_qaddr_used: u64, + pub vrs_last_chk_uidx: u16, } pub const VIONA_PROMISC_NONE: i32 = 0; @@ -154,7 +155,7 @@ pub struct vioc_set_params { /// This is the viona interface version which viona_api expects to operate /// against. All constants and structs defined by the crate are done so in /// terms of that specific version. -pub const VIONA_CURRENT_INTERFACE_VERSION: u32 = 6; +pub const VIONA_CURRENT_INTERFACE_VERSION: u32 = 7; /// Maximum size of packed nvlists used in viona parameter ioctls pub const VIONA_MAX_PARAM_NVLIST_SZ: usize = 4096; diff --git a/lib/propolis/src/hw/virtio/bits.rs b/lib/propolis/src/hw/virtio/bits.rs index a7372886a..802dc2e37 100644 --- a/lib/propolis/src/hw/virtio/bits.rs +++ b/lib/propolis/src/hw/virtio/bits.rs @@ -2,6 +2,9 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +// device-independent features +pub const VIRTIO_F_EVENT_IDX: u64 = 1 << 29; + // virtio-net feature bits pub const VIRTIO_NET_F_CSUM: u64 = 1 << 0; pub const VIRTIO_NET_F_GUEST_CSUM: u64 = 1 << 1; diff --git a/lib/propolis/src/hw/virtio/mod.rs b/lib/propolis/src/hw/virtio/mod.rs index 88894c759..834c6e0e0 100644 --- a/lib/propolis/src/hw/virtio/mod.rs +++ b/lib/propolis/src/hw/virtio/mod.rs @@ -30,11 +30,28 @@ use crate::common::RWOp; use crate::hw::pci as pci_hw; use crate::lifecycle::Lifecycle; use queue::VirtQueue; +use serde::{Deserialize, Serialize}; pub use block::PciVirtioBlock; pub use viona::PciVirtioViona; pub use vsock::PciVirtioSock; +#[cfg(target_os = "illumos")] + +mod os { + unsafe extern "C" { + pub fn membar_enter(); + } +} + +#[inline(always)] +pub fn membar_enter() { + #[cfg(target_os = "illumos")] + unsafe { + os::membar_enter(); + } +} + bitflags! { pub struct LegacyFeatures: u64 { const NOTIFY_ON_EMPTY = 1 << 24; @@ -246,6 +263,7 @@ pub enum VqChange { IntrCfg, } +#[derive(Debug)] pub enum VqIntr { /// Pin (lintr) interrupt Pin, @@ -254,12 +272,43 @@ pub enum VqIntr { Msi(u64, u32, bool), } +#[derive(Copy, Clone, Default, Deserialize, Serialize)] +pub struct VqNeedsIntrProbeInfo { + used_event: u16, + last_chk_uidx: u16, + uidx: u16, + needed: bool, +} + #[usdt::provider(provider = "propolis")] mod probes { fn virtio_vq_notify(virtio_dev_addr: u64, virtqueue_id: u16) {} fn virtio_vq_pop(vq_addr: u64, desc_idx: u16, avail_idx: u16) {} fn virtio_vq_push(vq_addr: u64, used_idx: u16, used_len: u32) {} + fn virtio_vq_needs_intr( + vq_addr: u64, + vq_id: u16, + f_event_idx: u64, + info: crate::hw::virtio::VqNeedsIntrProbeInfo, + ) { + } + fn virtio_vq_send_intr(vq_addr: u64, vq_id: u16, sent: u64) {} + fn virtio_vq_enable_intr( + vq_addr: u64, + vq_id: u16, + f_event_idx: u64, + avail_event: u16, + ) { + } + fn virtio_vq_disable_intr( + vq_addr: u64, + vq_id: u16, + f_event_idx: u64, + avail_event: u16, + ) { + } + fn virtio_viona_mq_set_use_pairs(cause: u8, npairs: u16) {} fn virtio_device_needs_reset() {} diff --git a/lib/propolis/src/hw/virtio/queue.rs b/lib/propolis/src/hw/virtio/queue.rs index 9c9925505..d51f1a9eb 100644 --- a/lib/propolis/src/hw/virtio/queue.rs +++ b/lib/propolis/src/hw/virtio/queue.rs @@ -85,6 +85,7 @@ pub struct VqAvail { cur_avail_idx: Wrapping, gpa_desc: GuestAddr, + gpa_used_event: GuestAddr, } impl VqAvail { @@ -130,12 +131,14 @@ impl VqAvail { self.cur_avail_idx = Wrapping(0); } - fn map_split(&mut self, desc_addr: u64, avail_addr: u64) { + fn map_split(&mut self, desc_addr: u64, avail_addr: u64, sz: u16) { self.gpa_desc = GuestAddr(desc_addr); - // 16-bit flags, followed by 16-bit idx, followed by avail desc ring + // 16-bit flags, followed by 16-bit idx, avail desc ring + // (u16[sz]), followed by 16-bit used_event. self.gpa_flags = GuestAddr(avail_addr); self.gpa_idx = GuestAddr(avail_addr + 2); self.gpa_ring = GuestAddr(avail_addr + 4); + self.gpa_used_event = self.gpa_ring.offset::(usize::from(sz)); } /// Returns guest flags. @@ -145,11 +148,13 @@ impl VqAvail { AvailFlags::from_bits_truncate(value) } - /// Returns true IFF interrupts are supressed. - #[allow(dead_code)] - fn _intr_supressed(&self, mem: &MemCtx) -> bool { - let flags = self.flags(mem); - flags.contains(AvailFlags::NO_INTERRUPT) + /// Return used_event value. + fn used_event(&self, mem: &MemCtx) -> u16 { + if self.valid { + *mem.read(self.gpa_used_event).unwrap() + } else { + 0 + } } } @@ -160,7 +165,13 @@ pub struct VqUsed { gpa_flags: GuestAddr, gpa_idx: GuestAddr, gpa_ring: GuestAddr, + /// Used by device to control notification when F_EVENT_IDX is + /// negotiated. + gpa_avail_event: GuestAddr, used_idx: Wrapping, + /// The value of uidx the last time we checked if the guest + /// required notification. + last_chk_uidx: Wrapping, interrupt: Option>, } @@ -193,25 +204,28 @@ impl VqUsed { mem.write(self.gpa_flags, &value); } - /// Disables notifications on this queue; returns whether notfications were - /// enabled before. - fn disable_notify(&self, mem: &MemCtx) -> bool { - let flags = self.flags(mem); - let current = !flags.contains(UsedFlags::NO_NOTIFY); - self.set_flags(flags | UsedFlags::NO_NOTIFY, mem); - current - } - - fn enable_notify(&self, mem: &MemCtx) { - let mut flags = self.flags(mem); - flags.remove(UsedFlags::NO_NOTIFY); - self.set_flags(flags, mem); + /// Disables notifications on this queue. + fn disable_notify(&self, avail_idx: Option>, mem: &MemCtx) { + if let Some(idx) = avail_idx { + assert!(self.gpa_avail_event.0 != 0); + mem.write(self.gpa_avail_event, &idx); + } else { + let flags = self.flags(mem); + self.set_flags(flags | UsedFlags::NO_NOTIFY, mem); + } } - /// Returns true iff notifications are supressed for this queue. - fn notify_supressed(&self, mem: &MemCtx) -> bool { - let flags = self.flags(mem); - flags.contains(UsedFlags::NO_NOTIFY) + /// Let the driver know that we are interested in receiving + /// interrupts. + fn enable_notify(&self, avail_idx: Option, mem: &MemCtx) { + if let Some(idx) = avail_idx { + assert!(self.gpa_avail_event.0 != 0); + mem.write(self.gpa_avail_event, &idx); + } else { + let mut flags = self.flags(mem); + flags.remove(UsedFlags::NO_NOTIFY); + self.set_flags(flags, mem); + } } fn reset(&mut self) { @@ -221,11 +235,14 @@ impl VqUsed { self.gpa_ring = GuestAddr(0); self.used_idx = Wrapping(0); } - fn map_split(&mut self, gpa: u64) { - // 16-bit flags, followed by 16-bit idx, followed by used desc ring + + fn map_split(&mut self, gpa: u64, sz: u16) { + // 16-bit flags, followed by 16-bit idx, followed by used desc + // ring, followed by avail_event self.gpa_flags = GuestAddr(gpa); self.gpa_idx = GuestAddr(gpa + 2); self.gpa_ring = GuestAddr(gpa + 4); + self.gpa_avail_event = self.gpa_ring.offset::(usize::from(sz)); } } @@ -282,6 +299,8 @@ pub struct VirtQueue { avail: Mutex, used: Mutex, pub acc_mem: MemAccessor, + /// Is the F_EVENT_IDX feature in use? + f_event_idx: AtomicBool, } const fn qalign(addr: u64, align: u64) -> u64 { @@ -306,16 +325,20 @@ impl VirtQueue { gpa_ring: GuestAddr(0), cur_avail_idx: Wrapping(0), gpa_desc: GuestAddr(0), + gpa_used_event: GuestAddr(0), }), used: Mutex::new(VqUsed { valid: false, gpa_flags: GuestAddr(0), gpa_idx: GuestAddr(0), gpa_ring: GuestAddr(0), + gpa_avail_event: GuestAddr(0), used_idx: Wrapping(0), + last_chk_uidx: Wrapping(0), interrupt: None, }), acc_mem: MemAccessor::new_orphan(), + f_event_idx: AtomicBool::new(false), } } @@ -355,6 +378,14 @@ impl VirtQueue { self.is_control.store(val, Ordering::Release); } + fn f_event_idx(&self) -> bool { + self.f_event_idx.load(Ordering::Acquire) + } + + fn set_f_event_idx(&self, val: bool) { + self.f_event_idx.store(val, Ordering::Release); + } + #[inline(always)] pub fn size(&self) -> u16 { let size = *self.size.lock().unwrap(); @@ -376,8 +407,8 @@ impl VirtQueue { ) { let mut avail = self.avail.lock().expect("avail is initialized"); let mut used = self.used.lock().expect("used is initialized"); - avail.map_split(desc_addr, avail_addr); - used.map_split(used_addr); + avail.map_split(desc_addr, avail_addr, self.size()); + used.map_split(used_addr, self.size()); avail.valid = true; used.valid = true; } @@ -433,6 +464,7 @@ impl VirtQueue { }, avail_idx: avail.cur_avail_idx.0, used_idx: used.used_idx.0, + last_chk_uidx: used.last_chk_uidx.0, } } @@ -440,12 +472,17 @@ impl VirtQueue { let mut avail = self.avail.lock().unwrap(); let mut used = self.used.lock().unwrap(); - avail.map_split(info.mapping.desc_addr, info.mapping.avail_addr); - used.map_split(info.mapping.used_addr); + avail.map_split( + info.mapping.desc_addr, + info.mapping.avail_addr, + self.size(), + ); + used.map_split(info.mapping.used_addr, self.size()); avail.valid = info.mapping.valid; used.valid = info.mapping.valid; avail.cur_avail_idx = Wrapping(info.avail_idx); used.used_idx = Wrapping(info.used_idx); + used.last_chk_uidx = Wrapping(info.last_chk_uidx); } /// Accummulates a sequence of available descriptors into a `Chain`. @@ -538,19 +575,16 @@ impl VirtQueue { pub fn push_used(&self, chain: &mut Chain, mem: &MemCtx) { assert!(chain.idx.is_some()); + let avail = self.avail.lock().unwrap(); let mut used = self.used.lock().unwrap(); let id = mem::replace(&mut chain.idx, None).unwrap(); // XXX: for now, just go off of the write stats let len = chain.write_stat.bytes - chain.write_stat.bytes_remain; probes::virtio_vq_push!(|| (self as *const VirtQueue as u64, id, len)); used.write_used(id, len, self.size(), mem); - // XXX: This is wrong. Interrupt notification is on the avail ring, - // not used. - #[allow(clippy::overly_complex_bool_expr)] - if true || !used.notify_supressed(mem) { - if let Some(intr) = used.interrupt.as_ref() { - intr.notify(); - } + + if self.needs_intr_locked(&avail, &mut used, mem) { + self.send_intr_locked(&used); } chain.reset(); } @@ -567,31 +601,124 @@ impl VirtQueue { used.interrupt.as_ref().map(|x| x.read()) } - /// Disables interrupts (notifications) on the `Used` ring. - /// - /// Returns `true` if notifications were previously enabled. - pub(super) fn disable_intr(&self, mem: &MemCtx) -> bool { - let used = self.used.lock().unwrap(); - used.disable_notify(mem) + /// Disable interrupts (notifications) from driver to device. + pub(super) fn disable_intr(&self, mem: &MemCtx) { + let idx = if self.f_event_idx() { + let aidx = self.avail.lock().unwrap().cur_avail_idx - Wrapping(1); + probes::virtio_vq_disable_intr!(|| ( + self as *const VirtQueue as u64, + self.id, + 1, + aidx.0, + )); + Some(aidx) + } else { + probes::virtio_vq_disable_intr!(|| ( + self as *const VirtQueue as u64, + self.id, + 0, + 0, + )); + None + }; + + self.used.lock().unwrap().disable_notify(idx, mem); } - /// Enables interrupts (notifications) on the `Used` ring + /// Enable interrupts (notifications) from driver to device. pub(super) fn enable_intr(&self, mem: &MemCtx) { - let used = self.used.lock().unwrap(); - used.enable_notify(mem); + let aidx = if self.f_event_idx() { + let aidx = self.avail.lock().unwrap().cur_avail_idx; + probes::virtio_vq_enable_intr!(|| ( + self as *const VirtQueue as u64, + self.id, + 1, + aidx.0, + )); + Some(aidx.0) + } else { + probes::virtio_vq_enable_intr!(|| ( + self as *const VirtQueue as u64, + self.id, + 0, + 0, + )); + None + }; + + self.used.lock().unwrap().enable_notify(aidx, mem); + } + + pub fn needs_intr(&self, mem: &MemCtx) -> bool { + let avail = self.avail.lock().unwrap(); + let mut used = self.used.lock().unwrap(); + self.needs_intr_locked(&avail, &mut used, mem) + } + + /// Returns true if the guest should receive an interrupt. + fn needs_intr_locked( + &self, + avail: &VqAvail, + used: &mut VqUsed, + mem: &MemCtx, + ) -> bool { + if self.f_event_idx() { + let used_event = Wrapping(avail.used_event(mem)); + let uidx = used.used_idx; + let last_chk_uidx = used.last_chk_uidx; + let res = + (uidx - used_event - Wrapping(1)) < (uidx - last_chk_uidx); + used.last_chk_uidx = uidx; + let info = crate::hw::virtio::VqNeedsIntrProbeInfo { + used_event: used_event.0, + last_chk_uidx: last_chk_uidx.0, + uidx: uidx.0, + needed: res, + }; + probes::virtio_vq_needs_intr!(|| ( + self as *const VirtQueue as u64, + self.id, + 1, + info, + )); + res + } else { + let flags = avail.flags(mem); + let res = !flags.contains(AvailFlags::NO_INTERRUPT); + let info = crate::hw::virtio::VqNeedsIntrProbeInfo { + used_event: 0, + last_chk_uidx: 0, + uidx: 0, + needed: res, + }; + probes::virtio_vq_needs_intr!(|| ( + self as *const VirtQueue as u64, + self.id, + 0, + info, + )); + res + } } /// Send an interrupt for this virtual queue. - pub(super) fn send_intr(&self, mem: &MemCtx) { + pub(super) fn send_intr(&self) { let used = self.used.lock().unwrap(); - // XXX: This is wrong. Interrupt notification is on the avail ring, - // not used. - #[allow(clippy::overly_complex_bool_expr)] - if true || !used.notify_supressed(mem) { - if let Some(intr) = used.interrupt.as_ref() { - intr.notify(); - } - } + self.send_intr_locked(&used); + } + + fn send_intr_locked(&self, used: &VqUsed) { + let sent = if let Some(intr) = used.interrupt.as_ref() { + intr.notify(); + true + } else { + false + }; + probes::virtio_vq_send_intr!(|| ( + self as *const VirtQueue as u64, + self.id, + sent as u64, + )); } pub fn export(&self) -> migrate::VirtQueueV1 { @@ -676,11 +803,11 @@ impl VirtQueue { ))); } - avail.map_split(state.descr_gpa, state.avail_gpa); + avail.map_split(state.descr_gpa, state.avail_gpa, self.size()); avail.valid = state.mapping_valid; avail.cur_avail_idx = Wrapping(state.avail_cur_idx); - used.map_split(state.used_gpa); + used.map_split(state.used_gpa, self.size()); used.valid = state.mapping_valid; used.used_idx = Wrapping(state.used_idx); @@ -1035,6 +1162,7 @@ pub struct Info { pub mapping: MapInfo, pub avail_idx: u16, pub used_idx: u16, + pub last_chk_uidx: u16, } pub struct VirtQueues { @@ -1107,6 +1235,12 @@ impl VirtQueues { Ok(()) } + pub fn set_f_event_idx(&self, f_event_idx: bool) { + for vq in &self.queues { + vq.set_f_event_idx(f_event_idx); + } + } + pub fn count(&self) -> NonZeroU16 { NonZeroU16::try_from(self.iter().count() as u16) .expect("queue count already validated") diff --git a/lib/propolis/src/hw/virtio/viona.rs b/lib/propolis/src/hw/virtio/viona.rs index 4f2dc8ea6..2a47b3116 100644 --- a/lib/propolis/src/hw/virtio/viona.rs +++ b/lib/propolis/src/hw/virtio/viona.rs @@ -624,7 +624,9 @@ impl PciVirtioViona { .intr_poll(self.virtio_state.queues.len(), |vq_idx| { self.hdl.ring_intr_clear(vq_idx).unwrap(); let vq = self.virtio_state.queues.get(vq_idx).unwrap(); - vq.send_intr(&mem); + if vq.needs_intr(&mem) { + vq.send_intr(); + } }) .unwrap(); } @@ -632,19 +634,32 @@ impl PciVirtioViona { fn ctl_queue_notify(&self, vq: &VirtQueue) { if let Some(mem) = self.pci_state.acc_mem.access() { - while !vq.avail_is_empty(&mem) { - let mut chain = Chain::with_capacity(4); - let intrs_en = vq.disable_intr(&mem); - while let Some((_idx, _len)) = vq.pop_avail(&mut chain, &mem) { - let res = match self.ctl_msg(&mut chain, &mem) { - Ok(_) => control::Ack::Ok, - Err(_) => control::Ack::Err, - } as u8; - chain.write(&res, &mem); - vq.push_used(&mut chain, &mem); + loop { + vq.disable_intr(&mem); + + while !vq.avail_is_empty(&mem) { + let mut chain = Chain::with_capacity(4); + + while let Some((_idx, _len)) = + vq.pop_avail(&mut chain, &mem) + { + let res = match self.ctl_msg(&mut chain, &mem) { + Ok(_) => control::Ack::Ok, + Err(_) => control::Ack::Err, + } as u8; + chain.write(&res, &mem); + vq.push_used(&mut chain, &mem); + } } - if intrs_en { - vq.enable_intr(&mem); + + vq.enable_intr(&mem); + + crate::hw::virtio::membar_enter(); + + // Check if enabling the interrupt raced with the + // driver publishing new entries to avail. + if vq.avail_is_empty(&mem) { + break; } } } @@ -1101,6 +1116,13 @@ impl VirtioDevice for PciVirtioViona { fn set_features(&self, feat: u64) -> Result<(), ()> { self.hdl.set_features(feat).map_err(|_| ())?; + eprintln!("set_features: {:?}", feat); + + if (feat & VIRTIO_F_EVENT_IDX) != 0 { + eprintln!("set f_event_idx on all queues"); + self.virtio_state.queues.set_f_event_idx(true); + } + // Any remaining setup is for control-queue based features. let control_queue = if (feat & VIRTIO_NET_F_CTRL_VQ) == 0 { None @@ -1439,6 +1461,7 @@ impl From<&VirtQueue> for viona_api::vioc_ring_state { let used_addr = state.mapping.used_addr; let avail_idx = state.avail_idx; let used_idx = state.used_idx; + let last_chk_uidx = state.last_chk_uidx; viona_api::vioc_ring_state { vrs_index: id, vrs_qsize: size, @@ -1446,6 +1469,7 @@ impl From<&VirtQueue> for viona_api::vioc_ring_state { vrs_qaddr_avail: avail_addr, vrs_qaddr_used: used_addr, vrs_used_idx: used_idx, + vrs_last_chk_uidx: last_chk_uidx, vrs_avail_idx: avail_idx, } } @@ -1562,6 +1586,7 @@ impl VionaHdl { }, avail_idx: cfg.vrs_avail_idx, used_idx: cfg.vrs_used_idx, + last_chk_uidx: cfg.vrs_last_chk_uidx, }) } fn ring_cfg_msi(