Re: [Qemu-devel] [PATCH 1/2] virtio-input: use fixed device config space size

[Michael S. Tsirkin]

On Mon, Jun 15, 2015 at 02:53:08PM +0200, Gerd Hoffmann wrote:
> virtio-input doesn't support legacy, so config spice doesn't live in IO
> address space (on virtio-pci) and size is not something we have to worry
> about.  There is a full page (in mmio bar) available anyway.
> 
> Don't try to size it dynamically but simply use the full struct size
> (136 bytes) unconditionally.
> 
> Signed-off-by: Gerd Hoffmann <address@hidden>

I'm not sure this is a good idea.  Struct will grow with time as we add
features, and this will break compatibility. Size must depend on
host feature bits.

We really need to move the logic into core, too.

> ---
>  hw/input/virtio-input.c          | 14 +++++++-------
>  include/hw/virtio/virtio-input.h |  1 -
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
> index c4f4b3c..c5c39fa 100644
> --- a/hw/input/virtio-input.c
> +++ b/hw/input/virtio-input.c
> @@ -149,9 +149,9 @@ static void virtio_input_get_config(VirtIODevice *vdev, 
> uint8_t *config_data)
>      config = virtio_input_find_config(vinput, vinput->cfg_select,
>                                        vinput->cfg_subsel);
>      if (config) {
> -        memcpy(config_data, config, vinput->cfg_size);
> +        memcpy(config_data, config, sizeof(virtio_input_config));
>      } else {
> -        memset(config_data, 0, vinput->cfg_size);
> +        memset(config_data, 0, sizeof(virtio_input_config));
>      }
>  }
>  
> @@ -206,6 +206,7 @@ static void virtio_input_device_realize(DeviceState *dev, 
> Error **errp)
>      VirtIOInput *vinput = VIRTIO_INPUT(dev);
>      VirtIOInputConfig *cfg;
>      Error *local_err = NULL;
> +    uint32_t cfg_size = 0;
>  
>      if (vic->realize) {
>          vic->realize(dev, &local_err);
> @@ -219,15 +220,14 @@ static void virtio_input_device_realize(DeviceState 
> *dev, Error **errp)
>                                vinput->input.serial);
>  
>      QTAILQ_FOREACH(cfg, &vinput->cfg_list, node) {
> -        if (vinput->cfg_size < cfg->config.size) {
> -            vinput->cfg_size = cfg->config.size;
> +        if (cfg_size < cfg->config.size) {
> +            cfg_size = cfg->config.size;
>          }
>      }
> -    vinput->cfg_size += 8;
> -    assert(vinput->cfg_size <= sizeof(virtio_input_config));
> +    assert(cfg_size + 8 <= sizeof(virtio_input_config));
>  
>      virtio_init(vdev, "virtio-input", VIRTIO_ID_INPUT,
> -                vinput->cfg_size);
> +                sizeof(virtio_input_config));
>      vinput->evt = virtio_add_queue(vdev, 64, virtio_input_handle_evt);
>      vinput->sts = virtio_add_queue(vdev, 64, virtio_input_handle_sts);
>  }
> diff --git a/include/hw/virtio/virtio-input.h 
> b/include/hw/virtio/virtio-input.h
> index 8134178..7b46e62 100644
> --- a/include/hw/virtio/virtio-input.h
> +++ b/include/hw/virtio/virtio-input.h
> @@ -71,7 +71,6 @@ struct VirtIOInput {
>      VirtIODevice                      parent_obj;
>      uint8_t                           cfg_select;
>      uint8_t                           cfg_subsel;
> -    uint32_t                          cfg_size;
>      QTAILQ_HEAD(, VirtIOInputConfig)  cfg_list;
>      VirtQueue                         *evt, *sts;
>      virtio_input_conf                 input;
> -- 
> 1.8.3.1