[dpdk-dev] [PATCH 2/6] virtio: introduce struct virtio_pci_ops
Yuanhan Liu
yuanhan.liu at linux.intel.com
Wed Dec 30 04:45:25 CET 2015
On Tue, Dec 29, 2015 at 11:31:35AM +0000, Tan, Jianfeng wrote:
>
>
> > -----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().
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
More information about the dev
mailing list