| Message ID | 1457593280-25412-1-git-send-email-yuanhan.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Thomas Monjalon |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 6EB9B2BB2; Thu, 10 Mar 2016 07:58:41 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 408E62BA2 for <dev@dpdk.org>; Thu, 10 Mar 2016 07:58:40 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 09 Mar 2016 22:58:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,314,1455004800"; d="scan'208";a="930734650" Received: from yliu-dev.sh.intel.com ([10.239.66.49]) by orsmga002.jf.intel.com with ESMTP; 09 Mar 2016 22:58:38 -0800 From: Yuanhan Liu <yuanhan.liu@linux.intel.com> To: dev@dpdk.org Date: Thu, 10 Mar 2016 15:01:20 +0800 Message-Id: <1457593280-25412-1-git-send-email-yuanhan.liu@linux.intel.com> X-Mailer: git-send-email 1.9.0 Subject: [dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Yuanhan Liu
March 10, 2016, 7:01 a.m. UTC
Declare dst as type uint32_t instead of uint64_t, otherwise, we will get
a random upper 32 bit feature bits, as the following io port read reads
lower 32 bit only. It could lead a feature bits that include VIRTIO_F_VERSION_1
(the 32th bit) for legacy virtio, which is obviously wrong.
Fixes: b8f04520ad71 ("virtio: use PCI ioport API")
Cc: David Marchand <david.marchand@6wind.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
drivers/net/virtio/virtio_pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 3/10/2016 3:01 PM, Yuanhan Liu wrote: > Declare dst as type uint32_t instead of uint64_t, otherwise, we will get > a random upper 32 bit feature bits, as the following io port read reads > lower 32 bit only. It could lead a feature bits that include VIRTIO_F_VERSION_1 > (the 32th bit) for legacy virtio, which is obviously wrong. > > Fixes: b8f04520ad71 ("virtio: use PCI ioport API") > > Cc: David Marchand <david.marchand@6wind.com> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > --- > drivers/net/virtio/virtio_pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c > index 98fc370..c007959 100644 > --- a/drivers/net/virtio/virtio_pci.c > +++ b/drivers/net/virtio/virtio_pci.c > @@ -74,7 +74,7 @@ legacy_write_dev_config(struct virtio_hw *hw, size_t offset, > static uint64_t > legacy_get_features(struct virtio_hw *hw) > { > - uint64_t dst; > + uint32_t dst; > > rte_eal_pci_ioport_read(&hw->io, &dst, 4, VIRTIO_PCI_HOST_FEATURES); > return dst; Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
On Thu, Mar 10, 2016 at 8:01 AM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > Declare dst as type uint32_t instead of uint64_t, otherwise, we will get > a random upper 32 bit feature bits, as the following io port read reads > lower 32 bit only. It could lead a feature bits that include VIRTIO_F_VERSION_1 > (the 32th bit) for legacy virtio, which is obviously wrong. > > Fixes: b8f04520ad71 ("virtio: use PCI ioport API") > > Cc: David Marchand <david.marchand@6wind.com> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> Argh, good catch. Relooked at my patch, this should be the only bug (of this kind ;-)). Reviewed-by: David Marchand <david.marchand@6wind.com>
On Thu, Mar 10, 2016 at 08:43:37AM +0100, David Marchand wrote: > On Thu, Mar 10, 2016 at 8:01 AM, Yuanhan Liu > <yuanhan.liu@linux.intel.com> wrote: > > Declare dst as type uint32_t instead of uint64_t, otherwise, we will get > > a random upper 32 bit feature bits, as the following io port read reads > > lower 32 bit only. It could lead a feature bits that include VIRTIO_F_VERSION_1 > > (the 32th bit) for legacy virtio, which is obviously wrong. > > > > Fixes: b8f04520ad71 ("virtio: use PCI ioport API") > > > > Cc: David Marchand <david.marchand@6wind.com> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > Argh, good catch. > Relooked at my patch, this should be the only bug (of this kind ;-)). Yes, I also have a check while making this patch. --yliu > > Reviewed-by: David Marchand <david.marchand@6wind.com> > > > -- > David Marchand
On Thu, Mar 10, 2016 at 03:50:54PM +0800, Yuanhan Liu wrote: > On Thu, Mar 10, 2016 at 08:43:37AM +0100, David Marchand wrote: > > On Thu, Mar 10, 2016 at 8:01 AM, Yuanhan Liu > > <yuanhan.liu@linux.intel.com> wrote: > > > Declare dst as type uint32_t instead of uint64_t, otherwise, we will get > > > a random upper 32 bit feature bits, as the following io port read reads > > > lower 32 bit only. It could lead a feature bits that include VIRTIO_F_VERSION_1 > > > (the 32th bit) for legacy virtio, which is obviously wrong. > > > > > > Fixes: b8f04520ad71 ("virtio: use PCI ioport API") > > > > > > Cc: David Marchand <david.marchand@6wind.com> > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > Argh, good catch. > > Relooked at my patch, this should be the only bug (of this kind ;-)). > > Yes, I also have a check while making this patch. > > --yliu > > > > Reviewed-by: David Marchand <david.marchand@6wind.com> > > > > > > -- > > David Marchand Hi, this patch no longer applies to dpdk-next-net/rel_16_04 branch due to changes in legacy_get_features function. Is it still needed? If so, please do a V2. Regards, /Bruce
On Mon, Mar 14, 2016 at 10:48:31AM +0000, Bruce Richardson wrote: > On Thu, Mar 10, 2016 at 03:50:54PM +0800, Yuanhan Liu wrote: > > On Thu, Mar 10, 2016 at 08:43:37AM +0100, David Marchand wrote: > > > On Thu, Mar 10, 2016 at 8:01 AM, Yuanhan Liu > > > <yuanhan.liu@linux.intel.com> wrote: > > > > Declare dst as type uint32_t instead of uint64_t, otherwise, we will get > > > > a random upper 32 bit feature bits, as the following io port read reads > > > > lower 32 bit only. It could lead a feature bits that include VIRTIO_F_VERSION_1 > > > > (the 32th bit) for legacy virtio, which is obviously wrong. > > > > > > > > Fixes: b8f04520ad71 ("virtio: use PCI ioport API") > > > > > > > > Cc: David Marchand <david.marchand@6wind.com> > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > > > Argh, good catch. > > > Relooked at my patch, this should be the only bug (of this kind ;-)). > > > > Yes, I also have a check while making this patch. > > > > --yliu > > > > > > Reviewed-by: David Marchand <david.marchand@6wind.com> > > > > > > > > > -- > > > David Marchand > Hi, > > this patch no longer applies to dpdk-next-net/rel_16_04 branch due to changes > in legacy_get_features function. Hi Bruce, It's a patch based on Thomas' tree. And this patch fixes an regression at there, too. --yliu > Is it still needed? If so, please do a V2. > > Regards, > /Bruce
On Mon, Mar 14, 2016 at 08:44:32PM +0800, Yuanhan Liu wrote: > On Mon, Mar 14, 2016 at 10:48:31AM +0000, Bruce Richardson wrote: > > On Thu, Mar 10, 2016 at 03:50:54PM +0800, Yuanhan Liu wrote: > > > On Thu, Mar 10, 2016 at 08:43:37AM +0100, David Marchand wrote: > > > > On Thu, Mar 10, 2016 at 8:01 AM, Yuanhan Liu > > > > <yuanhan.liu@linux.intel.com> wrote: > > > > > Declare dst as type uint32_t instead of uint64_t, otherwise, we will get > > > > > a random upper 32 bit feature bits, as the following io port read reads > > > > > lower 32 bit only. It could lead a feature bits that include VIRTIO_F_VERSION_1 > > > > > (the 32th bit) for legacy virtio, which is obviously wrong. > > > > > > > > > > Fixes: b8f04520ad71 ("virtio: use PCI ioport API") > > > > > > > > > > Cc: David Marchand <david.marchand@6wind.com> > > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > > > > > Argh, good catch. > > > > Relooked at my patch, this should be the only bug (of this kind ;-)). > > > > > > Yes, I also have a check while making this patch. > > > > > > --yliu > > > > > > > > Reviewed-by: David Marchand <david.marchand@6wind.com> > > > > > > > > > > > > -- > > > > David Marchand > > Hi, > > > > this patch no longer applies to dpdk-next-net/rel_16_04 branch due to changes > > in legacy_get_features function. > > Hi Bruce, > > It's a patch based on Thomas' tree. And this patch fixes an regression > at there, too. > > --yliu > Ok, thanks for the info. Thomas, I'm delegating this patch to you in patchwork to apply to your tree. /Bruce
2016-03-10 08:43, David Marchand: > On Thu, Mar 10, 2016 at 8:01 AM, Yuanhan Liu > <yuanhan.liu@linux.intel.com> wrote: > > Declare dst as type uint32_t instead of uint64_t, otherwise, we will get > > a random upper 32 bit feature bits, as the following io port read reads > > lower 32 bit only. It could lead a feature bits that include VIRTIO_F_VERSION_1 > > (the 32th bit) for legacy virtio, which is obviously wrong. > > > > Fixes: b8f04520ad71 ("virtio: use PCI ioport API") > > > > Cc: David Marchand <david.marchand@6wind.com> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > Argh, good catch. > Relooked at my patch, this should be the only bug (of this kind ;-)). > > Reviewed-by: David Marchand <david.marchand@6wind.com> Applied, thanks
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c index 98fc370..c007959 100644 --- a/drivers/net/virtio/virtio_pci.c +++ b/drivers/net/virtio/virtio_pci.c @@ -74,7 +74,7 @@ legacy_write_dev_config(struct virtio_hw *hw, size_t offset, static uint64_t legacy_get_features(struct virtio_hw *hw) { - uint64_t dst; + uint32_t dst; rte_eal_pci_ioport_read(&hw->io, &dst, 4, VIRTIO_PCI_HOST_FEATURES); return dst;