From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57544) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XX5x0-00081s-3k for qemu-devel@nongnu.org; Thu, 25 Sep 2014 06:07:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XX5wt-0001FS-SK for qemu-devel@nongnu.org; Thu, 25 Sep 2014 06:07:02 -0400 Received: from mail-pd0-f182.google.com ([209.85.192.182]:47320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XX5wt-0001EV-Jh for qemu-devel@nongnu.org; Thu, 25 Sep 2014 06:06:55 -0400 Received: by mail-pd0-f182.google.com with SMTP id p10so10210898pdj.41 for ; Thu, 25 Sep 2014 03:06:49 -0700 (PDT) Message-ID: <5423E930.6070704@ozlabs.ru> Date: Thu, 25 Sep 2014 20:06:40 +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> In-Reply-To: <5423E3C6.4070708@suse.de> 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: Alexander Graf , qemu-devel@nongnu.org Cc: Paolo Bonzini , qemu-ppc@nongnu.org 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? >> + } >> } 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