[dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[Yuanhan Liu]

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;
-- 
1.9.0

Re: [dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[Tan, Jianfeng]

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>

Re: [dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[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>


-- 
David Marchand

Re: [dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[Yuanhan Liu]

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

Re: [dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[Bruce Richardson]

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

Re: [dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[Yuanhan Liu]

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

Re: [dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[Bruce Richardson]

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

Re: [dpdk-dev] [PATCH] virtio: fix wrong features returned for legacy virtio

[Thomas Monjalon]

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