[Qemu-devel] [PATCH 0/2] Here are 2 patches to enable sPAPR NVRAM migration.

[Qemu-devel] [PATCH 0/2] Here are 2 patches to enable sPAPR NVRAM migration.

[Alexey Kardashevskiy]

Please comment. Thanks!

Changes:
v2:
* reworked to always have a copy of NVRAM in RAM and use file only for writes




Alexey Kardashevskiy (2):
  vmstate: Allow dynamic allocation for VBUFFER during migration
  spapr_nvram: Enable migration

 hw/nvram/spapr_nvram.c      | 81 ++++++++++++++++++++++++++++++++++++---------
 include/migration/vmstate.h | 11 ++++++
 vmstate.c                   | 13 ++++++--
 3 files changed, 86 insertions(+), 19 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH 1/2] vmstate: Allow dynamic allocation for VBUFFER during migration

[Alexey Kardashevskiy]

This extends use of VMS_ALLOC flag from arrays to VBUFFER as well.

This defines VMSTATE_VBUFFER_ALLOC_UINT32 which makes use of VMS_ALLOC
and uses uint32_t type for a size.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/migration/vmstate.h | 11 +++++++++++
 vmstate.c                   | 13 ++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9a001bd..e45fc49 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -484,6 +484,17 @@ extern const VMStateInfo vmstate_info_bitmap;
     .start        = (_start),                                        \
 }
 
+#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, _start, _field_size) { \
+    .name         = (stringify(_field)),                             \
+    .version_id   = (_version),                                      \
+    .field_exists = (_test),                                         \
+    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
+    .info         = &vmstate_info_buffer,                            \
+    .flags        = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC,               \
+    .offset       = offsetof(_state, _field),                        \
+    .start        = (_start),                                        \
+}
+
 #define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, _size) { \
     .name       = (stringify(_field)),                               \
     .version_id = (_version),                                        \
