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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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