Re: [PATCH 2/6] Python: expose QEMUMachine's temporary directory

2021-02-17 Thread Wainer dos Santos Moschetta



On 2/15/21 7:27 PM, John Snow wrote:

On 2/15/21 1:50 PM, Wainer dos Santos Moschetta wrote:


In qtest.QEMUQtestMachine.__init__(), the argument named 'test_dir' 
still make sense, right?


- Wainer


It might upset pylint/mypy to rename parameters in the initializer for 
a parent class. If we rename it in the base class, we should rename it 
in the descendants, too.


(I say "might" because I have not yet worked out under the exact 
conditions that mypy will give you LSP warnings for initializer 
methods. It definitely doesn't always seem to, but I have run afoul of 
it enough times that I try to avoid it as a matter of habit now.)


Thanks for the explanation, John!

So Cleber just got another:

Reviewed-by: Wainer dos Santos Moschetta 











Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

2021-02-17 Thread Wainer dos Santos Moschetta



On 2/15/21 11:34 PM, Cleber Rosa wrote:

On Mon, Feb 15, 2021 at 03:30:16PM -0300, Wainer dos Santos Moschetta wrote:

Hi,

On 2/11/21 7:01 PM, Cleber Rosa wrote:

Closing a file that is open for writing, and then reading from it
sounds like a better idea than the opposite, given that the content
will be flushed.

Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
Signed-off-by: Cleber Rosa 
---
   python/qemu/machine.py | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 7a40f4604b..6e44bda337 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -337,12 +337,12 @@ class QEMUMachine:
   self._qmp.close()
   self._qmp_connection = None
-self._load_io_log()
-
   if self._qemu_log_file is not None:
   self._qemu_log_file.close()
   self._qemu_log_file = None
+self._load_io_log()
+


IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` being
called before `self._load_io_log()` but a future change can make this
condition unmet again. Maybe you could document that in the code. Or change
the `_load_io_log()` to close the log file if it is open (also document it
in code).

- Wainer


I'm glad you see this is needed... and then something else.  I'll investigate
making this more robust as time allows it.

Question is: do you ack/nack this change?


hmm... /me thinking hmmm... okay, good deal. :)

Acked-by: Wainer dos Santos Moschetta 




Cheers,
- Cleber.





Re: [PATCH v4 08/24] python: Add pipenv support

2021-02-17 Thread Cleber Rosa
On Wed, Feb 17, 2021 at 12:28:22PM -0500, John Snow wrote:
> On 2/16/21 10:02 PM, Cleber Rosa wrote:
> > On Tue, Feb 16, 2021 at 09:59:47PM -0500, Cleber Rosa wrote:
> > > On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
> > > > pipenv is a tool used for managing virtual environments with pinned,
> > > > explicit dependencies. It is used for precisely recreating python
> > > > virtual environments.
> > > > 
> > > > pipenv uses two files to do this:
> > > > 
> > > > (1) Pipfile, which is similar in purpose and scope to what setup.py
> > > > lists. It specifies the requisite minimum to get a functional
> > > > environment for using this package.
> > > > 
> > > > (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> > > > requirements.txt`. It specifies a canonical virtual environment used for
> > > > deployment or testing. This ensures that all users have repeatable
> > > > results.
> > > > 
> > > > The primary benefit of using this tool is to ensure repeatable CI
> > > > results with a known set of packages. Although I endeavor to support as
> > > > many versions as I can, the fluid nature of the Python toolchain often
> > > > means tailoring code for fairly specific versions.
> > > > 
> > > > Note that pipenv is *not* required to install or use this module; this 
> > > > is
> > > > purely for the sake of repeatable testing by CI or developers.
> > > > 
> > > > Here, a "blank" pipfile is added with no dependencies, but specifies
> > > > Python 3.6 for the virtual environment.
> > > > 
> > > > Pipfile will specify our version minimums, while Pipfile.lock specifies
> > > > an exact loudout of packages that were known to operate correctly. This
> > > 
> > > Layout? Loadout?
> > > 
> > > > latter file provides the real value for easy setup of container images
> > > > and CI environments.
> > > > 
> > > > Signed-off-by: John Snow 
> > > > ---
> > > >   python/Pipfile | 11 +++
> > > >   1 file changed, 11 insertions(+)
> > > >   create mode 100644 python/Pipfile
> > > > 
> > > 
> > > Other than that,
> > > 
> > > Reviewed-by: Cleber Rosa 
> > 
> > Actually, just one suggestion: bump the position of this patch twice.
> > It makes it easier to understand its purpose if it is placed right
> > before the "python: add pylint to pipenv" patch.
> > 
> > Cheers,
> > - Cleber.
> > 
> 
> The way the series is laid out is:
> 
> 01-02: pre-requisite fixes
> 03-07: Create the package, readmes, etc.
> 08:Pipenv support
> 09-11: Pylint
> 12-13: flake8
> 14-15: mypy
> 16-17: isort
> 18-20: Testing and pre-requisites
> 21-23: Polish
> 24: CI support
> 
> Moving the pipenv patch to just before the final pylint patch works OK, but
> breaks up the pylint section. Should I still do it?
>

