[PATCH 0/2] virtio: enable shared memory regions

[Fotis Xenakis]

This is groundwork for supporting DAX windows in the virtio-fs driver.
It just adds the code to discover shared memory regions (for the modern
PCI transport only, which is used by the current implementation of
virtio-fs in QEMU), but does not yet expose them to the driver.

The overall process of using the DAX window consists of:
1. Discovering the (optional) shared memory region offered by the
virtio device. This is transport-specific.
2. Mapping the region in virtual memory as necessary.
3. Using it in the driver with the custom FUSE_SETUPMAPPING,
FUSE_REMOVEMAPPING requests as per the virtio-fs spec.

My next step is #2 above, mapping the device memory. For PCI at least,
this might be automatic with the current implementation: the memory
region is represented with a PCI BAR, which the current seems to be
mapped automatically, using pci::bar::map(). I need to look more into
this though, to get a better grasp of virtual memory in OSv. Any
references here would be welcome!

Since this is my first patch, please feel free to point out any
omission and room for improvement.

References:
[1] The official (draft) virtio specification:
https://github.com/oasis-tcs/virtio-spec
[2] A comprehensive summary of PCI configuration space:
https://wiki.osdev.org/Pci

Fotis Xenakis (2):
pci: allow 64-bit BAR offsets
virtio-pci: discover shared memory regions

drivers/pci-function.cc | 31 ++++++--------
drivers/pci-function.hh | 19 ++++-----
drivers/virtio-pci-device.cc | 78 +++++++++++++++++++++++++++++-------
drivers/virtio-pci-device.hh | 44 +++++++++++++-------
4 files changed, 114 insertions(+), 58 deletions(-)

--
2.25.1

