Skip to content

Commit

Permalink
virtio: fix feature set logic
Browse files Browse the repository at this point in the history 
Fix feature select and feature set logic to use previously selected
feature select to be in sync.

Also add a device type specific feature validate cb that will validate
and dump features in debug mode in more readable format.

Change-Id: I2685a01176d71d02e5d941fd061a224cf43e5d1b
Signed-off-by: Nithin Dabilpuram <[email protected]>
Reviewed-on: https://sj1git1.cavium.com/c/IP/SW/dataplane/dpu-offload/+/144184
Tested-by: sa_ip-toolkits-Jenkins <[email protected]>
Reviewed-by: Srujana Challa <[email protected]>
Reviewed-by: Jerin Jacob <[email protected]>
  • Loading branch information
@nithind1988 @jerinjacobk
nithind1988 authored and jerinjacobk committed Feb 4, 2025
1 parent b011e75 commit 4f3712e
Show file tree
Hide file tree
Showing 3 changed files with 163 additions and 16 deletions.
57 changes: 41 additions & 16 deletions lib/virtio/virtio_dev.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
#define VIRTIO_QUEUE_SELECT_DELAY 3
#define VIRTIO_DEVICE_STATUS_DELAY 5

#define VIRTIO_INVALID_QUEUE_INDEX 0xFFFF
#define VIRTIO_INVALID_QUEUE_INDEX 0xFFFF
#define VIRTIO_INVALID_FEATURE_SELECT 0xFFFF

