diff mbox series

[11/40] net/virtio: validate features at bus level

Message ID 20201220211405.313012-12-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: Virtio PMD rework |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin Dec. 20, 2020, 9:13 p.m. UTC 
  This patch provides a new callback for the bus type
to validate negotiated features are compatible with it.

Only user for now is PCI modern bus type, which implies
that the device supports Virtio 1.0+.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c      | 11 +++++------
 drivers/net/virtio/virtio_pci.c         | 19 +++++++++++++++++++
 drivers/net/virtio/virtio_pci.h         |  1 +
 drivers/net/virtio/virtio_user_ethdev.c |  7 +++++++
 4 files changed, 32 insertions(+), 6 deletions(-)

Comments

Chenbo Xia Dec. 30, 2020, 3:08 a.m. UTC | #1 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, December 21, 2020 5:14 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
> amorenoz@redhat.com; david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 11/40] net/virtio: validate features at bus level
> 
> This patch provides a new callback for the bus type
> to validate negotiated features are compatible with it.
> 
> Only user for now is PCI modern bus type, which implies
> that the device supports Virtio 1.0+.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c      | 11 +++++------
>  drivers/net/virtio/virtio_pci.c         | 19 +++++++++++++++++++
>  drivers/net/virtio/virtio_pci.h         |  1 +
>  drivers/net/virtio/virtio_user_ethdev.c |  7 +++++++
>  4 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 00aa38e4ef..91a93b2b6e 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1315,17 +1315,16 @@ virtio_negotiate_features(struct virtio_hw *hw,
> uint64_t req_features)
>  	PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
>  		hw->guest_features);
> 
> -	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN && !vtpci_with_feature(hw,
> VIRTIO_F_VERSION_1)) {
> -		PMD_INIT_LOG(ERR,
> -			"VIRTIO_F_VERSION_1 features is not enabled.");
> +	if (VTPCI_OPS(hw)->features_ok(hw) < 0) {
> +		PMD_INIT_LOG(ERR, "Features not OK at bus level\n");
>  		return -1;
>  	}
> 
> -	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type ==
> VIRTIO_BUS_USER) {
> +	if (vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
>  		vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> +
>  		if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) {
> -			PMD_INIT_LOG(ERR,
> -				"failed to set FEATURES_OK status!");
> +			PMD_INIT_LOG(ERR, "Failed to set FEATURES_OK status!");
>  			return -1;
>  		}
>  	}
> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> index 599d8afa6b..3de7980b4f 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c
> @@ -151,6 +151,12 @@ legacy_set_features(struct virtio_hw *hw, uint64_t
> features)
>  		VIRTIO_PCI_GUEST_FEATURES);
>  }
> 
> +static int
> +legacy_features_ok(struct virtio_hw *hw __rte_unused)
> +{
> +	return 0;
> +}
> +
>  static uint8_t
>  legacy_get_status(struct virtio_hw *hw)
>  {
> @@ -259,6 +265,7 @@ const struct virtio_pci_ops legacy_ops = {
>  	.set_status	= legacy_set_status,
>  	.get_features	= legacy_get_features,
>  	.set_features	= legacy_set_features,
> +	.features_ok	= legacy_features_ok,
>  	.get_isr	= legacy_get_isr,
>  	.set_config_irq	= legacy_set_config_irq,
>  	.set_queue_irq  = legacy_set_queue_irq,
> @@ -332,6 +339,17 @@ modern_set_features(struct virtio_hw *hw, uint64_t
> features)
>  		    &hw->common_cfg->guest_feature);
>  }
> 
> +static int
> +modern_features_ok(struct virtio_hw *hw)
> +{
> +	if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
> +		PMD_INIT_LOG(ERR, "Version 1+ required with modern devices\n");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static uint8_t
>  modern_get_status(struct virtio_hw *hw)
>  {
> @@ -475,6 +493,7 @@ const struct virtio_pci_ops modern_ops = {
>  	.set_status	= modern_set_status,
>  	.get_features	= modern_get_features,
>  	.set_features	= modern_set_features,
> +	.features_ok	= modern_features_ok,
>  	.get_isr	= modern_get_isr,
>  	.set_config_irq	= modern_set_config_irq,
>  	.set_queue_irq  = modern_set_queue_irq,
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index 4f7d0e479e..22c21e6896 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -227,6 +227,7 @@ struct virtio_pci_ops {
> 
>  	uint64_t (*get_features)(struct virtio_hw *hw);
>  	void     (*set_features)(struct virtio_hw *hw, uint64_t features);
> +	int      (*features_ok)(struct virtio_hw *hw);
> 
>  	uint8_t (*get_isr)(struct virtio_hw *hw);
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index f9a2dbae71..c93e0e43f5 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -327,6 +327,12 @@ virtio_user_set_features(struct virtio_hw *hw, uint64_t
> features)
>  	dev->features = features & dev->device_features;
>  }
> 
> +static int
> +virtio_user_features_ok(struct virtio_hw *hw __rte_unused)
> +{
> +	return 0;
> +}
> +
>  static uint8_t
>  virtio_user_get_isr(struct virtio_hw *hw __rte_unused)
>  {
> @@ -479,6 +485,7 @@ const struct virtio_pci_ops virtio_user_ops = {
>  	.set_status	= virtio_user_set_status,
>  	.get_features	= virtio_user_get_features,
>  	.set_features	= virtio_user_set_features,
> +	.features_ok	= virtio_user_features_ok,
>  	.get_isr	= virtio_user_get_isr,
>  	.set_config_irq	= virtio_user_set_config_irq,
>  	.set_queue_irq	= virtio_user_set_queue_irq,
> --
> 2.29.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
David Marchand Jan. 6, 2021, 9:33 a.m. UTC | #2 
On Sun, Dec 20, 2020 at 10:15 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 00aa38e4ef..91a93b2b6e 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1315,17 +1315,16 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
>         PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
>                 hw->guest_features);
>
> -       if (hw->bus_type == VIRTIO_BUS_PCI_MODERN && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
> -               PMD_INIT_LOG(ERR,
> -                       "VIRTIO_F_VERSION_1 features is not enabled.");
> +       if (VTPCI_OPS(hw)->features_ok(hw) < 0) {
> +               PMD_INIT_LOG(ERR, "Features not OK at bus level\n");

We have a log which gives more context in the modern features_ok() callback.
I don't think we need both log messages.

>                 return -1;
>         }
>
> -       if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_USER) {
> +       if (vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
>                 vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> +
>                 if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) {
> -                       PMD_INIT_LOG(ERR,
> -                               "failed to set FEATURES_OK status!");
> +                       PMD_INIT_LOG(ERR, "Failed to set FEATURES_OK status!");
>                         return -1;
>                 }
>         }
> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> index 599d8afa6b..3de7980b4f 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c

[snip]


> @@ -332,6 +339,17 @@ modern_set_features(struct virtio_hw *hw, uint64_t features)
>                     &hw->common_cfg->guest_feature);
>  }
>
> +static int
> +modern_features_ok(struct virtio_hw *hw)
> +{
> +       if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
> +               PMD_INIT_LOG(ERR, "Version 1+ required with modern devices\n");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
>  static uint8_t
>  modern_get_status(struct virtio_hw *hw)
>  {
Maxime Coquelin Jan. 15, 2021, 9:20 a.m. UTC | #3 
On 1/6/21 10:33 AM, David Marchand wrote:
> On Sun, Dec 20, 2020 at 10:15 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 00aa38e4ef..91a93b2b6e 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1315,17 +1315,16 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
>>         PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
>>                 hw->guest_features);
>>
>> -       if (hw->bus_type == VIRTIO_BUS_PCI_MODERN && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
>> -               PMD_INIT_LOG(ERR,
>> -                       "VIRTIO_F_VERSION_1 features is not enabled.");
>> +       if (VTPCI_OPS(hw)->features_ok(hw) < 0) {
>> +               PMD_INIT_LOG(ERR, "Features not OK at bus level\n");
> 
> We have a log which gives more context in the modern features_ok() callback.
> I don't think we need both log messages.

Yes, maybe overkill. I will remove.

Thanks,
Maxime

>>                 return -1;
>>         }
>>
>> -       if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_USER) {
>> +       if (vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
>>                 vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
>> +
>>                 if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) {
>> -                       PMD_INIT_LOG(ERR,
>> -                               "failed to set FEATURES_OK status!");
>> +                       PMD_INIT_LOG(ERR, "Failed to set FEATURES_OK status!");
>>                         return -1;
>>                 }
>>         }
>> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
>> index 599d8afa6b..3de7980b4f 100644
>> --- a/drivers/net/virtio/virtio_pci.c
>> +++ b/drivers/net/virtio/virtio_pci.c
> 
> [snip]
> 
> 
>> @@ -332,6 +339,17 @@ modern_set_features(struct virtio_hw *hw, uint64_t features)
>>                     &hw->common_cfg->guest_feature);
>>  }
>>
>> +static int
>> +modern_features_ok(struct virtio_hw *hw)
>> +{
>> +       if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
>> +               PMD_INIT_LOG(ERR, "Version 1+ required with modern devices\n");
>> +               return -1;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static uint8_t
>>  modern_get_status(struct virtio_hw *hw)
>>  {
> 
>
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 00aa38e4ef..91a93b2b6e 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1315,17 +1315,16 @@  virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
 	PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
 		hw->guest_features);
 
-	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
-		PMD_INIT_LOG(ERR,
-			"VIRTIO_F_VERSION_1 features is not enabled.");
+	if (VTPCI_OPS(hw)->features_ok(hw) < 0) {
+		PMD_INIT_LOG(ERR, "Features not OK at bus level\n");
 		return -1;
 	}
 
-	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_USER) {
+	if (vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
 		vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
+
 		if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) {
-			PMD_INIT_LOG(ERR,
-				"failed to set FEATURES_OK status!");
+			PMD_INIT_LOG(ERR, "Failed to set FEATURES_OK status!");
 			return -1;
 		}
 	}
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 599d8afa6b..3de7980b4f 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -151,6 +151,12 @@  legacy_set_features(struct virtio_hw *hw, uint64_t features)
 		VIRTIO_PCI_GUEST_FEATURES);
 }
 
