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 <address@hidden>
>>>>> ---
>>>>>  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