Patchwork [1/6] Make fw_cfg interface 32-bit aware

login
register
mail settings
Submitter Alexander Graf
Date Nov. 12, 2009, 8:53 p.m.
Message ID <1258059195-17582-2-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/38292/
State New
Headers show

Comments

Alexander Graf - Nov. 12, 2009, 8:53 p.m.
The fw_cfg interface can only handle up to 16 bits of data for its streams.
While that isn't too much of a problem when handling integers, we would
like to stream full kernel images over that interface!

So let's extend it to 32 bit length variables.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - add savevm compat code (untested!)
---
 hw/fw_cfg.c |   30 ++++++++++++++++++++++++------
 hw/fw_cfg.h |    2 +-
 2 files changed, 25 insertions(+), 7 deletions(-)
Glauber Costa - Nov. 13, 2009, 12:48 a.m.
On Thu, Nov 12, 2009 at 09:53:10PM +0100, Alexander Graf wrote:
> The fw_cfg interface can only handle up to 16 bits of data for its streams.
> While that isn't too much of a problem when handling integers, we would
> like to stream full kernel images over that interface!
> 
> So let's extend it to 32 bit length variables.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> v1 -> v2:
> 
>   - add savevm compat code (untested!)
> ---
>  hw/fw_cfg.c |   30 ++++++++++++++++++++++++------
>  hw/fw_cfg.h |    2 +-
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index a6d811b..0cd6f68 100644
> --- a/hw/fw_cfg.c
> +++ b/hw/fw_cfg.c
> @@ -39,7 +39,7 @@
>  #define FW_CFG_SIZE 2
>  
>  typedef struct _FWCfgEntry {
> -    uint16_t len;
> +    uint32_t len;
>      uint8_t *data;
>      void *callback_opaque;
>      FWCfgCallback callback;
> @@ -48,7 +48,7 @@ typedef struct _FWCfgEntry {
>  typedef struct _FWCfgState {
>      FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
>      uint16_t cur_entry;
> -    uint16_t cur_offset;
> +    uint32_t cur_offset;
>  } FWCfgState;
>  
>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
> @@ -164,19 +164,37 @@ static void fw_cfg_reset(void *opaque)
>      fw_cfg_select(s, 0);
>  }
>  
> +static int fw_cfg_load_old(QEMUFile *f, void *opaque, int version_id)
> +{
> +    FWCfgState *s = opaque;
> +    uint16_t cur_offset;
> +
> +    if (version_id != 1)
> +        return -EINVAL;
> +
> +    qemu_get_be16s(f, &s->cur_entry);
> +
> +    /* Convert old 16 bit value to new 32 bit width */
> +    qemu_get_be16s(f, &cur_offset);
> +    s->cur_offset = cur_offset;
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_fw_cfg = {
>      .name = "fw_cfg",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .minimum_version_id_old = 1,
> +    .load_state_old = fw_cfg_load_old,
>      .fields      = (VMStateField []) {
>          VMSTATE_UINT16(cur_entry, FWCfgState),
> -        VMSTATE_UINT16(cur_offset, FWCfgState),
> +        VMSTATE_UINT32(cur_offset, FWCfgState),
>          VMSTATE_END_OF_LIST()
>      }
>  };

Why don't we just add another field for the upper bits, and add it through
VMSTATE_UINT16_V ?
Alexander Graf - Nov. 13, 2009, 6:15 a.m.
On 13.11.2009, at 01:48, Glauber Costa wrote:

> On Thu, Nov 12, 2009 at 09:53:10PM +0100, Alexander Graf wrote:
>> The fw_cfg interface can only handle up to 16 bits of data for its  
>> streams.
>> While that isn't too much of a problem when handling integers, we  
>> would
>> like to stream full kernel images over that interface!
>>
>> So let's extend it to 32 bit length variables.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> ---
>>
>> v1 -> v2:
>>
>>  - add savevm compat code (untested!)
>> ---
>> hw/fw_cfg.c |   30 ++++++++++++++++++++++++------
>> hw/fw_cfg.h |    2 +-
>> 2 files changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>> index a6d811b..0cd6f68 100644
>> --- a/hw/fw_cfg.c
>> +++ b/hw/fw_cfg.c
>> @@ -39,7 +39,7 @@
>> #define FW_CFG_SIZE 2
>>
>> typedef struct _FWCfgEntry {
>> -    uint16_t len;
>> +    uint32_t len;
>>     uint8_t *data;
>>     void *callback_opaque;
>>     FWCfgCallback callback;
>> @@ -48,7 +48,7 @@ typedef struct _FWCfgEntry {
>> typedef struct _FWCfgState {
>>     FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
>>     uint16_t cur_entry;
>> -    uint16_t cur_offset;
>> +    uint32_t cur_offset;
>> } FWCfgState;
>>
>> static void fw_cfg_write(FWCfgState *s, uint8_t value)
>> @@ -164,19 +164,37 @@ static void fw_cfg_reset(void *opaque)
>>     fw_cfg_select(s, 0);
>> }
>>
>> +static int fw_cfg_load_old(QEMUFile *f, void *opaque, int  
>> version_id)
>> +{
>> +    FWCfgState *s = opaque;
>> +    uint16_t cur_offset;
>> +
>> +    if (version_id != 1)
>> +        return -EINVAL;
>> +
>> +    qemu_get_be16s(f, &s->cur_entry);
>> +
>> +    /* Convert old 16 bit value to new 32 bit width */
>> +    qemu_get_be16s(f, &cur_offset);
>> +    s->cur_offset = cur_offset;
>> +
>> +    return 0;
>> +}
>> +
>> static const VMStateDescription vmstate_fw_cfg = {
>>     .name = "fw_cfg",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>     .minimum_version_id_old = 1,
>> +    .load_state_old = fw_cfg_load_old,
>>     .fields      = (VMStateField []) {
>>         VMSTATE_UINT16(cur_entry, FWCfgState),
>> -        VMSTATE_UINT16(cur_offset, FWCfgState),
>> +        VMSTATE_UINT32(cur_offset, FWCfgState),
>>         VMSTATE_END_OF_LIST()
>>     }
>> };
>
> Why don't we just add another field for the upper bits, and add it  
> through
> VMSTATE_UINT16_V ?

Because that would mean I'd have to deal with it in the code later on  
and I don't see the point of writing code that's not in the load/save  
cycle because of limitations there.

Alex

Patch

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index a6d811b..0cd6f68 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -39,7 +39,7 @@ 
 #define FW_CFG_SIZE 2
 
 typedef struct _FWCfgEntry {
-    uint16_t len;
+    uint32_t len;
     uint8_t *data;
     void *callback_opaque;
     FWCfgCallback callback;
@@ -48,7 +48,7 @@  typedef struct _FWCfgEntry {
 typedef struct _FWCfgState {
     FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
     uint16_t cur_entry;
-    uint16_t cur_offset;
+    uint32_t cur_offset;
 } FWCfgState;
 
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
@@ -164,19 +164,37 @@  static void fw_cfg_reset(void *opaque)
     fw_cfg_select(s, 0);
 }
 
+static int fw_cfg_load_old(QEMUFile *f, void *opaque, int version_id)
+{
+    FWCfgState *s = opaque;
+    uint16_t cur_offset;
+
+    if (version_id != 1)
+        return -EINVAL;
+
+    qemu_get_be16s(f, &s->cur_entry);
+
+    /* Convert old 16 bit value to new 32 bit width */
+    qemu_get_be16s(f, &cur_offset);
+    s->cur_offset = cur_offset;
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_fw_cfg = {
     .name = "fw_cfg",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .minimum_version_id_old = 1,
+    .load_state_old = fw_cfg_load_old,
     .fields      = (VMStateField []) {
         VMSTATE_UINT16(cur_entry, FWCfgState),
-        VMSTATE_UINT16(cur_offset, FWCfgState),
+        VMSTATE_UINT32(cur_offset, FWCfgState),
         VMSTATE_END_OF_LIST()
     }
 };
 
-int fw_cfg_add_bytes(void *opaque, uint16_t key, uint8_t *data, uint16_t len)
+int fw_cfg_add_bytes(void *opaque, uint16_t key, uint8_t *data, uint32_t len)
 {
     FWCfgState *s = opaque;
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
index 30dfec7..359d45a 100644
--- a/hw/fw_cfg.h
+++ b/hw/fw_cfg.h
@@ -28,7 +28,7 @@ 
 #ifndef NO_QEMU_PROTOS
 typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
 
-int fw_cfg_add_bytes(void *opaque, uint16_t key, uint8_t *data, uint16_t len);
+int fw_cfg_add_bytes(void *opaque, uint16_t key, uint8_t *data, uint32_t len);
 int fw_cfg_add_i16(void *opaque, uint16_t key, uint16_t value);
 int fw_cfg_add_i32(void *opaque, uint16_t key, uint32_t value);
 int fw_cfg_add_i64(void *opaque, uint16_t key, uint64_t value);