[dpdk-dev] [PATCH 2/6] virtio: introduce struct virtio_pci_ops

Tan, Jianfeng jianfeng.tan at intel.com
Tue Dec 29 12:31:35 CET 2015



> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Yuanhan Liu
> Sent: Thursday, December 10, 2015 11:54 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH 2/6] virtio: introduce struct virtio_pci_ops
> 
> Introduce struct virtio_pci_ops, to let legacy virtio (v0.95) and
> modern virtio (1.0) have different implementation regarding to a
> specific pci action, such as read host status.
> 
...
> +static void
> +legacy_reset(struct virtio_hw *hw)
> +{
> +	/*
> +	 * Setting the status to RESET sets the host device to
> +	 * the original, uninitialized state.
> +	 */
> +	legacy_set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> +	legacy_get_status(hw);


May need a comment to explain why here we need to get_status().


> +}
> +
> +static uint8_t
> +legacy_get_isr(struct virtio_hw *hw)
> +{

Please delete the following blank line.

> +
> +	return VIRTIO_READ_REG_1(hw, VIRTIO_PCI_ISR);
> +}
> +
> +/* Enable one vector (0) for Link State Intrerrupt */
> +static uint16_t
> +legacy_set_irq(struct virtio_hw *hw, uint16_t vec)
> +{
> +	VIRTIO_WRITE_REG_2(hw, VIRTIO_MSI_CONFIG_VECTOR, vec);
> +	return VIRTIO_READ_REG_2(hw, VIRTIO_MSI_CONFIG_VECTOR);
> +}
> +
...
> +
> +struct virtio_pci_ops {
> +	void (*read_dev_cfg)(struct virtio_hw *hw, uint64_t offset,
> +			     void *dst, int len);
> +	void (*write_dev_cfg)(struct virtio_hw *hw, uint64_t offset,
> +			      void *src, int len);
> +	void (*reset)(struct virtio_hw *hw);
> +
> +	uint8_t (*get_status)(struct virtio_hw *hw);
> +	void    (*set_status)(struct virtio_hw *hw, uint8_t status);
> +
> +	uint32_t (*get_features)(struct virtio_hw *hw);
> +	void     (*set_features)(struct virtio_hw *hw, uint32_t features);
> +
> +	uint8_t (*get_isr)(struct virtio_hw *hw);
> +
> +	uint16_t (*set_irq)(struct virtio_hw *hw, uint16_t vec);
> +
> +	uint16_t (*get_queue_num)(struct virtio_hw *hw, uint16_t
> queue_id);

How about changing the name into get_queue_size? From my side, queue_num is very confusing.

Thanks,
Jianfeng

> +	void (*setup_queue)(struct virtio_hw *hw, struct virtqueue *vq);
> +	void (*del_queue)(struct virtio_hw *hw, struct virtqueue *vq);
> +	void (*notify_queue)(struct virtio_hw *hw, struct virtqueue *vq);
> +};
> +
...
>  #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
> --
> 1.9.0



More information about the dev mailing list