[Fotis Xenakis]

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---
drivers/pci-function.cc | 31 +++++++++++++------------------
drivers/pci-function.hh | 19 ++++++++-----------
2 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/drivers/pci-function.cc b/drivers/pci-function.cc
index ac585bc6..369f22e9 100644
--- a/drivers/pci-function.cc
+++ b/drivers/pci-function.cc
@@ -20,16 +20,6 @@ namespace pci {
_addr_lo(0), _addr_hi(0), _addr_64(0), _addr_size(0),
_addr_mmio(mmio_nullptr),
_is_mmio(false), _is_64(false), _is_prefetchable(false)
- {
- init();
- }
-
- bar::~bar()
- {
-
- }
-
- void bar::init()
{
u32 val = _dev->pci_readl(_pos);

@@ -56,6 +46,11 @@ namespace pci {
_addr_64 = ((u64)_addr_hi << 32) | (u64)(_addr_lo);
}

+ bar::~bar()
+ {
+
+ }
+
u64 bar::read_bar_size()
{
u32 lo_orig = _dev->pci_readl(_pos);
@@ -110,7 +105,7 @@ namespace pci {
return _addr_mmio;
}

- u64 bar::readq(u32 offset)
+ u64 bar::readq(u64 offset)
{
if (_is_mmio) {
return mmio_getq(_addr_mmio + offset);
@@ -119,7 +114,7 @@ namespace pci {
}
}

- u32 bar::readl(u32 offset)
+ u32 bar::readl(u64 offset)
{
if (_is_mmio) {
return mmio_getl(_addr_mmio + offset);
@@ -128,7 +123,7 @@ namespace pci {
}
}

- u16 bar::readw(u32 offset)
+ u16 bar::readw(u64 offset)
{
if (_is_mmio) {
return mmio_getw(_addr_mmio + offset);
@@ -137,7 +132,7 @@ namespace pci {
}
}

- u8 bar::readb(u32 offset)
+ u8 bar::readb(u64 offset)
{
if (_is_mmio) {
return mmio_getb(_addr_mmio + offset);
@@ -146,7 +141,7 @@ namespace pci {
}
}

- void bar::writeq(u32 offset, u64 val)
+ void bar::writeq(u64 offset, u64 val)
{
if (_is_mmio) {
mmio_setq(_addr_mmio + offset, val);
@@ -155,7 +150,7 @@ namespace pci {
}
}

- void bar::writel(u32 offset, u32 val)
+ void bar::writel(u64 offset, u32 val)
{
if (_is_mmio) {
mmio_setl(_addr_mmio + offset, val);
@@ -164,7 +159,7 @@ namespace pci {
}
}

- void bar::writew(u32 offset, u16 val)
+ void bar::writew(u64 offset, u16 val)
{
if (_is_mmio) {
mmio_setw(_addr_mmio + offset, val);
@@ -173,7 +168,7 @@ namespace pci {
}
}

- void bar::writeb(u32 offset, u8 val)
+ void bar::writeb(u64 offset, u8 val)
{
if (_is_mmio) {
mmio_setb(_addr_mmio + offset, val);
diff --git a/drivers/pci-function.hh b/drivers/pci-function.hh
index 06ec96f6..59d57a1e 100644
--- a/drivers/pci-function.hh
+++ b/drivers/pci-function.hh
@@ -77,19 +77,16 @@ namespace pci {
mmioaddr_t get_mmio();

// Access the pio or mmio bar
- u64 readq(u32 offset);
- u32 readl(u32 offset);
- u16 readw(u32 offset);
- u8 readb(u32 offset);
- void writeq(u32 offset, u64 val);
- void writel(u32 offset, u32 val);
- void writew(u32 offset, u16 val);
- void writeb(u32 offset, u8 val);
+ u64 readq(u64 offset);
+ u32 readl(u64 offset);
+ u16 readw(u64 offset);
+ u8 readb(u64 offset);
+ void writeq(u64 offset, u64 val);
+ void writel(u64 offset, u32 val);
+ void writew(u64 offset, u16 val);
+ void writeb(u64 offset, u8 val);

private:
-
- void init();
-
/* Architecture-specific hook on bar creation, which allows
* rewriting the bar registers. Returns the bar register.
*/
--
2.25.1

[Fotis Xenakis]

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 <fo...@windowslive.com>
---

drivers/virtio-pci-device.cc | 78 +++++++++++++++++++++++++++++-------
drivers/virtio-pci-device.hh | 44 +++++++++++++-------

2 files changed, 93 insertions(+), 29 deletions(-)

diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc
index c107ddd7..8e02c1c1 100644
--- a/drivers/virtio-pci-device.cc
+++ b/drivers/virtio-pci-device.cc
@@ -269,37 +269,87 @@ 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);
+ parse_virtio_capability(_common_cfg, VIRTIO_PCI_CAP_COMMON_CFG, nullptr);
+ parse_virtio_capability(_isr_cfg, VIRTIO_PCI_CAP_ISR_CFG, nullptr);
+ parse_virtio_capability(_notify_cfg, VIRTIO_PCI_CAP_NOTIFY_CFG, nullptr);
+ parse_virtio_capability(_device_cfg, VIRTIO_PCI_CAP_DEVICE_CFG, nullptr);
+ parse_virtio_capabilities(_shm_cfgs, VIRTIO_PCI_CAP_SHARED_MEMORY_CFG);

if (_notify_cfg) {
- _notify_offset_multiplier =_dev->pci_readl(_notify_cfg->get_cfg_offset() +
+ _notify_offset_multiplier = _dev->pci_readl(_notify_cfg->get_cfg_offset() +
offsetof(virtio_pci_notify_cap, notify_offset_multiplier));
}

return _common_cfg && _isr_cfg && _notify_cfg && _device_cfg;
}

-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;
+// Parse all virtio PCI capabilities whose types match @type and append them to
+// @caps.
+// 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."
+void virtio_modern_pci_device::parse_virtio_capabilities(
+ std::vector<std::unique_ptr<virtio_capability>>& caps, u8 type)
+{
+ std::vector<u8> past_ids;
+ auto pred = [&past_ids] (u8 id) {
+ return std::none_of(past_ids.cbegin(), past_ids.cend(), [id] (u8 past_id) {
+ return past_id == id;
+ });
+ };
+
+ while (true) {
+ std::unique_ptr<virtio_capability> ptr;
+ // Search for a matching capability with an id not already in past_ids
+ parse_virtio_capability(ptr, type, pred);
+ if (!ptr) {
+ return;
+ }
+ u8 id = _dev->pci_readb(ptr->get_cfg_offset() + offsetof(virtio_pci_cap, id));
+ past_ids.push_back(id);
+ caps.emplace_back(ptr.release());
+ }
+}
+
+// Parse a single virtio PCI capability, whose type must match @type and id must
+// satisfy @id_pred.
+void virtio_modern_pci_device::parse_virtio_capability(std::unique_ptr<virtio_capability>& ptr, u8 type,
+ std::function<bool (u8)> id_pred)
+{
+ u8 cfg_offset = _dev->find_capability(pci::function::PCI_CAP_VENDOR,
+ [type, id_pred] (pci::function *fun, u8 offset) {
+ u8 cfg_type = fun->pci_readb(offset + offsetof(virtio_pci_cap, cfg_type));
+ if (cfg_type != type) {
+ return false;
+ }
+ u8 id = fun->pci_readb(offset + offsetof(virtio_pci_cap, id));
+ if (id_pred && !id_pred(id)) {
+ return false;
+ }
+ return true;
});

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));
-
+ 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();
}

+ 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);
+ }
+
ptr.reset(new virtio_modern_pci_device::virtio_capability(cfg_offset, bar, bar_no, offset, length));
}
}
diff --git a/drivers/virtio-pci-device.hh b/drivers/virtio-pci-device.hh
index 5a891d93..9a32aa18 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,15 @@ 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 parse_virtio_capability(std::unique_ptr<virtio_capability> &ptr, u8 type,
+ std::function<bool (u8)> id_pred);

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 +307,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

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

pci: allow 64-bit BAR offsets

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
Message-Id: <VI1PR03MB4383D028B2...@VI1PR03MB4383.eurprd03.prod.outlook.com>

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

[Waldek Kozaczuk]

Hey Fotis,

Thank you for the patches. I have applied the 1st one from the series. I have some questions and suggestions regarding this one though. Please see them below.

So the idea with the implementation of parse_virtio_capabilities() above is really to collect all capabilities of the given type (in our case so far VIRTIO_PCI_CAP_SHARED_MEMORY_CFG)? I wonder if the code can be simplified if we add a method function::find_capabilities(u8 cap_id) in pci-function.cc that will simply return a collection of matching offsets?

The general idea - we do not have to do it in this patch. I noticed we re-read same data in u8 function::find_capability() over and over (and each read is an exit I think) and that is what cause the pci initialization slower especially for modern devices (I see 10ms difference in boot time between running the same program with '--virtio modern' and '--virtio legacy' (default). Maybe some easy caching - again let us not worry now.

+
+// Parse a single virtio PCI capability, whose type must match @type and id must
+// satisfy @id_pred.
+void virtio_modern_pci_device::parse_virtio_capability(std::unique_ptr<virtio_capability>& ptr, u8 type,
+    std::function<bool (u8)> id_pred)
+{
+    u8 cfg_offset = _dev->find_capability(pci::function::PCI_CAP_VENDOR,
+        [type, id_pred] (pci::function *fun, u8 offset) {
+            u8 cfg_type = fun->pci_readb(offset + offsetof(virtio_pci_cap, cfg_type));
+            if (cfg_type != type) {
+                return false;
+            }
+            u8 id = fun->pci_readb(offset + offsetof(virtio_pci_cap, id));
+            if (id_pred && !id_pred(id)) {
+                return false;
+            }
+            return true;

Easy optimization suggestion (given that I think each pci_readb is an exit to hypervisor) - maybe first check if id_pred not null and only then do pci_readb() and call id_pred. Most parse_virtio_capability() calls pass id_pred as null, right?

[Fotis Xenakis]

This is exactly the purpose of parse_virtio_capabilities(): to parse all the virtio PCI capabilities of a specific type (virtio_pci_cap.cfg_type). I tried to make the patch non-pervasive as possible (not changing any public interface), so I haven't really thought how such changes could help simplify. I am looking into the option you suggested though, as it would sure help reduce the number of pci_read*() calls, if not also simplify the code!

It hadn't dawned on me that the pci_read*() calls probably cause VM exits (which makes sense) and was pretty liberal with them. I am revisiting the overall configuration parsing with this in mind and will come up with a v2 of the patch. My target is to avoid most of the duplicate calls to pci_read*(), while not making it too complex. Do you think there is any concern of the PCI configuration space changing during the parsing, prohibiting caching (seems unlikely, just a thought)?

Also, is it better if I include the changes that actually expose the shared memory regions to the drivers in v2 of the patch or create a new patch for that when this is merged?

+
+// Parse a single virtio PCI capability, whose type must match @type and id must
+// satisfy @id_pred.
+void virtio_modern_pci_device::parse_virtio_capability(std::unique_ptr<virtio_capability>& ptr, u8 type,
+    std::function<bool (u8)> id_pred)
+{
+    u8 cfg_offset = _dev->find_capability(pci::function::PCI_CAP_VENDOR,
+        [type, id_pred] (pci::function *fun, u8 offset) {
+            u8 cfg_type = fun->pci_readb(offset + offsetof(virtio_pci_cap, cfg_type));
+            if (cfg_type != type) {
+                return false;
+            }
+            u8 id = fun->pci_readb(offset + offsetof(virtio_pci_cap, id));
+            if (id_pred && !id_pred(id)) {
+                return false;
+            }
+            return true;

Easy optimization suggestion (given that I think each pci_readb is an exit to hypervisor) - maybe first check if id_pred not null and only then do pci_readb() and call id_pred. Most parse_virtio_capability() calls pass id_pred as null, right?

A very good suggestion indeed, I will include it in v2, thank you!

[Fotis Xenakis]

Changes since previous version:
- Refactored the discovery of the shm regions, taking into account
Waldek's input. I think this results in simpler code: the generic PCI
code (drivers/pci-function.cc) returns the offsets of all capabilities
of a given device and the virtio code (drivers/virtio-pci-device.cc)
iterates over them and parses the capabilities it expects. Also, this
should be somewhat more efficient, since there are no redundant calls
to pci::function::pci_read*() (that probably cause VM exits).
- Exposed the shared memory regions in the public interface of
virtio::virtio_device. Note that the corresponding implementations for
legacy PCI devices and MMIO devices are stubs: shm regions are not
supported on legaci PCI and MMIO is TODO (not urgent since there is no
VMM implementing virtio shared memory regions over MMIO right now that
I am aware of).

Please take a look, I will be glad to discuss further.

A note on performance: I did some rough comparison of current master
(752b6bb8b2fe01d693b9ffb751669ec476771087) and this, on QEMU, with a
single virtio-fs device. Boot times were pretty much equivalent: around
405 ms median in 6 runs each, so it seems that at least the discovery of
the shm regions as well as the refactor did not introduce a regression
there.
I did not compare against legacy virtio PCI because I am not sure how
fair that would be, I don't have a clear image of the legacy
implementation and wouldn't know what to expect out of such a comparison
to be honest.

Fotis Xenakis (3):
pci: add batch capability discovery

virtio-pci: discover shared memory regions

virtio: expose shared memory regions

drivers/pci-function.cc | 39 +++++++++--
drivers/pci-function.hh | 1 +
drivers/virtio-device.hh | 2 +
drivers/virtio-mmio.hh | 1 +
drivers/virtio-pci-device.cc | 121 ++++++++++++++++++++++++++++-------
drivers/virtio-pci-device.hh | 47 +++++++++-----
6 files changed, 167 insertions(+), 44 deletions(-)

--
2.25.1

[Fotis Xenakis]

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

drivers/pci-function.cc | 39 +++++++++++++++++++++++++++++++++------
drivers/pci-function.hh | 1 +
2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/pci-function.cc b/drivers/pci-function.cc
index 369f22e9..2cc8b837 100644
--- a/drivers/pci-function.cc
+++ b/drivers/pci-function.cc
@@ -807,36 +807,63 @@ 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)
{
- return this->find_capability(cap_id, [](function *fun, u8 off) { return true; } );
+ 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 bad_offset = 0xFF;
+ std::vector<u8> cap_offs;
+
+ if (!find_capabilities(cap_offs, cap_id)) {
+ return bad_offset;
+ }
+
+ if (!predicate) {
+ predicate = [](function *fun, u8 off) { return true; };
+ }
+ for (auto off: cap_offs) {
+ if (predicate(this, off)) {
+ return off;
+ }
+ }
+ 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)
{
u8 capabilities_base = pci_readb(PCI_CAPABILITIES_PTR);
u8 off = capabilities_base;
- u8 bad_offset = 0xFF;
u8 max_capabilities = 0xF0;
u8 ctr = 0;
+ bool found = false;

while (off != 0) {
// Read capability
u8 capability = pci_readb(off + PCI_CAP_OFF_ID);
- if (capability == cap_id && predicate(this, off)) {
- return off;
+ if (capability == cap_id) {
+ cap_offs.push_back(off);
+ found = true;
}

ctr++;
if (ctr > max_capabilities) {
- return bad_offset;
+ return found;
}

// Next
off = pci_readb(off + PCI_CAP_OFF_NEXT);
}

- return bad_offset;
+ return found;
}

bar * function::get_bar(int idx)
diff --git a/drivers/pci-function.hh b/drivers/pci-function.hh
index 59d57a1e..a8904cb0 100644
--- a/drivers/pci-function.hh
+++ b/drivers/pci-function.hh
@@ -342,6 +342,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);

bar * get_bar(int idx);
void add_bar(int idx, bar* bar);
--
2.25.1

[Fotis Xenakis]

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 <fo...@windowslive.com>
---

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

+ 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);
}

- 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

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

[Fotis Xenakis]

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

drivers/virtio-device.hh | 2 ++
drivers/virtio-mmio.hh | 1 +
drivers/virtio-pci-device.cc | 24 ++++++++++++++++++++++++
drivers/virtio-pci-device.hh | 4 ++++
4 files changed, 31 insertions(+)

diff --git a/drivers/virtio-device.hh b/drivers/virtio-device.hh
index 1327a159..732f9b18 100644
--- a/drivers/virtio-device.hh
+++ b/drivers/virtio-device.hh
@@ -76,6 +76,8 @@ public:
virtual u8 read_config(u32 offset) = 0;
virtual void dump_config() = 0;

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length) = 0;
+
virtual bool is_modern() = 0;
virtual size_t get_vring_alignment() = 0;
};
diff --git a/drivers/virtio-mmio.hh b/drivers/virtio-mmio.hh
index 024190ac..0cbf7000 100644
--- a/drivers/virtio-mmio.hh
+++ b/drivers/virtio-mmio.hh
@@ -136,6 +136,7 @@ public:

bool parse_config();

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length) { /* TODO */ return false; }
private:
mmio_device_info _dev_info;
//u64 _id;
diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc
index 0451923b..69cc7e22 100644
--- a/drivers/virtio-pci-device.cc
+++ b/drivers/virtio-pci-device.cc
@@ -253,6 +253,30 @@ u8 virtio_modern_pci_device::read_and_ack_isr()
return _isr_cfg->virtio_conf_readb(0);
}

+// Stores the address and length of the shared memory region identified by @id
+// in @addr and @length respectively. Returns false and doesn't modify @addr and
+// @length if no region with a matching id is found.
+bool virtio_modern_pci_device::get_shm(u8 id, mmioaddr_t &addr, u64 &length)
+{
+ auto cap = std::find_if(_shm_cfgs.cbegin(), _shm_cfgs.cend(),
+ [id, this] (const std::unique_ptr<virtio_capability>& cap) {
+ u8 cap_id = _dev->pci_readb(cap->get_cfg_offset() +
+ offsetof(virtio_pci_cap, id));
+ return cap_id == id;
+ });
+ if (cap == _shm_cfgs.cend()) {

+ return false;
+ }
+

+ auto bar = cap->get()->get_bar();
+ if (!bar->is_mmio()) {
+ return false;
+ }
+ addr = bar->get_mmio();
+ length = bar->get_size();
+ return true;
+}
+
bool virtio_modern_pci_device::parse_pci_config()
{
// Check ABI version
diff --git a/drivers/virtio-pci-device.hh b/drivers/virtio-pci-device.hh
index b8a5c02f..9a3c7b87 100644
--- a/drivers/virtio-pci-device.hh
+++ b/drivers/virtio-pci-device.hh
@@ -116,6 +116,8 @@ public:
virtual u8 read_and_ack_isr();

virtual bool is_modern() { return false; }
+

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length) { return false; }
protected:
virtual bool parse_pci_config();

@@ -243,6 +245,7 @@ public:

_bar->writel(_bar_offset + offset, val);
};

u32 get_cfg_offset() { return _cfg_offset; }
+ pci::bar* get_bar() { return _bar; }

void print(const char *prefix) {

virtio_d("%s bar=%d, offset=%x, size=%x", prefix, _bar_no, _bar_offset, _length);

@@ -284,6 +287,7 @@ public:

virtual bool is_modern() { return true; };

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length);

protected:
virtual bool parse_pci_config();
private:

--
2.25.1

[Waldek Kozaczuk]

Like it overall but see my suggestion below.

Let us keep the original implementation of "u8 function::find_capability(u8 cap_id, std::function<bool (function*, u8)> predicate)" as it is now. I think the current implementation of it is easier to understand and I do not see much benefit of it delegating to the new find_capabilities() function below.

I like your comments so please keep them of course. 

[Fotis Xenakis]

My rationale behind this change was to avoid duplication, but truth is the re-implementation turned out more complex than I imagined. I am reverting it to the original (with an added check for predicate) and will send a v3.

Another option I considered when thinking of how to avoid the duplication was to remove this function altogether (it is currently unused), but I turned it down. Since this was brought up though, I 'd like your opinion on it (pluralism vs smaller code base).

Thank you!

[Waldek Kozaczuk]

I think that sometimes a little duplication is not bad especially when it brings clarity to the code. No worries.  

[Waldek Kozaczuk]

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. 

Let us keep for now as it is. 

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.

[Waldek Kozaczuk]

It looks good to me. 

--
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+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/VI1PR03MB438350F248BA69EB1E8BFD17A6F80%40VI1PR03MB4383.eurprd03.prod.outlook.com.

[Fotis Xenakis]

Changes since previous version:
- Reverted the implementation of pci::function::find_capability() to its
original.
- Minimized the changes in virtio::virtio_modern_pci_device:
parse_virtio_capability() is left the same as current master and a
parse_virtio_capabilities() is added to handle multiple capabilities
of the same type.

Fotis Xenakis (3):
pci: add batch capability discovery
virtio-pci: discover shared memory regions
virtio: expose shared memory regions

drivers/pci-function.cc | 36 +++++++++++++++-

drivers/pci-function.hh | 1 +
drivers/virtio-device.hh | 2 +
drivers/virtio-mmio.hh | 1 +

drivers/virtio-pci-device.cc | 79 ++++++++++++++++++++++++++++++++++--
drivers/virtio-pci-device.hh | 45 +++++++++++++-------
6 files changed, 145 insertions(+), 19 deletions(-)

--
2.25.1

[Fotis Xenakis]

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

drivers/pci-function.cc | 36 +++++++++++++++++++++++++++++++++++-
drivers/pci-function.hh | 1 +
2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/pci-function.cc b/drivers/pci-function.cc
index 369f22e9..b0fb3674 100644
--- a/drivers/pci-function.cc
+++ b/drivers/pci-function.cc
@@ -807,11 +807,15 @@ 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)
{
- return this->find_capability(cap_id, [](function *fun, u8 off) { return true; } );
+ 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);
@@ -839,6 +843,36 @@ namespace pci {
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)

+ {
+ u8 capabilities_base = pci_readb(PCI_CAPABILITIES_PTR);
+ u8 off = capabilities_base;
+ u8 max_capabilities = 0xF0;
+ u8 ctr = 0;

+ bool found = false;

+
+ while (off != 0) {
+ // Read capability
+ u8 capability = pci_readb(off + PCI_CAP_OFF_ID);

+ if (capability == cap_id) {
+ cap_offs.push_back(off);
+ found = true;

+ }
+
+ ctr++;
+ if (ctr > max_capabilities) {
+ return found;
+ }
+
+ // Next
+ off = pci_readb(off + PCI_CAP_OFF_NEXT);
+ }
+
+ return found;
+ }
+

bar * function::get_bar(int idx)

{
auto it = _bars.find(idx);

[Fotis Xenakis]

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 <fo...@windowslive.com>
---

drivers/virtio-pci-device.cc | 55 +++++++++++++++++++++++++++++++++---
drivers/virtio-pci-device.hh | 41 ++++++++++++++++++---------
2 files changed, 78 insertions(+), 18 deletions(-)

diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc
index c107ddd7..a88e3d2b 100644
--- a/drivers/virtio-pci-device.cc
+++ b/drivers/virtio-pci-device.cc
@@ -269,19 +269,24 @@ 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);

if (_notify_cfg) {

_notify_offset_multiplier =_dev->pci_readl(_notify_cfg->get_cfg_offset() +

offsetof(virtio_pci_notify_cap, notify_offset_multiplier));

}

- return _common_cfg && _isr_cfg && _notify_cfg && _device_cfg;
+ // The common, isr and notifications configurations are mandatory
+ return _common_cfg && _isr_cfg && _notify_cfg;
}

+// 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) {

@@ -291,19 +296,61 @@ void virtio_modern_pci_device::parse_virtio_capability(std::unique_ptr<virtio_ca

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

-
auto bar_no = bar_index + 1;

auto bar = _dev->get_bar(bar_no);

if (bar && bar->is_mmio() && !bar->is_mapped()) {

bar->map();
}

+ 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));
+
ptr.reset(new virtio_modern_pci_device::virtio_capability(cfg_offset, bar, bar_no, offset, length));

}
}

