[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On Tue, Jan 19, 2016 at 07:44:04AM +, Xie, Huawei wrote:
> On 1/19/2016 1:53 PM, Yuanhan Liu wrote:
> > On Mon, Jan 18, 2016 at 04:50:16PM +, Xie, Huawei wrote:
> >> On 1/15/2016 12:34 PM, Yuanhan Liu wrote:
> >>> -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);
> >> Here if we are not modern device, we should remove VIRTIO_F_VERSION_1 in
> >> guest features.
> > VIRTIO_F_VERSION_1 should not be set for legacy virtio device at all.
>
> Yes, but here this patch sets this VIRTIO_F_VERSION_1 feature even for
> legacy virtio device.
> It doesn't cause issues, but better remove it for legacy virtio device.
It's not set for legacy virtio device (at hw->guest_features), how could
you remove it then???
--yliu
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On Mon, Jan 18, 2016 at 04:50:16PM +, Xie, Huawei wrote:
> On 1/15/2016 12:34 PM, Yuanhan Liu wrote:
> > -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);
>
> Here if we are not modern device, we should remove VIRTIO_F_VERSION_1 in
> guest features.
VIRTIO_F_VERSION_1 should not be set for legacy virtio device at all.
--yliu
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On Tue, Jan 19, 2016 at 02:48:39AM +, Xie, Huawei wrote:
> On 1/19/2016 10:44 AM, Yuanhan Liu wrote:
> > On Tue, Jan 19, 2016 at 01:51:30AM +, Xie, Huawei wrote:
> >> On 1/19/2016 9:34 AM, Yuanhan Liu wrote:
> >>> On Mon, Jan 18, 2016 at 05:07:51PM +, Xie, Huawei wrote:
> .On 1/15/2016 12:34 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.
> >
> [snip]
> > +
> > +static inline void
> > +io_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi)
> > +{
> > + io_write32((uint32_t)val, lo);
> > + io_write32(val >> 32, hi);
> Firstly your second iowrite32 doesn't do the conversion.
> >>> Because it's not necessary. The first one is for retrieving the low 32
> >>> bits.
> >> I don't mean the shift operation, but the conversion from 64bit to 32bit.
> >> Same applied to below.
> > It's more than a casting here: it's same as "val & (1<<32 - 1)", as
> > stated above, to retrieve the low 32 bits.
> >
> > I know it still could work without it, but, hey, what's wrong to make
> > it explicit?
>
> Say x = val, y = val >> 32, both with (uint32_t) or both not. Be
> consistent and simple.
fine.
--yliu
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On Tue, Jan 19, 2016 at 01:51:30AM +, Xie, Huawei wrote:
> On 1/19/2016 9:34 AM, Yuanhan Liu wrote:
> > On Mon, Jan 18, 2016 at 05:07:51PM +, Xie, Huawei wrote:
> >> .On 1/15/2016 12:34 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.
> >>>
> >> [snip]
> >>> +
> >>> +static inline void
> >>> +io_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi)
> >>> +{
> >>> + io_write32((uint32_t)val, lo);
> >>> + io_write32(val >> 32, hi);
> >> Firstly your second iowrite32 doesn't do the conversion.
> > Because it's not necessary. The first one is for retrieving the low 32
> > bits.
>
> I don't mean the shift operation, but the conversion from 64bit to 32bit.
> Same applied to below.
It's more than a casting here: it's same as "val & (1<<32 - 1)", as
stated above, to retrieve the low 32 bits.
I know it still could work without it, but, hey, what's wrong to make
it explicit?
--yliu
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On Mon, Jan 18, 2016 at 05:07:51PM +, Xie, Huawei wrote:
> .On 1/15/2016 12:34 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.
> >
> [snip]
> > +
> > +static inline void
> > +io_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi)
> > +{
> > + io_write32((uint32_t)val, lo);
> > + io_write32(val >> 32, hi);
>
> Firstly your second iowrite32 doesn't do the conversion.
Because it's not necessary. The first one is for retrieving the low 32
bits.
> The conversion is duplicated.
What do you mean by "duplicated".
--yliu
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On 1/19/2016 3:44 PM, Xie, Huawei wrote:
> On 1/19/2016 1:53 PM, Yuanhan Liu wrote:
>> On Mon, Jan 18, 2016 at 04:50:16PM +, Xie, Huawei wrote:
>>> On 1/15/2016 12:34 PM, Yuanhan Liu wrote:
-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);
>>> Here if we are not modern device, we should remove VIRTIO_F_VERSION_1 in
>>> guest features.
>> VIRTIO_F_VERSION_1 should not be set for legacy virtio device at all.
> Yes, but here this patch sets this VIRTIO_F_VERSION_1 feature even for
> legacy virtio device.
> It doesn't cause issues, but better remove it for legacy virtio device.
OK, I see the legacy_get_features return a 32bit value which masks the
higher bits which contains the host VIRTIO_F_VERSION_1 feature. This is
very implict.
>> --yliu
>>
>
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On 1/19/2016 1:53 PM, Yuanhan Liu wrote:
> On Mon, Jan 18, 2016 at 04:50:16PM +, Xie, Huawei wrote:
>> On 1/15/2016 12:34 PM, Yuanhan Liu wrote:
>>> -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);
>> Here if we are not modern device, we should remove VIRTIO_F_VERSION_1 in
>> guest features.
> VIRTIO_F_VERSION_1 should not be set for legacy virtio device at all.
Yes, but here this patch sets this VIRTIO_F_VERSION_1 feature even for
legacy virtio device.
It doesn't cause issues, but better remove it for legacy virtio device.
>
> --yliu
>
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On 1/19/2016 10:44 AM, Yuanhan Liu wrote:
> On Tue, Jan 19, 2016 at 01:51:30AM +, Xie, Huawei wrote:
>> On 1/19/2016 9:34 AM, Yuanhan Liu wrote:
>>> On Mon, Jan 18, 2016 at 05:07:51PM +, Xie, Huawei wrote:
.On 1/15/2016 12:34 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.
>
[snip]
> +
> +static inline void
> +io_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi)
> +{
> + io_write32((uint32_t)val, lo);
> + io_write32(val >> 32, hi);
Firstly your second iowrite32 doesn't do the conversion.
>>> Because it's not necessary. The first one is for retrieving the low 32
>>> bits.
>> I don't mean the shift operation, but the conversion from 64bit to 32bit.
>> Same applied to below.
> It's more than a casting here: it's same as "val & (1<<32 - 1)", as
> stated above, to retrieve the low 32 bits.
>
> I know it still could work without it, but, hey, what's wrong to make
> it explicit?
Say x = val, y = val >> 32, both with (uint32_t) or both not. Be
consistent and simple.
>
> --yliu
>
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
.On 1/15/2016 12:34 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.
>
[snip]
> +
> +static inline void
> +io_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi)
> +{
> + io_write32((uint32_t)val, lo);
> + io_write32(val >> 32, hi);
Firstly your second iowrite32 doesn't do the conversion. The conversion
is duplicated.
> +}
> +
> +static void
> +modern_read_dev_config(struct virtio_hw *hw, uint64_t offset,
here and there, size_t is more accurate for offset as we get it from
offsetof.
> +void *dst, int length)
> +{
> + int i;
> + uint8_t *p;
> + uint8_t old_gen, new_gen;
> +
> + do {
> + old_gen = io_read8(>common_cfg->config_generation);
> +
> + p = dst;
> + for (i = 0; i < length; i++)
> + *p++ = io_read8((uint8_t *)hw->dev_cfg + offset + i);
> +
> + new_gen = io_read8(>common_cfg->config_generation);
> + } while (old_gen != new_gen);
> +}
> +
> +static void
> +modern_write_dev_config(struct virtio_hw *hw, uint64_t offset,
> + const void *src, int length)
> +{
> + int i;
> + const uint8_t *p = src;
> +
> + for (i = 0; i < length; i++)
> + io_write8(*p++, (uint8_t *)hw->dev_cfg + offset + i);
> +}
> +
> +static uint64_t
> +modern_get_features(struct virtio_hw *hw)
> +{
> + uint32_t features_lo, features_hi;
> +
> + io_write32(0, >common_cfg->device_feature_select);
> + features_lo = io_read32(>common_cfg->device_feature);
> +
> + io_write32(1, >common_cfg->device_feature_select);
> + features_hi = io_read32(>common_cfg->device_feature);
> +
> + return ((uint64_t)(features_hi) << 32) | features_lo;
> +}
> +
> +static void
> +modern_set_features(struct virtio_hw *hw, uint64_t features)
> +{
> + io_write32(0, >common_cfg->guest_feature_select);
> + io_write32(features & ((1ULL << 32) - 1),
again, duplicated conversion
> + >common_cfg->guest_feature);
> +
> + io_write32(1, >common_cfg->guest_feature_select);
[snip]
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On 1/15/2016 12:34 PM, Yuanhan Liu wrote:
> -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);
Here if we are not modern device, we should remove VIRTIO_F_VERSION_1 in
guest 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;
> }
[dpdk-dev] [PATCH v4 7/8] virtio: add 1.0 support
On 1/15/2016 12:34 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.
>
[snip]
>
> +static void *
> +get_cfg_addr(struct rte_pci_device *dev, struct virtio_pci_cap *cap)
> +{
> + uint8_t bar= cap->bar;
> + uint32_t length = cap->length;
> + uint32_t offset = cap->offset;
> + uint8_t *base;
> +
> + if (bar > 5) {
> + PMD_INIT_LOG(ERR, "invalid bar: %u", bar);
> + return NULL;
> + }
> +
> + if (offset + length > dev->mem_resource[bar].len) {
offset + length could overflow 32bit limit
> + PMD_INIT_LOG(ERR,
> + "invalid cap: overflows bar space: %u > %"PRIu64,
> + offset + length, dev->mem_resource[bar].len);
> + return NULL;
> + }
> +
> +
[snip]
[dpdk-dev] [PATCH v4 7/8] 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 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 virtqueue 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 is located.
- 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
v3: - invoke rte_eal_pci_unmap_device() at uninit stage
v4: - remove unnessary inline, and likely/unlikely
- mark "src" arg as const for write_dev_cfg operation
---
doc/guides/rel_notes/release_2_3.rst | 3 +
drivers/net/virtio/virtio_ethdev.c | 25 ++-
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, 430 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..deb0382 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);
@@ -1159,6 +1177,7 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
