Re: [RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again

[Alexander Bulekov]

On 210624 1012, Philippe Mathieu-Daudé wrote:
> On 6/24/21 4:50 AM, Alexander Bulekov wrote:
> > On 210623 2000, Philippe Mathieu-Daudé wrote:
> >> Hi Ubi-Wan Kenubi and Tom,
> >>
> >> In commit a9bcedd (SD card size has to be power of 2) we decided
> >> to restrict SD card size to avoid security problems (CVE-2020-13253)
> >> but this became not practical to some users.
> >>
> >> This RFC series tries to remove the limitation, keeping our
> >> functional tests working. It is unfinished work because I had to
> >> attend other topics, but sending it early as RFC to get feedback.
> >> I'll keep working when I get more time, except if one if you can
> >> help me.
> >>
> >> Alexander, could you generate a qtest reproducer with the fuzzer
> >> corpus? See: https://bugs.launchpad.net/qemu/+bug/1878054
> > 
> > I think that bug was already fixed - the reproducer no logner causes a
> > timeout on 6.0. Did I misunderstand something?
> 
> That bug was fixed but now I'm changing the code and would like to feel
> sure I'm not re-introducing the problem, so having the reproducer in the
> tree would help.

Ok - I'll try to come up with one. Minimizing reproducers for timeouts
is trickier than crashes :-)

> 
> > I applied this series and ran the OSS-Fuzz corpus for the sdhci-v3
> > config. The only problem it found is this assert() (that exists without the
> > patch anyways):
> > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29225 
> 
> Sigh.
> 
> > Let me know if this is something you think I should report on gitlab..
> 
> Yes please :(

Ok here it is: https://gitlab.com/qemu-project/qemu/-/issues/450

The fuzzer I left running for 24h also found another bug (looks like DMA
reentrancy), which I thought was introduced by this patchset, since
OSS-Fuzz had not reported it in months of fuzzing. However, as a
sanity-check I tried it against qemu.git and it crashed - strange...
Anyways here it is: https://gitlab.com/qemu-project/qemu/-/issues/451

-Alex

> 
> > I'll leave the fuzzer running for another 24h or so, but otherwise I'm
> > happy to leave a Tested-by, once there is a V1 series 
> > -Alex
> 
> Thanks!
> 
> Phil.

Re: [RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler

[Bin Meng]

On Sat, Jun 26, 2021 at 1:17 AM Philippe Mathieu-Daudé  wrote:
>
> On 6/25/21 3:49 PM, Bin Meng wrote:
> > On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé  
> > wrote:
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé 
> >> ---
> >>  hw/sd/sd.c | 21 -
> >>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> >>  qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", 
> >> req.cmd);
> >> @@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
> >>  [26]= sd_cmd_illegal,
> >>  [52 ... 54] = sd_cmd_illegal,
> >>  },
> >> +.cmd = {
> >> +[6] = sd_cmd_unimplemented,
> >> +},
> >>  };
> >
> > Does this compile?
>
> Yes.
>
> > Or is this another GCC extension to C?
>
> I haven't checked when this was introduced, but QEMU uses it since
> quite some time now.
>
> Apparently this is:
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

Yep, I know designated initialization of a C array, but I don't know
gcc does not complain two .cmd here

>
>  "In ISO C99 you can give the elements in any order, specifying
>   the array indices or structure field names they apply to, and
>   GNU C allows this as an extension in C90 mode as well."
>
> > But I think you wanted to say .acmd = ?
>
> Oops!
>
> Thanks for the review,

Regards,
Bin

Re: [PATCH v2 00/11] hw/nvme: reimplement all multi-aio commands with custom aiocbs

[Keith Busch]

