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 [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/4b21a11e-8b02-4ed8-a1f1-65667c6e3d0f%40googlegroups.com.
