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 <jwkoz...@gmail.com <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 osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/6a045a5c-0ec4-4023-bc83-382b931bf0ba%40googlegroups.com.
