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]> --- drivers/virtio-blk.cc | 46 ++++++++++++++++++++---------------- drivers/virtio-blk.hh | 20 +++++++++------- drivers/virtio-device.hh | 4 ---- drivers/virtio-net.cc | 23 ++++++++---------- drivers/virtio-pci-device.cc | 37 ----------------------------- drivers/virtio-pci-device.hh | 8 ------- drivers/virtio-rng.cc | 2 +- drivers/virtio-scsi.cc | 2 +- drivers/virtio.cc | 30 ++++++++++++++--------- drivers/virtio.hh | 2 +- 10 files changed, 69 insertions(+), 105 deletions(-) diff --git a/drivers/virtio-blk.cc b/drivers/virtio-blk.cc index 0bc6fb0f..4fa4bbf9 100644 --- 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 index b707b5bf..00e679d3 100644 --- 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 index e9846fb3..1327a159 100644 --- 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 index ef2dff66..e4a5db51 100644 --- 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 index dc444757..91367d1c 100644 --- 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 index e9554558..5a891d93 100644 --- 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 index 48331ebd..c276af04 100644 --- 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 index ff92eef1..4fd89c24 100644 --- 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 index 5ef0f94d..18a5d054 100644 --- 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 index ef915e1c..0cdc6ec5 100644 --- 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> -- 2.19.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]. For more options, visit https://groups.google.com/d/optout.
