qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3
@ 2015-12-21 11:06 Miao Yan
  2015-12-21 11:06 ` [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure Miao Yan
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Miao Yan @ 2015-12-21 11:06 UTC (permalink / raw)
  To: jasowang, dmitry, qemu-devel

Qemu vmxnet3 emulation doesn't recognize VMXNET3_CMD_GET_DID_LO,
VMXNET3_CMD_GET_DID_HI and VMXNET3_CMD_GET_DEV_EXTRA_INFO command and 
returns -1 on all of them.

This patchset makes them return correct values.

Miao Yan (3):
  net/vmxnet3: return 1 on device activation failure
  net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID command
  net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO

 hw/net/vmxnet3.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

-- 
1.9.1

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure
  2015-12-21 11:06 [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Miao Yan
@ 2015-12-21 11:06 ` Miao Yan
  2015-12-21 18:15   ` P J P
  2015-12-21 11:06 ` [Qemu-devel] [PATCH 2/3] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command Miao Yan
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Miao Yan @ 2015-12-21 11:06 UTC (permalink / raw)
  To: jasowang, dmitry, qemu-devel

When reading device status, 0 means device is successfully
activated and 1 means error.

So return 1 on device activation failure instead of -1;

Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
---
 hw/net/vmxnet3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index e168285..9185408 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1652,7 +1652,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
 
     switch (s->last_command) {
     case VMXNET3_CMD_ACTIVATE_DEV:
-        ret = (s->device_active) ? 0 : -1;
+        ret = (s->device_active) ? 0 : 1;
         VMW_CFPRN("Device active: %" PRIx64, ret);
         break;
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH 2/3] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command
  2015-12-21 11:06 [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Miao Yan
  2015-12-21 11:06 ` [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure Miao Yan
@ 2015-12-21 11:06 ` Miao Yan
  2015-12-21 11:06 ` [Qemu-devel] [PATCH 3/3] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO Miao Yan
  2015-12-21 17:19 ` [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Dmitry Fleytman
  3 siblings, 0 replies; 14+ messages in thread
From: Miao Yan @ 2015-12-21 11:06 UTC (permalink / raw)
  To: jasowang, dmitry, qemu-devel

VMXNET3_CMD_GET_DID_LO should return PCI ID of the device
and VMXNET3_CMD_GET_DID_HI should return vmxnet3 revision ID.

Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
---
 hw/net/vmxnet3.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 9185408..3517aab 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1683,6 +1683,14 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
         ret = VMXNET3_DISABLE_ADAPTIVE_RING;
         break;
 
+    case VMXNET3_CMD_GET_DID_LO:
+        ret = PCI_DEVICE_ID_VMWARE_VMXNET3;
+        break;
+
+    case VMXNET3_CMD_GET_DID_HI:
+        ret = VMXNET3_DEVICE_REVISION;
+        break;
+
     default:
         VMW_WRPRN("Received request for unknown command: %x", s->last_command);
         ret = -1;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH 3/3] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO
  2015-12-21 11:06 [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Miao Yan
  2015-12-21 11:06 ` [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure Miao Yan
  2015-12-21 11:06 ` [Qemu-devel] [PATCH 2/3] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command Miao Yan
@ 2015-12-21 11:06 ` Miao Yan
  2015-12-21 17:19 ` [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Dmitry Fleytman
  3 siblings, 0 replies; 14+ messages in thread
From: Miao Yan @ 2015-12-21 11:06 UTC (permalink / raw)
  To: jasowang, dmitry, qemu-devel

VMXNET3_CMD_GET_DEV_EXTRA_INFO should return 0 for emulation
mode

Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
---
 hw/net/vmxnet3.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 3517aab..5b96a02 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1659,6 +1659,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
     case VMXNET3_CMD_RESET_DEV:
     case VMXNET3_CMD_QUIESCE_DEV:
     case VMXNET3_CMD_GET_QUEUE_STATUS:
+    case VMXNET3_CMD_GET_DEV_EXTRA_INFO:
         ret = 0;
         break;
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3
  2015-12-21 11:06 [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Miao Yan
                   ` (2 preceding siblings ...)
  2015-12-21 11:06 ` [Qemu-devel] [PATCH 3/3] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO Miao Yan
@ 2015-12-21 17:19 ` Dmitry Fleytman
  2015-12-22  2:44   ` Miao Yan
  3 siblings, 1 reply; 14+ messages in thread
From: Dmitry Fleytman @ 2015-12-21 17:19 UTC (permalink / raw)
  To: Miao Yan; +Cc: jasowang, qemu-devel

Hello Miao,

While patches look good technically, I cannot find any code snippets that prove correctness of these changes.
How do you know this is the correct behaviour? Could you please extend commit messages with corresponding references?

Thanks,
Dmitry

> On 21 Dec 2015, at 13:06 PM, Miao Yan <yanmiaobest@gmail.com> wrote:
> 
> Qemu vmxnet3 emulation doesn't recognize VMXNET3_CMD_GET_DID_LO,
> VMXNET3_CMD_GET_DID_HI and VMXNET3_CMD_GET_DEV_EXTRA_INFO command and 
> returns -1 on all of them.
> 
> This patchset makes them return correct values.
> 
> Miao Yan (3):
>  net/vmxnet3: return 1 on device activation failure
>  net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID command
>  net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO
> 
> hw/net/vmxnet3.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
> 
> -- 
> 1.9.1
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure
  2015-12-21 11:06 ` [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure Miao Yan
@ 2015-12-21 18:15   ` P J P
  2015-12-22  2:56     ` Miao Yan
  0 siblings, 1 reply; 14+ messages in thread
From: P J P @ 2015-12-21 18:15 UTC (permalink / raw)
  To: Miao Yan; +Cc: dmitry, jasowang, qemu-devel

+-- On Mon, 21 Dec 2015, Miao Yan wrote --+
| So return 1 on device activation failure instead of -1;
| 
| Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
| ---
|  hw/net/vmxnet3.c | 2 +-
|  1 file changed, 1 insertion(+), 1 deletion(-)
| 
| diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
| index e168285..9185408 100644
| --- a/hw/net/vmxnet3.c
| +++ b/hw/net/vmxnet3.c
| @@ -1652,7 +1652,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
|  
|      switch (s->last_command) {
|      case VMXNET3_CMD_ACTIVATE_DEV:
| -        ret = (s->device_active) ? 0 : -1;
| +        ret = (s->device_active) ? 0 : 1;
|          VMW_CFPRN("Device active: %" PRIx64, ret);
|          break;

  It seems okay. Considering that the function returns 'uint64_t', -1 would 
become an extremely large value. I wonder if that is intended.

If '1' indicates the error, the 'default:' case in the same switch needs to be 
updated too.

  default:                                                                    
        VMW_WRPRN("Received request for unknown command: %x", s->last_command); 
        ret = -1;                                                               
        break;


Why does the function return 'uint64_t' type? All return values from other 
cases seem to be within uint32_t type.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3
  2015-12-21 17:19 ` [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Dmitry Fleytman
@ 2015-12-22  2:44   ` Miao Yan
  2015-12-22  7:05     ` Dmitry Fleytman
  0 siblings, 1 reply; 14+ messages in thread
From: Miao Yan @ 2015-12-22  2:44 UTC (permalink / raw)
  To: Dmitry Fleytman; +Cc: Jason Wang, QEMU

Hi Dmitry,

2015-12-22 1:19 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com>:
> Hello Miao,
>
> While patches look good technically, I cannot find any code snippets that prove correctness of these changes.

Linux driver does not read those registers currently,
not sure about Windows version.


> How do you know this is the correct behaviour? Could you please extend commit messages with corresponding references?


This behavior can be observed by modifying linux driver to read those registers
at probe time (in vmxnet3_device_probe) and kernel log will have the following
message (running on esxi server, of source):

[  198.427389] VMware vmxnet3 virtual NIC driver - version 1.2.0.0-k-NAPI
[  198.428863] vmxnet3 0000:03:00.0: # of Tx queues : 1, # of Rx queues : 1
[  198.559625] vmxnet3 DID lo: 0x7b0, high: 0x1, dev_info: 0x0
[  198.561151] vmxnet3 0000:03:00.0: irq 72 for MSI/MSI-X

Here DID_LO is the pci device id, DID_HIGH is 0x1 and
GET_DEV_EXTRA_INFO returns 0.

putting above into commit message should be enough ?


>
> Thanks,
> Dmitry
>
>> On 21 Dec 2015, at 13:06 PM, Miao Yan <yanmiaobest@gmail.com> wrote:
>>
>> Qemu vmxnet3 emulation doesn't recognize VMXNET3_CMD_GET_DID_LO,
>> VMXNET3_CMD_GET_DID_HI and VMXNET3_CMD_GET_DEV_EXTRA_INFO command and
>> returns -1 on all of them.
>>
>> This patchset makes them return correct values.
>>
>> Miao Yan (3):
>>  net/vmxnet3: return 1 on device activation failure
>>  net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID command
>>  net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO
>>
>> hw/net/vmxnet3.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> --
>> 1.9.1
>>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure
  2015-12-21 18:15   ` P J P
@ 2015-12-22  2:56     ` Miao Yan
  2015-12-22  9:06       ` P J P
  0 siblings, 1 reply; 14+ messages in thread
From: Miao Yan @ 2015-12-22  2:56 UTC (permalink / raw)
  To: P J P; +Cc: Dmitry Fleytman, Jason Wang, QEMU

2015-12-22 2:15 GMT+08:00 P J P <ppandit@redhat.com>:
> +-- On Mon, 21 Dec 2015, Miao Yan wrote --+
> | So return 1 on device activation failure instead of -1;
> |
> | Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
> | ---
> |  hw/net/vmxnet3.c | 2 +-
> |  1 file changed, 1 insertion(+), 1 deletion(-)
> |
> | diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> | index e168285..9185408 100644
> | --- a/hw/net/vmxnet3.c
> | +++ b/hw/net/vmxnet3.c
> | @@ -1652,7 +1652,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
> |
> |      switch (s->last_command) {
> |      case VMXNET3_CMD_ACTIVATE_DEV:
> | -        ret = (s->device_active) ? 0 : -1;
> | +        ret = (s->device_active) ? 0 : 1;
> |          VMW_CFPRN("Device active: %" PRIx64, ret);
> |          break;
>
>   It seems okay. Considering that the function returns 'uint64_t', -1 would
> become an extremely large value. I wonder if that is intended.
>
> If '1' indicates the error, the 'default:' case in the same switch needs to be
> updated too.


'1' indicates an error on device activation. Not sure
about the 'unknown command' case.


>
>   default:
>         VMW_WRPRN("Received request for unknown command: %x", s->last_command);
>         ret = -1;
>         break;
>
>
> Why does the function return 'uint64_t' type? All return values from other
> cases seem to be within uint32_t type.

Linux driver uses VXMNET3_READ_BAR1_REG() which calls readl().
That should be an indication that the driver expects 32bit values.
But the prototype in MemoryRegionOps requires uint64_t.


>
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3
  2015-12-22  2:44   ` Miao Yan
@ 2015-12-22  7:05     ` Dmitry Fleytman
  2015-12-23  2:15       ` Jason Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Fleytman @ 2015-12-22  7:05 UTC (permalink / raw)
  To: Miao Yan, Jason Wang; +Cc: QEMU

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]


> On 22 Dec 2015, at 04:44 AM, Miao Yan <yanmiaobest@gmail.com> wrote:
> 
> Hi Dmitry,
> 
> 2015-12-22 1:19 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com>:
>> Hello Miao,
>> 
>> While patches look good technically, I cannot find any code snippets that prove correctness of these changes.
> 
> Linux driver does not read those registers currently,
> not sure about Windows version.
> 
> 
>> How do you know this is the correct behaviour? Could you please extend commit messages with corresponding references?
> 
> 
> This behavior can be observed by modifying linux driver to read those registers
> at probe time (in vmxnet3_device_probe) and kernel log will have the following
> message (running on esxi server, of source):
> 
> [  198.427389] VMware vmxnet3 virtual NIC driver - version 1.2.0.0-k-NAPI
> [  198.428863] vmxnet3 0000:03:00.0: # of Tx queues : 1, # of Rx queues : 1
> [  198.559625] vmxnet3 DID lo: 0x7b0, high: 0x1, dev_info: 0x0
> [  198.561151] vmxnet3 0000:03:00.0: irq 72 for MSI/MSI-X
> 
> Here DID_LO is the pci device id, DID_HIGH is 0x1 and
> GET_DEV_EXTRA_INFO returns 0.
> 
> putting above into commit message should be enough ?

Yes, this should be good enough. Please also provide ESX version you’re running on.

Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>

Jason, these changes are guest visible but init time only. Do you think compatibility code is needed in this case?

> 
> 
>> 
>> Thanks,
>> Dmitry
>> 
>>> On 21 Dec 2015, at 13:06 PM, Miao Yan <yanmiaobest@gmail.com> wrote:
>>> 
>>> Qemu vmxnet3 emulation doesn't recognize VMXNET3_CMD_GET_DID_LO,
>>> VMXNET3_CMD_GET_DID_HI and VMXNET3_CMD_GET_DEV_EXTRA_INFO command and
>>> returns -1 on all of them.
>>> 
>>> This patchset makes them return correct values.
>>> 
>>> Miao Yan (3):
>>> net/vmxnet3: return 1 on device activation failure
>>> net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID command
>>> net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO
>>> 
>>> hw/net/vmxnet3.c | 11 ++++++++++-
>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>> 
>>> --
>>> 1.9.1
>>> 
>> 


[-- Attachment #2: Type: text/html, Size: 4496 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure
  2015-12-22  2:56     ` Miao Yan
@ 2015-12-22  9:06       ` P J P
  2015-12-22  9:26         ` Miao Yan
  0 siblings, 1 reply; 14+ messages in thread
From: P J P @ 2015-12-22  9:06 UTC (permalink / raw)
  To: Miao Yan; +Cc: Dmitry Fleytman, Jason Wang, QEMU

+-- On Tue, 22 Dec 2015, Miao Yan wrote --+
| > If '1' indicates the error, the 'default:' case in the same switch needs to be
| > updated too.
| 
| '1' indicates an error on device activation. Not sure about the 'unknown 
| command' case.

  Ideally it should be same, inconsistent return codes wouldn't be helpful.
 
| Linux driver uses VXMNET3_READ_BAR1_REG() which calls readl().
| That should be an indication that the driver expects 32bit values.

  Right.

| But the prototype in MemoryRegionOps requires uint64_t.

  That is odd. It does not seem right to use uint64_t for 32bit values, and 
then return a negative(-1) value.


@Dmitry...?
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure
  2015-12-22  9:06       ` P J P
@ 2015-12-22  9:26         ` Miao Yan
  2015-12-22  9:33           ` Dmitry Fleytman
  0 siblings, 1 reply; 14+ messages in thread
From: Miao Yan @ 2015-12-22  9:26 UTC (permalink / raw)
  To: P J P; +Cc: Dmitry Fleytman, Jason Wang, QEMU

2015-12-22 17:06 GMT+08:00 P J P <ppandit@redhat.com>:
> +-- On Tue, 22 Dec 2015, Miao Yan wrote --+
> | > If '1' indicates the error, the 'default:' case in the same switch needs to be
> | > updated too.
> |
> | '1' indicates an error on device activation. Not sure about the 'unknown
> | command' case.
>
>   Ideally it should be same, inconsistent return codes wouldn't be helpful.

ESXi returns 0 on 'unknown command', I thought we should follow here.


>
> | Linux driver uses VXMNET3_READ_BAR1_REG() which calls readl().
> | That should be an indication that the driver expects 32bit values.
>
>   Right.
>
> | But the prototype in MemoryRegionOps requires uint64_t.
>
>   That is odd. It does not seem right to use uint64_t for 32bit values, and
> then return a negative(-1) value.

I've already sent v2 patch to change this to 0. But 'read' in MemoryRegionOps
does require a uint64_t return type, to which vmxnet3_io_bar1_read()
connects. Seems 'read' handles memory access of any size.

>
>
> @Dmitry...?
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure
  2015-12-22  9:26         ` Miao Yan
@ 2015-12-22  9:33           ` Dmitry Fleytman
  0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Fleytman @ 2015-12-22  9:33 UTC (permalink / raw)
  To: Miao Yan; +Cc: Jason Wang, QEMU, P J P


> On 22 Dec 2015, at 11:26 AM, Miao Yan <yanmiaobest@gmail.com> wrote:
> 
> 2015-12-22 17:06 GMT+08:00 P J P <ppandit@redhat.com>:
>> +-- On Tue, 22 Dec 2015, Miao Yan wrote --+
>> | > If '1' indicates the error, the 'default:' case in the same switch needs to be
>> | > updated too.
>> |
>> | '1' indicates an error on device activation. Not sure about the 'unknown
>> | command' case.
>> 
>>  Ideally it should be same, inconsistent return codes wouldn't be helpful.
> 
> ESXi returns 0 on 'unknown command', I thought we should follow here.

Agree. This is the idea of the emulation.

> 
> 
>> 
>> | Linux driver uses VXMNET3_READ_BAR1_REG() which calls readl().
>> | That should be an indication that the driver expects 32bit values.
>> 
>>  Right.
>> 
>> | But the prototype in MemoryRegionOps requires uint64_t.
>> 
>>  That is odd. It does not seem right to use uint64_t for 32bit values, and
>> then return a negative(-1) value.
> 
> I've already sent v2 patch to change this to 0. But 'read' in MemoryRegionOps
> does require a uint64_t return type, to which vmxnet3_io_bar1_read()
> connects. Seems 'read' handles memory access of any size.

This is how QEMU device API looks like.
This doesn’t matter BTW because 64 bit value will be properly stripped to 32 bits for 4 bytes reads.

> 
>> 
>> 
>> @Dmitry...?
>> --
>> Prasad J Pandit / Red Hat Product Security Team
>> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3
  2015-12-22  7:05     ` Dmitry Fleytman
@ 2015-12-23  2:15       ` Jason Wang
  2015-12-23  3:12         ` Miao Yan
  0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2015-12-23  2:15 UTC (permalink / raw)
  To: Dmitry Fleytman, Miao Yan; +Cc: QEMU



On 12/22/2015 03:05 PM, Dmitry Fleytman wrote:
>
>> On 22 Dec 2015, at 04:44 AM, Miao Yan <yanmiaobest@gmail.com
>> <mailto:yanmiaobest@gmail.com>> wrote:
>>
>> Hi Dmitry,
>>
>> 2015-12-22 1:19 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com
>> <mailto:dmitry@daynix.com>>:
>>> Hello Miao,
>>>
>>> While patches look good technically, I cannot find any code snippets
>>> that prove correctness of these changes.
>>
>> Linux driver does not read those registers currently,
>> not sure about Windows version.
>>
>>
>>> How do you know this is the correct behaviour? Could you please
>>> extend commit messages with corresponding references?
>>
>>
>> This behavior can be observed by modifying linux driver to read those
>> registers
>> at probe time (in vmxnet3_device_probe) and kernel log will have the
>> following
>> message (running on esxi server, of source):
>>
>> [  198.427389] VMware vmxnet3 virtual NIC driver - version 1.2.0.0-k-NAPI
>> [  198.428863] vmxnet3 0000:03:00.0: # of Tx queues : 1, # of Rx
>> queues : 1
>> [  198.559625] vmxnet3 DID lo: 0x7b0, high: 0x1, dev_info: 0x0
>> [  198.561151] vmxnet3 0000:03:00.0: irq 72 for MSI/MSI-X
>>
>> Here DID_LO is the pci device id, DID_HIGH is 0x1 and
>> GET_DEV_EXTRA_INFO returns 0.
>>
>> putting above into commit message should be enough ?
>
> Yes, this should be good enough. Please also provide ESX version
> you’re running on.
>
> Reviewed-by: Dmitry Fleytman <dmitry@daynix.com
> <mailto:dmitry@daynix.com>>
>
> Jason, these changes are guest visible but init time only. Do you
> think compatibility code is needed in this case?

I think it was probably no need to do compatibility things here,
consider no device state but only register read changes and I believe no
driver should depend on those values.

>
>>
>>
>>>
>>> Thanks,
>>> Dmitry
>>>
>>>> On 21 Dec 2015, at 13:06 PM, Miao Yan <yanmiaobest@gmail.com
>>>> <mailto:yanmiaobest@gmail.com>> wrote:
>>>>
>>>> Qemu vmxnet3 emulation doesn't recognize VMXNET3_CMD_GET_DID_LO,
>>>> VMXNET3_CMD_GET_DID_HI and VMXNET3_CMD_GET_DEV_EXTRA_INFO command and
>>>> returns -1 on all of them.
>>>>
>>>> This patchset makes them return correct values.
>>>>
>>>> Miao Yan (3):
>>>> net/vmxnet3: return 1 on device activation failure
>>>> net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID command
>>>> net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO
>>>>
>>>> hw/net/vmxnet3.c | 11 ++++++++++-
>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> --
>>>> 1.9.1
>>>>
>>>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3
  2015-12-23  2:15       ` Jason Wang
@ 2015-12-23  3:12         ` Miao Yan
  0 siblings, 0 replies; 14+ messages in thread
From: Miao Yan @ 2015-12-23  3:12 UTC (permalink / raw)
  To: Jason Wang; +Cc: Dmitry Fleytman, QEMU

2015-12-23 10:15 GMT+08:00 Jason Wang <jasowang@redhat.com>:
>
>
> On 12/22/2015 03:05 PM, Dmitry Fleytman wrote:
>>
>>> On 22 Dec 2015, at 04:44 AM, Miao Yan <yanmiaobest@gmail.com
>>> <mailto:yanmiaobest@gmail.com>> wrote:
>>>
>>> Hi Dmitry,
>>>
>>> 2015-12-22 1:19 GMT+08:00 Dmitry Fleytman <dmitry@daynix.com
>>> <mailto:dmitry@daynix.com>>:
>>>> Hello Miao,
>>>>
>>>> While patches look good technically, I cannot find any code snippets
>>>> that prove correctness of these changes.
>>>
>>> Linux driver does not read those registers currently,
>>> not sure about Windows version.
>>>
>>>
>>>> How do you know this is the correct behaviour? Could you please
>>>> extend commit messages with corresponding references?
>>>
>>>
>>> This behavior can be observed by modifying linux driver to read those
>>> registers
>>> at probe time (in vmxnet3_device_probe) and kernel log will have the
>>> following
>>> message (running on esxi server, of source):
>>>
>>> [  198.427389] VMware vmxnet3 virtual NIC driver - version 1.2.0.0-k-NAPI
>>> [  198.428863] vmxnet3 0000:03:00.0: # of Tx queues : 1, # of Rx
>>> queues : 1
>>> [  198.559625] vmxnet3 DID lo: 0x7b0, high: 0x1, dev_info: 0x0
>>> [  198.561151] vmxnet3 0000:03:00.0: irq 72 for MSI/MSI-X
>>>
>>> Here DID_LO is the pci device id, DID_HIGH is 0x1 and
>>> GET_DEV_EXTRA_INFO returns 0.
>>>
>>> putting above into commit message should be enough ?
>>
>> Yes, this should be good enough. Please also provide ESX version
>> you’re running on.
>>
>> Reviewed-by: Dmitry Fleytman <dmitry@daynix.com
>> <mailto:dmitry@daynix.com>>
>>
>> Jason, these changes are guest visible but init time only. Do you
>> think compatibility code is needed in this case?
>
> I think it was probably no need to do compatibility things here,
> consider no device state but only register read changes and I believe no
> driver should depend on those values.

Yes, if they depend on those values, then things are already
broken as we give them -1 which is hardly any driver would
expect.

I will prepare v3 to address review comments.


>
>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Dmitry
>>>>
>>>>> On 21 Dec 2015, at 13:06 PM, Miao Yan <yanmiaobest@gmail.com
>>>>> <mailto:yanmiaobest@gmail.com>> wrote:
>>>>>
>>>>> Qemu vmxnet3 emulation doesn't recognize VMXNET3_CMD_GET_DID_LO,
>>>>> VMXNET3_CMD_GET_DID_HI and VMXNET3_CMD_GET_DEV_EXTRA_INFO command and
>>>>> returns -1 on all of them.
>>>>>
>>>>> This patchset makes them return correct values.
>>>>>
>>>>> Miao Yan (3):
>>>>> net/vmxnet3: return 1 on device activation failure
>>>>> net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID command
>>>>> net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO
>>>>>
>>>>> hw/net/vmxnet3.c | 11 ++++++++++-
>>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>
>>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-12-23  3:12 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-21 11:06 [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Miao Yan
2015-12-21 11:06 ` [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure Miao Yan
2015-12-21 18:15   ` P J P
2015-12-22  2:56     ` Miao Yan
2015-12-22  9:06       ` P J P
2015-12-22  9:26         ` Miao Yan
2015-12-22  9:33           ` Dmitry Fleytman
2015-12-21 11:06 ` [Qemu-devel] [PATCH 2/3] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command Miao Yan
2015-12-21 11:06 ` [Qemu-devel] [PATCH 3/3] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO Miao Yan
2015-12-21 17:19 ` [Qemu-devel] [PATCH 0/3] correct some register return values for vxmnet3 Dmitry Fleytman
2015-12-22  2:44   ` Miao Yan
2015-12-22  7:05     ` Dmitry Fleytman
2015-12-23  2:15       ` Jason Wang
2015-12-23  3:12         ` Miao Yan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).