+static int
+legacy_features_ok(struct virtio_hw *hw __rte_unused)
+{
+	return 0;
+}
+
 static uint8_t
 legacy_get_status(struct virtio_hw *hw)
 {
@@ -259,6 +265,7 @@  const struct virtio_pci_ops legacy_ops = {
 	.set_status	= legacy_set_status,
 	.get_features	= legacy_get_features,
 	.set_features	= legacy_set_features,
+	.features_ok	= legacy_features_ok,
 	.get_isr	= legacy_get_isr,
 	.set_config_irq	= legacy_set_config_irq,
 	.set_queue_irq  = legacy_set_queue_irq,
@@ -332,6 +339,17 @@  modern_set_features(struct virtio_hw *hw, uint64_t features)
 		    &hw->common_cfg->guest_feature);
 }
 
+static int
+modern_features_ok(struct virtio_hw *hw)
+{
+	if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
+		PMD_INIT_LOG(ERR, "Version 1+ required with modern devices\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 static uint8_t
 modern_get_status(struct virtio_hw *hw)
 {
@@ -475,6 +493,7 @@  const struct virtio_pci_ops modern_ops = {
 	.set_status	= modern_set_status,
 	.get_features	= modern_get_features,
 	.set_features	= modern_set_features,
+	.features_ok	= modern_features_ok,
 	.get_isr	= modern_get_isr,
 	.set_config_irq	= modern_set_config_irq,
 	.set_queue_irq  = modern_set_queue_irq,
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 4f7d0e479e..22c21e6896 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -227,6 +227,7 @@  struct virtio_pci_ops {
 
 	uint64_t (*get_features)(struct virtio_hw *hw);
 	void     (*set_features)(struct virtio_hw *hw, uint64_t features);
+	int      (*features_ok)(struct virtio_hw *hw);
 
 	uint8_t (*get_isr)(struct virtio_hw *hw);
 
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index f9a2dbae71..c93e0e43f5 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -327,6 +327,12 @@  virtio_user_set_features(struct virtio_hw *hw, uint64_t features)
 	dev->features = features & dev->device_features;
 }
 
+static int
+virtio_user_features_ok(struct virtio_hw *hw __rte_unused)
+{
+	return 0;
+}
+
 static uint8_t
 virtio_user_get_isr(struct virtio_hw *hw __rte_unused)
 {
@@ -479,6 +485,7 @@  const struct virtio_pci_ops virtio_user_ops = {
 	.set_status	= virtio_user_set_status,
 	.get_features	= virtio_user_get_features,
 	.set_features	= virtio_user_set_features,
+	.features_ok	= virtio_user_features_ok,
 	.get_isr	= virtio_user_get_isr,
 	.set_config_irq	= virtio_user_set_config_irq,
 	.set_queue_irq	= virtio_user_set_queue_irq,