t lid, uint8_t rae, uint32_t len,
> uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off
> %"PRIu64""
> 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""
> @@ -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"
Best regards,
Maxim Levitsky
a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -688,7 +688,13 @@ enum NvmeIdCtrlOncs {
> typedef struct NvmeFeatureVal {
> uint32_tarbitration;
> uint32_tpower_mgmt;
> -uint32_ttemp_thresh;
> +union {
> +struct {
> +uint16_t temp_thresh_hi;
> +uint16_t temp_thresh_low;
> +};
> +uint32_t temp_thresh;
> +};
> uint32_terr_rec;
> uint32_tvolatile_wc;
> uint32_tnum_queues;
With 'temperature' field removed from the header:
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
trace_nvme_dev_err_invalid_identify_cns(le32_to_cpu(c->cns));
This is a very good candidate to be squished with the patch 5 IMHO,
but you can leave this as is as well. I don't mind.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
465560eea 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -67,6 +67,22 @@ typedef struct NvmeNamespace {
> NvmeIdNsid_ns;
> } NvmeNamespace;
>
> +static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns)
> +{
> +NvmeIdNs *id_ns = >id_ns;
> +return _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)
> @@ -88,8 +104,6 @@ typedef struct NvmeCtrl {
> uint32_tnum_namespaces;
> uint32_tmax_q_ents;
> uint64_tns_size;
> -uint32_tcmbsz;
> -uint32_tcmbloc;
> uint8_t *cmbuf;
> uint64_tirq_status;
> uint64_thost_timestamp; /* Timestamp sent by the
> host */
> @@ -103,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);
> +}
> +
> #endif /* HW_NVME_H */
Small nitpick: To be honest this not only refactoring in the device realization
since you also (rightfully)
removed the duplicated cmbsz/cmbloc so I would add a mention for this in the
commit message.
But that doesn't matter that much, so
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
ommand Limit
> (four
> + * concurrently outstanding Abort commands), so lets use that though it
> is
> + * inconsequential.
> + */
> +id->acl = 3;
> id->frmw = 7 << 1;
> id->lpa = 1 << 0;
> id->sqes = (0x6 << 4) | 0x6;
You forgot to move my reviewed-by from the previous version
I see that you also fixed the white space problem, thanks!
So,
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
}
>
> static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
Note that this patch still contains a bug that it removes the check against the
accessed
size, which you fix in later patch.
I prefer to not add a bug in first place
However if you have a reason for this, I won't mind.
Best regards,
Maxim Levitsky
"PRIu16""
> +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_
don't mind leaving this as is.
Fine grained patches never cause any harm.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
values as well
> +#define NVME_TEMP_TMPSEL(temp) ((temp >> 16) & 0xf)
> +#define NVME_TEMP_TMPTH(temp) ((temp >> 0) & 0x)
> +
> enum NvmeFeatureIds {
> NVME_ARBITRATION= 0x1,
> NVME_POWER_MANAGEMENT = 0x2,
> @@ -653,18 +753,37 @@ typedef struct NvmeIdNs {
> uint8_t mc;
> uint8_t dpc;
> uint8_t dps;
> -
> uint8_t nmic;
> uint8_t rescap;
> uint8_t fpi;
> uint8_t dlfeat;
> -
> -uint8_t res34[94];
> +uint16_tnawun;
> +uint16_tnawupf;
> +uint16_tnacwu;
> +uint16_tnabsn;
> +uint16_tnabo;
> +uint16_tnabspf;
> +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];
> uint8_t vs[3712];
> } NvmeIdNs;
Also checked this against V5, looks OK now
>
> +typedef struct NvmeIdNsDescr {
> +uint8_t nidt;
> +uint8_t nidl;
> +uint8_t rsvd2[2];
> +} NvmeIdNsDescr;
OK
> +
> +#define NVME_NIDT_UUID_LEN 16
> +
> +enum {
> +NVME_NIDT_UUID = 0x3,
Very minor nitpick: I'll would add others as well just for the sake
of better understanding what this is
> +};
>
> /*Deallocate Logical Block Features*/
> #define NVME_ID_NS_DLFEAT_GUARD_CRC(dlfeat) ((dlfeat) & 0x10)
Looks very good.
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Move device configuration parameters to separate struct to make it
> explicit what is configurable and what is set internally.
>
> Signed-off-by: Klaus Jensen
> Acked-by: Keith Busch
> Rev
of sense.
I don't think this is bad.
You might actually found the ultimate question of life the universe and
everything.
;-)
Best regards,
Maxim Levitsky
>
> v6 primarily splits up the big nasty patches into more digestible parts.
> Specifically the 'nvme: refactor prp mapping' and
On Mon, 2020-03-16 at 00:45 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 11:35, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > Add support for the Get Log Page command and basic implementations of
> > > the mandatory Error
On Mon, 2020-03-16 at 00:54 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 14:07, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:52 +0100, Klaus Jensen wrote:
> > > For now, support the Data Block, Segment and Last Segment descriptor
> > > types.
> > >
On Mon, 2020-03-16 at 00:55 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 14:34, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:52 +0100, Klaus Jensen wrote:
> > > This adds support for multiple namespaces by introducing a new 'nvme-ns'
> > > device model. Th
On Mon, 2020-03-16 at 00:50 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 12:35, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > Add new fields to the Identify Controller and Identify Namespace data
> > > structures a
On Mon, 2020-03-16 at 00:51 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 13:44, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > Refactor nvme_map_prp and allow PRPs to be located in the CMB. The logic
> > > ensures that if so
On Mon, 2020-03-16 at 00:48 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 12:30, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > 0x is not an allowed value for NCQR and NSQR in Set Features on
> > > Number of Queues.
> &
On Mon, 2020-03-16 at 00:53 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 13:48, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > This refactors how the device issues asynchronous block backend
> > > requests. The NvmeRequest no
On Mon, 2020-03-16 at 00:53 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 13:52, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:52 +0100, Klaus Jensen wrote:
> > > Handling DMA errors gracefully is required for the device to pass the
> > > block/011 test ("di
On Mon, 2020-03-16 at 00:45 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 11:35, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > Add support for the Get Log Page command and basic implementations of
> > > the mandatory Error
On Mon, 2020-03-16 at 00:47 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 12:27, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > Add support for returning a resonable response to Get/Set Features of
> > > mandatory featur
On Mon, 2020-03-16 at 00:44 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 11:31, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > It might seem wierd to implement this feature for an emulated device,
> > > but it is mandatory
On Mon, 2020-03-16 at 00:43 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 11:27, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > This patch splits up nvme_realize into multiple individual functions,
> > > each initializing a dif
On Thu, 2020-03-12 at 06:56 -0500, Eric Blake wrote:
> On 3/8/20 10:18 AM, Maxim Levitsky wrote:
> > Hi!
> > Here is the updated series of my patches, incorporating all the feedback I
> > received.
> >
> > Patches are strictly divided by topic to 3 groups, and
On Tue, 2020-03-10 at 14:02 +0200, Maxim Levitsky wrote:
> On Tue, 2020-03-10 at 12:59 +0100, Kevin Wolf wrote:
> > Am 10.03.2020 um 12:05 hat Maxim Levitsky geschrieben:
> > > On Tue, 2020-03-10 at 11:58 +0100, Max Reitz wrote:
> > > > On 08.03.20 16:18, Maxim Levi
On Tue, 2020-03-10 at 12:59 +0100, Kevin Wolf wrote:
> Am 10.03.2020 um 12:05 hat Maxim Levitsky geschrieben:
> > On Tue, 2020-03-10 at 11:58 +0100, Max Reitz wrote:
> > > On 08.03.20 16:18, Maxim Levitsky wrote:
> > > > Next few patches will expose that fun
On Tue, 2020-03-10 at 11:58 +0100, Max Reitz wrote:
> On 08.03.20 16:18, Maxim Levitsky wrote:
> > Next few patches will expose that functionality
> > to the user.
> >
> > Signed-off-by: Maxim Levitsky
> > ---
> > crypto/block-luks.c | 398 +++
On Mon, 2020-03-09 at 18:30 +, 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
On Mon, 2020-03-09 at 16:31 +, Dr. David Alan Gilbert wrote:
> * Maxim Levitsky (mlevi...@redhat.com) wrote:
> >
> > I see that I have the same issue of long subject line here.
> > Its because I forgot the space after first line, when adding this.
> > If I nee
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
Signed-off-by: Maxim Levitsky
Reviewed-by: Daniel P. Berrangé
---
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
index 727a3fde58
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
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
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/qemu
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 | 278 +
tests/qemu-iotests/302.out | 40 ++
tests/qemu-iotests/303 | 233
'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
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
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 changed, 163 insertions
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
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 | 80 --
tests/qemu-iotests/082.out | 45 +
2 files changed, 114
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
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
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: add generic infr
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/block/block-
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 | 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 | 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
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 | 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 | 52 ++
include/block/block-hmp-cmds.h | 6
include/monitor/hmp.h | 5
monitor/hmp-cmds.c | 52
and 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
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
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
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/monitor/block-hmp-cmds.c
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
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.
> >
> > Th
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 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:
w2
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
ive 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
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
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
, 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
"state": "inactive", "keyslot": 0 }
> >
> > Possibly less dangerous:
> >
> > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" }
> >
> > Option: Make use of
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
ot;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
t; > ---
> > 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
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 clash with
r 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
*req)
> +static uint16_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(>cmd, , sizeof(NvmeCmd));
>
> -status = sq->sqid ? nvme_io_cmd(n, , req) :
> -nvme_admin_cmd(n, , 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
rams;
>
> 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
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
ze;
> 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
pe;
> +} 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
"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
bytes = qemu_iovec_from_buf(>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();
> -
> return status;
> }
>
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
; }
>
> 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
gt; --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -27,11 +27,11 @@ typedef struct NvmeRequest {
> struct NvmeSQueue *sq;
> BlockAIOCB *aiocb;
> uint16_tstatus;
> -bool has_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
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
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
ot;
> 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
le 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
uot;
> 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
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
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
r_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 = >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
> (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
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(>parent_obj, addr, buf, size);
> }
>
> static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
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
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
INT32("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 num_queues;
> +uint32_t cmb_size_mb;
> +} NvmeParams;
&g
quot;"
> +nvme_dev_err_invalid_setfeat(uint32_t dw10) "invalid set features,
> dw10=0
d);
> -break;
> -
> default:
> trace_nvme_dev_err_invalid_setfeat(dw10);
> return NVME_INVALID_FIELD | NVME_DNR;
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
ion 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
mon(PCIQXLDevice *qxl,
> Error **errp)
> pci_device_rev = QXL_REVISION_STABLE_V12;
> io_size = pow2ceil(QXL_IO_RANGE_SIZE);
> break;
> + case 5: /* qxl-5 */
> +pci_device_rev = QXL_REVISION_STABLE_V12 + 1;
> +io_size = pow2ceil(QXL_IO_RANGE_SIZE);
> +break;
> default:
> error_setg(errp, "Invalid revision %d for qxl device (max %d)",
> qxl->revision, QXL_DEFAULT_REVISION);
Reviewed-by: Maxim Levitsky
Best regards,
Maxim Levitsky
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
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/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é
---
block/qcow2.c| 39 +++
qapi/block-core.json | 16 +++-
2 files
501 - 600 of 1129 matches
Mail list logo
