diff mbox series

[2/5] net/virtio: add DEVICE_NEEDS_RESET status bit

Message ID 20200715171828.95887-3-amorenoz@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series Add support for GET/SET_STATUS on virtio-user pmd |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Adrian Moreno July 15, 2020, 5:18 p.m. UTC 
  For the sake of completeness, add the definition of the missing status
bit in accordance with the virtio spec

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_pci.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Chenbo Xia July 16, 2020, 2:26 a.m. UTC | #1 
Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 1:18 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
> 
> For the sake of completeness, add the definition of the missing status bit in
> accordance with the virtio spec
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_pci.h | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h index
> 74ed77e33..ab61e911b 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -57,12 +57,13 @@ struct virtnet_ctl;
>  #define VIRTIO_ID_9P       0x09
> 
>  /* Status byte for guest to report progress. */
> -#define VIRTIO_CONFIG_STATUS_RESET     0x00
> -#define VIRTIO_CONFIG_STATUS_ACK       0x01
> -#define VIRTIO_CONFIG_STATUS_DRIVER    0x02
> -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 -#define
> VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08
> -#define VIRTIO_CONFIG_STATUS_FAILED    0x80
> +#define VIRTIO_CONFIG_STATUS_RESET		0x00
> +#define VIRTIO_CONFIG_STATUS_ACK		0x01
> +#define VIRTIO_CONFIG_STATUS_DRIVER		0x02
> +#define VIRTIO_CONFIG_STATUS_DRIVER_OK		0x04
> +#define VIRTIO_CONFIG_STATUS_FEATURES_OK	0x08
> +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET	0x40
> +#define VIRTIO_CONFIG_STATUS_FAILED		0x80

Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib does not
have it now. And I read virtio 1.1 spec and find the description of writing 0 to
reset device is deleted. I think virtio 1.1 spec is not very clear about reset status.
Does 'DEV_NEED_RESET' equal old 'RESET'?

Thanks!
Chenbo 

> 
>  /*
>   * Each virtqueue indirect descriptor list must be physically contiguous.
> --
> 2.26.2
Adrian Moreno July 16, 2020, 7:34 a.m. UTC | #2 
Hi,

On 7/16/20 4:26 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 1:18 AM
>> To: dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
>> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
>>
>> For the sake of completeness, add the definition of the missing status bit in
>> accordance with the virtio spec
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_pci.h | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h index
>> 74ed77e33..ab61e911b 100644
>> --- a/drivers/net/virtio/virtio_pci.h
>> +++ b/drivers/net/virtio/virtio_pci.h
>> @@ -57,12 +57,13 @@ struct virtnet_ctl;
>>  #define VIRTIO_ID_9P       0x09
>>
>>  /* Status byte for guest to report progress. */
>> -#define VIRTIO_CONFIG_STATUS_RESET     0x00
>> -#define VIRTIO_CONFIG_STATUS_ACK       0x01
>> -#define VIRTIO_CONFIG_STATUS_DRIVER    0x02
>> -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 -#define
>> VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08
>> -#define VIRTIO_CONFIG_STATUS_FAILED    0x80
>> +#define VIRTIO_CONFIG_STATUS_RESET		0x00
>> +#define VIRTIO_CONFIG_STATUS_ACK		0x01
>> +#define VIRTIO_CONFIG_STATUS_DRIVER		0x02
>> +#define VIRTIO_CONFIG_STATUS_DRIVER_OK		0x04
>> +#define VIRTIO_CONFIG_STATUS_FEATURES_OK	0x08
>> +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET	0x40
>> +#define VIRTIO_CONFIG_STATUS_FAILED		0x80
> 
> Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib does not
> have it now. And I read virtio 1.1 spec and find the description of writing 0 to
> reset device is deleted. I think virtio 1.1 spec is not very clear about reset status.
> Does 'DEV_NEED_RESET' equal old 'RESET'?
> 
In virtio 1.1
"2.1.2 Device Requirements: Device Status Field

The device MUST initialize device status to 0 upon reset.
...
"
So I think having "#define VIRTIO_CONFIG_STATUS_RESET 0x00" is still useful to
understand what is going on in:

void
vtpci_reset(struct virtio_hw *hw)
{
	VTPCI_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
	/* flush status write */
	VTPCI_OPS(hw)->get_status(hw);
}

DEV_NEED_RESET is used for the device to notify that it has encountered an
unrecoverable error. Therefore, the driver would never
"set_status(VIRTIO_CONFIG_STATUS_DEV_NEED_RESET)"; instead, it should read the
status and if this bit is set, reset the device.


