Re: [PATCH v2 12/17] vdpa_sim: add get_config callback in vdpasim_dev_attr

[Stefano Garzarella]

On Mon, Nov 30, 2020 at 11:25:31AM +0800, Jason Wang wrote:
> On 2020/11/26 下午10:49, 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@xxxxxxxxxx>
> > ---
> >  drivers/vdpa/vdpa_sim/vdpa_sim.c | 33 +++++++++++++++++++-------------
> >  1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > index c07ddf6af720..8b87ce0485b6 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > @@ -58,6 +58,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;
> > @@ -65,6 +67,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 */
> > @@ -520,8 +523,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)))
> > @@ -529,15 +530,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);
> >  	return 0;
> >  }
> > @@ -593,8 +585,12 @@ 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;
> > +
> > +	vdpasim->dev_attr.get_config(vdpasim, vdpasim->config);
> > +
> > +	memcpy(buf, vdpasim->config + offset, len);
> >  }
>
>
> I wonder how much value we can get from vdpasim->config consider we've 
> already had get_config() method.
>
> Is it possible to copy to the buffer directly here?

I had thought of eliminating it too, but then I wanted to do something 
similar to what we do in QEMU (hw/virtio/virtio.c), leaving in the 
simulator core the buffer, the memory copy (handling offset and len), 
and the boundary checks.

In this way each device should simply fill the entire configuration and 
we can avoid code duplication.

Storing the configuration in the core may also be useful if some device 
needs to support config writes.

Do you think it makes sense, or is it better to move everything in the 
device?

Thanks,
Stefano