Messages in this thread | | | Subject | Re: [PATCH v2 12/17] vdpa_sim: add get_config callback in vdpasim_dev_attr | From | Jason Wang <> | Date | Tue, 1 Dec 2020 11:44:19 +0800 |
| |
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 >
| |