Re: [Qemu-block] [PATCH v2 1/3] block/qcow2: refactoring of threaded encryption code
06.09.2019 22:57, Maxim Levitsky wrote:
> This commit tries to clarify few function arguments,
> and add comments describing the encrypt/decrypt interface
>
> Signed-off-by: Maxim Levitsky
> ---
> block/qcow2-cluster.c | 10 +++
> block/qcow2-threads.c | 61 ++-
> 2 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f09cc992af..1989b423da 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -463,8 +463,8 @@ static int coroutine_fn
> do_perform_cow_read(BlockDriverState *bs,
> }
>
> static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> -uint64_t src_cluster_offset,
> -uint64_t cluster_offset,
> +uint64_t
> guest_cluster_offset,
> +uint64_t host_cluster_offset,
> unsigned offset_in_cluster,
> uint8_t *buffer,
> unsigned bytes)
> @@ -474,8 +474,8 @@ static bool coroutine_fn
> do_perform_cow_encrypt(BlockDriverState *bs,
> assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> assert(s->crypto);
> -if (qcow2_co_encrypt(bs, cluster_offset,
> - src_cluster_offset + offset_in_cluster,
> +if (qcow2_co_encrypt(bs, host_cluster_offset,
> + guest_cluster_offset + offset_in_cluster,
>buffer, bytes) < 0) {
> return false;
> }
> @@ -496,7 +496,7 @@ static int coroutine_fn
> do_perform_cow_write(BlockDriverState *bs,
> }
>
> ret = qcow2_pre_write_overlap_check(bs, 0,
> -cluster_offset + offset_in_cluster, qiov->size, true);
> + cluster_offset + offset_in_cluster, qiov->size, true);
Hmm, unrelated hunk.
> if (ret < 0) {
> return ret;
> }
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..c3cda0c6a5 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -234,15 +234,19 @@ static int qcow2_encdec_pool_func(void *opaque)
> }
>
> static int coroutine_fn
> -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> - uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc
> func)
> +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
> +uint64_t guest_offset, void *buf, size_t len,
> +Qcow2EncDecFunc func)
> {
> BDRVQcow2State *s = bs->opaque;
> +
> +uint64_t offset = s->crypt_physical_offset ?
> +host_cluster_offset + offset_into_cluster(s, guest_offset) :
> +guest_offset;
> +
> Qcow2EncDecData arg = {
> .block = s->crypto,
> -.offset = s->crypt_physical_offset ?
> - file_cluster_offset + offset_into_cluster(s, offset) :
> - offset,
> +.offset = offset,
> .buf = buf,
> .len = len,
> .func = func,
> @@ -251,18 +255,49 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t
> file_cluster_offset,
> return qcow2_co_process(bs, qcow2_encdec_pool_func, );
> }
>
> +
> +/*
> + * qcow2_co_encrypt()
> + *
> + * Encrypts one or more contiguous aligned sectors
> + *
> + * @host_cluster_offset - on disk offset of the first cluster in which
> + * the encrypted data will be written
It's not quite right, it's not on disk, but on .file child of qcow2 node, which
may be any other format or protocol node.. So, I called it file_cluster_offset.
But I'm OK with new naming anyway. And it may be better for encryption related
logic..
> + * Used as an initialization vector for encryption
Hmm, is it default now?
> + *
> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted
Hmm, stop. It's wrong. Data to be encrypted is in buffer, so, it's not first
sector of
the data to be encrypted, but first sector in which guest writes data (to be
encrypted
in meantime).
> + * Used as an initialization vector for older, qcow2 native encryption
> + *
> + * @buf - buffer with the data to encrypt
> + * @len - length of the buffer (in sector size multiplies)
> + *
> + * Note that the group of the sectors, don't have to be aligned
> + * on cluster boundary and can also cross a cluster boundary.
And I doubt in it now. I'm afraid that if we call qcow2_co_encrypt for a group
of the sectors crossing a cluster boundary, we will finish up with similar bug:
we'll
use first cluster offset as a vector for all the sectors. We still never do
it.. So,
I think it worth assertion and corresponding
[Qemu-block] [PATCH v7 0/4] virtio/block: handle zoned backing devices
Currently, attaching zoned block devices (i.e., storage devices compliant to ZAC/ZBC standards) using several virtio methods doesn't work properly as zoned devices appear as regular block devices at the guest. This may cause unexpected i/o errors and, potentially, some data corruption. To be more precise, attaching a zoned device via virtio-pci-blk, virtio-scsi-pci/scsi-disk or virtio-scsi-pci/scsi-hd demonstrates the above behavior. The virtio-scsi-pci/scsi-block method works with a recent patch. The virtio-scsi-pci/scsi-generic method also appears to handle zoned devices without problems. This patch set adds code to check if the backing device that is being opened is a zoned Host Managed device. If this is the case, the patch prohibits attaching such device for all use cases lacking proper zoned support. Host Aware zoned block devices are designed to work as regular block devices at a guest system that does not support ZBD. Therefore, this patch set doesn't prohibit attachment of Host Aware devices. Considering that there is still a couple of different working ways to attach a ZBD, this patch set provides a reasonable short-term solution for this problem. ZBD support for virtio-scsi-pci/scsi-disk and virtio-scsi-pci/scsi-hd does not seem as necessary. Users will be expected to attach zoned block devices via virtio-scsi-pci/scsi-block instead. This patch set contains some Linux-specific code. This code is necessary to obtain Zoned Block Device model value from Linux sysfs. History: v1 -> v2: - rework code to be permission-based - always allow Host Aware devices to be attached - add fix for Host Aware attachments aka RCAP output snoop v2 -> v3: - drop the patch for RCAP output snoop - merged separately v3 -> v4: - rebase to the current code v4 -> v5: - avoid checkpatch warning v5 -> v6: - address review comments from Stefan Hajnoczi v6 -> v7: - address review comment from Stefano Garzarella * fix BLK_PERM_ALL mask * add BLK_PERM_SUPPORT_HM_ZONED permission to QAPI schema * add a text name for BLK_PERM_SUPPORT_HM_ZONED permission Dmitry Fomichev (4): block: Add zoned device model property raw: Recognize zoned backing devices block/ide/scsi: Set BLK_PERM_SUPPORT_HM_ZONED raw: Don't open ZBDs if backend can't handle them block.c | 37 +++- block/file-posix.c| 89 +-- block/io.c| 5 +++ hw/block/block.c | 6 ++- hw/block/fdc.c| 5 ++- hw/block/nvme.c | 2 +- hw/block/virtio-blk.c | 2 +- hw/block/xen-block.c | 2 +- hw/ide/qdev.c | 2 +- hw/scsi/scsi-disk.c | 13 +++--- hw/scsi/scsi-generic.c| 2 +- hw/usb/dev-storage.c | 2 +- include/block/block.h | 21 - include/block/block_int.h | 3 ++ include/hw/block/block.h | 3 +- qapi/block-core.json | 5 ++- 16 files changed, 157 insertions(+), 42 deletions(-) -- 2.21.0
[Qemu-block] [PATCH v7 1/4] block: Add zoned device model property
This commit adds Zoned Device Model (as defined in T10 ZBC and
T13 ZAC standards) as a block driver property, along with some
useful access functions.
A new backend driver permission, BLK_PERM_SUPPORT_HM_ZONED, is also
introduced. Only the drivers having this permission will be allowed
to open host managed zoned block devices.
No code is added yet to initialize or check the value of this new
property, therefore this commit doesn't change any functionality.
Signed-off-by: Dmitry Fomichev
Reviewed-by: Stefan Hajnoczi
---
block.c | 37 +++--
include/block/block.h | 21 +++--
include/block/block_int.h | 3 +++
qapi/block-core.json | 5 -
4 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/block.c b/block.c
index 5944124845..f0390196f2 100644
--- a/block.c
+++ b/block.c
@@ -1968,11 +1968,12 @@ char *bdrv_perm_names(uint64_t perm)
uint64_t perm;
const char *name;
} permissions[] = {
-{ BLK_PERM_CONSISTENT_READ, "consistent read" },
-{ BLK_PERM_WRITE, "write" },
-{ BLK_PERM_WRITE_UNCHANGED, "write unchanged" },
-{ BLK_PERM_RESIZE, "resize" },
-{ BLK_PERM_GRAPH_MOD, "change children" },
+{ BLK_PERM_CONSISTENT_READ, "consistent read" },
+{ BLK_PERM_WRITE,"write" },
+{ BLK_PERM_WRITE_UNCHANGED, "write unchanged" },
+{ BLK_PERM_RESIZE, "resize" },
+{ BLK_PERM_GRAPH_MOD,"change children" },
+{ BLK_PERM_SUPPORT_HM_ZONED, "attach hm-zoned" },
{ 0, NULL }
};
@@ -4678,6 +4679,21 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t
*nb_sectors_ptr)
*nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
}
+BdrvZonedModel bdrv_get_zoned_model(BlockDriverState *bs)
+{
+return bs->bl.zoned_model;
+}
+
+bool bdrv_is_hm_zoned(BlockDriverState *bs)
+{
+/*
+ * Host Aware zone devices are supposed to be able to work
+ * just like regular block devices. Thus, we only consider
+ * Host Managed devices to be zoned here.
+ */
+return bdrv_get_zoned_model(bs) == BDRV_ZONED_MODEL_HM;
+}
+
bool bdrv_is_sg(BlockDriverState *bs)
{
return bs->sg;
@@ -4871,11 +4887,12 @@ static void
xdbg_graph_add_edge(XDbgBlockGraphConstructor *gr, void *parent,
} PermissionMap;
static const PermissionMap permissions[] = {
-{ BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
-{ BLK_PERM_WRITE, BLOCK_PERMISSION_WRITE },
-{ BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
-{ BLK_PERM_RESIZE, BLOCK_PERMISSION_RESIZE },
-{ BLK_PERM_GRAPH_MOD, BLOCK_PERMISSION_GRAPH_MOD },
+{ BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
+{ BLK_PERM_WRITE,BLOCK_PERMISSION_WRITE },
+{ BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
+{ BLK_PERM_RESIZE, BLOCK_PERMISSION_RESIZE },
+{ BLK_PERM_GRAPH_MOD,BLOCK_PERMISSION_GRAPH_MOD },
+{ BLK_PERM_SUPPORT_HM_ZONED, BLOCK_PERMISSION_SUPPORT_HM_ZONED },
{ 0, 0 }
};
const PermissionMap *p;
diff --git a/include/block/block.h b/include/block/block.h
index 124ad40809..46cfa5bfa9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -271,18 +271,33 @@ enum {
*/
BLK_PERM_GRAPH_MOD = 0x10,
-BLK_PERM_ALL= 0x1f,
+/**
+ * This permission is required to open host-managed zoned block devices.
+ */
+BLK_PERM_SUPPORT_HM_ZONED = 0x20,
+
+BLK_PERM_ALL= 0x3f,
DEFAULT_PERM_PASSTHROUGH= BLK_PERM_CONSISTENT_READ
| BLK_PERM_WRITE
| BLK_PERM_WRITE_UNCHANGED
- | BLK_PERM_RESIZE,
+ | BLK_PERM_RESIZE
+ | BLK_PERM_SUPPORT_HM_ZONED,
DEFAULT_PERM_UNCHANGED = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
};
char *bdrv_perm_names(uint64_t perm);
+/*
+ * Known zoned device models.
+ */
+typedef enum {
+BDRV_ZONED_MODEL_NONE, /* Regular block device */
+BDRV_ZONED_MODEL_HA, /* Host-aware zoned block device */
+BDRV_ZONED_MODEL_HM, /* Host-managed zoned block device */
+} BdrvZonedModel;
+
/* disk I/O throttling */
void bdrv_init(void);
void bdrv_init_with_whitelist(void);
@@ -359,6 +374,8 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
BlockDriverState *in_bs, Error **errp);
void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
+BdrvZonedModel bdrv_get_zoned_model(BlockDriverState *bs);
+bool bdrv_is_hm_zoned(BlockDriverState *bs);
void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
[Qemu-block] [PATCH v7 2/4] raw: Recognize zoned backing devices
The purpose of this patch is to recognize a zoned block device (ZBD)
when it is opened as a raw file. The new code initializes the zoned
model propery introduced by the previous commit.
This commit is Linux-specific as it gets the Zoned Block Device Model
value (none/host-managed/host-aware) from sysfs on the host.
In order to avoid code duplication in file-posix.c, a common helper
function is added to read values of sysfs entries under
/sys/block//queue. This way, the existing function that reads
the value of "max_segments" entry and the the new function that reads
"zoned" value both share the same helper code.
Signed-off-by: Dmitry Fomichev
Reviewed-by: Stefan Hajnoczi
---
block/file-posix.c | 75 ++
block/io.c | 5
2 files changed, 67 insertions(+), 13 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 71f168ee2f..caacb21f07 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1067,15 +1067,13 @@ static int sg_get_max_transfer_length(int fd)
#endif
}
-static int sg_get_max_segments(int fd)
+static int hdev_read_blk_queue_entry(int fd, const char *key,
+char *buf, int buf_len)
{
#ifdef CONFIG_LINUX
-char buf[32];
-const char *end;
char *sysfspath = NULL;
int ret;
int sysfd = -1;
-long max_segments;
struct stat st;
if (fstat(fd, )) {
@@ -1083,23 +1081,45 @@ static int sg_get_max_segments(int fd)
goto out;
}
-sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
-major(st.st_rdev), minor(st.st_rdev));
+sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
+major(st.st_rdev), minor(st.st_rdev), key);
sysfd = open(sysfspath, O_RDONLY);
if (sysfd == -1) {
ret = -errno;
goto out;
}
do {
-ret = read(sysfd, buf, sizeof(buf) - 1);
+ret = read(sysfd, buf, buf_len - 1);
} while (ret == -1 && errno == EINTR);
if (ret < 0) {
ret = -errno;
-goto out;
} else if (ret == 0) {
ret = -EIO;
+}
+out:
+if (sysfd != -1) {
+close(sysfd);
+}
+g_free(sysfspath);
+return ret;
+#else
+return -ENOTSUP;
+#endif
+}
+
+static int sg_get_max_segments(int fd)
+{
+#ifdef CONFIG_LINUX
+char buf[32];
+const char *end;
+int ret;
+long max_segments;
+
+ret = hdev_read_blk_queue_entry(fd, "max_segments", buf, sizeof(buf));
+if (ret < 0) {
goto out;
}
+
buf[ret] = 0;
/* The file is ended with '\n', pass 'end' to accept that. */
ret = qemu_strtol(buf, , 10, _segments);
@@ -1108,22 +1128,45 @@ static int sg_get_max_segments(int fd)
}
out:
-if (sysfd != -1) {
-close(sysfd);
-}
-g_free(sysfspath);
return ret;
#else
return -ENOTSUP;
#endif
}
+static BdrvZonedModel hdev_get_zoned_model(int fd)
+{
+#ifdef CONFIG_LINUX
+char buf[32];
+BdrvZonedModel zm = BDRV_ZONED_MODEL_NONE;
+int ret;
+
+ret = hdev_read_blk_queue_entry(fd, "zoned", buf, sizeof(buf));
+if (ret < 0) {
+goto out;
+}
+
+buf[ret - 1] = '\0'; /* replace the newline character with NULL */
+if (strcmp(buf, "host-managed") == 0) {
+zm = BDRV_ZONED_MODEL_HM;
+} else if (strcmp(buf, "host-aware") == 0) {
+zm = BDRV_ZONED_MODEL_HA;
+}
+
+out:
+return zm;
+#else
+return BDRV_ZONED_MODEL_NONE;
+#endif
+}
+
static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
{
BDRVRawState *s = bs->opaque;
+int ret;
if (bs->sg) {
-int ret = sg_get_max_transfer_length(s->fd);
+ret = sg_get_max_transfer_length(s->fd);
if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
bs->bl.max_transfer = pow2floor(ret);
@@ -1133,6 +1176,12 @@ static void raw_refresh_limits(BlockDriverState *bs,
Error **errp)
if (ret > 0) {
bs->bl.max_transfer = MIN(bs->bl.max_transfer, ret *
getpagesize());
}
+
+}
+
+ret = hdev_get_zoned_model(s->fd);
+if (ret >= 0) {
+bs->bl.zoned_model = ret;
}
raw_probe_alignment(bs, s->fd, errp);
diff --git a/block/io.c b/block/io.c
index 0fa10831ed..147c320061 100644
--- a/block/io.c
+++ b/block/io.c
@@ -157,6 +157,11 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error
**errp)
return;
}
bdrv_merge_limits(>bl, >file->bs->bl);
+
+/* Propagate zoned model */
+if (!bs->probed) {
+bs->bl.zoned_model = bs->file->bs->bl.zoned_model;
+}
} else {
bs->bl.min_mem_alignment = 512;
bs->bl.opt_mem_alignment = getpagesize();
--
2.21.0
[Qemu-block] [PATCH v7 4/4] raw: Don't open ZBDs if backend can't handle them
Abort opening a zoned device as a raw file in case the chosen
block backend driver lacks proper support for this type of
storage.
Signed-off-by: Dmitry Fomichev
Reviewed-by: Stefan Hajnoczi
---
block/file-posix.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index caacb21f07..3de371a97d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2934,6 +2934,20 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t
perm, uint64_t shared,
goto fail;
}
}
+
+/*
+ * If we are opening a zoned block device, check if the backend
+ * driver can properly handle host-managed devices, abort if not.
+ */
+if (bdrv_is_hm_zoned(bs) &&
+!(shared & BLK_PERM_SUPPORT_HM_ZONED) &&
+!(perm & BLK_PERM_SUPPORT_HM_ZONED)) {
+error_setg(errp,
+ "block backend driver doesn't support host-managed zoned
devices");
+ret = -ENOTSUP;
+goto fail;
+}
+
return 0;
fail:
--
2.21.0
[Qemu-block] [PATCH v7 3/4] block/ide/scsi: Set BLK_PERM_SUPPORT_HM_ZONED
Added a new boolean argument to blkconf_apply_backend_options()
to let the common block code know whether the chosen block
backend can handle host managed zoned block devices.
blkconf_apply_backend_options() then sets BLK_PERM_SUPPORT_HM_ZONED
permission accordingly. The raw code can then use this permission
to allow or deny opening a zone device by a particular block driver.
Signed-off-by: Dmitry Fomichev
Acked-by: Paul Durrant
Reviewed-by: Stefan Hajnoczi
---
hw/block/block.c | 6 +-
hw/block/fdc.c | 5 +++--
hw/block/nvme.c | 2 +-
hw/block/virtio-blk.c| 2 +-
hw/block/xen-block.c | 2 +-
hw/ide/qdev.c| 2 +-
hw/scsi/scsi-disk.c | 13 +++--
hw/scsi/scsi-generic.c | 2 +-
hw/usb/dev-storage.c | 2 +-
include/hw/block/block.h | 3 ++-
10 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index bf56c7612b..1c6019eee7 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -86,7 +86,8 @@ void blkconf_blocksizes(BlockConf *conf)
}
bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
- bool resizable, Error **errp)
+ bool resizable, bool zoned_support,
+ Error **errp)
{
BlockBackend *blk = conf->blk;
BlockdevOnError rerror, werror;
@@ -98,6 +99,9 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool
readonly,
if (!readonly) {
perm |= BLK_PERM_WRITE;
}
+if (zoned_support) {
+perm |= BLK_PERM_SUPPORT_HM_ZONED;
+}
shared_perm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
BLK_PERM_GRAPH_MOD;
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index ac5d31e8c1..673a8b39bc 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -477,7 +477,7 @@ static void fd_change_cb(void *opaque, bool load, Error
**errp)
} else {
if (!blkconf_apply_backend_options(drive->conf,
blk_is_read_only(drive->blk), false,
- errp)) {
+ false, errp)) {
return;
}
}
@@ -569,7 +569,8 @@ static void floppy_drive_realize(DeviceState *qdev, Error
**errp)
dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO;
dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
-if (!blkconf_apply_backend_options(>conf, read_only, false, errp)) {
+if (!blkconf_apply_backend_options(>conf, read_only, false, false,
+ errp)) {
return;
}
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 12d8254250..07f08d0768 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1334,7 +1334,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
}
blkconf_blocksizes(>conf);
if (!blkconf_apply_backend_options(>conf, blk_is_read_only(n->conf.blk),
- false, errp)) {
+ false, false, errp)) {
return;
}
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 18851601cb..8be62903e2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1127,7 +1127,7 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
if (!blkconf_apply_backend_options(>conf,
blk_is_read_only(conf->conf.blk), true,
- errp)) {
+ false, errp)) {
return;
}
s->original_wce = blk_enable_write_cache(conf->conf.blk);
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index f77343db60..57fe970908 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -229,7 +229,7 @@ static void xen_block_realize(XenDevice *xendev, Error
**errp)
}
if (!blkconf_apply_backend_options(conf, blockdev->info & VDISK_READONLY,
- true, errp)) {
+ true, false, errp)) {
return;
}
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6fba6b62b8..a57a8f1a8f 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -200,7 +200,7 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind
kind, Error **errp)
}
}
if (!blkconf_apply_backend_options(>conf, kind == IDE_CD,
- kind != IDE_CD, errp)) {
+ kind != IDE_CD, false, errp)) {
return;
}
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 915641a0f1..8a57caafd7 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2318,7 +2318,7 @@ static void scsi_disk_unit_attention_reported(SCSIDevice
*dev)
}
}
-static void scsi_realize(SCSIDevice *dev, Error **errp)
+static void scsi_realize(SCSIDevice *dev, bool zoned_support, Error
Re: [Qemu-block] [PATCH v9 7/9] scsi: account unmap operations
06.09.2019 19:01, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov
Reviewed-by: Vladimir Sementsov-Ogievskiy
> ---
> hw/scsi/scsi-disk.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index a002fdabe8..68b1675fd9 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1617,10 +1617,16 @@ static void scsi_unmap_complete_noio(UnmapCBData
> *data, int ret)
> r->sector_count = (ldl_be_p(>inbuf[8]) & 0xULL)
> * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> if (!check_lba_range(s, r->sector, r->sector_count)) {
> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
> + BLOCK_ACCT_UNMAP);
> scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
> goto done;
> }
>
> +block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
> + r->sector_count * BDRV_SECTOR_SIZE,
> + BLOCK_ACCT_UNMAP);
> +
> r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
> r->sector * BDRV_SECTOR_SIZE,
> r->sector_count * BDRV_SECTOR_SIZE,
> @@ -1647,10 +1653,11 @@ static void scsi_unmap_complete(void *opaque, int ret)
> r->req.aiocb = NULL;
>
> aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> -if (scsi_disk_req_check_error(r, ret, false)) {
> +if (scsi_disk_req_check_error(r, ret, true)) {
> scsi_req_unref(>req);
> g_free(data);
> } else {
> +block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
> scsi_unmap_complete_noio(data, ret);
> }
> aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> @@ -1682,6 +1689,7 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r,
> uint8_t *inbuf)
> }
>
> if (blk_is_read_only(s->qdev.conf.blk)) {
> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
> BLOCK_ACCT_UNMAP);
> scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
> return;
> }
> @@ -1697,10 +1705,12 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r,
> uint8_t *inbuf)
> return;
>
> invalid_param_len:
> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
> scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
> return;
>
> invalid_field:
> +block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
> scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
> }
>
>
--
Best regards,
Vladimir
[Qemu-block] [PATCH] nbd/client: Add hint when TLS is missing
I received an off-list report of failure to connect to an NBD server expecting an x509 certificate, when the client was attempting something similar to this command line: $ ./x86_64-softmmu/qemu-system-x86_64 -name 'blah' -machine q35 -nodefaults \ -object tls-creds-x509,id=tls0,endpoint=client,dir=$path_to_certs \ -device virtio-scsi-pci,id=virtio_scsi_pci0,bus=pcie.0,addr=0x6 \ -drive id=drive_image1,if=none,snapshot=off,aio=threads,cache=none,format=raw,file=nbd:localhost:9000,werror=stop,rerror=stop,tls-creds=tls0 \ -device scsi-hd,id=image1,drive=drive_image1,bootindex=0 qemu-system-x86_64: -drive id=drive_image1,if=none,snapshot=off,aio=threads,cache=none,format=raw,file=nbd:localhost:9000,werror=stop,rerror=stop,tls-creds=tls0: TLS negotiation required before option 7 (go) server reported: Option 0x7 not permitted before TLS The problem? As specified, -drive is trying to pass tls-creds to the raw format driver instead of the nbd protocol driver, but before we get to the point where we can detect that raw doesn't know what to do with tls-creds, the nbd driver has already failed because the server complained. The fix to the broken command line? Pass '...,file.tls-creds=tls0' to ensure the tls-creds option is handed to nbd, not raw. But since the error message was rather cryptic, I'm trying to improve the error message. With this patch, the error message adds a line: qemu-system-x86_64: -drive id=drive_image1,if=none,snapshot=off,aio=threads,cache=none,format=raw,file=nbd:localhost:9000,werror=stop,rerror=stop,tls-creds=tls0: TLS negotiation required before option 7 (go) Did you forget a valid tls-creds? server reported: Option 0x7 not permitted before TLS And with luck, someone grepping for that error message will find this commit message and figure out their command line mistake. Sadly, the only mention of file.tls-creds in our docs relates to an --image-opts use of PSK encryption with qemu-img as the client, rather than x509 certificate encryption with qemu-kvm as the client. CC: Tingting Mao CC: Daniel P. Berrangé Signed-off-by: Eric Blake --- nbd/client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/nbd/client.c b/nbd/client.c index b9dc829175f9..f6733962b49b 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -204,6 +204,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply, case NBD_REP_ERR_TLS_REQD: error_setg(errp, "TLS negotiation required before option %" PRIu32 " (%s)", reply->option, nbd_opt_lookup(reply->option)); +error_append_hint(errp, "Did you forget a valid tls-creds?\n"); break; case NBD_REP_ERR_UNKNOWN: -- 2.21.0
Re: [Qemu-block] [PATCH v2 2/3] block/qcow2: fix the corruption when rebasing luks encrypted files
06.09.2019 22:57, Maxim Levitsky wrote:
> This fixes subtle corruption introduced by luks threaded encryption
> in commit 8ac0f15f335
My fault, I'm sorry :( And great thanks for investigating this!
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
>
> The corruption happens when we do a write that
> * writes to two or more unallocated clusters at once
> * doesn't fully cover the first sector
> * doesn't fully cover the last sector
>
> In this case, when allocating the new clusters we COW both areas
> prior to the write and after the write, and we encrypt them.
>
> The above mentioned commit accidentally made it so we encrypt the
> second COW area using the physical cluster offset of the first area.
>
> Fix this by:
> * Remove the offset_in_cluster parameter of do_perform_cow_encrypt,
> since it is misleading. That offset can be larger than cluster size
> currently.
Ohh, tricky thing. For sure I missed this logic.
>
> Instead just add the start and the end COW area offsets to both host
> and guest offsets that do_perform_cow_encrypt receives.
>
> * in do_perform_cow_encrypt, remove the cluster offset from the host_offset,
> and thus pass correctly to the qcow2_co_encrypt, the host cluster offset
> and full guest offset
>
> In the bugreport that was triggered by rebasing a luks image to new,
> zero filled base, which lot of such writes, and causes some files
> with zero areas to contain garbage there instead.
> But as described above it can happen elsewhere as well
>
>
> Signed-off-by: Maxim Levitsky
Reviewed-by: Vladimir Sementsov-Ogievskiy
> ---
> block/qcow2-cluster.c | 28
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 1989b423da..6df17dd296 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -463,20 +463,20 @@ static int coroutine_fn
> do_perform_cow_read(BlockDriverState *bs,
> }
>
> static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> -uint64_t
> guest_cluster_offset,
> -uint64_t host_cluster_offset,
> -unsigned offset_in_cluster,
> +uint64_t guest_offset,
> +uint64_t host_offset,
> uint8_t *buffer,
> unsigned bytes)
> {
> if (bytes && bs->encrypted) {
> BDRVQcow2State *s = bs->opaque;
> -assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> -assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> +
> +assert(QEMU_IS_ALIGNED(guest_offset | host_offset | bytes,
> + BDRV_SECTOR_SIZE));
> assert(s->crypto);
> -if (qcow2_co_encrypt(bs, host_cluster_offset,
> - guest_cluster_offset + offset_in_cluster,
> - buffer, bytes) < 0) {
> +
> +if (qcow2_co_encrypt(bs, start_of_cluster(s, host_offset),
> + guest_offset, buffer, bytes) < 0) {
> return false;
> }
> }
> @@ -890,11 +890,15 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta
> *m)
>
> /* Encrypt the data if necessary before writing it */
> if (bs->encrypted) {
> -if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -start->offset, start_buffer,
> +if (!do_perform_cow_encrypt(bs,
> +m->offset + start->offset,
> +m->alloc_offset + start->offset,
> +start_buffer,
> start->nb_bytes) ||
> -!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -end->offset, end_buffer, end->nb_bytes))
> {
> +!do_perform_cow_encrypt(bs,
> +m->offset + end->offset,
> +m->alloc_offset + end->offset,
> +end_buffer, end->nb_bytes)) {
> ret = -EIO;
> goto fail;
> }
>
--
Best regards,
Vladimir
