[PATCH 1/2] virtio: optimize number of exits when parsing capabilities

[Waldemar Kozaczuk]

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
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

[Waldemar Kozaczuk]

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---

drivers/virtio-pci-device.cc | 27 +++++++++++++++------------
drivers/virtio-pci-device.hh | 1 +
2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc
index 89130e01..4171f3a6 100644
--- a/drivers/virtio-pci-device.cc
+++ b/drivers/virtio-pci-device.cc
@@ -322,6 +322,17 @@ bool virtio_modern_pci_device::parse_pci_config()
return _common_cfg && _isr_cfg && _notify_cfg;
}

+pci::bar* virtio_modern_pci_device::map_capability_bar(u8 cap_offset, u8 &bar_no)
+{
+ u8 bar_index = _dev->pci_readb(cap_offset + offsetof(struct virtio_pci_cap, bar));
+ bar_no = bar_index + 1;
+ auto bar = _dev->get_bar(bar_no);
+ if (bar && bar->is_mmio() && !bar->is_mapped()) {
+ bar->map();
+ }
+ return bar;
+}
+

// 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::vector<std::pair<u8,u8>> &offsets_and_types,

@@ -337,12 +348,8 @@ void virtio_modern_pci_device::parse_virtio_capability(std::vector<std::pair<u8,
}

if (cfg_offset != 0xFF) {
- 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);
- if (bar && bar->is_mmio() && !bar->is_mapped()) {
- bar->map();
- }
+ u8 bar_no;
+ auto bar = map_capability_bar(cfg_offset, bar_no);

u64 offset = _dev->pci_readl(cfg_offset + offsetof(struct virtio_pci_cap, offset));
u64 length = _dev->pci_readl(cfg_offset + offsetof(struct virtio_pci_cap, length));
@@ -368,12 +375,8 @@ void virtio_modern_pci_device::parse_virtio_capabilities( std::vector<std::pair<
}

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);
- if (bar && bar->is_mmio() && !bar->is_mapped()) {
- bar->map();
- }
+ u8 bar_no;
+ auto bar = map_capability_bar(cfg_offset, bar_no);

u64 offset = _dev->pci_readl(cfg_offset + offsetof(struct virtio_pci_cap, offset));
u64 length = _dev->pci_readl(cfg_offset + offsetof(struct virtio_pci_cap, length));
diff --git a/drivers/virtio-pci-device.hh b/drivers/virtio-pci-device.hh
index 9941bcac..701fe820 100644
--- a/drivers/virtio-pci-device.hh
+++ b/drivers/virtio-pci-device.hh
@@ -292,6 +292,7 @@ protected:
virtual bool parse_pci_config();
private:

void find_vendor_capabilities(std::vector<std::pair<u8,u8>>& offsets_and_types);

+ pci::bar* map_capability_bar(u8 cap_offset, u8 &bar_no);

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,

--
2.20.1

[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.

[Fotis Xenakis]

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.

[Waldek Kozaczuk]

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 

[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)

@@ -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)) {

[Fotis Xenakis]

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).

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio: optimize number of exits when parsing capabilities

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

---

diff --git a/drivers/pci-function.cc b/drivers/pci-function.cc

--- 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

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio: extract common logic to map capability bar

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

---

diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc
--- a/drivers/virtio-pci-device.cc
+++ b/drivers/virtio-pci-device.cc

@@ -322,6 +322,17 @@ bool virtio_modern_pci_device::parse_pci_config()
return _common_cfg && _isr_cfg && _notify_cfg;
}

+pci::bar* virtio_modern_pci_device::map_capability_bar(u8 cap_offset, u8 &bar_no)
+{
+ u8 bar_index = _dev->pci_readb(cap_offset + offsetof(struct virtio_pci_cap, bar));
+ bar_no = bar_index + 1;
+ auto bar = _dev->get_bar(bar_no);
+ if (bar && bar->is_mmio() && !bar->is_mapped()) {
+ bar->map();
+ }
+ return bar;
+}
+

// 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::vector<std::pair<u8,u8>> &offsets_and_types,

@@ -337,12 +348,8 @@ void virtio_modern_pci_device::parse_virtio_capability(std::vector<std::pair<u8,
}

if (cfg_offset != 0xFF) {
- 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);
- if (bar && bar->is_mmio() && !bar->is_mapped()) {
- bar->map();
- }
+ u8 bar_no;
+ auto bar = map_capability_bar(cfg_offset, bar_no);

u64 offset = _dev->pci_readl(cfg_offset + offsetof(struct virtio_pci_cap, offset));
u64 length = _dev->pci_readl(cfg_offset + offsetof(struct virtio_pci_cap, length));
@@ -368,12 +375,8 @@ void virtio_modern_pci_device::parse_virtio_capabilities( std::vector<std::pair<
}

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);
- if (bar && bar->is_mmio() && !bar->is_mapped()) {
- bar->map();
- }
+ u8 bar_no;
+ auto bar = map_capability_bar(cfg_offset, bar_no);

u64 offset = _dev->pci_readl(cfg_offset + offsetof(struct virtio_pci_cap, offset));
u64 length = _dev->pci_readl(cfg_offset + offsetof(struct virtio_pci_cap, length));

diff --git a/drivers/virtio-pci-device.hh b/drivers/virtio-pci-device.hh
--- a/drivers/virtio-pci-device.hh
+++ b/drivers/virtio-pci-device.hh

@@ -292,6 +292,7 @@ protected:
virtual bool parse_pci_config();
private:

void find_vendor_capabilities(std::vector<std::pair<u8,u8>>& offsets_and_types);

+ pci::bar* map_capability_bar(u8 cap_offset, u8 &bar_no);

void parse_virtio_capability(std::vector<std::pair<u8,u8>> &offsets_and_types,

std::unique_ptr<virtio_capability> &ptr, u8 type);