+// Parse all virtio PCI capabilities whose types match @type and append them to
+// @caps.

+// 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."
+void virtio_modern_pci_device::parse_virtio_capabilities(
+ 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));

+ if (cfg_type != type) {
+ continue;
+ }
+
+ 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();
+ }

+
+ 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));

+ 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);
+ }

+
+ caps.emplace_back(new virtio_modern_pci_device::virtio_capability(
+ cfg_offset, bar, bar_no, offset, length));
+ }
+}
+
virtio_device* create_virtio_pci_device(pci::device *dev) {
if (dev->get_device_id() >= VIRTIO_PCI_MODERN_ID_MIN && dev->get_device_id() <= VIRTIO_PCI_MODERN_ID_MAX)
return new virtio_modern_pci_device(dev);
diff --git a/drivers/virtio-pci-device.hh b/drivers/virtio-pci-device.hh
index 5a891d93..a2c70e28 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();

_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);

@@ -277,11 +288,13 @@ 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);

[Fotis Xenakis]

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

drivers/virtio-device.hh | 2 ++
drivers/virtio-mmio.hh | 1 +
drivers/virtio-pci-device.cc | 24 ++++++++++++++++++++++++
drivers/virtio-pci-device.hh | 4 ++++
4 files changed, 31 insertions(+)

