Re: [Qemu-devel] fw_cfg specification ?

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/03/2015 17:16, Gabriel L. Somlo wrote:

>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> +//FIXME: additional checks before we nuke fw_cfg_data_mem_write() ?

yes, "&& !is_write".  Or just leave an empty fw_cfg_data_mem_write.

>      return addr == 0;
>  }
>  
> @@ -336,7 +314,7 @@ static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>  {
>      switch (size) {
>      case 1:
> -        fw_cfg_write(opaque, (uint8_t)value);
> +        //FIXME: unused, fix fw_cfg_comb_mem_ops then remove case
>          break;
>      case 2:
>          fw_cfg_select(opaque, (uint16_t)value);
> @@ -347,6 +325,7 @@ static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>  static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> +//FIXME: update checks before removing size==1 fw_cfg_comb_write() case
>      return (size == 1) || (is_write && size == 2);

Same here: "size == 1 && !is_write", or just leave an empty "case" after
removing the call to fw_cfg_write.

>  }
>  
> @@ -358,6 +337,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
>  
>  static const MemoryRegionOps fw_cfg_data_mem_ops = {
>      .read = fw_cfg_data_mem_read,
> +//FIXME: nuke this after updating fw_cfg_data_mem_valid()

If you want, but it's not necessary.

Paolo

>      .write = fw_cfg_data_mem_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid = {
> @@ -458,7 +438,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = len;
>      s->entries[arch][key].callback_opaque = NULL;
> -    s->entries[arch][key].callback = NULL;
>  
>      return ptr;
>  }
> @@ -502,23 +481,6 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> -void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
> -                         void *callback_opaque, void *data, size_t len)
> -{
> -    int arch = !!(key & FW_CFG_ARCH_LOCAL);
> -
> -    assert(key & FW_CFG_WRITE_CHANNEL);
> -
> -    key &= FW_CFG_ENTRY_MASK;
> -
> -    assert(key < FW_CFG_MAX_ENTRY && len <= UINT32_MAX);
> -
> -    s->entries[arch][key].data = data;
> -    s->entries[arch][key].len = (uint32_t)len;
> -    s->entries[arch][key].callback_opaque = callback_opaque;
> -    s->entries[arch][key].callback = callback;
> -}
> -
>  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>                                FWCfgReadCallback callback, void *callback_opaque,
>                                void *data, size_t len)
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 6d8a8ac..b2e10c2 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -69,8 +69,6 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
>  void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
>  void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
>  void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
> -void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
> -                         void *callback_opaque, void *data, size_t len);
>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>                       size_t len);
>  void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
> 
>