Skip to content

Wire up viona notify fast path #754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Wire up viona notify fast path #754

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dbcf0c
Wire up viona notify fast path
pfmooney Aug 27, 2024
dcd32de
Review feedback
pfmooney Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions crates/viona-api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ impl VionaFd {
Ok(vers as u32)
}

/// Check VMM ioctl command against those known to not require any
/// Check viona ioctl command against those known to not require any
/// copyin/copyout to function.
const fn ioctl_usize_safe(cmd: i32) -> bool {
matches!(
Expand All @@ -76,7 +76,8 @@ impl VionaFd {
| ioctls::VNA_IOC_RING_PAUSE
| ioctls::VNA_IOC_RING_INTR_CLR
| ioctls::VNA_IOC_VERSION
| ioctls::VNA_IOC_SET_PROMISC
| ioctls::VNA_IOC_SET_NOTIFY_IOP
| ioctls::VNA_IOC_SET_PROMISC,
)
}
}
Expand Down
32 changes: 23 additions & 9 deletions lib/propolis/src/hw/pci/bar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,33 +121,35 @@ impl Bars {
&mut self,
bar: BarN,
val: u32,
) -> Option<(BarDefine, u64, u64)> {
) -> Option<WriteResult> {
let idx = bar as usize;
let ent = &mut self.entries[idx];
let (def, old, new) = match ent.kind {
let (id, def, val_old, val_new) = match ent.kind {
EntryKind::Empty => return None,
EntryKind::Pio(size) => {
let mask = u32::from(!(size - 1));
let old = ent.value;
ent.value = u64::from(val & mask);
(BarDefine::Pio(size), old, ent.value)
(bar, BarDefine::Pio(size), old, ent.value)
}
EntryKind::Mmio(size) => {
let mask = !(size - 1);
let old = ent.value;
ent.value = u64::from(val & mask);
(BarDefine::Mmio(size), old, ent.value)
(bar, BarDefine::Mmio(size), old, ent.value)
}
EntryKind::Mmio64(size) => {
let old = ent.value;
let mask = !(size - 1) as u32;
let low = val & mask;
ent.value = (old & (0xffffffff << 32)) | u64::from(low);
(BarDefine::Mmio64(size), old, ent.value)
(bar, BarDefine::Mmio64(size), old, ent.value)
}
EntryKind::Mmio64High => {
assert!(idx > 0);
let ent = &mut self.entries[idx - 1];
let real_idx = idx - 1;
let id = BarN::from_repr(real_idx as u8).unwrap();
let ent = &mut self.entries[real_idx];
let size = match ent.kind {
EntryKind::Mmio64(sz) => sz,
_ => panic!(),
Expand All @@ -156,11 +158,11 @@ impl Bars {
let old = ent.value;
let high = ((u64::from(val) << 32) & mask) & 0xffffffff00000000;
ent.value = high | (old & 0xffffffff);
(BarDefine::Mmio64(size), old, ent.value)
(id, BarDefine::Mmio64(size), old, ent.value)
}
};
if old != new {
return Some((def, old, new));
if val_old != val_new {
return Some(WriteResult { id, def, val_old, val_new });
}
None
}
Expand Down Expand Up @@ -287,6 +289,18 @@ impl Bars {
}
}

/// Result from a write to a BAR
pub struct WriteResult {
/// Identifier of the actual impacted BAR.
///
/// If write was to the high word of a 64-bit BAR, this would hold the
/// `BarN` for the lower word.
pub id: BarN,
pub def: BarDefine,
pub val_old: u64,
pub val_new: u64,
}

pub mod migrate {
use serde::{Deserialize, Serialize};

Expand Down
83 changes: 67 additions & 16 deletions lib/propolis/src/hw/pci/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ use crate::migrate::*;
use crate::util::regmap::{Flags, RegMap};

use lazy_static::lazy_static;
use strum::IntoEnumIterator;

pub trait Device: Send + Sync + 'static {
fn device_state(&self) -> &DeviceState;

#[allow(unused_variables)]
fn bar_rw(&self, bar: BarN, rwo: RWOp) {
match rwo {
RWOp::Read(ro) => {
Expand All @@ -30,7 +30,6 @@ pub trait Device: Send + Sync + 'static {
}
}
}
#[allow(unused_variables)]
fn cfg_rw(&self, region: u8, rwo: RWOp) {
match rwo {
RWOp::Read(ro) => {
Expand All @@ -46,6 +45,13 @@ pub trait Device: Send + Sync + 'static {
fn interrupt_mode_change(&self, mode: IntrMode) {}
#[allow(unused_variables)]
fn msi_update(&self, info: MsiUpdate) {}

/// Notification that configuration of BAR(s) has changed, either due to
/// writes to the BARs themselves, or an overall status change (via the
/// Command register or a device reset).
#[allow(unused_variables)]
fn bar_update(&self, bstate: BarState) {}

// TODO
// fn cap_read(&self);
// fn cap_write(&self);
Expand Down Expand Up @@ -181,6 +187,14 @@ impl State {
fn attached(&self) -> &bus::Attachment {
self.attach.as_ref().unwrap()
}
/// Is MMIO access decoding enabled?
fn mmio_en(&self) -> bool {
self.reg_command.contains(RegCmd::MMIO_EN)
}
/// Is PIO access decoding enabled?
fn pio_en(&self) -> bool {
self.reg_command.contains(RegCmd::IO_EN)
}
}

pub(super) struct Cap {
Expand Down Expand Up @@ -375,15 +389,19 @@ impl DeviceState {
StdCfgReg::Bar(bar) => {
let val = wo.read_u32();
let mut state = self.state.lock().unwrap();
if let Some((def, _old, new)) = state.bars.reg_write(*bar, val)
{
let pio_en = state.reg_command.contains(RegCmd::IO_EN);
let mmio_en = state.reg_command.contains(RegCmd::MMIO_EN);

if let Some(res) = state.bars.reg_write(*bar, val) {
let attach = state.attached();
if (pio_en && def.is_pio()) || (mmio_en && def.is_mmio()) {
attach.bar_unregister(*bar);
attach.bar_register(*bar, def, new);
if (state.pio_en() && res.def.is_pio())
|| (state.mmio_en() && res.def.is_mmio())
{
attach.bar_unregister(res.id);
attach.bar_register(res.id, res.def, res.val_new);
dev.bar_update(BarState {
id: res.id,
def: res.def,
value: res.val_new,
decode_en: true,
});
}
}
}
Expand Down Expand Up @@ -425,6 +443,8 @@ impl DeviceState {

// Update BAR registrations
if diff.intersects(RegCmd::IO_EN | RegCmd::MMIO_EN) {
let pio_en = val.contains(RegCmd::IO_EN);
let mmio_en = val.contains(RegCmd::MMIO_EN);
for n in BarN::iter() {
let bar = state.bars.get(n);
if bar.is_none() {
Expand All @@ -433,18 +453,30 @@ impl DeviceState {
let (def, v) = bar.unwrap();

if diff.contains(RegCmd::IO_EN) && def.is_pio() {
if val.contains(RegCmd::IO_EN) {
if pio_en {
attach.bar_register(n, def, v);
} else {
attach.bar_unregister(n);
}
dev.bar_update(BarState {
id: n,
def,
value: v,
decode_en: pio_en,
});
}
if diff.contains(RegCmd::MMIO_EN) && def.is_mmio() {
if val.contains(RegCmd::MMIO_EN) {
if mmio_en {
attach.bar_register(n, def, v);
} else {
attach.bar_unregister(n);
}
dev.bar_update(BarState {
id: n,
def,
value: v,
decode_en: mmio_en,
});
}
}
}
Expand Down Expand Up @@ -482,6 +514,17 @@ impl DeviceState {
self.which_intr_mode(&state)
}

pub(crate) fn bar(&self, id: BarN) -> Option<BarState> {
let state = self.state.lock().unwrap();
state.bars.get(id).map(|(def, value)| {
let decode_en = match def {
BarDefine::Pio(_) => state.pio_en(),
BarDefine::Mmio(_) | BarDefine::Mmio64(_) => state.mmio_en(),
};
BarState { id, def, value, decode_en }
})
}

fn cfg_cap_rw(&self, dev: &dyn Device, id: &CfgReg, rwo: RWOp) {
match id {
CfgReg::CapId(i) => {
Expand Down Expand Up @@ -608,10 +651,9 @@ impl DeviceState {
let attach = inner.attached();
for n in BarN::iter() {
if let Some((def, addr)) = inner.bars.get(n) {
let pio_en = inner.reg_command.contains(RegCmd::IO_EN);
let mmio_en = inner.reg_command.contains(RegCmd::MMIO_EN);

if (pio_en && def.is_pio()) || (mmio_en && def.is_mmio()) {
if (inner.pio_en() && def.is_pio())
|| (inner.mmio_en() && def.is_mmio())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, take it or leave it: I wonder if there ought to be a method like

impl State {
    fn should_register_bar(&self, bar: &BarDefine) -> bool {
         (self.pio_en() && bar.is_pio()) || (self.mmio_en() && def.is_mmio())
    }
}

that could be called both here and in cfg_std_write, above?

Not sure if it's worth factoring out or not, just noticed that it was repeated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (with slightly tweaked naming)

{
attach.bar_register(n, def, addr);
}
}
Expand Down Expand Up @@ -1093,6 +1135,15 @@ impl Clone for MsixHdl {
}
}

/// Describes the state of a BAR
pub struct BarState {
pub id: BarN,
pub def: BarDefine,
pub value: u64,
/// Is decoding for this BAR enabled in the device control?
pub decode_en: bool,
}

pub struct Builder {
ident: Ident,
lintr_support: bool,
Expand Down
23 changes: 4 additions & 19 deletions lib/propolis/src/hw/pci/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::sync::{Arc, Mutex};
use crate::common::*;
use crate::intr_pins::IntrPin;

use strum::FromRepr;
use strum::{EnumIter, FromRepr};

pub mod bar;
pub mod bits;
Expand Down Expand Up @@ -244,7 +244,9 @@ impl Display for Bdf {
}
}

#[derive(Copy, Clone, Eq, PartialEq, Debug, Ord, PartialOrd, FromRepr)]
#[derive(
Copy, Clone, Eq, PartialEq, Debug, Ord, PartialOrd, FromRepr, EnumIter,
)]
#[repr(u8)]
pub enum BarN {
BAR0 = 0,
Expand All @@ -254,23 +256,6 @@ pub enum BarN {
BAR4,
BAR5,
}
impl BarN {
fn iter() -> BarIter {
BarIter { n: 0 }
}
}
struct BarIter {
n: u8,
}
impl Iterator for BarIter {
type Item = BarN;

fn next(&mut self) -> Option<Self::Item> {
let res = BarN::from_repr(self.n)?;
self.n += 1;
Some(res)
}
}

#[repr(u8)]
#[derive(Copy, Clone)]
Expand Down
35 changes: 33 additions & 2 deletions lib/propolis/src/hw/virtio/pci.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,26 @@ impl VirtioState {
pub trait PciVirtio: VirtioDevice + Send + Sync + 'static {
fn virtio_state(&self) -> &PciVirtioState;
fn pci_state(&self) -> &pci::DeviceState;

#[allow(unused_variables)]
/// Notification that the IO port representing the queue notification
/// register in the device BAR has changed.
fn notify_port_update(&self, state: Option<u16>) {}

/// Noticiation from the PCI emulation that one of the BARs has undergone a
/// change of configuration
fn bar_update(&self, bstate: pci::BarState) {
if bstate.id == pci::BarN::BAR0 {
// Notify the device about the location (if any) of the queue notify
// register in the containing BAR region.
let port = if bstate.decode_en {
Some(bstate.value as u16 + LEGACY_REG_OFF_QUEUE_NOTIFY as u16)
} else {
None
};
self.notify_port_update(port);
}
}
}

impl<D: PciVirtio + Send + Sync + 'static> pci::Device for D {
Expand Down Expand Up @@ -163,6 +183,10 @@ impl<D: PciVirtio + Send + Sync + 'static> pci::Device for D {
state.intr_mode_updating = false;
vs.state_cv.notify_all();
}

fn bar_update(&self, bstate: pci::BarState) {
PciVirtio::bar_update(self, bstate);
}
}

pub struct PciVirtioState {
Expand Down Expand Up @@ -422,9 +446,9 @@ impl PciVirtioState {

/// Indicate to the guest that the VirtIO device has encountered an error of
/// some sort and requires a reset.
pub fn set_needs_reset(&self, _dev: &dyn VirtioDevice) {
pub fn set_needs_reset(&self, dev: &dyn VirtioDevice) {
let mut state = self.state.lock().unwrap();
self.needs_reset_locked(_dev, &mut state);
self.needs_reset_locked(dev, &mut state);
}

fn queue_notify(&self, dev: &dyn VirtioDevice, queue: u16) {
Expand Down Expand Up @@ -618,6 +642,12 @@ impl MigrateMulti for dyn PciVirtio {
// to the VirtIO state.
vs.set_intr_mode(ps, ps.get_intr_mode().into(), true);

// Perform a (potentially spurious) update notification for the BAR
// containing the virtio registers. This ensures that anything
// interested in the placement of that BAR (such as the notify-port
// logic) is kept well aware
self.bar_update(ps.bar(pci::BarN::BAR0).unwrap());

Ok(())
}
}
Expand Down Expand Up @@ -790,6 +820,7 @@ enum VirtioTop {

const LEGACY_REG_SZ: usize = 0x18;
const LEGACY_REG_SZ_NO_MSIX: usize = 0x14;
const LEGACY_REG_OFF_QUEUE_NOTIFY: usize = 0x10;

#[derive(Copy, Clone, Eq, PartialEq, Debug)]
enum LegacyReg {
Expand Down
Loading
Loading