I think you are trying to bite too much in this patch. I would suggest taking a more incremental approach and just implement void virtio_modern_pci_device::parse_virtio_capabilities() like you had in V1 patch and any other necessary changes to virtio_modern_pci_device() as minimal as possible to implement new functionality to detect and expose shared memory regions.
Let us worry about optimizing the code later possibly in a separate series of patches. Let us focus on getting the functionality right first. On Sunday, March 15, 2020 at 7:45:37 AM UTC-4, Fotis Xenakis wrote: > > The latest virtio spec adds shared memory regions: > "Shared memory regions are an additional facility available to devices > that need a region of memory that’s continuously shared between the > device and the driver, rather than passed between them in the way > virtqueue elements are." > > In virtio over PCI, these are enumerated as a sequence of > VIRTIO_PCI_CAP_SHARED_MEMORY_CFG capabilities, one per region. > > This patch extends the virtio over PCI implementation to discover all > such regions provided by a device. > > Signed-off-by: Fotis Xenakis <[email protected] <javascript:>> > --- > drivers/virtio-pci-device.cc | 97 +++++++++++++++++++++++++++--------- > drivers/virtio-pci-device.hh | 43 ++++++++++------ > 2 files changed, 102 insertions(+), 38 deletions(-) > > diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc > index c107ddd7..0451923b 100644 > --- a/drivers/virtio-pci-device.cc > +++ b/drivers/virtio-pci-device.cc > @@ -269,38 +269,89 @@ bool virtio_modern_pci_device::parse_pci_config() > return false; > } > > - 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); Let us keep for now as it is. > > + return parse_virtio_capabilities(); > +} > > - if (_notify_cfg) { > - _notify_offset_multiplier > =_dev->pci_readl(_notify_cfg->get_cfg_offset() + > - offsetof(virtio_pci_notify_cap, > notify_offset_multiplier)); > +// Parse all virtio capabilities of the device. Returns whether all > mandatory > +// capabilities were parsed successfully. > +bool virtio_modern_pci_device::parse_virtio_capabilities() > +{ > + std::vector<u8> cap_offs; > + if (!_dev->find_capabilities(cap_offs, > pci::function::PCI_CAP_VENDOR)) { > + return false; > + } > + > + for (auto cfg_offset: cap_offs) { > + u8 cfg_type = _dev->pci_readb(cfg_offset + > offsetof(virtio_pci_cap, cfg_type)); > + std::unique_ptr<virtio_capability>* cfg = nullptr; > + > + switch (cfg_type) { > + case VIRTIO_PCI_CAP_COMMON_CFG: > + cfg = &_common_cfg; > + break; > + case VIRTIO_PCI_CAP_ISR_CFG: > + cfg = &_isr_cfg; > + break; > + case VIRTIO_PCI_CAP_NOTIFY_CFG: > + cfg = &_notify_cfg; > + break; > + case VIRTIO_PCI_CAP_DEVICE_CFG: > + cfg = &_device_cfg; > + break; > + case VIRTIO_PCI_CAP_SHARED_MEMORY_CFG: > + _shm_cfgs.emplace_back(); > + cfg = &_shm_cfgs.back(); > + break; > + default: > + continue; > + } > + > + // Don't overwrite a cfg which was already parsed. From the spec: > "The > + // device MAY offer more than one structure of any type - this > makes it > + // possible for the device to expose multiple interfaces to > 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." > + if (*cfg) { > + continue; > + } > + > + parse_virtio_capability(*cfg, cfg_type, cfg_offset); > } > For simplicity let use just directly call bool pci_function::find_capabilities(std::vector<u8>& caps, u8 cap_id) you added in the 1st patch of the V2 series. > > - return _common_cfg && _isr_cfg && _notify_cfg && _device_cfg; > + // The common, isr and notifications configurations are mandatory > + return _common_cfg && _isr_cfg && _notify_cfg; > } > > -void > virtio_modern_pci_device::parse_virtio_capability(std::unique_ptr<virtio_capability> > > &ptr, u8 type) > +// Parse the virtio capability at @cfg_offset in the configuration space, > which > +// is of type @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) > { > - 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; > - }); > + u8 bar_index = _dev->pci_readb(cfg_offset + offsetof(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(); > + } > > - if (cfg_offset != 0xFF) { > - u8 bar_index = _dev->pci_readb(cfg_offset + offsetof(struct > virtio_pci_cap, bar)); > - u32 offset = _dev->pci_readl(cfg_offset + offsetof(struct > virtio_pci_cap, offset)); > - u32 length = _dev->pci_readl(cfg_offset + offsetof(struct > virtio_pci_cap, length)); > + u64 offset = _dev->pci_readl(cfg_offset + offsetof(virtio_pci_cap, > offset)); > + u64 length = _dev->pci_readl(cfg_offset + offsetof(virtio_pci_cap, > length)); > + if (type == VIRTIO_PCI_CAP_SHARED_MEMORY_CFG) { > + // The shared memory region capability is defined by a struct > + // virtio_pci_cap64 > + u32 offset_hi = _dev->pci_readl(cfg_offset + > offsetof(virtio_pci_cap64, offset_hi)); > + u32 length_hi = _dev->pci_readl(cfg_offset + > offsetof(virtio_pci_cap64, length_hi)); > + offset |= ((u64)offset_hi << 32); > + length |= ((u64)length_hi << 32); > + } > > - auto bar_no = bar_index + 1; > - auto bar = _dev->get_bar(bar_no); > - if (bar && bar->is_mmio() && !bar->is_mapped()) { > - bar->map(); > - } > + ptr.reset(new virtio_modern_pci_device::virtio_capability(cfg_offset, > bar, > + bar_no, offset, length)); > > - ptr.reset(new > virtio_modern_pci_device::virtio_capability(cfg_offset, bar, bar_no, > offset, length)); > + if (type == VIRTIO_PCI_CAP_NOTIFY_CFG) { > + _notify_offset_multiplier = _dev->pci_readl(cfg_offset + > + offsetof(virtio_pci_notify_cap, > notify_offset_multiplier)); > } > } > > diff --git a/drivers/virtio-pci-device.hh b/drivers/virtio-pci-device.hh > index 5a891d93..b8a5c02f 100644 > --- a/drivers/virtio-pci-device.hh > +++ b/drivers/virtio-pci-device.hh > @@ -98,7 +98,7 @@ public: > ~virtio_legacy_pci_device() {} > > virtual const char *get_version() { return "legacy"; } > - virtual u16 get_type_id() { return _dev->get_subsystem_id(); }; > + virtual u16 get_type_id() { return _dev->get_subsystem_id(); } > > virtual void select_queue(int queue); > virtual u16 get_queue_size(); > @@ -115,7 +115,7 @@ public: > virtual u8 read_config(u32 offset); > virtual u8 read_and_ack_isr(); > > - virtual bool is_modern() { return false; }; > + virtual bool is_modern() { return false; } > protected: > virtual bool parse_pci_config(); > > @@ -145,6 +145,8 @@ enum VIRTIO_MODERN_PCI_CONFIG { > VIRTIO_PCI_CAP_DEVICE_CFG = 4, > /* PCI configuration access */ > VIRTIO_PCI_CAP_PCI_CFG = 5, > + /* Shared memory region */ > + VIRTIO_PCI_CAP_SHARED_MEMORY_CFG = 8, > }; > > /* This is the PCI capability header: */ > @@ -154,11 +156,20 @@ struct virtio_pci_cap { > u8 cap_len; /* Generic PCI field: capability length */ > u8 cfg_type; /* Identifies the structure. */ > u8 bar; /* Where to find it. */ > - u8 padding[3]; /* Pad to full dword. */ > + u8 id; /* Multiple capabilities of the same type */ > + u8 padding[2]; /* Pad to full dword. */ > u32 offset; /* Offset within bar. */ > u32 length; /* Length of the structure, in bytes. */ > }; > > +/* A variant of virtio_pci_cap, for capabilities that require offsets or > lengths > + * larger than 4GiB */ > +struct virtio_pci_cap64 { > + struct virtio_pci_cap cap; > + u32 offset_hi; > + u32 length_hi; > +}; > + > /* The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG > capability. > * This capability is immediately followed by an additional field, like > so:*/ > struct virtio_pci_notify_cap { > @@ -198,7 +209,7 @@ struct virtio_pci_common_cfg { > class virtio_modern_pci_device : public virtio_pci_device { > public: > struct virtio_capability { > - virtio_capability(u32 cfg_offset, pci::bar* bar, u32 bar_no, u32 > bar_offset, u32 length) : > + virtio_capability(u32 cfg_offset, pci::bar* bar, u32 bar_no, u64 > bar_offset, u64 length) : > _cfg_offset(cfg_offset), > _bar(bar), > _bar_no(bar_no), > @@ -207,27 +218,27 @@ public: > assert(_length > 0 && _bar_offset >= 0 && _bar_offset + > _length <= _bar->get_size()); > } > > - u8 virtio_conf_readb(u32 offset) { > + u8 virtio_conf_readb(u64 offset) { > verify_offset(offset, sizeof(u8)); > return _bar->readb(_bar_offset + offset); > }; > - u16 virtio_conf_readw(u32 offset) { > + u16 virtio_conf_readw(u64 offset) { > verify_offset(offset, sizeof(u16)); > return _bar->readw(_bar_offset + offset); > }; > - u32 virtio_conf_readl(u32 offset) { > + u32 virtio_conf_readl(u64 offset) { > verify_offset(offset, sizeof(u32)); > return _bar->readl(_bar_offset + offset); > }; > - void virtio_conf_writeb(u32 offset, u8 val) { > + void virtio_conf_writeb(u64 offset, u8 val) { > verify_offset(offset, sizeof(u8)); > _bar->writeb(_bar_offset + offset, val); > }; > - void virtio_conf_writew(u32 offset, u16 val) { > + void virtio_conf_writew(u64 offset, u16 val) { > verify_offset(offset, sizeof(u16)); > _bar->writew(_bar_offset + offset, val); > }; > - void virtio_conf_writel(u32 offset, u32 val) { > + void virtio_conf_writel(u64 offset, u32 val) { > verify_offset(offset, sizeof(u32)); > _bar->writel(_bar_offset + offset, val); > }; > @@ -237,15 +248,15 @@ public: > virtio_d("%s bar=%d, offset=%x, size=%x", prefix, _bar_no, > _bar_offset, _length); > } > private: > - inline void verify_offset(u32 offset, u32 size) { > + inline void verify_offset(u64 offset, u32 size) { > assert(offset >= 0 && offset + size <= _length); > } > > u32 _cfg_offset; > pci::bar* _bar; > u32 _bar_no; > - u32 _bar_offset; > - u32 _length; > + u64 _bar_offset; > + u64 _length; > }; > > explicit virtio_modern_pci_device(pci::device *dev); > @@ -276,12 +287,14 @@ public: > protected: > virtual bool parse_pci_config(); > private: > - void parse_virtio_capability(std::unique_ptr<virtio_capability> &ptr, > u8 type); > + bool parse_virtio_capabilities(); > + void parse_virtio_capability(std::unique_ptr<virtio_capability>& ptr, > u8 type, u8 cfg_offset); > > std::unique_ptr<virtio_capability> _common_cfg; > std::unique_ptr<virtio_capability> _isr_cfg; > std::unique_ptr<virtio_capability> _notify_cfg; > std::unique_ptr<virtio_capability> _device_cfg; > + std::vector<std::unique_ptr<virtio_capability>> _shm_cfgs; > > u32 _notify_offset_multiplier; > u32 _queues_notify_offsets[64]; > @@ -293,4 +306,4 @@ virtio_device* create_virtio_pci_device(pci::device > *dev); > > } > > -#endif //VIRTIO_PCI_DEVICE_HH > \ No newline at end of file > +#endif //VIRTIO_PCI_DEVICE_HH > -- > 2.25.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/5c1a7498-4f96-4e5d-82ca-d82d1a0b8660%40googlegroups.com.
