[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
Am 13.11.2009 um 11:59 schrieb Juan Quintela : Alexander Graf wrote: On 13.11.2009, at 01:48, Glauber Costa wrote: 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. Hi could you take a look at this one? This don't use the old_state function and should work as well. I haven't tested it yet (test machine down), but will do a bit later. Well I suppose your untested is better than my untested :-). When it comes to save/restore please take anything from Juan rather than my patch. I don't have access to a PC until Monday anyways and IMHO -kernel is a pretty important feature for developers that sholdn't stay vroken for too long in HEAD. Alex
[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
Alexander Graf wrote:
> On 13.11.2009, at 01:48, Glauber Costa wrote:
> 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.
Hi
could you take a look at this one?
This don't use the old_state function and should work as well.
I haven't tested it yet (test machine down), but will do a bit later.
Later, Juan.
PD. Yeap, I would have to add the HACK types to hw.h as several places
have decided to change the size of several fields.
>From 25f7a6e401d72a0584fa4630a9dc97ce34520f7b Mon Sep 17 00:00:00 2001
From: Juan Quintela
Date: Fri, 13 Nov 2009 11:56:38 +0100
Subject: [PATCH] fw_cfg: change cur_offset to 32 bits
Signed-off-by: Juan Quintela
---
hw/fw_cfg.c | 44 +++-
hw/fw_cfg.h |2 +-
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index a6d811b..b79d58f 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,53 @@ static void fw_cfg_reset(void *opaque)
fw_cfg_select(s, 0);
}
+/* Save restore 32 bit int as uint16_t
+ This is a Big hack, but it is how the old state did it.
+ Or we broke compatibility in the state, or we can't use struct tm
+ */
+
+static int get_uint32_as_uint16(QEMUFile *f, void *pv, size_t size)
+{
+uint32_t *v = pv;
+*v = qemu_get_be16(f);
+return 0;
+}
+
+static void put_unused(QEMUFile *f, void *pv, size_t size)
+{
+fprintf(stderr, "uint32_as_uint16 is only used for backward
compatibilty.\n");
+fprintf(stderr, "This functions shouldn't be called.\n");
+}
+
+const VMStateInfo vmstate_hack_uint32_as_uint16 = {
+.name = "int32_as_uint16",
+.get = get_uint32_as_uint16,
+.put = put_unused,
+};
+
+#define VMSTATE_UINT16_HACK(_f, _s, _t)\
+VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint32_as_uint16, uint32_t)
+
+
+static bool is_version_1(void *opaque, int version_id)
+{
+return version_id == 1;
+}
+
static const VMStateDescription vmstate_fw_cfg = {
.name = "fw_cfg",
-.version_id = 1,
+.version_id = 2,
.minimum_version_id = 1,
.minimum_version_id_old = 1,
.fields = (VMStateField []) {
VMSTATE_UINT16(cur_entry, FWCfgState),
-VMSTATE_UINT16(cur_offset, FWCfgState),
+VMSTATE_UINT16_HACK(cur_offset, FWCfgState, is_version_1),
+VMSTATE_UINT32_V(cur_offset, FWCfgState, 2),
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);
--
1.6.2.5
[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
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
---
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
[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
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
>
> ---
>
> 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 ?
[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
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
[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
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
---
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
[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
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
---
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
[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
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
>> ---
>> 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
[Qemu-devel] Re: [PATCH 1/6] Make fw_cfg interface 32-bit aware
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
---
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);