OK, now with that approach of groupping in min, it sounds reasonable.
My previous point was that pipenv is not needed until right before #10,
but that's just nitpicking and almost bikeshedding (I won't admit it
easily).

> --js
> 
> 
> (Hm, by this layout, I should probably actually move the pylint fix in #01
> down to appear after the pipenv patch. I could also move the flake8 fixes in
> #21 up to be near the other flake8 patches.)

Sounds more consistent.

Cheers,
- Cleber.


signature.asc
Description: PGP signature


Re: [PATCH RFC v3 00/12] hw/block/nvme: metadata and end-to-end data protection support

2021-02-17 Thread Keith Busch
On Wed, Feb 17, 2021 at 10:06:49AM +0100, Klaus Jensen wrote:
> On Feb 16 16:19, Keith Busch wrote:
> > The verify implementation looked fine, but lacking a generic backing for
> > it sounds to me the use cases aren't there to justify taking on this
> > feature.
> 
> Please check my reply on the verify patch - can you elaborate on
> "generic backing"? I'm not sure I understand what you have in mind,
> API-wise.

I meant it'd be nice if qemu block api provided a function like
"blk_aio_pverify()" to handle the details that you're implementing in
the nvme device. As you mentioned though, handling the protection
information part is problematic.



Re: [PATCH v4 08/24] python: Add pipenv support

2021-02-17 Thread John Snow

On 2/16/21 10:02 PM, Cleber Rosa wrote:

On Tue, Feb 16, 2021 at 09:59:47PM -0500, Cleber Rosa wrote:

On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:

pipenv is a tool used for managing virtual environments with pinned,
explicit dependencies. It is used for precisely recreating python
virtual environments.

pipenv uses two files to do this:

(1) Pipfile, which is similar in purpose and scope to what setup.py
lists. It specifies the requisite minimum to get a functional
environment for using this package.

(2) Pipfile.lock, which is similar in purpose to `pip freeze >
requirements.txt`. It specifies a canonical virtual environment used for
deployment or testing. This ensures that all users have repeatable
results.

The primary benefit of using this tool is to ensure repeatable CI
results with a known set of packages. Although I endeavor to support as
many versions as I can, the fluid nature of the Python toolchain often
means tailoring code for fairly specific versions.

Note that pipenv is *not* required to install or use this module; this is
purely for the sake of repeatable testing by CI or developers.

Here, a "blank" pipfile is added with no dependencies, but specifies
Python 3.6 for the virtual environment.

Pipfile will specify our version minimums, while Pipfile.lock specifies
an exact loudout of packages that were known to operate correctly. This


Layout? Loadout?


latter file provides the real value for easy setup of container images
and CI environments.

Signed-off-by: John Snow 
---
  python/Pipfile | 11 +++
  1 file changed, 11 insertions(+)
  create mode 100644 python/Pipfile



Other than that,

Reviewed-by: Cleber Rosa 


Actually, just one suggestion: bump the position of this patch twice.
It makes it easier to understand its purpose if it is placed right
before the "python: add pylint to pipenv" patch.

Cheers,
- Cleber.



The way the series is laid out is:

01-02: pre-requisite fixes
03-07: Create the package, readmes, etc.
08:Pipenv support
09-11: Pylint
12-13: flake8
14-15: mypy
16-17: isort
18-20: Testing and pre-requisites
21-23: Polish
24: CI support

Moving the pipenv patch to just before the final pylint patch works OK, 
but breaks up the pylint section. Should I still do it?


--js


(Hm, by this layout, I should probably actually move the pylint fix in 
#01 down to appear after the pipenv patch. I could also move the flake8 
fixes in #21 up to be near the other flake8 patches.)





Re: [PATCH v4 18/24] python/qemu: add qemu package itself to pipenv

2021-02-17 Thread John Snow

On 2/16/21 11:47 PM, Cleber Rosa wrote:

On Thu, Feb 11, 2021 at 01:58:50PM -0500, John Snow wrote:

This adds the python qemu packages themselves to the pipenv manifest.
'pipenv sync' will create a virtual environment sufficient to use the SDK.
'pipenv sync --dev' will create a virtual environment sufficient to use
and test the SDK (with pylint, mypy, isort, flake8, etc.)

The qemu packages are installed in 'editable' mode; all changes made to
the python package inside the git tree will be reflected in the
installed package without reinstallation. This includes changes made
via git pull and so on.

Signed-off-by: John Snow 
---
  python/Pipfile  | 1 +
  python/Pipfile.lock | 9 +++--
  2 files changed, 8 insertions(+), 2 deletions(-)



Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 



Thanks for the reviews so far!




Re: [PATCH v4 15/24] python: add mypy to pipenv

2021-02-17 Thread John Snow

On 2/16/21 11:38 PM, Cleber Rosa wrote:

On Thu, Feb 11, 2021 at 01:58:47PM -0500, John Snow wrote:

0.730 appears to be about the oldest version that works with the
features we want, including nice human readable output (to make sure
iotest 297 passes), and type-parameterized Popen generics.

0.770, however, supports adding 'strict' to the config file, so require
at least 0.770.

Now that we are checking a namespace package, we need to tell mypy to
allow PEP420 namespaces, so modify the mypy config as part of the move.

mypy can now be run from the python root by typing 'mypy qemu'.



  $ mypy qemu
  qemu/utils/accel.py: error: Source file found twice under different module 
names: 'qmp' and 'qemu.qmp'
  Found 1 error in 1 file (errors prevented further checking)

I guess you meant 'mypy -p qemu'.



Ah, crud! Yes, this is something that has popped up recently.

mypy's "figure out where we are when run without arguments" 
functionality does not work exactly correct in some cases.


I forget the specifics, but "mypy qemu" used to work for this series, 
and at some point it ... stopped working. I updated the pytest 
invocation, but I didn't update the comments here.


There's a github meta-issue about this, and about how mypy's package 
discovery is extremely confusing:


https://github.com/python/mypy/issues/8584

It's extremely a big landmine on which you may hoist yourself.


Signed-off-by: John Snow 
---
  python/Pipfile  |  1 +
  python/Pipfile.lock | 37 -
  python/setup.cfg|  1 +
  3 files changed, 38 insertions(+), 1 deletion(-)



With that change,

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 






Re: [PATCH] qsd: Document FUSE exports

2021-02-17 Thread Eric Blake
On 2/17/21 5:58 AM, Max Reitz wrote:
> Implementing FUSE exports required no changes to the storage daemon, so
> we forgot to document them there.  Considering that both NBD and
> vhost-user-blk exports are documented in its man page (and NBD exports
> in its --help text), we should probably do the same for FUSE.
> 
> Signed-off-by: Max Reitz 
> ---
>  docs/tools/qemu-storage-daemon.rst   | 19 +++
>  storage-daemon/qemu-storage-daemon.c |  4 
>  2 files changed, 23 insertions(+)

> @@ -142,6 +153,14 @@ domain socket ``vhost-user-blk.sock``::
>--blockdev driver=qcow2,node-name=qcow2,file=file \
>--export 
> type=vhost-user-blk,id=export,addr.type=unix,addr.path=vhost-user-blk.sock,node-name=qcow2
>  
> +Export a qcow2 image file ``disk.qcow2`` via FUSE on itself, so the disk 
> image
> +file will then appear as a raw image::
> +
> +  $ qemu-storage-daemon \
> +  --blockdev driver=file,node-name=file,filename=disk.qcow2 \
> +  --blockdev driver=qcow2,node-name=qcow2,file=file \
> +  --export 
> type=fuse,id=export,node-name=qcow2,mountpoint=disk.qcow2,writable=on
> +

Should the example also mention how to unmount the file when you're done?

Otherwise looks good to me.  Any documentation is better than none, even
if we can add more, so

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




Re: [PATCH v2 4/6] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()

2021-02-17 Thread Alexander Bulekov
On 210216 1146, Bin Meng wrote:
> s->prnsts is updated in both branches of the if () else () statement.
> Move the common bits outside so that it is cleaner.
> 
> Signed-off-by: Bin Meng 

Reviewed-by: Alexander Bulekov 

> ---
> 
> (no changes since v1)
> 
>  hw/sd/sdhci.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 0b0ca6f..7a2003b 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -598,9 +598,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState 
> *s)
>  page_aligned = true;
>  }
>  
> +s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
>  if (s->trnmod & SDHC_TRNS_READ) {
> -s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
> -SDHC_DAT_LINE_ACTIVE;
> +s->prnsts |= SDHC_DOING_READ;
>  while (s->blkcnt) {
>  if (s->data_count == 0) {
>  sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
> @@ -627,8 +627,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState 
> *s)
>  }
>  }
>  } else {
> -s->prnsts |= SDHC_DOING_WRITE | SDHC_DATA_INHIBIT |
> -SDHC_DAT_LINE_ACTIVE;
> +s->prnsts |= SDHC_DOING_WRITE;
>  while (s->blkcnt) {
>  begin = s->data_count;
>  if (((boundary_count + begin) < block_size) && page_aligned) {
> -- 
> 2.7.4
> 



[PATCH] qsd: Document FUSE exports

2021-02-17 Thread Max Reitz
Implementing FUSE exports required no changes to the storage daemon, so
we forgot to document them there.  Considering that both NBD and
vhost-user-blk exports are documented in its man page (and NBD exports
in its --help text), we should probably do the same for FUSE.

Signed-off-by: Max Reitz 
---
 docs/tools/qemu-storage-daemon.rst   | 19 +++
 storage-daemon/qemu-storage-daemon.c |  4 
 2 files changed, 23 insertions(+)

diff --git a/docs/tools/qemu-storage-daemon.rst 
b/docs/tools/qemu-storage-daemon.rst
index f63627eaf6..f5a906f6fc 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -74,6 +74,7 @@ Standard options:
 .. option:: --export 
[type=]nbd,id=,node-name=[,name=][,writable=on|off][,bitmap=]
   --export 
[type=]vhost-user-blk,id=,node-name=,addr.type=unix,addr.path=[,writable=on|off][,logical-block-size=][,num-queues=]
   --export 
[type=]vhost-user-blk,id=,node-name=,addr.type=fd,addr.str=[,writable=on|off][,logical-block-size=][,num-queues=]
+  --export 
[type=]fuse,id=,node-name=,mountpoint=[,growable=on|off][,writable=on|off]
 
   is a block export definition. ``node-name`` is the block node that should be
   exported. ``writable`` determines whether or not the export allows write
@@ -91,6 +92,16 @@ Standard options:
   ``logical-block-size`` sets the logical block size in bytes (the default is
   512). ``num-queues`` sets the number of virtqueues (the default is 1).
 
+  The ``fuse`` export type takes a mount point, which must be a regular file,
+  on which to export the given block node. That file will not be changed, it
+  will just appear to have the block node's content while the export is active
+  (very much like mounting a filesystem on a directory does not change what the
+  directory contains, it only shows a different content while the filesystem is
+  mounted). Consequently, applications that have opened the given file before
+  the export became active will continue to see its original content. If
+  ``growable`` is set, writes after the end of the exported file will grow the
+  block node to fit.
+
 .. option:: --monitor MONITORDEF
 
   is a QMP monitor definition. See the :manpage:`qemu(1)` manual page for
@@ -142,6 +153,14 @@ domain socket ``vhost-user-blk.sock``::
   --blockdev driver=qcow2,node-name=qcow2,file=file \
   --export 
type=vhost-user-blk,id=export,addr.type=unix,addr.path=vhost-user-blk.sock,node-name=qcow2
 
+Export a qcow2 image file ``disk.qcow2`` via FUSE on itself, so the disk image
+file will then appear as a raw image::
+
+  $ qemu-storage-daemon \
+  --blockdev driver=file,node-name=file,filename=disk.qcow2 \
+  --blockdev driver=qcow2,node-name=qcow2,file=file \
+  --export 
type=fuse,id=export,node-name=qcow2,mountpoint=disk.qcow2,writable=on
+
 See also
 
 
diff --git a/storage-daemon/qemu-storage-daemon.c 
b/storage-daemon/qemu-storage-daemon.c
index 9021a46b3a..bdf8877995 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -97,6 +97,10 @@ static void help(void)
 " export the specified block node over NBD\n"
 " (requires --nbd-server)\n"
 "\n"
+"  --export [type=]fuse,id=,node-name=,mountpoint=\n"
+"   [,growable=on|off][,writable=on|off]\n"
+" export the specified block node over FUSE\n"
+"\n"
 "  --monitor [chardev=]name[,mode=control][,pretty[=on|off]]\n"
 " configure a QMP monitor\n"
 "\n"
-- 
2.29.2




Re: [PATCH RFC v3 12/12] hw/block/nvme: add support for the format nvm command

2021-02-17 Thread Klaus Jensen
On Feb 17 09:26, Klaus Jensen wrote:
> On Feb 16 15:16, Keith Busch wrote:
> > On Mon, Feb 15, 2021 at 12:02:40AM +0100, Klaus Jensen wrote:
> > > From: Minwoo Im 
> > > 
> > > Format NVM admin command can make a namespace or namespaces to be
> > > with different LBA size and metadata size with protection information
> > > types.
> > > 
> > > This patch introduces Format NVM command with LBA format, Metadata, and
> > > Protection Information for the device. The secure erase operation things
> > > are yet to be added.
> > > 
> > > The parameter checks inside of this patch has been referred from
> > > Keith's old branch.
> > 
> > Oh, and here's the format command now, so my previous comment on patch
> > 11 doesn't matter.
> > 
> > > +struct nvme_aio_format_ctx {
> > > +NvmeRequest   *req;
> > > +NvmeNamespace *ns;
> > > +
> > > +/* number of outstanding write zeroes for this namespace */
> > > +int *count;
> > 
> > Shouldn't this count be the NvmeRequest's opaque value?
> 
> That is already occupied by `num_formats` which tracks formats of
> individual namespaces. `count` is for outstanding write zeroes on one
> particular namespace.

And, btw, I have a seperate "aiocblist" RFC patch that replaces this
manual aio tracking in favor of actually tracking multiple aiocbs,
removing the need for for this ad-hoc accounting and fixing the cancel
bug in the process.

On vacation this week, so I expect to post it early next week ;)


signature.asc
Description: PGP signature


Re: [PATCH RFC v3 00/12] hw/block/nvme: metadata and end-to-end data protection support

2021-02-17 Thread Klaus Jensen
On Feb 16 16:19, Keith Busch wrote:
> On Mon, Feb 15, 2021 at 12:02:28AM +0100, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > This is RFC v3 of a series that adds support for metadata and end-to-end 
> > data
> > protection.
> > 
> > First, on the subject of metadata, in v1, support was restricted to
> > extended logical blocks, which was pretty trivial to implement, but
> > required special initialization and broke DULBE. In v2, metadata is
> > always stored continuously at the end of the underlying block device.
> > This has the advantage of not breaking DULBE since the data blocks
> > remains aligned and allows bdrv_block_status to be used to determinate
> > allocation status. It comes at the expense of complicating the extended
> > LBA emulation, but on the other hand it also gains support for metadata
> > transfered as a separate buffer.
> > 
> > The end-to-end data protection support blew up in terms of required
> > changes. This is due to the fact that a bunch of new commands has been
> > added to the device since v1 (zone append, compare, copy), and they all
> > require various special handling for protection information. If
> > potential reviewers would like it split up into multiple patches, each
> > adding pi support to one command, shout out.
> > 
> > The core of the series (metadata and eedp) is preceeded by a set of
> > patches that refactors mapping (yes, again) and tries to deal with the
> > qsg/iov duality mess (maybe also again?).
> > 
> > Support fro metadata and end-to-end data protection is all joint work
> > with Gollu Appalanaidu.
> 
> Patches 1 - 8 look good to me:
> 
> Reviewed-by: Keith Busch 
> 
> I like the LBA format and protection info support too, but might need
> some minor changes.
> 

Cool, thanks for the reviews Keith!

> The verify implementation looked fine, but lacking a generic backing for
> it sounds to me the use cases aren't there to justify taking on this
> feature.

Please check my reply on the verify patch - can you elaborate on
"generic backing"? I'm not sure I understand what you have in mind,
API-wise.


signature.asc
Description: PGP signature


Re: [PATCH RFC v3 09/12] hw/block/nvme: add verify command

2021-02-17 Thread Klaus Jensen
On Feb 16 15:12, Keith Busch wrote:
> On Mon, Feb 15, 2021 at 12:02:37AM +0100, Klaus Jensen wrote:
> > From: Gollu Appalanaidu 
> > 
> > See NVM Express 1.4, section 6.14 ("Verify Command").
> > 
> > Signed-off-by: Gollu Appalanaidu 
> > [k.jensen: rebased, refactored for e2e]
> > Signed-off-by: Klaus Jensen 
> 
> Verify is a generic block command supported in other protocols beyond
> nvme. If we're going to support the command in nvme, I prefer the
> implementation had generic backing out of the qemu block API rather than
> emulate the entirety out of the nvme device.

You mean that the block API could provide a basic "check that we can
read this stuff without error"-call? Sounds reasonable enough, but since
the end-to-end data protection checks are performed in the device, we
need to pass the data buffers up anyway. If we had basic I/O (non-pi)
verify in the block API it would defeat the purpose if it provided those
buffers.

We've actually been asked directly on the availablity of Verify support
in QEMU, so I think this implementation as-is provides something useful
to users.


signature.asc
Description: PGP signature


Re: [PATCH RFC v3 12/12] hw/block/nvme: add support for the format nvm command

2021-02-17 Thread Klaus Jensen
On Feb 16 15:16, Keith Busch wrote:
> On Mon, Feb 15, 2021 at 12:02:40AM +0100, Klaus Jensen wrote:
> > From: Minwoo Im 
> > 
> > Format NVM admin command can make a namespace or namespaces to be
> > with different LBA size and metadata size with protection information
> > types.
> > 
> > This patch introduces Format NVM command with LBA format, Metadata, and
> > Protection Information for the device. The secure erase operation things
> > are yet to be added.
> > 
> > The parameter checks inside of this patch has been referred from
> > Keith's old branch.
> 
> Oh, and here's the format command now, so my previous comment on patch
> 11 doesn't matter.
> 
> > +struct nvme_aio_format_ctx {
> > +NvmeRequest   *req;
> > +NvmeNamespace *ns;
> > +
> > +/* number of outstanding write zeroes for this namespace */
> > +int *count;
> 
> Shouldn't this count be the NvmeRequest's opaque value?

That is already occupied by `num_formats` which tracks formats of
individual namespaces. `count` is for outstanding write zeroes on one
particular namespace.


signature.asc
Description: PGP signature


Re: [PATCH RFC v3 08/12] hw/block/nvme: end-to-end data protection

2021-02-17 Thread Klaus Jensen
On Feb 16 15:08, Keith Busch wrote:
> On Mon, Feb 15, 2021 at 12:02:36AM +0100, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Add support for namespaces formatted with protection information. The
> > type of end-to-end data protection (i.e. Type 1, Type 2 or Type 3) is
> > selected with the `pi` nvme-ns device parameter. If the number of
> > metadata bytes is larger than 8, the `pil` nvme-ns device parameter may
> > be used to control the location of the 8-byte DIF tuple. The default
> > `pil` value of '0', causes the DIF tuple to be transferred as the last
> > 8 bytes of the metadata. Set to 1 to store this in the first eight bytes
> > instead.
> 
> 
> This file is getting quite large. I think this feature can have the bulk
> of the implementation in a separate file. For ex, nvme-dif.c.

Yes, makes sense to split it off. I think moving[1] the device to hw/nvme
first would be good.

> But like the linux implementation this is based on, it isn't really
> nvme specific, so even better if t10 dif is implemented in a generic
> location with an API for nvme and others.

That is true, but in the absence of any interest from other subsystems
to implement this, I think we can keep it local for now? I keep a pretty
close eye on qemu-block, so if other subsystems should care about this,
I promise that I will pitch in :)


  [1]: <20210209110826.585987-1-...@irrelevant.dk>


signature.asc
Description: PGP signature