Re: [PATCH 2/6] virtio: introduce struct virtio_pci_ops

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Tue, Dec 29, 2015 at 11:31:35AM +0000, Tan, Jianfeng wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yuanhan Liu
> > Sent: Thursday, December 10, 2015 11:54 AM
> > To: dev@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().

I don't know, those are old code; I just moved them into a new
function. Maybe I need look into the spec for answer.
> 
> 
> > +}
> > +
> > +static uint8_t
> > +legacy_get_isr(struct virtio_hw *hw)
> > +{
> 
> Please delete the following blank line.

oops ...

> > +
> > +	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.

I agree with you that queue_size is better here, however, queue_num is
taken for being consistent with the virtio spec naming:

    static uint16_t
    legacy_get_queue_num(struct virtio_hw *hw, uint16_t queue_id)
    {
            VIRTIO_WRITE_REG_2(hw, VIRTIO_PCI_QUEUE_SEL, queue_id);
            return VIRTIO_READ_REG_2(hw, VIRTIO_PCI_QUEUE_NUM);
    }


	--yliu