From: Waldemar Kozaczuk <[email protected]>
Committer: Nadav Har'El <[email protected]>
Branch: master
Clean virtio code
This patch cleans and refines various aspects of virtio
code. More specifically it:
- removes unused methods
- corrects logic to read elements of virtio configuration only
if corresponding feature bits indicate so
- adds step 6 of modern virtio device initialization
- adds code to register gsi edge interrupt handlers that will
be used by virtio mmio drivers code in next patch
Signed-off-by: Waldemar Kozaczuk <[email protected]>
Message-Id: <[email protected]>
---
diff --git a/drivers/virtio-blk.cc b/drivers/virtio-blk.cc
--- a/drivers/virtio-blk.cc
+++ b/drivers/virtio-blk.cc
@@ -115,12 +115,11 @@ bool blk::ack_irq()
blk::blk(virtio_device& virtio_dev)
: virtio_driver(virtio_dev), _ro(false)
{
-
_driver_name = "virtio-blk";
_id = _instance++;
virtio_i("VIRTIO BLK INSTANCE %d", _id);
- // Steps 4 & 5 - negotiate and confirm features
+ // Steps 4, 5 & 6 - negotiate and confirm features
setup_features();
read_config();
@@ -144,6 +143,13 @@ blk::blk(virtio_device& virtio_dev)
[=] { return this->ack_irq(); },
[=] { t->wake(); });
};
+
+ int_factory.create_gsi_edge_interrupt = [this,t]() {
+ return new gsi_edge_interrupt(
+ _dev.get_irq(),
+ [=] { if (this->ack_irq()) t->wake(); });
+ };
+
_dev.register_interrupt(int_factory);
// Enable indirect descriptor
@@ -172,33 +178,34 @@ blk::~blk()
// including the thread objects and their stack
}
+#define READ_CONFIGURATION_FIELD(config,field_name,field) \
+ virtio_conf_read(offsetof(config,field_name), &field, sizeof(field));
+
void blk::read_config()
{
- if (_dev.is_modern()) {
- //TODO: It may to do with legacy vs non-legacy device
- //but at least with latest spec we should check if individual
- //config fields are available vs reading whole config struct. For
example
- //firecracker reports memory read violation warnings
- virtio_conf_read(0, &_config, sizeof(_config.capacity));
- }
- else {
- //read all of the block config (including size, mce, topology,..)
in one shot
- virtio_conf_read(0, &_config, sizeof(_config));
- }
-
+ READ_CONFIGURATION_FIELD(blk_config,capacity,_config.capacity)
trace_virtio_blk_read_config_capacity(_config.capacity);
- if (get_guest_feature_bit(VIRTIO_BLK_F_SIZE_MAX))
+ if (get_guest_feature_bit(VIRTIO_BLK_F_SIZE_MAX)) {
+ READ_CONFIGURATION_FIELD(blk_config,size_max,_config.size_max)
trace_virtio_blk_read_config_size_max(_config.size_max);
- if (get_guest_feature_bit(VIRTIO_BLK_F_SEG_MAX))
+ }
+ if (get_guest_feature_bit(VIRTIO_BLK_F_SEG_MAX)) {
+ READ_CONFIGURATION_FIELD(blk_config,seg_max,_config.seg_max)
trace_virtio_blk_read_config_seg_max(_config.seg_max);
+ }
if (get_guest_feature_bit(VIRTIO_BLK_F_GEOMETRY)) {
+ READ_CONFIGURATION_FIELD(blk_config,geometry,_config.geometry)
trace_virtio_blk_read_config_geometry((u32)_config.geometry.cylinders,
(u32)_config.geometry.heads, (u32)_config.geometry.sectors);
}
- if (get_guest_feature_bit(VIRTIO_BLK_F_BLK_SIZE))
+ if (get_guest_feature_bit(VIRTIO_BLK_F_BLK_SIZE)) {
+ READ_CONFIGURATION_FIELD(blk_config,blk_size,_config.blk_size)
trace_virtio_blk_read_config_blk_size(_config.blk_size);
+ }
if (get_guest_feature_bit(VIRTIO_BLK_F_TOPOLOGY)) {
-
trace_virtio_blk_read_config_topology((u32)_config.physical_block_exp,
(u32)_config.alignment_offset, (u32)_config.min_io_size,
(u32)_config.opt_io_size);
+ READ_CONFIGURATION_FIELD(blk_config,topology,_config.topology)
+
trace_virtio_blk_read_config_topology((u32)_config.topology.physical_block_exp,
(u32)_config.topology.alignment_offset,
+ (u32)_config.topology.min_io_size,
(u32)_config.topology.opt_io_size);
}
if (get_guest_feature_bit(VIRTIO_BLK_F_CONFIG_WCE))
trace_virtio_blk_read_config_wce((u32)_config.wce);
@@ -260,8 +267,7 @@ int blk::make_request(struct bio* bio)
if (!bio) return EIO;
- // TODO: Check if seg_max is unavailable if modern ...
- if (!_dev.is_modern()) {
+ if (get_guest_feature_bit(VIRTIO_BLK_F_SEG_MAX)) {
if (bio->bio_bcount/mmu::page_size + 1 > _config.seg_max) {
trace_virtio_blk_make_request_seg_max(bio->bio_bcount,
_config.seg_max);
return EIO;
diff --git a/drivers/virtio-blk.hh b/drivers/virtio-blk.hh
--- a/drivers/virtio-blk.hh
+++ b/drivers/virtio-blk.hh
@@ -82,15 +82,17 @@ public:
/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
u32 blk_size;
- /* the next 4 entries are guarded by VIRTIO_BLK_F_TOPOLOGY */
- /* exponent for physical block per logical block. */
- u8 physical_block_exp;
- /* alignment offset in logical blocks. */
- u8 alignment_offset;
- /* minimum I/O size without performance penalty in logical
blocks. */
- u16 min_io_size;
- /* optimal sustained I/O size in logical blocks. */
- u32 opt_io_size;
+ struct blk_topology {
+ /* the next 4 entries are guarded by
VIRTIO_BLK_F_TOPOLOGY */
+ /* exponent for physical block per logical block. */
+ u8 physical_block_exp;
+ /* alignment offset in logical blocks. */
+ u8 alignment_offset;
+ /* minimum I/O size without performance penalty in
logical blocks. */
+ u16 min_io_size;
+ /* optimal sustained I/O size in logical blocks. */
+ u32 opt_io_size;
+ } topology;
/* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
u8 wce;
diff --git a/drivers/virtio-device.hh b/drivers/virtio-device.hh
--- a/drivers/virtio-device.hh
+++ b/drivers/virtio-device.hh
@@ -65,11 +65,7 @@ public:
virtual void kick_queue(int queue) = 0;
virtual u64 get_available_features() = 0;
- virtual bool get_available_feature_bit(int bit) = 0;
-
virtual void set_enabled_features(u64 features) = 0;
- virtual u64 get_enabled_features() = 0;
- virtual bool get_enabled_feature_bit(int bit) = 0;
// From the spec:
// "The device status field provides a simple low-level indication of
diff --git a/drivers/virtio-net.cc b/drivers/virtio-net.cc
--- a/drivers/virtio-net.cc
+++ b/drivers/virtio-net.cc
@@ -228,7 +228,7 @@ bool net::ack_irq()
void net::init()
{
- // Steps 4 & 5 - negotiate and confirm features
+ // Steps 4, 5 & 6 - negotiate and confirm features
setup_features();
read_config();
@@ -250,8 +250,8 @@ net::net(virtio_device& dev)
poll_task->set_priority(sched::thread::priority_infinity);
+ // Please look at the section 5.1.6.1 of virtio specification for
explanation
if (_dev.is_modern()) {
- //TODO: Legacy vs non-legacy -> the non-legacy header includes one
more field
_hdr_size = sizeof(net_hdr_mrg_rxbuf);
}
else {
@@ -316,6 +316,13 @@ net::net(virtio_device& dev)
[=] { return this->ack_irq(); },
[=] { poll_task->wake(); });
};
+
+ int_factory.create_gsi_edge_interrupt = [this,poll_task]() {
+ return new gsi_edge_interrupt(
+ _dev.get_irq(),
+ [=] { if (this->ack_irq()) poll_task->wake(); });
+ };
+
_dev.register_interrupt(int_factory);
fill_rx_ring();
@@ -341,17 +348,7 @@ net::~net()
void net::read_config()
{
- if (_dev.is_modern()) {
- //TODO: It may to do with legacy vs non-legacy device
- //but at least with latest spec we should check if individual
- //config fields are available vs reading whole config struct. For
example
- //firecracker reports memory read violation warnings
- virtio_conf_read(0, &(_config.mac[0]), sizeof(_config.mac));
- }
- else {
- //read all of the net config in one shot
- virtio_conf_read(0, &_config, sizeof(_config));
- }
+ virtio_conf_read(0, &(_config.mac[0]), sizeof(_config.mac));
if (get_guest_feature_bit(VIRTIO_NET_F_MAC))
net_i("The mac addr of the device is %x:%x:%x:%x:%x:%x",
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
@@ -97,26 +97,11 @@ u64 virtio_legacy_pci_device::get_available_features()
return virtio_conf_readl(VIRTIO_PCI_HOST_FEATURES);
}
-bool virtio_legacy_pci_device::get_available_feature_bit(int bit)
-{
- return get_virtio_config_bit(VIRTIO_PCI_HOST_FEATURES, bit);
-}
-
void virtio_legacy_pci_device::set_enabled_features(u64 features)
{
virtio_conf_writel(VIRTIO_PCI_GUEST_FEATURES, (u32)features);
}
-u64 virtio_legacy_pci_device::get_enabled_features()
-{
- return virtio_conf_readl(VIRTIO_PCI_GUEST_FEATURES);
-}
-
-bool virtio_legacy_pci_device::get_enabled_feature_bit(int bit)
-{
- return get_virtio_config_bit(VIRTIO_PCI_GUEST_FEATURES, bit);
-}
-
u8 virtio_legacy_pci_device::get_status()
{
return virtio_conf_readb(VIRTIO_PCI_STATUS);
@@ -241,11 +226,6 @@ u64 virtio_modern_pci_device::get_available_features()
return features;
}
-bool virtio_modern_pci_device::get_available_feature_bit(int bit)
-{
- return 0 != (get_available_features() & (1 << bit));
-}
-
void virtio_modern_pci_device::set_enabled_features(u64 features)
{
_common_cfg->virtio_conf_writel(COMMON_CFG_OFFSET_OF(driver_feature_select),
0);
@@ -254,23 +234,6 @@ void
virtio_modern_pci_device::set_enabled_features(u64 features)
_common_cfg->virtio_conf_writel(COMMON_CFG_OFFSET_OF(driver_feature),
features >> 32);
}
-u64 virtio_modern_pci_device::get_enabled_features()
-{
- u64 features;
-
-
_common_cfg->virtio_conf_writel(COMMON_CFG_OFFSET_OF(driver_feature_select),
0);
- features =
_common_cfg->virtio_conf_readl(COMMON_CFG_OFFSET_OF(driver_feature));
-
_common_cfg->virtio_conf_writel(COMMON_CFG_OFFSET_OF(driver_feature_select),
1);
- features |=
((u64)_common_cfg->virtio_conf_readl(COMMON_CFG_OFFSET_OF(driver_feature))
<< 32);
-
- return features;
-}
-
-bool virtio_modern_pci_device::get_enabled_feature_bit(int bit)
-{
- return 0 != (get_enabled_features() & (1 << bit));
-}
-
u8 virtio_modern_pci_device::get_status()
{
return
_common_cfg->virtio_conf_readb(COMMON_CFG_OFFSET_OF(device_status));
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
@@ -107,11 +107,7 @@ public:
virtual void kick_queue(int queue);
virtual u64 get_available_features();
- virtual bool get_available_feature_bit(int bit);
-
virtual void set_enabled_features(u64 features);
- virtual u64 get_enabled_features();
- virtual bool get_enabled_feature_bit(int bit);
virtual u8 get_status();
virtual void set_status(u8 status);
@@ -267,11 +263,7 @@ public:
virtual void kick_queue(int queue);
virtual u64 get_available_features();
- virtual bool get_available_feature_bit(int bit);
-
virtual void set_enabled_features(u64 features);
- virtual u64 get_enabled_features();
- virtual bool get_enabled_feature_bit(int bit);
virtual u8 get_status();
virtual void set_status(u8 status);
diff --git a/drivers/virtio-rng.cc b/drivers/virtio-rng.cc
--- a/drivers/virtio-rng.cc
+++ b/drivers/virtio-rng.cc
@@ -40,7 +40,7 @@ rng::rng(virtio_device& dev)
: virtio_driver(dev)
, _thread(sched::thread::make([&] { worker(); },
sched::thread::attr().name("virtio-rng")))
{
- // Steps 4 & 5 - negotiate and confirm features
+ // Steps 4, 5 & 6 - negotiate and confirm features
setup_features();
// Step 7 - generic init of virtqueues
diff --git a/drivers/virtio-scsi.cc b/drivers/virtio-scsi.cc
--- a/drivers/virtio-scsi.cc
+++ b/drivers/virtio-scsi.cc
@@ -147,7 +147,7 @@ scsi::scsi(virtio_device& dev)
_driver_name = "virtio-scsi";
_id = _instance++;
- // Steps 4 & 5 - negotiate and confirm features
+ // Steps 4, 5 & 6 - negotiate and confirm features
setup_features();
read_config();
diff --git a/drivers/virtio.cc b/drivers/virtio.cc
--- a/drivers/virtio.cc
+++ b/drivers/virtio.cc
@@ -63,22 +63,32 @@ void virtio_driver::setup_features()
set_indirect_buf_cap(true);
if (subset & (1 << VIRTIO_RING_F_EVENT_IDX))
- set_event_idx_cap(true);
+ set_event_idx_cap(true);
set_guest_features(subset);
- // Step 5 - confirm features (only applies to modern devices)
- if (_dev.is_modern())
+ if (_dev.is_modern()) {
+ //
+ // Step 5 - confirm features (only applies to modern devices)
add_dev_status(VIRTIO_CONFIG_S_FEATURES_OK);
+ //
+ // Step 6 - re-read device status to ensure the FEATURES_OK bit is
still set
+ assert(get_dev_status() & VIRTIO_CONFIG_S_FEATURES_OK);
+ }
}
void virtio_driver::dump_config()
{
_dev.dump_config();
+#if CONF_logger_debug
+ auto device_features = get_device_features();
virtio_d(" virtio features: ");
- for (int i = 0; i < 64; i++)
- virtio_d(" %d ", get_device_feature_bit(i));
+
+ for (int i = 0; i < 64; i++) {
+ virtio_d(" %d ", 0 != (device_features & (1 << i)));
+ }
+#endif
}
void virtio_driver::reset_device()
@@ -172,19 +182,17 @@ u64 virtio_driver::get_device_features()
return _dev.get_available_features();
}
-bool virtio_driver::get_device_feature_bit(int bit)
-{
- return _dev.get_available_feature_bit(bit);
-}
-
void virtio_driver::set_guest_features(u64 features)
{
+ // Write negotiated features to the device
_dev.set_enabled_features(features);
+ // Save negotiated features
+ _enabled_features = features;
}
bool virtio_driver::get_guest_feature_bit(int bit)
{
- return _dev.get_enabled_feature_bit(bit);
+ return (_enabled_features & (1 << bit)) != 0;
}
u8 virtio_driver::get_dev_status()
diff --git a/drivers/virtio.hh b/drivers/virtio.hh
--- a/drivers/virtio.hh
+++ b/drivers/virtio.hh
@@ -80,7 +80,6 @@ public:
// guest/host features physical access
u64 get_device_features();
- bool get_device_feature_bit(int bit);
void set_guest_features(u64 features);
bool get_guest_feature_bit(int bit);
@@ -114,6 +113,7 @@ protected:
bool _cap_indirect_buf;
bool _cap_event_idx = false;
static int _disk_idx;
+ u64 _enabled_features;
};
template <typename T, u16 ID>
--
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].
For more options, visit https://groups.google.com/d/optout.
