it is fixed now by my qtest patches.
The iotest 67 still fails with this, something that Paulo
is looking to fix before merging this.
Best regards,
Maxim Levitsky
Maxim Levitsky (10):
qtest: rename qtest_qmp_receive to qtest_qmp_receive_dict
qtest: Reintroduce qtest_qmp_receive
qtest
-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-6-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/core/qdev.c | 19 ++-
include/hw/qdev-core.h | 2 ++
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/hw/core/qdev.
.
This is a very small first step to make this code thread safe.
Suggested-by: Paolo Bonzini
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-5-mlevi...@redhat.com>
[Use RCU_READ_LOCK_GUARD in more places, adjust testcase now that
the
Soon, a device removal might only happen on RCU callback execution.
This is okay for device-del which provides a DEVICE_DELETED event,
but not for the failure case of device-add. To avoid changing
monitor semantics, just drain all pending RCU callbacks on error.
Signed-off-by: Maxim Levitsky
This change will allow us to convert the bus children list to RCU,
while not changing the logic of this function
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-2-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.
On Thu, 2020-09-17 at 16:22 +0300, Maxim Levitsky wrote:
> On Thu, 2020-09-17 at 15:16 +0200, Max Reitz wrote:
> > On 12.08.20 00:51, Dmitry Fomichev wrote:
> > > If scsi-generic driver is in use, an SG node can be specified in
> > > the command line in place of a regula
le is a character device.
>
> So is this path ever taken, or can we just replace it all with the ioctl?
>
> (Before 867eccfed84, this function was used for all host devices, which
> might explain why the code even exists.)
>
> Max
I have another proposal which I am currently evaluating.
How about we drop all the SG_IO limits code alltogher from the raw driver, and
instead just let the scsi drivers (scsi-block and scsi-generic) query
the device directly, since I don't think that the kernel (I will double check
this)?
Best regards,
Maxim Levitsky
is probably
> beyond what I can take.
Implementation wise, I think I can do this, but I'll wait few days for the
feedback on my proposal.
Thanks for finding this bug!
Best regards,
Maxim Levitsky
>
> IMHO my patches should be merged first (they are really just fixing
you mention, nbd, etc)
can choose which limit to use, depending on which IO api they use.
The scsi-generic, and scsi-block can use the SG_IO limits,
while the rest can use the normal (unlimited for file I/O) limits, including
the NBD.
Best regards,
Maxim Levitsky
> >
> >
On Thu, 2020-07-16 at 14:33 +0300, Maxim Levitsky wrote:
> This is basically the same thing as commit
> 'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
> does but for qcow2 files to ensure that we don't leave qcow2 files
> when creation fails.
>
> Si
On Tue, 2020-07-07 at 21:15 +0200, Klaus Jensen wrote:
> On Jul 7 19:27, Maxim Levitsky wrote:
> > On Wed, 2020-07-01 at 14:48 -0700, Andrzej Jakowski wrote:
> > > This patch sets CMBS bit in controller capabilities register when user
> > > configures NVMe driver with
nt64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
>
> -return nvme_map_prp(>qsg, >iov, prp1, prp2, len, n);
> +return nvme_map_prp(n, prp1, prp2, len, req);
> }
>
> static void nvme_post_cqes(void *opaque)
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
ist_destroy(>qsg);
> > +}
> > +
> > +if (req->iov.iov) {
> > +qemu_iovec_destroy(>iov);
> > +}
> > +}
> > +
>
> Klaus,
>
> What is differences between 'clear' and 'exit' from the request
> perspective?
>
> Thanks,
>
In my personal opinion, I don't think the name matters that much here.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
t sqid, uint8_t opcode)
> "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
> pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid
> %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8""
> pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count,
> uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
ist_destroy(>qsg);
> }
> +if (req->iov.nalloc) {
> +qemu_iovec_destroy(>iov);
> + }
> +
> nvme_enqueue_req_completion(cq, req);
> }
>
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
On Thu, 2020-07-30 at 00:06 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Remove the has_sg member from NvmeRequest since it's redundant.
>
> Signed-off-by: Klaus Jensen
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
> ---
> hw/block/nvme.c |
On Wed, 2020-07-29 at 22:08 +0200, Klaus Jensen wrote:
> On Jul 29 21:45, Maxim Levitsky wrote:
> > On Wed, 2020-07-29 at 15:37 +0200, Klaus Jensen wrote:
> > > On Jul 29 13:43, Maxim Levitsky wrote:
> > > > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
&g
On Wed, 2020-07-29 at 15:48 +0200, Klaus Jensen wrote:
> On Jul 29 16:17, Maxim Levitsky wrote:
> > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen
> > >
> > > Since the device does not have any persistent state stor
On Wed, 2020-07-29 at 15:37 +0200, Klaus Jensen wrote:
> On Jul 29 13:43, Maxim Levitsky wrote:
> > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen
> > >
> > > Add support for the Asynchronous Event Request command. Required for
On Wed, 2020-07-29 at 13:44 +0200, Klaus Jensen wrote:
> On Jul 29 13:24, Maxim Levitsky wrote:
> > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen
> > >
> > > Add support for the Get Log Page command and basic implemen
On Thu, 2020-07-30 at 00:52 +0900, Minwoo Im wrote:
> Klaus,
>
> On 20-07-20 13:37:39, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Introduce the nvme_map helper to remove some noise in the main nvme_rw
> > function.
> >
> > Signed-off-by:
OK, but I might have missed something.
Best regards,
Maxim Levitsky
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme.c | 177 +---
> hw/block/nvme.h | 1 +
> 2 files changed, 93 insertions(+), 85 deletions(-)
>
qemu_sglist_destroy(qsg);
> -}
> -
> -return status;
> }
>
> static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> @@ -601,13 +594,6 @@ static void nvme_rw_cb(void *opaque, int ret)
> req->status = NVME_INTERNAL_DEV_ERROR;
> }
>
> -if (req->qsg.nalloc) {
> -qemu_sglist_destroy(>qsg);
> -}
> -if (req->iov.nalloc) {
> -qemu_iovec_destroy(>iov);
> -}
> -
> nvme_enqueue_req_completion(cq, req);
> }
>
This and former patch I guess answer my own question about why to clear the
request after its cqe got posted.
Looks reasonable.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
code);
> return NVME_INVALID_OPCODE | NVME_DNR;
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 137cd8c2bf20..586fd3d62700 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -21,6 +21,7 @@ typedef struct NvmeAsyncEvent {
>
> typedef struct NvmeRequest {
> struct NvmeSQueue *sq;
> +struct NvmeNamespace*ns;
> BlockAIOCB *aiocb;
> uint16_tstatus;
> NvmeCqe cqe;
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
}
> if (cq->tail != cq->head) {
> @@ -1601,7 +1607,6 @@ static void nvme_process_sq(void *opaque)
> req = QTAILQ_FIRST(>req_list);
> QTAILQ_REMOVE(>req_list, req, entry);
> QTAILQ_INSERT_TAIL(>out_req_list, req, entry);
> -memset(>cqe, 0, sizeof(req->cqe));
> req->cqe.cid = cmd.cid;
>
> status = sq->sqid ? nvme_io_cmd(n, , req) :
Best regards,
Maxim Levitsky
EROS= 0x08,
> +NVME_CMD_WRITE_ZEROES = 0x08,
> NVME_CMD_DSM= 0x09,
> };
>
> @@ -838,7 +838,7 @@ enum NvmeIdCtrlOncs {
> NVME_ONCS_COMPARE = 1 << 0,
> NVME_ONCS_WRITE_UNCORR = 1 << 1,
> NVME_ONCS_DSM = 1 << 2,
> -NVME_ONCS_WRITE_ZEROS = 1 << 3,
> +NVME_ONCS_WRITE_ZEROES = 1 << 3,
> NVME_ONCS_FEATURES = 1 << 4,
> NVME_ONCS_RESRVATIONS = 1 << 5,
> NVME_ONCS_TIMESTAMP = 1 << 6,
Nothing against this.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Refactor the nvme_dma_{read,write}_prp functions into a common function
> taking a DMADirection parameter.
>
> Signed-off-by: Klaus Jensen
> Reviewed-by: Maxim Levitsky
> ---
a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -33,6 +33,8 @@ pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ
> vector %u"
> pci_nvme_irq_pin(void) "pulsing IRQ pin"
> pci_nvme_irq_masked(void) "IRQ is masked"
> pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64"
> prp2=0x%"PRIx64""
> +pci_nvme_map_addr(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len
> %"PRIu64""
> +pci_nvme_map_addr_cmb(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len
> %"PRIu64""
> pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode)
> "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
> pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid
> %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8""
> pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count,
> uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
Looks good. I could have missed something, but compared to older version of
similiar code I reviewed,
this looks much better and easy to t understand.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
id->subnqn), subnqn, '\0');
> +g_free(subnqn);
> +
> id->psd[0].mp = cpu_to_le16(0x9c4);
> id->psd[0].enlat = cpu_to_le32(0x10);
> id->psd[0].exlat = cpu_to_le32(0x4);
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
so
> specified
> + * in the spec (NVM Express v1.3d, Section 5.15.4).
> + */
> +if (min_nsid >= NVME_NSID_BROADCAST - 1) {
> +return NVME_INVALID_NSID | NVME_DNR;
> +}
> +
> list = g_malloc0(data_len);
> for (i = 0; i < n->num
me_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> +pci_nvme_identify_ns_descr_list(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae,
> uint32_t len, uint64_t off) "cid %"PRIu16"
> +} NvmeFeatureCap;
> +
> +typedef enum NvmeGetFeatureSelect {
> +NVME_GETFEAT_SELECT_CURRENT = 0x0,
> +NVME_GETFEAT_SELECT_DEFAULT = 0x1,
> +NVME_GETFEAT_SELECT_SAVED = 0x2,
> +NVME_GETFEAT_SELECT_CAP = 0x3,
> +} NvmeGetFeatureSelect;
> +
> #define NVME_GETSETFEAT_FID_MASK 0xff
> #define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
>
> +#define NVME_GETFEAT_SELECT_SHIFT 8
> +#define NVME_GETFEAT_SELECT_MASK 0x7
> +#define NVME_GETFEAT_SELECT(dw10) \
> +((dw10 >> NVME_GETFEAT_SELECT_SHIFT) & NVME_GETFEAT_SELECT_MASK)
> +
> +#define NVME_SETFEAT_SAVE_SHIFT 31
> +#define NVME_SETFEAT_SAVE_MASK 0x1
> +#define NVME_SETFEAT_SAVE(dw10) \
> +((dw10 >> NVME_SETFEAT_SAVE_SHIFT) & NVME_SETFEAT_SAVE_MASK)
OK.
> +
> typedef struct NvmeRangeType {
> uint8_t type;
> uint8_t attributes;
> @@ -926,6 +949,8 @@ typedef struct NvmeLBAF {
> uint8_t rp;
> } NvmeLBAF;
>
> +#define NVME_NSID_BROADCAST 0x
Cool, you probably want eventually to go over code and
change all places that use the number to the define.
(No need to do this now)
> +
> typedef struct NvmeIdNs {
> uint64_tnsze;
> uint64_tncap;
Overall looks OK, other that nitpick about that goto so
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
pa {
> #define NVME_INTC_THR(intc) (intc & 0xff)
> #define NVME_INTC_TIME(intc) ((intc >> 8) & 0xff)
>
> +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
> +
> #define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
> #define NVME_TEMP_THSEL_OVER 0x0
> #define NVME_TEMP_THSEL_UNDER 0x1
> @@ -899,9 +903,13 @@ enum NvmeFeatureIds {
> NVME_WRITE_ATOMICITY= 0xa,
> NVME_ASYNCHRONOUS_EVENT_CONF= 0xb,
> NVME_TIMESTAMP = 0xe,
> -NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> +NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
> +NVME_FID_MAX= 0x100,
> };
>
> +#define NVME_GETSETFEAT_FID_MASK 0xff
> +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
> +
> typedef struct NvmeRangeType {
> uint8_t type;
> uint8_t attributes;
Looks good,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
NvmeRequest *req)
>
> break;
> case NVME_VOLATILE_WRITE_CACHE:
> +if (!(dw11 & 0x1) && blk_enable_write_cache(n->conf.blk)) {
> +blk_flush(n->conf.blk);
> +}
> +
> blk_set_enable_write_cache(n->conf.blk, dw
into the nvme device specific header file as it is the only
> user of the structure. Also, remove the unused members.
Agree.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
>
> Signed-off-by: Klaus Jensen
> Reviewed-by: Dmitry Fomichev
> ---
> hw/block/nvm
ned-off-by: Klaus Jensen
> Acked-by: Keith Busch
> Reviewed-by: Maxim Levitsky
> Reviewed-by: Dmitry Fomichev
> ---
> hw/block/nvme.c | 180 --
> hw/block/nvme.h | 10 ++-
> hw/block/trace-events | 9 +++
> include/
set features,
> dw10=0x%"PRIx32""
> +pci_nvme_err_invalid_log_page(uint16_t cid, uint16_t lid) "cid %"PRIu16" lid
> 0x%"PRIx16""
> pci_nvme_err_startfail_cq(void) "nvme_start_ctrl failed because there are
> non-admin completion queues"
> pci_nvme_err_startfail_sq(void) "nvme_start_ctrl failed because there are
> non-admin submission queues"
> pci_nvme_err_startfail_nbarasq(void) "nvme_start_ctrl failed because the
> admin submission queue address is null"
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index d639e8bbee92..49ce97ae1ab4 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -704,9 +704,9 @@ typedef struct NvmeErrorLog {
> uint8_t resv[35];
> } NvmeErrorLog;
>
> -typedef struct NvmeSmartLog {
> +typedef struct QEMU_PACKED NvmeSmartLog {
> uint8_t critical_warning;
> -uint8_t temperature[2];
> +uint16_ttemperature;
> uint8_t available_spare;
> uint8_t available_spare_threshold;
> uint8_t percentage_used;
> @@ -846,6 +846,10 @@ enum NvmeIdCtrlFrmw {
> NVME_FRMW_SLOT1_RO = 1 << 0,
> };
>
> +enum NvmeIdCtrlLpa {
> +NVME_LPA_EXTENDED = 1 << 2,
> +};
> +
> #define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
> #define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
> #define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
Other than few nitpicks that don't matter much,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
dCtrlOncs {
> NVME_ONCS_TIMESTAMP = 1 << 6,
> };
>
> +enum NvmeIdCtrlFrmw {
> +NVME_FRMW_SLOT1_RO = 1 << 0,
> +};
> +
> #define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
> #define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
> #define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
Looks good,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
uot;"
> pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write
> cache, result=%s"
> pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
> pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested
> cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> pci_nvme_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
> pci_nvme_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
> +pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint16_t
> status) "cid %"PRIu16" cqid %"PRIu16" status 0x%"PRIx16""
> +pci_nvme_mmio_read(uint64_t addr) "addr 0x%"PRIx64""
> +pci_nvme_mmio_write(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data
> 0x%"PRIx64""
> +pci_nvme_mmio_doorbell_cq(uint16_t cqid, uint16_t new_head) "cqid %"PRIu16"
> new_head %"PRIu16""
> +pci_nvme_mmio_doorbell_sq(uint16_t sqid, uint16_t new_tail) "cqid %"PRIu16"
> new_tail %"PRIu16""
> pci_nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO,
> interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> pci_nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO,
> interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> pci_nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller
> config=0x%"PRIx64""
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> -req->cqe.result = result;
> +req->cqe.result = cpu_to_le32(result);
> return NVME_SUCCESS;
> }
>
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
On Thu, 2020-07-16 at 14:33 +0300, Maxim Levitsky wrote:
> This is basically the same thing as commit
> 'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
> does but for qcow2 files to ensure that we don't leave qcow2 files
> when creation fails.
>
> Si
On Wed, 2020-07-22 at 19:14 +0200, Kevin Wolf wrote:
> Am 22.07.2020 um 19:01 hat Maxim Levitsky geschrieben:
> > On Mon, 2020-07-20 at 15:18 +0200, Kevin Wolf wrote:
> > > qcow2 version 2 images don't support the zero flag for clusters, so for
> > > write_zeroes r
can have special 'deallocate' flag that hints the
controller
to discard the sectors.
So woudn't discarding the clusters have theoretical risk of introducing garbage
there?
Best regards,
Maxim Levitsky
lla: https://bugzilla.redhat.com/show_bug.cgi?id=1857490
Signed-off-by: Maxim Levitsky
---
block/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/crypto.c b/block/crypto.c
index 8725c1bc02..0807557763 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -88
A rebase gone wrong, and I ended up allowing a luks image
to be opened at the same time by two VMs without any warnings/overrides.
Fix that and also add an iotest to prevent this from happening.
Best regards,
Maxim Levisky
Maxim Levitsky (2):
block/crypto: disallow write sharing
Test that we can't write-share raw luks images by default,
but we still can with share-rw=on
Signed-off-by: Maxim Levitsky
---
tests/qemu-iotests/296 | 44 +-
tests/qemu-iotests/296.out | 12 +--
2 files changed, 53 insertions(+), 3 deletions
This is basically the same thing as commit
'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
does but for qcow2 files to ensure that we don't leave qcow2 files
when creation fails.
Signed-off-by: Maxim Levitsky
---
block/qcow2.c | 11 +++
1 file changed, 11
ot found or not supported",
> fmt);
Yep, this looks like a real bug, sorry about that.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
On Wed, 2020-07-08 at 16:23 +0200, Gerd Hoffmann wrote:
> On Wed, Jul 08, 2020 at 04:06:45PM +0300, Maxim Levitsky wrote:
> > On Wed, 2020-07-08 at 14:33 +0200, Gerd Hoffmann wrote:
> > > On Mon, Jun 08, 2020 at 12:40:27PM +0300, Maxim Levitsky wrote:
> > > > blockd
ckDriverInfo
> > - Fix qcow2 preallocation when the image size is not a multiple of the
> > cluster size
> > - Fix in block-copy code
> >
>
>
> Applied, thanks.
>
> Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
> for any user
On Wed, 2020-07-08 at 14:33 +0200, Gerd Hoffmann wrote:
> On Mon, Jun 08, 2020 at 12:40:27PM +0300, Maxim Levitsky wrote:
> > blockdev-amend will be used similiar to blockdev-create
> > to allow on the fly changes of the structure of the format based block
> > devices.
On Wed, 2020-07-08 at 18:11 +0300, Maxim Levitsky wrote:
> On Tue, 2020-07-07 at 21:40 +0100, Peter Maydell wrote:
> > On Mon, 6 Jul 2020 at 11:04, Max Reitz wrote:
> > > The following changes since commit
> > > eb6490f544388dd24c0d054a96dd304bc7284450:
> >
fine NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & CAP_PMR_MASK)
> \
> << CAP_PMR_SHIFT)
> +#define NVME_CAP_SET_CMBS(cap, val) (cap |= (uint64_t)(val & CAP_CMB_MASK)
> \
> +
t; > While this is a valid outcome, if we can avoid it it will save all of us
> > review time.
>
> If Klaus wants to do that, fine with me. I'm just trying to find the
> easiest solution for all of us.
>
> > > It would be good to have at least one review, though.
> >
> > Maxim catched this issue, I'd feel safer if he acks your pre-merge queue.
>
> Ok. Maxim, can you please review this series then?
>
> Kevin
I am slowly getting through the heap of the patches trying to understand the
current state of things.
I will start reviewing all these patches today.
Best regards,
Maxim Levitsky
o complicate things these days. If sending
> > separate pull requests directly to Peter would make things easier, I
> > certainly wouldn't object.
> >
>
> I don't think there is any reason to by-pass your tree. I think the
> volume would need to increase even further for that to make sense.
>
It my fault as well - I need to get back to reviewing these.
(I'll review few of them today I hope)
Best regards,
Maxim Levitsky
On Tue, 2020-06-30 at 10:56 +0200, Max Reitz wrote:
> On 29.06.20 14:05, Maxim Levitsky wrote:
> > On Thu, 2020-06-25 at 14:55 +0200, Max Reitz wrote:
> > > From: Maxim Levitsky
> > >
> > > This commit adds two tests, which test the new amend interface
>
On Thu, 2020-06-25 at 14:55 +0200, Max Reitz wrote:
> From: Maxim Levitsky
>
> This commit adds two tests that cover the
> new blockdev-amend functionality of luks and qcow2 driver
>
> Signed-off-by: Maxim Levitsky
> Reviewed-by: Daniel P. Berrangé
> [mreitz: Let 29
On Thu, 2020-06-25 at 14:55 +0200, Max Reitz wrote:
> From: Maxim Levitsky
>
> This commit adds two tests, which test the new amend interface
> of both luks raw images and qcow2 luks encrypted images.
>
> Signed-off-by: Maxim Levitsky
> Reviewed-by: Daniel P. Berrang
'luks':
> +verify_working_luks()
> return
>
> not_sup = supported_fmts and (imgfmt not in supported_fmts)
> if not_sup or (imgfmt in unsupported_fmts):
> notrun('not suitable for this image format: %s' % imgfmt)
>
> +if imgfmt =
then this test.
THe whilelist only affects the qmp it seems so this check doesn't catch it,
plus you could have case when luks driver is blacklisted but qcow2 isn't
When I build qemu with
'--block-drv-whitelist='raw,qcow2' I was able to break iotests 295 296
But this is such a non issue, that I am just noting for reference.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
> - % (-exitcode, ' '.join(qemu_img_args + list(args
> -return subp.communicate()[0]
> +return qemu_img_pipe_and_status(*args)[0]
>
> def qemu_img_log(*args):
> result = qemu_img_pipe(*args)
You made me learn a bit about python type hints, and I don't regret it :-)
Looks OK.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
he patches applied,a
and with --disable-nettle --disable-gcrypt
And now all my tests are skipped with this nice message:
"No crypto library supporting PBKDF in this build: Function not implemented"
Thank you very very much for implementing this!
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
n get away with changing
> as few reference outputs in this patch here as possible.
>
> Signed-off-by: Max Reitz
I went over this patch again, and it looks OK, but
I might have missed something.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
> ---
> tests
-e 's/^\(fmt\)/0-\1/' \
> +-e 's/^\(size\)/1-\1/' \
> +-e 's/^\(backing\)/2-\1/' \
> +-e 's/^\(data_file\)/3-\1/' \
> +-e 's/^\(encryption\)/4-\1/' \
> +-e 's/^\(encrypt\.format\)/5-\1/' \
> +-e 's/^\(encrypt\.key-secret\)/6-\1/' \
> +-e 's/^\(encrypt\.iter-time\)/7-\1/' \
> +-e 's/^\(preallocation\)/8-\1/' \
> +| sort \
> +| $SED -e 's/^[0-9]-//' \
> +| tr '\n\0' ' \n' \
> +| $SED -e 's/^ *$//' -e 's/ *$//'
> +)
> +
> +echo "$filename_part, $options"
> +}
> +
> +# Filter the "Formatting..." line in QMP output (leaving the QMP output
> +# untouched)
> +# (In contrast to _filter_img_create(), this function does not support
> +# multi-line Formatting output)
> +_filter_img_create_in_qmp()
> +{
> +while read -r line; do
> +if echo "$line" | grep -q '^Formatting'; then
> +echo "$line" | _filter_img_create
> +else
> +echo "$line"
> +fi
> +done
> }
Good as well.
>
> _filter_img_create_size()
Looks good now,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
On Wed, 2020-06-17 at 15:50 +0200, Max Reitz wrote:
> On 17.06.20 13:46, Maxim Levitsky wrote:
> > On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
> > > Right now, _filter_img_create just filters out everything that looks
> > > format-dependent, and applies some fi
On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
> From: Maxim Levitsky
>
> This allows more tests to be able to have same output on both qcow2 luks
> encrypted images
> and raw luks images
>
> Signed-off-by: Maxim Levitsky
> Signed-off-by: Max Reitz
> ---
&g
(fmt\)/0-\1/' \
> +-e 's/^\(size\)/1-\1/' \
> +-e 's/^\(backing\)/2-\1/' \
> +-e 's/^\(data_file\)/3-\1/' \
> +-e 's/^\(encryption\)/4-\1/' \
> +-e 's/^\(encrypt\.format\)/5-\1/' \
> +-e 's/^\(encrypt\.key-secret\)/6-\1/' \
> +-e 's/^\(encrypt\.iter-time\)/7-\1/' \
> +-e 's/^\(preallocation\)/8-\1/' \
All right, I understand this now, but do we have to do this?
Maybe it is better to just update the outputs once to avoid keeping
the custom sort order?
> +| sort \
> +| $SED -e 's/^[0-9]-//' \
> +| tr '\n\0' ' \n' \
> +| $SED -e 's/^ *$//' -e 's/ *$//'
> +)
For the above bash pipeline overall: It was hard to decipher :-), but I must
admit
I learned something from it.
> +
> +echo -n "$qmp_pre"
> +if [ -n "$options" ]; then
> +echo "$filename_part, $options"
> +elif [ -n "$filename_part" ]; then
> +echo "$filename_part"
> +fi
> +echo -n "$qmp_post"
> }
>
> _filter_img_create_size()
Overall I like the idea of this patch.
Best regards,
Maxim Levitsky
On Mon, 2020-06-08 at 14:14 +0200, Max Reitz wrote:
> On 08.06.20 11:40, Maxim Levitsky wrote:
> > This implements the encryption key management using the generic code in
> > qcrypto layer and exposes it to the user via qemu-img
> >
> > This code adds a
This commit adds two tests that cover the
new blockdev-amend functionality of luks and qcow2 driver
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
tests/qemu-iotests/295 | 279 +
tests/qemu-iotests/295.out | 40 ++
tests/qemu
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/crypto.c | 72
qapi/block-core.json | 14 -
2 files changed, 66 insertions(+), 20 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
This allows more tests to be able to have same output on both qcow2 luks
encrypted images
and raw luks images
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
tests/qemu-iotests/087.out | 6 +++---
tests/qemu-iotests/134.out | 2 +-
tests/qemu-iotests/158.out
This commit adds two tests, which test the new amend interface
of both luks raw images and qcow2 luks encrypted images.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
tests/qemu-iotests/293 | 207 +
tests/qemu-iotests/293.out | 99
blockdev-amend will be used similiar to blockdev-create
to allow on the fly changes of the structure of the format based block devices.
Current plan is to first support encryption keyslot management for luks
based formats (raw and embedded in qcow2)
Signed-off-by: Maxim Levitsky
Reviewed
Now that we have all the infrastructure in place,
wire it in the qcow2 driver and expose this to the user.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/qcow2.c | 71 +-
tests/qemu-iotests/082
Currently the implementation only supports amending the encryption
options, unlike the qemu-img version
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/qcow2.c| 39 +++
qapi/block-core.json | 16
Next few patches will expose that functionality to the user.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
crypto/block-luks.c | 416 +++-
qapi/crypto.json| 59 ++-
2 files changed, 469 insertions(+), 6 deletions(-)
diff
'force' option will be used for some unsafe amend operations.
This includes things like erasing last keyslot in luks based formats
(which destroys the data, unless the master key is backed up
by external means), but that _might_ be desired result.
Signed-off-by: Maxim Levitsky
Reviewed
rename the write_func to create_write_func, and init_func to create_init_func.
This is preparation for other write_func that will be used to update the
encryption keys.
No functional changes
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/crypto.c | 25
This will be used first to implement luks keyslot management.
block_crypto_amend_opts_init will be used to convert
qemu-img cmdline to QCryptoBlockAmendOptions
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/crypto.c | 17 +
block/crypto.h
of
unsharing read, rather that write permission which allows
to avoid cases when the other user had opened the image read-only.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/crypto.c | 130 +++--
block/crypto.h
it in each action option list.
In future it might be useful to remove some options which are
not supported anyway from amend list, which currently
cause an error message if amended.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/qcow2.c
Some qcow2 create options can't be used for amend.
Remove them from the qcow2 create options and add generic logic to detect
such options in qemu-img
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/qcow2.c | 138 --
qemu-img.c
clone of "luks-keymgmnt-v2"
Maxim Levitsky (14):
qcrypto/core: add generic infrastructure for crypto options amendment
qcrypto/luks: implement encryption key management
block/amend: add 'force' option
block/amend: separate amend and create options for qemu-img
block/amend
On Sun, 2020-06-07 at 15:51 +0300, Maxim Levitsky wrote:
> On Tue, 2020-06-02 at 17:31 +0200, Kevin Wolf wrote:
> > Am 14.05.2020 um 06:45 hat Klaus Jensen geschrieben:
> > > From: Klaus Jensen
> > >
> > > Changes since v5
> > >
>
On Tue, 2020-06-02 at 18:29 +0200, Max Reitz wrote:
> On 18.05.20 14:20, Maxim Levitsky wrote:
> > Hi!
> > Here is the updated series of my patches, incorporating all the feedback I
> > received.
>
> You asked me on IRC what to do to get this series to move forward;
at while I kind of didn't review some of
the latest patches mostly becasue I was doing other things,
I do plan to keep on reviewing following patches in this activety.
Best regards,
Maxim Levitsky
>
> Kevin
>
>
This commit adds two tests that cover the
new blockdev-amend functionality of luks and qcow2 driver
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
tests/qemu-iotests/295 | 279 +
tests/qemu-iotests/295.out | 40 ++
tests/qemu
Currently the implementation only supports amending the encryption
options, unlike the qemu-img version
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/qcow2.c| 39 +++
qapi/block-core.json | 16
Now that we have all the infrastructure in place,
wire it in the qcow2 driver and expose this to the user.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/qcow2.c | 71 +-
tests/qemu-iotests/082
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/crypto.c | 72
qapi/block-core.json | 14 -
2 files changed, 66 insertions(+), 20 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
blockdev-amend will be used similiar to blockdev-create
to allow on the fly changes of the structure of the format based block devices.
Current plan is to first support encryption keyslot management for luks
based formats (raw and embedded in qcow2)
Signed-off-by: Maxim Levitsky
Reviewed
This allows more tests to be able to have same output on both qcow2 luks
encrypted images
and raw luks images
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
tests/qemu-iotests/087.out | 6 +++---
tests/qemu-iotests/134.out | 2 +-
tests/qemu-iotests/158.out
Some qcow2 create options can't be used for amend.
Remove them from the qcow2 create options and add generic logic to detect
such options in qemu-img
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/qcow2.c | 138 --
qemu-img.c
it in each action option list.
In future it might be useful to remove some options which are
not supported anyway from amend list, which currently
cause an error message if amended.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Max Reitz
---
block/qcow2.c
rename the write_func to create_write_func, and init_func to create_init_func.
This is preparation for other write_func that will be used to update the
encryption keys.
No functional changes
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/crypto.c | 25
'force' option will be used for some unsafe amend operations.
This includes things like erasing last keyslot in luks based formats
(which destroys the data, unless the master key is backed up
by external means), but that _might_ be desired result.
Signed-off-by: Maxim Levitsky
Reviewed
This will be used first to implement luks keyslot management.
block_crypto_amend_opts_init will be used to convert
qemu-img cmdline to QCryptoBlockAmendOptions
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/crypto.c | 17 +
block/crypto.h
Next few patches will expose that functionality to the user.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
crypto/block-luks.c | 416 +++-
qapi/crypto.json| 59 ++-
2 files changed, 469 insertions(+), 6 deletions(-)
diff
Max Reitz
* Rebased on latest qemu master
Best regards,
Maxim Levitsky
clone of "luks-keymgmnt-v2"
Maxim Levitsky (14):
qcrypto/core: add generic infrastructure for crypto options amendment
qcrypto/luks: implement encryption key management
block/amend: add 'force' opti
This commit adds two tests, which test the new amend interface
of both luks raw images and qcow2 luks encrypted images.
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
tests/qemu-iotests/293 | 207 +
tests/qemu-iotests/293.out | 99
101 - 200 of 956 matches
Mail list logo