> Thanks!
> Chenbo 
> 
>>
>>  /*
>>   * Each virtqueue indirect descriptor list must be physically contiguous.
>> --
>> 2.26.2
>
Chenbo Xia July 16, 2020, 8:14 a.m. UTC | #3 
Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 3:34 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> Li, Miao <miao.li@intel.com>
> Subject: Re: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
> 
> Hi,
> 
> On 7/16/20 4:26 AM, Xia, Chenbo wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Moreno <amorenoz@redhat.com>
> >> Sent: Thursday, July 16, 2020 1:18 AM
> >> To: dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
> >> <chenbo.xia@intel.com>
> >> Subject: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
> >>
> >> For the sake of completeness, add the definition of the missing
> >> status bit in accordance with the virtio spec
> >>
> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >> ---
> >>  drivers/net/virtio/virtio_pci.h | 13 +++++++------
> >>  1 file changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/virtio/virtio_pci.h
> >> b/drivers/net/virtio/virtio_pci.h index 74ed77e33..ab61e911b 100644
> >> --- a/drivers/net/virtio/virtio_pci.h
> >> +++ b/drivers/net/virtio/virtio_pci.h
> >> @@ -57,12 +57,13 @@ struct virtnet_ctl;
> >>  #define VIRTIO_ID_9P       0x09
> >>
> >>  /* Status byte for guest to report progress. */
> >> -#define VIRTIO_CONFIG_STATUS_RESET     0x00
> >> -#define VIRTIO_CONFIG_STATUS_ACK       0x01
> >> -#define VIRTIO_CONFIG_STATUS_DRIVER    0x02
> >> -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 -#define
> >> VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08
> >> -#define VIRTIO_CONFIG_STATUS_FAILED    0x80
> >> +#define VIRTIO_CONFIG_STATUS_RESET		0x00
> >> +#define VIRTIO_CONFIG_STATUS_ACK		0x01
> >> +#define VIRTIO_CONFIG_STATUS_DRIVER		0x02
> >> +#define VIRTIO_CONFIG_STATUS_DRIVER_OK		0x04
> >> +#define VIRTIO_CONFIG_STATUS_FEATURES_OK	0x08
> >> +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET	0x40
> >> +#define VIRTIO_CONFIG_STATUS_FAILED		0x80
> >
> > Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib
> > does not have it now. And I read virtio 1.1 spec and find the
> > description of writing 0 to reset device is deleted. I think virtio 1.1 spec is not
> very clear about reset status.
> > Does 'DEV_NEED_RESET' equal old 'RESET'?
> >
> In virtio 1.1
> "2.1.2 Device Requirements: Device Status Field
> 
> The device MUST initialize device status to 0 upon reset.
> ...
> "
> So I think having "#define VIRTIO_CONFIG_STATUS_RESET 0x00" is still useful to
> understand what is going on in:
> 
> void
> vtpci_reset(struct virtio_hw *hw)
> {
> 	VTPCI_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> 	/* flush status write */
> 	VTPCI_OPS(hw)->get_status(hw);
> }
> 
> DEV_NEED_RESET is used for the device to notify that it has encountered an
> unrecoverable error. Therefore, the driver would never
> "set_status(VIRTIO_CONFIG_STATUS_DEV_NEED_RESET)"; instead, it should
> read the status and if this bit is set, reset the device.

Yes, you are correct! I missed that part. Btw, should we add STATUS_RESET to
Vhost lib? I see there's no reset status now.

Thanks!
Chenbo

> 
> 
> > Thanks!
> > Chenbo
> >
> >>
> >>  /*
> >>   * Each virtqueue indirect descriptor list must be physically contiguous.
> >> --
> >> 2.26.2
> >
> 
> --
> Adrián Moreno
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 74ed77e33..ab61e911b 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -57,12 +57,13 @@  struct virtnet_ctl;
 #define VIRTIO_ID_9P       0x09
 
 /* Status byte for guest to report progress. */
-#define VIRTIO_CONFIG_STATUS_RESET     0x00
-#define VIRTIO_CONFIG_STATUS_ACK       0x01
-#define VIRTIO_CONFIG_STATUS_DRIVER    0x02
-#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04
-#define VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08
-#define VIRTIO_CONFIG_STATUS_FAILED    0x80
+#define VIRTIO_CONFIG_STATUS_RESET		0x00
+#define VIRTIO_CONFIG_STATUS_ACK		0x01
+#define VIRTIO_CONFIG_STATUS_DRIVER		0x02
+#define VIRTIO_CONFIG_STATUS_DRIVER_OK		0x04
+#define VIRTIO_CONFIG_STATUS_FEATURES_OK	0x08
+#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET	0x40
+#define VIRTIO_CONFIG_STATUS_FAILED		0x80
 
 /*
  * Each virtqueue indirect descriptor list must be physically contiguous.