On Thu, Jun 17, 2021 at 09:06:46PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> This series reimplements flush, dsm, copy, zone reset and format nvm to
> allow cancellation. I posted an RFC back in March ("hw/block/nvme:
> convert ad-hoc aio tracking to aiocb") and I've applied some feedback
> from Stefan and reimplemented the remaining commands.
> 
> The basic idea is to define custom AIOCBs for these commands. The custom
> AIOCB takes care of issuing all the "nested" AIOs one by one instead of
> blindly sending them off simultaneously without tracking the returned
> aiocbs.


Klaus,

THis series looks good to me.

Reviewed-by: Keith Busch 

[PATCH 10/10] python: Add iotest linters to test suite

[John Snow]

As a convenience, since iotests is an extremely prominent user of the
qemu.qmp and qemu.machine packages and already implements a linting
regime, run those tests as well so that it's very hard to miss
regressions caused by changes to the python library.

Signed-off-by: John Snow 
---
 python/tests/iotests.sh | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100755 python/tests/iotests.sh

diff --git a/python/tests/iotests.sh b/python/tests/iotests.sh
new file mode 100755
index 00..ec2fc58066
--- /dev/null
+++ b/python/tests/iotests.sh
@@ -0,0 +1,2 @@
+#!/bin/sh -e
+PYTHONPATH=../tests/qemu-iotests/ python3 -m linters
-- 
2.31.1

[PATCH 09/10] iotests/linters: Add entry point for Python CI linters

[John Snow]

Add a main() function to linters.py so that the Python CI infrastructure
has something it can run.

Now, linters.py represents an invocation of the linting scripts that
more resembles a "normal" execution of pylint/mypy, like you'd expect to
use if 'qemu' was a bona-fide package you obtained from PyPI.

297, by contrast, now represents the iotests-specific configuration bits
you need to get it to function correctly as a part of iotests, and with
'qemu' as a namespace package that isn't "installed" to the current
environment, but just lives elsewhere in our source tree.

By doing this, we will able to run the same linting configuration from
the Python CI tests without calling iotest logging functions or messing
around with PYTHONPATH / MYPYPATH.

iotest 297 continues to operate in a standalone fashion for now --
presumably, it's convenient for block maintainers and contributors to
run in this manner.

See the following commit for how this is used from the Python packaging side.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/linters.py | 13 +
 1 file changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 6fa7ba2d22..1bbcfd1088 100755
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -115,3 +115,16 @@ def run_linters(
 print(p.stdout)
 
 return ret
+
+
+def main() -> int:
+"""
+Used by the Python CI system as an entry point to run these linters.
+"""
+directory = os.path.dirname(os.path.realpath(__file__))
+files = get_test_files(directory)
+return run_linters(files, directory)
+
+
+if __name__ == '__main__':
+sys.exit(main())
-- 
2.31.1

[PATCH 08/10] iotests/297: split linters.py off from 297

[John Snow]

Split the linter execution itself out from 297, leaving just environment
setup in 297. This is done so that non-iotest code can invoke the
linters without needing to worry about imports of unpackaged iotest
code.

Eventually, it should be possible to replace linters.py with mypy.ini
and pylintrc files that instruct these tools how to run properly in this
directory, but ... not yet, and not in this series.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297| 105 ++
 tests/qemu-iotests/linters.py | 117 ++
 2 files changed, 121 insertions(+), 101 deletions(-)
 create mode 100755 tests/qemu-iotests/linters.py

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 7db1f9ed45..3d29af5b78 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,110 +17,13 @@
 # along with this program.  If not, see .
 
 import os
-import re
 import shutil
-import subprocess
-import sys
-from typing import List, Mapping, Optional
 
 import iotests
+import linters
 
 
-# TODO: Empty this list!
-SKIP_FILES = (
-'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
-'096', '118', '124', '132', '136', '139', '147', '148', '149',
-'151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
-'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
-'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-'299', '302', '303', '304', '307',
-'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
-)
-
-
-def is_python_file(filename: str, directory: str = '.') -> bool:
-filepath = os.path.join(directory, filename)
-
-if not os.path.isfile(filepath):
-return False
-
-if filename.endswith('.py'):
-return True
-
-with open(filepath) as f:
-try:
-first_line = f.readline()
-return re.match('^#!.*python', first_line) is not None
-except UnicodeDecodeError:  # Ignore binary files
-return False
-
-
-def get_test_files(directory: str = '.') -> List[str]:
-return [
-f for f in (set(os.listdir(directory)) - set(SKIP_FILES))
-if is_python_file(f, directory)
-]
-
-
-def run_linters(
-files: List[str],
-directory: str = '.',
-env: Optional[Mapping[str, str]] = None,
-) -> int:
-ret = 0
-
-print('=== pylint ===')
-sys.stdout.flush()
-
-# Todo notes are fine, but fixme's or xxx's should probably just be
-# fixed (in tests, at least)
-p = subprocess.run(
-('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
-cwd=directory,
-env=env,
-check=False,
-universal_newlines=True,
-)
-ret += p.returncode
-
-print('=== mypy ===')
-sys.stdout.flush()
-
-# We have to call mypy separately for each file.  Otherwise, it
-# will interpret all given files as belonging together (i.e., they
-# may not both define the same classes, etc.; most notably, they
-# must not both define the __main__ module).
-for filename in files:
-p = subprocess.run(
-(
-'python3', '-m', 'mypy',
-'--warn-unused-configs',
-'--disallow-subclassing-any',
-'--disallow-any-generics',
-'--disallow-incomplete-defs',
-'--disallow-untyped-decorators',
-'--no-implicit-optional',
-'--warn-redundant-casts',
-'--warn-unused-ignores',
-'--no-implicit-reexport',
-'--namespace-packages',
-filename,
-),
-cwd=directory,
-env=env,
-check=False,
-stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT,
-universal_newlines=True
-)
-
-ret += p.returncode
-if p.returncode != 0:
-print(p.stdout)
-
-return ret
+# Looking for the list of excluded tests? See linters.py !
 
 
 def main() -> None:
@@ -128,7 +31,7 @@ def main() -> None:
 if shutil.which(linter) is None:
 iotests.notrun(f'{linter} not found')
 
-files = get_test_files()
+files = linters.get_test_files()
 
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
@@ -143,7 +46,7 @@ def main() -> None:
 
 env['MYPYPATH'] = env['PYTHONPATH']
 
-run_linters(files, env=env)
+linters.run_linters(files, env=env)
 
 
 iotests.script_main(main)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100755
index 00..6fa7ba2d22
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,117 @@
+# Copyright (C) 2020 Red Hat, 

[PATCH 06/10] iotests/297: Add 'directory' argument to run_linters

[John Snow]

Allow run_linters to work well if it's executed from a different
directory.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index c7428af901..1e8334d1d4 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -66,6 +66,7 @@ def get_test_files(directory: str = '.') -> List[str]:
 
 def run_linters(
 files: List[str],
+directory: str = '.',
 env: Optional[Mapping[str, str]] = None,
 ) -> None:
 
@@ -76,6 +77,7 @@ def run_linters(
 # fixed (in tests, at least)
 subprocess.run(
 ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+cwd=directory,
 env=env,
 check=False,
 )
@@ -103,6 +105,7 @@ def run_linters(
 '--namespace-packages',
 filename,
 ),
+cwd=directory,
 env=env,
 check=False,
 stdout=subprocess.PIPE,
-- 
2.31.1

[PATCH 04/10] iotests/297: Create main() function

[John Snow]

Instead of running "run_linters" directly, create a main() function that
will be responsible for environment setup, leaving run_linters()
responsible only for execution of the linters.

(That environment setup will be moved over in forthcoming commits.)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 43af361622..f35687d021 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -123,8 +123,12 @@ def run_linters():
 print(p.stdout)
 
 
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
+def main() -> None:
+for linter in ('pylint-3', 'mypy'):
+if shutil.which(linter) is None:
+iotests.notrun(f'{linter} not found')
 
-iotests.script_main(run_linters)
+run_linters()
+
+
+iotests.script_main(main)
-- 
2.31.1

[PATCH 07/10] iotests/297: return error code from run_linters()

[John Snow]

This turns run_linters() into a bit of a hybrid test; returning non-zero
on failed execution while also printing diffable information. This is
done for the benefit of the avocado simple test runner, which will soon
be attempting to execute this test from a different environment.

(Note: universal_newlines is added to the pylint invocation for type
consistency with the mypy run -- it's not strictly necessary, but it
avoids some typing errors caused by our re-use of the 'p' variable.)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 1e8334d1d4..7db1f9ed45 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,19 +68,22 @@ def run_linters(
 files: List[str],
 directory: str = '.',
 env: Optional[Mapping[str, str]] = None,
-) -> None:
+) -> int:
+ret = 0
 
 print('=== pylint ===')
 sys.stdout.flush()
 
 # Todo notes are fine, but fixme's or xxx's should probably just be
 # fixed (in tests, at least)
-subprocess.run(
+p = subprocess.run(
 ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
 cwd=directory,
 env=env,
 check=False,
+universal_newlines=True,
 )
+ret += p.returncode
 
 print('=== mypy ===')
 sys.stdout.flush()
@@ -113,9 +116,12 @@ def run_linters(
 universal_newlines=True
 )
 
+ret += p.returncode
 if p.returncode != 0:
 print(p.stdout)
 
+return ret
+
 
 def main() -> None:
 for linter in ('pylint-3', 'mypy'):
-- 
2.31.1

[PATCH 03/10] iotests/297: Don't rely on distro-specific linter binaries

[John Snow]

'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
mypy" to access these scripts instead. This style of invocation will
prefer the "correct" tool when run in a virtual environment.

Note that we still check for "pylint-3" before the test begins -- this
check is now "overly strict", but shouldn't cause anything that was
already running correctly to start failing.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 45 --
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 0bc1195805..43af361622 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -82,8 +82,11 @@ def run_linters():
 env['PYTHONPATH'] += os.pathsep + qemu_module_path
 except KeyError:
 env['PYTHONPATH'] = qemu_module_path
-subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
-   env=env, check=False)
+subprocess.run(
+('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+env=env,
+check=False,
+)
 
 print('=== mypy ===')
 sys.stdout.flush()
@@ -94,23 +97,27 @@ def run_linters():
 # must not both define the __main__ module).
 env['MYPYPATH'] = env['PYTHONPATH']
 for filename in files:
-p = subprocess.run(('mypy',
-'--warn-unused-configs',
-'--disallow-subclassing-any',
-'--disallow-any-generics',
-'--disallow-incomplete-defs',
-'--disallow-untyped-decorators',
-'--no-implicit-optional',
-'--warn-redundant-casts',
-'--warn-unused-ignores',
-'--no-implicit-reexport',
-'--namespace-packages',
-filename),
-   env=env,
-   check=False,
-   stdout=subprocess.PIPE,
-   stderr=subprocess.STDOUT,
-   universal_newlines=True)
+p = subprocess.run(
+(
+'python3', '-m', 'mypy',
+'--warn-unused-configs',
+'--disallow-subclassing-any',
+'--disallow-any-generics',
+'--disallow-incomplete-defs',
+'--disallow-untyped-decorators',
+'--no-implicit-optional',
+'--warn-redundant-casts',
+'--warn-unused-ignores',
+'--no-implicit-reexport',
+'--namespace-packages',
+filename,
+),
+env=env,
+check=False,
+stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT,
+universal_newlines=True
+)
 
 if p.returncode != 0:
 print(p.stdout)
-- 
2.31.1

[PATCH 02/10] iotests/297: Add get_files() function

[John Snow]

Split out file discovery into its own method to begin separating out the
"environment setup" and "test execution" phases.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 493dda17fb..0bc1195805 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,6 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
+from typing import List
 
 import iotests
 
@@ -56,9 +57,15 @@ def is_python_file(filename: str, directory: str = '.') -> 
bool:
 return False
 
 
+def get_test_files(directory: str = '.') -> List[str]:
+return [
+f for f in (set(os.listdir(directory)) - set(SKIP_FILES))
+if is_python_file(f, directory)
+]
+
+
 def run_linters():
-files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
- if is_python_file(filename)]
+files = get_test_files()
 
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
-- 
2.31.1

[PATCH 05/10] iotests/297: Separate environment setup from test execution

[John Snow]

Move environment setup into main(), leaving pure test execution behind
in run_linters().

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index f35687d021..c7428af901 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,7 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
-from typing import List
+from typing import List, Mapping, Optional
 
 import iotests
 
@@ -64,24 +64,16 @@ def get_test_files(directory: str = '.') -> List[str]:
 ]
 
 
-def run_linters():
-files = get_test_files()
-
-iotests.logger.debug('Files to be checked:')
-iotests.logger.debug(', '.join(sorted(files)))
+def run_linters(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
 
 print('=== pylint ===')
 sys.stdout.flush()
 
 # Todo notes are fine, but fixme's or xxx's should probably just be
 # fixed (in tests, at least)
-env = os.environ.copy()
-qemu_module_path = os.path.join(os.path.dirname(__file__),
-'..', '..', 'python')
-try:
-env['PYTHONPATH'] += os.pathsep + qemu_module_path
-except KeyError:
-env['PYTHONPATH'] = qemu_module_path
 subprocess.run(
 ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
 env=env,
@@ -95,7 +87,6 @@ def run_linters():
 # will interpret all given files as belonging together (i.e., they
 # may not both define the same classes, etc.; most notably, they
 # must not both define the __main__ module).
-env['MYPYPATH'] = env['PYTHONPATH']
 for filename in files:
 p = subprocess.run(
 (
@@ -128,7 +119,22 @@ def main() -> None:
 if shutil.which(linter) is None:
 iotests.notrun(f'{linter} not found')
 
-run_linters()
+files = get_test_files()
+
+iotests.logger.debug('Files to be checked:')
+iotests.logger.debug(', '.join(sorted(files)))
+
+env = os.environ.copy()
+qemu_module_path = os.path.join(os.path.dirname(__file__),
+'..', '..', 'python')
+try:
+env['PYTHONPATH'] += os.pathsep + qemu_module_path
+except KeyError:
+env['PYTHONPATH'] = qemu_module_path
+
+env['MYPYPATH'] = env['PYTHONPATH']
+
+run_linters(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH 01/10] iotests/297: modify is_python_file to work from any CWD

[John Snow]

Add a directory argument to is_python_file to allow it to work correctly
no matter what CWD we happen to run it from. This is done in
anticipation of running the iotests from another directory (./python/).

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 433b732336..493dda17fb 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -39,14 +39,16 @@ SKIP_FILES = (
 )
 
 
-def is_python_file(filename):
-if not os.path.isfile(filename):
+def is_python_file(filename: str, directory: str = '.') -> bool:
+filepath = os.path.join(directory, filename)
+
+if not os.path.isfile(filepath):
 return False
 
 if filename.endswith('.py'):
 return True
 
-with open(filename) as f:
+with open(filepath) as f:
 try:
 first_line = f.readline()
 return re.match('^#!.*python', first_line) is not None
-- 
2.31.1

[PATCH 00/10] python/iotests: Run iotest linters during Python CI

[John Snow]

Based-on: <20210625154540.783306-1-js...@redhat.com>
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest
CI: https://gitlab.com/jsnow/qemu/-/pipelines/327413868

Since iotests are such a heavy and prominent user of the Python qemu.qmp
and qemu.machine packages, it would be convenient if the Python linting
suite also checked this client for any possible regressions introduced
by shifting around signatures, types, or interfaces in these packages.

(We'd eventually find those problems when iotest 297 ran, but with
increasing distance between Python development and Block development,
the risk of an accidental breakage in this regard increases. I,
personally, know to run iotests (and especially 297) after changing
Python code, but not everyone in the future might. Plus, I am lazy, and
I like only having to push one button.)

Add the ability for the Python CI to run the iotest linters too, which
means that the iotest linters would be checked against:

- Python 3.6, using a frozen set of linting packages at their oldest
  supported versions, using 'pipenv'
- Python 3.6 through Python 3.10 inclusive, using 'tox' and the latest
  versions of mypy/pylint that happen to be installed during test
  time. This CI test is allowed to fail with a warning, and can serve
  as a bellwether for when new incompatible changes may disrupt the
  linters. Testing against old and new Python interpreters alike can
  help surface incompatibility issues we may need to be aware of.)

Here are example outputs of those CI jobs with this series applied:
 - "check-python-pipenv": https://gitlab.com/jsnow/qemu/-/jobs/1377735087
 - "check-python-tox": https://gitlab.com/jsnow/qemu/-/jobs/1377735088

You can also run these same tests locally from ./python, plus one more:

- "make check-venv" to test against whatever python you have.
- "make check-pipenv", if you have Python 3.6 and pipenv installed.
- "make check-tox", if you have Python 3.6 through Python 3.10 installed.

Here are example outputs from each of the three different local
execution methods, in order as outlined above:

(1)

jsnow@scv ~/s/q/python (python-package-iotest)> make check-venv
VENV ~/.cache/qemu-pyvenv
ACTIVATE ~/.cache/qemu-pyvenv
INSTALL qemu[devel] ~/.cache/qemu-pyvenv
ACTIVATE ~/.cache/qemu-pyvenv
make[1]: Entering directory '/home/jsnow/src/qemu/python'
JOB ID : 645eacd5ed7f4d5a76ea5f494984a55403a11081
JOB LOG: 
/home/jsnow/avocado/job-results/job-2021-06-25T12.01-645eacd/job.log
 (1/5) tests/flake8.sh: PASS (0.28 s)
 (2/5) tests/iotests.sh: PASS (9.61 s)
 (3/5) tests/isort.sh: PASS (0.09 s)
 (4/5) tests/mypy.sh: PASS (0.25 s)
 (5/5) tests/pylint.sh: PASS (4.13 s)
RESULTS: PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 14.73 s
make[1]: Leaving directory '/home/jsnow/src/qemu/python'

(2)

jsnow@scv ~/s/q/python (python-package-iotest)> make check-pipenv
Creating a virtualenv for this project...
Pipfile: /home/jsnow/src/qemu/python/Pipfile
Using /usr/bin/python3.6m (3.6.13) to create virtualenv...
⠹ Creating virtual environment...created virtual environment 
CPython3.6.13.final.0-64 in 104ms
  creator CPython3Posix(dest=/home/jsnow/src/qemu/python/.venv, clear=False, 
no_vcs_ignore=False, global=False)
  seeder FromAppData(download=False, pip=bundle, setuptools=bundle, 
wheel=bundle, via=copy, app_data_dir=/home/jsnow/.local/share/virtualenv)
added seed packages: pip==21.1.2, setuptools==57.0.0, wheel==0.36.2
  activators 
BashActivator,CShellActivator,FishActivator,PowerShellActivator,PythonActivator,XonshActivator

✔ Successfully created virtual environment!
Virtualenv location: /home/jsnow/src/qemu/python/.venv
Installing dependencies from Pipfile.lock (c13e91)...
      30/30 — 00:00:08
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
All dependencies are now up-to-date!
rm -f pyproject.toml
make[1]: Entering directory '/home/jsnow/src/qemu/python'
JOB ID : a56f53f3c5ef0058d341917228362db8ff24075f
JOB LOG: 
/home/jsnow/avocado/job-results/job-2021-06-25T11.58-a56f53f/job.log
 (1/5) tests/flake8.sh: PASS (0.42 s)
 (2/5) tests/iotests.sh: PASS (8.89 s)
 (3/5) tests/isort.sh: PASS (0.24 s)
 (4/5) tests/mypy.sh: PASS (0.30 s)
 (5/5) tests/pylint.sh: PASS (3.96 s)
RESULTS: PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 14.16 s
make[1]: Leaving directory '/home/jsnow/src/qemu/python'

(3)

jsnow@scv ~/s/q/python (python-package-iotest)> make check-tox
GLOB sdist-make: /home/jsnow/src/qemu/python/setup.py
py36 create: /home/jsnow/src/qemu/python/.tox/py36
py36 installdeps: .[devel], .[fuse]
py36 inst: /home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip
py36 installed: 

Re: [RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler

[Philippe Mathieu-Daudé]

On 6/25/21 3:49 PM, Bin Meng wrote:
> On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé  
> wrote:
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/sd/sd.c | 21 -
>>  1 file changed, 12 insertions(+), 9 deletions(-)

>>  qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", 
>> req.cmd);
>> @@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
>>  [26]= sd_cmd_illegal,
>>  [52 ... 54] = sd_cmd_illegal,
>>  },
>> +.cmd = {
>> +[6] = sd_cmd_unimplemented,
>> +},
>>  };
> 
> Does this compile?

Yes.

> Or is this another GCC extension to C?

I haven't checked when this was introduced, but QEMU uses it since
quite some time now.

Apparently this is:
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

 "In ISO C99 you can give the elements in any order, specifying
  the array indices or structure field names they apply to, and
  GNU C allows this as an extension in C90 mode as well."

> But I think you wanted to say .acmd = ?

Oops!

Thanks for the review,

Phil.

Re: [PULL 0/1] Block patch

[Peter Maydell]

On Thu, 24 Jun 2021 at 12:43, Max Reitz  wrote:
>
> The following changes since commit b22726abdfa54592d6ad88f65b0297c0e8b363e2:
>
>   Merge remote-tracking branch 
> 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging 
> (2021-06-22 16:07:53 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2021-06-24
>
> for you to fetch changes up to 32a9a245d719a883eef2cbf07d2cf89efa0206d0:
>
>   block/snapshot: Clarify goto fallback behavior (2021-06-24 09:49:04 +0200)
>
> 
> Block patch:
> - Fix Coverity complaint in block/snapshot.c


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1
for any user-visible changes.

-- PMM

[PATCH v2 6/6] iotests/fuse-allow-other: Test allow-other

[Max Reitz]

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/tests/fuse-allow-other | 175 ++
 tests/qemu-iotests/tests/fuse-allow-other.out |  88 +
 2 files changed, 263 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/fuse-allow-other
 create mode 100644 tests/qemu-iotests/tests/fuse-allow-other.out

diff --git a/tests/qemu-iotests/tests/fuse-allow-other 
b/tests/qemu-iotests/tests/fuse-allow-other
new file mode 100755
index 00..a513dbce66
--- /dev/null
+++ b/tests/qemu-iotests/tests/fuse-allow-other
@@ -0,0 +1,175 @@
+#!/usr/bin/env bash
+# group: rw
+#
+# Test FUSE exports' allow-other option
+#
+# Copyright (C) 2021 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+seq=$(basename "$0")
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_qemu
+_cleanup_test_img
+rm -f "$EXT_MP"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ../common.rc
+. ../common.filter
+. ../common.qemu
+
+_supported_fmt generic
+
+_supported_proto file # We create the FUSE export manually
+
+sudo -n -u nobody true || \
+_notrun 'Password-less sudo as nobody required to test allow_other'
+
+# $1: Export ID
+# $2: Options (beyond the node-name and ID)
+# $3: Expected return value (defaults to 'return')
+# $4: Node to export (defaults to 'node-format')
+fuse_export_add()
+{
+allow_other_not_supported='option allow_other only allowed if'
+
+output=$(
+success_or_failure=yes _send_qemu_cmd $QEMU_HANDLE \
+"{'execute': 'block-export-add',
+  'arguments': {
+  'type': 'fuse',
+  'id': '$1',
+  'node-name': '${4:-node-format}',
+  $2
+  } }" \
+"${3:-return}" \
+"$allow_other_not_supported" \
+| _filter_imgfmt
+)
+
+if echo "$output" | grep -q "$allow_other_not_supported"; then
+# Shut down qemu gracefully so it can unmount the export
+_send_qemu_cmd $QEMU_HANDLE \
+"{'execute': 'quit'}" \
+'return'
+
+wait=yes _cleanup_qemu
+
+_notrun "allow_other not supported"
+fi
+
+echo "$output"
+}
+
+EXT_MP="$TEST_DIR/fuse-export"
+
+_make_test_img 64k
+touch "$EXT_MP"
+
+echo
+echo '=== Test permissions ==='
+
+# Test that you can only change permissions on the export with 
allow-other=true.
+# We cannot really test the primary reason behind allow-other (i.e. to allow
+# users other than the current one access to the export), because for that we
+# would need sudo, which realistically nobody will allow this test to use.
+# What we can do is test that allow-other=true also enables 
default_permissions,
+# i.e. whether we can still read from the file if we remove the read 
permission.
+
+# $1: allow-other value ('true' or 'false')
+run_permission_test()
+{
+_launch_qemu \
+-blockdev \
+
"$IMGFMT,node-name=node-format,file.driver=file,file.filename=$TEST_IMG"
+
+_send_qemu_cmd $QEMU_HANDLE \
+"{'execute': 'qmp_capabilities'}" \
+'return'
+
+fuse_export_add 'export' \
+"'mountpoint': '$EXT_MP',
+ 'allow-other': '$1'"
+
+# Should always work
+echo '(Removing all permissions)'
+chmod 000 "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+stat -c 'Permissions post-chmod: %a' "$EXT_MP"
+
+# Should always work
+echo '(Granting u+r)'
+chmod u+r "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+stat -c 'Permissions post-chmod: %a' "$EXT_MP"
+
+# Should only work with allow-other: Otherwise, no permissions can be
+# granted to the group or others
+echo '(Granting read permissions for everyone)'
+chmod 444 "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+stat -c 'Permissions post-chmod: %a' "$EXT_MP"
+
+echo 'Doing operations as nobody:'
+# Change to TEST_DIR, so nobody will not have to attempt a lookup
+pushd "$TEST_DIR" >/dev/null
+
+# This is already prevented by the permissions (without allow-other, FUSE
+# exports always have o-r), but test it anyway
+sudo -n -u nobody cat fuse-export >/dev/null
+
+# If the only problem were the lack of permissions, we should still be able
+# to stat the export as nobody; it should not work without 

[PATCH v2 5/6] iotests/308: Test +w on read-only FUSE exports

[Max Reitz]

Test that +w on read-only FUSE exports returns an EROFS error.  u+x on
the other hand should work.  (There is no special reason to choose u+x
here, it simply is like +w another flag that is not set by default.)

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/308 | 11 +++
 tests/qemu-iotests/308.out |  4 
 2 files changed, 15 insertions(+)

diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
index d13a9a969c..6b386bd523 100755
--- a/tests/qemu-iotests/308
+++ b/tests/qemu-iotests/308
@@ -170,6 +170,17 @@ fuse_export_add 'export-mp' "'mountpoint': '$EXT_MP'"
 # Check that the export presents the same data as the original image
 $QEMU_IMG compare -f raw -F $IMGFMT -U "$EXT_MP" "$TEST_IMG"
 
+# Some quick chmod tests
+stat -c 'Permissions pre-chmod: %a' "$EXT_MP"
+
+# Verify that we cannot set +w
+chmod u+w "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+stat -c 'Permissions post-+w: %a' "$EXT_MP"
+
+# But that we can set, say, +x (if we are so inclined)
+chmod u+x "$EXT_MP" 2>&1 | _filter_testdir | _filter_imgfmt
+stat -c 'Permissions post-+x: %a' "$EXT_MP"
+
 echo
 echo '=== Mount over existing file ==='
 
diff --git a/tests/qemu-iotests/308.out b/tests/qemu-iotests/308.out
index 0e9420645f..fc47bb11a2 100644
--- a/tests/qemu-iotests/308.out
+++ b/tests/qemu-iotests/308.out
@@ -50,6 +50,10 @@ wrote 67108864/67108864 bytes at offset 0
   } }
 {"return": {}}
 Images are identical.
+Permissions pre-chmod: 400
+chmod: changing permissions of 'TEST_DIR/t.IMGFMT.fuse': Read-only file system
+Permissions post-+w: 400
+Permissions post-+x: 500
 
 === Mount over existing file ===
 {'execute': 'block-export-add',
-- 
2.31.1

[PATCH v2 1/6] export/fuse: Pass default_permissions for mount

[Max Reitz]

We do not do any permission checks in fuse_open(), so let the kernel do
them.  We already let fuse_getattr() report the proper UNIX permissions,
so this should work the way we want.

This causes a change in 308's reference output, because now opening a
non-writable export with O_RDWR fails already, instead of only actually
attempting to write to it.  (That is an improvement.)

Signed-off-by: Max Reitz 
---
 block/export/fuse.c| 8 ++--
 tests/qemu-iotests/308 | 3 ++-
 tests/qemu-iotests/308.out | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 38f74c94da..d0b88e8f80 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -153,8 +153,12 @@ static int setup_fuse_export(FuseExport *exp, const char 
*mountpoint,
 struct fuse_args fuse_args;
 int ret;
 
-/* Needs to match what fuse_init() sets.  Only max_read must be supplied. 
*/
-mount_opts = g_strdup_printf("max_read=%zu", FUSE_MAX_BOUNCE_BYTES);
+/*
+ * max_read needs to match what fuse_init() sets.
+ * max_write need not be supplied.
+ */
+mount_opts = g_strdup_printf("max_read=%zu,default_permissions",
+ FUSE_MAX_BOUNCE_BYTES);
 
 fuse_argv[0] = ""; /* Dummy program name */
 fuse_argv[1] = "-o";
diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
index f122065d0f..11c28a75f2 100755
--- a/tests/qemu-iotests/308
+++ b/tests/qemu-iotests/308
@@ -215,7 +215,8 @@ echo '=== Writable export ==='
 fuse_export_add 'export-mp' "'mountpoint': '$EXT_MP', 'writable': true"
 
 # Check that writing to the read-only export fails
-$QEMU_IO -f raw -c 'write -P 42 1M 64k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -f raw -c 'write -P 42 1M 64k' "$TEST_IMG" 2>&1 \
+| _filter_qemu_io | _filter_testdir | _filter_imgfmt
 
 # But here it should work
 $QEMU_IO -f raw -c 'write -P 42 1M 64k' "$EXT_MP" | _filter_qemu_io
diff --git a/tests/qemu-iotests/308.out b/tests/qemu-iotests/308.out
index 466e7e0267..0e9420645f 100644
--- a/tests/qemu-iotests/308.out
+++ b/tests/qemu-iotests/308.out
@@ -91,7 +91,7 @@ virtual size: 0 B (0 bytes)
   'mountpoint': 'TEST_DIR/t.IMGFMT.fuse', 'writable': true
   } }
 {"return": {}}
-write failed: Permission denied
+qemu-io: can't open device TEST_DIR/t.IMGFMT: Could not open 
'TEST_DIR/t.IMGFMT': Permission denied
 wrote 65536/65536 bytes at offset 1048576
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 1048576
-- 
2.31.1

[PATCH v2 3/6] export/fuse: Give SET_ATTR_SIZE its own branch

[Max Reitz]

In order to support changing other attributes than the file size in
fuse_setattr(), we have to give each its own independent branch.  This
also applies to the only attribute we do support right now.

Signed-off-by: Max Reitz 
Reviewed-by: Kevin Wolf 
---
 block/export/fuse.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 4068250241..26ad644cd7 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -417,20 +417,22 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t 
inode, struct stat *statbuf,
 FuseExport *exp = fuse_req_userdata(req);
 int ret;
 
-if (!exp->writable) {
-fuse_reply_err(req, EACCES);
-return;
-}
-
 if (to_set & ~FUSE_SET_ATTR_SIZE) {
 fuse_reply_err(req, ENOTSUP);
 return;
 }
 
-ret = fuse_do_truncate(exp, statbuf->st_size, true, PREALLOC_MODE_OFF);
-if (ret < 0) {
-fuse_reply_err(req, -ret);
-return;
+if (to_set & FUSE_SET_ATTR_SIZE) {
+if (!exp->writable) {
+fuse_reply_err(req, EACCES);
+return;
+}
+
+ret = fuse_do_truncate(exp, statbuf->st_size, true, PREALLOC_MODE_OFF);
+if (ret < 0) {
+fuse_reply_err(req, -ret);
+return;
+}
 }
 
 fuse_getattr(req, inode, fi);
-- 
2.31.1

[PATCH v2 2/6] export/fuse: Add allow-other option

[Max Reitz]

Without the allow_other mount option, no user (not even root) but the
one who started qemu/the storage daemon can access the export.  Allow
users to configure the export such that such accesses are possible.

While allow_other is probably what users want, we cannot make it an
unconditional default, because passing it is only possible (for non-root
users) if the global fuse.conf configuration file allows it.  Thus, the
default is an 'auto' mode, in which we first try with allow_other, and
then fall back to without.

FuseExport.allow_other reports whether allow_other was actually used as
a mount option or not.  Currently, this information is not used, but a
future patch will let this field decide whether e.g. an export's UID and
GID can be changed through chmod.

One notable thing about 'auto' mode is that libfuse may print error
messages directly to stderr, and so may fusermount (which it executes).
Our export code cannot really filter or hide them.  Therefore, if 'auto'
fails its first attempt and has to fall back, fusermount will print an
error message that mounting with allow_other failed.

This behavior necessitates a change to iotest 308, namely we need to
filter out this error message (because if the first attempt at mounting
with allow_other succeeds, there will be no such message).

Furthermore, common.rc's _make_test_img should use allow-other=off for
FUSE exports, because iotests generally do not need to access images
from other users, so allow-other=on or allow-other=auto have no
advantage.  OTOH, allow-other=on will not work on systems where
user_allow_other is disabled, and with allow-other=auto, we get said
error message that we would need to filter out again.  Just disabling
allow-other is simplest.

Signed-off-by: Max Reitz 
---
 qapi/block-export.json   | 33 -
 block/export/fuse.c  | 28 +++-
 tests/qemu-iotests/308   |  6 +-
 tests/qemu-iotests/common.rc |  6 +-
 4 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index e819e70cac..0ed63442a8 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -120,6 +120,23 @@
'*logical-block-size': 'size',
 '*num-queues': 'uint16'} }
 
+##
+# @FuseExportAllowOther:
+#
+# Possible allow_other modes for FUSE exports.
+#
+# @off: Do not pass allow_other as a mount option.
+#
+# @on: Pass allow_other as a mount option.
+#
+# @auto: Try mounting with allow_other first, and if that fails, retry
+#without allow_other.
+#
+# Since: 6.1
+##
+{ 'enum': 'FuseExportAllowOther',
+  'data': ['off', 'on', 'auto'] }
+
 ##
 # @BlockExportOptionsFuse:
 #
@@ -132,11 +149,25 @@
 # @growable: Whether writes beyond the EOF should grow the block node
 #accordingly. (default: false)
 #
+# @allow-other: If this is off, only qemu's user is allowed access to
+#   this export.  That cannot be changed even with chmod or
+#   chown.
+#   Enabling this option will allow other users access to
+#   the export with the FUSE mount option "allow_other".
+#   Note that using allow_other as a non-root user requires
+#   user_allow_other to be enabled in the global fuse.conf
+#   configuration file.
+#   In auto mode (the default), the FUSE export driver will
+#   first attempt to mount the export with allow_other, and
+#   if that fails, try again without.
+#   (since 6.1; default: auto)
+#
 # Since: 6.0
 ##
 { 'struct': 'BlockExportOptionsFuse',
   'data': { 'mountpoint': 'str',
-'*growable': 'bool' },
+'*growable': 'bool',
+'*allow-other': 'FuseExportAllowOther' },
   'if': 'defined(CONFIG_FUSE)' }
 
 ##
