Re: [Qemu-devel] [PATCH 04/17] qcow2: Dirty Bitmaps Ext: structs and consts

[John Snow]

On 09/05/2015 12:43 PM, Vladimir Sementsov-Ogievskiy wrote:
> Add data structures and constraints accordingly to docs/specs/qcow2.txt
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/Makefile.objs        |  2 +-
>  block/qcow2-dirty-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h              | 28 ++++++++++++++++++++++++++++
>  3 files changed, 71 insertions(+), 1 deletion(-)
>  create mode 100644 block/qcow2-dirty-bitmap.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 58ef2ef..c6e1f4b 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -1,5 +1,5 @@
>  block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o
> -block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> qcow2-cache.o
> +block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> qcow2-cache.o qcow2-dirty-bitmap.o
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
> new file mode 100644
> index 0000000..fd4e0ef
> --- /dev/null
> +++ b/block/qcow2-dirty-bitmap.c
> @@ -0,0 +1,42 @@
> +/*
> + * Dirty bitmaps for the QCOW version 2 format
> + *
> + * Copyright (c) 2014-2015 Vladimir Sementsov-Ogievskiy
> + *
> + * This file is derived from qcow2-snapshot.c, original copyright:
> + * Copyright (c) 2004-2006 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * 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 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +/* NOTICE: DBM here means Dirty Bitmap and used as a namespace for _internal_
> + * constants. Please do not use this _internal_ abbreviation for other needs
> + * and/or outside of this file. */
> +
> +/* Dirty Bitmap Directory Enrty constraints */

Entry

> +#define DBM_MAX_TABLE_SIZE 0x8000000
> +#define DBM_MAX_PHYS_SIZE 0x20000000 /* 512 mb */
> +#define DBM_MAX_GRANULARITY_BITS 63
> +#define DBM_MAX_NAME_SIZE 1023
> +

MAX_TABLE_SIZE matches the spec, OK.
MAX_PHYS_SIZE is documented as:

"Also, (dirty_bitmap_table_size * cluster_size) should not be greater
than 0x20000000 (512 MB)"

I might use stricter wording:

"(dirty_bitmap_table_size * cluster_size) must not exceed 0x20000000
(512 MB)"

granularity is fine, but is high like stated in #03.
Eric has also pointed out that NAME_SIZE might not be entirely accurate,
as well.

> +/* Dirty Bitmap Directory Enrty flags */

Entry

> +#define DBM_RESERVED_FLAGS 0xffffffff

What does this one correlate to? The table entry appears to be described
below (ff.....1ff), but the dirty bitmap directory flags appear to
define the lower four bits and no others. what's *this* mask for?

I would expect to see 0xfffffff0 somewhere.

> +
> +/* bits [0, 8] U [56, 63] are reserved */
> +#define DBM_TABLE_ENTRY_RESERVED_MASK 0xff000000000001ff
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 72e1328..a2a5d4a 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -52,6 +52,10 @@
>   * space for snapshot names and IDs */
>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>  
> +/* Dirty Bitmap Header Extension constraints */
> +#define QCOW_MAX_DIRTY_BITMAPS 65536
> +#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
> +

Matches spec.

>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1ULL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) 
> */
> @@ -141,6 +145,19 @@ typedef struct QEMU_PACKED QCowSnapshotHeader {
>      /* name follows  */
>  } QCowSnapshotHeader;
>  
> +typedef struct QEMU_PACKED QCowDirtyBitmapHeader {
> +    /* header is 8 byte aligned */
> +    uint64_t dirty_bitmap_table_offset;
> +    uint64_t nb_virtual_bits;
> +
> +    uint32_t dirty_bitmap_table_size;
> +    uint32_t granularity_bits;
> +
> +    uint32_t flags;
> +    uint16_t name_size;
> +    /* name follows  */

Is it against our style to put zero-length arrays in structures? It's a
habit I got into, but maybe it's not appreciated in QEMU.

> +} QCowDirtyBitmapHeader;
> +

This is called the "header" here -- but it's the header for the Dirty
Bitmap Directory. I was confused for a second, no big deal.

Matches spec, though.

>  typedef struct QEMU_PACKED QCowSnapshotExtraData {
>      uint64_t vm_state_size_large;
>      uint64_t disk_size;
> @@ -159,6 +176,11 @@ typedef struct QCowSnapshot {
>      uint64_t vm_clock_nsec;
>  } QCowSnapshot;
>  
> +typedef struct QCowDirtyBitmap {
> +    uint64_t offset;
> +    char *name;
> +} QCowDirtyBitmap;
> +
>  struct Qcow2Cache;
>  typedef struct Qcow2Cache Qcow2Cache;
>  
> @@ -221,6 +243,12 @@ typedef uint64_t Qcow2GetRefcountFunc(const void 
> *refcount_array,
>  typedef void Qcow2SetRefcountFunc(void *refcount_array,
>                                    uint64_t index, uint64_t value);
>  
> +typedef struct Qcow2DirtyBitmapHeaderExt {
> +    uint32_t nb_dirty_bitmaps;
> +    uint32_t dirty_bitmap_directory_size;
> +    uint64_t dirty_bitmap_directory_offset;
> +} QEMU_PACKED Qcow2DirtyBitmapHeaderExt;
> +

Matches spec.

>  typedef struct BDRVQcowState {
>      int cluster_bits;
>      int cluster_size;
> 

Needs a trivial rebase due to missing context in qcow2.h, but otherwise
it appears OK.