diff mbox

fw_cfg specification ?

Message ID 20150312161631.GX1832@HEDWIG.INI.CMU.EDU
State New
Headers show

Commit Message

Gabriel L. Somlo March 12, 2015, 4:16 p.m. UTC
On Thu, Mar 12, 2015 at 04:42:25PM +0100, Laszlo Ersek wrote:
> On 03/12/15 15:17, Paolo Bonzini wrote:
> > Let's nuke it. :)
> 
> Okay... Gabriel, do you want to include such a patch in your upcoming
> series, or should I do the nuking sooner?

I was working on it (good for practice :) and so far I have the patch
below.

Still trying to understand how to update .valid.accepts in the various
mem_ops structures, but I think I'm on the right track. Will send a
proper patch once I wrap my head around that :)

Thanks,
--Gabriel

Comments

Paolo Bonzini March 12, 2015, 4:35 p.m. UTC | #1
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,
> 
>
diff mbox

Patch

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 78a37be..0a9eaf6 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -46,7 +46,6 @@  typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
     void *callback_opaque;
-    FWCfgCallback callback;
     FWCfgReadCallback read_callback;
 } FWCfgEntry;
 
@@ -230,23 +229,6 @@  static void fw_cfg_reboot(FWCfgState *s)
     fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4);
 }
 
-static void fw_cfg_write(FWCfgState *s, uint8_t value)
-{
-    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
-
-    trace_fw_cfg_write(s, value);
-
-    if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback &&
-        s->cur_offset < e->len) {
-        e->data[s->cur_offset++] = value;
-        if (s->cur_offset == e->len) {
-            e->callback(e->callback_opaque, e->data);
-            s->cur_offset = 0;
-        }
-    }
-}
-
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
 {
     int ret;
@@ -299,17 +281,13 @@  static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
 static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
                                   uint64_t value, unsigned size)
 {
-    FWCfgState *s = opaque;
-    unsigned i = size;
-
-    do {
-        fw_cfg_write(s, value >> (8 * --i));
-    } while (i);
+//FIXME: unused, remove from fw_cfg_data_mem_ops
 }
 
 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() ?
     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);
 }
 
@@ -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()
     .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,