From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49471) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFupl-0001l8-Rs for qemu-devel@nongnu.org; Wed, 13 Mar 2013 19:11:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UFupi-00067o-LR for qemu-devel@nongnu.org; Wed, 13 Mar 2013 19:11:45 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:48534) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFupi-00067j-HU for qemu-devel@nongnu.org; Wed, 13 Mar 2013 19:11:42 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 13 Mar 2013 19:11:41 -0400 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 2ACE6C9001D for ; Wed, 13 Mar 2013 19:11:39 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2DNBcmh30802026 for ; Wed, 13 Mar 2013 19:11:38 -0400 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2DNBcAl032374 for ; Wed, 13 Mar 2013 20:11:38 -0300 Message-ID: <514107A9.5040309@linux.vnet.ibm.com> Date: Wed, 13 Mar 2013 19:11:37 -0400 From: Stefan Berger MIME-Version: 1.0 References: <1363200988-17865-1-git-send-email-jschopp@linux.vnet.ibm.com> <1363200988-17865-5-git-send-email-jschopp@linux.vnet.ibm.com> <20130313211135.GB6188@vm> <5140EF98.8020307@linux.vnet.ibm.com> <20130313224133.GH4005@vm> <20130313224730.GI4005@vm> In-Reply-To: <20130313224730.GI4005@vm> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/9] qemu_qsb.diff List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mdroth Cc: Joel Schopp , qemu-devel@nongnu.org, quintela@redhat.com On 03/13/2013 06:47 PM, mdroth wrote: > On Wed, Mar 13, 2013 at 05:41:33PM -0500, mdroth wrote: >> On Wed, Mar 13, 2013 at 05:28:56PM -0400, Stefan Berger wrote: >>> On 03/13/2013 05:11 PM, mdroth wrote: >>>> On Wed, Mar 13, 2013 at 01:56:23PM -0500, Joel Schopp wrote: >>>>> This patch adds support functions for operating on in memory sized file buffers. >>>> There's been some past refactorings to remove non-migration users of >>>> QEMUFile, and AFAIK that's still the case today. QEMUFile satisfies >>>> funky requirements like rate-limiting, buffering, etc that were specific >>>> to migration. >>>> >>>> IIUC all we want here is an abstraction on top of write()/memcpy(), >>>> and access to qemu_{put|get}_be* utility functions. >>>> >>>> Have you considered rolling those abstractions in the visitor >>>> implementations as opposed to extending QEMUFile, and using >>>> be*_to_cpus/cpus_to_be* helpers directly instead (like block/qcow2.c >>>> does, for example)? >>> The advantage of using the QEMUFile abstractions is that now you can >>> build a visitor on top of it and read from buffers, sockets, BDRV's >>> (later on), plain files, and whatever else you can hide underneath >>> that interface. Back in 2011 when I initially wrote this code there >> Maybe a case can be made for making it a general utility library, but >> I'm having a hard time thinking of any reasons that aren't specific to >> migration, and I wonder if it's even necessary now that we have a >> migration thread that can handle the rate-limiting/buffering >> considerations. >> >> But I'm not sure exactly why we decided to drop non-migration users, so >> I think it's worth clarifying before we attempt to tether another >> component to it. It almost sounds like there is a lock on QEMUFile forbidding it to be used for anything else.. strange. >>> that we later want to use the visitors for writing into virtual >>> NVRAM, which we would build on top of a QEMUFile wrapping BDRVs. So >>> there are some immediate advantages of using the common QEMUFile >>> interface for reading and writing of data from different types of >>> sources. >> Can you describe the requirements for the BDRV wrapper a bit more? >> Everything else seems reasonably doable via visitor internals but >> maybe there's more to it I'm not considering. The vNVRAM would be used for writing persistent state of the software TPM into. The vNVRAM uses the block devices and with that we get the benefit of encryption (QCOW2) and migration. The contents of the vNVRAM are layed out as one large ASN.1 stream. At the beginning is the header, then come the individual blobs, which may each be of different size and each is identified by a unique name. When reading a certain blob (given its name), we use the input visitor to scan through the BDRV to find the blob and then read it (using the sized buffer input visitor). When writing we scan for the blob and switch from input visitor to output visitor to replace the blob. The requirement here is of course that the blob maintains its size so that the subsequent blobs are still part of that ASN.1 stream and don't destroy the integrity of the ASN.1 stream. Code talks, so here's the hunk of the patch for BDRV support as it stands right now. The header may need some changes, but that's not the point. Index: qemu-git.pt/qemu-bdrv-file.c =================================================================== --- /dev/null +++ qemu-git.pt/qemu-bdrv-file.c @@ -0,0 +1,116 @@ +/* + * QEMU File : wrapper for bdrv + * + * Copyright (c) 2011, 2012, 2013 IBM Corporation + * + * Authors: + * Stefan Berger + * + * Permission is hereby granted, free of charge, to any person obtaining a cop + * of this software and associated documentation files (the "Software"), to de + * in the Software without restriction, including without limitation the right + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FRO + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +/* + * Note: This QEMUFile wrapper introduces a dependency on the block layer. + * We keep it in this separate file to avoid pulling block.o and its + * dependencies into test programs. + */ + +#include "qemu-common.h" +#include "hw/hw.h" +#include "block/block.h" + +typedef struct QEMUBdrvFile { + BlockDriverState *bds; + uint64_t offset; + QEMUFile *file; +} QEMUBdrvFile; + +static int qf_bdrv_put_buffer(void *opaque, const uint8_t *buf, + int64_t pos, int size) +{ + QEMUBdrvFile *s = opaque; + + return bdrv_pwrite(s->bds, pos + s->offset, buf, size); +} + +static int qf_bdrv_get_buffer(void *opaque, uint8_t *buf, + int64_t pos, int size) +{ + QEMUBdrvFile *s = opaque; + ssize_t len = bdrv_getlength(s->bds) - (s->offset + pos); + + if (len <= 0) { + return 0; + } + + if (len > size) { + len = size; + } + + len = bdrv_pread(s->bds, pos + s->offset, buf, len); + + return len; +} + +static int qf_bdrv_close(void *opaque) +{ + QEMUBdrvFile *s = opaque; + + g_free(s); + + return 0; +} + +static const QEMUFileOps bdrv_read_ops = { + .get_buffer = qf_bdrv_get_buffer, + .close = qf_bdrv_close +}; + +static const QEMUFileOps bdrv_write_ops = { + .put_buffer = qf_bdrv_put_buffer, + .close = qf_bdrv_close +}; + + +QEMUFile *qemu_bdrv_open(const char *mode, BlockDriverState *bds, + uint64_t offset) +{ + QEMUBdrvFile *s; + + if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) { + fprintf(stderr, "qemu_bdrv_open: Argument validity check failed\n"); + return NULL; + } + + s = g_malloc0(sizeof(QEMUBdrvFile)); + if (!s) { + return NULL; + } + + s->bds = bds; + s->offset = offset; + + if (mode[0] == 'r') { + s->file = qemu_fopen_ops(s, &bdrv_read_ops); + } else { + s->file = qemu_fopen_ops(s, &bdrv_write_ops); + } + return s->file; +} + Turns out this QEMUFile abstraction can be used for more than just migration... Regards, Stefan