On Mon, 2020-01-27 at 12:36 +0200, Maxim Levitsky wrote:
> This changes most of qemu's error messages,
> but it feels like the right thing to do.
>
> This is WIP patch, since I updated most of iotests but not all of them,
> and will be updated if this patch is accepeted in th
On Tue, 2020-01-28 at 16:56 +, Dr. David Alan Gilbert wrote:
> * Maxim Levitsky (mlevi...@redhat.com) wrote:
> > These days device-hotplug.c only contains the hmp_drive_add
> > In the next patch, rest of hmp_drive* functions will be moved
> > there.
> >
> >
On Wed, 2020-01-29 at 11:38 +0100, Markus Armbruster wrote:
> Maxim Levitsky writes:
>
> > On Mon, 2020-01-27 at 12:36 +0200, Maxim Levitsky wrote:
> > > This changes most of qemu's error messages,
> > > but it feels like the right thing to do.
> > &g
On Wed, 2020-01-29 at 11:27 +0100, Markus Armbruster wrote:
> Maxim Levitsky writes:
>
> > On Mon, 2020-01-27 at 12:36 +0200, Maxim Levitsky wrote:
> > > This way they all will be prefixed with 'Error:' which some parsers
> > > (e.g li
On Wed, 2020-01-29 at 11:19 +0100, Markus Armbruster wrote:
> Maxim Levitsky writes:
>
> > On Tue, 2020-01-28 at 16:56 +, Dr. David Alan Gilbert wrote:
> > > * Maxim Levitsky (mlevi...@redhat.com) wrote:
> > > > These days device-hotplug.c only contains the
On Tue, 2020-01-28 at 16:59 +, Daniel P. Berrangé wrote:
> On Tue, Jan 14, 2020 at 09:33:38PM +0200, Maxim Levitsky wrote:
> > This will be used first to implement luks keyslot management.
> >
> > block_crypto_amend_opts_init will be used to convert
&g
On Tue, 2020-01-28 at 17:32 +, Daniel P. Berrangé wrote:
> On Tue, Jan 28, 2020 at 05:11:16PM +, Daniel P. Berrangé wrote:
> > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote:
> > > On Tue, 2020-01-21 at 08:54 +0100, Marku
Signed-off-by: Maxim Levitsky
Reviewed-by: Philippe Mathieu-Daudé
---
hw/usb/dev-storage.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 90da008df1..5629213d55 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -19,7 +19,6
thus according
to LICENSE is under GPLv2+
Signed-off-by: Maxim Levitsky
Reviewed-by: Markus Armbruster
---
MAINTAINERS | 1 +
Makefile.objs| 2 +-
block/Makefile.objs | 1 +
block/monitor
e that this is right thing to do.
Best regards,
Maxim Levitsky
Maxim Levitsky (11):
usb/dev-storage: remove unused include
monitor/hmp: uninline add_init_drive
monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c
monitor/hmp: move hmp_drive_del and hmp_commit
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 390 +
include/block/block-hmp-cmds.h | 4 +
include/monitor/hmp.h | 4 -
monitor/hmp-cmds.c | 388
4
This is only used by hmp_drive_add.
The code is just a bit shorter this way.
No functional changes
Signed-off-by: Maxim Levitsky
Reviewed-by: Markus Armbruster
---
device-hotplug.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/device
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 108 -
blockdev.c | 95 -
include/block/block-hmp-cmds.h | 4 ++
include/sysemu/blockdev.h | 4 --
4
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 56 --
include/block/block-hmp-cmds.h | 4 +++
include/monitor/hmp.h | 3 --
monitor/hmp-cmds.c | 47
4
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 52 ++
include/block/block-hmp-cmds.h | 6
include/monitor/hmp.h | 5
monitor/hmp-cmds.c | 52
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 140 +
include/block/block-hmp-cmds.h | 9 +++
include/monitor/hmp.h | 6 --
monitor/hmp-cmds.c | 137
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 60 ++
include/block/block-hmp-cmds.h | 12 +--
include/monitor/hmp.h | 2 --
monitor/hmp-cmds.c | 58
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 89 ++
include/block/block-hmp-cmds.h | 5 ++
include/monitor/hmp.h | 4 --
monitor/hmp-cmds.c | 87 -
4
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 30
blockdev.c | 42 +++---
include/block/block_int.h | 5 ++--
3 files changed, 41 insertions(+), 36 deletions
On Tue, 2020-01-28 at 17:21 +, Daniel P. Berrangé wrote:
> On Tue, Jan 14, 2020 at 09:33:39PM +0200, Maxim Levitsky wrote:
> > Next few patches will expose that functionality
> > to the user.
> >
> > Signed-off-by: Maxim Levitsky
> > -
M +, Daniel P. Berrangé wrote:
> > > > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote:
> > > > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote:
> > > > > >
> > > > > >
> > > &
On Tue, 2020-01-28 at 17:23 +, Daniel P. Berrangé wrote:
> On Tue, Jan 14, 2020 at 09:33:41PM +0200, Maxim Levitsky wrote:
> > Some options are only useful for creation
> > (or hard to be amended, like cluster size for qcow2), while some other
> > options are only u
On Tue, 2020-01-28 at 17:27 +, Daniel P. Berrangé wrote:
> On Tue, Jan 14, 2020 at 09:33:43PM +0200, 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
> >
>
On Tue, 2020-01-28 at 17:30 +, Daniel P. Berrangé wrote:
> On Tue, Jan 14, 2020 at 09:33:44PM +0200, Maxim Levitsky wrote:
> > Now that we have all the infrastructure in place,
> > wire it in the qcow2 driver and expose this to the user.
> >
> > Si
On Tue, 2020-01-28 at 17:36 +, Daniel P. Berrangé wrote:
> On Tue, Jan 14, 2020 at 09:33:45PM +0200, Maxim Levitsky wrote:
> > This allows more tests to be able to have same output on both qcow2 luks
> > encrypted images
> > and raw luks images
> >
> &
On Tue, 2020-01-28 at 17:40 +, Daniel P. Berrangé wrote:
> On Tue, Jan 14, 2020 at 09:33:48PM +0200, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky
> > ---
> > block/crypto.c | 70
> > qapi/block-core
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
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
---
block/qcow2.c | 160 +-
include
'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
R
qcow2 amend options
* Reworked main code implementation to be hopefully cleaner.
* Fixed iotests hopefully.
Best regards,
Maxim Levitsky
Maxim Levitsky (14):
qcrypto/core: add generic infrastructure for crypto options amendment
qcrypto/luks: implement encryptio
Next few patches will expose that functionality
to the user.
Signed-off-by: Maxim Levitsky
---
crypto/block-luks.c | 391 +++-
qapi/crypto.json| 50 +-
2 files changed, 438 insertions(+), 3 deletions(-)
diff --git a/crypto/block-luks.c b/crypto
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
---
block/qcow2.c | 83 +--
tests/qemu-iotests/082.out | 288 +
2 files changed, 360 insertions
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/crypto.c | 70
qapi/block-core.json | 14 -
2 files changed, 64 insertions(+), 20 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 1f1a6da6a9
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
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
---
block
ange for the idea 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
---
block/crypto.c | 130 +++--
block/crypto.h | 42 ++--
2 files cha
Currently the implementation only supports amending the encryption
options, unlike the qemu-img version
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
block/qcow2.c| 39 +++
qapi/block-core.json | 16 +++-
2 files
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
---
tests/qemu-iotests/087.out | 6 +++---
tests/qemu-iotests/134.out | 2 +-
tests/qemu-iotests/158.out | 4 ++--
tests/qemu-iotests
This commit adds two tests that cover the
new blockdev-amend functionality of luks and qcow2 driver
Signed-off-by: Maxim Levitsky
---
tests/qemu-iotests/302 | 284 +
tests/qemu-iotests/302.out | 40 ++
tests/qemu-iotests/303 | 235
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
---
block/qcow2.c | 108 ++---
qemu-img.c | 18 +++-
tests
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
---
tests/qemu-iotests/300 | 207 +
tests/qemu-iotests/300.out | 99 ++
tests/qemu
On Mon, 2020-02-03 at 19:57 +, Dr. David Alan Gilbert wrote:
> * Maxim Levitsky (mlevi...@redhat.com) wrote:
> > This patch series is bunch of cleanups to the hmp monitor code.
> > It mostly moves the blockdev related hmp handlers to its own file,
> > and does s
which fails. Simple enough, but it's not really "describe the
> new state", it's still "specify a series of state transitions".
>
> If we merge the array elements into a description of the new state of
> all eight slots, where a slot's description can be "same as old state",
> then this makes slot 0 active with either secret 'sec0' or 'sec1',
> depending on how we resolve the conflict. We could even make conflicts
> an error, and then this would fail without changing anything.
>
> What do we want?
>
> Is this worth the trouble?
Yes, that is my thoughts on this as well.
Best regards,
Maxim Levitsky
valid_getfeat(int dw10) "invalid get features,
> dw10=0x%"PRIx32""
> +nvme_dev_err_invalid_setfeat(uint3
d);
> -break;
> -
> default:
> trace_nvme_dev_err_invalid_setfeat(dw10);
> return NVME_INVALID_FIELD | NVME_DNR;
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
", _state, _props.serial), \
> +DEFINE_PROP_UINT32("cmb_size_mb", _state, _props.cmb_size_mb, 0), \
> +DEFINE_PROP_UINT32("num_queues", _state, _props.num_queues, 64)
> +
> +typedef struct NvmeParams {
> +char *serial;
> +uint32_t n
uint8_t vs[3712];
I reviewed this patch by cross referencing the nvme structures as defined in
the kernel,
and the spec.
I prefer to merge this patch with all other spec updates you do in following
patches,
to bring nvme.h up to date to 1.3d,
so that it will be easier to review this and remove some noise from other
patches.
Best regards,
Maxim Levitsky
ES - The device currently
doesn't implement some mandatory features.
And there are probably more. This is what I can recall from my nvme-mdev.
However I see that you implmented these in following patches, so I suggest you
first put patches that implement all that features,
and then bump the NVME version.
Most of these features I mentioned were mandatory even in version 1.0 of the
spec, so current version is not even
compliant with 1.0 IMHO.
Best regards,
Maxim Levitsky
suspect that there is extra space
after that (void *).
Also note that in following patches you fix a serious bug in this function that
it doesn't
check that the whole range is in CMB but only that the start of the area is.
I would move it here, or even to a separate patch.
> +return;
> }
> +
> +pci_dma_read(&n->parent_obj, addr, buf, size);
> }
>
> static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
Best regards,
Maxim Levitsky
Limit
> (four
> + * concurrently outstanding Abort commands), so lets use that though it
> is
> + * inconsequential.
> + */
> +id->acl = 3;
Yep.
> id->frmw = 7 << 1;
> id->lpa = 1 << 0;
> id->sqes = (0x6 << 4) | 0x6;
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
>nsze =
> -cpu_to_le64(n->ns_size >>
> -id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds);
> +if (nvme_init_namespace(n, &n->namespaces[i], &local_err)) {
> +error_propagate_prepend(errp, local_err, "nvme_init_namespace:
> ");
And here
> +return;
> +}
> }
> +
> +nvme_init_pci(n, pci_dev);
> +nvme_init_ctrl(n);
> }
>
> static void nvme_exit(PCIDevice *pci_dev)
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 9957c4a200e2..a867bdfabafd 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -65,6 +65,22 @@ typedef struct NvmeNamespace {
> NvmeIdNsid_ns;
> } NvmeNamespace;
>
> +static inline NvmeLBAF nvme_ns_lbaf(NvmeNamespace *ns)
> +{
Its not common to return a structure in C, usually pointer is returned to
avoid copying. In this case this doesn't matter that much though.
> +NvmeIdNs *id_ns = &ns->id_ns;
> +return id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(id_ns->flbas)];
> +}
> +
> +static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
> +{
> +return nvme_ns_lbaf(ns).ds;
> +}
> +
> +static inline size_t nvme_ns_lbads_bytes(NvmeNamespace *ns)
> +{
> +return 1 << nvme_ns_lbads(ns);
> +}
> +
> #define TYPE_NVME "nvme"
> #define NVME(obj) \
> OBJECT_CHECK(NvmeCtrl, (obj), TYPE_NVME)
> @@ -101,4 +117,9 @@ typedef struct NvmeCtrl {
> NvmeIdCtrl id_ctrl;
> } NvmeCtrl;
>
> +static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, NvmeNamespace *ns)
> +{
> +return n->ns_size >> nvme_ns_lbads(ns);
> +}
Unless you need all these functions in the future, this feels like
it is a bit verbose.
> +
> #endif /* HW_NVME_H */
Best regards,
Maxim Levitsky
enum NvmeIdCtrlOncs {
> typedef struct NvmeFeatureVal {
> uint32_tarbitration;
> uint32_tpower_mgmt;
> -uint32_ttemp_thresh;
> +uint16_ttemp_thresh_hi;
> +uint16_ttemp_thresh_low;
> uint32_terr_rec;
> uint32_tvolatile_wc;
> uint32_tnum_queues;
> @@ -635,6 +636,10 @@ typedef struct NvmeFeatureVal {
> #define NVME_INTC_THR(intc) (intc & 0xff)
> #define NVME_INTC_TIME(intc)((intc >> 8) & 0xff)
>
> +#define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
> +#define NVME_TEMP_TMPSEL(temp) ((temp >> 16) & 0xf)
> +#define NVME_TEMP_TMPTH(temp) (temp & 0x)
> +
> enum NvmeFeatureIds {
> NVME_ARBITRATION= 0x1,
> NVME_POWER_MANAGEMENT = 0x2,
Best regards,
Maxim Levitsky
RIx64""
> @@ -85,6 +86,7 @@ nvme_dev_err_invalid_create_cq_qflags(uint16_t qflags)
> "failed creating completi
> nvme_dev_err_invalid_identify_cns(uint16_t cns) "identify, invalid
> cns=0x%"PRIx16""
> nvme_dev_err_invalid_getfeat(int dw10) "invalid get features,
> dw10=0x%"PRIx32""
> nvme_dev_err_invalid_setfeat(uint32_t dw10) "invalid set features,
> dw10=0x%"PRIx32""
> +nvme_dev_err_invalid_log_page(uint16_t cid, uint16_t lid) "cid %"PRIu16" lid
> 0x%"PRIx16""
> nvme_dev_err_startfail_cq(void) "nvme_start_ctrl failed because there are
> non-admin completion queues"
> nvme_dev_err_startfail_sq(void) "nvme_start_ctrl failed because there are
> non-admin submission queues"
> nvme_dev_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 ff31cb32117c..9a6055adeb61 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -515,7 +515,7 @@ enum NvmeSmartWarn {
> NVME_SMART_FAILED_VOLATILE_MEDIA = 1 << 4,
> };
>
> -enum LogIdentifier {
> +enum NvmeLogIdentifier {
> NVME_LOG_ERROR_INFO = 0x01,
> NVME_LOG_SMART_INFO = 0x02,
> NVME_LOG_FW_SLOT_INFO = 0x03,
Best regards,
Maxim Levitsky
"
> +nvme_dev_no_outstanding_aers(void) "ignoring event; no outstanding AERs"
> nvme_dev_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO,
> interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> nvme_dev_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO,
> interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> nvme_dev_mmio_cfg(uint64_t data) "wrote MMIO, config controller
> config=0x%"PRIx64""
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 9a6055adeb61..a24be047a311 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -386,8 +386,8 @@ enum NvmeAsyncEventRequest {
> NVME_AER_TYPE_SMART = 1,
> NVME_AER_TYPE_IO_SPECIFIC = 6,
> NVME_AER_TYPE_VENDOR_SPECIFIC = 7,
> -NVME_AER_INFO_ERR_INVALID_SQ= 0,
> -NVME_AER_INFO_ERR_INVALID_DB= 1,
> +NVME_AER_INFO_ERR_INVALID_DB_REGISTER = 0,
> +NVME_AER_INFO_ERR_INVALID_DB_VALUE = 1,
> NVME_AER_INFO_ERR_DIAG_FAIL = 2,
> NVME_AER_INFO_ERR_PERS_INTERNAL_ERR = 3,
> NVME_AER_INFO_ERR_TRANS_INTERNAL_ERR= 4,
> @@ -640,6 +640,10 @@ typedef struct NvmeFeatureVal {
> #define NVME_TEMP_TMPSEL(temp) ((temp >> 16) & 0xf)
> #define NVME_TEMP_TMPTH(temp) (temp & 0x)
>
> +#define NVME_AEC_SMART(aec) (aec & 0xff)
> +#define NVME_AEC_NS_ATTR(aec) ((aec >> 8) & 0x1)
> +#define NVME_AEC_FW_ACTIVATION(aec) ((aec >> 9) & 0x1)
> +
> enum NvmeFeatureIds {
> NVME_ARBITRATION= 0x1,
> NVME_POWER_MANAGEMENT = 0x2,
Overall looks very good. This feature is very tricky to
get right due to somewhat unclear spec but after reading the
spec again, it looks OK.
I might have missed something though. I cross checked against my
implementation of this and it looks like I misunderstood the spec
in few places back then in my nvme-mdev implementation.
Reminding to fix all the split code line alignment issues (when C statement is
split over to next line it should be aligned on first '(').
There are plenty of these here.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
olatile write
> cache, result=%s"
> nvme_dev_getfeat_numq(int result) "get feature number of queues, result=%d"
> nvme_dev_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"
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index a24be047a311..09419ed499d0 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -445,7 +445,8 @@ enum NvmeStatusCodes {
> NVME_FW_REQ_RESET = 0x010b,
> NVME_INVALID_QUEUE_DEL = 0x010c,
> NVME_FID_NOT_SAVEABLE = 0x010d,
> -NVME_FID_NOT_NSID_SPEC = 0x010f,
> +NVME_FEAT_NOT_CHANGABLE = 0x010e,
> +NVME_FEAT_NOT_NSID_SPEC = 0x010f,
> NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
> NVME_CONFLICTING_ATTRS = 0x0180,
> NVME_INVALID_PROT_INFO = 0x0181,
Best regards,
Maxim Levitsky
> 0x%"PRIx8""
> nvme_dev_aer_post_cqe(uint8_t typ, uint8_t info, uint8_t log_page) "type
> 0x%"PRIx8" info 0x%"PRIx8" lid 0x%"PRIx8""
> +nvme_dev_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint16_t
> status) "cid %"PRIu16" cqid %"PRIu16" status 0x%"PRIx16""
> nvme_dev_enqueue_event(uint8_t typ, uint8_t info, uint8_t log_page) "type
> 0x%"PRIx8" info 0x%"PRIx8" lid 0x%"PRIx8""
> nvme_dev_enqueue_event_noqueue(int queued) "queued %d"
> nvme_dev_enqueue_event_masked(uint8_t typ) "type 0x%"PRIx8""
With alignment fixed:
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
irement is not obvious, a
quote/reference to the spec
would be nice to have here.
> +
> trace_nvme_dev_setfeat_numq((dw11 & 0x) + 1,
> ((dw11 >> 16) & 0xFFFF) + 1, n->params.num_queues - 1,
> n->params.num_queues - 1);
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
uct NvmeIdNs {
> uint8_t rescap;
> uint8_t fpi;
> uint8_t dlfeat;
> -uint8_t rsvd33;
> uint16_tnawun;
> uint16_tnawupf;
> +uint16_tnacwu;
Aha! Here you 'fix' the bug you had in patch 4.
> uint16_tnabsn;
> uint16_tnabo;
> uint16_tnabspf;
> -uint8_t rsvd46[2];
> +uint16_tnoiob;
> uint8_t nvmcap[16];
> uint8_t rsvd64[40];
> uint8_t nguid[16];
> uint64_teui64;
> NvmeLBAFlbaf[16];
> -uint8_t res192[192];
> +uint8_t rsvd192[192];
And even do what I suggested with that field :-)
Please squash the changes.
> uint8_t vs[3712];
> } NvmeIdNs;
>
So I suggest you squash this set of changes with patch 4.
I also suggest you to split the other changes in this patch, 1 per feature
added.
The tracing change can also be squashed with the other tracing patch you
submitted.
In summary I would suggest you to have:
1. patch that only adds all the fields from the 1.3d spec, and overall updates
nvme.h
to be up to 1.3d spec
2. patches that do refactoring, add more tracing (also form of refactoring,
since tracing
isn't a functional thing)
3. set of patches that implement all the 1.3d features.
4. patch that only bumps the supported version right to 1.3d
Best regards,
Maxim Levitsky
r.cmbsz, 1);
> NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
> NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2);
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 7ced5fd485a9..d27baa9d5391 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -27,11 +27,11 @@ typedef struct NvmeRequest {
> struct NvmeSQueue *sq;
> BlockAIOCB *aiocb;
> uint16_tstatus;
> -boolhas_sg;
> NvmeCqe cqe;
> BlockAcctCookie acct;
> QEMUSGList qsg;
> QEMUIOVectoriov;
> +NvmeCmd cmd;
> QTAILQ_ENTRY(NvmeRequest)entry;
> } NvmeRequest;
>
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 9e5a4548bde0..77aa0da99ee0 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -33,6 +33,7 @@ nvme_dev_irq_msix(uint32_t vector) "raising MSI-X IRQ
> vector %u"
> nvme_dev_irq_pin(void) "pulsing IRQ pin"
> nvme_dev_irq_masked(void) "IRQ is masked"
> nvme_dev_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64"
> prp2=0x%"PRIx64""
> +nvme_dev_map_prp(uint16_t cid, uint8_t opc, uint64_t trans_len, uint32_t
> len, uint64_t prp1, uint64_t prp2, int num_prps) "cid %"PRIu16" opc
> 0x%"PRIx8" trans_len %"PRIu64" len %"PRIu32" prp1
> 0x%"PRIx64" prp2 0x%"PRIx64" num_prps %d"
> nvme_dev_rw(const char *verb, uint32_t blk_count, uint64_t byte_count,
> uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
> nvme_dev_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t
> qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64",
> sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16",
> qflags=%"PRIu16""
> nvme_dev_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t
> size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64",
> cqid=%"PRIu16", vector=%"PRIu16",
> qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 31eb9397d8c6..c1de92179596 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -427,6 +427,7 @@ enum NvmeStatusCodes {
> NVME_CMD_ABORT_MISSING_FUSE = 0x000a,
> NVME_INVALID_NSID = 0x000b,
> NVME_CMD_SEQ_ERROR = 0x000c,
> +NVME_INVALID_USE_OF_CMB = 0x0012,
> NVME_LBA_RANGE = 0x0080,
> NVME_CAP_EXCEEDED = 0x0081,
> NVME_NS_NOT_READY = 0x0082,
Overall I would split this commit into real refactoring and bugfixes.
Best regards,
Maxim Levitsky
eturn false;
> +}
> +}
> +
> #define TYPE_NVME "nvme"
> #define NVME(obj) \
> OBJECT_CHECK(NvmeCtrl, (obj), TYPE_NVME)
> @@ -139,10 +236,21 @@ static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n,
> NvmeNamespace *ns)
> static inline uint16_t nvme_cid(NvmeRequest *req)
> {
> if (req) {
> -return le16_to_cpu(req->cqe.cid);
> +return le16_to_cpu(req->cmd.cid);
> }
>
> return 0x;
> }
>
> +static inline bool nvme_status_is_error(uint16_t status, uint16_t err)
> +{
> +/* strip DNR and MORE */
> +return (status & 0xfff) == err;
> +}
> +
> +static inline NvmeCtrl *nvme_ctrl(NvmeRequest *req)
> +{
> +return req->sq->ctrl;
> +}
> +
> #endif /* HW_NVME_H */
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 77aa0da99ee0..90a57fb6099a 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -34,7 +34,12 @@ nvme_dev_irq_pin(void) "pulsing IRQ pin"
> nvme_dev_irq_masked(void) "IRQ is masked"
> nvme_dev_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64"
> prp2=0x%"PRIx64""
> nvme_dev_map_prp(uint16_t cid, uint8_t opc, uint64_t trans_len, uint32_t
> len, uint64_t prp1, uint64_t prp2, int num_prps) "cid %"PRIu16" opc
> 0x%"PRIx8" trans_len %"PRIu64" len %"PRIu32" prp1
> 0x%"PRIx64" prp2 0x%"PRIx64" num_prps %d"
> +nvme_dev_req_register_aio(uint16_t cid, void *aio, const char *blkname,
> uint64_t offset, uint64_t count, const char *opc, void *req) "cid %"PRIu16"
> aio %p blk \"%s\" offset %"PRIu64" count
> %"PRIu64" opc \"%s\" req %p"
> +nvme_dev_aio_cb(uint16_t cid, void *aio, const char *blkname, uint64_t
> offset, const char *opc, void *req) "cid %"PRIu16" aio %p blk \"%s\" offset
> %"PRIu64" opc \"%s\" req %p"
> +nvme_dev_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode)
> "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
> nvme_dev_rw(const char *verb, uint32_t blk_count, uint64_t byte_count,
> uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
> +nvme_dev_rw_cb(uint16_t cid, uint32_t nsid) "cid %"PRIu16" nsid %"PRIu32""
> +nvme_dev_write_zeros(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t
> nlb) "cid %"PRIu16" nsid %"PRIu32" slba %"PRIu64" nlb %"PRIu32""
> nvme_dev_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t
> qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64",
> sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16",
> qflags=%"PRIu16""
> nvme_dev_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t
> size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64",
> cqid=%"PRIu16", vector=%"PRIu16",
> qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
> nvme_dev_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
> @@ -75,6 +80,9 @@ nvme_dev_mmio_shutdown_set(void) "shutdown bit set"
> nvme_dev_mmio_shutdown_cleared(void) "shutdown bit cleared"
>
> # nvme traces for error conditions
> +nvme_dev_err_mdts(uint16_t cid, size_t mdts, size_t len) "cid %"PRIu16" mdts
> %"PRIu64" len %"PRIu64""
> +nvme_dev_err_prinfo(uint16_t cid, uint16_t ctrl) "cid %"PRIu16" ctrl
> %"PRIu16""
> +nvme_dev_err_aio(uint16_t cid, void *aio, const char *blkname, uint64_t
> offset, const char *opc, void *req, uint16_t status) "cid %"PRIu16" aio %p
> blk \"%s\" offset %"PRIu64" opc \"%s\" req %p
> status 0x%"PRIx16""
> nvme_dev_err_invalid_dma(void) "PRP/SGL is too small for transfer size"
> nvme_dev_err_invalid_prplist_ent(uint64_t prplist) "PRP list entry is null
> or not page aligned: 0x%"PRIx64""
> nvme_dev_err_invalid_prp2_align(uint64_t prp2) "PRP2 is not page aligned:
> 0x%"PRIx64""
The patch is large, I tried my best to spot issues, but I might have missed
some.
Please split it as I pointed out.
Overall I do like most of the changes.
Best regards,
Maxim Levitsky
n);
> } else {
> -bytes = qemu_iovec_from_buf(&iov, 0, ptr, len);
> +bytes = qemu_iovec_from_buf(&req->iov, 0, ptr, len);
> }
>
> if (unlikely(bytes != len)) {
> @@ -338,8 +334,6 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr,
> uint32_t len,
> status = NVME_INVALID_FIELD | NVME_DNR;
> }
>
> -qemu_iovec_destroy(&iov);
> -
> return status;
> }
>
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
r_addr_read(uint64_t addr) "addr 0x%"PRIx64""
> +nvme_dev_err_addr_write(uint64_t addr) "addr 0x%"PRIx64""
> nvme_dev_err_invalid_dma(void) "PRP/SGL is too small for transfer size"
> nvme_dev_err_invalid_prplist_ent(uint64_t prplist) "PRP list entry is null
> or not page aligned: 0x%"PRIx64""
> nvme_dev_err_invalid_prp2_align(uint64_t prp2) "PRP2 is not page aligned:
> 0x%"PRIx64""
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index c1de92179596..a873776d98b8 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -418,7 +418,7 @@ enum NvmeStatusCodes {
> NVME_INVALID_OPCODE = 0x0001,
> NVME_INVALID_FIELD = 0x0002,
> NVME_CID_CONFLICT = 0x0003,
> -NVME_DATA_TRAS_ERROR= 0x0004,
> +NVME_DATA_TRANSFER_ERROR= 0x0004,
> NVME_POWER_LOSS_ABORT = 0x0005,
> NVME_INTERNAL_DEV_ERROR = 0x0006,
> NVME_CMD_ABORT_REQ = 0x0007,
Best regards,
Maxim Levitsky
0x4,
> +
> +NVME_SGL_DESCR_TYPE_VENDOR_SPECIFIC = 0xf,
> +};
> +
> +enum NvmeSglDescriptorSubtype {
> +NVME_SGL_DESCR_SUBTYPE_ADDRESS = 0x0,
> +};
> +
> +typedef struct NvmeSglDescriptor {
> +uint64_t addr;
> +uint32_t len;
> +uint8_t rsvd[3];
> +uint8_t type;
> +} NvmeSglDescriptor;
I suggest you add a build time struct size check for this,
just in case compiler tries something funny.
(look at _nvme_check_size, at nvme.h)
Also I think that the spec update change that adds the NvmeSglDescriptor
should be split into separate patch (or better be added in one big patch that
adds all 1.3d features),
which would make it also easier to see changes that touch the other nvme driver
we have.
> +
> +#define NVME_SGL_TYPE(type) ((type >> 4) & 0xf)
> +#define NVME_SGL_SUBTYPE(type) (type & 0xf)
> +
> +typedef union NvmeCmdDptr {
> +struct {
> +uint64_tprp1;
> +uint64_tprp2;
> +} prp;
> +
> +NvmeSglDescriptor sgl;
> +} NvmeCmdDptr;
> +
> +enum NvmePsdt {
> +PSDT_PRP = 0x0,
> +PSDT_SGL_MPTR_CONTIGUOUS = 0x1,
> +PSDT_SGL_MPTR_SGL= 0x2,
> +};
> +
> typedef struct NvmeCmd {
> uint8_t opcode;
> -uint8_t fuse;
> +uint8_t flags;
> uint16_tcid;
> uint32_tnsid;
> uint64_tres1;
> uint64_tmptr;
> -uint64_tprp1;
> -uint64_tprp2;
> +NvmeCmdDptr dptr;
> uint32_tcdw10;
> uint32_tcdw11;
> uint32_tcdw12;
> @@ -222,6 +260,9 @@ typedef struct NvmeCmd {
> uint32_tcdw15;
> } NvmeCmd;
>
> +#define NVME_CMD_FLAGS_FUSE(flags) (flags & 0x3)
> +#define NVME_CMD_FLAGS_PSDT(flags) ((flags >> 6) & 0x3)
> +
> enum NvmeAdminCommands {
> NVME_ADM_CMD_DELETE_SQ = 0x00,
> NVME_ADM_CMD_CREATE_SQ = 0x01,
> @@ -427,6 +468,11 @@ enum NvmeStatusCodes {
> NVME_CMD_ABORT_MISSING_FUSE = 0x000a,
> NVME_INVALID_NSID = 0x000b,
> NVME_CMD_SEQ_ERROR = 0x000c,
> +NVME_INVALID_SGL_SEG_DESCRIPTOR = 0x000d,
> +NVME_INVALID_NUM_SGL_DESCRIPTORS = 0x000e,
> +NVME_DATA_SGL_LENGTH_INVALID = 0x000f,
> +NVME_METADATA_SGL_LENGTH_INVALID = 0x0010,
> +NVME_SGL_DESCRIPTOR_TYPE_INVALID = 0x0011,
> NVME_INVALID_USE_OF_CMB = 0x0012,
> NVME_LBA_RANGE = 0x0080,
> NVME_CAP_EXCEEDED = 0x0081,
> @@ -623,6 +669,16 @@ enum NvmeIdCtrlOncs {
> #define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
> #define NVME_CTRL_CQES_MAX(cqes) (((cqes) >> 4) & 0xf)
>
> +#define NVME_CTRL_SGLS_SUPPORTED(sgls) ((sgls) & 0x3)
> +#define NVME_CTRL_SGLS_SUPPORTED_NO_ALIGNMENT(sgls)((sgls) & (0x1 << 0))
> +#define NVME_CTRL_SGLS_SUPPORTED_DWORD_ALIGNMENT(sgls) ((sgls) & (0x1 << 1))
> +#define NVME_CTRL_SGLS_KEYED(sgls) ((sgls) & (0x1 << 2))
> +#define NVME_CTRL_SGLS_BITBUCKET(sgls) ((sgls) & (0x1 << 16))
> +#define NVME_CTRL_SGLS_MPTR_CONTIGUOUS(sgls) ((sgls) & (0x1 << 17))
> +#define NVME_CTRL_SGLS_EXCESS_LENGTH(sgls) ((sgls) & (0x1 << 18))
> +#define NVME_CTRL_SGLS_MPTR_SGL(sgls) ((sgls) & (0x1 << 19))
> +#define NVME_CTRL_SGLS_ADDR_OFFSET(sgls) ((sgls) & (0x1 << 20))
> +
> typedef struct NvmeFeatureVal {
> uint32_tarbitration;
> uint32_tpower_mgmt;
Best regards,
Maxim Levitsky
olqs_created;
> uint32_tpage_size;
> @@ -203,7 +194,6 @@ typedef struct NvmeCtrl {
> uint32_treg_size;
> uint32_tnum_namespaces;
> uint32_tmax_q_ents;
> -uint64_tns_size;
> uint8_t outstanding_aers;
> uint32_tcmbsz;
> uint32_tcmbloc;
> @@ -219,7 +209,8 @@ typedef struct NvmeCtrl {
> QTAILQ_HEAD(, NvmeAsyncEvent) aer_queue;
> int aer_queued;
>
> -NvmeNamespace *namespaces;
> +NvmeNamespace namespace;
> +NvmeNamespace *namespaces[NVME_MAX_NAMESPACES];
> NvmeSQueue **sq;
> NvmeCQueue **cq;
> NvmeSQueue admin_sq;
> @@ -228,9 +219,13 @@ typedef struct NvmeCtrl {
> NvmeFeatureVal features;
> } NvmeCtrl;
>
> -static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, NvmeNamespace *ns)
> +static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
> {
> -return n->ns_size >> nvme_ns_lbads(ns);
> +if (!nsid || nsid > n->num_namespaces) {
> +return NULL;
> +}
> +
> +return n->namespaces[nsid - 1];
> }
>
> static inline uint16_t nvme_cid(NvmeRequest *req)
> @@ -253,4 +248,6 @@ static inline NvmeCtrl *nvme_ctrl(NvmeRequest *req)
> return req->sq->ctrl;
> }
>
> +int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp);
> +
> #endif /* HW_NVME_H */
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 81d69e15fc32..aaf1fcda7923 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -29,6 +29,7 @@ hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads,
> uint32_t secs, int t
>
> # nvme.c
> # nvme traces for successful events
> +nvme_dev_register_namespace(uint32_t nsid) "nsid %"PRIu32""
> nvme_dev_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u"
> nvme_dev_irq_pin(void) "pulsing IRQ pin"
> nvme_dev_irq_masked(void) "IRQ is masked"
> @@ -38,7 +39,7 @@ nvme_dev_map_sgl(uint16_t cid, uint8_t typ, uint32_t nlb,
> uint64_t len) "cid %"P
> nvme_dev_req_register_aio(uint16_t cid, void *aio, const char *blkname,
> uint64_t offset, uint64_t count, const char *opc, void *req) "cid %"PRIu16"
> aio %p blk \"%s\" offset %"PRIu64" count
> %"PRIu64" opc \"%s\" req %p"
> nvme_dev_aio_cb(uint16_t cid, void *aio, const char *blkname, uint64_t
> offset, const char *opc, void *req) "cid %"PRIu16" aio %p blk \"%s\" offset
> %"PRIu64" opc \"%s\" req %p"
> nvme_dev_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode)
> "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
> -nvme_dev_rw(const char *verb, uint32_t blk_count, uint64_t byte_count,
> uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
> +nvme_dev_rw(uint16_t cid, const char *verb, uint32_t nsid, uint32_t nlb,
> uint64_t count, uint64_t lba) "cid %"PRIu16" %s nsid %"PRIu32" nlb %"PRIu32"
> count %"PRIu64" lba 0x%"PRIx64""
> nvme_dev_rw_cb(uint16_t cid, uint32_t nsid) "cid %"PRIu16" nsid %"PRIu32""
> nvme_dev_write_zeros(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t
> nlb) "cid %"PRIu16" nsid %"PRIu32" slba %"PRIu64" nlb %"PRIu32""
> nvme_dev_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t
> qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64",
> sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16",
> qflags=%"PRIu16""
> @@ -94,7 +95,8 @@ nvme_dev_err_invalid_prplist_ent(uint64_t prplist) "PRP
> list entry is null or no
> nvme_dev_err_invalid_prp2_align(uint64_t prp2) "PRP2 is not page aligned:
> 0x%"PRIx64""
> nvme_dev_err_invalid_prp2_missing(void) "PRP2 is null and more data to be
> transferred"
> nvme_dev_err_invalid_prp(void) "invalid PRP"
> -nvme_dev_err_invalid_ns(uint32_t ns, uint32_t limit) "invalid namespace %u
> not within 1-%u"
> +nvme_dev_err_invalid_ns(uint32_t nsid, uint32_t nn) "nsid %"PRIu32" nn
> %"PRIu32""
> +nvme_dev_err_inactive_ns(uint32_t nsid, uint32_t nn) "nsid %"PRIu32" nn
> %"PRIu32""
> nvme_dev_err_invalid_opc(uint8_t opc) "invalid opcode 0x%"PRIx8""
> nvme_dev_err_invalid_admin_opc(uint8_t opc) "invalid admin opcode 0x%"PRIx8""
> nvme_dev_err_invalid_lba_range(uint64_t start, uint64_t len, uint64_t limit)
> "Invalid LBA start=%"PRIu64" len=%"PRIu64" limit=%"PRIu64""
Best regards,
Maxim Levitsky
AT_NVME0x0010
> #define PCI_DEVICE_ID_REDHAT_QXL 0x0100
>
> #define FMT_PCIBUS PRIx64
Other than the actual ID assignment which is not something
I can approve/allocate:
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
> {
> -switch (cmd->opcode) {
> + switch (req->cmd.opcode) {
> case NVME_ADM_CMD_DELETE_SQ:
> -return nvme_del_sq(n, cmd);
> +return nvme_del_sq(n, req);
> case NVME_ADM_CMD_CREATE_SQ:
> -return nvme_create_sq(n, cmd);
> +return nvme_create_sq(n, req);
> case NVME_ADM_CMD_GET_LOG_PAGE:
> -return nvme_get_log(n, cmd, req);
> +return nvme_get_log(n, req);
> case NVME_ADM_CMD_DELETE_CQ:
> -return nvme_del_cq(n, cmd);
> +return nvme_del_cq(n, req);
> case NVME_ADM_CMD_CREATE_CQ:
> -return nvme_create_cq(n, cmd);
> +return nvme_create_cq(n, req);
> case NVME_ADM_CMD_IDENTIFY:
> -return nvme_identify(n, cmd, req);
> +return nvme_identify(n, req);
> case NVME_ADM_CMD_ABORT:
> -return nvme_abort(n, cmd, req);
> +return nvme_abort(n, req);
> case NVME_ADM_CMD_SET_FEATURES:
> -return nvme_set_feature(n, cmd, req);
> +return nvme_set_feature(n, req);
> case NVME_ADM_CMD_GET_FEATURES:
> -return nvme_get_feature(n, cmd, req);
> +return nvme_get_feature(n, req);
> case NVME_ADM_CMD_ASYNC_EV_REQ:
> -return nvme_aer(n, cmd, req);
> +return nvme_aer(n, req);
> default:
> -trace_nvme_dev_err_invalid_admin_opc(cmd->opcode);
> +trace_nvme_dev_err_invalid_admin_opc(req->cmd.opcode);
> return NVME_INVALID_OPCODE | NVME_DNR;
> }
> }
> @@ -1919,8 +1917,8 @@ static void nvme_process_sq(void *opaque)
> req->cqe.cid = cmd.cid;
> memcpy(&req->cmd, &cmd, sizeof(NvmeCmd));
>
> -status = sq->sqid ? nvme_io_cmd(n, &cmd, req) :
> -nvme_admin_cmd(n, &cmd, req);
> +status = sq->sqid ? nvme_io_cmd(n, req) :
> +nvme_admin_cmd(n, req);
> if (status != NVME_NO_COMPLETE) {
> req->status = status;
> nvme_enqueue_req_completion(cq, req);
Other that line wrapping issues,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
} NvmeParams;
>
> typedef struct NvmeAsyncEvent {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 3e288bfceb7f..984412d98c9d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -34,6 +34,7 @@ GlobalProperty hw_compat_4_2[] = {
> { "vhost-blk-device", "seg_max_adjust", "off"},
> { "usb-host", "suppress-remote-wake", "off" },
> { "usb-redir", "suppress-remote-wake", "off" },
> +{ "nvme", "x-use-intel-id", "on"},
> };
> const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
>
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
he parameter is between 9 and 12 before
> > trusting it can be used safely.
> >
> > Alternatively, add supported formats to the lbaf array and let the host
> > decide on a live system with the 'format' command.
>
> The device does not yet support Format NVM, but we have a patch ready
> for that to be submitted with a new series when this is merged.
>
> For now, while it does not support Format, I will change this patch such
> that it defaults to 9 (BRDV_SECTOR_BITS) and only accept 12 as an
> alternative (while always keeping the number of formats available to 1).
Looks like a good idea.
Best regards,
Maxim Levitsky
On Wed, 2020-02-12 at 14:08 +0100, Klaus Birkelund Jensen wrote:
> On Feb 12 11:08, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > Change the prefix of all nvme device related trace events to 'nvme_dev'
> > > to not
f-by: Eric Blake
> > ---
> > block/nbd.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
>
> Reviewed-by: Ján Tomko
>
> Jano
Note that I had a bug open for this.
https://bugzilla.redhat.com/show_bug.cgi?id=1728545
I expected this to be a feature to be honest,
and was afraid to break existing users that might rely on this.
Best regards,
Maxim Levitsky
"active", "secret": "CIA/GRU/MI6", "keyslot": 0 }
>
> * Deactivate a specific keyslot:
>
> { "state": "inactive", "keyslot": 0 }
>
> Possibly less dangerous:
>
> { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" }
>
> Option: Make use of Max's patches to support optional union tag with
> default value to let us default @state to "active". I doubt this makes
> much of a difference in QMP. A human-friendly interface should probably
> be higher level anyway (Daniel pointed to cryptsetup).
Also agree.
>
> Option: LUKSKeyslotInactive member @old-secret could also be named
> @secret. I don't care.
I prefer old-secret.
>
> Option: delete @keyslot. It provides low-level slot access.
> Complicates the interface. Fine if we need lov-level slot access. Do
> we?
I don't have strong opinion on that. I'll probably would like to keep
this for tests/debugging/etc.
>
> I apologize for the time it has taken me to write this.
Thank you very much for doing this.
>
> Comments?
Looks good to me.
Best regards,
Maxim Levitsky
On Mon, 2020-02-17 at 07:45 +0100, Markus Armbruster wrote:
> Maxim Levitsky writes:
>
> > On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote:
> > > Review of this patch led to a lengthy QAPI schema design discussion.
> > > Let me try to conden
tate": "inactive", "old-secret": "CIA/GRU/MI6" }
> >
> > * Add a secret to a specific keyslot:
> >
> > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 }
> >
> &g
, added a note to the new test why the second case
> is expected to fail (as requested by Maxim), and applied the series to
> my block branch:
>
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
>
> Max
>
Thank you too!
Best regards,
Maxim Levitsky
On Fri, 2020-02-07 at 18:28 +, Dr. David Alan Gilbert wrote:
> * Maxim Levitsky (mlevi...@redhat.com) wrote:
> > On Mon, 2020-02-03 at 19:57 +, Dr. David Alan Gilbert wrote:
> > > * Maxim Levitsky (mlevi...@redhat.com) wrote:
> > > > This patch series i
On Mon, 2020-02-24 at 14:46 +, Daniel P. Berrangé wrote:
> On Mon, Feb 17, 2020 at 01:07:23PM +0200, Maxim Levitsky wrote:
> > On Mon, 2020-02-17 at 11:37 +0100, Kevin Wolf wrote:
> > > Am 15.02.2020 um 15:51 hat Markus Armbruster geschrieben:
> > > > Review of th
conflict by picking "active, both absent":
> > >
> > > keyslot old-secret slot(s) selected
> > > absent absent one inactive slot if exist, else error
> > > present absent the slot given by @keyslot
> > > absent present all active slots holding @old-secret
> > > present present the slot given by @keyslot, error unless
> > > it's active holding @old-secret
> > >
> > > Changes:
> > >
> > > * inactive, both absent: changed; we select "one inactive slot" instead of
> > > "all slots".
> > >
> > > "All slots" is a no-op when the current state has no active keyslots,
> > > else error.
> > >
> > > "One inactive slot" is a no-op when the current state has one, else
> > > error. Thus, we no-op rather than error in some states.
> > >
> > > * active, keyslot absent or present, old-secret present: new; selects
> > > active slot(s) holding @old-secret, no-op when old-secret == secret,
> > > else error (no in place update)
> > >
> > > Can do. It's differently irregular, and has a few more combinations
> > > that are basically useless, which I find unappealing. Matter of taste,
> > > I guess.
> > >
> > > Anyone got strong feelings here?
> >
> > The only strong feeling I have is that I absolutely don’t have a strong
> > feeling about this. :)
> >
> > As such, I think we should just treat my rambling as such and stick to
> > your proposal, since we’ve already gathered support for it.
>
> Thanks!
So in summary, do I have the green light to implement the Markus's proposal as
is?
Best regards,
Maxim Levitsky
So far I see the following solutions
1. Drop the QCryptoBlockAmendOptionsLUKS union for now.
This will bring the schema pretty much to be the same as my original proposal,
however the API will be the same thus once nested unions are implemented this
union
can always be introduced again.
2. Drop the QCryptoBlockAmendOptions union. Strictly speaking this union is not
needed
since it only has one member anyway, however this union is used both by qcow2
QAPI scheme,
so that it doesn't hardcode an encryption format for amend just like it doesn't
for creation,
(this can be hardcoded for now as well for now as long as we don't have more
amendable encryption formats).
However I also use the QCryptoBlockAmendOptions in C code in QCrypto API thus
it will be ugly to use the
QCryptoBlockAmendOptionsLUKS instead.
3. Make QCryptoBlockAmendOptionsLUKS a struct and add to it a nested member
with new union type
(say QCryptoBlockAmendOptionsLUKS1) which will be exactly as
QCryptoBlockAmendOptionsLUKS was.
This IMHO is even uglier since it changes the API (which we can't later fix)
and adds both a dummy struct
field and a dummy struct name.
I personally vote 1.
Best regards,
Maxim Levitsky
On Tue, 2020-03-03 at 18:10 +0100, Kevin Wolf wrote:
> Am 30.01.2020 um 13:34 hat Maxim Levitsky geschrieben:
> > This is only used by hmp_drive_add.
> > The code is just a bit shorter this way.
> >
> > No functional changes
> >
> > Signed-off-by:
On Tue, 2020-03-03 at 18:15 +0100, Kevin Wolf wrote:
> Am 30.01.2020 um 13:34 hat Maxim Levitsky geschrieben:
> > Signed-off-by: Maxim Levitsky
> > Reviewed-by: Dr. David Alan Gilbert
>
> Very long subject line. I suppose the license notice should be in the
> body instea
On Tue, 2020-03-03 at 11:18 +0200, Maxim Levitsky wrote:
> On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote:
> > Review of this patch led to a lengthy QAPI schema design discussion.
> > Let me try to condense it into a concrete proposal.
> >
> > This is
e that this is right thing to do.
Changes from V4:
* Rebase with recent changes
* Fixed review feedback
Best regards,
Maxim Levitsky
Maxim Levitsky (11):
usb/dev-storage: remove unused include
monitor/hmp: inline add_init_drive
monitor/hmp: rename device-hotplug.c to block/moni
Signed-off-by: Maxim Levitsky
Reviewed-by: Philippe Mathieu-Daudé
---
hw/usb/dev-storage.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 90da008df1..5629213d55 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -19,7 +19,6
thus according
to LICENSE is under GPLv2+
Signed-off-by: Maxim Levitsky
Reviewed-by: Markus Armbruster
---
MAINTAINERS | 1 +
Makefile.objs| 2 +-
block/Makefile.objs | 1 +
block/monitor
This function is only used by hmp_drive_add.
The code is just a bit shorter this way.
No functional changes
Signed-off-by: Maxim Levitsky
Reviewed-by: Markus Armbruster
---
device-hotplug.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 108 -
blockdev.c | 96 +
include/block/block-hmp-cmds.h | 4 ++
include/sysemu/blockdev.h | 4 --
4
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 52 ++
include/block/block-hmp-cmds.h | 6
include/monitor/hmp.h | 5
monitor/hmp-cmds.c | 52
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 60 ++
include/block/block-hmp-cmds.h | 12 +--
include/monitor/hmp.h | 2 --
monitor/hmp-cmds.c | 58
hmp_snapshot_blkdev is from GPLv2 version of the hmp-cmds.c thus
have to change the licence to GPLv2
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 58 --
include/block/block-hmp-cmds.h | 4 +++
include
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 389 +
include/block/block-hmp-cmds.h | 4 +
include/monitor/hmp.h | 4 -
monitor/hmp-cmds.c | 388
4
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 101 +
include/block/block-hmp-cmds.h | 5 ++
include/monitor/hmp.h | 4 --
monitor/hmp-cmds.c | 100
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 30
blockdev.c | 43 +++---
include/block/block_int.h | 5 ++--
3 files changed, 41 insertions(+), 37 deletions
Signed-off-by: Maxim Levitsky
Reviewed-by: Dr. David Alan Gilbert
---
block/monitor/block-hmp-cmds.c | 140 +
include/block/block-hmp-cmds.h | 9 +++
include/monitor/hmp.h | 6 --
monitor/hmp-cmds.c | 137
Sorry for noise.
Best regards,
Maxim Levitsky
On Sun, 2020-03-08 at 11:24 +0200, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky
> Reviewed-by: Dr. David Alan Gilbert
> ---
> block/monitor/block-hmp-cmds.c | 60 ++
> include/bl
encryption slot management
also using
the code from patches 1,2, and also add a bunch of iotests to cover this.
Tested with -raw,-qcow2 and -luks iotests and 'make check'
Best regards,
Maxim Levitsky
clone of "luks-keymgmnt-v2"
Maxim Levitsky (14):
qcrypto/core:
Next few patches will expose that functionality
to the user.
Signed-off-by: Maxim Levitsky
---
crypto/block-luks.c | 398 +++-
qapi/crypto.json| 61 ++-
2 files changed, 455 insertions(+), 4 deletions(-)
diff --git a/crypto/block-luks.c b/crypto
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
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
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
---
block/qcow2.c | 160 +-
include
ange for the idea 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
---
block/crypto.c | 127 +++--
block/crypto.h | 44 +++--
2 files cha
701 - 800 of 959 matches
Mail list logo
