[Qemu-devel] [PATCH v2 0/4] correct some register return values for vxmnet3

[Qemu-devel] [PATCH v2 0/4] correct some register return values for vxmnet3

[Miao Yan]

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.

Changes in v2:
 - return 0 on unknown command

Miao Yan (4):
  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
  net/vmxnet3: return 0 on unknown command

 hw/net/vmxnet3.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/4] net/vmxnet3: return 1 on device activation failure

[Miao Yan]

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

This behavior can be observed by the following steps:

1) run a Linux distro on esxi server
2) modify vmxnet3 Linux driver to give it an invalid
   address to 'adapter->shared_pa' which is the
   shared memory for guest/host communication

This will trigger device activation failure and kernel
log will have the following message:

   [ 7138.403256] vmxnet3 0000:03:00.0 eth1: Failed to activate dev: error 1

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

Re: [Qemu-devel] [PATCH v2 1/4] net/vmxnet3: return 1 on device activation failure

[Shmulik Ladkani]

Hi,

On Mon, 21 Dec 2015 22:18:21 -0800 Miao Yan <yanmiaobest@gmail.com> wrote:
> When reading device status, 0 means device is successfully
> activated and 1 means error.
> 
> This behavior can be observed by the following steps:
> 
> 1) run a Linux distro on esxi server
> 2) modify vmxnet3 Linux driver to give it an invalid
>    address to 'adapter->shared_pa' which is the
>    shared memory for guest/host communication
> 
> This will trigger device activation failure and kernel
> log will have the following message:
> 
>    [ 7138.403256] vmxnet3 0000:03:00.0 eth1: Failed to activate dev: error 1

Could it be that various other error codes might be returned from ESXi
for a VMXNET3_CMD_ACTIVATE_DEV read, depending on device condition?

Regards,
Shmulik

Re: [Qemu-devel] [PATCH v2 1/4] net/vmxnet3: return 1 on device activation failure

[Shmulik Ladkani]

On Tue, 22 Dec 2015 22:41:34 +0200 Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> Could it be that various other error codes might be returned from ESXi
> for a VMXNET3_CMD_ACTIVATE_DEV read, depending on device condition?

Thinking this over, the commit takes us one step into a finer device
implementation, and we can always fine-tune it in the future.

Reviewed-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>

[Qemu-devel] [PATCH v2 2/4] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command

[Miao Yan]

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

This behavior can be observed by the following steps:

1) run a Linux distro on esxi server
2) modify vmxnet3 Linux driver to read DID_HI and DID_LO:

  VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DID_LO);
  lo =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);

  VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DID_HI);
  high =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
  pr_info("vmxnet3 DID lo: 0x%x, high: 0x%x\n", lo, high);

The kernel log will have something like the following message:

  [ 7005.111170] vmxnet3 DID lo: 0x7b0, high: 0x1

Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
---
Changes in v2: 
 - update vmxnet3_handle_command not to print error when issuing these commands

 hw/net/vmxnet3.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 9185408..cddbf6d 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1640,6 +1640,14 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
                   "adaptive ring info flags");
         break;
 
+    case VMXNET3_CMD_GET_DID_LO:
+        VMW_CBPRN("Set: Get lower part of device ID");
+        break;
+
+    case VMXNET3_CMD_GET_DID_HI:
+        VMW_CBPRN("Set: Get upper part of device ID");
+        break;
+
     default:
         VMW_CBPRN("Received unknown command: %" PRIx64, cmd);
         break;
@@ -1683,6 +1691,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

Re: [Qemu-devel] [PATCH v2 2/4] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command

[Shmulik Ladkani]

Hi,

On Mon, 21 Dec 2015 22:18:22 -0800 Miao Yan <yanmiaobest@gmail.com> wrote:
> VMXNET3_CMD_GET_DID_LO should return PCI ID of the device
> and VMXNET3_CMD_GET_DID_HI should return vmxnet3 revision ID.
> 
> This behavior can be observed by the following steps:
> 
> 1) run a Linux distro on esxi server
> 2) modify vmxnet3 Linux driver to read DID_HI and DID_LO:
> 
>   VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DID_LO);
>   lo =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
> 
>   VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DID_HI);
>   high =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
>   pr_info("vmxnet3 DID lo: 0x%x, high: 0x%x\n", lo, high);
> 
> The kernel log will have something like the following message:
> 
>   [ 7005.111170] vmxnet3 DID lo: 0x7b0, high: 0x1

[...]

> +    case VMXNET3_CMD_GET_DID_HI:
> +        ret = VMXNET3_DEVICE_REVISION;
> +        break;
> +

Do we know whether VMXNET3_DEVICE_REVISION needs to be returned, or
should it be VMXNET3_DEVICE_VERSION instead?

