On Tuesday, March 17, 2020 at 6:54:09 PM UTC-4, Fotis Xenakis wrote: > > 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. > Good point. I will send an updated patch
> > Τη Τρίτη, 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]> >>> --- >>> 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/63a04631-85cd-4140-82a4-fd5a01081c22%40googlegroups.com.
