From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35067) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYWMN-0003Yb-LZ for qemu-devel@nongnu.org; Mon, 29 Sep 2014 04:31:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYWMH-0000j0-3s for qemu-devel@nongnu.org; Mon, 29 Sep 2014 04:31:07 -0400 Message-ID: <542918BD.2000705@suse.de> Date: Mon, 29 Sep 2014 10:30:53 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1411628523-3498-1-git-send-email-aik@ozlabs.ru> <1411628523-3498-3-git-send-email-aik@ozlabs.ru> <5423E3C6.4070708@suse.de> <5423E930.6070704@ozlabs.ru> <20140926023112.GG24332@voom.redhat.com> <5424D521.5090305@ozlabs.ru> In-Reply-To: <5424D521.5090305@ozlabs.ru> Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] spapr_nvram: Enable migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy , David Gibson Cc: Paolo Bonzini , qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 >>>>> --- >>>>> 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