Fotis (and anybody else), Please let me what you think about this and another patch mostly focused on minimizing the number of exits to hypervisor when reading capabilities data. I hope the code is clear what is going on.
We do not need to apply these patches right away. First, we want to make sure they are correct and according to the spec. The unit tests suggest they are but I may have missed something. On Tuesday, March 17, 2020 at 12:41:27 AM UTC-4, Waldek Kozaczuk wrote: > > Signed-off-by: Waldemar Kozaczuk <[email protected]> > --- > drivers/pci-function.cc | 46 +++++++++--------------------- > drivers/pci-function.hh | 3 +- > drivers/virtio-pci-device.cc | 54 +++++++++++++++++++++++------------- > drivers/virtio-pci-device.hh | 7 +++-- > 4 files changed, 54 insertions(+), 56 deletions(-) > > diff --git a/drivers/pci-function.cc b/drivers/pci-function.cc > index b0fb3674..4866075e 100644 > --- a/drivers/pci-function.cc > +++ b/drivers/pci-function.cc > @@ -811,41 +811,17 @@ namespace pci { > // 0xFF if none found. > u8 function::find_capability(u8 cap_id) > { > - return find_capability(cap_id, [](function *fun, u8 off) { return > true; } ); > - } > - > - // Returns the offset of the first capability with id matching > @cap_id and > - // satisfying @predicate (if specified). If none found, returns 0xFF. > - u8 function::find_capability(u8 cap_id, std::function<bool > (function*, u8)> predicate) > - { > - u8 capabilities_base = pci_readb(PCI_CAPABILITIES_PTR); > - u8 off = capabilities_base; > - u8 bad_offset = 0xFF; > - u8 max_capabilities = 0xF0; > - u8 ctr = 0; > - > - while (off != 0) { > - // Read capability > - u8 capability = pci_readb(off + PCI_CAP_OFF_ID); > - if (capability == cap_id && predicate(this, off)) { > - return off; > - } > - > - ctr++; > - if (ctr > max_capabilities) { > - return bad_offset; > - } > - > - // Next > - off = pci_readb(off + PCI_CAP_OFF_NEXT); > + std::vector<u8> cap_offs; > + if (find_capabilities(cap_id, cap_offs, false)) { > + return cap_offs[0]; > + } else { > + return 0xFF; > } > - > - return bad_offset; > } > > - // Append to @cap_offs the offsets of all capabilities with id > matching > - // @cap_id. Returns whether any such capabilities were found. > - bool function::find_capabilities(std::vector<u8>& cap_offs, u8 > cap_id) > + // Append to @cap_offs the offsets of the first one or all > capabilities with id matching > + // @cap_id. Returns whether any such capability/-ies were found. > + bool function::find_capabilities(u8 cap_id, std::vector<u8>& > cap_offs, bool all) > { > u8 capabilities_base = pci_readb(PCI_CAPABILITIES_PTR); > u8 off = capabilities_base; > @@ -858,7 +834,11 @@ namespace pci { > u8 capability = pci_readb(off + PCI_CAP_OFF_ID); > if (capability == cap_id) { > cap_offs.push_back(off); > - found = true; > + if (all) { > + found = true; > + } else { > + return true; > + } > } > > ctr++; > diff --git a/drivers/pci-function.hh b/drivers/pci-function.hh > index a8904cb0..ef568403 100644 > --- a/drivers/pci-function.hh > +++ b/drivers/pci-function.hh > @@ -341,8 +341,7 @@ namespace pci { > > // Capability parsing > u8 find_capability(u8 cap_id); > - u8 find_capability(u8 cap_id, std::function<bool (function*, u8)> > predicate); > - bool find_capabilities(std::vector<u8>& caps, u8 cap_id); > + bool find_capabilities(u8 cap_id, std::vector<u8>& cap_offs, bool > all); > > bar * get_bar(int idx); > void add_bar(int idx, bar* bar); > diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc > index d484f3df..89130e01 100644 > --- a/drivers/virtio-pci-device.cc > +++ b/drivers/virtio-pci-device.cc > @@ -277,6 +277,17 @@ bool virtio_modern_pci_device::get_shm(u8 id, > mmioaddr_t &addr, u64 &length) > return true; > } > > +void > virtio_modern_pci_device::find_vendor_capabilities(std::vector<std::pair<u8,u8>>& > > offsets_and_types) > +{ > + std::vector<u8> cap_offsets; > + if (_dev->find_capabilities(pci::function::PCI_CAP_VENDOR, > cap_offsets, true)) { > + for (auto offset : cap_offsets) { > + u8 cfg_type = _dev->pci_readb(offset + offsetof(struct > virtio_pci_cap, cfg_type)); > + offsets_and_types.emplace_back(std::pair<u8,u8>(offset, > cfg_type)); > + } > + } > +} > + > bool virtio_modern_pci_device::parse_pci_config() > { > // Check ABI version > @@ -293,12 +304,14 @@ bool virtio_modern_pci_device::parse_pci_config() > return false; > } > > - // TODO: Consider consolidating these (they duplicate work) > - parse_virtio_capability(_common_cfg, VIRTIO_PCI_CAP_COMMON_CFG); > - parse_virtio_capability(_isr_cfg, VIRTIO_PCI_CAP_ISR_CFG); > - parse_virtio_capability(_notify_cfg, VIRTIO_PCI_CAP_NOTIFY_CFG); > - parse_virtio_capability(_device_cfg, VIRTIO_PCI_CAP_DEVICE_CFG); > - parse_virtio_capabilities(_shm_cfgs, > VIRTIO_PCI_CAP_SHARED_MEMORY_CFG); > + std::vector<std::pair<u8,u8>> offsets_and_types; > + find_vendor_capabilities(offsets_and_types); > + > + parse_virtio_capability(offsets_and_types, _common_cfg, > VIRTIO_PCI_CAP_COMMON_CFG); > + parse_virtio_capability(offsets_and_types, _isr_cfg, > VIRTIO_PCI_CAP_ISR_CFG); > + parse_virtio_capability(offsets_and_types, _notify_cfg, > VIRTIO_PCI_CAP_NOTIFY_CFG); > + parse_virtio_capability(offsets_and_types, _device_cfg, > VIRTIO_PCI_CAP_DEVICE_CFG); > + parse_virtio_capabilities(offsets_and_types, _shm_cfgs, > VIRTIO_PCI_CAP_SHARED_MEMORY_CFG); > > if (_notify_cfg) { > _notify_offset_multiplier > =_dev->pci_readl(_notify_cfg->get_cfg_offset() + > @@ -311,12 +324,17 @@ bool virtio_modern_pci_device::parse_pci_config() > > // Parse a single virtio PCI capability, whose type must match @type and > store > // it in @ptr. > -void > virtio_modern_pci_device::parse_virtio_capability(std::unique_ptr<virtio_capability> > > &ptr, u8 type) > -{ > - u8 cfg_offset = _dev->find_capability(pci::function::PCI_CAP_VENDOR, > [type] (pci::function *fun, u8 offset) { > - u8 cfg_type = fun->pci_readb(offset + offsetof(struct > virtio_pci_cap, cfg_type)); > - return type == cfg_type; > - }); > +void > virtio_modern_pci_device::parse_virtio_capability(std::vector<std::pair<u8,u8>> > > &offsets_and_types, > + std::unique_ptr<virtio_capability> &ptr, u8 type) > +{ > + u8 cfg_offset = 0xFF; > + for (auto cfg_offset_and_type: offsets_and_types) { > + auto cfg_type = cfg_offset_and_type.second; > + if (cfg_type == type) { > + cfg_offset = cfg_offset_and_type.first; > + break; > + } > + } > > if (cfg_offset != 0xFF) { > u8 bar_index = _dev->pci_readb(cfg_offset + offsetof(struct > virtio_pci_cap, bar)); > @@ -340,18 +358,16 @@ void > virtio_modern_pci_device::parse_virtio_capability(std::unique_ptr<virtio_ca > // drivers. The order of the capabilities in the capability list > specifies the > // order of preference suggested by the device. A device may specify that > this > // ordering mechanism be overridden by the use of the id field." > -void virtio_modern_pci_device::parse_virtio_capabilities( > - std::vector<std::unique_ptr<virtio_capability>>& caps, u8 type) > +void virtio_modern_pci_device::parse_virtio_capabilities( > std::vector<std::pair<u8,u8>> &offsets_and_types, > + > std::vector<std::unique_ptr<virtio_capability>>& caps, u8 type) > { > - std::vector<u8> cap_offs; > - _dev->find_capabilities(cap_offs, pci::function::PCI_CAP_VENDOR); > - > - for (auto cfg_offset: cap_offs) { > - u8 cfg_type = _dev->pci_readb(cfg_offset + > offsetof(virtio_pci_cap, cfg_type)); > + for (auto cfg_offset_and_type: offsets_and_types) { > + auto cfg_type = cfg_offset_and_type.second; > if (cfg_type != type) { > continue; > } > > + auto cfg_offset = cfg_offset_and_type.first; > u8 bar_index = _dev->pci_readb(cfg_offset + offsetof(struct > virtio_pci_cap, bar)); > auto bar_no = bar_index + 1; > auto bar = _dev->get_bar(bar_no); > diff --git a/drivers/virtio-pci-device.hh b/drivers/virtio-pci-device.hh > index 851b562a..9941bcac 100644 > --- a/drivers/virtio-pci-device.hh > +++ b/drivers/virtio-pci-device.hh > @@ -291,8 +291,11 @@ public: > protected: > virtual bool parse_pci_config(); > private: > - void parse_virtio_capability(std::unique_ptr<virtio_capability> &ptr, > u8 type); > - void > parse_virtio_capabilities(std::vector<std::unique_ptr<virtio_capability>>& > caps, u8 type); > + void find_vendor_capabilities(std::vector<std::pair<u8,u8>>& > offsets_and_types); > + void parse_virtio_capability(std::vector<std::pair<u8,u8>> > &offsets_and_types, > + std::unique_ptr<virtio_capability> &ptr, u8 type); > + void parse_virtio_capabilities(std::vector<std::pair<u8,u8>> > &offsets_and_types, > + std::vector<std::unique_ptr<virtio_capability>>& caps, u8 > type); > > std::unique_ptr<virtio_capability> _common_cfg; > std::unique_ptr<virtio_capability> _isr_cfg; > -- > 2.20.1 > > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/66ccb99d-0d5d-4b74-a42b-58acd1509d1f%40googlegroups.com.
