Looks good to me!
Also, nice to make pci::function::find_capability() private, de-cluttering
the public interface (since all uses are from other functions of
pci::function).
Τη Τετάρτη, 18 Μαρτίου 2020 - 5:34:29 μ.μ. UTC+2, ο χρήστης Waldemar
Kozaczuk έγραψε:
>
> ---
> drivers/pci-function.cc | 53 +++++++++++++----------------------
> drivers/pci-function.hh | 9 ++++--
> drivers/virtio-pci-device.cc | 54 +++++++++++++++++++++++-------------
> drivers/virtio-pci-device.hh | 7 +++--
> 4 files changed, 66 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/pci-function.cc b/drivers/pci-function.cc
> index b0fb3674..9d3641eb 100644
> --- a/drivers/pci-function.cc
> +++ b/drivers/pci-function.cc
> @@ -807,45 +807,28 @@ namespace pci {
> write_pci_config(_bus, _device, _func, offset, val);
> }
>
> - // Returns the offset of the first capability with id matching
> @cap_id, or
> - // 0xFF if none found.
> - u8 function::find_capability(u8 cap_id)
> + // 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(u8 cap_id, std::vector<u8>&
> cap_offs)
> {
> - return find_capability(cap_id, [](function *fun, u8 off) { return
> true; } );
> + return find_capabilities(cap_id, cap_offs, 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)
> + // Returns the offset of the first capability with id matching
> @cap_id, or
> + // 0xFF if none found.
> + u8 function::find_capability(u8 cap_id)
> {
> - 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 +841,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..9847b027 100644
> --- a/drivers/pci-function.hh
> +++ b/drivers/pci-function.hh
> @@ -340,9 +340,7 @@ namespace pci {
> virtual void pci_writel(u8 offset, u32 val);
>
> // 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);
>
> bar * get_bar(int idx);
> void add_bar(int idx, bar* bar);
> @@ -402,6 +400,11 @@ namespace pci {
> bool _have_msi;
> pcicfg_msi _msi;
> bool _msi_enabled;
> +
> + private:
> + // Capability parsing
> + u8 find_capability(u8 cap_id);
> + bool find_capabilities(u8 cap_id, std::vector<u8>& cap_offs, bool
> all);
> };
> }
>
> diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc
> index d484f3df..a793111a 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)) {
> + 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 osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/4b21a11e-8b02-4ed8-a1f1-65667c6e3d0f%40googlegroups.com.
