Wire up viona notify fast path #754
Conversation
The viona driver has the ability to hook an ioport (as specified by the userspace vmm) in order to more efficiently process notifications from the guest that virtqueues have new available entries. This cuts down on the number of VM exits which must be processed by userspace.
lib/propolis/src/hw/pci/device.rs
Outdated
| if (inner.pio_en() && def.is_pio()) | ||
| || (inner.mmio_en() && def.is_mmio()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done (with slightly tweaked naming)
| poller: None, | ||
| iop_state: None, | ||
| vring_state: [Default::default(); 2], |
There was a problem hiding this comment.
take it or leave it: this is identical to what a derived Default implementation would do...
There was a problem hiding this comment.
When there are state variables involved, I sometimes prefer to spell it out by hand. I go either way sometimes. For this one, I think I'm going to leave it (at least for now).
| @@ -648,6 +687,14 @@ impl VionaHdl { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn set_notify_iop(&self, port: Option<u16>) -> io::Result<()> { | |||
There was a problem hiding this comment.
Perhaps this is just because I don't have an illumos background, but it took a bit for me to realize that "IOP" here referred to "IO port"...I might prefer to expand it to set_notify_io_port? I note that we have a function above notify_port_update that calls this function, so perhaps we could call this set_notify_port to mirror that? But, if you prefer to more closely mirror the name of the ioctl, that's fair, too. :)
There was a problem hiding this comment.
I'd like to keep the name consistent with the ioctl here in VionaHdl. I've written up a doc comment which hopefully makes it clearer.
| @@ -475,6 +504,16 @@ impl PciVirtio for PciVirtioViona { | |||
| fn pci_state(&self) -> &pci::DeviceState { | |||
| &self.pci_state | |||
| } | |||
| fn notify_port_update(&self, port: Option<u16>) { | |||
There was a problem hiding this comment.
It might be worth a comment explaining the difference between notify_port_update and set_notify_iop, which this calls if the device is running? It could be helpful if we explained when this should be called, and when it's okay to call set_notify_iop unconditionally?
| IndicatedState::from_usize(self.0.load(Ordering::SeqCst)) | ||
| } | ||
|
|
||
| fn state_change(&self, new: IndicatedState) { | ||
| let old = self.0.swap(new as usize, Ordering::SeqCst); |
There was a problem hiding this comment.
I don't think sequentially-consistent ordering is required here. I don't believe there is another memory location that we want to have a total ordering of sequentially-consistent stores/loads with, so AcqRel should be sufficient here?
I doubt the performance penalty for sequentially-consistent stores matters a whole lot here, though, so 🤷♀️
There was a problem hiding this comment.
I gravitate towards using excessively strong ordering until performance sensitivities prove otherwise. Also, is AcqRel functionally different than SeqCst on amd64?
The viona driver has the ability to hook an ioport (as specified by the userspace vmm) in order to more efficiently process notifications from the guest that virtqueues have new available entries. This cuts down on the number of VM exits which must be processed by userspace.