lkml.org 
[lkml]   [2020]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 12/17] vdpa_sim: add get_config callback in vdpasim_dev_attr
    From
    Date

    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@redhat.com>
    >>> ---
    >>>  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
    >

    \
     
     \ /
      Last update: 2020-12-01 04:48    [W:4.438 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site