struct virtio_ctrl_queue {
uintptr_t desc_base;
Expand All @@ -40,6 +41,12 @@ virtio_process_device_feature_select(struct virtio_dev *dev, uintptr_t shadow,
volatile struct virtio_pci_common_cfg *common_cfg = dev->common_cfg;
uint32_t feature_select = device_feature_select & 0x7fff;

/* Skip if it is during init */
if (device_feature_select == VIRTIO_INVALID_FEATURE_SELECT) {
shadow_cfg->device_feature_select = device_feature_select;
return 0;
}

if (feature_select == 0)
common_cfg->device_feature = (uint32_t)dev->dev_feature_bits & BIT_MASK32;
else if (feature_select == 1)
Expand Down Expand Up @@ -75,6 +82,11 @@ virtio_process_driver_feature_select(struct virtio_dev *dev, uintptr_t shadow,
uint32_t prev_feature_select = dev->prev_drv_feature_select;
uint32_t feature_select = driver_feature_select & 0x7fff;

if (driver_feature_select == VIRTIO_INVALID_FEATURE_SELECT) {
shadow_cfg->driver_feature_select = driver_feature_select;
return 0;
}

if (prev_feature_select != 0xffff) {
if (prev_feature_select == 0)
dev->drv_feature_bits_lo = dev->common_cfg->driver_feature;
Expand All @@ -85,6 +97,7 @@ virtio_process_driver_feature_select(struct virtio_dev *dev, uintptr_t shadow,
dev->common_cfg->driver_feature);
}

dao_dbg("[dev %u] feature selected: %u", dev->dev_id, feature_select);
/* Store feature select as driver can proceed to write driver feature and again
* change driver feature select before device processing device_feature of
* previous one.
Expand All @@ -95,9 +108,10 @@ virtio_process_driver_feature_select(struct virtio_dev *dev, uintptr_t shadow,
else if (feature_select == 1)
dev->common_cfg->driver_feature = dev->drv_feature_bits_hi;

shadow_cfg->driver_feature = dev->common_cfg->driver_feature;
rte_wmb();

shadow_cfg->driver_feature_select = feature_select;
shadow_cfg->driver_feature = dev->common_cfg->driver_feature;
dev->common_cfg->driver_feature_select = feature_select;
rte_wmb();

Expand All @@ -110,7 +124,7 @@ static int
virtio_process_driver_feature(struct virtio_dev *dev, uintptr_t shadow, uint32_t driver_feature)
{
struct virtio_pci_common_cfg *shadow_cfg = (struct virtio_pci_common_cfg *)shadow;
uint32_t feature_select = dev->common_cfg->driver_feature_select;
uint32_t feature_select = dev->prev_drv_feature_select;

if (feature_select == 0)
dev->drv_feature_bits_lo = driver_feature;
Expand Down Expand Up @@ -146,8 +160,8 @@ virtio_config_populate(struct virtio_dev *dev)

/* Populate common config and init notify area with initial wrap count */
common_cfg->num_queues = max_virtio_queues;
common_cfg->device_feature_select = -1;
common_cfg->driver_feature_select = -1;
common_cfg->device_feature_select = VIRTIO_INVALID_FEATURE_SELECT;
common_cfg->driver_feature_select = VIRTIO_INVALID_FEATURE_SELECT;

/* Reset notification area */
for (i = 0; i < max_virtio_queues; i++) {
Expand All @@ -156,7 +170,7 @@ virtio_config_populate(struct virtio_dev *dev)
}

dev->prev_queue_select = VIRTIO_INVALID_QUEUE_INDEX;
dev->prev_drv_feature_select = 0xffff;
dev->prev_drv_feature_select = VIRTIO_INVALID_FEATURE_SELECT;

/* Initialize queue config defaults */
memset(dev->queue_conf, 0, max_virtio_queues * sizeof(struct virtio_queue_conf));
Expand Down Expand Up @@ -389,9 +403,11 @@ virtio_setup_cq_info(struct virtio_dev *dev)
static int
virtio_process_device_status(struct virtio_dev *dev, uintptr_t shadow, uint8_t device_status)
{
virtio_feature_validate_cb_t feature_validate_cb = dev_cbs[dev->dev_type].feature_validate;
struct virtio_pci_common_cfg *shadow_cfg = (struct virtio_pci_common_cfg *)shadow;
virtio_dev_status_cb_t dev_status_cb = dev_cbs[dev->dev_type].dev_status;
uint8_t status = device_status & 0x7f;
uint64_t feature_bits;

dao_dbg("[dev %u] device_status: 0x%x", dev->dev_id, device_status);

Expand All @@ -414,22 +430,31 @@ virtio_process_device_status(struct virtio_dev *dev, uintptr_t shadow, uint8_t d
}

if (status & VIRTIO_DEV_FEATURES_OK && !dev->features_ok) {
feature_bits = dev->drv_feature_bits_lo | (uint64_t)dev->drv_feature_bits_hi << 32;
dao_info("[dev %u] %s", dev->dev_id,
dao_virtio_dev_status_to_str(VIRTIO_DEV_FEATURES_OK));

dev->feature_bits = dev->drv_feature_bits_lo | (uint64_t)dev->drv_feature_bits_hi
<< 32;
dao_info("[dev %u] Feature bits negotiated : %lx", dev->dev_id, feature_bits);
if (feature_validate_cb && feature_validate_cb(dev, feature_bits)) {
/* Feature validation failed */
dao_err("[dev %u] Feature validation failed", dev->dev_id);
dev->common_cfg->device_status &= ~VIRTIO_DEV_FEATURES_OK;
shadow_cfg->device_status &= ~VIRTIO_DEV_FEATURES_OK;
goto next;
}

dev->feature_bits = feature_bits;
dev->features_ok = 1;
shadow_cfg->device_status |= VIRTIO_DEV_FEATURES_OK;
dao_info("[dev %u] Feature bits negotiated : %lx", dev->dev_id, dev->feature_bits);

if ((dev->feature_bits & RTE_BIT64(VIRTIO_F_ORDER_PLATFORM)) == 0) {
dao_warn("[dev %u] !!! VIRTIO_F_ORDER_PLATFORM not negotiated !!!",
dev->dev_id);
dao_warn("[dev %u] !!! Can lead to out-of-sync descriptor data !!!",
dev->dev_id);
}
}

next:
if (status & VIRTIO_DEV_DRIVER_OK && !dev->driver_ok) {
/* Return to go through other changes and come back as we might not seeing
* writes in order.
Expand Down Expand Up @@ -798,25 +823,25 @@ dao_virtio_common_cfg_cb(void *ctx, uintptr_t shadow, uint32_t offset, uint64_t
up_cfg.w0 = val;
shd_cfg.w0 = shadow_val;
update_shadow = false;
if (up_cfg.device_feature != shd_cfg.device_feature)
virtio_process_device_feature(dev, up_cfg.device_feature);

if (up_cfg.device_feature_select != shd_cfg.device_feature_select)
virtio_process_device_feature_select(dev, shadow,
up_cfg.device_feature_select);

if (up_cfg.device_feature != shd_cfg.device_feature)
virtio_process_device_feature(dev, up_cfg.device_feature);
break;

case 1:
/* driver_feature_select, driver_feature */
up_cfg.w1 = val;
shd_cfg.w1 = shadow_val;
update_shadow = false;
if (up_cfg.driver_feature != shd_cfg.driver_feature)
virtio_process_driver_feature(dev, shadow, up_cfg.driver_feature);

if (up_cfg.driver_feature_select != shd_cfg.driver_feature_select)
virtio_process_driver_feature_select(dev, shadow,
up_cfg.driver_feature_select);

if (up_cfg.driver_feature != shd_cfg.driver_feature)
virtio_process_driver_feature(dev, shadow, up_cfg.driver_feature);
break;

case 2:
Expand Down
2 changes: 2 additions & 0 deletions lib/virtio/virtio_dev_priv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@ typedef void (*virtio_cq_cmd_process_cb_t)(struct virtio_dev *dev, struct rte_dm
typedef int (*virtio_dev_status_cb_t)(struct virtio_dev *dev, uint8_t status);
typedef uint16_t (*virtio_cq_id_get_cb_t)(struct virtio_dev *dev, uint64_t feature_bits);
typedef int (*virtio_queue_enable_cb_t)(struct virtio_dev *dev, uint16_t queue_id);
typedef int (*virtio_feature_validate_cb_t)(struct virtio_dev *dev, uint64_t feature_bits);

struct virtio_dev_cbs {
virtio_cq_cmd_process_cb_t cq_cmd_process;
virtio_dev_status_cb_t dev_status;
virtio_cq_id_get_cb_t cq_id_get;
virtio_queue_enable_cb_t queue_enable;
virtio_feature_validate_cb_t feature_validate;
};

struct virtio_pci_cap {
Expand Down
120 changes: 120 additions & 0 deletions lib/virtio_net/virtio_netdev.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,125 @@ dao_net_desc_manage_fn_t dao_net_desc_manage_fns[VIRTIO_NET_DESC_MANAGE_LAST <<
struct dao_virtio_netdev_cbs user_cbs;
int virtio_netdev_clear_queue_info(struct virtio_netdev *netdev);

static int
virtio_netdev_feature_validate(struct virtio_dev *dev, uint64_t feature_bits)
{
uint16_t dev_id = dev->dev_id;
int i;

if ((feature_bits | dev->dev_feature_bits) != dev->dev_feature_bits) {
dao_err("[dev %u] Invalid feature bits negotiated 0x%" PRIx64 "(dev %" PRIx64 ")",
dev_id, feature_bits, dev->dev_feature_bits);
return -EINVAL;
}

/* Dump features enabled for debug purpose */
dao_dbg("[dev %u] Features enabled:", dev_id);
for (i = 0; i < 64; i++) {
if (!(feature_bits & DAO_BIT(i)))
continue;
switch (i) {
case VIRTIO_NET_F_CSUM:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_CSUM");
break;
case VIRTIO_NET_F_GUEST_CSUM:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_GUEST_CSUM");
break;
case VIRTIO_NET_F_MTU:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_MTU");
break;
case VIRTIO_NET_F_MAC:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_MAC");
break;
case VIRTIO_NET_F_GUEST_TSO4:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_GUEST_TSO4");
break;
case VIRTIO_NET_F_GUEST_TSO6:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_GUEST_TSO6");
break;
case VIRTIO_NET_F_GUEST_ECN:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_GUEST_ECN");
break;
case VIRTIO_NET_F_GUEST_UFO:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_GUEST_UFO");
break;
case VIRTIO_NET_F_HOST_TSO4:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_HOST_TSO4");
break;
case VIRTIO_NET_F_HOST_TSO6:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_HOST_TSO6");
break;
case VIRTIO_NET_F_HOST_ECN:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_HOST_ECN");
break;
case VIRTIO_NET_F_HOST_UFO:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_HOST_UFO");
break;
case VIRTIO_NET_F_MRG_RXBUF:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_MRG_RXBUF");
break;
case VIRTIO_NET_F_STATUS:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_STATUS");
break;
case VIRTIO_NET_F_CTRL_VQ:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_CTRL_VQ");
break;
case VIRTIO_NET_F_CTRL_RX:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_CTRL_RX");
break;
case VIRTIO_NET_F_CTRL_VLAN:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_CTRL_VLAN");
break;
case VIRTIO_NET_F_CTRL_RX_EXTRA:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_CTRL_RX_EXTRA");
break;
case VIRTIO_NET_F_GUEST_ANNOUNCE:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_GUEST_ANNOUNCE");
break;
case VIRTIO_NET_F_MQ:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_MQ");
break;
case VIRTIO_NET_F_CTRL_MAC_ADDR:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_CTRL_MAC_ADDR");
break;
case VIRTIO_NET_F_HASH_REPORT:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_HASH_REPORT");
break;
case VIRTIO_NET_F_GUEST_HDRLEN:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_GUEST_HDRLEN");
break;
case VIRTIO_NET_F_RSS:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_RSS");
break;
case VIRTIO_NET_F_SPEED_DUPLEX:
dao_dbg("[dev %u] +%s", dev_id, "VIRTIO_NET_F_SPEED_DUPLEX");
break;
case VIRTIO_F_VERSION_1:
dao_dbg("[dev %u] +%s", dev->dev_id, "VIRTIO_F_VERSION_1");
break;
case VIRTIO_F_IOMMU_PLATFORM:
dao_dbg("[dev %u] +%s", dev->dev_id, "VIRTIO_F_IOMMU_PLATFORM");
break;
case VIRTIO_F_RING_PACKED:
dao_dbg("[dev %u] +%s", dev->dev_id, "VIRTIO_F_RING_PACKED");
break;
case VIRTIO_F_IN_ORDER:
dao_dbg("[dev %u] +%s", dev->dev_id, "VIRTIO_F_IN_ORDER");
break;
case VIRTIO_F_ORDER_PLATFORM:
dao_dbg("[dev %u] +%s", dev->dev_id, "VIRTIO_F_ORDER_PLATFORM");
break;
case VIRTIO_F_NOTIFICATION_DATA:
dao_dbg("[dev %u] +%s", dev->dev_id, "VIRTIO_F_NOTIFICATION_DATA");
break;
default:
dao_err("[dev %u] Unknown feature bit %d", dev_id, i);
break;
};
}
return 0;
}

static void
virtio_hash_types_to_hash_report(struct virtio_netdev *netdev, uint64_t virtio_hash_types)
{
Expand Down Expand Up @@ -762,6 +881,7 @@ dao_virtio_netdev_init(uint16_t devid, struct dao_virtio_netdev_conf *conf)
dev_cbs[VIRTIO_DEV_TYPE_NET].cq_cmd_process = virtio_netdev_cq_cmd_process;
dev_cbs[VIRTIO_DEV_TYPE_NET].cq_id_get = virtio_netdev_cq_id_get;
dev_cbs[VIRTIO_DEV_TYPE_NET].queue_enable = virtio_netdev_queue_enable;
dev_cbs[VIRTIO_DEV_TYPE_NET].feature_validate = virtio_netdev_feature_validate;
return 0;
}

Expand Down

0 comments on commit 4f3712e

Please sign in to comment.