Jason Wang
2020-Dec-01 03:44 UTC
[PATCH v2 12/17] vdpa_sim: add get_config callback in vdpasim_dev_attr
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 at 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 >
Stefano Garzarella
2020-Dec-01 10:52 UTC
[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 at 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