Re: [Qemu-devel] [PATCH 4/9] qemu_qsb.diff

[Stefan Berger <stefanb@linux.vnet.ibm.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 <stefanb@linux.vnet.ibm.com>
+ *
+ * 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