diff --git a/vmstate.c b/vmstate.c
index ef2f87b..3dde574 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -49,9 +49,16 @@ static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
 
     if (field->flags & VMS_POINTER) {
         if (alloc && (field->flags & VMS_ALLOC)) {
-            int n_elems = vmstate_n_elems(opaque, field);
-            if (n_elems) {
-                gsize size = n_elems * field->size;
+            gsize size = 0;
+            if (field->flags & VMS_VBUFFER) {
+                size = vmstate_size(opaque, field);
+            } else {
+                int n_elems = vmstate_n_elems(opaque, field);
+                if (n_elems) {
+                    size = n_elems * field->size;
+                }
+            }
+            if (size) {
                 *((void **)base_addr + field->start) = g_malloc(size);
             }
         }
-- 
2.0.0

Re: [Qemu-devel] [PATCH 1/2] vmstate: Allow dynamic allocation for VBUFFER during migration

[Alexey Kardashevskiy]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/02/2014 07:56 PM, Alexey Kardashevskiy wrote:
> This extends use of VMS_ALLOC flag from arrays to VBUFFER as well.
> 
> This defines VMSTATE_VBUFFER_ALLOC_UINT32 which makes use of
> VMS_ALLOC and uses uint32_t type for a size.


Adding Juan to cc:. Please, ack :)


> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- 
> include/migration/vmstate.h | 11 +++++++++++ vmstate.c
> | 13 ++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/include/migration/vmstate.h
> b/include/migration/vmstate.h index 9a001bd..e45fc49 100644 ---
> a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@
> -484,6 +484,17 @@ extern const VMStateInfo vmstate_info_bitmap; .start
> = (_start),                                        \ }
> 
> +#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test,
> _start, _field_size) { \ +    .name         = (stringify(_field)),
> \ +    .version_id   = (_version),
> \ +    .field_exists = (_test),
> \ +    .size_offset  = vmstate_offset_value(_state, _field_size,
> uint32_t),\ +    .info         = &vmstate_info_buffer,
> \ +    .flags        = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC,
> \ +    .offset       = offsetof(_state, _field),
> \ +    .start        = (_start),
> \ +} + #define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version,
> _info, _size) { \ .name       = (stringify(_field)),
> \ .version_id = (_version),                                        \ 
> diff --git a/vmstate.c b/vmstate.c index ef2f87b..3dde574 100644 ---
> a/vmstate.c +++ b/vmstate.c @@ -49,9 +49,16 @@ static void
> *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
> 
> if (field->flags & VMS_POINTER) { if (alloc && (field->flags &
> VMS_ALLOC)) { -            int n_elems = vmstate_n_elems(opaque,
> field); -            if (n_elems) { -                gsize size =
> n_elems * field->size; +            gsize size = 0; +            if
> (field->flags & VMS_VBUFFER) { +                size =
> vmstate_size(opaque, field); +            } else { +
> int n_elems = vmstate_n_elems(opaque, field); +                if
> (n_elems) { +                    size = n_elems * field->size; +
> } +            } +            if (size) { *((void **)base_addr +
> field->start) = g_malloc(size); } }
> 


- -- 
Alexey
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJULeLkAAoJEIYTPdgrwSC5r2UP/RSiuKEanpXfhwyhudAB1nGM
9t3NJxrLO5PhkhfIveGgcf/t3hWpGOGu1JiiwpD8//ipjX0nBGt0O8p6Tp6xATtm
2NOid+9F6yUxfhLVB3vGQZ4KocguPt789t37HRgn9oT9YVxsJdeb1ftcDXw+opB7
qM113ZdrOVBPVeH1+vSC0vUwNYokjRQf/kX8IwwFXyJirt9/Rf9FaXf+KuturpJo
lp2yYVhuWmBy+MID5HZNlTfqVU5iMmX6ngqVm7rkaFxRA/3Ywlj1OtYGyf9htgya
OauFrvwUlM4yh3CiqLqu12Jabadu1Oa8yIeY9r2O5A4JLZt9JS2n3vxM70H8Z67Z
XJpQDF2Il26w9jIlmJ10GvVFt7sBaV0Yzu9SfgzNdSO/zteaODulOLj3eciUwnDk
DxbFvf8zn1vCuFjIwq3lqPaH1mNHEnLnV+6XlJLYNrD9Vngcc2UN/hXRMKwWovdL
rJAtOCObDB8VlilqS5/sFCekZsQasAnY7NckQvrx+0V/I7BJ99I7kKGcNIZF8pAU
84XHrw8j80+jh5W+6byn8M0lKgd7dn11KXcXzE1nuIVAP9KxG7Br5od3zePqhiIl
mdJ9meR3aOfNAr6N9iW/El+2g3FvhH3TQaZ+R5ZJMGCTVt9tqIVf1XLuXVbjdV3X
/xd/ACb10y32PH2sIEhl
=tSNm
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 1/2] vmstate: Allow dynamic allocation for VBUFFER during migration

[Alexey Kardashevskiy]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/03/2014 09:42 AM, Alexey Kardashevskiy wrote:
> On 10/02/2014 07:56 PM, Alexey Kardashevskiy wrote:
>> This extends use of VMS_ALLOC flag from arrays to VBUFFER as well.
> 
>> This defines VMSTATE_VBUFFER_ALLOC_UINT32 which makes use of 
>> VMS_ALLOC and uses uint32_t type for a size.
> 
> 
> Adding Juan to cc:. Please, ack :)


Juan, ping?



> 
> 
> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- 
>> include/migration/vmstate.h | 11 +++++++++++ vmstate.c | 13
>> ++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)
> 
>> diff --git a/include/migration/vmstate.h 
>> b/include/migration/vmstate.h index 9a001bd..e45fc49 100644 --- 
>> a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ 
>> -484,6 +484,17 @@ extern const VMStateInfo vmstate_info_bitmap;
>> .start = (_start),                                        \ }
> 
>> +#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version,
>> _test, _start, _field_size) { \ +    .name         =
>> (stringify(_field)), \ +    .version_id   = (_version), \ +
>> .field_exists = (_test), \ +    .size_offset  =
>> vmstate_offset_value(_state, _field_size, uint32_t),\ +    .info
>> = &vmstate_info_buffer, \ +    .flags        =
>> VMS_VBUFFER|VMS_POINTER|VMS_ALLOC, \ +    .offset       =
>> offsetof(_state, _field), \ +    .start        = (_start), \ +} +
>> #define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info,
>> _size) { \ .name       = (stringify(_field)), \ .version_id =
>> (_version),                                        \ diff --git
>> a/vmstate.c b/vmstate.c index ef2f87b..3dde574 100644 --- 
>> a/vmstate.c +++ b/vmstate.c @@ -49,9 +49,16 @@ static void 
>> *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
> 
>> if (field->flags & VMS_POINTER) { if (alloc && (field->flags & 
>> VMS_ALLOC)) { -            int n_elems = vmstate_n_elems(opaque, 
>> field); -            if (n_elems) { -                gsize size = 
>> n_elems * field->size; +            gsize size = 0; +            if 
>> (field->flags & VMS_VBUFFER) { +                size = 
>> vmstate_size(opaque, field); +            } else { + int n_elems =
>> vmstate_n_elems(opaque, field); +                if (n_elems) { +
>> size = n_elems * field->size; + } +            } +            if
>> (size) { *((void **)base_addr + field->start) = g_malloc(size); } }
> 
> 
> 
> 

- -- 
Alexey
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJUNhsTAAoJEIYTPdgrwSC5CzgQAJbQ7C259BGZqPics7P1FhIm
wzHhwfUwfEML6nRtaVMeime8qqfbD048VNH43/nCiCoy2MjkvXV/uT+j2f+hLjcG
46Ys004S7SDn+bl+7Z24sFGZ5iqBO3is+kLj79w878IwC5SPM0jH7dfnU+ZSimVE
rCug/D95IvU4gOVfxtDbFg5oq6sgGHC+SpLsg9CAo2lBcOje5ylSAwyd5/s3pmnC
FMXf3nJI/aAQTcbrVeuB4+khGP4hncqzh2w07IP8olszZcyTzRIzQnyU3eqIbPrM
8nX6HdrauVj0VxtSZJzticaJnzO7qqOl+1TiCZWCV5m7jchbPbKnuvovoRQ1v5cw
DSmZ8BULGK6XnuOmqhEY8AMDAYcHP8DvnJ1N+dmAcsfCr6jFIb0HAW6VWsqMJxDG
J7G3jIKooOSXCmciFkS+2QMJ0TrAgavF/K2njdcA/7up+nVBDnPsEX+bFnox9N+w
KnyBYS8tWqP4NuK+q53BtkZllMXQYSLFQUoi5XgKoCL1dk1xoxlbZg/3rbXzMB3a
sWt3W9zxx5PzsNGEUfQyl8mgmR02y+N1cqJRu/6HJfxfXJ2m+LvTPhlp1oWJVuHT
YKam971lTrPja0Z7OfNgWjPgv/OOW7NjzMGbq51SV20TDKgn1sJMSGqJ/cdqBkeS
HZBj6AVhEzX4V+NDAkrH
=JyEK
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 1/2] vmstate: Allow dynamic allocation for VBUFFER during migration

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Thu, Oct 02, 2014 at 07:56:02PM +1000, Alexey Kardashevskiy wrote:
> This extends use of VMS_ALLOC flag from arrays to VBUFFER as well.
> 
> This defines VMSTATE_VBUFFER_ALLOC_UINT32 which makes use of VMS_ALLOC
> and uses uint32_t type for a size.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/2] vmstate: Allow dynamic allocation for VBUFFER during migration

[Juan Quintela]

Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> This extends use of VMS_ALLOC flag from arrays to VBUFFER as well.
>
> This defines VMSTATE_VBUFFER_ALLOC_UINT32 which makes use of VMS_ALLOC
> and uses uint32_t type for a size.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration

[Alexey Kardashevskiy]

The only case when sPAPR NVRAM migrates now is if is backed by a file and
copy-storage migration is performed. In other cases NVRAM does not
migrate regardless whether it is backed by a file or not.

This enables shadow copy of NVRAM in RAM which is read from a file
(if used) and used for reads. Writes to NVRAM are mirrored to the file.

This defines a VMSTATE descriptor for NVRAM device so the memory copy
of NVRAM can migrate and be flushed to a backing file on the destination
if one is specified.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* implemented full-time shadow copy of NVRAM which is read from a file
at start and stays in RAM during live of the guest
---
 hw/nvram/spapr_nvram.c | 81 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 16 deletions(-)

diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 6a72ef4..d34edfb 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -51,7 +51,6 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 {
     sPAPRNVRAM *nvram = spapr->nvram;
     hwaddr offset, buffer, len;
-    int alen;
     void *membuf;
 
     if ((nargs != 3) || (nret != 2)) {
@@ -76,19 +75,16 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
         return;
     }
 
+    assert(nvram->buf);
+
     membuf = cpu_physical_memory_map(buffer, &len, 1);
-    if (nvram->drive) {
-        alen = bdrv_pread(nvram->drive, offset, membuf, len);
-    } else {
-        assert(nvram->buf);
 
-        memcpy(membuf, nvram->buf + offset, len);
-        alen = len;
-    }
+    memcpy(membuf, nvram->buf + offset, len);
+
     cpu_physical_memory_unmap(membuf, len, 1, len);
 
-    rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
-    rtas_st(rets, 1, (alen < 0) ? 0 : alen);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    rtas_st(rets, 1, len);
 }
 
 static void rtas_nvram_store(PowerPCCPU *cpu, sPAPREnvironment *spapr,
@@ -122,14 +118,15 @@ static void rtas_nvram_store(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     }
 
     membuf = cpu_physical_memory_map(buffer, &len, 0);
+
+    alen = len;
     if (nvram->drive) {
         alen = bdrv_pwrite(nvram->drive, offset, membuf, len);
-    } else {
-        assert(nvram->buf);
-
-        memcpy(nvram->buf + offset, membuf, len);
-        alen = len;
     }
+
+    assert(nvram->buf);
+    memcpy(nvram->buf + offset, membuf, len);
+
     cpu_physical_memory_unmap(membuf, len, 0, len);
 
     rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
@@ -144,15 +141,24 @@ static int spapr_nvram_init(VIOsPAPRDevice *dev)
         nvram->size = bdrv_getlength(nvram->drive);
     } else {
         nvram->size = DEFAULT_NVRAM_SIZE;
-        nvram->buf = g_malloc0(nvram->size);
     }
 
+    nvram->buf = g_malloc0(nvram->size);
+
     if ((nvram->size < MIN_NVRAM_SIZE) || (nvram->size > MAX_NVRAM_SIZE)) {
         fprintf(stderr, "spapr-nvram must be between %d and %d bytes in size\n",
                 MIN_NVRAM_SIZE, MAX_NVRAM_SIZE);
         return -1;
     }
 
+    if (nvram->drive) {
+        int alen = bdrv_pread(nvram->drive, 0, nvram->buf, nvram->size);
+
+        if (alen != nvram->size) {
+            return -1;
+        }
+    }
+
     spapr_rtas_register(RTAS_NVRAM_FETCH, "nvram-fetch", rtas_nvram_fetch);
     spapr_rtas_register(RTAS_NVRAM_STORE, "nvram-store", rtas_nvram_store);
 
@@ -166,6 +172,48 @@ static int spapr_nvram_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
     return fdt_setprop_cell(fdt, node_off, "#bytes", nvram->size);
 }
 