diff --git a/drivers/virtio-device.hh b/drivers/virtio-device.hh
index 1327a159..732f9b18 100644
--- a/drivers/virtio-device.hh
+++ b/drivers/virtio-device.hh

@@ -76,6 +76,8 @@ public:

virtual u8 read_config(u32 offset) = 0;
virtual void dump_config() = 0;

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length) = 0;
+
virtual bool is_modern() = 0;
virtual size_t get_vring_alignment() = 0;
};
diff --git a/drivers/virtio-mmio.hh b/drivers/virtio-mmio.hh
index 024190ac..0cbf7000 100644
--- a/drivers/virtio-mmio.hh
+++ b/drivers/virtio-mmio.hh
@@ -136,6 +136,7 @@ public:

bool parse_config();

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length) { /* TODO */ return false; }
private:
mmio_device_info _dev_info;
//u64 _id;

diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc
index a88e3d2b..d484f3df 100644
--- a/drivers/virtio-pci-device.cc
+++ b/drivers/virtio-pci-device.cc

diff --git a/drivers/virtio-pci-device.hh b/drivers/virtio-pci-device.hh
index a2c70e28..851b562a 100644
--- a/drivers/virtio-pci-device.hh
+++ b/drivers/virtio-pci-device.hh

@@ -116,6 +116,8 @@ public:
virtual u8 read_and_ack_isr();

