Messages in this thread | | | Date | Tue, 1 Dec 2020 11:52:50 +0100 | From | Stefano Garzarella <> | Subject | Re: [PATCH v2 12/17] vdpa_sim: add get_config callback in vdpasim_dev_attr |
| |
On Tue, Dec 01, 2020 at 11:44:19AM +0800, Jason Wang wrote: > >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().
Yes, I'll add set_config() callback.
> > >> >>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.
Okay, for now I'll keep it and add the set_config() callback, but I'm open to move it in the device.
Thanks, Stefano
| |