The patches are as correct and according to spec as the code previously was, so I think they are good, and testing agrees with this. Also, the code is pretty clear (to me at least).
The only (minor) point I could consider changing is making pci::function::find_capabilities(u8 cap_id, std::vector<u8>& cap_offs, bool all) private and exposing a public pci::function::find_capabilities(u8 cap_id, std::vector<u8>& cap_offs) implemented as a proxy to the private function of course, so that the public interface is slightly more clear (with the current patch find_capability() and find_capabilities() overlap in functionality). But of course this is very minor and probably a matter of taste. Τη Τρίτη, 17 Μαρτίου 2020 - 6:46:31 π.μ. UTC+2, ο χρήστης Waldek Kozaczuk έγραψε: > > 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] <javascript:>> >> --- >> 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/6a045a5c-0ec4-4023-bc83-382b931bf0ba%40googlegroups.com.
