Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v5 8/8] eni_vdpa: add vDPA driver for Alibaba ENI
To:     Wu Zongyong <wuzongyong@linux.alibaba.com>,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, mst@redhat.com
Cc:     wei.yang1@linux.alibaba.com
References: <cover.1632882380.git.wuzongyong@linux.alibaba.com>
 <cover.1634281805.git.wuzongyong@linux.alibaba.com>
 <57a04a9e516ec4055cb887e9c7b24658ca5b0228.1634281805.git.wuzongyong@linux.alibaba.com>
From:   Jason Wang <jasowang@redhat.com>
Message-ID: <71914be7-827a-b496-90cb-1c0b5f573c37@redhat.com>
Date:   Fri, 15 Oct 2021 16:18:25 +0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <57a04a9e516ec4055cb887e9c7b24658ca5b0228.1634281805.git.wuzongyong@linux.alibaba.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


?? 2021/10/15 ????3:15, Wu Zongyong д??:
> +
> +static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
> +{
> +	struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> +	u64 features = vp_legacy_get_features(ldev);
> +
> +	features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
> +	features |= BIT_ULL(VIRTIO_F_ORDER_PLATFORM);
> +
> +	return features;
> +}
> +
> +static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> +{
> +	struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> +
> +	if (!(features & BIT_ULL(VIRTIO_NET_F_MRG_RXBUF)) && features) {
> +		ENI_ERR(ldev->pci_dev,
> +			"VIRTIO_NET_F_MRG_RXBUF is not negotiated\n");
> +		return -EINVAL;
> +	}
> +
> +	vp_legacy_set_features(ldev, (u32)features);
> +
> +	return 0;
> +}


Hi:

It looks like some of my previous comments were ignored?

> +static u64 eni_vdpa_get_features(struct vdpa_device *vdpa)
> +{
> +	struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> +	u64 features = vp_legacy_get_features(ldev);
> +
> +	features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
> +	features |= BIT_ULL(VIRTIO_F_ORDER_PLATFORM);

VERSION_1 is also needed?


> +
> +	return features;
> +}
> +
> +static int eni_vdpa_set_features(struct vdpa_device *vdpa, u64 features)
> +{
> +	struct virtio_pci_legacy_device *ldev = vdpa_to_ldev(vdpa);
> +
> +	if (!(features & BIT_ULL(VIRTIO_NET_F_MRG_RXBUF)) && features) {
> +		ENI_ERR(ldev->pci_dev,
> +			"VIRTIO_NET_F_MRG_RXBUF is not negotiated\n");
> +		return -EINVAL;

Do we need to make sure FEATURE_OK is not set in this case or the ENI can do
this for us?

Other looks good.

Thanks