From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53177) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XXLfE-0002Mz-Ef for qemu-devel@nongnu.org; Thu, 25 Sep 2014 22:53:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XXLf8-0002bH-3Y for qemu-devel@nongnu.org; Thu, 25 Sep 2014 22:53:44 -0400 Received: from mail-pa0-f54.google.com ([209.85.220.54]:55833) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XXLf7-0002Q5-SS for qemu-devel@nongnu.org; Thu, 25 Sep 2014 22:53:37 -0400 Received: by mail-pa0-f54.google.com with SMTP id ey11so376191pad.27 for ; Thu, 25 Sep 2014 19:53:30 -0700 (PDT) Message-ID: <5424D521.5090305@ozlabs.ru> Date: Fri, 26 Sep 2014 12:53:21 +1000 From: Alexey Kardashevskiy 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> In-Reply-To: <20140926023112.GG24332@voom.redhat.com> 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: David Gibson Cc: Paolo Bonzini , qemu-ppc@nongnu.org, Alexander Graf , qemu-devel@nongnu.org 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. Thanks! -- Alexey