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

login
register
mail settings
Submitter Alexander Graf
Date Nov. 11, 2009, 6:09 p.m.
Message ID <1257962966-22902-2-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/38166/
State New
Headers show

Comments

Alexander Graf - Nov. 11, 2009, 6:09 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>
---
 hw/fw_cfg.c |    8 ++++----
 hw/fw_cfg.h |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)
Anthony Liguori - Nov. 11, 2009, 9:53 p.m.
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>
> ---
>  hw/fw_cfg.c |    8 ++++----
>  hw/fw_cfg.h |    2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index a6d811b..3a3f694 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)
> @@ -171,12 +171,12 @@ static const VMStateDescription vmstate_fw_cfg = {
>      .minimum_version_id_old = 1,
>      .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);
>   

We need to bump a version here.

> 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);
>
Alexander Graf - Nov. 11, 2009, 10:15 p.m.
Anthony Liguori wrote:
> 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>
>> ---
>>  hw/fw_cfg.c |    8 ++++----
>>  hw/fw_cfg.h |    2 +-
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>> index a6d811b..3a3f694 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)
>> @@ -171,12 +171,12 @@ static const VMStateDescription vmstate_fw_cfg = {
>>      .minimum_version_id_old = 1,
>>      .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);
>>   
>
> We need to bump a version here.

Sure - which one?


Alex
Anthony Liguori - Nov. 11, 2009, 10:22 p.m.
Alexander Graf wrote:
> Anthony Liguori wrote:
>   
>> 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>
>>> ---
>>>  hw/fw_cfg.c |    8 ++++----
>>>  hw/fw_cfg.h |    2 +-
>>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>>> index a6d811b..3a3f694 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)
>>> @@ -171,12 +171,12 @@ static const VMStateDescription vmstate_fw_cfg = {
>>>      .minimum_version_id_old = 1,
>>>      .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);
>>>   
>>>       
>> We need to bump a version here.
>>     
>
> Sure - which one?
>   

The version_id field in vmstate_fw_cfg.  You also have to try to support 
older versions which means you may want to either split cur_offset into 
a high and low or ask Juan what the appropriate vodoo would be.

Regards,

Anthony Liguori

> Alex
>
Alexander Graf - Nov. 12, 2009, 12:03 a.m.
On 11.11.2009, at 23:22, Anthony Liguori wrote:

> Alexander Graf wrote:
>> Anthony Liguori wrote:
>>
>>> 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>
>>>> ---
>>>> hw/fw_cfg.c |    8 ++++----
>>>> hw/fw_cfg.h |    2 +-
>>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>>>> index a6d811b..3a3f694 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)
>>>> @@ -171,12 +171,12 @@ static const VMStateDescription  
>>>> vmstate_fw_cfg = {
>>>>     .minimum_version_id_old = 1,
>>>>     .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);
>>>>
>>> We need to bump a version here.
>>>
>>
>> Sure - which one?
>>
>
> The version_id field in vmstate_fw_cfg.  You also have to try to  
> support older versions which means you may want to either split  
> cur_offset into a high and low or ask Juan what the appropriate  
> vodoo would be.

Juan, I'd really love to learn some new voodoo :-).
This whole new qdev whatever based save format was supposed to make  
things like this easy, right? I would've known what to do with the old  
code ...

Alex
Anthony Liguori - Nov. 12, 2009, 12:14 a.m.
Alexander Graf wrote:
> Juan, I'd really love to learn some new voodoo :-).
> This whole new qdev whatever based save format was supposed to make 
> things like this easy, right? I would've known what to do with the old 
> code ...

I think Juan's mentioned something about writing a doc explaining how to 
use VMState correctly.  I think it would certainly be helpful for 
situations like this.

But the most important part of VMState is that it converts something 
that was previously open coded and opaque to something that is 
data-driven and introspectable.  I think it's done an extremely good job 
of achieving those goals.   As we get everything converted, we can 
potentially figure out some ways to make this all a bit easier to 
understand.  Right now, I think how we support backwards compatibility 
is admittedly awkward.

Regards,

Anthony Liguori

> Alex

Patch

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index a6d811b..3a3f694 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)
@@ -171,12 +171,12 @@  static const VMStateDescription vmstate_fw_cfg = {
     .minimum_version_id_old = 1,
     .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);