I see both are currently defined as 1, but I assume this could
potentially be changed in the future.

How can we tell what's the right semantics for VMXNET3_CMD_GET_DID_HI?

Regards,
Shmulik

Re: [Qemu-devel] [PATCH v2 2/4] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command

[Miao Yan]

2015-12-23 4:49 GMT+08:00 Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>:
> Hi,
>
> On Mon, 21 Dec 2015 22:18:22 -0800 Miao Yan <yanmiaobest@gmail.com> wrote:
>> VMXNET3_CMD_GET_DID_LO should return PCI ID of the device
>> and VMXNET3_CMD_GET_DID_HI should return vmxnet3 revision ID.
>>
>> This behavior can be observed by the following steps:
>>
>> 1) run a Linux distro on esxi server
>> 2) modify vmxnet3 Linux driver to read DID_HI and DID_LO:
>>
>>   VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DID_LO);
>>   lo =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
>>
>>   VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DID_HI);
>>   high =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
>>   pr_info("vmxnet3 DID lo: 0x%x, high: 0x%x\n", lo, high);
>>
>> The kernel log will have something like the following message:
>>
>>   [ 7005.111170] vmxnet3 DID lo: 0x7b0, high: 0x1
>
> [...]
>
>> +    case VMXNET3_CMD_GET_DID_HI:
>> +        ret = VMXNET3_DEVICE_REVISION;
>> +        break;
>> +
>
> Do we know whether VMXNET3_DEVICE_REVISION needs to be returned, or
> should it be VMXNET3_DEVICE_VERSION instead?

VMXNET3_DEVICE_VERSION is only used as return value of reading
UPT version register, maybe it's better to rename it to
VMXNET3_UPT_REVERSION.

>
> I see both are currently defined as 1, but I assume this could
> potentially be changed in the future.
>
> How can we tell what's the right semantics for VMXNET3_CMD_GET_DID_HI?

By inspecting what esxi does. It always returns 1 for VMXNET3_CMD_GET_DID_HI.
So maybe we should stick it to 1.

>
> Regards,
> Shmulik

Re: [Qemu-devel] [PATCH v2 2/4] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command

[Shmulik Ladkani]

On Mon, 21 Dec 2015 22:18:22 -0800 Miao Yan <yanmiaobest@gmail.com> wrote:
> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>

Reviewed-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>

[Qemu-devel] [PATCH v2 3/4] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO

[Miao Yan]

VMXNET3_CMD_GET_DEV_EXTRA_INFO should return 0 for emulation
mode

This behavior can be observed by the following steps:

1) run a Linux distro on esxi server
2) modify vmxnet3 Linux driver to read the register:

  VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DEV_EXTRA_INFO);
  ret =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
  pr_info("vmxnet3 dev_info: 0x%x\n", ret);

The kernel log will have some like the following message:

  [ 7005.111170] vmxnet3 dev_info: 0x0

Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
---
Changes in v2: 
 - update vmxnet3_handle_command not to print error when issuing these commands

 hw/net/vmxnet3.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index cddbf6d..b8bc360 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1648,6 +1648,10 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd)
         VMW_CBPRN("Set: Get upper part of device ID");
         break;
 
+    case VMXNET3_CMD_GET_DEV_EXTRA_INFO:
+        VMW_CBPRN("Set: Get device extra info");
+        break;
+
     default:
         VMW_CBPRN("Received unknown command: %" PRIx64, cmd);
         break;
@@ -1667,6 +1671,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

Re: [Qemu-devel] [PATCH v2 3/4] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO

[Shmulik Ladkani]

Hi,

On Mon, 21 Dec 2015 22:18:23 -0800 Miao Yan <yanmiaobest@gmail.com> wrote:
> VMXNET3_CMD_GET_DEV_EXTRA_INFO should return 0 for emulation
> mode
> 
> This behavior can be observed by the following steps:
> 
> 1) run a Linux distro on esxi server
> 2) modify vmxnet3 Linux driver to read the register:
> 
>   VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DEV_EXTRA_INFO);
>   ret =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
>   pr_info("vmxnet3 dev_info: 0x%x\n", ret);
> 
> The kernel log will have some like the following message:
> 
>   [ 7005.111170] vmxnet3 dev_info: 0x0
> 
> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>

Reviewed-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>

[Qemu-devel] [PATCH v2 4/4] net/vmxnet3: return 0 on unknown command

[Miao Yan]

Return 0 on unknown command, this is what esxi behaves.

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 b8bc360..a429405 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1706,7 +1706,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s)
 
     default:
         VMW_WRPRN("Received request for unknown command: %x", s->last_command);
-        ret = -1;
+        ret = 0;
         break;
     }
 
-- 
1.9.1