virtual bool is_modern() { return false; }
+

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length) { return false; }
protected:
virtual bool parse_pci_config();

@@ -243,6 +245,7 @@ public:

_bar->writel(_bar_offset + offset, val);
};

u32 get_cfg_offset() { return _cfg_offset; }
+ pci::bar* get_bar() { return _bar; }

void print(const char *prefix) {

virtio_d("%s bar=%d, offset=%x, size=%x", prefix, _bar_no, _bar_offset, _length);

@@ -284,6 +287,7 @@ public:

virtual bool is_modern() { return true; };

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length);

protected:
virtual bool parse_pci_config();
private:

--
2.25.1

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

pci: add batch capability discovery

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
Message-Id: <VI1PR03MB4383120D29...@VI1PR03MB4383.eurprd03.prod.outlook.com>

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

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio-pci: discover shared memory regions

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 <fo...@windowslive.com>

Message-Id: <VI1PR03MB438308A0EF...@VI1PR03MB4383.eurprd03.prod.outlook.com>

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

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio: expose shared memory regions

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
Message-Id: <VI1PR03MB4383690324...@VI1PR03MB4383.eurprd03.prod.outlook.com>

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

--- a/drivers/virtio-device.hh
+++ b/drivers/virtio-device.hh
@@ -76,6 +76,8 @@ public:
virtual u8 read_config(u32 offset) = 0;
virtual void dump_config() = 0;

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length) = 0;
+
virtual bool is_modern() = 0;
virtual size_t get_vring_alignment() = 0;
};
diff --git a/drivers/virtio-mmio.hh b/drivers/virtio-mmio.hh

--- a/drivers/virtio-mmio.hh
+++ b/drivers/virtio-mmio.hh
@@ -136,6 +136,7 @@ public:

bool parse_config();

+ virtual bool get_shm(u8 id, mmioaddr_t &addr, u64 &length) { /* TODO */ return false; }
private:
mmio_device_info _dev_info;
//u64 _id;
diff --git a/drivers/virtio-pci-device.cc b/drivers/virtio-pci-device.cc