Re: [dpdk-dev] [PATCH 11/40] net/virtio: validate features at bus level

["Xia, Chenbo" <chenbo.xia@intel.com>]

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