[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
Sigh... I have just send out v3 ...
On Thu, Jan 14, 2016 at 07:50:00AM +, Xie, Huawei wrote:
> On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
> > +static inline void
> > +modern_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi)
> > +{
> > + modern_write32((uint32_t)val, lo);
> > + modern_write32(val >> 32, hi);
> > +}
> > +
>
> This is normal mmio read/write operation. ioread8/16/32/64 or just
> readxx is more meaningful name here.
I just want to make them looks like modern device related, which they
are.
> > +static void
> [SNIP]
> > +
> > +static void
> > +modern_write_dev_config(struct virtio_hw *hw, uint64_t offset,
> > + void *src, int length)
>
> define src as const
okay.
>
> [snip]
> >
> > +static inline void *
> > +get_cfg_addr(struct rte_pci_device *dev, struct virtio_pci_cap *cap)
>
> No explicit inline for non performance critical functions.
okay.
>
> > +{
> > + uint8_t bar= cap->bar;
> > + uint32_t length = cap->length;
> > + uint32_t offset = cap->offset;
> > + uint8_t *base;
> > +
> > + if (unlikely(bar > 5)) {
> Don't use constant value number whenever possible
I normally will not bother to define a macro for used once number,
espeically for some well known ones. Say, I won't define
#define UINT8_MAX_VALUE 0xff
>
> No likely/unlikely for non performance critical functions
makes sense.
> > + if (rte_eal_pci_map_device(dev) < 0) {
> > + PMD_INIT_LOG(DEBUG, "failed to map pci device!");
>
> s /DEBUG/ERR/
It's not an error; it's expected, say, when no UIO is bond.
--yliu
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On Thu, Jan 14, 2016 at 08:08:28AM +, Xie, Huawei wrote:
> On 1/14/2016 3:58 PM, Yuanhan Liu wrote:
> > On Thu, Jan 14, 2016 at 07:51:08AM +, Xie, Huawei wrote:
> >> On 1/14/2016 3:49 PM, Yuanhan Liu wrote:
> >>> On Thu, Jan 14, 2016 at 07:47:17AM +, Xie, Huawei wrote:
> On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
> > Modern (v1.0) virtio pci device defines several pci capabilities.
> [snip]
> > +static void
> > +modern_notify_queue(struct virtio_hw *hw __rte_unused, struct
> > virtqueue *vq)
> > +{
> > + modern_write16(1, vq->notify_addr);
> > +}
> Does virtio 1.0 only supports MMIO? MMIO has long VMEXIT latency than
> PORT IO.
> >>> Virtio 1.0 supports three transport layer, including MMIO and PCI. And
> >>> we use PCI only in our pmd driver.
> >> I don't mean that MMIO but use memory mapped IO for configuration.
> > Then, yes.
>
> 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> Subsystem: Red Hat, Inc Device 0001
> Physical Slot: 3
> Flags: bus master, fast devsel, latency 0, IRQ 10
> I/O ports at c100 [size=32]
> Memory at febd1000 (32-bit, non-prefetchable) [size=4K]
> Memory at fe00 (64-bit, prefetchable) [size=8M]
> Expansion ROM at feb4 [disabled] [size=256K]
> Capabilities: [98] MSI-X: Enable+ Count=3 Masked-
> Capabilities: [84] Vendor Specific Information: Len=14
> Capabilities: [70] Vendor Specific Information: Len=14
> Capabilities: [60] Vendor Specific Information: Len=10
> Capabilities: [50] Vendor Specific Information: Len=10
> Capabilities: [40] Vendor Specific Information: Len=10
> Kernel driver in use: igb_uio
> Kernel modules: virtio_pci
>
> c100 is still there.
Yes,
> For the notification, try PORT IO if possible.
But it doesn't seem right to me to mix legacy registers in modern pci device.
--yliu
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On Thu, Jan 14, 2016 at 07:51:08AM +, Xie, Huawei wrote:
> On 1/14/2016 3:49 PM, Yuanhan Liu wrote:
> > On Thu, Jan 14, 2016 at 07:47:17AM +, Xie, Huawei wrote:
> >> On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
> >>> Modern (v1.0) virtio pci device defines several pci capabilities.
> >> [snip]
> >>> +static void
> >>> +modern_notify_queue(struct virtio_hw *hw __rte_unused, struct virtqueue
> >>> *vq)
> >>> +{
> >>> + modern_write16(1, vq->notify_addr);
> >>> +}
> >> Does virtio 1.0 only supports MMIO? MMIO has long VMEXIT latency than
> >> PORT IO.
> > Virtio 1.0 supports three transport layer, including MMIO and PCI. And
> > we use PCI only in our pmd driver.
>
> I don't mean that MMIO but use memory mapped IO for configuration.
Then, yes.
--yliu
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On Thu, Jan 14, 2016 at 07:47:17AM +, Xie, Huawei wrote:
> On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
> > Modern (v1.0) virtio pci device defines several pci capabilities.
> [snip]
> > +static void
> > +modern_notify_queue(struct virtio_hw *hw __rte_unused, struct virtqueue
> > *vq)
> > +{
> > + modern_write16(1, vq->notify_addr);
> > +}
>
> Does virtio 1.0 only supports MMIO? MMIO has long VMEXIT latency than
> PORT IO.
Virtio 1.0 supports three transport layer, including MMIO and PCI. And
we use PCI only in our pmd driver.
--yliu
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On 1/14/2016 4:21 PM, Yuanhan Liu wrote:
> On Thu, Jan 14, 2016 at 08:08:28AM +, Xie, Huawei wrote:
>> On 1/14/2016 3:58 PM, Yuanhan Liu wrote:
>>> On Thu, Jan 14, 2016 at 07:51:08AM +, Xie, Huawei wrote:
On 1/14/2016 3:49 PM, Yuanhan Liu wrote:
> On Thu, Jan 14, 2016 at 07:47:17AM +, Xie, Huawei wrote:
>> On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
>>> Modern (v1.0) virtio pci device defines several pci capabilities.
>> [snip]
>>> +static void
>>> +modern_notify_queue(struct virtio_hw *hw __rte_unused, struct
>>> virtqueue *vq)
>>> +{
>>> + modern_write16(1, vq->notify_addr);
>>> +}
>> Does virtio 1.0 only supports MMIO? MMIO has long VMEXIT latency than
>> PORT IO.
> Virtio 1.0 supports three transport layer, including MMIO and PCI. And
> we use PCI only in our pmd driver.
I don't mean that MMIO but use memory mapped IO for configuration.
>>> Then, yes.
>> 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
>> Subsystem: Red Hat, Inc Device 0001
>> Physical Slot: 3
>> Flags: bus master, fast devsel, latency 0, IRQ 10
>> I/O ports at c100 [size=32]
>> Memory at febd1000 (32-bit, non-prefetchable) [size=4K]
>> Memory at fe00 (64-bit, prefetchable) [size=8M]
>> Expansion ROM at feb4 [disabled] [size=256K]
>> Capabilities: [98] MSI-X: Enable+ Count=3 Masked-
>> Capabilities: [84] Vendor Specific Information: Len=14
>> Capabilities: [70] Vendor Specific Information: Len=14
>> Capabilities: [60] Vendor Specific Information: Len=10
>> Capabilities: [50] Vendor Specific Information: Len=10
>> Capabilities: [40] Vendor Specific Information: Len=10
>> Kernel driver in use: igb_uio
>> Kernel modules: virtio_pci
>>
>> c100 is still there.
> Yes,
>
>> For the notification, try PORT IO if possible.
> But it doesn't seem right to me to mix legacy registers in modern pci device.
On TLB and cache miss, this could cause plenty of cycles. Considering
that our current focus is dpdkvhost which doesn't need notification, let
us revisit this later.
>
> --yliu
>
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On 1/14/2016 3:58 PM, Yuanhan Liu wrote:
> On Thu, Jan 14, 2016 at 07:51:08AM +, Xie, Huawei wrote:
>> On 1/14/2016 3:49 PM, Yuanhan Liu wrote:
>>> On Thu, Jan 14, 2016 at 07:47:17AM +, Xie, Huawei wrote:
On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
> Modern (v1.0) virtio pci device defines several pci capabilities.
[snip]
> +static void
> +modern_notify_queue(struct virtio_hw *hw __rte_unused, struct virtqueue
> *vq)
> +{
> + modern_write16(1, vq->notify_addr);
> +}
Does virtio 1.0 only supports MMIO? MMIO has long VMEXIT latency than
PORT IO.
>>> Virtio 1.0 supports three transport layer, including MMIO and PCI. And
>>> we use PCI only in our pmd driver.
>> I don't mean that MMIO but use memory mapped IO for configuration.
> Then, yes.
00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
Subsystem: Red Hat, Inc Device 0001
Physical Slot: 3
Flags: bus master, fast devsel, latency 0, IRQ 10
I/O ports at c100 [size=32]
Memory at febd1000 (32-bit, non-prefetchable) [size=4K]
Memory at fe00 (64-bit, prefetchable) [size=8M]
Expansion ROM at feb4 [disabled] [size=256K]
Capabilities: [98] MSI-X: Enable+ Count=3 Masked-
Capabilities: [84] Vendor Specific Information: Len=14
Capabilities: [70] Vendor Specific Information: Len=14
Capabilities: [60] Vendor Specific Information: Len=10
Capabilities: [50] Vendor Specific Information: Len=10
Capabilities: [40] Vendor Specific Information: Len=10
Kernel driver in use: igb_uio
Kernel modules: virtio_pci
c100 is still there. For the notification, try PORT IO if possible.
>
> --yliu
>
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On 1/14/2016 3:49 PM, Yuanhan Liu wrote:
> On Thu, Jan 14, 2016 at 07:47:17AM +, Xie, Huawei wrote:
>> On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
>>> Modern (v1.0) virtio pci device defines several pci capabilities.
>> [snip]
>>> +static void
>>> +modern_notify_queue(struct virtio_hw *hw __rte_unused, struct virtqueue
>>> *vq)
>>> +{
>>> + modern_write16(1, vq->notify_addr);
>>> +}
>> Does virtio 1.0 only supports MMIO? MMIO has long VMEXIT latency than
>> PORT IO.
> Virtio 1.0 supports three transport layer, including MMIO and PCI. And
> we use PCI only in our pmd driver.
I don't mean that MMIO but use memory mapped IO for configuration.
>
> --yliu
>
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
> Modern (v1.0) virtio pci device defines several pci capabilities.
> Each cap has a configure structure corresponding to it, and the
> cap.bar and cap.offset fields tell us where to find it.
>
> Firstly, we map the pci resources by rte_eal_pci_map_device().
> We then could easily locate to a cfg structure by:
s/Locate/Locate to/
>
> cfg_addr = dev->mem_resources[cap.bar].addr + cap.offset;
>
> Therefore, the entrance of enabling modern (v1.0) pci device support
> is to iterate the pci capability lists, and to locate some configs
> we care; and they are:
>
> - common cfg
>
> For generic virtio and virtuqueu configuration, such as setting/getting
typo for virtqueue
> features, enabling a specific queue, and so on.
>
> - nofity cfg
>
> Combining with `queue_notify_off' from common cfg, we could use it to
> notify a specific virt queue.
>
> - device cfg
>
> Where virtio_net_config structure locates.
is located
> If any of above cap is not found, we fallback to the legacy virtio
>
[SNIP]
>
>
>
> +#define MODERN_READ_DEF(nr_bits, type) \
> +static inline type \
> +modern_read##nr_bits(type *addr) \
> +{\
> + return *(volatile type *)addr; \
> +}
> +
> +#define MODERN_WRITE_DEF(nr_bits, type) \
> +static inline void \
> +modern_write##nr_bits(type val, type *addr) \
> +{\
> + *(volatile type *)addr = val; \
> +}
> +
> +MODERN_READ_DEF (8, uint8_t)
> +MODERN_WRITE_DEF(8, uint8_t)
> +
> +MODERN_READ_DEF (16, uint16_t)
> +MODERN_WRITE_DEF(16, uint16_t)
> +
> +MODERN_READ_DEF (32, uint32_t)
> +MODERN_WRITE_DEF(32, uint32_t)
> +
> +static inline void
> +modern_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi)
> +{
> + modern_write32((uint32_t)val, lo);
> + modern_write32(val >> 32, hi);
> +}
> +
This is normal mmio read/write operation. ioread8/16/32/64 or just
readxx is more meaningful name here.
> +static void
[SNIP]
> +
> +static void
> +modern_write_dev_config(struct virtio_hw *hw, uint64_t offset,
> + void *src, int length)
define src as const
[snip]
>
> +static inline void *
> +get_cfg_addr(struct rte_pci_device *dev, struct virtio_pci_cap *cap)
No explicit inline for non performance critical functions.
> +{
> + uint8_t bar= cap->bar;
> + uint32_t length = cap->length;
> + uint32_t offset = cap->offset;
> + uint8_t *base;
> +
> + if (unlikely(bar > 5)) {
Don't use constant value number whenever possible
No likely/unlikely for non performance critical functions
> + PMD_INIT_LOG(ERR, "invalid bar: %u", bar);
> + return NULL;
> + }
> +
> + if (unlikely(offset + length > dev->mem_resource[bar].len)) {
> + PMD_INIT_LOG(ERR,
> + "invalid cap: overflows bar space: %u > %"PRIu64,
> + offset + length, dev->mem_resource[bar].len);
> + return NULL;
> + }
> +
> + base = dev->mem_resource[bar].addr;
> + if (unlikely(base == NULL)) {
> + PMD_INIT_LOG(ERR, "bar %u base addr is NULL", bar);
> + return NULL;
> + }
> +
> + return base + offset;
> +}
> +
> +static int
> +virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
> +{
> + uint8_t pos;
> + struct virtio_pci_cap cap;
> + int ret;
> +
> + if (rte_eal_pci_map_device(dev) < 0) {
> + PMD_INIT_LOG(DEBUG, "failed to map pci device!");
s /DEBUG/ERR/
> + return -1;
> + }
> +
> + ret = rte_eal_pci_read_config(dev, , 1, PCI_CAPABILITY_LIST);
> +
[snip]
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On 1/12/2016 2:58 PM, Yuanhan Liu wrote:
> Modern (v1.0) virtio pci device defines several pci capabilities.
[snip]
> +static void
> +modern_notify_queue(struct virtio_hw *hw __rte_unused, struct virtqueue *vq)
> +{
> + modern_write16(1, vq->notify_addr);
> +}
Does virtio 1.0 only supports MMIO? MMIO has long VMEXIT latency than
PORT IO.
[snip]
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On Wed, Jan 13, 2016 at 12:31:43PM +0900, Tetsuya Mukawa wrote:
> On 2016/01/12 15:59, Yuanhan Liu wrote:
> > +static int
> > +virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
> > +{
> > + uint8_t pos;
> > + struct virtio_pci_cap cap;
> > + int ret;
> > +
> > + if (rte_eal_pci_map_device(dev) < 0) {
> > + PMD_INIT_LOG(DEBUG, "failed to map pci device!");
> > + return -1;
> > + }
> > +
>
> Do you need to call rte_eal_pci_unmap_device() in somewhere in this file?
Yes, we should. I will see where I can find a proper place for it in
next version; eth_virtio_dev_uninit sounds like a good option.
> Anyway, I've reviewed and tested your all patches.
> And it seems except for it, I guess your patches are good.
Thank you for reviewing and testing it!
--yliu
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
On 2016/01/12 15:59, Yuanhan Liu wrote:
> +static int
> +virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
> +{
> + uint8_t pos;
> + struct virtio_pci_cap cap;
> + int ret;
> +
> + if (rte_eal_pci_map_device(dev) < 0) {
> + PMD_INIT_LOG(DEBUG, "failed to map pci device!");
> + return -1;
> + }
> +
Do you need to call rte_eal_pci_unmap_device() in somewhere in this file?
Anyway, I've reviewed and tested your all patches.
And it seems except for it, I guess your patches are good.
Thanks,
Tetsuya
[dpdk-dev] [PATCH v2 7/7] virtio: add 1.0 support
Modern (v1.0) virtio pci device defines several pci capabilities.
Each cap has a configure structure corresponding to it, and the
cap.bar and cap.offset fields tell us where to find it.
Firstly, we map the pci resources by rte_eal_pci_map_device().
We then could easily locate to a cfg structure by:
cfg_addr = dev->mem_resources[cap.bar].addr + cap.offset;
Therefore, the entrance of enabling modern (v1.0) pci device support
is to iterate the pci capability lists, and to locate some configs
we care; and they are:
- common cfg
For generic virtio and virtuqueu configuration, such as setting/getting
features, enabling a specific queue, and so on.
- nofity cfg
Combining with `queue_notify_off' from common cfg, we could use it to
notify a specific virt queue.
- device cfg
Where virtio_net_config structure locates.
- isr cfg
Where to read isr (interrupt status).
If any of above cap is not found, we fallback to the legacy virtio
handling.
If succeed, hw->vtpci_ops is assigned to modern_ops, where all
operations are implemented by reading/writing a (or few) specific
configuration space from above 4 cfg structures. And that's basically
how this patch works.
Besides those changes, virtio 1.0 introduces a new status field:
FEATURES_OK, which is set after features negotiation is done.
Last, set the VIRTIO_F_VERSION_1 feature flag.
Signed-off-by: Yuanhan Liu
---
v2: - re-read status after setting FEATURES_OK to make sure status is
set correctly.
- Add isr reading and config irq setting support.
- Define some pci macro on our own to not get the dependency of
linux/pci_regs.h, as there should be no such file at non-Linux
platform
---
doc/guides/rel_notes/release_2_3.rst | 3 +
drivers/net/virtio/virtio_ethdev.c | 24 ++-
drivers/net/virtio/virtio_ethdev.h | 3 +-
drivers/net/virtio/virtio_pci.c | 335 ++-
drivers/net/virtio/virtio_pci.h | 67 +++
drivers/net/virtio/virtqueue.h | 2 +
6 files changed, 429 insertions(+), 5 deletions(-)
diff --git a/doc/guides/rel_notes/release_2_3.rst
b/doc/guides/rel_notes/release_2_3.rst
index 99de186..c390d97 100644
--- a/doc/guides/rel_notes/release_2_3.rst
+++ b/doc/guides/rel_notes/release_2_3.rst
@@ -4,6 +4,9 @@ DPDK Release 2.3
New Features
+* **Virtio 1.0 support.**
+
+ Enabled virtio 1.0 support for virtio pmd driver.
Resolved Issues
---
diff --git a/drivers/net/virtio/virtio_ethdev.c
b/drivers/net/virtio/virtio_ethdev.c
index 94e0c4a..1afaba4 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -927,7 +927,7 @@ virtio_vlan_filter_set(struct rte_eth_dev *dev, uint16_t
vlan_id, int on)
return virtio_send_command(hw->cvq, , , 1);
}
-static void
+static int
virtio_negotiate_features(struct virtio_hw *hw)
{
uint64_t host_features;
@@ -949,6 +949,22 @@ virtio_negotiate_features(struct virtio_hw *hw)
hw->guest_features = vtpci_negotiate_features(hw, host_features);
PMD_INIT_LOG(DEBUG, "features after negotiate = %"PRIx64,
hw->guest_features);
+
+ if (hw->modern) {
+ if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
+ PMD_INIT_LOG(ERR,
+ "VIRTIO_F_VERSION_1 features is not enabled.");
+ return -1;
+ }
+ vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
+ if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK))
{
+ PMD_INIT_LOG(ERR,
+ "failed to set FEATURES_OK status!");
+ return -1;
+ }
+ }
+
+ return 0;
}
/*
@@ -1032,7 +1048,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
/* Tell the host we've known how to drive the device. */
vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
- virtio_negotiate_features(hw);
+ if (virtio_negotiate_features(hw) < 0)
+ return -1;
/* If host does not support status then disable LSC */
if (!vtpci_with_feature(hw, VIRTIO_NET_F_STATUS))
@@ -1043,7 +1060,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
rx_func_get(eth_dev);
/* Setting up rx_header size for the device */
- if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+ if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
+ vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
else
hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr);
diff --git a/drivers/net/virtio/virtio_ethdev.h
b/drivers/net/virtio/virtio_ethdev.h
index ae2d47d..fed9571 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -64,7 +64,8 @@
1u << VIRTIO_NET_F_CTRL_VQ | \
1u