+static int spapr_nvram_pre_load(void *opaque)
+{
+    sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
+
+    g_free(nvram->buf);
+    nvram->buf = NULL;
+    nvram->size = 0;
+
+    return 0;
+}
+
+static int spapr_nvram_post_load(void *opaque, int version_id)
+{
+    sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
+
+    if (nvram->drive) {
+        int alen = bdrv_pwrite(nvram->drive, 0, nvram->buf, nvram->size);
+
+        if (alen < 0) {
+            return alen;
+        }
+        if (alen != nvram->size) {
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_spapr_nvram = {
+    .name = "spapr_nvram",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .pre_load = spapr_nvram_pre_load,
+    .post_load = spapr_nvram_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(size, sPAPRNVRAM),
+        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, 0, size),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static Property spapr_nvram_properties[] = {
     DEFINE_SPAPR_PROPERTIES(sPAPRNVRAM, sdev),
     DEFINE_PROP_DRIVE("drive", sPAPRNVRAM, drive),
@@ -184,6 +232,7 @@ static void spapr_nvram_class_init(ObjectClass *klass, void *data)
     k->dt_compatible = "qemu,spapr-nvram";
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->props = spapr_nvram_properties;
+    dc->vmsd = &vmstate_spapr_nvram;
 }
 
 static const TypeInfo spapr_nvram_type_info = {
-- 
2.0.0

Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

On Thu, Oct 02, 2014 at 07:56:03PM +1000, Alexey Kardashevskiy wrote:
> The only case when sPAPR NVRAM migrates now is if is backed by a file and
> copy-storage migration is performed. In other cases NVRAM does not
> migrate regardless whether it is backed by a file or not.
> 
> This enables shadow copy of NVRAM in RAM which is read from a file
> (if used) and used for reads. Writes to NVRAM are mirrored to the file.
> 
> This defines a VMSTATE descriptor for NVRAM device so the memory copy
> of NVRAM can migrate and be flushed to a backing file on the destination
> if one is specified.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/2] Here are 2 patches to enable sPAPR NVRAM migration.

[Alexander Graf]

On 02.10.14 11:56, Alexey Kardashevskiy wrote:
> Please comment. Thanks!
> 
> Changes:
> v2:
> * reworked to always have a copy of NVRAM in RAM and use file only for writes

I like it, please get an ack from Juan on patch 1/2, then I can apply it.


Alex

Re: [Qemu-devel] [PATCH 0/2] Here are 2 patches to enable sPAPR NVRAM migration.

[Alexander Graf]

On 02.10.14 11:56, Alexey Kardashevskiy wrote:
> Please comment. Thanks!
> 
> Changes:
> v2:
> * reworked to always have a copy of NVRAM in RAM and use file only for writes

Thanks, applied to ppc-next.


Alex

[Qemu-devel] [PATCH 0/2] spapr_nvram: Support migration

[Alexey Kardashevskiy]

Here are 2 patches to enable sPAPR NVRAM migration.

Please comment. Thanks!


Alexey Kardashevskiy (2):
  vmstate: Allow dynamic allocation for VBUFFER during migration
  spapr_nvram: Enable migration

 hw/nvram/spapr_nvram.c      | 68 +++++++++++++++++++++++++++++++++++++++------
 include/migration/vmstate.h | 11 ++++++++
 vmstate.c                   | 13 +++++++--
 3 files changed, 80 insertions(+), 12 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration

[Alexey Kardashevskiy]

The only case when sPAPR NVRAM migrates now is if is backed by a file and
copy-storage migration is performed.

This enables RAM copy of NVRAM even if NVRAM is backed by a file.

This defines a VMSTATE descriptor for NVRAM device so the memory copy
of NVRAM can migrate and be written to a backing file on the destination
if one is provided.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/nvram/spapr_nvram.c | 68 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 9 deletions(-)

diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 6a72ef4..254009e 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
         return;
     }
 
+    assert(nvram->buf);
+
     membuf = cpu_physical_memory_map(buffer, &len, 1);
+
+    alen = len;
     if (nvram->drive) {
         alen = bdrv_pread(nvram->drive, offset, membuf, len);
+        if (alen > 0) {
+            memcpy(nvram->buf + offset, membuf, alen);
+        }
     } else {
-        assert(nvram->buf);
-
         memcpy(membuf, nvram->buf + offset, len);
-        alen = len;
     }
+
     cpu_physical_memory_unmap(membuf, len, 1, len);
 
     rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
@@ -122,14 +127,15 @@ static void rtas_nvram_store(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     }
 
     membuf = cpu_physical_memory_map(buffer, &len, 0);
+
+    alen = len;
     if (nvram->drive) {
         alen = bdrv_pwrite(nvram->drive, offset, membuf, len);
-    } else {
-        assert(nvram->buf);
-
-        memcpy(nvram->buf + offset, membuf, len);
-        alen = len;
     }
+
+    assert(nvram->buf);
+    memcpy(nvram->buf + offset, membuf, len);
+
     cpu_physical_memory_unmap(membuf, len, 0, len);
 
     rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
@@ -144,9 +150,10 @@ static int spapr_nvram_init(VIOsPAPRDevice *dev)
         nvram->size = bdrv_getlength(nvram->drive);
     } else {
         nvram->size = DEFAULT_NVRAM_SIZE;
-        nvram->buf = g_malloc0(nvram->size);
     }
 
+    nvram->buf = g_malloc0(nvram->size);
+
     if ((nvram->size < MIN_NVRAM_SIZE) || (nvram->size > MAX_NVRAM_SIZE)) {
         fprintf(stderr, "spapr-nvram must be between %d and %d bytes in size\n",
                 MIN_NVRAM_SIZE, MAX_NVRAM_SIZE);
@@ -166,6 +173,48 @@ static int spapr_nvram_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
     return fdt_setprop_cell(fdt, node_off, "#bytes", nvram->size);
 }
 
+static int spapr_nvram_pre_load(void *opaque)
+{
+    sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
+
+    g_free(nvram->buf);
+    nvram->buf = NULL;
+    nvram->size = 0;
+
+    return 0;
+}
+
+static int spapr_nvram_post_load(void *opaque, int version_id)
+{
+    sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
+
+    if (nvram->drive) {
+        int alen = bdrv_pwrite(nvram->drive, 0, nvram->buf, nvram->size);
+
+        if (alen < 0) {
+            return alen;
+        }
+        if (alen != nvram->size) {
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_spapr_nvram = {
+    .name = "spapr_nvram",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .pre_load = spapr_nvram_pre_load,
+    .post_load = spapr_nvram_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(size, sPAPRNVRAM),
+        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, 0, size),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static Property spapr_nvram_properties[] = {
     DEFINE_SPAPR_PROPERTIES(sPAPRNVRAM, sdev),
     DEFINE_PROP_DRIVE("drive", sPAPRNVRAM, drive),
@@ -184,6 +233,7 @@ static void spapr_nvram_class_init(ObjectClass *klass, void *data)
     k->dt_compatible = "qemu,spapr-nvram";
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->props = spapr_nvram_properties;
+    dc->vmsd = &vmstate_spapr_nvram;
 }
 
 static const TypeInfo spapr_nvram_type_info = {
-- 
2.0.0

Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration

[Alexander Graf]

On 25.09.14 09:02, Alexey Kardashevskiy wrote:
> The only case when sPAPR NVRAM migrates now is if is backed by a file and
> copy-storage migration is performed.
> 
> This enables RAM copy of NVRAM even if NVRAM is backed by a file.
> 
> This defines a VMSTATE descriptor for NVRAM device so the memory copy
> of NVRAM can migrate and be written to a backing file on the destination
> if one is provided.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/nvram/spapr_nvram.c | 68 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> index 6a72ef4..254009e 100644
> --- a/hw/nvram/spapr_nvram.c
> +++ b/hw/nvram/spapr_nvram.c
> @@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>          return;
>      }
>  
> +    assert(nvram->buf);
> +
>      membuf = cpu_physical_memory_map(buffer, &len, 1);
> +
> +    alen = len;
>      if (nvram->drive) {
>          alen = bdrv_pread(nvram->drive, offset, membuf, len);
> +        if (alen > 0) {
> +            memcpy(nvram->buf + offset, membuf, alen);

Why?

> +        }
>      } else {
> -        assert(nvram->buf);
> -
>          memcpy(membuf, nvram->buf + offset, len);
> -        alen = len;
>      }
> +
>      cpu_physical_memory_unmap(membuf, len, 1, len);
>  
>      rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
> @@ -122,14 +127,15 @@ static void rtas_nvram_store(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>      }
>  
>      membuf = cpu_physical_memory_map(buffer, &len, 0);
> +
> +    alen = len;
>      if (nvram->drive) {
>          alen = bdrv_pwrite(nvram->drive, offset, membuf, len);
> -    } else {
> -        assert(nvram->buf);
> -
> -        memcpy(nvram->buf + offset, membuf, len);
> -        alen = len;
>      }
> +
> +    assert(nvram->buf);
> +    memcpy(nvram->buf + offset, membuf, len);
> +
>      cpu_physical_memory_unmap(membuf, len, 0, len);
>  
>      rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
> @@ -144,9 +150,10 @@ static int spapr_nvram_init(VIOsPAPRDevice *dev)
>          nvram->size = bdrv_getlength(nvram->drive);
>      } else {
>          nvram->size = DEFAULT_NVRAM_SIZE;
> -        nvram->buf = g_malloc0(nvram->size);
>      }
>  
> +    nvram->buf = g_malloc0(nvram->size);
> +
>      if ((nvram->size < MIN_NVRAM_SIZE) || (nvram->size > MAX_NVRAM_SIZE)) {
>          fprintf(stderr, "spapr-nvram must be between %d and %d bytes in size\n",
>                  MIN_NVRAM_SIZE, MAX_NVRAM_SIZE);
> @@ -166,6 +173,48 @@ static int spapr_nvram_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
>      return fdt_setprop_cell(fdt, node_off, "#bytes", nvram->size);
>  }
>  
> +static int spapr_nvram_pre_load(void *opaque)
> +{
> +    sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
> +
> +    g_free(nvram->buf);
> +    nvram->buf = NULL;
> +    nvram->size = 0;
> +
> +    return 0;
> +}
> +
> +static int spapr_nvram_post_load(void *opaque, int version_id)
> +{
> +    sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
> +
> +    if (nvram->drive) {
> +        int alen = bdrv_pwrite(nvram->drive, 0, nvram->buf, nvram->size);

In the file backed case you're already overwriting the disk backed
version, no?

Also, couldn't you just do the copy and provisioning of buf in a
pre_save hook?


Alex

> +
> +        if (alen < 0) {
> +            return alen;
> +        }
> +        if (alen != nvram->size) {
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_spapr_nvram = {
> +    .name = "spapr_nvram",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .pre_load = spapr_nvram_pre_load,
> +    .post_load = spapr_nvram_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(size, sPAPRNVRAM),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, 0, size),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static Property spapr_nvram_properties[] = {
>      DEFINE_SPAPR_PROPERTIES(sPAPRNVRAM, sdev),
>      DEFINE_PROP_DRIVE("drive", sPAPRNVRAM, drive),
> @@ -184,6 +233,7 @@ static void spapr_nvram_class_init(ObjectClass *klass, void *data)
>      k->dt_compatible = "qemu,spapr-nvram";
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      dc->props = spapr_nvram_properties;
> +    dc->vmsd = &vmstate_spapr_nvram;
>  }
>  
>  static const TypeInfo spapr_nvram_type_info = {
> 

Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration

[Alexey Kardashevskiy]

On 09/25/2014 07:43 PM, Alexander Graf wrote:
> 
> 
> On 25.09.14 09:02, Alexey Kardashevskiy wrote:
>> The only case when sPAPR NVRAM migrates now is if is backed by a file and
>> copy-storage migration is performed.
>>
>> This enables RAM copy of NVRAM even if NVRAM is backed by a file.
>>
>> This defines a VMSTATE descriptor for NVRAM device so the memory copy
>> of NVRAM can migrate and be written to a backing file on the destination
>> if one is provided.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  hw/nvram/spapr_nvram.c | 68 +++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 59 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
>> index 6a72ef4..254009e 100644
>> --- a/hw/nvram/spapr_nvram.c
>> +++ b/hw/nvram/spapr_nvram.c
>> @@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>          return;
>>      }
>>  
>> +    assert(nvram->buf);
>> +
>>      membuf = cpu_physical_memory_map(buffer, &len, 1);
>> +
>> +    alen = len;
>>      if (nvram->drive) {
>>          alen = bdrv_pread(nvram->drive, offset, membuf, len);
>> +        if (alen > 0) {
>> +            memcpy(nvram->buf + offset, membuf, alen);
> 
> Why?

This way I do not need pre_save hook and I keep the buf in sync with the
file. If I implement pre_save, then buf will serve 2 purposes - it is
either NVRAM itself (if there is no backing file, exists during guest's
lifetime) or it is a migration copy (exists between pre_save and post_load
and then it is disposed). Two quite different uses of the same thing
confuse me. But - I do not mind doing it your way, no big deal, should I?


>> +        }
>>      } else {
>> -        assert(nvram->buf);
>> -
>>          memcpy(membuf, nvram->buf + offset, len);
>> -        alen = len;
>>      }
>> +
>>      cpu_physical_memory_unmap(membuf, len, 1, len);
>>  
>>      rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
>> @@ -122,14 +127,15 @@ static void rtas_nvram_store(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>      }
>>  
>>      membuf = cpu_physical_memory_map(buffer, &len, 0);
>> +
>> +    alen = len;
>>      if (nvram->drive) {
>>          alen = bdrv_pwrite(nvram->drive, offset, membuf, len);
>> -    } else {
>> -        assert(nvram->buf);
>> -
>> -        memcpy(nvram->buf + offset, membuf, len);
>> -        alen = len;
>>      }
>> +
>> +    assert(nvram->buf);
>> +    memcpy(nvram->buf + offset, membuf, len);
>> +
>>      cpu_physical_memory_unmap(membuf, len, 0, len);
>>  
>>      rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS);
>> @@ -144,9 +150,10 @@ static int spapr_nvram_init(VIOsPAPRDevice *dev)
>>          nvram->size = bdrv_getlength(nvram->drive);
>>      } else {
>>          nvram->size = DEFAULT_NVRAM_SIZE;
>> -        nvram->buf = g_malloc0(nvram->size);
>>      }
>>  
>> +    nvram->buf = g_malloc0(nvram->size);
>> +
>>      if ((nvram->size < MIN_NVRAM_SIZE) || (nvram->size > MAX_NVRAM_SIZE)) {
>>          fprintf(stderr, "spapr-nvram must be between %d and %d bytes in size\n",
>>                  MIN_NVRAM_SIZE, MAX_NVRAM_SIZE);
>> @@ -166,6 +173,48 @@ static int spapr_nvram_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
>>      return fdt_setprop_cell(fdt, node_off, "#bytes", nvram->size);
>>  }
>>  
>> +static int spapr_nvram_pre_load(void *opaque)
>> +{
>> +    sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
>> +
>> +    g_free(nvram->buf);
>> +    nvram->buf = NULL;
>> +    nvram->size = 0;
>> +
>> +    return 0;
>> +}
>> +
>> +static int spapr_nvram_post_load(void *opaque, int version_id)
>> +{
>> +    sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(opaque);
>> +
>> +    if (nvram->drive) {
>> +        int alen = bdrv_pwrite(nvram->drive, 0, nvram->buf, nvram->size);
> 
> In the file backed case you're already overwriting the disk backed
> version, no?

Yes. Is that bad?


> Also, couldn't you just do the copy and provisioning of buf in a
> pre_save hook?


I can do this too. I just do not see why that would be lot better though :)



-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

On Thu, Sep 25, 2014 at 08:06:40PM +1000, Alexey Kardashevskiy wrote:
> On 09/25/2014 07:43 PM, Alexander Graf wrote:
> > 
> > 
> > On 25.09.14 09:02, Alexey Kardashevskiy wrote:
> >> The only case when sPAPR NVRAM migrates now is if is backed by a file and
> >> copy-storage migration is performed.
> >>
> >> This enables RAM copy of NVRAM even if NVRAM is backed by a file.
> >>
> >> This defines a VMSTATE descriptor for NVRAM device so the memory copy
> >> of NVRAM can migrate and be written to a backing file on the destination
> >> if one is provided.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >>  hw/nvram/spapr_nvram.c | 68 +++++++++++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 59 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> >> index 6a72ef4..254009e 100644
> >> --- a/hw/nvram/spapr_nvram.c
> >> +++ b/hw/nvram/spapr_nvram.c
> >> @@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> >>          return;
> >>      }
> >>  
> >> +    assert(nvram->buf);
> >> +
> >>      membuf = cpu_physical_memory_map(buffer, &len, 1);
> >> +
> >> +    alen = len;
> >>      if (nvram->drive) {
> >>          alen = bdrv_pread(nvram->drive, offset, membuf, len);
> >> +        if (alen > 0) {
> >> +            memcpy(nvram->buf + offset, membuf, alen);
> > 
> > Why?
> 
> This way I do not need pre_save hook and I keep the buf in sync with the
> file. If I implement pre_save, then buf will serve 2 purposes - it is
> either NVRAM itself (if there is no backing file, exists during guest's
> lifetime) or it is a migration copy (exists between pre_save and post_load
> and then it is disposed). Two quite different uses of the same thing
> confuse me. But - I do not mind doing it your way, no big deal,
> should I?

This doesn't seem quite right to me.  I don't see anything that pulls
in the whole of the nvram contents at initialization, so it looks like
the buffer will only be in sync with the driver for the portions that
are either read or written by the guest.  Then, if you migrate while
not all of the memory copy is in sync, you could clobber the
out-of-sync parts of the disk copy as well.

Instead, I think you need to suck in the whole of the contents during
init, then all reads can just be supplied from the memory buffer, and
you'll only need to access the backing disk for stores.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration

[Alexey Kardashevskiy]

On 09/26/2014 12:31 PM, David Gibson wrote:
> On Thu, Sep 25, 2014 at 08:06:40PM +1000, Alexey Kardashevskiy wrote:
>> On 09/25/2014 07:43 PM, Alexander Graf wrote:
>>>
>>>
>>> On 25.09.14 09:02, Alexey Kardashevskiy wrote:
>>>> The only case when sPAPR NVRAM migrates now is if is backed by a file and
>>>> copy-storage migration is performed.
>>>>
>>>> This enables RAM copy of NVRAM even if NVRAM is backed by a file.
>>>>
>>>> This defines a VMSTATE descriptor for NVRAM device so the memory copy
>>>> of NVRAM can migrate and be written to a backing file on the destination
>>>> if one is provided.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>>  hw/nvram/spapr_nvram.c | 68 +++++++++++++++++++++++++++++++++++++++++++-------
>>>>  1 file changed, 59 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
>>>> index 6a72ef4..254009e 100644
>>>> --- a/hw/nvram/spapr_nvram.c
>>>> +++ b/hw/nvram/spapr_nvram.c
>>>> @@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>>          return;
>>>>      }
>>>>  
>>>> +    assert(nvram->buf);
>>>> +
>>>>      membuf = cpu_physical_memory_map(buffer, &len, 1);
>>>> +
>>>> +    alen = len;
>>>>      if (nvram->drive) {
>>>>          alen = bdrv_pread(nvram->drive, offset, membuf, len);
>>>> +        if (alen > 0) {
>>>> +            memcpy(nvram->buf + offset, membuf, alen);
>>>
>>> Why?
>>
>> This way I do not need pre_save hook and I keep the buf in sync with the
>> file. If I implement pre_save, then buf will serve 2 purposes - it is
>> either NVRAM itself (if there is no backing file, exists during guest's
>> lifetime) or it is a migration copy (exists between pre_save and post_load
>> and then it is disposed). Two quite different uses of the same thing
>> confuse me. But - I do not mind doing it your way, no big deal,
>> should I?
> 
> This doesn't seem quite right to me.  I don't see anything that pulls
> in the whole of the nvram contents at initialization, so it looks like
> the buffer will only be in sync with the driver for the portions that
> are either read or written by the guest.  Then, if you migrate while
> not all of the memory copy is in sync, you could clobber the
> out-of-sync parts of the disk copy as well.

Yes. I missed that :-/


> Instead, I think you need to suck in the whole of the contents during
> init, then all reads can just be supplied from the memory buffer, and
> you'll only need to access the backing disk for stores.

I like this and I will do this if Alex does not mind.
Thanks!


-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration

[Alexander Graf]

On 26.09.14 04:53, Alexey Kardashevskiy wrote:
> On 09/26/2014 12:31 PM, David Gibson wrote:
>> On Thu, Sep 25, 2014 at 08:06:40PM +1000, Alexey Kardashevskiy wrote:
>>> On 09/25/2014 07:43 PM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 25.09.14 09:02, Alexey Kardashevskiy wrote:
>>>>> The only case when sPAPR NVRAM migrates now is if is backed by a file and
>>>>> copy-storage migration is performed.
>>>>>
>>>>> This enables RAM copy of NVRAM even if NVRAM is backed by a file.
>>>>>
>>>>> This defines a VMSTATE descriptor for NVRAM device so the memory copy
>>>>> of NVRAM can migrate and be written to a backing file on the destination
>>>>> if one is provided.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>>  hw/nvram/spapr_nvram.c | 68 +++++++++++++++++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 59 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
>>>>> index 6a72ef4..254009e 100644
>>>>> --- a/hw/nvram/spapr_nvram.c
>>>>> +++ b/hw/nvram/spapr_nvram.c
>>>>> @@ -76,15 +76,20 @@ static void rtas_nvram_fetch(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>>>          return;
>>>>>      }
>>>>>  
>>>>> +    assert(nvram->buf);
>>>>> +
>>>>>      membuf = cpu_physical_memory_map(buffer, &len, 1);
>>>>> +
>>>>> +    alen = len;
>>>>>      if (nvram->drive) {
>>>>>          alen = bdrv_pread(nvram->drive, offset, membuf, len);
>>>>> +        if (alen > 0) {
>>>>> +            memcpy(nvram->buf + offset, membuf, alen);
>>>>
>>>> Why?
>>>
>>> This way I do not need pre_save hook and I keep the buf in sync with the
>>> file. If I implement pre_save, then buf will serve 2 purposes - it is
>>> either NVRAM itself (if there is no backing file, exists during guest's
>>> lifetime) or it is a migration copy (exists between pre_save and post_load
>>> and then it is disposed). Two quite different uses of the same thing
>>> confuse me. But - I do not mind doing it your way, no big deal,
>>> should I?
>>
>> This doesn't seem quite right to me.  I don't see anything that pulls
>> in the whole of the nvram contents at initialization, so it looks like
>> the buffer will only be in sync with the driver for the portions that
>> are either read or written by the guest.  Then, if you migrate while
>> not all of the memory copy is in sync, you could clobber the
>> out-of-sync parts of the disk copy as well.
> 
> Yes. I missed that :-/
> 
> 
>> Instead, I think you need to suck in the whole of the contents during
>> init, then all reads can just be supplied from the memory buffer, and
>> you'll only need to access the backing disk for stores.
> 
> I like this and I will do this if Alex does not mind.

So you'd always keep a shadow copy in RAM and only use the file for
writes? Sounds like a good plan to me.


Alex