diff --git a/block/export/fuse.c b/block/export/fuse.c
index d0b88e8f80..4068250241 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -46,6 +46,8 @@ typedef struct FuseExport {
 char *mountpoint;
 bool writable;
 bool growable;
+/* Whether allow_other was used as a mount option or not */
+bool allow_other;
 } FuseExport;
 
 static GHashTable *exports;
@@ -57,7 +59,7 @@ static void fuse_export_delete(BlockExport *exp);
 static void init_exports_table(void);
 
 static int setup_fuse_export(FuseExport *exp, const char *mountpoint,
- Error **errp);
+ bool allow_other, Error **errp);
 static void read_from_fuse_export(void *opaque);
 
 static bool is_regular_file(const char *path, Error **errp);
@@ -118,7 +120,22 @@ static int fuse_export_create(BlockExport *blk_exp,
 exp->writable = blk_exp_args->writable;
 exp->growable = args->growable;
 
-ret = setup_fuse_export(exp, args->mountpoint, errp);
+/* set default */
+if (!args->has_allow_other) {
+args->allow_other = FUSE_EXPORT_ALLOW_OTHER_AUTO;
+}
+
+if 

[PATCH v2 0/6] export/fuse: Allow other users access to the export

[Max Reitz]

Hi,

The v1 cover letter is here:
https://lists.nongnu.org/archive/html/qemu-block/2021-06/msg00730.html

In v2, I changed the following:
- default_permissions is now passed always.  This is the right thing to
  do regardless of whether allow_other is active or not.

- allow_other is no longer a bool, but an off/on/auto enum.  `auto` is
  the default, in which case we will try to mount the export with
  allow_other first, and then fall back to mounting it without.

- Changing the file mode is now possible even without allow_other
  (because default_permissions is always active now), but only for the
  user/owner.  Giving the group or others any permissions only makes
  sense with allow_other, the same applies to changing the UID or GID.
  Giving a read-only export +w makes no sense and hence yields an EROFS
  error now.

- I decided just testing some default_permission quirks is boring.  So
  the new fuse-allow-other iotest does rely on `sudo -n -u nobody`
  working now, and actually tests what allow_other is supposed to do.
  (Also, it is skipped if allow_other does not work.)


git-backport-diff against v1:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[down] 'export/fuse: Pass default_permissions for mount'
002/6:[0089] [FC] 'export/fuse: Add allow-other option'
003/6:[] [--] 'export/fuse: Give SET_ATTR_SIZE its own branch'
004/6:[0039] [FC] 'export/fuse: Let permissions be adjustable'
005/6:[down] 'iotests/308: Test +w on read-only FUSE exports'
006/6:[down] 'iotests/fuse-allow-other: Test allow-other'


Max Reitz (6):
  export/fuse: Pass default_permissions for mount
  export/fuse: Add allow-other option
  export/fuse: Give SET_ATTR_SIZE its own branch
  export/fuse: Let permissions be adjustable
  iotests/308: Test +w on read-only FUSE exports
  iotests/fuse-allow-other: Test allow-other

 qapi/block-export.json|  33 +++-
 block/export/fuse.c   | 121 +---
 tests/qemu-iotests/308|  20 +-
 tests/qemu-iotests/308.out|   6 +-
 tests/qemu-iotests/common.rc  |   6 +-
 tests/qemu-iotests/tests/fuse-allow-other | 175 ++
 tests/qemu-iotests/tests/fuse-allow-other.out |  88 +
 7 files changed, 421 insertions(+), 28 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/fuse-allow-other
 create mode 100644 tests/qemu-iotests/tests/fuse-allow-other.out

-- 
2.31.1

[PATCH v2 4/6] export/fuse: Let permissions be adjustable

[Max Reitz]

Allow changing the file mode, UID, and GID through SETATTR.

Without allow_other, UID and GID are not allowed to be changed, because
it would not make sense.  Also, changing group or others' permissions
is not allowed either.

For read-only exports, +w cannot be set.

Signed-off-by: Max Reitz 
---
 block/export/fuse.c | 73 ++---
 1 file changed, 62 insertions(+), 11 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 26ad644cd7..ada9e263eb 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -48,6 +48,10 @@ typedef struct FuseExport {
 bool growable;
 /* Whether allow_other was used as a mount option or not */
 bool allow_other;
+
+mode_t st_mode;
+uid_t st_uid;
+gid_t st_gid;
 } FuseExport;
 
 static GHashTable *exports;
@@ -125,6 +129,13 @@ static int fuse_export_create(BlockExport *blk_exp,
 args->allow_other = FUSE_EXPORT_ALLOW_OTHER_AUTO;
 }
 
+exp->st_mode = S_IFREG | S_IRUSR;
+if (exp->writable) {
+exp->st_mode |= S_IWUSR;
+}
+exp->st_uid = getuid();
+exp->st_gid = getgid();
+
 if (args->allow_other == FUSE_EXPORT_ALLOW_OTHER_AUTO) {
 /* Ignore errors on our first attempt */
 ret = setup_fuse_export(exp, args->mountpoint, true, NULL);
@@ -338,7 +349,6 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
 int64_t length, allocated_blocks;
 time_t now = time(NULL);
 FuseExport *exp = fuse_req_userdata(req);
-mode_t mode;
 
 length = blk_getlength(exp->common.blk);
 if (length < 0) {
@@ -353,17 +363,12 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
 allocated_blocks = DIV_ROUND_UP(allocated_blocks, 512);
 }
 
-mode = S_IFREG | S_IRUSR;
-if (exp->writable) {
-mode |= S_IWUSR;
-}
-
 statbuf = (struct stat) {
 .st_ino = inode,
-.st_mode= mode,
+.st_mode= exp->st_mode,
 .st_nlink   = 1,
-.st_uid = getuid(),
-.st_gid = getgid(),
+.st_uid = exp->st_uid,
+.st_gid = exp->st_gid,
 .st_size= length,
 .st_blksize = blk_bs(exp->common.blk)->bl.request_alignment,
 .st_blocks  = allocated_blocks,
@@ -409,19 +414,52 @@ static int fuse_do_truncate(const FuseExport *exp, 
int64_t size,
 }
 
 /**
- * Let clients set file attributes.  Only resizing is supported.
+ * Let clients set file attributes.  Only resizing and changing
+ * permissions (st_mode, st_uid, st_gid) is allowed.
+ * Changing permissions is only allowed as far as it will actually
+ * permit access: Read-only exports cannot be given +w, and exports
+ * without allow_other cannot be given a different UID or GID, and
+ * they cannot be given non-owner access.
  */
 static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat 
*statbuf,
  int to_set, struct fuse_file_info *fi)
 {
 FuseExport *exp = fuse_req_userdata(req);
+int supported_attrs;
 int ret;
 
-if (to_set & ~FUSE_SET_ATTR_SIZE) {
+supported_attrs = FUSE_SET_ATTR_SIZE | FUSE_SET_ATTR_MODE;
+if (exp->allow_other) {
+supported_attrs |= FUSE_SET_ATTR_UID | FUSE_SET_ATTR_GID;
+}
+
+if (to_set & ~supported_attrs) {
 fuse_reply_err(req, ENOTSUP);
 return;
 }
 
+/* Do some argument checks first before committing to anything */
+if (to_set & FUSE_SET_ATTR_MODE) {
+/*
+ * Without allow_other, non-owners can never access the export, so do
+ * not allow setting permissions for them
+ */
+if (!exp->allow_other &&
+(statbuf->st_mode & (S_IRWXG | S_IRWXO)) != 0)
+{
+fuse_reply_err(req, EPERM);
+return;
+}
+
+/* +w for read-only exports makes no sense, disallow it */
+if (!exp->writable &&
+(statbuf->st_mode & (S_IWUSR | S_IWGRP | S_IWOTH)) != 0)
+{
+fuse_reply_err(req, EROFS);
+return;
+}
+}
+
 if (to_set & FUSE_SET_ATTR_SIZE) {
 if (!exp->writable) {
 fuse_reply_err(req, EACCES);
@@ -435,6 +473,19 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, 
struct stat *statbuf,
 }
 }
 
+if (to_set & FUSE_SET_ATTR_MODE) {
+/* Ignore FUSE-supplied file type, only change the mode */
+exp->st_mode = (statbuf->st_mode & 0) | S_IFREG;
+}
+
+if (to_set & FUSE_SET_ATTR_UID) {
+exp->st_uid = statbuf->st_uid;
+}
+
+if (to_set & FUSE_SET_ATTR_GID) {
+exp->st_gid = statbuf->st_gid;
+}
+
 fuse_getattr(req, inode, fi);
 }
 
-- 
2.31.1

Re: [RFC PATCH 10/10] hw/sd: Add sd_cmd_SEND_RELATIVE_ADDR() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé  wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 28 +++-
>  1 file changed, 15 insertions(+), 13 deletions(-)
>

Reviewed-by: Bin Meng 

Re: [RFC PATCH 07/10] hw/sd: Add sd_cmd_GO_IDLE_STATE() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:29 PM Philippe Mathieu-Daudé  wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
>

Reviewed-by: Bin Meng 

Re: [RFC PATCH 09/10] hw/sd: Add sd_cmd_ALL_SEND_CID() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:23 PM Philippe Mathieu-Daudé  wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 23 ---
>  1 file changed, 12 insertions(+), 11 deletions(-)
>

Reviewed-by: Bin Meng 

Re: [RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé  wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 21 -
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 0215bdb3689..2647fd26566 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -973,6 +973,15 @@ static sd_rsp_type_t sd_cmd_illegal(SDState *sd, 
> SDRequest req)
>  return sd_illegal;
>  }
>
> +/* Commands that are recognised but not yet implemented. */
> +static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
> +{
> +qemu_log_mask(LOG_UNIMP, "%s: CMD%i not implemented\n",
> +  sd->proto->name, req.cmd);
> +
> +return sd_illegal;
> +}
> +
>  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>  {
>  uint32_t rca = 0x;
> @@ -1522,9 +1531,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>
>  switch (req.cmd) {
>  case 6:/* ACMD6:  SET_BUS_WIDTH */
> -if (sd->spi) {
> -goto unimplemented_spi_cmd;
> -}
>  switch (sd->state) {
>  case sd_transfer_state:
>  sd->sd_status[0] &= 0x3f;
> @@ -1655,12 +1661,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>  default:
>  /* Fall back to standard commands.  */
>  return sd_normal_command(sd, req);
> -
> -unimplemented_spi_cmd:
> -/* Commands that are recognised but not yet implemented in SPI mode. 
>  */
> -qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
> -  req.cmd);
> -return sd_illegal;
>  }
>
>  qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
> @@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
>  [26]= sd_cmd_illegal,
>  [52 ... 54] = sd_cmd_illegal,
>  },
> +.cmd = {
> +[6] = sd_cmd_unimplemented,
> +},
>  };

Does this compile? Or is this another GCC extension to C?

But I think you wanted to say .acmd = ?

Regards,
Bin

Re: [RFC PATCH 05/10] hw/sd: Add sd_cmd_illegal() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:23 PM Philippe Mathieu-Daudé  wrote:
>
> Log illegal commands as GUEST_ERROR.
>
> Note: we are logging back the SDIO commands (CMD5, CMD52-54).
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 57 ++
>  1 file changed, 23 insertions(+), 34 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ce1eec0374f..0215bdb3689 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -965,6 +965,14 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState 
> *sd, SDRequest req)
>  return sd_illegal;
>  }
>
> +static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
> +{
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i\n",
> +  sd->proto->name, req.cmd);
> +
> +return sd_illegal;
> +}
> +
>  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>  {
>  uint32_t rca = 0x;
> @@ -1017,15 +1025,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  break;
>
>  case 1:/* CMD1:   SEND_OP_CMD */
> -if (!sd->spi)
> -goto bad_cmd;
> -
>  sd->state = sd_transfer_state;
>  return sd_r1;
>
>  case 2:/* CMD2:   ALL_SEND_CID */
> -if (sd->spi)
> -goto bad_cmd;
>  switch (sd->state) {
>  case sd_ready_state:
>  sd->state = sd_identification_state;
> @@ -1037,8 +1040,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  break;
>
>  case 3:/* CMD3:   SEND_RELATIVE_ADDR */
> -if (sd->spi)
> -goto bad_cmd;
>  switch (sd->state) {
>  case sd_identification_state:
>  case sd_standby_state:
> @@ -1052,8 +1053,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  break;
>
>  case 4:/* CMD4:   SEND_DSR */
> -if (sd->spi)
> -goto bad_cmd;
>  switch (sd->state) {
>  case sd_standby_state:
>  break;
> @@ -1063,9 +1062,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  }
>  break;
>
> -case 5: /* CMD5: reserved for SDIO cards */
> -return sd_illegal;
> -
>  case 6:/* CMD6:   SWITCH_FUNCTION */
>  switch (sd->mode) {
>  case sd_data_transfer_mode:
> @@ -1081,8 +1077,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  break;
>
>  case 7:/* CMD7:   SELECT/DESELECT_CARD */
> -if (sd->spi)
> -goto bad_cmd;
>  switch (sd->state) {
>  case sd_standby_state:
>  if (sd->rca != rca)
> @@ -1212,8 +1206,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  break;
>
>  case 15:   /* CMD15:  GO_INACTIVE_STATE */
> -if (sd->spi)
> -goto bad_cmd;
>  switch (sd->mode) {
>  case sd_data_transfer_mode:
>  if (sd->rca != rca)
> @@ -1320,8 +1312,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  break;
>
>  case 26:   /* CMD26:  PROGRAM_CID */
> -if (sd->spi)
> -goto bad_cmd;
>  switch (sd->state) {
>  case sd_transfer_state:
>  sd->state = sd_receivingdata_state;
> @@ -1466,15 +1456,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  }
>  break;
>
> -case 52 ... 54:
> -/* CMD52, CMD53, CMD54: reserved for SDIO cards
> - * (see the SDIO Simplified Specification V2.0)
> - * Handle as illegal command but do not complain
> - * on stderr, as some OSes may use these in their
> - * probing for presence of an SDIO card.
> - */
> -return sd_illegal;
> -
>  /* Application specific commands (Class 8) */
>  case 55:   /* CMD55:  APP_CMD */
>  switch (sd->state) {
> @@ -1515,19 +1496,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>  break;
>
>  case 58:/* CMD58:   READ_OCR (SPI) */
> -if (!sd->spi) {
> -goto bad_cmd;
> -}
>  return sd_r3;
>
>  case 59:/* CMD59:   CRC_ON_OFF (SPI) */
> -if (!sd->spi) {
> -goto bad_cmd;
> -}
>  return sd_r1;
>
>  default:
> -bad_cmd:
>  qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
>  return sd_illegal;
>  }
> @@ -2114,10 +2088,25 @@ void sd_enable(SDState *sd, bool enable)
>
>  static const SDProto sd_proto_spi = {
>  .name = "SPI",
> +.cmd = {
> +[2 ... 4]   = sd_cmd_illegal,
> +[5] = sd_cmd_illegal,

Above 2 can be merged into [2 ... 5]

> +[7] = sd_cmd_illegal,
> +[15]= sd_cmd_illegal,
> +[26]= sd_cmd_illegal,
> +[52 ... 54] = sd_cmd_illegal,
> +},
>  };
>
>  static const SDProto 

Re: [RFC PATCH 04/10] hw/sd: Introduce sd_cmd_handler type

[Bin Meng]

On Thu, Jun 24, 2021 at 10:27 PM Philippe Mathieu-Daudé  wrote:
>
> Add 2 command handler arrays in SDProto, for CMD and ACMD.
> Have sd_normal_command() / sd_app_command() use these arrays:
> if an command handler is registered, call it, otherwise fall

a command

> back to current code base.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 13 +
>  1 file changed, 13 insertions(+)
>

Reviewed-by: Bin Meng 

[PULL 02/10] block-copy: let ratelimit handle a speed of 0

[Vladimir Sementsov-Ogievskiy]

From: Paolo Bonzini 

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
Message-Id: <20210614081130.22134-3-eespo...@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/block-copy.c | 28 +++-
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 5808cfe657..020f9846d8 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -114,7 +114,6 @@ typedef struct BlockCopyState {
 
 SharedResource *mem;
 
-uint64_t speed;
 RateLimit rate_limit;
 } BlockCopyState;
 
@@ -647,21 +646,19 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
 task->copy_range = false;
 }
 
-if (s->speed) {
-if (!call_state->ignore_ratelimit) {
-uint64_t ns = ratelimit_calculate_delay(>rate_limit, 0);
-if (ns > 0) {
-block_copy_task_end(task, -EAGAIN);
-g_free(task);
-qemu_co_sleep_ns_wakeable(_state->sleep,
-  QEMU_CLOCK_REALTIME, ns);
-continue;
-}
+if (!call_state->ignore_ratelimit) {
+uint64_t ns = ratelimit_calculate_delay(>rate_limit, 0);
+if (ns > 0) {
+block_copy_task_end(task, -EAGAIN);
+g_free(task);
+qemu_co_sleep_ns_wakeable(_state->sleep,
+  QEMU_CLOCK_REALTIME, ns);
+continue;
 }
-
-ratelimit_calculate_delay(>rate_limit, task->bytes);
 }
 
+ratelimit_calculate_delay(>rate_limit, task->bytes);
+
 trace_block_copy_process(s, task->offset);
 
 co_get_from_shres(s->mem, task->bytes);
@@ -853,10 +850,7 @@ void block_copy_set_skip_unallocated(BlockCopyState *s, 
bool skip)
 
 void block_copy_set_speed(BlockCopyState *s, uint64_t speed)
 {
-s->speed = speed;
-if (speed > 0) {
-ratelimit_set_speed(>rate_limit, speed, BLOCK_COPY_SLICE_TIME);
-}
+ratelimit_set_speed(>rate_limit, speed, BLOCK_COPY_SLICE_TIME);
 
 /*
  * Note: it's good to kick all call states from here, but it should be done
-- 
2.29.2

[PULL 10/10] block-copy: atomic .cancelled and .finished fields in BlockCopyCallState

[Vladimir Sementsov-Ogievskiy]

From: Emanuele Giuseppe Esposito 

By adding acquire/release pairs, we ensure that .ret and .error_is_read
fields are written by block_copy_dirty_clusters before .finished is true,
and that they are read by API user after .finished is true.

The atomic here are necessary because the fields are concurrently modified
in coroutines, and read outside.

Signed-off-by: Emanuele Giuseppe Esposito 
Message-Id: <20210624072043.180494-6-eespo...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block-copy.h |  2 ++
 block/block-copy.c | 37 ++---
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 338f2ea7fd..5c8278895c 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -18,6 +18,8 @@
 #include "block/block.h"
 #include "qemu/co-shared-resource.h"
 
+/* All APIs are thread-safe */
+
 typedef void (*BlockCopyAsyncCallbackFunc)(void *opaque);
 typedef struct BlockCopyState BlockCopyState;
 typedef struct BlockCopyCallState BlockCopyCallState;
diff --git a/block/block-copy.c b/block/block-copy.c
index f3550d0825..0becad52da 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -52,9 +52,9 @@ typedef struct BlockCopyCallState {
 Coroutine *co;
 
 /* Fields whose state changes throughout the execution */
-bool finished;
+bool finished; /* atomic */
 QemuCoSleep sleep; /* TODO: protect API with a lock */
-bool cancelled;
+bool cancelled; /* atomic */
 /* To reference all call states from BlockCopyState */
 QLIST_ENTRY(BlockCopyCallState) list;
 
@@ -667,7 +667,8 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
 assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
 assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
 
-while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) {
+while (bytes && aio_task_pool_status(aio) == 0 &&
+   !qatomic_read(_state->cancelled)) {
 BlockCopyTask *task;
 int64_t status_bytes;
 
@@ -779,7 +780,7 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
 do {
 ret = block_copy_dirty_clusters(call_state);
 
-if (ret == 0 && !call_state->cancelled) {
+if (ret == 0 && !qatomic_read(_state->cancelled)) {
 WITH_QEMU_LOCK_GUARD(>lock) {
 /*
  * Check that there is no task we still need to
@@ -815,9 +816,9 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
  * 2. We have waited for some intersecting block-copy request
  *It may have failed and produced new dirty bits.
  */
-} while (ret > 0 && !call_state->cancelled);
+} while (ret > 0 && !qatomic_read(_state->cancelled));
 
-call_state->finished = true;
+qatomic_store_release(_state->finished, true);
 
 if (call_state->cb) {
 call_state->cb(call_state->cb_opaque);
@@ -880,44 +881,50 @@ void block_copy_call_free(BlockCopyCallState *call_state)
 return;
 }
 
-assert(call_state->finished);
+assert(qatomic_read(_state->finished));
 g_free(call_state);
 }
 
 bool block_copy_call_finished(BlockCopyCallState *call_state)
 {
-return call_state->finished;
+return qatomic_read(_state->finished);
 }
 
 bool block_copy_call_succeeded(BlockCopyCallState *call_state)
 {
-return call_state->finished && !call_state->cancelled &&
-call_state->ret == 0;
+return qatomic_load_acquire(_state->finished) &&
+   !qatomic_read(_state->cancelled) &&
+   call_state->ret == 0;
 }
 
 bool block_copy_call_failed(BlockCopyCallState *call_state)
 {
-return call_state->finished && !call_state->cancelled &&
-call_state->ret < 0;
+return qatomic_load_acquire(_state->finished) &&
+   !qatomic_read(_state->cancelled) &&
+   call_state->ret < 0;
 }
 
 bool block_copy_call_cancelled(BlockCopyCallState *call_state)
 {
-return call_state->cancelled;
+return qatomic_read(_state->cancelled);
 }
 
 int block_copy_call_status(BlockCopyCallState *call_state, bool *error_is_read)
 {
-assert(call_state->finished);
+assert(qatomic_load_acquire(_state->finished));
 if (error_is_read) {
 *error_is_read = call_state->error_is_read;
 }
 return call_state->ret;
 }
 
+/*
+ * Note that cancelling and finishing are racy.
+ * User can cancel a block-copy that is already finished.
+ */
 void block_copy_call_cancel(BlockCopyCallState *call_state)
 {
-call_state->cancelled = true;
+qatomic_set(_state->cancelled, true);
 block_copy_kick(call_state);
 }
 
-- 
2.29.2

[PULL 08/10] block-copy: move progress_set_remaining in block_copy_task_end

[Vladimir Sementsov-Ogievskiy]

From: Emanuele Giuseppe Esposito 

Moving this function in task_end ensures to update the progress
anyways, even if there is an error.

It also helps in next patch, allowing task_end to have only
one critical section.

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Emanuele Giuseppe Esposito 
Message-Id: <20210624072043.180494-4-eespo...@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/block-copy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 0a1cf3d0cb..b7bcb9da86 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -248,6 +248,9 @@ static void coroutine_fn block_copy_task_end(BlockCopyTask 
*task, int ret)
 bdrv_set_dirty_bitmap(task->s->copy_bitmap, task->offset, task->bytes);
 }
 QLIST_REMOVE(task, list);
+progress_set_remaining(task->s->progress,
+   bdrv_get_dirty_count(task->s->copy_bitmap) +
+   task->s->in_flight_bytes);
 qemu_co_queue_restart_all(>wait_queue);
 }
 
@@ -638,9 +641,6 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
 }
 if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
 block_copy_task_end(task, 0);
-progress_set_remaining(s->progress,
-   bdrv_get_dirty_count(s->copy_bitmap) +
-   s->in_flight_bytes);
 trace_block_copy_skip_range(s, task->offset, task->bytes);
 offset = task_end(task);
 bytes = end - offset;
-- 
2.29.2

[PULL 05/10] co-shared-resource: protect with a mutex

[Vladimir Sementsov-Ogievskiy]

From: Emanuele Giuseppe Esposito 

co-shared-resource is currently not thread-safe, as also reported
in co-shared-resource.h. Add a QemuMutex because co_try_get_from_shres
can also be invoked from non-coroutine context.

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Emanuele Giuseppe Esposito 
Message-Id: <20210614081130.22134-6-eespo...@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/qemu/co-shared-resource.h |  4 +---
 util/qemu-co-shared-resource.c| 24 +++-
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/qemu/co-shared-resource.h 
b/include/qemu/co-shared-resource.h
index 4e4503004c..78ca5850f8 100644
--- a/include/qemu/co-shared-resource.h
+++ b/include/qemu/co-shared-resource.h
@@ -26,15 +26,13 @@
 #ifndef QEMU_CO_SHARED_RESOURCE_H
 #define QEMU_CO_SHARED_RESOURCE_H
 
-
+/* Accesses to co-shared-resource API are thread-safe */
 typedef struct SharedResource SharedResource;
 
 /*
  * Create SharedResource structure
  *
  * @total: total amount of some resource to be shared between clients
- *
- * Note: this API is not thread-safe.
  */
 SharedResource *shres_create(uint64_t total);
 
diff --git a/util/qemu-co-shared-resource.c b/util/qemu-co-shared-resource.c
index 1c83cd9d29..a66cc07e75 100644
--- a/util/qemu-co-shared-resource.c
+++ b/util/qemu-co-shared-resource.c
@@ -28,10 +28,13 @@
 #include "qemu/co-shared-resource.h"
 
 struct SharedResource {
-uint64_t total;
-uint64_t available;
+uint64_t total; /* Set in shres_create() and not changed anymore */
 
+/* State fields protected by lock */
+uint64_t available;
 CoQueue queue;
+
+QemuMutex lock;
 };
 
 SharedResource *shres_create(uint64_t total)
@@ -40,6 +43,7 @@ SharedResource *shres_create(uint64_t total)
 
 s->total = s->available = total;
 qemu_co_queue_init(>queue);
+qemu_mutex_init(>lock);
 
 return s;
 }
@@ -47,10 +51,12 @@ SharedResource *shres_create(uint64_t total)
 void shres_destroy(SharedResource *s)
 {
 assert(s->available == s->total);
+qemu_mutex_destroy(>lock);
 g_free(s);
 }
 
-bool co_try_get_from_shres(SharedResource *s, uint64_t n)
+/* Called with lock held. */
+static bool co_try_get_from_shres_locked(SharedResource *s, uint64_t n)
 {
 if (s->available >= n) {
 s->available -= n;
@@ -60,16 +66,24 @@ bool co_try_get_from_shres(SharedResource *s, uint64_t n)
 return false;
 }
 
+bool co_try_get_from_shres(SharedResource *s, uint64_t n)
+{
+QEMU_LOCK_GUARD(>lock);
+return co_try_get_from_shres_locked(s, n);
+}
+
 void coroutine_fn co_get_from_shres(SharedResource *s, uint64_t n)
 {
 assert(n <= s->total);
-while (!co_try_get_from_shres(s, n)) {
-qemu_co_queue_wait(>queue, NULL);
+QEMU_LOCK_GUARD(>lock);
+while (!co_try_get_from_shres_locked(s, n)) {
+qemu_co_queue_wait(>queue, >lock);
 }
 }
 
 void coroutine_fn co_put_to_shres(SharedResource *s, uint64_t n)
 {
+QEMU_LOCK_GUARD(>lock);
 assert(s->total - s->available >= n);
 s->available += n;
 qemu_co_queue_restart_all(>queue);
-- 
2.29.2

[PULL 06/10] block-copy: small refactor in block_copy_task_entry and block_copy_common

[Vladimir Sementsov-Ogievskiy]

From: Emanuele Giuseppe Esposito 

Use a local variable instead of referencing BlockCopyState through a
BlockCopyCallState or BlockCopyTask every time.
This is in preparation for next patches.

No functional change intended.

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Emanuele Giuseppe Esposito 
Message-Id: <20210624072043.180494-2-eespo...@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/block-copy.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 020f9846d8..a437978e35 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -452,14 +452,15 @@ static void 
block_copy_handle_copy_range_result(BlockCopyState *s,
 static coroutine_fn int block_copy_task_entry(AioTask *task)
 {
 BlockCopyTask *t = container_of(task, BlockCopyTask, task);
+BlockCopyState *s = t->s;
 bool error_is_read = false;
 bool copy_range = t->copy_range;
 int ret;
 
-ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,
+ret = block_copy_do_copy(s, t->offset, t->bytes, t->zeroes,
  _range, _is_read);
 if (t->copy_range) {
-block_copy_handle_copy_range_result(t->s, copy_range);
+block_copy_handle_copy_range_result(s, copy_range);
 }
 if (ret < 0) {
 if (!t->call_state->ret) {
@@ -467,9 +468,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
 t->call_state->error_is_read = error_is_read;
 }
 } else {
-progress_work_done(t->s->progress, t->bytes);
+progress_work_done(s->progress, t->bytes);
 }
-co_put_to_shres(t->s->mem, t->bytes);
+co_put_to_shres(s->mem, t->bytes);
 block_copy_task_end(t, ret);
 
 return ret;
@@ -714,14 +715,15 @@ void block_copy_kick(BlockCopyCallState *call_state)
 static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
 {
 int ret;
+BlockCopyState *s = call_state->s;
 
-QLIST_INSERT_HEAD(_state->s->calls, call_state, list);
+QLIST_INSERT_HEAD(>calls, call_state, list);
 
 do {
 ret = block_copy_dirty_clusters(call_state);
 
 if (ret == 0 && !call_state->cancelled) {
-ret = block_copy_wait_one(call_state->s, call_state->offset,
+ret = block_copy_wait_one(s, call_state->offset,
   call_state->bytes);
 }
 
-- 
2.29.2

[PULL 09/10] block-copy: add CoMutex lock

[Vladimir Sementsov-Ogievskiy]

From: Emanuele Giuseppe Esposito 

Group various structures fields, to better understand what we need to
protect with a lock and what doesn't need it.
Then, add a CoMutex to protect concurrent access of block-copy
data structures. This mutex also protects .copy_bitmap, because its thread-safe
API does not prevent it from assigning two tasks to the same
bitmap region.

Exceptions to the lock:
- .sleep_state is handled in the series "coroutine: new sleep/wake API"
and thus here left as TODO.

- .finished, .cancelled and reads to .ret and .error_is_read will be
protected in the following patch, because are used also outside
coroutines.

- .skip_unallocated is atomic. Including it under the mutex would
increase the critical sections and make them also much more complex.
We can have it as atomic since it is only written from outside and
read by block-copy coroutines.

Signed-off-by: Emanuele Giuseppe Esposito 
Message-Id: <20210624072043.180494-5-eespo...@redhat.com>
  [vsementsov: fix typo in comment]
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/block-copy.c | 155 +
 1 file changed, 116 insertions(+), 39 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index b7bcb9da86..f3550d0825 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -39,7 +39,7 @@ typedef enum {
 static coroutine_fn int block_copy_task_entry(AioTask *task);
 
 typedef struct BlockCopyCallState {
-/* IN parameters. Initialized in block_copy_async() and never changed. */
+/* Fields initialized in block_copy_async() and never changed. */
 BlockCopyState *s;
 int64_t offset;
 int64_t bytes;
@@ -48,33 +48,60 @@ typedef struct BlockCopyCallState {
 bool ignore_ratelimit;
 BlockCopyAsyncCallbackFunc cb;
 void *cb_opaque;
-
 /* Coroutine where async block-copy is running */
 Coroutine *co;
 
-/* To reference all call states from BlockCopyState */
-QLIST_ENTRY(BlockCopyCallState) list;
-
-/* State */
-int ret;
+/* Fields whose state changes throughout the execution */
 bool finished;
-QemuCoSleep sleep;
+QemuCoSleep sleep; /* TODO: protect API with a lock */
 bool cancelled;
+/* To reference all call states from BlockCopyState */
+QLIST_ENTRY(BlockCopyCallState) list;
 
-/* OUT parameters */
+/*
+ * Fields that report information about return values and erros.
+ * Protected by lock in BlockCopyState.
+ */
 bool error_is_read;
+/*
+ * @ret is set concurrently by tasks under mutex. Only set once by first
+ * failed task (and untouched if no task failed).
+ * After finishing (call_state->finished is true), it is not modified
+ * anymore and may be safely read without mutex.
+ */
+int ret;
 } BlockCopyCallState;
 
 typedef struct BlockCopyTask {
 AioTask task;
 
+/*
+ * Fields initialized in block_copy_task_create()
+ * and never changed.
+ */
 BlockCopyState *s;
 BlockCopyCallState *call_state;
 int64_t offset;
-int64_t bytes;
+/*
+ * @method can also be set again in the while loop of
+ * block_copy_dirty_clusters(), but it is never accessed concurrently
+ * because the only other function that reads it is
+ * block_copy_task_entry() and it is invoked afterwards in the same
+ * iteration.
+ */
 BlockCopyMethod method;
-QLIST_ENTRY(BlockCopyTask) list;
+
+/*
+ * Fields whose state changes throughout the execution
+ * Protected by lock in BlockCopyState.
+ */
 CoQueue wait_queue; /* coroutines blocked on this task */
+/*
+ * Only protect the case of parallel read while updating @bytes
+ * value in block_copy_task_shrink().
+ */
+int64_t bytes;
+QLIST_ENTRY(BlockCopyTask) list;
 } BlockCopyTask;
 
 static int64_t task_end(BlockCopyTask *task)
@@ -90,17 +117,25 @@ typedef struct BlockCopyState {
  */
 BdrvChild *source;
 BdrvChild *target;
-BdrvDirtyBitmap *copy_bitmap;
-int64_t in_flight_bytes;
+
+/*
+ * Fields initialized in block_copy_state_new()
+ * and never changed.
+ */
 int64_t cluster_size;
-BlockCopyMethod method;
 int64_t max_transfer;
 uint64_t len;
-QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
*/
-QLIST_HEAD(, BlockCopyCallState) calls;
-
 BdrvRequestFlags write_flags;
 
+/*
+ * Fields whose state changes throughout the execution
+ * Protected by lock.
+ */
+CoMutex lock;
+int64_t in_flight_bytes;
+BlockCopyMethod method;
+QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
*/
+QLIST_HEAD(, BlockCopyCallState) calls;
 /*
  * skip_unallocated:
  *
@@ -115,15 +150,15 @@ typedef struct BlockCopyState {
  * skip unallocated regions, clear them in the copy_bitmap, and invoke
  * block_copy_reset_unallocated() 

[PULL 07/10] block-copy: streamline choice of copy_range vs. read/write

[Vladimir Sementsov-Ogievskiy]

From: Paolo Bonzini 

Put the logic to determine the copy size in a separate function, so
that there is a simple state machine for the possible methods of
copying data from one BlockDriverState to the other.

Use .method instead of .copy_range as in-out argument, and
include also .zeroes as an additional copy method.

While at it, store the common computation of block_copy_max_transfer
into a new field of BlockCopyState, and make sure that we always
obey max_transfer; that's more efficient even for the
COPY_RANGE_READ_WRITE case.

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Emanuele Giuseppe Esposito 
Signed-off-by: Paolo Bonzini 
Message-Id: <20210624072043.180494-3-eespo...@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/block-copy.c | 176 +++--
 1 file changed, 90 insertions(+), 86 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index a437978e35..0a1cf3d0cb 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -28,6 +28,14 @@
 #define BLOCK_COPY_MAX_WORKERS 64
 #define BLOCK_COPY_SLICE_TIME 1ULL /* ns */
 
+typedef enum {
+COPY_READ_WRITE_CLUSTER,
+COPY_READ_WRITE,
+COPY_WRITE_ZEROES,
+COPY_RANGE_SMALL,
+COPY_RANGE_FULL
+} BlockCopyMethod;
+
 static coroutine_fn int block_copy_task_entry(AioTask *task);
 
 typedef struct BlockCopyCallState {
@@ -64,8 +72,7 @@ typedef struct BlockCopyTask {
 BlockCopyCallState *call_state;
 int64_t offset;
 int64_t bytes;
-bool zeroes;
-bool copy_range;
+BlockCopyMethod method;
 QLIST_ENTRY(BlockCopyTask) list;
 CoQueue wait_queue; /* coroutines blocked on this task */
 } BlockCopyTask;
@@ -86,8 +93,8 @@ typedef struct BlockCopyState {
 BdrvDirtyBitmap *copy_bitmap;
 int64_t in_flight_bytes;
 int64_t cluster_size;
-bool use_copy_range;
-int64_t copy_size;
+BlockCopyMethod method;
+int64_t max_transfer;
 uint64_t len;
 QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
*/
 QLIST_HEAD(, BlockCopyCallState) calls;
@@ -149,6 +156,24 @@ static bool coroutine_fn 
block_copy_wait_one(BlockCopyState *s, int64_t offset,
 return true;
 }
 
+static int64_t block_copy_chunk_size(BlockCopyState *s)
+{
+switch (s->method) {
+case COPY_READ_WRITE_CLUSTER:
+return s->cluster_size;
+case COPY_READ_WRITE:
+case COPY_RANGE_SMALL:
+return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER),
+   s->max_transfer);
+case COPY_RANGE_FULL:
+return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+   s->max_transfer);
+default:
+/* Cannot have COPY_WRITE_ZEROES here.  */
+abort();
+}
+}
+
 /*
  * Search for the first dirty area in offset/bytes range and create task at
  * the beginning of it.
@@ -158,8 +183,9 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState 
*s,
  int64_t offset, int64_t bytes)
 {
 BlockCopyTask *task;
-int64_t max_chunk = MIN_NON_ZERO(s->copy_size, call_state->max_chunk);
+int64_t max_chunk;
 
+max_chunk = MIN_NON_ZERO(block_copy_chunk_size(s), call_state->max_chunk);
 if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
offset, offset + bytes,
max_chunk, , ))
@@ -183,7 +209,7 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState 
*s,
 .call_state = call_state,
 .offset = offset,
 .bytes = bytes,
-.copy_range = s->use_copy_range,
+.method = s->method,
 };
 qemu_co_queue_init(>wait_queue);
 QLIST_INSERT_HEAD(>tasks, task, list);
@@ -267,28 +293,28 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
BdrvChild *target,
 .len = bdrv_dirty_bitmap_size(copy_bitmap),
 .write_flags = write_flags,
 .mem = shres_create(BLOCK_COPY_MAX_MEM),
+.max_transfer = QEMU_ALIGN_DOWN(
+block_copy_max_transfer(source, target),
+cluster_size),
 };
 
-if (block_copy_max_transfer(source, target) < cluster_size) {
+if (s->max_transfer < cluster_size) {
 /*
  * copy_range does not respect max_transfer. We don't want to bother
  * with requests smaller than block-copy cluster size, so fallback to
  * buffered copying (read and write respect max_transfer on their
  * behalf).
  */
-s->use_copy_range = false;
-s->copy_size = cluster_size;
+s->method = COPY_READ_WRITE_CLUSTER;
 } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
 /* Compression supports only cluster-size writes and no copy-range. */
-s->use_copy_range = false;
-s->copy_size = cluster_size;
+s->method = COPY_READ_WRITE_CLUSTER;
 } else {
 /*
- * We 

[PULL 03/10] blockjob: let ratelimit handle a speed of 0

[Vladimir Sementsov-Ogievskiy]

From: Paolo Bonzini 

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
Message-Id: <20210614081130.22134-4-eespo...@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 blockjob.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index dc1d9e0e46..22e5bb9b1f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -300,10 +300,6 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, 
Error **errp)
 
 int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
 {
-if (!job->speed) {
-return 0;
-}
-
 return ratelimit_calculate_delay(>limit, n);
 }
 
@@ -472,12 +468,9 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 blk_set_disable_request_queuing(blk, true);
 blk_set_allow_aio_context_change(blk, true);
 
-/* Only set speed when necessary to avoid NotSupported error */
-if (speed != 0) {
-if (!block_job_set_speed(job, speed, errp)) {
-job_early_fail(>job);
-return NULL;
-}
+if (!block_job_set_speed(job, speed, errp)) {
+job_early_fail(>job);
+return NULL;
 }
 
 return job;
-- 
2.29.2

[PULL 04/10] progressmeter: protect with a mutex

[Vladimir Sementsov-Ogievskiy]

From: Emanuele Giuseppe Esposito 

Progressmeter is protected by the AioContext mutex, which
is taken by the block jobs and their caller (like blockdev).

We would like to remove the dependency of block layer code on the
AioContext mutex, since most drivers and the core I/O code are already
not relying on it.

Create a new C file to implement the ProgressMeter API, but keep the
struct as public, to avoid forcing allocation on the heap.

Also add a mutex to be able to provide an accurate snapshot of the
progress values to the caller.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
Message-Id: <20210614081130.22134-5-eespo...@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/qemu/progress_meter.h | 34 +++
 block/progress_meter.c| 64 +++
 blockjob.c| 33 +-
 job-qmp.c |  8 +++--
 job.c |  3 ++
 qemu-img.c|  9 +++--
 block/meson.build |  1 +
 7 files changed, 124 insertions(+), 28 deletions(-)
 create mode 100644 block/progress_meter.c

diff --git a/include/qemu/progress_meter.h b/include/qemu/progress_meter.h
index 9a23ff071c..dadf822bbf 100644
--- a/include/qemu/progress_meter.h
+++ b/include/qemu/progress_meter.h
@@ -27,6 +27,8 @@
 #ifndef QEMU_PROGRESS_METER_H
 #define QEMU_PROGRESS_METER_H
 
+#include "qemu/lockable.h"
+
 typedef struct ProgressMeter {
 /**
  * Current progress. The unit is arbitrary as long as the ratio between
@@ -37,22 +39,24 @@ typedef struct ProgressMeter {
 
 /** Estimated current value at the completion of the process */
 uint64_t total;
+
+QemuMutex lock; /* protects concurrent access to above fields */
 } ProgressMeter;
 
-static inline void progress_work_done(ProgressMeter *pm, uint64_t done)
-{
-pm->current += done;
-}
-
-static inline void progress_set_remaining(ProgressMeter *pm, uint64_t 
remaining)
-{
-pm->total = pm->current + remaining;
-}
-
-static inline void progress_increase_remaining(ProgressMeter *pm,
-   uint64_t delta)
-{
-pm->total += delta;
-}
+void progress_init(ProgressMeter *pm);
+void progress_destroy(ProgressMeter *pm);
+
+/* Get a snapshot of internal current and total values  */
+void progress_get_snapshot(ProgressMeter *pm, uint64_t *current,
+   uint64_t *total);
+
+/* Increases the amount of work done so far by @done */
+void progress_work_done(ProgressMeter *pm, uint64_t done);
+
+/* Sets how much work has to be done to complete to @remaining */
+void progress_set_remaining(ProgressMeter *pm, uint64_t remaining);
+
+/* Increases the total work to do by @delta */
+void progress_increase_remaining(ProgressMeter *pm, uint64_t delta);
 
 #endif /* QEMU_PROGRESS_METER_H */
diff --git a/block/progress_meter.c b/block/progress_meter.c
new file mode 100644
index 00..aa2e60248c
--- /dev/null
+++ b/block/progress_meter.c
@@ -0,0 +1,64 @@
+/*
+ * Helper functionality for some process progress tracking.
+ *
+ * Copyright (c) 2011 IBM Corp.
+ * Copyright (c) 2012, 2018 Red Hat, Inc.
+ * Copyright (c) 2020 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "qemu/progress_meter.h"
+
+void progress_init(ProgressMeter *pm)
+{
+qemu_mutex_init(>lock);
+}
+
+void progress_destroy(ProgressMeter *pm)
+{
+qemu_mutex_destroy(>lock);
+}
+
+void progress_get_snapshot(ProgressMeter *pm, uint64_t *current,
+   uint64_t *total)
+{
+QEMU_LOCK_GUARD(>lock);
+
+*current = pm->current;
+*total = pm->total;
+}
+
+void progress_work_done(ProgressMeter *pm, uint64_t done)
+{
+QEMU_LOCK_GUARD(>lock);
+pm->current += done;
+}
+
+void progress_set_remaining(ProgressMeter *pm, uint64_t remaining)
+{
+QEMU_LOCK_GUARD(>lock);
+   

[PULL 00/10] Block Jobs patches

[Vladimir Sementsov-Ogievskiy]

The following changes since commit e0da9171e02f4534124b9a9e0782b38376c6:

  Merge remote-tracking branch 'remotes/kraxel/tags/ui-20210624-pull-request' 
into staging (2021-06-25 09:10:37 +0100)

are available in the Git repository at:

  ssh://g...@src.openvz.org/~vsementsov/qemu.git tags/pull-jobs-2021-06-25

for you to fetch changes up to 149009bef4b4b4db37b3cf72b41dc2c6e8ca1885:

  block-copy: atomic .cancelled and .finished fields in BlockCopyCallState 
(2021-06-25 14:33:51 +0300)


block: Make block-copy API thread-safe



Emanuele Giuseppe Esposito (6):
  progressmeter: protect with a mutex
  co-shared-resource: protect with a mutex
  block-copy: small refactor in block_copy_task_entry and
block_copy_common
  block-copy: move progress_set_remaining in block_copy_task_end
  block-copy: add CoMutex lock
  block-copy: atomic .cancelled and .finished fields in
BlockCopyCallState

Paolo Bonzini (4):
  ratelimit: treat zero speed as unlimited
  block-copy: let ratelimit handle a speed of 0
  blockjob: let ratelimit handle a speed of 0
  block-copy: streamline choice of copy_range vs. read/write

 include/block/block-copy.h|   2 +
 include/qemu/co-shared-resource.h |   4 +-
 include/qemu/progress_meter.h |  34 +--
 include/qemu/ratelimit.h  |  12 +-
 block/block-copy.c| 396 ++
 block/progress_meter.c|  64 +
 blockjob.c|  46 ++--
 job-qmp.c |   8 +-
 job.c |   3 +
 qemu-img.c|   9 +-
 util/qemu-co-shared-resource.c|  24 +-
 block/meson.build |   1 +
 12 files changed, 399 insertions(+), 204 deletions(-)
 create mode 100644 block/progress_meter.c

-- 
2.29.2

[PULL 01/10] ratelimit: treat zero speed as unlimited

[Vladimir Sementsov-Ogievskiy]

From: Paolo Bonzini 

Both users of RateLimit, block-copy.c and blockjob.c, treat
a speed of zero as unlimited, while RateLimit treats it as
"as slow as possible".  The latter is nicer from the code
point of view but pretty useless, so disable rate limiting
if a speed of zero is provided.

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
Message-Id: <20210614081130.22134-2-eespo...@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/qemu/ratelimit.h | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/qemu/ratelimit.h b/include/qemu/ratelimit.h
index 003ea6d5a3..48bf59e857 100644
--- a/include/qemu/ratelimit.h
+++ b/include/qemu/ratelimit.h
@@ -43,7 +43,11 @@ static inline int64_t ratelimit_calculate_delay(RateLimit 
*limit, uint64_t n)
 double delay_slices;
 
 QEMU_LOCK_GUARD(>lock);
-assert(limit->slice_quota && limit->slice_ns);
+if (!limit->slice_quota) {
+/* Throttling disabled.  */
+return 0;
+}
+assert(limit->slice_ns);
 
 if (limit->slice_end_time < now) {
 /* Previous, possibly extended, time slice finished; reset the
@@ -83,7 +87,11 @@ static inline void ratelimit_set_speed(RateLimit *limit, 
uint64_t speed,
 {
 QEMU_LOCK_GUARD(>lock);
 limit->slice_ns = slice_ns;
-limit->slice_quota = MAX(((double)speed * slice_ns) / 10ULL, 1);
+if (speed == 0) {
+limit->slice_quota = 0;
+} else {
+limit->slice_quota = MAX(((double)speed * slice_ns) / 10ULL, 
1);
+}
 }
 
 #endif
-- 
2.29.2

[PULL 5/6] hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c

[John Snow]

From: Philippe Mathieu-Daudé 

Some machines use floppy controllers via the SysBus interface,
and don't need to pull in all the SysBus code.
Extract the SysBus specific code to a new unit: fdc-sysbus.c,
and add a new Kconfig symbol: "FDC_SYSBUS".

Reviewed-by: John Snow 
Acked-by: Mark Cave-Ayland 
Reviewed-by: Mark Cave-Ayland 
Acked-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
Message-id: 20210614193220.2007159-6-phi...@redhat.com
Signed-off-by: John Snow 
---
 hw/block/fdc-sysbus.c | 249 ++
 hw/block/fdc.c| 220 -
 MAINTAINERS   |   1 +
 hw/block/Kconfig  |   4 +
 hw/block/meson.build  |   1 +
 hw/block/trace-events |   2 +
 hw/mips/Kconfig   |   2 +-
 hw/sparc/Kconfig  |   2 +-
 8 files changed, 259 insertions(+), 222 deletions(-)
 create mode 100644 hw/block/fdc-sysbus.c

diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
new file mode 100644
index 00..c6308f5300
--- /dev/null
+++ b/hw/block/fdc-sysbus.c
@@ -0,0 +1,249 @@
+/*
+ * QEMU Floppy disk emulator (Intel 82078)
+ *
+ * Copyright (c) 2003, 2007 Jocelyn Mayer
+ * Copyright (c) 2008 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qom/object.h"
+#include "hw/sysbus.h"
+#include "hw/block/fdc.h"
+#include "migration/vmstate.h"
+#include "fdc-internal.h"
+#include "trace.h"
+
+#define TYPE_SYSBUS_FDC "base-sysbus-fdc"
+typedef struct FDCtrlSysBusClass FDCtrlSysBusClass;
+typedef struct FDCtrlSysBus FDCtrlSysBus;
+DECLARE_OBJ_CHECKERS(FDCtrlSysBus, FDCtrlSysBusClass,
+ SYSBUS_FDC, TYPE_SYSBUS_FDC)
+
+struct FDCtrlSysBusClass {
+/*< private >*/
+SysBusDeviceClass parent_class;
+/*< public >*/
+
+bool use_strict_io;
+};
+
+struct FDCtrlSysBus {
+/*< private >*/
+SysBusDevice parent_obj;
+/*< public >*/
+
+struct FDCtrl state;
+};
+
+static uint64_t fdctrl_read_mem(void *opaque, hwaddr reg, unsigned ize)
+{
+return fdctrl_read(opaque, (uint32_t)reg);
+}
+
+static void fdctrl_write_mem(void *opaque, hwaddr reg,
+ uint64_t value, unsigned size)
+{
+fdctrl_write(opaque, (uint32_t)reg, value);
+}
+
+static const MemoryRegionOps fdctrl_mem_ops = {
+.read = fdctrl_read_mem,
+.write = fdctrl_write_mem,
+.endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const MemoryRegionOps fdctrl_mem_strict_ops = {
+.read = fdctrl_read_mem,
+.write = fdctrl_write_mem,
+.endianness = DEVICE_NATIVE_ENDIAN,
+.valid = {
+.min_access_size = 1,
+.max_access_size = 1,
+},
+};
+
+static void fdctrl_external_reset_sysbus(DeviceState *d)
+{
+FDCtrlSysBus *sys = SYSBUS_FDC(d);
+FDCtrl *s = >state;
+
+fdctrl_reset(s, 0);
+}
+
+static void fdctrl_handle_tc(void *opaque, int irq, int level)
+{
+trace_fdctrl_tc_pulse(level);
+}
+
+void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
+hwaddr mmio_base, DriveInfo **fds)
+{
+FDCtrl *fdctrl;
+DeviceState *dev;
+SysBusDevice *sbd;
+FDCtrlSysBus *sys;
+
+dev = qdev_new("sysbus-fdc");
+sys = SYSBUS_FDC(dev);
+fdctrl = >state;
+fdctrl->dma_chann = dma_chann; /* FIXME */
+sbd = SYS_BUS_DEVICE(dev);
+sysbus_realize_and_unref(sbd, _fatal);
+sysbus_connect_irq(sbd, 0, irq);
+sysbus_mmio_map(sbd, 0, mmio_base);
+
+fdctrl_init_drives(>state.bus, fds);
+}
+
+void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
+   DriveInfo **fds, qemu_irq *fdc_tc)
+{
+DeviceState *dev;
+FDCtrlSysBus *sys;
+
+dev = qdev_new("sun-fdtwo");
+sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
+sys = SYSBUS_FDC(dev);
+sysbus_connect_irq(SYS_BUS_DEVICE(sys), 0, irq);
+sysbus_mmio_map(SYS_BUS_DEVICE(sys), 0, io_base);
+*fdc_tc = qdev_get_gpio_in(dev, 0);
+
+   

[PULL 2/6] hw/block/fdc: Replace disabled fprintf() by trace event

[John Snow]

From: Philippe Mathieu-Daudé 

Reviewed-by: John Snow 
Acked-by: Mark Cave-Ayland 
Reviewed-by: Mark Cave-Ayland 
Acked-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
Message-id: 20210614193220.2007159-3-phi...@redhat.com
Signed-off-by: John Snow 
---
 hw/block/fdc.c| 7 +--
 hw/block/trace-events | 1 +
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a825c2acba..1d3a047367 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1242,12 +1242,7 @@ static void fdctrl_external_reset_isa(DeviceState *d)
 
 static void fdctrl_handle_tc(void *opaque, int irq, int level)
 {
-//FDCtrl *s = opaque;
-
-if (level) {
-// XXX
-FLOPPY_DPRINTF("TC pulsed\n");
-}
+trace_fdctrl_tc_pulse(level);
 }
 
 /* Change IRQ state */
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 70bed9ddb7..6a0d973805 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -3,6 +3,7 @@
 # fdc.c
 fdc_ioport_read(uint8_t reg, uint8_t value) "read reg 0x%02x val 0x%02x"
 fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
+fdctrl_tc_pulse(int level) "TC pulse: %u"
 
 # pflash_cfi01.c
 # pflash_cfi02.c
-- 
2.31.1

[PULL 1/6] hw/isa/Kconfig: Fix missing dependency ISA_SUPERIO -> FDC

[John Snow]

From: Philippe Mathieu-Daudé 

isa_superio_realize() calls isa_fdc_init_drives(), which is defined
in hw/block/fdc.c, so ISA_SUPERIO needs to select the FDC symbol.

Reported-by: John Snow 
Reviewed-by: Thomas Huth 
Acked-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
Message-id: 20210614193220.2007159-2-phi...@redhat.com
Fixes: c0ff3795143 ("Introduce a CONFIG_ISA_SUPERIO switch for isa-superio.c")
Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: John Snow 
---
 hw/isa/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index 55e0003ce4..7216f66a54 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -17,6 +17,7 @@ config ISA_SUPERIO
 bool
 select ISA_BUS
 select PCKBD
+select FDC
 
 config PC87312
 bool
-- 
2.31.1

[PULL 6/6] hw/block/fdc: Add description to floppy controllers

[John Snow]

From: Philippe Mathieu-Daudé 

Change the '-device help' output from:

  Storage devices:
  name "floppy", bus floppy-bus, desc "virtual floppy drive"
  name "isa-fdc", bus ISA

to:

  Storage devices:
  name "floppy", bus floppy-bus, desc "virtual floppy drive"
  name "isa-fdc", bus ISA, desc "virtual floppy controller"

Signed-off-by: Philippe Mathieu-Daudé 
Message-id: 20210614193220.2007159-7-phi...@redhat.com
Signed-off-by: John Snow 
---
 hw/block/fdc-isa.c| 1 +
 hw/block/fdc-sysbus.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index 0e22a10732..3bf64e0665 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -282,6 +282,7 @@ static void isabus_fdc_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
 
+dc->desc = "virtual floppy controller";
 dc->realize = isabus_fdc_realize;
 dc->fw_name = "fdc";
 dc->reset = fdctrl_external_reset_isa;
diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
index c6308f5300..57fc8773f1 100644
--- a/hw/block/fdc-sysbus.c
+++ b/hw/block/fdc-sysbus.c
@@ -205,6 +205,7 @@ static void sysbus_fdc_class_init(ObjectClass *klass, void 
*data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
+dc->desc = "virtual floppy controller";
 device_class_set_props(dc, sysbus_fdc_properties);
 }
 
@@ -230,6 +231,7 @@ static void sun4m_fdc_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 sbdc->use_strict_io = true;
+dc->desc = "virtual floppy controller";
 device_class_set_props(dc, sun4m_fdc_properties);
 }
 
-- 
2.31.1

[PULL 4/6] hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c

[John Snow]

From: Philippe Mathieu-Daudé 

Some machines use floppy controllers via the SysBus interface,
and don't need to pull in all the ISA code.
Extract the ISA specific code to a new unit: fdc-isa.c, and
add a new Kconfig symbol: "FDC_ISA".
This allows us to remove the FIXME from commit dd0ff8191ab
("isa: express SuperIO dependencies with Kconfig").

Reviewed-by: John Snow 
Acked-by: Mark Cave-Ayland 
Reviewed-by: Mark Cave-Ayland 
Acked-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
Message-id: 20210614193220.2007159-5-phi...@redhat.com
Signed-off-by: John Snow 
---
 hw/block/fdc-isa.c   | 319 +++
 hw/block/fdc.c   | 265 ---
 MAINTAINERS  |   1 +
 hw/block/Kconfig |   8 +-
 hw/block/meson.build |   1 +
 hw/i386/Kconfig  |   2 +-
 hw/isa/Kconfig   |   8 +-
 hw/sparc64/Kconfig   |   2 +-
 8 files changed, 332 insertions(+), 274 deletions(-)
 create mode 100644 hw/block/fdc-isa.c

diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
new file mode 100644
index 00..0e22a10732
--- /dev/null
+++ b/hw/block/fdc-isa.c
@@ -0,0 +1,319 @@
+/*
+ * QEMU Floppy disk emulator (Intel 82078)
+ *
+ * Copyright (c) 2003, 2007 Jocelyn Mayer
+ * Copyright (c) 2008 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+/*
+ * The controller is used in Sun4m systems in a slightly different
+ * way. There are changes in DOR register and DMA is not available.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/block/fdc.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/timer.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/irq.h"
+#include "hw/isa/isa.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "migration/vmstate.h"
+#include "hw/block/block.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/sysemu.h"
+#include "qemu/log.h"
+#include "qemu/main-loop.h"
+#include "qemu/module.h"
+#include "trace.h"
+#include "qom/object.h"
+#include "fdc-internal.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
+
+struct FDCtrlISABus {
+/*< private >*/
+ISADevice parent_obj;
+/*< public >*/
+
+uint32_t iobase;
+uint32_t irq;
+uint32_t dma;
+struct FDCtrl state;
+int32_t bootindexA;
+int32_t bootindexB;
+};
+
+static void fdctrl_external_reset_isa(DeviceState *d)
+{
+FDCtrlISABus *isa = ISA_FDC(d);
+FDCtrl *s = >state;
+
+fdctrl_reset(s, 0);
+}
+
+void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds)
+{
+fdctrl_init_drives(_FDC(fdc)->state.bus, fds);
+}
+
+static const MemoryRegionPortio fdc_portio_list[] = {
+{ 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
+{ 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
+PORTIO_END_OF_LIST(),
+};
+
+static void isabus_fdc_realize(DeviceState *dev, Error **errp)
+{
+ISADevice *isadev = ISA_DEVICE(dev);
+FDCtrlISABus *isa = ISA_FDC(dev);
+FDCtrl *fdctrl = >state;
+Error *err = NULL;
+
+isa_register_portio_list(isadev, >portio_list,
+ isa->iobase, fdc_portio_list, fdctrl,
+ "fdc");
+
+isa_init_irq(isadev, >irq, isa->irq);
+fdctrl->dma_chann = isa->dma;
+if (fdctrl->dma_chann != -1) {
+IsaDmaClass *k;
+fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
+if (!fdctrl->dma) {
+error_setg(errp, "ISA controller does not support DMA");
+return;
+}
+k = ISADMA_GET_CLASS(fdctrl->dma);
+k->register_channel(fdctrl->dma, fdctrl->dma_chann,
+_transfer_handler, fdctrl);
+}
+
+qdev_set_legacy_instance_id(dev, isa->iobase, 2);
+
+fdctrl_realize_common(dev, fdctrl, );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
+}
+
+FloppyDriveType 

[PULL 3/6] hw/block/fdc: Declare shared prototypes in fdc-internal.h

[John Snow]

From: Philippe Mathieu-Daudé 

We want to extract ISA/SysBus code from the generic fdc.c file.
First, declare the prototypes we will access from the new units
into a new local header: "fdc-internal.h".

Acked-by: Mark Cave-Ayland 
Reviewed-by: Mark Cave-Ayland 
Acked-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
Message-id: 20210614193220.2007159-4-phi...@redhat.com
Signed-off-by: John Snow 
---
 hw/block/fdc-internal.h | 158 
 hw/block/fdc.c  | 131 +++--
 MAINTAINERS |   1 +
 3 files changed, 168 insertions(+), 122 deletions(-)
 create mode 100644 hw/block/fdc-internal.h

diff --git a/hw/block/fdc-internal.h b/hw/block/fdc-internal.h
new file mode 100644
index 00..036392e9fc
--- /dev/null
+++ b/hw/block/fdc-internal.h
@@ -0,0 +1,158 @@
+/*
+ * QEMU Floppy disk emulator (Intel 82078)
+ *
+ * Copyright (c) 2003, 2007 Jocelyn Mayer
+ * Copyright (c) 2008 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef HW_BLOCK_FDC_INTERNAL_H
+#define HW_BLOCK_FDC_INTERNAL_H
+
+#include "exec/memory.h"
+#include "exec/ioport.h"
+#include "hw/block/block.h"
+#include "hw/block/fdc.h"
+#include "qapi/qapi-types-block.h"
+
+typedef struct FDCtrl FDCtrl;
+
+/* Floppy bus emulation */
+
+typedef struct FloppyBus {
+BusState bus;
+FDCtrl *fdc;
+} FloppyBus;
+
+/* Floppy disk drive emulation */
+
+typedef enum FDriveRate {
+FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
+FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
+FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
+FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
+} FDriveRate;
+
+typedef enum FDriveSize {
+FDRIVE_SIZE_UNKNOWN,
+FDRIVE_SIZE_350,
+FDRIVE_SIZE_525,
+} FDriveSize;
+
+typedef struct FDFormat {
+FloppyDriveType drive;
+uint8_t last_sect;
+uint8_t max_track;
+uint8_t max_head;
+FDriveRate rate;
+} FDFormat;
+
+typedef enum FDiskFlags {
+FDISK_DBL_SIDES  = 0x01,
+} FDiskFlags;
+
+typedef struct FDrive {
+FDCtrl *fdctrl;
+BlockBackend *blk;
+BlockConf *conf;
+/* Drive status */
+FloppyDriveType drive;/* CMOS drive type*/
+uint8_t perpendicular;/* 2.88 MB access mode*/
+/* Position */
+uint8_t head;
+uint8_t track;
+uint8_t sect;
+/* Media */
+FloppyDriveType disk; /* Current disk type  */
+FDiskFlags flags;
+uint8_t last_sect;/* Nb sector per track*/
+uint8_t max_track;/* Nb of tracks   */
+uint16_t bps; /* Bytes per sector   */
+uint8_t ro;   /* Is read-only   */
+uint8_t media_changed;/* Is media changed   */
+uint8_t media_rate;   /* Data rate of medium*/
+
+bool media_validated; /* Have we validated the media? */
+} FDrive;
+
+struct FDCtrl {
+MemoryRegion iomem;
+qemu_irq irq;
+/* Controller state */
+QEMUTimer *result_timer;
+int dma_chann;
+uint8_t phase;
+IsaDma *dma;
+/* Controller's identification */
+uint8_t version;
+/* HW */
+uint8_t sra;
+uint8_t srb;
+uint8_t dor;
+uint8_t dor_vmstate; /* only used as temp during vmstate */
+uint8_t tdr;
+uint8_t dsr;
+uint8_t msr;
+uint8_t cur_drv;
+uint8_t status0;
+uint8_t status1;
+uint8_t status2;
+/* Command FIFO */
+uint8_t *fifo;
+int32_t fifo_size;
+uint32_t data_pos;
+uint32_t data_len;
+uint8_t data_state;
+uint8_t data_dir;
+uint8_t eot; /* last wanted sector */
+/* States kept only to be returned back */
+/* precompensation */
+uint8_t precomp_trk;
+uint8_t config;
+uint8_t lock;
+/* Power down config (also with status regB access mode */
+uint8_t pwrd;
+/* Floppy drives */
+FloppyBus bus;
+uint8_t num_floppies;
+FDrive drives[MAX_FD];
+struct {
+

[PULL 0/6] Floppy patches

[John Snow]

The following changes since commit e0da9171e02f4534124b9a9e0782b38376c6:

  Merge remote-tracking branch 'remotes/kraxel/tags/ui-20210624-pull-request' 
into staging (2021-06-25 09:10:37 +0100)

are available in the Git repository at:

  https://gitlab.com/jsnow/qemu.git tags/floppy-pull-request

for you to fetch changes up to 9362984f569a5b979714dfba556e370847d5fb87:

  hw/block/fdc: Add description to floppy controllers (2021-06-25 08:53:28 
-0400)


FDC Pull request



Philippe Mathieu-Daudé (6):
  hw/isa/Kconfig: Fix missing dependency ISA_SUPERIO -> FDC
  hw/block/fdc: Replace disabled fprintf() by trace event
  hw/block/fdc: Declare shared prototypes in fdc-internal.h
  hw/block/fdc: Extract ISA floppy controllers to fdc-isa.c
  hw/block/fdc: Extract SysBus floppy controllers to fdc-sysbus.c
  hw/block/fdc: Add description to floppy controllers

 hw/block/fdc-internal.h | 158 ++
 hw/block/fdc-isa.c  | 320 +
 hw/block/fdc-sysbus.c   | 251 
 hw/block/fdc.c  | 621 +---
 MAINTAINERS |   3 +
 hw/block/Kconfig|  12 +-
 hw/block/meson.build|   2 +
 hw/block/trace-events   |   3 +
 hw/i386/Kconfig |   2 +-
 hw/isa/Kconfig  |   7 +-
 hw/mips/Kconfig |   2 +-
 hw/sparc/Kconfig|   2 +-
 hw/sparc64/Kconfig  |   2 +-
 13 files changed, 763 insertions(+), 622 deletions(-)
 create mode 100644 hw/block/fdc-internal.h
 create mode 100644 hw/block/fdc-isa.c
 create mode 100644 hw/block/fdc-sysbus.c

-- 
2.31.1

Re: [PATCH v5 0/5] block-copy: protect block-copy internal structures

[Vladimir Sementsov-Ogievskiy]

24.06.2021 10:20, Emanuele Giuseppe Esposito wrote:

This serie of patches aims to reduce the usage of the
AioContexlock in block-copy, by introducing smaller granularity
locks thus on making the block layer thread safe.

This serie depends on my previous serie that brings thread safety
to the smaller API used by block-copy, like ratelimit, progressmeter
abd co-shared-resource.

What's missing for block-copy to be fully thread-safe is fixing
the CoSleep API to allow cross-thread sleep and wakeup.
Paolo is working on it.

Patch 1 provides a small refactoring, patch 2 introduces the
.method field in BlockCopyState, to be used instead of .use_copy_range,
.copy_size and .zeros.
Patch 3 provide a refactoring in preparation to
the lock added in patch 4 on BlockCopyTask, BlockCopyCallState and
BlockCopyState. Patch 5 uses load_acquire/store_release to make sure
BlockCopyCallState OUT fields are updated before finished is set to
true.



Thanks, applied to the jobs branch

--
Best regards,
Vladimir

Re: [PATCH v3 0/5] block-copy: make helper APIs thread safe

[Vladimir Sementsov-Ogievskiy]

14.06.2021 11:11, Emanuele Giuseppe Esposito wrote:

This serie of patches bring thread safety to the smaller APIs used by
block-copy, namely ratelimit, progressmeter, co-shared-resource
and aiotask.
The end goal is to reduce the usage of AioContexlock in block-copy,
by introducing smaller granularity locks thus on making the block layer
thread safe.

What's missing for block-copy to be fully thread-safe is fixing
the CoSleep API to allow cross-thread sleep and wakeup.
Paolo is working on it and will post the patches once his new
CoSleep API is accepted.

Patches 1-3 work on ratelimit, 4 covers progressmeter and
5 co-shared-resources.

Signed-off-by: Emanuele Giuseppe Esposito 


Thanks, applied to the jobs branch

--
Best regards,
Vladimir

Re: [PATCH v5 0/5] block-copy: protect block-copy internal structures

[Vladimir Sementsov-Ogievskiy]

24.06.2021 10:20, Emanuele Giuseppe Esposito wrote:

This serie of patches aims to reduce the usage of the
AioContexlock in block-copy, by introducing smaller granularity
locks thus on making the block layer thread safe.

This serie depends on my previous serie that brings thread safety
to the smaller API used by block-copy, like ratelimit, progressmeter
abd co-shared-resource.

What's missing for block-copy to be fully thread-safe is fixing
the CoSleep API to allow cross-thread sleep and wakeup.
Paolo is working on it.

Patch 1 provides a small refactoring, patch 2 introduces the
.method field in BlockCopyState, to be used instead of .use_copy_range,
.copy_size and .zeros.
Patch 3 provide a refactoring in preparation to
the lock added in patch 4 on BlockCopyTask, BlockCopyCallState and
BlockCopyState. Patch 5 uses load_acquire/store_release to make sure
BlockCopyCallState OUT fields are updated before finished is set to
true.

Based-on: <20210518094058.25952-1-eespo...@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito 
---
v5:
* Squash patch 3 (improve comments) with patch 5 (add CoMutex).
* Better comments in block-copy, drop IN/OUT/State categories
* Remove some load_acquire in patch 6, replace them with atomic reads


Emanuele Giuseppe Esposito (4):
   block-copy: small refactor in block_copy_task_entry and
 block_copy_common
   block-copy: move progress_set_remaining in block_copy_task_end
   block-copy: add CoMutex lock
   block-copy: atomic .cancelled and .finished fields in
 BlockCopyCallState

Paolo Bonzini (1):
   block-copy: streamline choice of copy_range vs. read/write

  include/block/block-copy.h |   2 +
  block/block-copy.c | 368 +++--
  2 files changed, 231 insertions(+), 139 deletions(-)




Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir

Re: [PATCH v5 00/11] block: file-posix queue

[Max Reitz]

On 25.06.21 10:52, Paolo Bonzini wrote:

On 25/06/21 10:37, Max Reitz wrote:
Thanks, looks good to me, though I’m afraid I’ll be on PTO the next 
two weeks so I can’t take this series through my tree... (I don’t 
really want to send a pull request today when I probably wouldn’t be 
able to deal with potential problems)


I can take it too, if it's okay for you.


Sure!

Re: [PATCH v5 00/11] block: file-posix queue

[Paolo Bonzini]

On 25/06/21 10:37, Max Reitz wrote:
Thanks, looks good to me, though I’m afraid I’ll be on PTO the next two 
weeks so I can’t take this series through my tree... (I don’t really 
want to send a pull request today when I probably wouldn’t be able to 
deal with potential problems)


I can take it too, if it's okay for you.

Paolo

Re: [PATCH v5 00/11] block: file-posix queue

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

New patches:
- 3/4 (for review comments),
- 9 (split for ease of review),
- 11 (new bugfix)

v1->v2: add missing patch

v2->v3: add max_hw_transfer to BlockLimits

v3->v4: fix compilation after patch 1, tweak commit messages according
 to Vladimir's review

v4->v5: round down max_transfer and max_hw_transfer to request alignment
 checkpatch fixes
 return -ENOTSUP, -not -EIO if block limits ioctls fail
 handle host_cdrom like host_device in QAPI
 split "block: try BSD disk size ioctls one after another"
 new bugfix patch "file-posix: handle EINTR during ioctl"


Thanks, looks good to me, though I’m afraid I’ll be on PTO the next two 
weeks so I can’t take this series through my tree... (I don’t really 
want to send a pull request today when I probably wouldn’t be able to 
deal with potential problems)


Max

Re: [PATCH 11/11] file-posix: handle EINTR during ioctl

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

Similar to other handle_aiocb_* functions, handle_aiocb_ioctl needs to cater
for the possibility that ioctl is interrupted by a signal.  Otherwise, the
I/O is incorrectly reported as a failure to the guest.

Reported-by: Gordon Watson 
Signed-off-by: Paolo Bonzini 
---
  block/file-posix.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 74b8216077..a26eab0ac3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1347,7 +1347,9 @@ static int handle_aiocb_ioctl(void *opaque)
  RawPosixAIOData *aiocb = opaque;
  int ret;
  
-ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);

+do {
+ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
+} while (ret == -1 && errno == EINTR);
  if (ret == -1) {
  return -errno;
  }


I think the macro TFR() from qemu-common.h could be applied here, 
probably like `TFR(ret = ioctl(...));`.


No matter:

Reviewed-by: Max Reitz 

Re: [PATCH 11/11] file-posix: handle EINTR during ioctl

[Philippe Mathieu-Daudé]

On 6/24/21 8:04 PM, Paolo Bonzini wrote:
> Similar to other handle_aiocb_* functions, handle_aiocb_ioctl needs to cater
> for the possibility that ioctl is interrupted by a signal.  Otherwise, the
> I/O is incorrectly reported as a failure to the guest.
> 
> Reported-by: Gordon Watson 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/file-posix.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 74b8216077..a26eab0ac3 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1347,7 +1347,9 @@ static int handle_aiocb_ioctl(void *opaque)
>  RawPosixAIOData *aiocb = opaque;
>  int ret;
>  
> -ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
> +do {
> +ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
> +} while (ret == -1 && errno == EINTR);

Shouldn't this use the TFR macro instead?

>  if (ret == -1) {
>  return -errno;
>  }
> 

Re: [PATCH 10/11] block: detect DKIOCGETBLOCKCOUNT/SIZE before use

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

From: Joelle van Dyne 

iOS hosts do not have these defined so we fallback to the
default behaviour.

Co-authored-by: Warner Losh 
Signed-off-by: Joelle van Dyne 
Signed-off-by: Paolo Bonzini 
---
  block/file-posix.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Max Reitz 

Re: [PATCH 09/11] block: try BSD disk size ioctls one after another

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

Try all the possible ioctls for disk size as long as they are
supported, to keep the #if ladder simple.

Extracted and cleaned up from a patch by Joelle van Dyne and
Warner Losh.

Signed-off-by: Paolo Bonzini 
---
  block/file-posix.c | 34 --
  1 file changed, 16 insertions(+), 18 deletions(-)


Thanks, that’s much better.

Reviewed-by: Max Reitz 

Re: [PATCH 03/11] osdep: provide ROUND_DOWN macro

[Philippe Mathieu-Daudé]

On 6/24/21 8:04 PM, Paolo Bonzini wrote:
> osdep.h provides a ROUND_UP macro to hide bitwise operations for the
> purpose of rounding a number up to a power of two; add a ROUND_DOWN
> macro that does the same with truncation towards zero.
> 
> While at it, change the formatting of some comments.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  include/qemu/osdep.h | 28 ++--
>  1 file changed, 22 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 07/11] block: feature detection for host block support

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

From: Joelle van Dyne 

On Darwin (iOS), there are no system level APIs for directly accessing
host block devices. We detect this at configure time.

Signed-off-by: Joelle van Dyne 
Message-Id: <20210315180341.31638-...@getutm.app>
Signed-off-by: Paolo Bonzini 
---
  block/file-posix.c   | 33 ++---
  meson.build  |  6 +-
  qapi/block-core.json | 14 ++
  3 files changed, 37 insertions(+), 16 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH 06/11] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

bs->sg is only true for character devices, but block devices can also
be used with scsi-block and scsi-generic.  Unfortunately BLKSECTGET
returns bytes in an int for /dev/sgN devices, and sectors in a short
for block devices, so account for that in the code.

The maximum transfer also need not be a power of 2 (for example I have
seen disks with 1280 KiB maximum transfer) so there's no need to pass
the result through pow2floor.

Signed-off-by: Paolo Bonzini 
---
  block/file-posix.c | 67 ++
  1 file changed, 38 insertions(+), 29 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH 05/11] block: add max_hw_transfer to BlockLimits

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

For block host devices, I/O can happen through either the kernel file
descriptor I/O system calls (preadv/pwritev, io_submit, io_uring)
or the SCSI passthrough ioctl SG_IO.

In the latter case, the size of each transfer can be limited by the
HBA, while for file descriptor I/O the kernel is able to split and
merge I/O in smaller pieces as needed.  Applying the HBA limits to
file descriptor I/O results in more system calls and suboptimal
performance, so this patch splits the max_transfer limit in two:
max_transfer remains valid and is used in general, while max_hw_transfer
is limited to the maximum hardware size.  max_hw_transfer can then be
included by the scsi-generic driver in the block limits page, to ensure
that the stricter hardware limit is used.

Signed-off-by: Paolo Bonzini 
---
  block/block-backend.c  | 13 +
  block/file-posix.c |  2 +-
  block/io.c |  2 ++
  hw/scsi/scsi-generic.c |  2 +-
  include/block/block_int.h  |  7 +++
  include/sysemu/block-backend.h |  1 +
  6 files changed, 25 insertions(+), 2 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH 04/11] block-backend: align max_transfer to request alignment

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

Block device requests must be aligned to bs->bl.request_alignment.
It makes sense for drivers to align bs->bl.max_transfer the same
way; however when there is no specified limit, blk_get_max_transfer
just returns INT_MAX.  Since the contract of the function does not
specify that INT_MAX means "no maximum", just align the outcome
of the function (whether INT_MAX or bs->bl.max_transfer) before
returning it.

Signed-off-by: Paolo Bonzini 
---
  block/block-backend.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH 03/11] osdep: provide ROUND_DOWN macro

[Max Reitz]

On 24.06.21 20:04, Paolo Bonzini wrote:

osdep.h provides a ROUND_UP macro to hide bitwise operations for the
purpose of rounding a number up to a power of two; add a ROUND_DOWN
macro that does the same with truncation towards zero.

While at it, change the formatting of some comments.

Signed-off-by: Paolo Bonzini 
---
  include/qemu/osdep.h | 28 ++--
  1 file changed, 22 insertions(+), 6 deletions(-)


What a nice thing to have.

Reviewed-by: Max Reitz 

Re: [RFC PATCH 03/10] hw/sd: Move proto_name to SDProto structure

[Bin Meng]

On Thu, Jun 24, 2021 at 10:24 PM Philippe Mathieu-Daudé  wrote:
>
> Introduce a new structure to hold the bus protocol specific
> fields: SDProto. The first field is the protocol name.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 28 
>  1 file changed, 20 insertions(+), 8 deletions(-)
>

Reviewed-by: Bin Meng 

Re: [RFC PATCH 02/10] hw/sd: Extract address_in_range() helper, log invalid accesses

[Bin Meng]

On Thu, Jun 24, 2021 at 10:24 PM Philippe Mathieu-Daudé  wrote:
>
> Multiple commands have to check the address requested is valid.
> Extract this code pattern as a new address_in_range() helper, and
> log invalid accesses as guest errors.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 32 
>  1 file changed, 20 insertions(+), 12 deletions(-)
>

Reviewed-by: Bin Meng 

Re: [RFC PATCH 01/10] hw/sd: When card is in wrong state, log which state it is

[Bin Meng]

On Thu, Jun 24, 2021 at 10:22 PM Philippe Mathieu-Daudé  wrote:
>
> We report the card is in an inconsistent state, but don't precise

%s/don't/is not

> in which state it is. Add this information, as it is useful when
> debugging problems.
>
> Since we will reuse this code, extract as sd_invalid_state_for_cmd()
> helper.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng