Re: [PATCH v3 13/19] vdpa_sim: add get_config callback in vdpasim_dev_attr

From: Stefano Garzarella <sgarzare@redhat.com>
To: Jason Wang <jasowang@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Oren Duer <oren@nvidia.com>, Laurent Vivier <lvivier@redhat.com>,
	linux-kernel@vger.kernel.org, Max Gurtovoy <mgurtovoy@nvidia.com>,
	Shahaf Shuler <shahafs@nvidia.com>, Eli Cohen <elic@nvidia.com>
Subject: Re: [PATCH v3 13/19] vdpa_sim: add get_config callback in vdpasim_dev_attr
Date: Wed, 9 Dec 2020 12:07:45 +0100	[thread overview]
Message-ID: <20201209110745.p4ybybanzip2lav3@steredhat> (raw)
In-Reply-To: <829a5026-a68c-6d02-49ef-f237dcae2460@redhat.com>

On Mon, Dec 07, 2020 at 01:29:17PM +0800, Jason Wang wrote:
>
>On 2020/12/4 上午1:05, Stefano Garzarella wrote:
>>The get_config callback can be used by the device to fill the
>>config structure.
>>The callback will be invoked in vdpasim_get_config() before copying
>>bytes into caller buffer.
>>
>>Move vDPA-net config updates from vdpasim_set_features() in the
>>new vdpasim_net_get_config() callback.
>>
>>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>---
>>v3:
>>- checked if get_config callback is set before call it
>>---
>>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 35 +++++++++++++++++++-------------
>>  1 file changed, 21 insertions(+), 14 deletions(-)
>>
>>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>index fe71ed7890e1..f935ade0806b 100644
>>--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>@@ -60,6 +60,8 @@ struct vdpasim_virtqueue {
>>  #define VDPASIM_NET_FEATURES	(VDPASIM_FEATURES | \
>>  				 (1ULL << VIRTIO_NET_F_MAC))
>>+struct vdpasim;
>>+
>>  struct vdpasim_dev_attr {
>>  	u64 supported_features;
>>  	size_t config_size;
>>@@ -67,6 +69,7 @@ struct vdpasim_dev_attr {
>>  	u32 id;
>>  	work_func_t work_fn;
>>+	void (*get_config)(struct vdpasim *vdpasim, void *config);
>>  };
>>  /* State of each vdpasim device */
>>@@ -522,8 +525,6 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa)
>>  static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
>>  {
>>  	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>-	struct virtio_net_config *config =
>>-		(struct virtio_net_config *)vdpasim->config;
>>  	/* DMA mapping must be done by driver */
>>  	if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
>>@@ -531,16 +532,6 @@ static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
>>  	vdpasim->features = features & vdpasim->dev_attr.supported_features;
>>-	/* We generally only know whether guest is using the legacy interface
>>-	 * here, so generally that's the earliest we can set config fields.
>>-	 * Note: We actually require VIRTIO_F_ACCESS_PLATFORM above which
>>-	 * implies VIRTIO_F_VERSION_1, but let's not try to be clever here.
>>-	 */
>>-
>>-	config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
>>-	config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
>>-	memcpy(config->mac, macaddr_buf, ETH_ALEN);
>
>
>Patch looks good to me.
>
>But we need Michael to confirm whether doing moving like this is safe. 
>I guess what has been done were trying to make sure get_config() fail 
>before set_features(), but it's not clear to me whether it's useful.

IIUC, also looking the QEMU code, the set_features() should be called 
every time before get_config(), but to be sure, in get_config(), I can 
check for example if 'vdpasim->features' is not zero (we require 
VIRTIO_F_ACCESS_PLATFORM set).

@Michael any suggestion?

Thanks,
Stefano

>
>Thanks
>
>
>>-
>>  	return 0;
>>  }
>>@@ -595,8 +586,13 @@ static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset,
>>  {
>>  	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>>-	if (offset + len < vdpasim->dev_attr.config_size)
>>-		memcpy(buf, vdpasim->config + offset, len);
>>+	if (offset + len > vdpasim->dev_attr.config_size)
>>+		return;
>>+
>>+	if (vdpasim->dev_attr.get_config)
>>+		vdpasim->dev_attr.get_config(vdpasim, vdpasim->config);
>>+
>>+	memcpy(buf, vdpasim->config + offset, len);
>>  }
>>  static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset,
>>@@ -739,6 +735,16 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
>>  	.free                   = vdpasim_free,
>>  };
>>+static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
>>+{
>>+	struct virtio_net_config *net_config =
>>+		(struct virtio_net_config *)config;
>>+
>>+	net_config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
>>+	net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
>>+	memcpy(net_config->mac, macaddr_buf, ETH_ALEN);
>>+}
>>+
>>  static int __init vdpasim_dev_init(void)
>>  {
>>  	struct vdpasim_dev_attr dev_attr = {};
>>@@ -747,6 +753,7 @@ static int __init vdpasim_dev_init(void)
>>  	dev_attr.supported_features = VDPASIM_NET_FEATURES;
>>  	dev_attr.nvqs = VDPASIM_VQ_NUM;
>>  	dev_attr.config_size = sizeof(struct virtio_net_config);
>>+	dev_attr.get_config = vdpasim_net_get_config;
>>  	dev_attr.work_fn = vdpasim_net_work;
>>  	vdpasim_dev = vdpasim_create(&dev_attr);
>