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

[Jason Wang <jasowang@xxxxxxxxxx>]

On 2020/11/30 下午10:14, Stefano Garzarella wrote:
> 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.


I think in that way we need should provide config_write().


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


I prefer to do that in the device but it's also fine keep what the patch 
has done.

Thanks


> Thanks,
> Stefano

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization