Re: [PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Andrey Shinkevich]

On 11.07.2020 22:11, Vladimir Sementsov-Ogievskiy wrote:

03.07.2020 16:13, Andrey Shinkevich wrote:

Read and dump entries from the bitmap directory of QCOW2 image.
It extends the output in the test case #291.



...
  diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py

index d8c058d..7c0dc9a 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -132,6 +132,50 @@ class Qcow2BitmapExt(Qcow2Struct):
    def __init__(self, fd):
  super().__init__(fd=fd)
+    self.read_bitmap_directory(fd)
+
+    def read_bitmap_directory(self, fd):
+    fd.seek(self.bitmap_directory_offset)
+    self.bitmap_directory = \
+    [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]


sounds good. I think, we should restore fd position after reading 
bitmap_directory, to point at the end of extension, to not break 
further extensions loading




Yes, it is done in the constructor of QcowHeaderExtension:

if self.magic == QCOW2_EXT_MAGIC_BITMAPS:

...

position = fd.tell()

...

self.obj = Qcow2BitmapExt(fd=fd)

fd.seek(position)


Andrey

Re: [PATCH] docs/system/arm/orangepi: add instructions for resizing SD image to power of two

[Philippe Mathieu-Daudé]

On 7/12/20 8:37 PM, Niek Linnenbank wrote:
> SD cards need to have a size of a power of two. This commit updates
> the Orange Pi machine documentation to include instructions for
> resizing downloaded images using the qemu-img command.
> 
> Signed-off-by: Niek Linnenbank 
> ---
>  docs/system/arm/orangepi.rst | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/system/arm/orangepi.rst b/docs/system/arm/orangepi.rst
> index c41adad488..6f23907fb6 100644
> --- a/docs/system/arm/orangepi.rst
> +++ b/docs/system/arm/orangepi.rst
> @@ -127,6 +127,16 @@ can be downloaded from:
>  Alternatively, you can also choose to build you own image with buildroot
>  using the orangepi_pc_defconfig. Also see https://buildroot.org for more 
> information.
>  
> +When using an image as an SD card, it must be resized to a power of two. 
> This can be
> +done with the qemu-img command. It is recommended to only increase the image 
> size
> +instead of shrinking it to a power of two, to avoid loss of data. For 
> example,
> +to prepare a downloaded Armbian image, first extract it and then increase
> +its size to one gigabyte as follows:
> +
> +.. code-block:: bash
> +
> +  $ qemu-img resize Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.img 1G
> +
>  You can choose to attach the selected image either as an SD card or as USB 
> mass storage.
>  For example, to boot using the Orange Pi PC Debian image on SD card, simply 
> add the -sd
>  argument and provide the proper root= kernel parameter:
> @@ -213,12 +223,12 @@ Next, unzip the NetBSD image and write the U-Boot 
> binary including SPL using:
>$ dd if=/path/to/u-boot-sunxi-with-spl.bin of=armv7.img bs=1024 seek=8 
> conv=notrunc
>  
>  Finally, before starting the machine the SD image must be extended such
> -that the NetBSD kernel will not conclude the NetBSD partition is larger than
> -the emulated SD card:
> +that the size of the SD image is a power of two and that the NetBSD kernel
> +will not conclude the NetBSD partition is larger than the emulated SD card:
>  
>  .. code-block:: bash
>  
> -  $ dd if=/dev/zero bs=1M count=64 >> armv7.img
> +  $ qemu-img resize armv7.img 2G
>  
>  Start the machine using the following command:
>  
> 

Thanks!

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 1/2] GitLab Gating CI: introduce pipeline-status contrib script

[Thomas Huth]

On 09/07/2020 12.13, Philippe Mathieu-Daudé wrote:
> On 7/9/20 10:55 AM, Erik Skultety wrote:
>> On Wed, Jul 08, 2020 at 10:46:56PM -0400, Cleber Rosa wrote:
>>> This script is intended to be used right after a push to a branch.
>>>
>>> By default, it will look for the pipeline associated with the commit
>>> that is the HEAD of the *local* staging branch.  It can be used as a
>>> one time check, or with the `--wait` option to wait until the pipeline
>>> completes.
>>>
>>> If the pipeline is successful, then a merge of the staging branch into
>>> the master branch should be the next step.
>>>
>>> Signed-off-by: Cleber Rosa 
>>> ---
>>>  scripts/ci/gitlab-pipeline-status | 156 ++
>>>  1 file changed, 156 insertions(+)
>>>  create mode 100755 scripts/ci/gitlab-pipeline-status
>>>
>>> diff --git a/scripts/ci/gitlab-pipeline-status 
>>> b/scripts/ci/gitlab-pipeline-status
>>> new file mode 100755
>>> index 00..4a9de39872
>>> --- /dev/null
>>> +++ b/scripts/ci/gitlab-pipeline-status
>>> @@ -0,0 +1,156 @@
>>> +#!/usr/bin/env python3
>>> +#
>>> +# Copyright (c) 2019-2020 Red Hat, Inc.
>>> +#
>>> +# Author:
>>> +#  Cleber Rosa 
>>> +#
>>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>>> +# later.  See the COPYING file in the top-level directory.
>>> +
>>> +"""
>>> +Checks the GitLab pipeline status for a given commit commit
>>
>> s/commit$/(hash|sha|ID|)
>>
>>> +"""
>>> +
>>> +# pylint: disable=C0103
>>> +
>>> +import argparse
>>> +import http.client
>>> +import json
>>> +import os
>>> +import subprocess
>>> +import time
>>> +import sys
>>> +
>>> +
>>> +def get_local_staging_branch_commit():
>>> +"""
>>> +Returns the commit sha1 for the *local* branch named "staging"
>>> +"""
>>> +result = subprocess.run(['git', 'rev-parse', 'staging'],
>>
>> If one day Peter decides that "staging" is not a cool name anymore and use a
>> different name for the branch :) we should account for that and make it a
>> variable, possibly even parametrize this function with it.
> 
> This script can be used by any fork, not only Peter.
> So having a parameter (default to 'staging') is a requisite IMO.
> 
>>> +stdin=subprocess.DEVNULL,
>>> +stdout=subprocess.PIPE,
>>> +stderr=subprocess.DEVNULL,
>>> +cwd=os.path.dirname(__file__),
>>> +universal_newlines=True).stdout.strip()
>>> +if result == 'staging':
>>> +raise ValueError("There's no local staging branch")
>>
>> "There's no local branch named 'staging'" would IMO be more descriptive, so 
>> as
>> not to confuse it with staging in git.
>>
>>> +if len(result) != 40:
>>> +raise ValueError("Branch staging HEAD doesn't look like a sha1")
>>> +return result
>>> +
>>> +
>>> +def get_pipeline_status(project_id, commit_sha1):
>>> +"""
>>> +Returns the JSON content of the pipeline status API response
>>> +"""
>>> +url = '/api/v4/projects/{}/pipelines?sha={}'.format(project_id,
>>> +commit_sha1)
>>> +connection = http.client.HTTPSConnection('gitlab.com')
>>> +connection.request('GET', url=url)
>>> +response = connection.getresponse()
>>> +if response.code != http.HTTPStatus.OK:
>>> +raise ValueError("Failed to receive a successful response")
>>> +json_response = json.loads(response.read())
>>
>> a blank line separating the commentary block would slightly help readability
>>
>>> +# afaict, there should one one pipeline for the same project + commit
>>
>> s/one one/be only one/
> 
> 'afaict' is not a word.
> 
>>
>>> +# if this assumption is false, we can add further filters to the
>>> +# url, such as username, and order_by.
>>> +if not json_response:
>>> +raise ValueError("No pipeline found")
>>> +return json_response[0]
>>> +
>>> +
>>> +def wait_on_pipeline_success(timeout, interval,
>>> + project_id, commit_sha):
>>> +"""
>>> +Waits for the pipeline to end up to the timeout given
>>
>> "Waits for the pipeline to finish within the given timeout"
>>
>>> +"""
>>> +start = time.time()
>>> +while True:
>>> +if time.time() >= (start + timeout):
>>> +print("Waiting on the pipeline success timed out")
>>
>> s/success//
>> (the pipeline doesn't always have to finish with success)
>>
>>> +return False
>>> +
>>> +status = get_pipeline_status(project_id, commit_sha)
>>> +if status['status'] == 'running':
>>> +time.sleep(interval)
>>> +print('running...')
> 
> If we want to automate the use of this script by a daemon, it would
> be better to use the logging class. Then maybe 'running...' is for
> the DEBUG level, Other print() calls can be updated to WARN/INFO
> levels.
> 
>>> +continue
>>> +
>>> +if status[

Re: [PATCH] Allow acpi-tmr size=2

[Michael Tokarev]

12.07.2020 15:00, Simon John wrote:
> macos guests no longer boot after commit 
> 5d971f9e672507210e77d020d89e0e89165c8fc9
> 
> acpi-tmr needs 2 byte memory accesses, so breaks as that commit only allows 4 
> bytes.
> 
> Fixes: 5d971f9e672507210e7 (memory: Revert "memory: accept mismatching sizes 
> in memory_region_access_valid")
> Buglink: https://bugs.launchpad.net/qemu/+bug/1886318

Actually this fixes 77d58b1e47c8d1c661f98f12b47ab519d3561488
Author: Gerd Hoffmann 
Date:   Thu Nov 22 12:12:30 2012 +0100
Subject: apci: switch timer to memory api
Signed-off-by: Gerd Hoffmann 

because this is the commit which put min_access_size = 4 in there
(5d971f9e672507210e7 is just a messenger, actual error were here
earlier but it went unnoticed).

While min_access_size=4 was most likely an error, I wonder why
we use 1 now, while the subject says it needs 2? What real min
size is here for ACPI PM timer?

/mjt

> Signed-off-by: Simon John 
> ---
>  hw/acpi/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index f6d9ec4f13..05ff29b9d7 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -527,7 +527,7 @@ static void acpi_pm_tmr_write(void *opaque, hwaddr addr, 
> uint64_t val,
>  static const MemoryRegionOps acpi_pm_tmr_ops = {
>  .read = acpi_pm_tmr_read,
>  .write = acpi_pm_tmr_write,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>  .valid.max_access_size = 4,
>  .endianness = DEVICE_LITTLE_ENDIAN,
>  };

Re: [PATCH v5 1/4] target/i386: add missing vmx features for several CPU models

[Chenyi Qiang]

On 7/11/2020 12:48 AM, Eduardo Habkost wrote:

On Fri, Jul 10, 2020 at 09:45:49AM +0800, Chenyi Qiang wrote:



On 7/10/2020 6:12 AM, Eduardo Habkost wrote:


I'm very sorry for taking so long to review this.  Question
below:

On Fri, Jun 19, 2020 at 03:31:11PM +0800, Chenyi Qiang wrote:

Add some missing VMX features in Skylake-Server, Cascadelake-Server and
Icelake-Server CPU models based on the output of Paolo's script.

Signed-off-by: Chenyi Qiang 


Why are you changing the v1 definition instead adding those new
features in a new version of the CPU model, just like you did in
patch 3/4?



I suppose these missing vmx features are not quite necessary for customers.
Just post it here to see if they are worth being added.
Adding a new version is reasonable. Is it appropriate to put all the missing
features in patch 1/4, 3/4, 4/4 in a same version?


Yes, it would be OK to add only one new version with all the new
features.



During the coding, I prefer to split the missing vmx features into a new 
version of CPU model, because the vmx features depends on CPUID_EXT_VMX. 
I think It would be better to distinguish it instead of enabling the vmx 
transparently. i.e.

{
.version = 4,
.props = (PropValue[]) {
{ "sha-ni", "on" },
... ...
{ "model", "106" },
{ /* end of list */ }
},
},
{
.version = 5,
.props = (PropValue[]) {
{ "vmx", "on" },
{ "vmx-eptp-switching", "on" },
{ /* end of list */ }
},
},

What do you think about?

[PATCH v9 08/10] qcow2.py: Introduce '-j' key to dump in JSON format

[Andrey Shinkevich]

Add the command key to the qcow2.py arguments list to dump QCOW2
metadata in JSON format. Here is the suggested way to do that. The
implementation of the dump in JSON format is in the patch that follows.

Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/qcow2.py| 19 +++
 tests/qemu-iotests/qcow2_format.py | 16 
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index 0910e6a..7402279 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -26,16 +26,19 @@ from qcow2_format import (
 )
 
 
+dump_json = False
+
+
 def cmd_dump_header(fd):
 h = QcowHeader(fd)
-h.dump()
+h.dump(dump_json)
 print()
-h.dump_extensions()
+h.dump_extensions(dump_json)
 
 
 def cmd_dump_header_exts(fd):
 h = QcowHeader(fd)
-h.dump_extensions()
+h.dump_extensions(dump_json)
 
 
 def cmd_set_header(fd, name, value):
@@ -134,6 +137,11 @@ cmds = [
 
 
 def main(filename, cmd, args):
+global dump_json
+dump_json = '-j' in sys.argv
+if dump_json:
+sys.argv.remove('-j')
+args.remove('-j')
 fd = open(filename, "r+b")
 try:
 for name, handler, num_args, desc in cmds:
@@ -151,11 +159,14 @@ def main(filename, cmd, args):
 
 
 def usage():
-print("Usage: %s   [, ...]" % sys.argv[0])
+print("Usage: %s   [, ...] [, ...]" % sys.argv[0])
 print("")
 print("Supported commands:")
 for name, handler, num_args, desc in cmds:
 print("%-20s - %s" % (name, desc))
+print("")
+print("Supported keys:")
+print("%-20s - %s" % ('-j', 'Dump in JSON format'))
 
 
 if __name__ == '__main__':
diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 22be3ee..a9bd5a8 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -109,7 +109,7 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
 self.__dict__ = dict((field[2], values[i])
  for i, field in enumerate(self.fields))
 
-def dump(self):
+def dump(self, dump_json=None):
 for f in self.fields:
 value = self.__dict__[f[2]]
 if isinstance(f[1], str):
@@ -140,8 +140,8 @@ class Qcow2BitmapExt(Qcow2Struct):
 [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
  for _ in range(self.nb_bitmaps)]
 
-def dump(self):
-super().dump()
+def dump(self, dump_json=None):
+super().dump(dump_json)
 for entry in self.bitmap_directory:
 print()
 entry.dump()
@@ -185,7 +185,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 self.bitmap_table = Qcow2BitmapTable(raw_table=table,
  cluster_size=self.cluster_size)
 
-def dump(self):
+def dump(self, dump_json=None):
 print(f'{"Bitmap name":<25} {self.name}')
 super(Qcow2BitmapDirEntry, self).dump()
 self.bitmap_table.dump()
@@ -293,13 +293,13 @@ class QcowHeaderExtension(Qcow2Struct):
 else:
 self.obj = None
 
-def dump(self):
+def dump(self, dump_json=None):
 super().dump()
 
 if self.obj is None:
 print(f'{"data":<25} {self.data_str}')
 else:
-self.obj.dump()
+self.obj.dump(dump_json)
 
 @classmethod
 def create(cls, magic, data):
@@ -398,8 +398,8 @@ class QcowHeader(Qcow2Struct):
 buf = buf[0:header_bytes-1]
 fd.write(buf)
 
-def dump_extensions(self):
+def dump_extensions(self, dump_json=None):
 for ex in self.extensions:
 print('Header extension:')
-ex.dump()
+ex.dump(dump_json)
 print()
-- 
1.8.3.1

[PATCH v9 02/10] qcow2_format.py: make printable data an extension class member

[Andrey Shinkevich]

Let us differ binary data type from string one for the extension data
variable and keep the string as the QcowHeaderExtension class member.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/qcow2_format.py | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index cc432e7..2f3681b 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -165,6 +165,13 @@ class QcowHeaderExtension(Qcow2Struct):
 self.data = fd.read(padded)
 assert self.data is not None
 
+data_str = self.data[:self.length]
+if all(c in string.printable.encode('ascii') for c in data_str):
+data_str = f"'{ data_str.decode('ascii') }'"
+else:
+data_str = ''
+self.data_str = data_str
+
 if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
 self.obj = Qcow2BitmapExt(data=self.data)
 else:
@@ -174,12 +181,7 @@ class QcowHeaderExtension(Qcow2Struct):
 super().dump()
 
 if self.obj is None:
-data = self.data[:self.length]
-if all(c in string.printable.encode('ascii') for c in data):
-data = f"'{ data.decode('ascii') }'"
-else:
-data = ''
-print(f'{"data":<25} {data}')
+print(f'{"data":<25} {self.data_str}')
 else:
 self.obj.dump()
 
-- 
1.8.3.1

[PATCH v9 07/10] qcow2_format.py: Dump bitmap table serialized entries

[Andrey Shinkevich]

Add bitmap table information to the QCOW2 metadata dump.
It extends the output of the test case #291

Bitmap name   bitmap-1
...
Bitmap table   typeoffset   size
0  serialized  4718592  65536
1  serialized  4294967296   65536
2  serialized  5348033147437056 65536
3  serialized  1379227385882214465536
4  serialized  4718592  65536
5  serialized  4294967296   65536
6  serialized  4503608217305088 65536
7  serialized  1407374883553280065536

Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/291.out | 45 ++
 tests/qemu-iotests/qcow2_format.py | 42 +++
 2 files changed, 87 insertions(+)

diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index 53a8eeb..9f465c6 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -41,6 +41,15 @@ type  1
 granularity_bits  19
 name_size 2
 extra_data_size   0
+Bitmap table   typeoffset   size
+0  serialized  5046272  65536
+1  all-zeroes  065536
+2  all-zeroes  065536
+3  all-zeroes  065536
+4  all-zeroes  065536
+5  all-zeroes  065536
+6  all-zeroes  065536
+7  all-zeroes  065536
 
 Bitmap name   b2
 bitmap_table_offset   0x50
@@ -50,6 +59,15 @@ type  1
 granularity_bits  16
 name_size 2
 extra_data_size   0
+Bitmap table   typeoffset   size
+0  serialized  5177344  65536
+1  all-zeroes  065536
+2  all-zeroes  065536
+3  all-zeroes  065536
+4  all-zeroes  065536
+5  all-zeroes  065536
+6  all-zeroes  065536
+7  all-zeroes  065536
 
 
 === Bitmap preservation not possible to non-qcow2 ===
@@ -124,6 +142,15 @@ type  1
 granularity_bits  19
 name_size 2
 extra_data_size   0
+Bitmap table   typeoffset   size
+0  serialized  4587520  65536
+1  all-zeroes  065536
+2  all-zeroes  065536
+3  all-zeroes  065536
+4  all-zeroes  065536
+5  all-zeroes  065536
+6  all-zeroes  065536
+7  all-zeroes  065536
 
 Bitmap name   b2
 bitmap_table_offset   0x49
@@ -133,6 +160,15 @@ type  1
 granularity_bits  16
 name_size 2
 extra_data_size   0
+Bitmap table   typeoffset   size
+0  serialized  4718592  65536
+1  serialized  4294967296   65536
+2  serialized  5348033147437056 65536
+3  serialized  1379227385882214465536
+4  serialized  4718592  65536
+5  serialized  4294967296   65536
+6  serialized  4503608217305088 65536
+7  serialized  1407374883553280065536
 
 Bitmap name   b0
 bitmap_table_offset   0x51
@@ -142,6 +178,15 @@ type  1
 granularity_bits  16
 name_size 2
 extra_data_size   0
+Bitmap table   typeoffset   size
+0  serialized  5242880  65536
+1  all-zeroes  065536
+2  all-zeroes  065536
+3  all-zeroes  065536
+4  all-zeroes  065536
+5  all-zeroes  065536
+6  all-zeroes  065536
+7  all-zeroes  065536
 
 
 === Check bitmap contents ===
diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index d71

[PATCH v9 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method

[Andrey Shinkevich]

There are two ways to initialize a class derived from Qcow2Struct:
1. Pass a block of binary data to the constructor.
2. Pass the file descriptor to allow reading the file from constructor.
Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
support a scattered reading in the initialization chain.
The implementation comes with the patch that follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/qcow2_format.py | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2f3681b..9eb6b5b 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -62,8 +62,8 @@ class Qcow2StructMeta(type):
 
 
 class Qcow2Struct(metaclass=Qcow2StructMeta):
-
-"""Qcow2Struct: base class for qcow2 data structures
+"""
+Qcow2Struct: base class for qcow2 data structures
 
 Successors should define fields class variable, which is: list of tuples,
 each of three elements:
@@ -173,7 +173,13 @@ class QcowHeaderExtension(Qcow2Struct):
 self.data_str = data_str
 
 if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
-self.obj = Qcow2BitmapExt(data=self.data)
+assert fd is not None
+position = fd.tell()
+# Step back to reread data
+padded = (self.length + 7) & ~7
+fd.seek(-padded, 1)
+self.obj = Qcow2BitmapExt(fd=fd)
+fd.seek(position)
 else:
 self.obj = None
 
-- 
1.8.3.1

[PATCH v9 09/10] qcow2_format.py: collect fields to dump in JSON format

[Andrey Shinkevich]

As __dict__ is being extended with class members we do not want to
print, make a light copy of the initial __dict__ and extend the copy
by adding lists we have to print in the JSON output.

Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/qcow2_format.py | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index a9bd5a8..4ce6d95 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
 self.__dict__ = dict((field[2], values[i])
  for i, field in enumerate(self.fields))
 
+self.fields_dict = self.__dict__.copy()
+
 def dump(self, dump_json=None):
 for f in self.fields:
 value = self.__dict__[f[2]]
@@ -139,6 +141,7 @@ class Qcow2BitmapExt(Qcow2Struct):
 self.bitmap_directory = \
 [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
  for _ in range(self.nb_bitmaps)]
+self.fields_dict.update(bitmap_directory=self.bitmap_directory)
 
 def dump(self, dump_json=None):
 super().dump(dump_json)
@@ -184,6 +187,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 table = [e[0] for e in struct.iter_unpack('>Q', fd.read(table_size))]
 self.bitmap_table = Qcow2BitmapTable(raw_table=table,
  cluster_size=self.cluster_size)
+self.fields_dict.update(bitmap_table=self.bitmap_table)
 
 def dump(self, dump_json=None):
 print(f'{"Bitmap name":<25} {self.name}')
-- 
1.8.3.1

[PATCH v9 05/10] qcow2_format.py: Dump bitmap directory information

[Andrey Shinkevich]

Read and dump entries from the bitmap directory of QCOW2 image.
It extends the output in the test case #291.

Header extension:
magic 0x23852875 (Bitmaps)
...

Bitmap name   bitmap-1
bitmap_table_offset   0xf
bitmap_table_size 1
flags 0x2 (['auto'])
type  1
granularity_bits  16
name_size 8
extra_data_size   0

Suggested-by: Kevin Wolf 
Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/291.out | 45 
 tests/qemu-iotests/qcow2_format.py | 47 ++
 2 files changed, 92 insertions(+)

diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index 08bfaaa..53a8eeb 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -33,6 +33,24 @@ reserved320
 bitmap_directory_size 0x40
 bitmap_directory_offset   0x51
 
+Bitmap name   b1
+bitmap_table_offset   0x4e
+bitmap_table_size 1
+flags 0x0 ([])
+type  1
+granularity_bits  19
+name_size 2
+extra_data_size   0
+
+Bitmap name   b2
+bitmap_table_offset   0x50
+bitmap_table_size 1
+flags 0x2 (['auto'])
+type  1
+granularity_bits  16
+name_size 2
+extra_data_size   0
+
 
 === Bitmap preservation not possible to non-qcow2 ===
 
@@ -98,6 +116,33 @@ reserved320
 bitmap_directory_size 0x60
 bitmap_directory_offset   0x52
 
+Bitmap name   b1
+bitmap_table_offset   0x47
+bitmap_table_size 1
+flags 0x0 ([])
+type  1
+granularity_bits  19
+name_size 2
+extra_data_size   0
+
+Bitmap name   b2
+bitmap_table_offset   0x49
+bitmap_table_size 1
+flags 0x2 (['auto'])
+type  1
+granularity_bits  16
+name_size 2
+extra_data_size   0
+
+Bitmap name   b0
+bitmap_table_offset   0x51
+bitmap_table_size 1
+flags 0x0 ([])
+type  1
+granularity_bits  16
+name_size 2
+extra_data_size   0
+
 
 === Check bitmap contents ===
 
diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 934062a..1438792 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -129,6 +129,53 @@ class Qcow2BitmapExt(Qcow2Struct):
 ('u64', '{:#x}', 'bitmap_directory_offset')
 )
 
+def __init__(self, fd):
+super().__init__(fd=fd)
+self.read_bitmap_directory(fd)
+
+def read_bitmap_directory(self, fd):
+fd.seek(self.bitmap_directory_offset)
+self.bitmap_directory = \
+[Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
+
+def dump(self):
+super().dump()
+for entry in self.bitmap_directory:
+print()
+entry.dump()
+
+
+class Qcow2BitmapDirEntry(Qcow2Struct):
+
+fields = (
+('u64', '{:#x}', 'bitmap_table_offset'),
+('u32', '{}', 'bitmap_table_size'),
+('u32', BitmapFlags, 'flags'),
+('u8',  '{}', 'type'),
+('u8',  '{}', 'granularity_bits'),
+('u16', '{}', 'name_size'),
+('u32', '{}', 'extra_data_size')
+)
+
+def __init__(self, fd):
+super().__init__(fd=fd)
+# Seek relative to the current position in the file
+fd.seek(self.extra_data_size, 1)
+bitmap_name = fd.read(self.name_size)
+self.name = bitmap_name.decode('ascii')
+# Move position to the end of the entry in the directory
+entry_raw_size = self.bitmap_dir_entry_raw_size()
+padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
+fd.seek(padding, 1)
+
+def bitmap_dir_entry_raw_size(self):
+return struct.calcsize(self.fmt) + self.name_size + \
+self.extra_data_size
+
+def dump(self):
+print(f'{"Bitmap name":<25} {self.name}')
+super(Qcow2BitmapDirEntry, self).dump()
+
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
 
-- 
1.8.3.1

[PATCH v9 00/10] iotests: Dump QCOW2 dirty bitmaps metadata

[Andrey Shinkevich]

Add dirty bitmap information to QCOW2 metadata dump in the qcow2_format.py.

v9:
  01: In patch 0003, removed explicit constructor in the class Qcow2BitmapExt.
  02: In patch 0004, the format string was changed.
 

Andrey Shinkevich (10):
  qcow2: Fix capitalization of header extension constant.
  qcow2_format.py: make printable data an extension class member
  qcow2_format.py: change Qcow2BitmapExt initialization method
  qcow2_format.py: dump bitmap flags in human readable way.
  qcow2_format.py: Dump bitmap directory information
  qcow2_format.py: pass cluster size to substructures
  qcow2_format.py: Dump bitmap table serialized entries
  qcow2.py: Introduce '-j' key to dump in JSON format
  qcow2_format.py: collect fields to dump in JSON format
  qcow2_format.py: support dumping metadata in JSON format

 block/qcow2.c  |   2 +-
 docs/interop/qcow2.txt |   2 +-
 tests/qemu-iotests/291.out |  90 
 tests/qemu-iotests/qcow2.py|  19 +++-
 tests/qemu-iotests/qcow2_format.py | 213 ++---
 5 files changed, 304 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH v9 06/10] qcow2_format.py: pass cluster size to substructures

[Andrey Shinkevich]

The cluster size of an image is the QcowHeader class member and may be
obtained by dependent extension structures such as Qcow2BitmapExt for
further bitmap table details print.

Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/qcow2_format.py | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 1438792..d7198a9 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -129,14 +129,16 @@ class Qcow2BitmapExt(Qcow2Struct):
 ('u64', '{:#x}', 'bitmap_directory_offset')
 )
 
-def __init__(self, fd):
+def __init__(self, fd, cluster_size):
 super().__init__(fd=fd)
+self.cluster_size = cluster_size
 self.read_bitmap_directory(fd)
 
 def read_bitmap_directory(self, fd):
 fd.seek(self.bitmap_directory_offset)
 self.bitmap_directory = \
-[Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
+[Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
+ for _ in range(self.nb_bitmaps)]
 
 def dump(self):
 super().dump()
@@ -157,8 +159,9 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 ('u32', '{}', 'extra_data_size')
 )
 
-def __init__(self, fd):
+def __init__(self, fd, cluster_size):
 super().__init__(fd=fd)
+self.cluster_size = cluster_size
 # Seek relative to the current position in the file
 fd.seek(self.extra_data_size, 1)
 bitmap_name = fd.read(self.name_size)
@@ -198,11 +201,13 @@ class QcowHeaderExtension(Qcow2Struct):
 # then padding to next multiply of 8
 )
 
-def __init__(self, magic=None, length=None, data=None, fd=None):
+def __init__(self, magic=None, length=None, data=None, fd=None,
+ cluster_size=None):
 """
 Support both loading from fd and creation from user data.
 For fd-based creation current position in a file will be used to read
 the data.
+The cluster_size value may be obtained by dependent structures.
 
 This should be somehow refactored and functionality should be moved to
 superclass (to allow creation of any qcow2 struct), but then, fields
@@ -241,7 +246,7 @@ class QcowHeaderExtension(Qcow2Struct):
 # Step back to reread data
 padded = (self.length + 7) & ~7
 fd.seek(-padded, 1)
-self.obj = Qcow2BitmapExt(fd=fd)
+self.obj = Qcow2BitmapExt(fd=fd, cluster_size=cluster_size)
 fd.seek(position)
 else:
 self.obj = None
@@ -317,7 +322,7 @@ class QcowHeader(Qcow2Struct):
 end = self.cluster_size
 
 while fd.tell() < end:
-ext = QcowHeaderExtension(fd=fd)
+ext = QcowHeaderExtension(fd=fd, cluster_size=self.cluster_size)
 if ext.magic == 0:
 break
 else:
-- 
1.8.3.1

[PATCH v9 04/10] qcow2_format.py: dump bitmap flags in human readable way.

[Andrey Shinkevich]

Introduce the class BitmapFlags that parses a bitmap flags mask.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/qcow2_format.py | 16 
 1 file changed, 16 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 9eb6b5b..934062a 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -40,6 +40,22 @@ class Flags64(Qcow2Field):
 return str(bits)
 
 
+class BitmapFlags(Qcow2Field):
+
+flags = {
+0x1: 'in-use',
+0x2: 'auto'
+}
+
+def __str__(self):
+bits = []
+for bit in range(64):
+flag = self.value & (1 << bit)
+if flag:
+bits.append(self.flags.get(flag, f'bit-{bit}'))
+return f'{self.value:#x} ({bits})'
+
+
 class Enum(Qcow2Field):
 
 def __str__(self):
-- 
1.8.3.1

[PATCH v9 01/10] qcow2: Fix capitalization of header extension constant.

[Andrey Shinkevich]

Make the capitalization of the hexadecimal numbers consistent for the
QCOW2 header extension constants in docs/interop/qcow2.txt.

Suggested-by: Eric Blake 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c  | 2 +-
 docs/interop/qcow2.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0cd2e67..80dfe5f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -66,7 +66,7 @@ typedef struct {
 } QEMU_PACKED QCowExtension;
 
 #define  QCOW2_EXT_MAGIC_END 0
-#define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
+#define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xe2792aca
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
 #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index cb72346..f072e27 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -231,7 +231,7 @@ be stored. Each extension has a structure like the 
following:
 
 Byte  0 -  3:   Header extension type:
 0x - End of the header extension area
-0xE2792ACA - Backing file format name string
+0xe2792aca - Backing file format name string
 0x6803f857 - Feature name table
 0x23852875 - Bitmaps extension
 0x0537be77 - Full disk encryption header pointer
-- 
1.8.3.1

[PATCH v9 10/10] qcow2_format.py: support dumping metadata in JSON format

[Andrey Shinkevich]

Implementation of dumping QCOW2 image metadata.
The sample output:
{
"Header_extensions": [
{
"name": "Feature table",
"magic": 1745090647,
"length": 192,
"data_str": ""
},
{
"name": "Bitmaps",
"magic": 595929205,
"length": 24,
"data": {
"nb_bitmaps": 2,
"reserved32": 0,
"bitmap_directory_size": 64,
"bitmap_directory_offset": 1048576,
"bitmap_directory": [
{
"name": "bitmap-1",
"bitmap_table_offset": 589824,
"bitmap_table_size": 1,
"flags": 2,
"type": 1,
"granularity_bits": 15,
"name_size": 8,
"extra_data_size": 0,
"bitmap_table": {
"entries": [
{
"type": "serialized",
"offset": 655360
},
...

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
 tests/qemu-iotests/qcow2_format.py | 59 ++
 1 file changed, 59 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 4ce6d95..1b953c1 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -19,6 +19,15 @@
 
 import struct
 import string
+import json
+
+
+class ComplexEncoder(json.JSONEncoder):
+def default(self, obj):
+if hasattr(obj, 'get_fields_dict'):
+return obj.get_fields_dict()
+else:
+return json.JSONEncoder.default(self, obj)
 
 
 class Qcow2Field:
@@ -112,6 +121,11 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
 self.fields_dict = self.__dict__.copy()
 
 def dump(self, dump_json=None):
+if dump_json:
+print(json.dumps(self.get_fields_dict(), indent=4,
+ cls=ComplexEncoder))
+return
+
 for f in self.fields:
 value = self.__dict__[f[2]]
 if isinstance(f[1], str):
@@ -149,6 +163,9 @@ class Qcow2BitmapExt(Qcow2Struct):
 print()
 entry.dump()
 
+def get_fields_dict(self):
+return self.fields_dict
+
 
 class Qcow2BitmapDirEntry(Qcow2Struct):
 
@@ -194,6 +211,11 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 super(Qcow2BitmapDirEntry, self).dump()
 self.bitmap_table.dump()
 
+def get_fields_dict(self):
+bmp_name = dict(name=self.name)
+bme_dict = {**bmp_name, **self.fields_dict}
+return bme_dict
+
 
 class Qcow2BitmapTableEntry:
 
@@ -209,6 +231,9 @@ class Qcow2BitmapTableEntry:
 else:
 self.type = 'all-zeroes'
 
+def get_fields_dict(self):
+return dict(type=self.type, offset=self.offset)
+
 
 class Qcow2BitmapTable:
 
@@ -225,6 +250,18 @@ class Qcow2BitmapTable:
 for i, entry in bitmap_table:
 print(f'{i:<14} {entry.type:<15} {entry.offset:<24} {size}')
 
+def get_fields_dict(self):
+return dict(entries=self.entries)
+
+
+class Qcow2HeaderExtensionsDoc:
+
+def __init__(self, extensions):
+self.extensions = extensions
+
+def get_fields_dict(self):
+return dict(Header_extensions=self.extensions)
+
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
 
@@ -240,6 +277,9 @@ class QcowHeaderExtension(Qcow2Struct):
 0x44415441: 'Data file'
 }
 
+def get_fields_dict(self):
+return self.mapping.get(self.value, "")
+
 fields = (
 ('u32', Magic, 'magic'),
 ('u32', '{}', 'length')
@@ -305,6 +345,16 @@ class QcowHeaderExtension(Qcow2Struct):
 else:
 self.obj.dump(dump_json)
 
+def get_fields_dict(self):
+ext_name = dict(name=self.Magic(self.magic))
+he_dict = {**ext_name, **self.fields_dict}
+if self.obj is not None:
+he_dict.update(data=self.obj)
+else:
+he_dict.update(data_str=self.data_str)
+
+return he_dict
+
 @classmethod
 def create(cls, magic, data):
 return QcowHeaderExtension(magic, len(data), data)
@@ -403,7 +453,16 @@ class QcowHeader(Qcow2Struct):
 fd.write(buf)
 
 def dump_extensions(self, dump_json=None):
+if dump_json:
+ext_doc = Qcow2HeaderExtensionsDoc(self.extensions)
+print(json.dumps(ext_doc.get_fields_dict(), indent=4,
+ cls=ComplexEncoder))
+return
+
 for ex in self.extensions:
 print('Header extension:')
 ex.dump(dump_json)
 print()
+
+def get_fields_dict(self):
+return self.fields

Re: [PATCH] docs/system/s390x: Improve the 3270 documentation

[Thomas Huth]

On 10/07/2020 12.55, Cornelia Huck wrote:
> On Thu,  9 Jul 2020 18:04:39 +0200
> Thomas Huth  wrote:
> 
>> There is some additional information about the 3270 support in our
>> Wiki at https://wiki.qemu.org/Features/3270 - so let's include this
>> information into the main documentation now to have one single
>> source of information (the Wiki page could later be removed).
> 
> No objection; but what should our general policy with regard to wiki
> pages vs. documentation be? The 3270 support is pretty much static, but
> e.g. for vfio-ccw, we use the wiki page as a place where we track
> things that should be worked on etc.

I think that docs which are "done" should go into the git repo. If they
are rather still work-in-progress and change very often, or relate to a
feature that has not been merged yet, the wiki is likely the better place.

> (Related: What is the scope of our QEMU documentation? Is a libvirt XML
> snippet on topic? Kernel configuration options (as in here)?)

As long as the focus is still on QEMU, I think it is fine. If the
information is only about libvirt or the kernel, that documentation
should maybe rather be added to libvirt.org or linux-kvm.org instead.

>> Signed-off-by: Thomas Huth 
>> ---
>>  docs/system/s390x/3270.rst | 43 --
>>  1 file changed, 37 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/system/s390x/3270.rst b/docs/system/s390x/3270.rst
>> index 1774cdcadf..80350264d7 100644
>> --- a/docs/system/s390x/3270.rst
>> +++ b/docs/system/s390x/3270.rst
>> @@ -1,9 +1,15 @@
>>  3270 devices
>>  
>>  
>> -QEMU supports connecting an external 3270 terminal emulator (such as
>> -``x3270``) to make a single 3270 device available to a guest. Note that this
>> -supports basic features only.
>> +The 3270 is the classic 'green-screen' console of the mainframes (see the
>> +`IBM 3270 Wikipedia article `__).
>> +
>> +The 3270 data stream is not implemented within QEMU; the device only 
>> provides
>> +TN3270 (a telnet extension; see `RFC 854 
>> `__
>> +and `RFC 1576 `__) and leaves the heavy
>> +lifting to an external 3270 terminal emulator (such as ``x3270``) to make a
>> +single 3270 device available to a guest. Note that this supports basic
>> +features only.
>>  
>>  To provide a 3270 device to a guest, create a ``x-terminal3270`` linked to
>>  a ``tn3270`` chardev. The guest will see a 3270 channel device. In order
>> @@ -12,10 +18,14 @@ to actually be able to use it, attach the ``x3270`` 
>> emulator to the chardev.
>>  Example configuration
>>  -
>>  
>> +* Make sure that 3270 support is enabled in the guest's kernel. You need
>> +  ``CONFIG_TN3270`` and at least one of ``CONFIG_TN3270_TTY`` (for 
>> additional
>> +  ttys) or ``CONFIG_TN3270_CONSOLE`` (for a 3270 console).
>> +
>>  * Add a ``tn3270`` chardev and a ``x-terminal3270`` to the QEMU command 
>> line::
>>  
>> --chardev socket,id=char_0,host=0.0.0.0,port=2300,nowait,server,tn3270
>> --device x-terminal3270,chardev=char_0,devno=fe.0.000a,id=terminal_0
>> +   -chardev socket,id=ch0,host=0.0.0.0,port=2300,nowait,server,tn3270
>> +   -device x-terminal3270,chardev=ch0,devno=fe.0.000a,id=terminal0
> 
> Any reason why you changed this?

The lines were too long - Firefox displayed this example with a
horizontal scrollbar, even if I resized the browser window large enough.
With my change, it shows up nicely without scrollbar.

>>  
>>  * Start the guest. In the guest, use ``chccwdev -e 0.0.000a`` to enable
>>the device.
>> @@ -29,4 +39,25 @@ Example configuration
>>  
>>  systemctl start serial-getty@3270-tty1.service
>>  
>> -This should get you an addtional tty for logging into the guest.
>> +  This should get you an addtional tty for logging into the guest.
>> +
>> +* If you want to use the 3270 device as the kernel console instead of an
>> +  additional tty, you can also append ``conmode=3270 condev=000a`` to the
>> +  guest's kernel command line. The kernel then should use the 3270 as
>> +  console after the next boot.
>> +
>> +Restrictions
>> +
>> +
>> +3270 support is still experimental. In particular:
> 
> s/still experimental/very basic/
> 
> I don't think there's much progress on the horizon; let's not give
> people false hope :)
> 
>> +
>> +* Only one 3270 device is supported.
>> +
>> +* It has only been tested with Linux guests and the x3270 emulator.
>> +
>> +* TLS/SSL is not yet supported.
> 
> s/yet //
> 
>> +
>> +* Resizing on reattach is not yet supported.
> 
> s/yet //
> 
>> +
>> +* Multiple commands in one inbound buffer (for example, when the reset key
>> +  is pressed while the network is slow) are not yet supported.
> 
> s/yet //
> 

I'll fix these in v2.

Thanks for the review!

 Thomas

Re: [PATCH] Allow acpi-tmr size=2

[Michael Tokarev]

13.07.2020 10:20, Michael Tokarev пишет:
> 12.07.2020 15:00, Simon John wrote:
>> macos guests no longer boot after commit 
>> 5d971f9e672507210e77d020d89e0e89165c8fc9
>>
>> acpi-tmr needs 2 byte memory accesses, so breaks as that commit only allows 
>> 4 bytes.
>>
>> Fixes: 5d971f9e672507210e7 (memory: Revert "memory: accept mismatching sizes 
>> in memory_region_access_valid")
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1886318
> 
> Actually this fixes 77d58b1e47c8d1c661f98f12b47ab519d3561488
> Author: Gerd Hoffmann 
> Date:   Thu Nov 22 12:12:30 2012 +0100
> Subject: apci: switch timer to memory api
> Signed-off-by: Gerd Hoffmann 
> 
> because this is the commit which put min_access_size = 4 in there
> (5d971f9e672507210e7 is just a messenger, actual error were here
> earlier but it went unnoticed).
> 
> While min_access_size=4 was most likely an error, I wonder why
> we use 1 now, while the subject says it needs 2? What real min
> size is here for ACPI PM timer?

Actually it is more twisted than that. We can't just change the size,
we must update the corresponding code too.


static uint64_t acpi_pm_tmr_read(void *opaque, hwaddr addr, unsigned width)
{
return acpi_pm_tmr_get(opaque);
}

note the actual read function does not even know neither the requested
address nor the requested width, it assumes the min/max constraints
are enforced and the read goes to all 4 bytes. If this pm timer can
be read byte-by-byte, we should return the right byte of the value,
not always the whole value.

/mjt

Re: [PATCH v5 1/4] target/i386: add missing vmx features for several CPU models

[Xiaoyao Li]

On 7/13/2020 3:23 PM, Chenyi Qiang wrote:



On 7/11/2020 12:48 AM, Eduardo Habkost wrote:

On Fri, Jul 10, 2020 at 09:45:49AM +0800, Chenyi Qiang wrote:



On 7/10/2020 6:12 AM, Eduardo Habkost wrote:


I'm very sorry for taking so long to review this.  Question
below:

On Fri, Jun 19, 2020 at 03:31:11PM +0800, Chenyi Qiang wrote:
Add some missing VMX features in Skylake-Server, Cascadelake-Server 
and

Icelake-Server CPU models based on the output of Paolo's script.

Signed-off-by: Chenyi Qiang 


Why are you changing the v1 definition instead adding those new
features in a new version of the CPU model, just like you did in
patch 3/4?



I suppose these missing vmx features are not quite necessary for 
customers.

Just post it here to see if they are worth being added.
Adding a new version is reasonable. Is it appropriate to put all the 
missing

features in patch 1/4, 3/4, 4/4 in a same version?


Yes, it would be OK to add only one new version with all the new
features.



During the coding, I prefer to split the missing vmx features into a new 
version of CPU model, because the vmx features depends on CPUID_EXT_VMX. 
I think It would be better to distinguish it instead of enabling the vmx 
transparently. i.e.

{
 .version = 4,
 .props = (PropValue[]) {
     { "sha-ni", "on" },
     ... ...
     { "model", "106" },
     { /* end of list */ }
 },
},
{
 .version = 5,
 .props = (PropValue[]) {
     { "vmx", "on" }


Chenyi,

This is not we have discussed. I prefer to changing the logic of 
versioned CPU model to not add the features in versioned CPU model to 
env->user_features[]. They're not supposed to be added to 
env->user_features[] since they're not set by user through -feature/+feature


Eduardo,

What do you think?

[PATCH v2] docs/system/s390x: Improve the 3270 documentation

[Thomas Huth]

There is some additional information about the 3270 support in our Wiki
at https://wiki.qemu.org/Features/3270 - so let's include this information
into the main documentation now to have one single source of information
(the Wiki page could later be removed).

While at it, I also shortened the lines of the first example a little bit.
Otherwise they showed up with a horizontal scrollbar in my Firefox browser.

Signed-off-by: Thomas Huth 
---
 v2:
 - Added the changes that have been suggested by Cornelia
 - Talk about "Linux kernel" instead of just saying "kernel", just in case.

 docs/system/s390x/3270.rst | 43 --
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/docs/system/s390x/3270.rst b/docs/system/s390x/3270.rst
index 1774cdcadf..36d1c96bfb 100644
--- a/docs/system/s390x/3270.rst
+++ b/docs/system/s390x/3270.rst
@@ -1,9 +1,15 @@
 3270 devices
 
 
-QEMU supports connecting an external 3270 terminal emulator (such as
-``x3270``) to make a single 3270 device available to a guest. Note that this
-supports basic features only.
+The 3270 is the classic 'green-screen' console of the mainframes (see the
+`IBM 3270 Wikipedia article `__).
+
+The 3270 data stream is not implemented within QEMU; the device only provides
+TN3270 (a telnet extension; see `RFC 854 
`__
+and `RFC 1576 `__) and leaves the heavy
+lifting to an external 3270 terminal emulator (such as ``x3270``) to make a
+single 3270 device available to a guest. Note that this supports basic
+features only.
 
 To provide a 3270 device to a guest, create a ``x-terminal3270`` linked to
 a ``tn3270`` chardev. The guest will see a 3270 channel device. In order
@@ -12,10 +18,14 @@ to actually be able to use it, attach the ``x3270`` 
emulator to the chardev.
 Example configuration
 -
 
+* Make sure that 3270 support is enabled in the guest's Linux kernel. You need
+  ``CONFIG_TN3270`` and at least one of ``CONFIG_TN3270_TTY`` (for additional
+  ttys) or ``CONFIG_TN3270_CONSOLE`` (for a 3270 console).
+
 * Add a ``tn3270`` chardev and a ``x-terminal3270`` to the QEMU command line::
 
--chardev socket,id=char_0,host=0.0.0.0,port=2300,nowait,server,tn3270
--device x-terminal3270,chardev=char_0,devno=fe.0.000a,id=terminal_0
+   -chardev socket,id=ch0,host=0.0.0.0,port=2300,nowait,server,tn3270
+   -device x-terminal3270,chardev=ch0,devno=fe.0.000a,id=terminal0
 
 * Start the guest. In the guest, use ``chccwdev -e 0.0.000a`` to enable
   the device.
@@ -29,4 +39,25 @@ Example configuration
 
 systemctl start serial-getty@3270-tty1.service
 
-This should get you an addtional tty for logging into the guest.
+  This should get you an addtional tty for logging into the guest.
+
+* If you want to use the 3270 device as the Linux kernel console instead of
+  an additional tty, you can also append ``conmode=3270 condev=000a`` to
+  the guest's kernel command line. The kernel then should use the 3270 as
+  console after the next boot.
+
+Restrictions
+
+
+3270 support is very basic. In particular:
+
+* Only one 3270 device is supported.
+
+* It has only been tested with Linux guests and the x3270 emulator.
+
+* TLS/SSL is not supported.
+
+* Resizing on reattach is not supported.
+
+* Multiple commands in one inbound buffer (for example, when the reset key
+  is pressed while the network is slow) are not supported.
-- 
2.18.1

Re: [PATCH v3 0/4] hw/block/nvme: Fix I/O BAR structure

[Klaus Jensen]

On Jun 30 13:04, Philippe Mathieu-Daudé wrote:
> Improvements for the I/O BAR structure:
> - correct pmrmsc register size (Klaus)
> - pack structures
> - align to 4KB
> 
> Since v2:
> - Added Klaus' tags with correct address
> 
> $ git backport-diff -u v2
> 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/4:[] [--] 'hw/block/nvme: Update specification URL'
> 002/4:[] [--] 'hw/block/nvme: Use QEMU_PACKED on hardware/packet 
> structures'
> 003/4:[] [--] 'hw/block/nvme: Fix pmrmsc register size'
> 004/4:[] [--] 'hw/block/nvme: Align I/O BAR to 4 KiB'
> 
> Philippe Mathieu-Daudé (4):
>   hw/block/nvme: Update specification URL
>   hw/block/nvme: Use QEMU_PACKED on hardware/packet structures
>   hw/block/nvme: Fix pmrmsc register size
>   hw/block/nvme: Align I/O BAR to 4 KiB
> 
>  include/block/nvme.h | 42 ++
>  hw/block/nvme.c  |  7 +++
>  2 files changed, 25 insertions(+), 24 deletions(-)
> 
> -- 
> 2.21.3
> 
> 

Thanks Philippe! Applied to nvme-next.

Re: hw/misc/aspeed_scu: 5d971f9e breaks Supermicro AST2400

[Cédric Le Goater]

On 7/1/20 7:17 AM, Erik Smit wrote:
> Hi Joel,
> 
> On Wed, 1 Jul 2020 at 02:54, Joel Stanley  wrote:
>>
>> On Tue, 30 Jun 2020 at 15:32, Erik Smit  wrote:
>>>
>>> Hi Philippe,
>>>
>>> On Tue, 30 Jun 2020 at 17:29, Philippe Mathieu-Daudé  
>>> wrote:

 Hi Erik,

 On 6/30/20 5:11 PM, Erik Smit wrote:
> Hello,
>
> 5d971f9e memory: Revert "memory: accept mismatching sizes in
> memory_region_access_valid" breaks Supermicro AST2400 u-boot.
> Supermicro AST2500 u-boot is fine.
>
> The u-boot tries to make a 2-byte read from address 0x84, but
> aspeed_ast2400_scu_ops has min_access = 4. When I change min_access to
> 2 or revert above commit, u-boot boots again.
>
> Is changing min_access to 2 the right way to fix this?
>>
>> Ryan, do all addresses on the AST2400's APB respond to single byte accesses?
>>

 If you have access to the datasheet and can verify, then yes.
 Else I suppose Cédric, Andrew or Joel can check for you.
>>>
>>> I do not have a datasheet. Aspeed seems quite picky about sharing this
>>> and I'm just a random researcher.
>>
>> Erik, can you point us to the image you're using?
> 
> SMT_X11_158.bin
> 
> You can get it on
> https://www.supermicro.com/en/products/motherboard/X11SSL-F by
> clicking on "BMC/IPMI Firmware" on the right.
> 
>> What is the command line are you testing with?
> 
> qemu-system-arm -machine palmetto-bmc -m 512 -drive
> file=SMT_X11_158.bin,format=raw,if=mtd -nodefaults -nographic -serial
> stdio
> 
> Best regards,
> 
> Erik Smit
> 

Here is a quick fix. But I think we should change valid.min_access_size
for all Aspeed models since the reads can be done with 1/2/4 bytes.
 
aspeed/scu: Fix valid access size on AST2400

The read access size of the SCU registers can be 1/2/4 bytes. Write is
only 4 bytes.

Signed-off-by: Cédric Le Goater 

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 0ccbab7afb60..ca37f935f607 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -327,7 +327,7 @@ static const MemoryRegionOps aspeed_ast2400_scu_ops = {
 .read = aspeed_scu_read,
 .write = aspeed_ast2400_scu_write,
 .endianness = DEVICE_LITTLE_ENDIAN,
-.valid.min_access_size = 4,
+.valid.min_access_size = 1,
 .valid.max_access_size = 4,
 .valid.unaligned = false,
 };



With this patch, the supermicro firmware boots further but there is still 
an issue. It might be the flash definition I used. The machine is detected
as an AST2300 SoC which is weird.

Cheers,

C.



U-Boot 2009.01 ( 4月 02 2019 - 15:29:02) ASPEED (v.0.21) 

I2C:   ready
DRAM:  128 MB
Flash: 32 MB
*** Warning - bad CRC, using default environment

In:serial
Out:   serial
Err:   serial
H/W:   AST2400 series chip
COM:   port1 and port2
PWM:   port[ABCDH]
Hit any key to stop autoboot:  0 
## Booting kernel from Legacy Image at 2140 ...
   Image Name:   2140
   Image Type:   ARM Linux Kernel Image (gzip compressed)
   Data Size:1536645 Bytes =  1.5 MB
   Load Address: 40008000
   Entry Point:  40008000
   Verifying Checksum ... cramfs size 15032320
cramfs size 7315456
OK
   Uncompressing Kernel Image ... OK

Starting kernel ...

Linux version 2.6.28.9 (root@localhost) (gcc version 4.9.1 (crosstool-NG 
1.20.0) ) #1 Wed Nov 20 18:20:38 CST 2019
CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
CPU: VIVT data cache, VIVT instruction cache
Machine: ASPEED-AST2300
Memory policy: ECC disabled, Data cache writeback
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 20066
Kernel command line: console=ttyS1,115200 root=/dev/mtdblock2 rootfstype=cramfs 
noinitrd rw mem=79M
PID hash table entries: 512 (order: 9, 2048 bytes)
Console: colour dummy device 80x30
console [ttyS1] enabled
Dentry cache hash table entries: 16384 (order: 4, 65536 bytes)
Inode-cache hash table entries: 8192 (order: 3, 32768 bytes)
Memory: 79MB = 79MB total
Memory: 76736KB available (2848K code, 358K data, 112K init)
SLUB: Genslabs=12, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
Calibrating delay loop... 835.58 BogoMIPS (lpj=4177920)
Mount-cache hash table entries: 512
CPU: Testing write buffer coherency: ok
net_namespace: 636 bytes
NET: Registered protocol family 16
SCSI subsystem initialized
NET: Registered protocol family 2
IP route cache hash table entries: 1024 (order: 0, 4096 bytes)
TCP established hash table entries: 4096 (order: 3, 32768 bytes)
TCP bind hash table entries: 4096 (order: 2, 16384 bytes)
TCP: Hash tables configured (established 4096 bind 4096)
TCP reno registered
NET: Registered protocol family 1
NetWinder Floating Point Emulator V0.97 (extended precision)
NTFS driver 2.1.29 [Flags: R/W].
JFFS2 version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red Hat, Inc.
fuse init (API version 7.10)
msgmni has been set to 149
alg: No test for stdrng (krng)
io scheduler noop registered
io scheduler anticipatory registered
io scheduler deadline registered
io scheduler cfq regis

Re: [PATCH 2/2] iotests: Set LC_ALL=C for sort

[Max Reitz]

On 11.07.20 10:57, Alex Bennée wrote:
> 
> Max Reitz  writes:
> 
>> Otherwise the result is basically unpredictable.
>>
>> (Note that the precise environment variable to control sorting order is
>> LC_COLLATE, but LC_ALL overrides LC_COLLATE, and we do not want the
>> sorting order to be messed up if LC_ALL is set in the environment.)
>>
>> Reported-by: John Snow 
>> Signed-off-by: Max Reitz 
> 
> Queued to pr/100720-testing-and-misc-2, thanks.
> 
> I've skipped patch 1/2 for now as I have an alternative fix but we can
> switch it back if you prefer?

I see you’ve sent your pull request already, so I’ll see whether I’ll
include 1/2 in some block pull request.  Maybe, maybe not.

Thanks for queuing 2/2, anyway. :)

Max



signature.asc
Description: OpenPGP digital signature

Re: hw/misc/aspeed_scu: 5d971f9e breaks Supermicro AST2400

[Erik Smit]

Hi Cédric,

On Mon, 13 Jul 2020 at 09:52, Cédric Le Goater  wrote:
>
> With this patch, the supermicro firmware boots further but there is still
> an issue. It might be the flash definition I used. The machine is detected
> as an AST2300 SoC which is weird.

> BMC flash ID:0x19ba20
> Unable to handle kernel NULL pointer dereference at virtual address 

The firmware is expecting the flash ID to repeat. The following makes it boot.
Not sure if this is the right way to go.

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8227088441..5000930800 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1041,7 +1041,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 s->data[i] = s->pi->id[i];
 }
 for (; i < SPI_NOR_MAX_ID_LEN; i++) {
-s->data[i] = 0;
+s->data[i] = s->pi->id[i % s->pi->id_len];
 }

 s->len = SPI_NOR_MAX_ID_LEN;

-- 
Best Regards,

Erik Smit

Re: cve patch wanted

[Philippe Mathieu-Daudé]

Hi,

On 7/11/20 2:28 PM, 林奕帆 wrote:
> Hello
>    I am a student from Fudan University in China. I am doing research on
> CVE patch recently. But i can not find the PATCH COMMIT of
> CVE-2019-12247 cve-2019-12155 cve-2019-6778.Can you give me the commit
> fix this cve?

* CVE-2019-12247

I don't know about this one, maybe related to CVE-2018-12617 fixed
by commit 1329651fb4 ("qga: Restrict guest-file-read count to 48 MB")
Cc'ing Michael for CVE-2019-12247.

* CVE-2019-12155

I don't have access to the information (still marked 'private'
one year after), but I *guess* it has been fixed by commit
d52680fc93 ("qxl: check release info object").
Cc'ing Gerd and Prasad.

* CVE-2019-6778

This one is in SLiRP, Cc'ing Samuel and Marc-André.

Re: [PATCH 1/1] MAINTAINERS: Add Python library stanza

[Philippe Mathieu-Daudé]

On 7/10/20 11:57 PM, John Snow wrote:
> I'm proposing that I split the actual Python library off from the other
> miscellaneous python scripts we have and declare it maintained. Add
> myself as a maintainer of this folder, along with Cleber.
> 
> Signed-off-by: John Snow 
> ---
>  MAINTAINERS | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6aa54f7f8f..fe1dcd5a76 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2280,11 +2280,18 @@ S: Maintained
>  F: include/sysemu/cryptodev*.h
>  F: backends/cryptodev*.c
>  
> +Python library
> +M: John Snow 
> +M: Cleber Rosa 
> +R: Eduardo Habkost 
> +S: Maintained
> +F: python/*
> +T: git https://gitlab.com/jsnow/qemu.git python
> +
>  Python scripts
>  M: Eduardo Habkost 
>  M: Cleber Rosa 
>  S: Odd fixes
> -F: python/qemu/*py
>  F: scripts/*.py
>  F: tests/*.py
>  
> 

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

Re: [PATCH] .cirrus.yml: add bash to the brew packages

[Philippe Mathieu-Daudé]

On 7/10/20 8:22 PM, Alex Bennée wrote:
> Like the sed we include earlier we want something more recent for
> iotests to work.
> 
> Fixes: 57ee95ed
> Cc: Max Reitz 
> Signed-off-by: Alex Bennée 
> ---
>  .cirrus.yml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 69342ae031bc..f287d23c5b9b 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -20,7 +20,7 @@ macos_task:
>osx_instance:
>  image: mojave-base
>install_script:
> -- brew install pkg-config python gnu-sed glib pixman make sdl2
> +- brew install pkg-config python gnu-sed glib pixman make sdl2 bash
>script:
>  - mkdir build
>  - cd build
> @@ -33,7 +33,7 @@ macos_xcode_task:
>  # this is an alias for the latest Xcode
>  image: mojave-xcode
>install_script:
> -- brew install pkg-config gnu-sed glib pixman make sdl2
> +- brew install pkg-config gnu-sed glib pixman make sdl2 bash
>script:
>  - mkdir build
>  - cd build
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Vladimir Sementsov-Ogievskiy]

13.07.2020 10:07, Andrey Shinkevich wrote:

On 11.07.2020 22:11, Vladimir Sementsov-Ogievskiy wrote:

03.07.2020 16:13, Andrey Shinkevich wrote:

Read and dump entries from the bitmap directory of QCOW2 image.
It extends the output in the test case #291.



...

  diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index d8c058d..7c0dc9a 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -132,6 +132,50 @@ class Qcow2BitmapExt(Qcow2Struct):
    def __init__(self, fd):
  super().__init__(fd=fd)
+    self.read_bitmap_directory(fd)
+
+    def read_bitmap_directory(self, fd):
+    fd.seek(self.bitmap_directory_offset)
+    self.bitmap_directory = \
+    [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]


sounds good. I think, we should restore fd position after reading 
bitmap_directory, to point at the end of extension, to not break further 
extensions loading



Yes, it is done in the constructor of QcowHeaderExtension:

if self.magic == QCOW2_EXT_MAGIC_BITMAPS:

...

position = fd.tell()

...

self.obj = Qcow2BitmapExt(fd=fd)

fd.seek(position)



I don't like it. If you want caller to care about fd, caller should know size 
of created child. But passing fd to constructor implies that caller not aware 
of size of new created structure. So I think good api is: constuctor starts to 
read the structure and left after this structure on exit from consturctor (so, 
caller may read following structures). Constructor may read some nested 
structures, but is responsible for restoring fd after it.


--
Best regards,
Vladimir

[PULL 2/8] chardev: don't abort on attempt to add duplicated chardev

[Marc-André Lureau]

This is a regression from commit d2623129a7d ("qom: Drop parameter @errp
of object_property_add() & friends").

(qemu) chardev-add id=null,backend=null
(qemu) chardev-add id=null,backend=null
Unexpected error in object_property_try_add() at 
/home/elmarco/src/qemu/qom/object.c:1166:
attempt to add duplicate property 'null' to object (type 'container')

That case is currently not covered in the test suite, but will be with
the queued patch "char: fix use-after-free with dup chardev &
reconnect".

Fixes: d2623129a7dec1d3041ad1221dda1ca49c667532
Signed-off-by: Marc-André Lureau 
Reviewed-by: Markus Armbruster 
---
 chardev/char.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/chardev/char.c b/chardev/char.c
index e5b43cb4b87..a0626d04d50 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -996,7 +996,11 @@ static Chardev *chardev_new(const char *id, const char 
*typename,
 }
 
 if (id) {
-object_property_add_child(get_chardevs_root(), id, obj);
+object_property_try_add_child(get_chardevs_root(), id, obj,
+  &local_err);
+if (local_err) {
+goto end;
+}
 object_unref(obj);
 }
 
-- 
2.27.0.221.ga08a83db2b

[PULL 0/8] Chardev patches

[Marc-André Lureau]

The following changes since commit d34498309cff7560ac90c422c56e3137e6a64b19:

  Merge remote-tracking branch 'remotes/philmd-gitlab/tags/avr-port-20200711' 
into staging (2020-07-11 19:27:59 +0100)

are available in the Git repository at:

  https://github.com/elmarco/qemu.git tags/chardev-pull-request

for you to fetch changes up to 30827bad3852fd85d86995e7ccab429679442889:

  chardev: Extract system emulation specific code (2020-07-13 11:59:47 +0400)





Li Feng (1):
  char-socket: initialize reconnect timer only when the timer doesn't
start

Marc-André Lureau (2):
  chardev: don't abort on attempt to add duplicated chardev
  char: fix use-after-free with dup chardev & reconnect

Philippe Mathieu-Daudé (5):
  monitor/misc: Remove unused "chardev/char-mux.h" include
  tests/test-char: Remove unused "chardev/char-mux.h" include
  chardev: Restrict msmouse / wctablet / testdev to system emulation
  chardev: Reduce "char-mux.h" scope, rename it "chardev-internal.h"
  chardev: Extract system emulation specific code

 .../char-mux.h => chardev/chardev-internal.h  |  10 +-
 chardev/char-fe.c |   2 +-
 chardev/char-mux.c|   2 +-
 chardev/char-socket.c |   5 +-
 chardev/char.c|  43 +-
 chardev/chardev-sysemu.c  |  69 ++
 monitor/misc.c|   1 -
 tests/test-char.c | 122 +++---
 chardev/Makefile.objs |   3 +-
 9 files changed, 195 insertions(+), 62 deletions(-)
 rename include/chardev/char-mux.h => chardev/chardev-internal.h (93%)
 create mode 100644 chardev/chardev-sysemu.c

-- 
2.27.0.221.ga08a83db2b

[PULL 1/8] char-socket: initialize reconnect timer only when the timer doesn't start

[Marc-André Lureau]

From: Li Feng 

When the disconnect event is triggered in the connecting stage,
the tcp_chr_disconnect_locked may be called twice.

The first call:
#0  qemu_chr_socket_restart_timer (chr=0x5582ee90) at 
chardev/char-socket.c:120
#1  0x5558e38c in tcp_chr_disconnect_locked (chr=) 
at chardev/char-socket.c:490
#2  0x5558e3cd in tcp_chr_disconnect (chr=0x5582ee90) at 
chardev/char-socket.c:497
#3  0x5558ea32 in tcp_chr_new_client (chr=chr@entry=0x5582ee90, 
sioc=sioc@entry=0x5582f0b0) at chardev/char-socket.c:892
#4  0x5558eeb8 in qemu_chr_socket_connected (task=0x5582f300, 
opaque=) at chardev/char-socket.c:1090
#5  0x55574352 in qio_task_complete 
(task=task@entry=0x5582f300) at io/task.c:196
#6  0x555745f4 in qio_task_thread_result (opaque=0x5582f300) at 
io/task.c:111
#7  qio_task_wait_thread (task=0x5582f300) at io/task.c:190
#8  0x5558f17e in tcp_chr_wait_connected (chr=0x5582ee90, 
errp=0x55802a08 ) at chardev/char-socket.c:1013
#9  0x55567cbd in char_socket_client_reconnect_test 
(opaque=0x557fe020 ) at tests/test-char.c:1152
The second call:
#0  0x75ac3277 in raise () from /lib64/libc.so.6
#1  0x75ac4968 in abort () from /lib64/libc.so.6
#2  0x75abc096 in __assert_fail_base () from /lib64/libc.so.6
#3  0x75abc142 in __assert_fail () from /lib64/libc.so.6
#4  0x5558d10a in qemu_chr_socket_restart_timer 
(chr=0x5582ee90) at chardev/char-socket.c:125
#5  0x5558df0c in tcp_chr_disconnect_locked (chr=) 
at chardev/char-socket.c:490
#6  0x5558df4d in tcp_chr_disconnect (chr=0x5582ee90) at 
chardev/char-socket.c:497
#7  0x5558e5b2 in tcp_chr_new_client (chr=chr@entry=0x5582ee90, 
sioc=sioc@entry=0x5582f0b0) at chardev/char-socket.c:892
#8  0x5558e93a in tcp_chr_connect_client_sync 
(chr=chr@entry=0x5582ee90, errp=errp@entry=0x7fffd178) at 
chardev/char-socket.c:944
#9  0x5558ec78 in tcp_chr_wait_connected (chr=0x5582ee90, 
errp=0x55802a08 ) at chardev/char-socket.c:1035
#10 0x5556804b in char_socket_client_test (opaque=0x557fe020 
) at tests/test-char.c:1023

Run test/test-char to reproduce this issue.

test-char: chardev/char-socket.c:125: qemu_chr_socket_restart_timer: Assertion 
`!s->reconnect_timer' failed.

Signed-off-by: Li Feng 
Acked-by: Marc-André Lureau 
Message-Id: <20200522025554.41063-1-fen...@smartx.com>
---
 chardev/char-socket.c |  2 +-
 tests/test-char.c | 73 +--
 2 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 5758d9900fc..320aa7c642f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -490,7 +490,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
 if (emit_close) {
 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
-if (s->reconnect_time) {
+if (s->reconnect_time && !s->reconnect_timer) {
 qemu_chr_socket_restart_timer(chr);
 }
 }
diff --git a/tests/test-char.c b/tests/test-char.c
index 3afc9b1b8d5..73ba1cf6010 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -625,12 +625,14 @@ static void char_udp_test(void)
 typedef struct {
 int event;
 bool got_pong;
+CharBackend *be;
 } CharSocketTestData;
 
 
 #define SOCKET_PING "Hello"
 #define SOCKET_PONG "World"
 
+typedef void (*char_socket_cb)(void *opaque, QEMUChrEvent event);
 
 static void
 char_socket_event(void *opaque, QEMUChrEvent event)
@@ -639,6 +641,27 @@ char_socket_event(void *opaque, QEMUChrEvent event)
 data->event = event;
 }
 
+static void
+char_socket_event_with_error(void *opaque, QEMUChrEvent event)
+{
+static bool first_error;
+CharSocketTestData *data = opaque;
+CharBackend *be = data->be;
+data->event = event;
+switch (event) {
+case CHR_EVENT_OPENED:
+if (!first_error) {
+first_error = true;
+qemu_chr_fe_disconnect(be);
+}
+return;
+case CHR_EVENT_CLOSED:
+return;
+default:
+return;
+}
+}
+
 
 static void
 char_socket_read(void *opaque, const uint8_t *buf, int size)
@@ -699,19 +722,24 @@ char_socket_addr_to_opt_str(SocketAddress *addr, bool 
fd_pass,
 }
 
 
-static void
-char_socket_ping_pong(QIOChannel *ioc)
+static int
+char_socket_ping_pong(QIOChannel *ioc, Error **errp)
 {
 char greeting[sizeof(SOCKET_PING)];
 const char *response = SOCKET_PONG;
 
-qio_channel_read_all(ioc, greeting, sizeof(greeting), &error_abort);
+int ret;
+ret = qio_channel_read_all(ioc, greeting, sizeof(greeting), errp);
+if (ret != 0) {
+object_unref(OBJECT(ioc));
+return -1;
+}
 
 g_assert(memcmp(greeting, SOCKET_PING, sizeof(greeting)) == 0);
 
-qio_channel_write_all(ioc, response, sizeof(SOCKET_PO

[PULL 3/8] char: fix use-after-free with dup chardev & reconnect

[Marc-André Lureau]

With a reconnect socket, qemu_char_open() will start a background
thread. It should keep a reference on the chardev.

Fixes invalid read:
READ of size 8 at 0x604ac858 thread T7
#0 0x598d37b8 in unix_connect_saddr 
/home/elmarco/src/qq/util/qemu-sockets.c:954
#1 0x598d4751 in socket_connect 
/home/elmarco/src/qq/util/qemu-sockets.c:1109
#2 0x59707c34 in qio_channel_socket_connect_sync 
/home/elmarco/src/qq/io/channel-socket.c:145
#3 0x596adebb in tcp_chr_connect_client_task 
/home/elmarco/src/qq/chardev/char-socket.c:1104
#4 0x59723d55 in qio_task_thread_worker 
/home/elmarco/src/qq/io/task.c:123
#5 0x598a6731 in qemu_thread_start 
/home/elmarco/src/qq/util/qemu-thread-posix.c:519
#6 0x740d4431 in start_thread (/lib64/libpthread.so.0+0x9431)
#7 0x740029d2 in __clone (/lib64/libc.so.6+0x1019d2)

Signed-off-by: Marc-André Lureau 
Reviewed-by: Daniel P. Berrangé 
Message-Id: <20200420112012.567284-1-marcandre.lur...@redhat.com>
---
 chardev/char-socket.c |  3 ++-
 tests/test-char.c | 54 +--
 2 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 320aa7c642f..ef62dbf3d73 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1129,7 +1129,8 @@ static void tcp_chr_connect_client_async(Chardev *chr)
  */
 s->connect_task = qio_task_new(OBJECT(sioc),
qemu_chr_socket_connected,
-   chr, NULL);
+   object_ref(OBJECT(chr)),
+   (GDestroyNotify)object_unref);
 qio_task_run_in_thread(s->connect_task,
tcp_chr_connect_client_task,
s->addr,
diff --git a/tests/test-char.c b/tests/test-char.c
index 73ba1cf6010..9d8746414d7 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -904,6 +904,52 @@ typedef struct {
 char_socket_cb event_cb;
 } CharSocketClientTestConfig;
 
+static void char_socket_client_dupid_test(gconstpointer opaque)
+{
+const CharSocketClientTestConfig *config = opaque;
+QIOChannelSocket *ioc;
+char *optstr;
+Chardev *chr1, *chr2;
+SocketAddress *addr;
+QemuOpts *opts;
+Error *local_err = NULL;
+
+/*
+ * Setup a listener socket and determine get its address
+ * so we know the TCP port for the client later
+ */
+ioc = qio_channel_socket_new();
+g_assert_nonnull(ioc);
+qio_channel_socket_listen_sync(ioc, config->addr, 1, &error_abort);
+addr = qio_channel_socket_get_local_address(ioc, &error_abort);
+g_assert_nonnull(addr);
+
+/*
+ * Populate the chardev address based on what the server
+ * is actually listening on
+ */
+optstr = char_socket_addr_to_opt_str(addr,
+ config->fd_pass,
+ config->reconnect,
+ false);
+
+opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"),
+   optstr, true);
+g_assert_nonnull(opts);
+chr1 = qemu_chr_new_from_opts(opts, NULL, &error_abort);
+g_assert_nonnull(chr1);
+
+chr2 = qemu_chr_new_from_opts(opts, NULL, &local_err);
+g_assert_null(chr2);
+error_free_or_abort(&local_err);
+
+object_unref(OBJECT(ioc));
+qemu_opts_del(opts);
+object_unparent(OBJECT(chr1));
+qapi_free_SocketAddress(addr);
+g_free(optstr);
+}
+
 static void char_socket_client_test(gconstpointer opaque)
 {
 const CharSocketClientTestConfig *config = opaque;
@@ -1456,7 +1502,7 @@ int main(int argc, char **argv)
 
 #define SOCKET_CLIENT_TEST(name, addr)  \
 static CharSocketClientTestConfig client1 ## name = \
-{ addr, NULL, false, false, char_socket_event}; \
+{ addr, NULL, false, false, char_socket_event };\
 static CharSocketClientTestConfig client2 ## name = \
 { addr, NULL, true, false, char_socket_event }; \
 static CharSocketClientTestConfig client3 ## name = \
@@ -1470,6 +1516,8 @@ int main(int argc, char **argv)
 static CharSocketClientTestConfig client7 ## name = \
 { addr, ",reconnect=1", true, false,\
 char_socket_event_with_error }; \
+static CharSocketClientTestConfig client8 ## name = \
+{ addr, ",reconnect=1", false, false, char_socket_event };  \
 g_test_add_data_func("/char/socket/client/mainloop/" # name,\
  &client1 ##name, char_socket_client_test); \
 g_test_add_data_func("/char/socket/client/wait-conn/" # name,   \
@@ -1483,7 +1531,9 @@ int main(int argc, char **argv)
 g_test_add_data_fun

[PULL 5/8] tests/test-char: Remove unused "chardev/char-mux.h" include

[Marc-André Lureau]

From: Philippe Mathieu-Daudé 

This test never required "chardev/char-mux.h", remove it.

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200423202112.644-3-phi...@redhat.com>
Reviewed-by: Richard Henderson 
Signed-off-by: Marc-André Lureau 
---
 tests/test-char.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/test-char.c b/tests/test-char.c
index 9d8746414d7..614bdac2df8 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -6,7 +6,6 @@
 #include "qemu/option.h"
 #include "qemu/sockets.h"
 #include "chardev/char-fe.h"
-#include "chardev/char-mux.h"
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-char.h"
-- 
2.27.0.221.ga08a83db2b

[PULL 6/8] chardev: Restrict msmouse / wctablet / testdev to system emulation

[Marc-André Lureau]

From: Philippe Mathieu-Daudé 

The msmouse / wctablet / testdev character devices are only
used by system emulation. Remove them from user mode and tools.

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200423202112.644-4-phi...@redhat.com>
Reviewed-by: Richard Henderson 
Signed-off-by: Marc-André Lureau 
---
 chardev/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
index 3a58c9d329d..62ec0a33235 100644
--- a/chardev/Makefile.objs
+++ b/chardev/Makefile.objs
@@ -17,7 +17,7 @@ chardev-obj-y += char-udp.o
 chardev-obj-$(CONFIG_WIN32) += char-win.o
 chardev-obj-$(CONFIG_WIN32) += char-win-stdio.o
 
-common-obj-y += msmouse.o wctablet.o testdev.o
+common-obj-$(CONFIG_SOFTMMU) += msmouse.o wctablet.o testdev.o
 
 ifeq ($(CONFIG_BRLAPI),y)
 common-obj-m += baum.o
-- 
2.27.0.221.ga08a83db2b

[PULL 4/8] monitor/misc: Remove unused "chardev/char-mux.h" include

[Marc-André Lureau]

From: Philippe Mathieu-Daudé 

monitor/misc.c never required "chardev/char-mux.h", remove it.

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200423202112.644-2-phi...@redhat.com>
Reviewed-by: Richard Henderson 
Signed-off-by: Marc-André Lureau 
---
 monitor/misc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/monitor/misc.c b/monitor/misc.c
index 89bb970b004..e847b58a8c9 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -33,7 +33,6 @@
 #include "exec/gdbstub.h"
 #include "net/net.h"
 #include "net/slirp.h"
-#include "chardev/char-mux.h"
 #include "ui/qemu-spice.h"
 #include "qemu/config-file.h"
 #include "qemu/ctype.h"
-- 
2.27.0.221.ga08a83db2b

[PULL 8/8] chardev: Extract system emulation specific code

[Marc-André Lureau]

From: Philippe Mathieu-Daudé 

Split out code only used during system emulation,
to reduce code pulled in user emulation and tools.

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200423202112.644-6-phi...@redhat.com>
Reviewed-by: Richard Henderson 
Signed-off-by: Marc-André Lureau 
---
 chardev/chardev-internal.h |  3 ++
 chardev/char.c | 35 +--
 chardev/chardev-sysemu.c   | 69 ++
 chardev/Makefile.objs  |  1 +
 4 files changed, 74 insertions(+), 34 deletions(-)
 create mode 100644 chardev/chardev-sysemu.c

diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index e0264ac3498..f4d0429763b 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -26,6 +26,7 @@
 
 #include "chardev/char.h"
 #include "chardev/char-fe.h"
+#include "qom/object.h"
 
 #define MAX_MUX 4
 #define MUX_BUFFER_SIZE 32 /* Must be a power of 2.  */
@@ -59,4 +60,6 @@ typedef struct MuxChardev {
 void mux_set_focus(Chardev *chr, int focus);
 void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event);
 
+Object *get_chardevs_root(void);
+
 #endif /* CHAR_MUX_H */
diff --git a/chardev/char.c b/chardev/char.c
index 807be52300e..77e7ec814f2 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -45,7 +45,7 @@
 /***/
 /* character device */
 
-static Object *get_chardevs_root(void)
+Object *get_chardevs_root(void)
 {
 return container_get(object_get_root(), "/chardevs");
 }
@@ -305,33 +305,6 @@ static const TypeInfo char_type_info = {
 .class_init = char_class_init,
 };
 
-static int chardev_machine_done_notify_one(Object *child, void *opaque)
-{
-Chardev *chr = (Chardev *)child;
-ChardevClass *class = CHARDEV_GET_CLASS(chr);
-
-if (class->chr_machine_done) {
-return class->chr_machine_done(chr);
-}
-
-return 0;
-}
-
-static void chardev_machine_done_hook(Notifier *notifier, void *unused)
-{
-int ret = object_child_foreach(get_chardevs_root(),
-   chardev_machine_done_notify_one, NULL);
-
-if (ret) {
-error_report("Failed to call chardev machine_done hooks");
-exit(1);
-}
-}
-
-static Notifier chardev_machine_done_notify = {
-.notify = chardev_machine_done_hook,
-};
-
 static bool qemu_chr_is_busy(Chardev *s)
 {
 if (CHARDEV_IS_MUX(s)) {
@@ -1198,12 +1171,6 @@ void qemu_chr_cleanup(void)
 static void register_types(void)
 {
 type_register_static(&char_type_info);
-
-/* this must be done after machine init, since we register FEs with muxes
- * as part of realize functions like serial_isa_realizefn when -nographic
- * is specified
- */
-qemu_add_machine_init_done_notifier(&chardev_machine_done_notify);
 }
 
 type_init(register_types);
diff --git a/chardev/chardev-sysemu.c b/chardev/chardev-sysemu.c
new file mode 100644
index 000..eecdc615ee1
--- /dev/null
+++ b/chardev/chardev-sysemu.c
@@ -0,0 +1,69 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * 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 "sysemu/sysemu.h"
+#include "chardev/char.h"
+#include "qemu/error-report.h"
+#include "chardev-internal.h"
+
+static int chardev_machine_done_notify_one(Object *child, void *opaque)
+{
+Chardev *chr = (Chardev *)child;
+ChardevClass *class = CHARDEV_GET_CLASS(chr);
+
+if (class->chr_machine_done) {
+return class->chr_machine_done(chr);
+}
+
+return 0;
+}
+
+static void chardev_machine_done_hook(Notifier *notifier, void *unused)
+{
+int ret = object_child_foreach(get_chardevs_root(),
+   chardev_machine_done_notify_one, NULL);
+
+if (ret) {
+error_report("Failed to call chardev machine_done hooks");
+exit(1);
+}
+}
+
+
+static Notifier chardev_machine_done_notify = {
+

[PULL 7/8] chardev: Reduce "char-mux.h" scope, rename it "chardev-internal.h"

[Marc-André Lureau]

From: Philippe Mathieu-Daudé 

No file out of chardev/ requires access to this header,
restrict its scope.

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200423202112.644-5-phi...@redhat.com>
Reviewed-by: Richard Henderson 
Signed-off-by: Marc-André Lureau 
---
 include/chardev/char-mux.h => chardev/chardev-internal.h | 7 ---
 chardev/char-fe.c| 2 +-
 chardev/char-mux.c   | 2 +-
 chardev/char.c   | 2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)
 rename include/chardev/char-mux.h => chardev/chardev-internal.h (96%)

diff --git a/include/chardev/char-mux.h b/chardev/chardev-internal.h
similarity index 96%
rename from include/chardev/char-mux.h
rename to chardev/chardev-internal.h
index 417fe32eedf..e0264ac3498 100644
--- a/include/chardev/char-mux.h
+++ b/chardev/chardev-internal.h
@@ -1,5 +1,5 @@
 /*
- * QEMU System Emulator
+ * QEMU Character device internals
  *
  * Copyright (c) 2003-2008 Fabrice Bellard
  *
@@ -21,8 +21,8 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-#ifndef CHAR_MUX_H
-#define CHAR_MUX_H
+#ifndef CHARDEV_INTERNAL_H
+#define CHARDEV_INTERNAL_H
 
 #include "chardev/char.h"
 #include "chardev/char-fe.h"
@@ -30,6 +30,7 @@
 #define MAX_MUX 4
 #define MUX_BUFFER_SIZE 32 /* Must be a power of 2.  */
 #define MUX_BUFFER_MASK (MUX_BUFFER_SIZE - 1)
+
 typedef struct MuxChardev {
 Chardev parent;
 CharBackend *backends[MAX_MUX];
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index f3530a90e63..474715c5a92 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -29,7 +29,7 @@
 
 #include "chardev/char-fe.h"
 #include "chardev/char-io.h"
-#include "chardev/char-mux.h"
+#include "chardev-internal.h"
 
 int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
 {
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 46c44af67c4..6f980bb8364 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -29,7 +29,7 @@
 #include "chardev/char.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/sysemu.h"
-#include "chardev/char-mux.h"
+#include "chardev-internal.h"
 
 /* MUX driver for serial I/O splitting */
 
diff --git a/chardev/char.c b/chardev/char.c
index a0626d04d50..807be52300e 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -40,7 +40,7 @@
 #include "qemu/id.h"
 #include "qemu/coroutine.h"
 
-#include "chardev/char-mux.h"
+#include "chardev-internal.h"
 
 /***/
 /* character device */
-- 
2.27.0.221.ga08a83db2b

Re: cve patch wanted

[Marc-André Lureau]

Hi

On Mon, Jul 13, 2020 at 12:16 PM Philippe Mathieu-Daudé
 wrote:
> * CVE-2019-6778
>
> This one is in SLiRP, Cc'ing Samuel and Marc-André.

I was about to send a patch to update slirp to 4.3.1. Note that this
particular CVE should be fixed since 4.1, where "emu" support is
disabled.

Re: hw/misc/aspeed_scu: 5d971f9e breaks Supermicro AST2400

[Philippe Mathieu-Daudé]

On 7/13/20 10:06 AM, Erik Smit wrote:
> Hi Cédric,
> 
> On Mon, 13 Jul 2020 at 09:52, Cédric Le Goater  wrote:
>>
>> With this patch, the supermicro firmware boots further but there is still
>> an issue. It might be the flash definition I used. The machine is detected
>> as an AST2300 SoC which is weird.
> 
>> BMC flash ID:0x19ba20
>> Unable to handle kernel NULL pointer dereference at virtual address 
> 
> The firmware is expecting the flash ID to repeat.

At what address?

> The following makes it boot.
> Not sure if this is the right way to go.
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8227088441..5000930800 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1041,7 +1041,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  s->data[i] = s->pi->id[i];
>  }
>  for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> -s->data[i] = 0;
> +s->data[i] = s->pi->id[i % s->pi->id_len];

Interesting. It makes sense to read the ID repeated (not sure
your patch is the safest way to do it).

I can't find the time to add a generic NOR flash device :/

>  }
> 
>  s->len = SPI_NOR_MAX_ID_LEN;
> 

[PATCH] slirp: update to v4.3.1

[Marc-André Lureau]

Switch from stable-4.2 branch back to master (which is actually
maintained, I think we tend to forget about stable...).

git shortlog 2faae0f7..a62d3673:

5eraph (2):
  disable_dns option
  limit vnameserver_addr to port 53

Akihiro Suda (1):
  libslirp.h: fix SlirpConfig v3 documentation

Jindrich Novy (4):
  Fix possible infinite loops and use-after-free
  Use secure string copy to avoid overflow
  Be sure to initialize sockaddr structure
  Check lseek() for failure

Marc-André Lureau (12):
  Merge branch 'master' into 'master'
  Merge branch 'fix-slirpconfig-3-doc' into 'master'
  Fix use-afte-free in ip_reass() (CVE-2020-1983)
  Update CHANGELOG
  Merge branch 'cve-2020-1983' into 'master'
  Release v4.3.0
  Merge branch 'release-v4.3.0' into 'master'
  changelog: post-release
  util: do not silently truncate
  Merge branch 'slirp-fmt-truncate' into 'master'
  Release v4.3.1
  Merge branch 'release-v4.3.1' into 'master'

Philippe Mathieu-Daudé (3):
  Fix win32 builds by using the SLIRP_PACKED definition
  Fix constness warnings
  Remove unnecessary break

Ralf Haferkamp (2):
  Drop bogus IPv6 messages
  Fix MTU check

Samuel Thibault (1):
  Merge branch 'ip6_payload_len' into 'master'

Signed-off-by: Marc-André Lureau 
---
 slirp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp b/slirp
index 2faae0f778f..a62d36734ff 16
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit 2faae0f778f818fadc873308f983289df697eb93
+Subproject commit a62d36734ffe9828d0f70df1b3898a3b4fbda755
-- 
2.27.0.221.ga08a83db2b

Re: [PATCH v8 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method

[Vladimir Sementsov-Ogievskiy]

13.07.2020 07:49, Andrey Shinkevich wrote:

On 11.07.2020 19:34, Vladimir Sementsov-Ogievskiy wrote:

03.07.2020 16:13, Andrey Shinkevich wrote:

There are two ways to initialize a class derived from Qcow2Struct:
1. Pass a block of binary data to the constructor.
2. Pass the file descriptor to allow reading the file from constructor.
Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
support a scattered reading in the initialization chain.
The implementation comes with the patch that follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 14 --
  1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2f3681b..1435e34 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -63,7 +63,8 @@ class Qcow2StructMeta(type):
    class Qcow2Struct(metaclass=Qcow2StructMeta):
  -    """Qcow2Struct: base class for qcow2 data structures
+    """
+    Qcow2Struct: base class for qcow2 data structures


Unrelated chunk. And why?



To conform to the common style for comments in the file as it is at

class QcowHeaderExtension::__init__()


It's OK to additionally fix/change style in the chunk where you change some 
logic anyway. But changing style in another places - I think it doesn't worth 
it. Remember that it may make patch porting more complicated for nothing.

And this is not only my position, https://wiki.qemu.org/Contribute/SubmitAPatch directly 
statates it in "Don't include irrelevant changes" paragraph:

In particular, don't include formatting, coding style or whitespace changes 
to bits of code that would otherwise not be touched by the patch.







    Successors should define fields class variable, which is: list of 
tuples,
  each of three elements:
@@ -113,6 +114,9 @@ class Qcow2BitmapExt(Qcow2Struct):
  ('u64', '{:#x}', 'bitmap_directory_offset')
  )
  +    def __init__(self, fd):
+    super().__init__(fd=fd)


this does nothing. We inherit the __init__ of super class, no need to define it 
just to call same __init__.


+
    QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
  @@ -173,7 +177,13 @@ class QcowHeaderExtension(Qcow2Struct):
  self.data_str = data_str
    if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
-    self.obj = Qcow2BitmapExt(data=self.data)
+    assert fd is not None
+    position = fd.tell()
+    # Step back to reread data


This definitely shows that we are doing something wrong



For Qcow2BitmapExt, we need both fd and data and they are mutualy exclusive

in the constructor of the class Qcow2Struct. Rereading the bitmap extension

is a solution without changing the Qcow2Struct. Any other suggestion?



I think, we want to modify QcowHeaderExtension so that it creates/reads its 
children appropriately. Probably, for bitmaps extension exclusively we pass fd 
to Qcow2BitmapExt, not reading data by ourselves, and for other extensions keep 
current semantics of just reading data by hand (or you may add a simple class 
Qcow2Ext.







+    padded = (self.length + 7) & ~7
+    fd.seek(-padded, 1)
+    self.obj = Qcow2BitmapExt(fd=fd)
+    fd.seek(position)
  else:
  self.obj = None







--
Best regards,
Vladimir

Re: [PULL v2 00/50] testing updates (vm, gitlab, misc build fixes)

[Peter Maydell]

On Sat, 11 Jul 2020 at 18:07, Alex Bennée  wrote:
>
> Fixed a few, dropped a few, added a few
>
> ---
>
> The following changes since commit 827937158b72ce2265841ff528bba3c44a1bfbc8:
>
>   Merge remote-tracking branch 'remotes/aperard/tags/pull-xen-20200710' into 
> staging (2020-07-11 13:56:03 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/stsquad/qemu.git tags/pull-testing-and-misc-110720-2
>
> for you to fetch changes up to 4a40f561d5ebb5050a8c6dcbdcee85621056590a:
>
>   iotests: Set LC_ALL=C for sort (2020-07-11 15:53:29 +0100)
>
> 
> Testing and misc build updates:
>
>   - tests/vm support for aarch64 VMs
>   - tests/tcg better cross-compiler detection
>   - update docker tooling to support registries
>   - update docker support for xtensa
>   - gitlab build docker images and store in registry
>   - gitlab use docker images for builds
>   - a number of skipIf updates to support move
>   - linux-user MAP_FIXED_NOREPLACE fix
>   - qht-bench compiler tweaks
>   - configure fix for secret keyring
>   - tsan fiber annotation clean-up
>   - doc updates for mttcg/icount/gdbstub
>   - fix cirrus to use brew bash for iotests
>   - revert virtio-gpu breakage
>   - fix LC_ALL to avoid sorting changes in iotests


Applied, thanks.

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

-- PMM

Re: hw/misc/aspeed_scu: 5d971f9e breaks Supermicro AST2400

[Cédric Le Goater]

Hello,

On 7/13/20 10:06 AM, Erik Smit wrote:
> Hi Cédric,
> 
> On Mon, 13 Jul 2020 at 09:52, Cédric Le Goater  wrote:
>>
>> With this patch, the supermicro firmware boots further but there is still
>> an issue. It might be the flash definition I used. The machine is detected
>> as an AST2300 SoC which is weird.
> 
>> BMC flash ID:0x19ba20
>> Unable to handle kernel NULL pointer dereference at virtual address 
> 
> The firmware is expecting the flash ID to repeat. The following makes it boot.

That doesn't work for me.

C.

> Not sure if this is the right way to go.
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8227088441..5000930800 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1041,7 +1041,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  s->data[i] = s->pi->id[i];
>  }
>  for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> -s->data[i] = 0;
> +s->data[i] = s->pi->id[i % s->pi->id_len];
>  }
> 
>  s->len = SPI_NOR_MAX_ID_LEN;
> 

Re: [PATCH v4 2/2] acpi: i386: Move VMBus DSDT entry to SB

[Igor Mammedov]

On Thu, 25 Jun 2020 07:50:11 +0300
Jon Doron  wrote:

> Signed-off-by: Jon Doron 

Reviewed-by: Igor Mammedov 

> ---
>  hw/i386/acpi-build.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 91af0d2d0d..1f938a53b2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1091,7 +1091,6 @@ static Aml *build_vmbus_device_aml(VMBusBridge 
> *vmbus_bridge)
>  static void build_isa_devices_aml(Aml *table)
>  {
>  ISADevice *fdc = pc_find_fdc0();
> -VMBusBridge *vmbus_bridge = vmbus_bridge_find();
>  bool ambiguous;
>  
>  Aml *scope = aml_scope("_SB.PCI0.ISA");
> @@ -1112,10 +,6 @@ static void build_isa_devices_aml(Aml *table)
>  isa_build_aml(ISA_BUS(obj), scope);
>  }
>  
> -if (vmbus_bridge) {
> -aml_append(scope, build_vmbus_device_aml(vmbus_bridge));
> -}
> -
>  aml_append(table, scope);
>  }
>  
> @@ -1660,6 +1655,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>  PCIBus *bus = NULL;
>  TPMIf *tpm = tpm_find();
>  int i;
> +VMBusBridge *vmbus_bridge = vmbus_bridge_find();
>  
>  dsdt = init_aml_allocator();
>  
> @@ -1702,6 +1698,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>  }
>  }
>  
> +if (vmbus_bridge) {
> +sb_scope = aml_scope("_SB");
> +aml_append(sb_scope, build_vmbus_device_aml(vmbus_bridge));
> +aml_append(dsdt, sb_scope);
> +}
> +
>  if (pcmc->legacy_cpu_hotplug) {
>  build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
>  } else {

Re: Migrating custom qemu.org infrastructure to GitLab

[Philippe Mathieu-Daudé]

On 7/10/20 4:04 PM, Philippe Mathieu-Daudé wrote:
> On 7/8/20 11:52 AM, Stefan Hajnoczi wrote:
>>
>> There is a snowball effect where the experience is improved the more
>> GitLab features we use, so I hope that most of these migrations will
>> be possible.
> 
> I've been looking at other features we might be interested in.
[...]

> 7/ License Compliance
> 
> https://docs.gitlab.com/ee/user/compliance/license_compliance/

WRT IRC feedback QEMU licensing is a lost cause, I can not tell,
but I think if GitLab has scripts to take care of it, it is an
improvement over what we have now:

  a) Users will have a clearer idea of the permitted licenses

  b) New contributions will be checked for correct license by
 a machine, so reviewer can focus on other topics.

> 8/ Connecting with other bug tracking services:
> 
> In case we want to link BZ:
> https://docs.gitlab.com/ee/user/project/integrations/bugzilla.html
> 
> Or eventually launchpad:
> https://docs.gitlab.com/ee/user/project/integrations/custom_issue_tracker.html
> 
> 
> From these points I'm interested in investigating
> 
> - CI testing metrics
> - d3js graphs
> - License Compliance
> 
> Regards,
> 
> Phil.
> 

Re: [PATCH v4 0/2] hyperv: vmbus: ACPI various corrections

[Igor Mammedov]

On Thu, 25 Jun 2020 07:50:09 +0300
Jon Doron  wrote:

> After doing further tests and looking at the latest HyperV ACPI DSDT.
> Do minor fix to our VMBus ACPI entry.

Michael,

could you queue it for 5.1, pls?


> v4:
> * Removed the patch which adds _ADR definition to the VMBus
> * Correct the change which moves the VMBus under the SB
> 
> v3:
> Removed accidental change for the dct submodule head
> 
> v2:
> Renamed irq0 to irq now that there is a single IRQ required
> 
> Jon Doron (2):
>   hyperv: vmbus: Remove the 2nd IRQ
>   acpi: i386: Move VMBus DSDT entry to SB
> 
>  hw/hyperv/vmbus.c|  3 +--
>  hw/i386/acpi-build.c | 16 
>  include/hw/hyperv/vmbus-bridge.h |  3 +--
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 

Re: [PATCH v2] scripts/simplebench: compare write request performance

[Vladimir Sementsov-Ogievskiy]

12.07.2020 19:07, Andrey Shinkevich wrote:

On 11.07.2020 16:05, Vladimir Sementsov-Ogievskiy wrote:

26.06.2020 17:31, Andrey Shinkevich wrote:

The script 'bench_write_req.py' allows comparing performances of write
request for two qemu-img binary files.
An example with (qemu-img binary 1) and without (qemu-img binary 2) the
applied patch "qcow2: skip writing zero buffers to empty COW areas"
(git commit ID: c8bb23cbdbe32f5)
The  case does not involve the COW optimization.


Good, this proves that c8bb23cbdbe32f5 makes sense.


Suggested-by: Denis V. Lunev 
Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
v2:
   01: Three more test cases added to the script:
   
   
   

  scripts/simplebench/bench_write_req.py | 201 +
  1 file changed, 201 insertions(+)
  create mode 100755 scripts/simplebench/bench_write_req.py

diff --git a/scripts/simplebench/bench_write_req.py 
b/scripts/simplebench/bench_write_req.py
new file mode 100755
index 000..fe92d01
--- /dev/null
+++ b/scripts/simplebench/bench_write_req.py
@@ -0,0 +1,201 @@


Next, I don't understand, are you trying to fill qcow2 image by dd directly? 
This is strange. Even if you don't break metadata, you don't change it, so all 
cluster will remain empty.



I have tested and it works as designed.



But how is it designed? You just filled unallocated clusters with some data. 
When you read from qcow2, you'll still read zeros, because L1/L2 tables are not 
filled. The random data will lay untouched.

 

This dd command doesn't hurt the metadata and fills the image with random data. 
The actual disk size becomes about 1G after the dd command.




--
Best regards,
Vladimir

Re: [PATCH v4 0/2] hyperv: vmbus: ACPI various corrections

[Igor Mammedov]

On Thu, 25 Jun 2020 07:50:09 +0300
Jon Doron  wrote:

> After doing further tests and looking at the latest HyperV ACPI DSDT.
> Do minor fix to our VMBus ACPI entry.

Jon,
vmbus feature needs a testcase, could you look into it please? 
(see tests/qtest/bios-tables-test.c for an example.
also look into comment blob at the top for the propper process
for acpi patches/tests)


> v4:
> * Removed the patch which adds _ADR definition to the VMBus
> * Correct the change which moves the VMBus under the SB
> 
> v3:
> Removed accidental change for the dct submodule head
> 
> v2:
> Renamed irq0 to irq now that there is a single IRQ required
> 
> Jon Doron (2):
>   hyperv: vmbus: Remove the 2nd IRQ
>   acpi: i386: Move VMBus DSDT entry to SB
> 
>  hw/hyperv/vmbus.c|  3 +--
>  hw/i386/acpi-build.c | 16 
>  include/hw/hyperv/vmbus-bridge.h |  3 +--
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 

Re: [PATCH] tests: improve performance of device-introspect-test

[Daniel P . Berrangé]

On Fri, Jul 10, 2020 at 10:03:56PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > Total execution time with "-m slow" and x86_64 QEMU, drops from 3
> > minutes 15 seconds, down to 54 seconds.
> >
> > Individual tests drop from 17-20 seconds, down to 3-4 seconds.
> 
> Nice!
> 
> A few observations on this test (impatient readers may skip to
> "Conclusions"):

snip

> * The number of known device types varies between targets from 33
>   (tricore) to several hundreds (x86_64+i386: 421, ppc 593, arm 667,
>   aarch64 680, ppc64 689).  Median is 215, sum is 7485.

snip

> * The test matrix is *expensive*.  Testing even a simple QMP query is
>   when you do it a quarter million times.  ARM is the greediest pig by
>   far (170k introspections, almost two thirds of the total!), followed
>   by ppc (36k), x86 (12k) and mips (11k).  Ideas on trimming excess are
>   welcome.  I'm not even sure anymore this should be a qtest.

We have 70 arm machines, 667 devices. IIUC we are roughly testing every
device against everything machine. 46,690 tests.

Most of the time devices are going to behave identically regardless of
which machine type is used. The trouble is some machines are different
enough that they can genuinely trigger different behaviour. It isn't
possible to slim the (machine, device) expansion down programatically
while still exercising the interesting combinations unless we get alot
more advanced.

eg if a have a PCI device, we only need test it in one PCI based machine,
and only need test it on one non-PCI based machine.

I would be interesting to actually get some CPU profiling data for
this test to see if it points out anything interesting about where the
time is being spent. Even if we don't reduce the complexity, reducing
a time factor will potentially greatly help. 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Igor Mammedov]

On Mon, 29 Jun 2020 16:09:37 +0200
Andrew Jones  wrote:

> The flash device is exclusively for the host-controlled firmware, so
> we should not expose it to the OS. Exposing it risks the OS messing
> with it, which could break firmware runtime services and surprise the
> OS when all its changes disappear after reboot.
> 
> As firmware needs the device and uses DT, we leave the device exposed
> there. It's up to firmware to remove the nodes from DT before sending
> it on to the OS. However, there's no need to force firmware to remove
> tables from ACPI (which it doesn't know how to do anyway), so we
> simply don't add the tables in the first place. But, as we've been
> adding the tables for quite some time and don't want to change the
> default hardware exposed to versioned machines, then we only stop
> exposing the flash device tables for 5.1 and later machine types.
> 
> Suggested-by: Ard Biesheuvel 
> Suggested-by: Laszlo Ersek 
> Signed-off-by: Andrew Jones 
> ---
>  hw/arm/virt-acpi-build.c | 5 -
>  hw/arm/virt.c| 3 +++
>  include/hw/arm/virt.h| 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1384a2cf2ab4..91f0df7b13a3 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, 
> BIOSLinker *linker,
>  static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> +VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>  Aml *scope, *dsdt;
>  MachineState *ms = MACHINE(vms);
>  const MemMapEntry *memmap = vms->memmap;
> @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>  acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>  acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> (irqmap[VIRT_UART] + ARM_SPI_BASE));
> -acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +if (vmc->acpi_expose_flash) {
> +acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +}
>  acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>  acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>  (irqmap[VIRT_MMIO] + ARM_SPI_BASE), 
> NUM_VIRTIO_TRANSPORTS);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index cd0834ce7faf..5adc9ff799ef 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
>  
>  static void virt_machine_5_0_options(MachineClass *mc)
>  {
> +VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>  virt_machine_5_1_options(mc);
>  compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
>  mc->numa_mem_supported = true;
> +vmc->acpi_expose_flash = true;

we usually do not version ACPI tables changes
(unless we have a good reason to do so)

>  }
>  DEFINE_VIRT_MACHINE(5, 0)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 31878ddc7223..c65be5fe0bb6 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -119,6 +119,7 @@ typedef struct {
>  bool no_highmem_ecam;
>  bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
>  bool kvm_no_adjvtime;
> +bool acpi_expose_flash;
>  } VirtMachineClass;
>  
>  typedef struct {

Re: Migrating custom qemu.org infrastructure to GitLab

[Peter Maydell]

On Mon, 13 Jul 2020 at 09:39, Philippe Mathieu-Daudé  wrote:
>
> On 7/10/20 4:04 PM, Philippe Mathieu-Daudé wrote:
>
> > 7/ License Compliance
> >
> > https://docs.gitlab.com/ee/user/compliance/license_compliance/
>
> WRT IRC feedback QEMU licensing is a lost cause, I can not tell,
> but I think if GitLab has scripts to take care of it, it is an
> improvement over what we have now:

The docs say it only supports C programs that use the Conan
package manager. We don't, we just use configure to find and
link against our external dependencies.

>   b) New contributions will be checked for correct license by
>  a machine, so reviewer can focus on other topics.

The documentation doesn't say anything about it handling license
checking on contributions to the project itself. It looks like
it's mainly intended for "I have a javascript project that
pulls in 5000 tiny dependencies from npm, I want to know that
none of them is accidentally using a license that's not compatible
with the project's license". That's not a problem QEMU has.

thanks
-- PMM

Re: [PATCH v2] MAINTAINERS: Cover the firmware JSON schema

[Kashyap Chamarthy]

On Fri, Jul 03, 2020 at 08:34:50PM +0200, Philippe Mathieu-Daudé wrote:

(Was on PTO; just catching up.)

> Add an entry to cover firmware.json (see commit 3a0adfc9bf:
> schema that describes the different uses and properties of
> virtual machine firmware).
> 
> Cc: Laszlo Ersek 
> Cc: Gerd Hoffmann 
> Cc: Michael S. Tsirkin 
> Cc: Kashyap Chamarthy 
> Cc: Daniel P. Berrange 
> Suggested-by: Laszlo Ersek 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Since RFC v1:
> - Added Daniel & Kashyap as reviewer
> - Added myself as co-maintainer with Laszlo
> 
> Based on a comment from Laszlo:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg604925.html
> Background info from Kashyap:
> https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg01978.html
> ---

Reviewed-by: Kashyap Chamarthy  

>  MAINTAINERS | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dec252f38b..64bcea658d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2601,6 +2601,14 @@ F: include/hw/i2c/smbus_master.h
>  F: include/hw/i2c/smbus_slave.h
>  F: include/hw/i2c/smbus_eeprom.h
>  
> +Firmware schema specifications
> +M: Laszlo Ersek 
> +M: Philippe Mathieu-Daudé 
> +R: Daniel P. Berrange 
> +R: Kashyap Chamarthy 
> +S: Maintained
> +F: docs/interop/firmware.json
> +
>  EDK2 Firmware
>  M: Laszlo Ersek 
>  M: Philippe Mathieu-Daudé 
> -- 
> 2.21.3
> 

-- 
/kashyap

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: drop Linux capabilities(7)

[Chirantan Ekbote]

On Thu, Jun 25, 2020 at 9:55 PM Vivek Goyal  wrote:
>
> On Thu, Jun 25, 2020 at 12:19:39PM +0900, Chirantan Ekbote wrote:
> [..]
> > > Chirantan,
> > >
> > > So you ended up renaming all "trusted", "security" and "system" xattrs?
> > > Only "user" xattrs are complete passthrough?
> > >
> >
> > No, we only rename "security" xattrs (except for selinux).
> >
> > >
> > > IOW, security.selinux will be renamed to user.virtiofs.security.selinux
> > > on host?
> > >
> >
> > We don't relabel security.selinux because it only requires CAP_FOWNER
> > in the process's user namespace and it also does its own MAC-based
> > checks.  Also we have some tools that label files beforehand so it
> > seemed easier to leave them unchanged.
>
> If we rename selinux xattr also, then we can support selinux both in
> guest and host and they both can have their own independent policies.
>

This works as long as you don't need to support setfscreatecon, which
exists to guarantee that an inode is atomically created with the
correct selinux context.  It's kind of possible for files with
O_TMPFILE + fsetxattr + linkat but there is no equivalent for
directories.  You're basically forced to create the directory and then
set the xattr and while it's possible to prevent other threads in the
server from seeing the unfinished directories with a rwlock or similar
there is no protection against power loss.  If the machine loses power
after the directory is created but before the selinux xattr is set
then that directory will have the wrong selinux context and the guest
would need to run restorecon at boot to assign the correct label.

> Otherwise we either have to disable selinux on host (if we want to
> support it in guest) or somehow guest and how policies will have
> to know about each other and be able to work together (which will
> be hard for a generic use case).

Yes, I agree this is hard to do for a generic case but unfortunately
the more I understand how selinux works the less I feel that it works
well with a passthrough style file system.  As you said it either
needs to be turned off on the host or the host and guest need to work
together.

Chirantan

[PATCH 1/1] configure: fix incorrect have_keyring check

[Alexey Kirillov]

In some shells, we can't use == sign (as example, in dash).

Signed-off-by: Alexey Kirillov 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 31e2ddbf28..f65914f98f 100755
--- a/configure
+++ b/configure
@@ -6486,7 +6486,7 @@ EOF
 fi
 if test "$secret_keyring" != "no"
 then
-if test "$have_keyring" == "yes"
+if test "$have_keyring" = "yes"
 then
secret_keyring=yes
 else
-- 
2.25.1

Re: [PATCH 2/2] hw/arm/palm.c: Encapsulate misc GPIO handling in a device

[Philippe Mathieu-Daudé]

Hi Peter,

On 6/28/20 11:42 PM, Peter Maydell wrote:
> Replace the free-floating set of IRQs and palmte_onoff_gpios()
> function with a simple QOM device that encapsulates this
> behaviour.
> 
> This fixes Coverity issue CID 1421944, which points out that
> the memory returned by qemu_allocate_irqs() is leaked.
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/arm/palm.c | 61 +++
>  1 file changed, 52 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/palm.c b/hw/arm/palm.c
> index 569836178f6..e7bc9ea4c6a 100644
> --- a/hw/arm/palm.c
> +++ b/hw/arm/palm.c
> @@ -124,6 +124,21 @@ static void palmte_button_event(void *opaque, int 
> keycode)
>  !(keycode & 0x80));
>  }
>  
> +/*
> + * Encapsulation of some GPIO line behaviour for the Palm board
> + *
> + * QEMU interface:
> + *  + unnamed GPIO inputs 0..6: for the various miscellaneous input lines
> + */
> +
> +#define TYPE_PALM_MISC_GPIO "palm-misc-gpio"
> +#define PALM_MISC_GPIO(obj) \
> +OBJECT_CHECK(PalmMiscGPIOState, (obj), TYPE_PALM_MISC_GPIO)
> +
> +typedef struct PalmMiscGPIOState {
> +SysBusDevice parent_obj;
> +} PalmMiscGPIOState;
> +
>  static void palmte_onoff_gpios(void *opaque, int line, int level)
>  {
>  switch (line) {
> @@ -151,23 +166,44 @@ static void palmte_onoff_gpios(void *opaque, int line, 
> int level)
>  }
>  }
>  
> +static void palm_misc_gpio_init(Object *obj)
> +{
> +DeviceState *dev = DEVICE(obj);
> +
> +qdev_init_gpio_in(dev, palmte_onoff_gpios, 7);
> +}
> +
> +static const TypeInfo palm_misc_gpio_info = {
> +.name = TYPE_PALM_MISC_GPIO,
> +.parent = TYPE_SYS_BUS_DEVICE,
> +.instance_size = sizeof(PalmMiscGPIOState),
> +.instance_init = palm_misc_gpio_init,
> +/*
> + * No class init required: device has no internal state so does not
> + * need to set up reset or vmstate, and has no realize method.
> + */
> +};
> +
>  static void palmte_gpio_setup(struct omap_mpu_state_s *cpu)
>  {
> -qemu_irq *misc_gpio;
> +DeviceState *misc_gpio;
> +
> +misc_gpio = sysbus_create_simple(TYPE_PALM_MISC_GPIO, -1, NULL);

Why not make it a generic container in the MachineState and create
the container in hw/core/machine.c::machine_initfn()?

>  
>  omap_mmc_handlers(cpu->mmc,
>  qdev_get_gpio_in(cpu->gpio, PALMTE_MMC_WP_GPIO),
>  qemu_irq_invert(omap_mpuio_in_get(cpu->mpuio)
>  [PALMTE_MMC_SWITCH_GPIO]));
>  
> -misc_gpio = qemu_allocate_irqs(palmte_onoff_gpios, cpu, 7);
> -qdev_connect_gpio_out(cpu->gpio, PALMTE_MMC_POWER_GPIO, 
> misc_gpio[0]);
> -qdev_connect_gpio_out(cpu->gpio, PALMTE_SPEAKER_GPIO,   
> misc_gpio[1]);
> -qdev_connect_gpio_out(cpu->gpio, 11,
> misc_gpio[2]);
> -qdev_connect_gpio_out(cpu->gpio, 12,
> misc_gpio[3]);
> -qdev_connect_gpio_out(cpu->gpio, 13,
> misc_gpio[4]);
> -omap_mpuio_out_set(cpu->mpuio, 1,   
> misc_gpio[5]);
> -omap_mpuio_out_set(cpu->mpuio, 3,   
> misc_gpio[6]);
> +qdev_connect_gpio_out(cpu->gpio, PALMTE_MMC_POWER_GPIO,
> +  qdev_get_gpio_in(misc_gpio, 0));
> +qdev_connect_gpio_out(cpu->gpio, PALMTE_SPEAKER_GPIO,
> +  qdev_get_gpio_in(misc_gpio, 1));
> +qdev_connect_gpio_out(cpu->gpio, 11, qdev_get_gpio_in(misc_gpio, 2));
> +qdev_connect_gpio_out(cpu->gpio, 12, qdev_get_gpio_in(misc_gpio, 3));
> +qdev_connect_gpio_out(cpu->gpio, 13, qdev_get_gpio_in(misc_gpio, 4));
> +omap_mpuio_out_set(cpu->mpuio, 1, qdev_get_gpio_in(misc_gpio, 5));
> +omap_mpuio_out_set(cpu->mpuio, 3, qdev_get_gpio_in(misc_gpio, 6));
>  
>  /* Reset some inputs to initial state.  */
>  qemu_irq_lower(qdev_get_gpio_in(cpu->gpio, PALMTE_USBDETECT_GPIO));
> @@ -276,3 +312,10 @@ static void palmte_machine_init(MachineClass *mc)
>  }
>  
>  DEFINE_MACHINE("cheetah", palmte_machine_init)
> +
> +static void palm_register_types(void)
> +{
> +type_register_static(&palm_misc_gpio_info);
> +}
> +
> +type_init(palm_register_types)
> 

Re: hw/misc/aspeed_scu: 5d971f9e breaks Supermicro AST2400

[Erik Smit]

On Mon, 13 Jul 2020 at 10:37, Cédric Le Goater  wrote:
> On 7/13/20 10:06 AM, Erik Smit wrote:
> > On Mon, 13 Jul 2020 at 09:52, Cédric Le Goater  wrote:
> >>
> >> With this patch, the supermicro firmware boots further but there is still
> >> an issue. It might be the flash definition I used. The machine is detected
> >> as an AST2300 SoC which is weird.
> >
> >> BMC flash ID:0x19ba20
> >> Unable to handle kernel NULL pointer dereference at virtual address 
> >> 
> >
> > The firmware is expecting the flash ID to repeat. The following makes it 
> > boot.
>
> That doesn't work for me.

You'll probably also need
https://github.com/qemu/qemu/commit/2d6c7194c230d334da70b88b1bee5e616595cabd

Here are all my patches:
https://github.com/qemu/qemu/compare/master...erik-smit:hack-smt-x11-158

-- 
Best regards,

Erik Smit

[PATCH 01/12] target/arm/kvm: Remove superfluous break

[Yi Wang]

From: Liao Pingfang 

Remove superfluous break.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé  
---
 target/arm/kvm64.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 1169237..ef1e960 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -330,7 +330,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
 switch (type) {
 case GDB_BREAKPOINT_HW:
 return delete_hw_breakpoint(addr);
-break;
 case GDB_WATCHPOINT_READ:
 case GDB_WATCHPOINT_WRITE:
 case GDB_WATCHPOINT_ACCESS:
-- 
2.9.5

[PATCH 03/12] tcg/riscv: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé  
---
 tcg/riscv/tcg-target.inc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tcg/riscv/tcg-target.inc.c b/tcg/riscv/tcg-target.inc.c
index 2bc0ba7..3c11ab8 100644
--- a/tcg/riscv/tcg-target.inc.c
+++ b/tcg/riscv/tcg-target.inc.c
@@ -502,10 +502,8 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
 break;
 case R_RISCV_JAL:
 return reloc_jimm20(code_ptr, (tcg_insn_unit *)value);
-break;
 case R_RISCV_CALL:
 return reloc_call(code_ptr, (tcg_insn_unit *)value);
-break;
 default:
 tcg_abort();
 }
-- 
2.9.5

[PATCH 05/12] virtfs-proxy-helper: Remove the superfluous break

[Yi Wang]

From: Liao Pingfang 

Remove the superfluous break, as there is a "return" before it.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé  
---
 fsdev/virtfs-proxy-helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index de061a8..e68acc1 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -825,7 +825,6 @@ static int process_reply(int sock, int type,
 break;
 default:
 return -1;
-break;
 }
 return 0;
 }
-- 
2.9.5

Re: [PATCH 1/1] configure: fix incorrect have_keyring check

[Philippe Mathieu-Daudé]

Hi Alexey,

On 7/13/20 10:55 AM, Alexey Kirillov wrote:
> In some shells, we can't use == sign (as example, in dash).
> 
> Signed-off-by: Alexey Kirillov 
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 31e2ddbf28..f65914f98f 100755
> --- a/configure
> +++ b/configure
> @@ -6486,7 +6486,7 @@ EOF
>  fi
>  if test "$secret_keyring" != "no"
>  then
> -if test "$have_keyring" == "yes"
> +if test "$have_keyring" = "yes"
>  then
>   secret_keyring=yes
>  else
> 

Thanks, but David already sent the same fix:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg713765.html

It should be merged in today or tomorrow via Alex Bennée tree.

[PATCH 04/12] scsi: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé  
---
 scsi/utils.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/scsi/utils.c b/scsi/utils.c
index c50e81f..b37c283 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -32,17 +32,13 @@ uint32_t scsi_cdb_xfer(uint8_t *buf)
 switch (buf[0] >> 5) {
 case 0:
 return buf[4];
-break;
 case 1:
 case 2:
 return lduw_be_p(&buf[7]);
-break;
 case 4:
 return ldl_be_p(&buf[10]) & 0xULL;
-break;
 case 5:
 return ldl_be_p(&buf[6]) & 0xULL;
-break;
 default:
 return -1;
 }
-- 
2.9.5

[PATCH 02/12] target/ppc: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé  
---
 target/ppc/misc_helper.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index 55b68d1..e43a3b4 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -234,25 +234,20 @@ target_ulong helper_clcs(CPUPPCState *env, uint32_t arg)
 case 0x0CUL:
 /* Instruction cache line size */
 return env->icache_line_size;
-break;
 case 0x0DUL:
 /* Data cache line size */
 return env->dcache_line_size;
-break;
 case 0x0EUL:
 /* Minimum cache line size */
 return (env->icache_line_size < env->dcache_line_size) ?
 env->icache_line_size : env->dcache_line_size;
-break;
 case 0x0FUL:
 /* Maximum cache line size */
 return (env->icache_line_size > env->dcache_line_size) ?
 env->icache_line_size : env->dcache_line_size;
-break;
 default:
 /* Undefined */
 return 0;
-break;
 }
 }
 
-- 
2.9.5

[PATCH 06/12] migration/migration.c: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé 
---
 migration/migration.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 92e44e0..2fd5fbb 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -985,7 +985,6 @@ static void fill_source_migration_info(MigrationInfo *info)
 /* no migration has happened ever */
 /* do not overwrite destination migration status */
 return;
-break;
 case MIGRATION_STATUS_SETUP:
 info->has_status = true;
 info->has_total_time = false;
@@ -1104,7 +1103,6 @@ static void fill_destination_migration_info(MigrationInfo 
*info)
 switch (mis->state) {
 case MIGRATION_STATUS_NONE:
 return;
-break;
 case MIGRATION_STATUS_SETUP:
 case MIGRATION_STATUS_CANCELLING:
 case MIGRATION_STATUS_CANCELLED:
-- 
2.9.5

[PATCH 08/12] block/vmdk: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé 
---
 block/vmdk.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 28cec50..8f222e3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1053,14 +1053,11 @@ static int vmdk_open_sparse(BlockDriverState *bs, 
BdrvChild *file, int flags,
 switch (magic) {
 case VMDK3_MAGIC:
 return vmdk_open_vmfs_sparse(bs, file, flags, errp);
-break;
 case VMDK4_MAGIC:
 return vmdk_open_vmdk4(bs, file, flags, options, errp);
-break;
 default:
 error_setg(errp, "Image not in VMDK format");
 return -EINVAL;
-break;
 }
 }
 
-- 
2.9.5

[PATCH 09/12] hw: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi01.c |  1 -
 hw/display/cirrus_vga.c |  1 -
 hw/display/qxl-logger.c |  2 --
 hw/gpio/max7310.c   |  3 ---
 hw/i386/intel_iommu.c   |  1 -
 hw/input/pxa2xx_keypad.c| 10 --
 hw/intc/armv7m_nvic.c   |  1 -
 hw/net/lan9118.c|  2 --
 hw/usb/ccid-card-emulated.c |  1 -
 9 files changed, 22 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 8ab1d66..f0fcd63 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -213,7 +213,6 @@ static uint32_t pflash_devid_query(PFlashCFI01 *pfl, hwaddr 
offset)
 default:
 trace_pflash_device_info(offset);
 return 0;
-break;
 }
 /* Replicate responses for each device in bank. */
 if (pfl->device_width < pfl->bank_width) {
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 212d6f5..02d9ed0 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -1637,7 +1637,6 @@ static int cirrus_vga_read_cr(CirrusVGAState * s, 
unsigned reg_index)
return s->vga.cr[s->vga.cr_index];
 case 0x26: // Attribute Controller Index Readback (R)
return s->vga.ar_index & 0x3f;
-   break;
 default:
 qemu_log_mask(LOG_GUEST_ERROR,
   "cirrus: inport cr_index 0x%02x\n", reg_index);
diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
index 2ec6d8f..c15175b 100644
--- a/hw/display/qxl-logger.c
+++ b/hw/display/qxl-logger.c
@@ -161,7 +161,6 @@ static int qxl_log_cmd_draw(PCIQXLDevice *qxl, QXLDrawable 
*draw, int group_id)
 switch (draw->type) {
 case QXL_DRAW_COPY:
 return qxl_log_cmd_draw_copy(qxl, &draw->u.copy, group_id);
-break;
 }
 return 0;
 }
@@ -180,7 +179,6 @@ static int qxl_log_cmd_draw_compat(PCIQXLDevice *qxl, 
QXLCompatDrawable *draw,
 switch (draw->type) {
 case QXL_DRAW_COPY:
 return qxl_log_cmd_draw_copy(qxl, &draw->u.copy, group_id);
-break;
 }
 return 0;
 }
diff --git a/hw/gpio/max7310.c b/hw/gpio/max7310.c
index bebb403..4f78774 100644
--- a/hw/gpio/max7310.c
+++ b/hw/gpio/max7310.c
@@ -51,11 +51,9 @@ static uint8_t max7310_rx(I2CSlave *i2c)
 switch (s->command) {
 case 0x00: /* Input port */
 return s->level ^ s->polarity;
-break;
 
 case 0x01: /* Output port */
 return s->level & ~s->direction;
-break;
 
 case 0x02: /* Polarity inversion */
 return s->polarity;
@@ -65,7 +63,6 @@ static uint8_t max7310_rx(I2CSlave *i2c)
 
 case 0x04: /* Timeout */
 return s->status;
-break;
 
 case 0xff: /* Reserved */
 return 0xff;
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c56398e..7b390ca 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3163,7 +3163,6 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t 
index,
   index, entry->irte.sid_vtype);
 /* Take this as verification failure. */
 return -VTD_FR_IR_SID_ERR;
-break;
 }
 }
 
diff --git a/hw/input/pxa2xx_keypad.c b/hw/input/pxa2xx_keypad.c
index 62aa6f6..7f2f739 100644
--- a/hw/input/pxa2xx_keypad.c
+++ b/hw/input/pxa2xx_keypad.c
@@ -192,10 +192,8 @@ static uint64_t pxa2xx_keypad_read(void *opaque, hwaddr 
offset,
 s->kpc &= ~(KPC_DI);
 qemu_irq_lower(s->irq);
 return tmp;
-break;
 case KPDK:
 return s->kpdk;
-break;
 case KPREC:
 tmp = s->kprec;
 if(tmp & KPREC_OF1)
@@ -207,31 +205,23 @@ static uint64_t pxa2xx_keypad_read(void *opaque, hwaddr 
offset,
 if(tmp & KPREC_UF0)
 s->kprec &= ~(KPREC_UF0);
 return tmp;
-break;
 case KPMK:
 tmp = s->kpmk;
 if(tmp & KPMK_MKP)
 s->kpmk &= ~(KPMK_MKP);
 return tmp;
-break;
 case KPAS:
 return s->kpas;
-break;
 case KPASMKP0:
 return s->kpasmkp[0];
-break;
 case KPASMKP1:
 return s->kpasmkp[1];
-break;
 case KPASMKP2:
 return s->kpasmkp[2];
-break;
 case KPASMKP3:
 return s->kpasmkp[3];
-break;
 case KPKDI:
 return s->kpkdi;
-break;
 default:
 qemu_log_mask(LOG_GUEST_ERROR,
   "%s: Bad read offset 0x%"HWADDR_PRIx"\n",
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 3c4b6e6..720ac97 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -1275,7 +1275,6 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, 
MemTxAttrs attrs)
 case 0xd90: /* MPU_TYPE */
 /* Unified MPU; if the MPU is not present this value is zero */
 return cpu->pmsav7_dre

[PATCH 11/12] target/sh4: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé 
---
 target/sh4/translate.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index 6192d83..60c863d 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -1542,7 +1542,6 @@ static void _decode_opc(DisasContext * ctx)
 tcg_gen_qemu_ld_i32(REG(0), REG(B11_8), ctx->memidx,
 MO_TEUL | MO_UNALN);
 return;
-break;
 case 0x40e9:/* movua.l @Rm+,R0 */
 CHECK_SH4A
 /* Load non-boundary-aligned data */
@@ -1550,7 +1549,6 @@ static void _decode_opc(DisasContext * ctx)
 MO_TEUL | MO_UNALN);
 tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);
 return;
-break;
 case 0x0029:   /* movt Rn */
 tcg_gen_mov_i32(REG(B11_8), cpu_sr_t);
return;
@@ -1638,7 +1636,6 @@ static void _decode_opc(DisasContext * ctx)
 CHECK_SH4A
 tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
 return;
-break;
 case 0x4024:   /* rotcl Rn */
{
TCGv tmp = tcg_temp_new();
-- 
2.9.5

[PATCH 07/12] vnc: Remove the superfluous break

[Yi Wang]

From: Liao Pingfang 

Remove the superfluous break, as there is a "return" before.

Signed-off-by: Liao Pingfang a
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé 
---
 ui/vnc-enc-tight.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index 1e08518..cebd358 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -1125,7 +1125,6 @@ static int send_palette_rect(VncState *vs, int x, int y,
 }
 default:
 return -1; /* No palette for 8bits colors */
-break;
 }
 bytes = w * h;
 vs->tight->tight.offset = bytes;
-- 
2.9.5

[PATCH 12/12] target/cris: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé 
---
 target/cris/translate.c | 7 +++
 target/cris/translate_v10.inc.c | 2 --
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/target/cris/translate.c b/target/cris/translate.c
index aaa46b5..64a478b 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -1178,12 +1178,11 @@ static inline void t_gen_zext(TCGv d, TCGv s, int size)
 static char memsize_char(int size)
 {
 switch (size) {
-case 1: return 'b';  break;
-case 2: return 'w';  break;
-case 4: return 'd';  break;
+case 1: return 'b';
+case 2: return 'w';
+case 4: return 'd';
 default:
 return 'x';
-break;
 }
 }
 #endif
diff --git a/target/cris/translate_v10.inc.c b/target/cris/translate_v10.inc.c
index ae34a0d..7f38fd2 100644
--- a/target/cris/translate_v10.inc.c
+++ b/target/cris/translate_v10.inc.c
@@ -1026,10 +1026,8 @@ static unsigned int dec10_ind(CPUCRISState *env, 
DisasContext *dc)
 switch (dc->opcode) {
 case CRISV10_IND_MOVE_M_R:
 return dec10_ind_move_m_r(env, dc, size);
-break;
 case CRISV10_IND_MOVE_R_M:
 return dec10_ind_move_r_m(dc, size);
-break;
 case CRISV10_IND_CMP:
 LOG_DIS("cmp size=%d op=%d %d\n",  size, dc->src, dc->dst);
 cris_cc_mask(dc, CC_MASK_NZVC);
-- 
2.9.5

[PATCH 10/12] target/openrisc: Remove superfluous breaks

[Yi Wang]

From: Liao Pingfang 

Remove superfluous breaks, as there is a "return" before them.

Signed-off-by: Liao Pingfang 
Signed-off-by: Yi Wang 
Reviewed-by: Philippe Mathieu-Daudé 
---
 target/openrisc/sys_helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index d9fe6c5..d9691d0 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -289,10 +289,8 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, 
target_ulong rd,
 
 case TO_SPR(5, 1):  /* MACLO */
 return (uint32_t)env->mac;
-break;
 case TO_SPR(5, 2):  /* MACHI */
 return env->mac >> 32;
-break;
 
 case TO_SPR(8, 0):  /* PMR */
 return env->pmr;
-- 
2.9.5

Re: [PATCH v1 1/1] migration/colo.c: migrate dirty ram pages before colo checkpoint

[Derek Su]

Hello,

Ping...

Anyone have comments about this path?
To reduce the downtime during checkpoints, the patch tries to migrate 
memory page as many as possible just before entering COLO state.


Thanks.

Regards,
Derek

On 2020/6/21 上午10:10, Derek Su wrote:

To reduce the guest's downtime during checkpoint, migrate dirty
ram pages as many as possible before colo checkpoint.

If the iteration count reaches COLO_RAM_MIGRATE_ITERATION_MAX or
ram pending size is lower than 'x-colo-migrate-ram-threshold',
stop the ram migration and colo checkpoint.

Signed-off-by: Derek Su 
---
  migration/colo.c   | 79 ++
  migration/migration.c  | 20 +++
  migration/trace-events |  2 ++
  monitor/hmp-cmds.c |  8 +
  qapi/migration.json| 18 --
  5 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index ea7d1e9d4e..a0c71d7c56 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -46,6 +46,13 @@ static Notifier packets_compare_notifier;
  static COLOMode last_colo_mode;
  
  #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)

+#define COLO_RAM_MIGRATE_ITERATION_MAX 10
+
+typedef enum {
+COLO_MIG_ITERATE_RESUME = 0,  /* Resume migration iteration */
+COLO_MIG_ITERATE_BREAK  = 1,  /* Break migration loop */
+} COLOMigIterateState;
+
  
  bool migration_in_colo_state(void)

  {
@@ -398,6 +405,68 @@ static uint64_t colo_receive_message_value(QEMUFile *f, 
uint32_t expect_msg,
  return value;
  }
  
+static int colo_migrate_ram_iteration(MigrationState *s, Error **errp)

+{
+int64_t threshold_size = s->parameters.x_colo_migrate_ram_threshold;
+uint64_t pending_size, pend_pre, pend_compat, pend_post;
+Error *local_err = NULL;
+int ret;
+
+if (threshold_size == 0) {
+return COLO_MIG_ITERATE_BREAK;
+}
+
+qemu_savevm_state_pending(s->to_dst_file, threshold_size,
+  &pend_pre, &pend_compat, &pend_post);
+pending_size = pend_pre + pend_compat + pend_post;
+
+trace_colo_migrate_pending(pending_size, threshold_size,
+   pend_pre, pend_compat, pend_post);
+
+/* Still a significant amount to transfer */
+if (pending_size && pending_size >= threshold_size) {
+colo_send_message(s->to_dst_file, COLO_MESSAGE_MIGRATE_RAM, 
&local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return COLO_MIG_ITERATE_BREAK;
+}
+
+qemu_savevm_state_iterate(s->to_dst_file, false);
+qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
+
+ret = qemu_file_get_error(s->to_dst_file);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to send dirty pages");
+return COLO_MIG_ITERATE_BREAK;
+}
+
+return COLO_MIG_ITERATE_RESUME;
+}
+
+trace_colo_migration_low_pending(pending_size);
+
+return COLO_MIG_ITERATE_BREAK;
+}
+
+static void colo_migrate_ram_before_checkpoint(MigrationState *s, Error **errp)
+{
+Error *local_err = NULL;
+int iterate_count = 0;
+
+while (iterate_count++ < COLO_RAM_MIGRATE_ITERATION_MAX) {
+COLOMigIterateState state;
+
+state = colo_migrate_ram_iteration(s, &local_err);
+if (state == COLO_MIG_ITERATE_BREAK) {
+if (local_err) {
+error_propagate(errp, local_err);
+}
+
+return;
+}
+};
+}
+
  static int colo_do_checkpoint_transaction(MigrationState *s,
QIOChannelBuffer *bioc,
QEMUFile *fb)
@@ -405,6 +474,11 @@ static int colo_do_checkpoint_transaction(MigrationState 
*s,
  Error *local_err = NULL;
  int ret = -1;
  
+colo_migrate_ram_before_checkpoint(s, &local_err);

+if (local_err) {
+goto out;
+}
+
  colo_send_message(s->to_dst_file, COLO_MESSAGE_CHECKPOINT_REQUEST,
&local_err);
  if (local_err) {
@@ -819,6 +893,11 @@ static void 
colo_wait_handle_message(MigrationIncomingState *mis,
  case COLO_MESSAGE_CHECKPOINT_REQUEST:
  colo_incoming_process_checkpoint(mis, fb, bioc, errp);
  break;
+case COLO_MESSAGE_MIGRATE_RAM:
+if (qemu_loadvm_state_main(mis->from_src_file, mis) < 0) {
+error_setg(errp, "Load ram failed");
+}
+break;
  default:
  error_setg(errp, "Got unknown COLO message: %d", msg);
  break;
diff --git a/migration/migration.c b/migration/migration.c
index 481a590f72..390937ae5d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -88,6 +88,7 @@
  
  /* The delay time (in ms) between two COLO checkpoints */

  #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100)
+#define DEFAULT_COLO_MIGRATE_RAM_THRESHOLD (100 * 1024 * 1024UL)
  #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2
  #define DEFAULT_MIGRATE_MULTIFD_COMPRESSION MULTIFD_COMPRESSION_NON

Re: [PATCH v7 01/47] block: Add child access functions

[Vladimir Sementsov-Ogievskiy]

25.06.2020 18:21, Max Reitz wrote:

There are BDS children that the general block layer code can access,
namely bs->file and bs->backing.  Since the introduction of filters and
external data files, their meaning is not quite clear.  bs->backing can
be a COW source, or it can be a filtered child; bs->file can be a
filtered child, it can be data and metadata storage, or it can be just
metadata storage.

This overloading really is not helpful.  This patch adds functions that
retrieve the correct child for each exact purpose.  Later patches in
this series will make use of them.  Doing so will allow us to handle
filter nodes in a meaningful way.

Signed-off-by: Max Reitz 
---


[..]


+/*
+ * Return the primary child of this node: For filters, that is the
+ * filtered child.  For other nodes, that is usually the child storing
+ * metadata.
+ * (A generally more helpful description is that this is (usually) the
+ * child that has the same filename as @bs.)
+ *
+ * Drivers do not necessarily have a primary child; for example quorum
+ * does not.
+ */
+BdrvChild *bdrv_primary_child(BlockDriverState *bs)
+{
+BdrvChild *c;
+
+QLIST_FOREACH(c, &bs->children, next) {
+if (c->role & BDRV_CHILD_PRIMARY) {
+return c;
+}
+}
+
+return NULL;
+}



Suggest squash-in to also assert that not more than one primary child:
--- a/block.c
+++ b/block.c
@@ -6998,13 +6998,14 @@ BdrvChild *bdrv_filter_or_cow_child(BlockDriverState 
*bs)
  */
 BdrvChild *bdrv_primary_child(BlockDriverState *bs)
 {
-BdrvChild *c;
+BdrvChild *c, *found = NULL;
 
 QLIST_FOREACH(c, &bs->children, next) {

 if (c->role & BDRV_CHILD_PRIMARY) {
-return c;
+assert(!found);
+found = c;
 }
 }
 
-return NULL;

+return c;
 }


with or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir

Re: [PATCH 05/12] virtfs-proxy-helper: Remove the superfluous break

[Philippe Mathieu-Daudé]

On 7/13/20 11:04 AM, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove the superfluous break, as there is a "return" before it.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé  

Hmm I never reviewed this patch, only the openrisc one:
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04078.html

Something is wrong in your workflow, you are posting each patch as
a new thread. You might be missing the --cc-cover option, see:
https://wiki.qemu.org/Contribute/SubmitAPatch#Use_git_format-patch

> ---
>  fsdev/virtfs-proxy-helper.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index de061a8..e68acc1 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -825,7 +825,6 @@ static int process_reply(int sock, int type,
>  break;
>  default:
>  return -1;
> -break;
>  }
>  return 0;
>  }
> 

Re: [PATCH for-5.1] file-posix: Mitigate file fragmentation with extent size hints

[Max Reitz]

On 10.07.20 18:12, Max Reitz wrote:
> On 07.07.20 18:17, Kevin Wolf wrote:
>> Am 07.07.2020 um 16:23 hat Kevin Wolf geschrieben:
>>> Espeically when O_DIRECT is used with image files so that the page cache
>>> indirection can't cause a merge of allocating requests, the file will
>>> fragment on the file system layer, with a potentially very small
>>> fragment size (this depends on the requests the guest sent).
>>>
>>> On Linux, fragmentation can be reduced by setting an extent size hint
>>> when creating the file (at least on XFS, it can't be set any more after
>>> the first extent has been allocated), basically giving raw files a
>>> "cluster size" for allocation.
>>>
>>> This adds an create option to set the extent size hint, and changes the
>>> default from not setting a hint to setting it to 1 MB. The main reason
>>> why qcow2 defaults to smaller cluster sizes is that COW becomes more
>>> expensive, which is not an issue with raw files, so we can choose a
>>> larger file. The tradeoff here is only potentially wasted disk space.
>>>
>>> For qcow2 (or other image formats) over file-posix, the advantage should
>>> even be greater because they grow sequentially without leaving holes, so
>>> there won't be wasted space. Setting even larger extent size hints for
>>> such images may make sense. This can be done with the new option, but
>>> let's keep the default conservative for now.
>>>
>>> The effect is very visible with a test that intentionally creates a
>>> badly fragmented file with qemu-img bench (the time difference while
>>> creating the file is already remarkable) and then looks at the number of
>>> extents and the take a simple "qemu-img map" takes.
>>>
>>> Without an extent size hint:
>>>
>>> $ ./qemu-img create -f raw -o extent_size_hint=0 ~/tmp/test.raw 10G
>>> Formatting '/home/kwolf/tmp/test.raw', fmt=raw size=10737418240 
>>> extent_size_hint=0
>>> $ ./qemu-img bench -f raw -t none -n -w ~/tmp/test.raw -c 100 -S 
>>> 8192 -o 0
>>> Sending 100 write requests, 4096 bytes each, 64 in parallel 
>>> (starting at offset 0, step size 8192)
>>> Run completed in 25.848 seconds.
>>> $ ./qemu-img bench -f raw -t none -n -w ~/tmp/test.raw -c 100 -S 
>>> 8192 -o 4096
>>> Sending 100 write requests, 4096 bytes each, 64 in parallel 
>>> (starting at offset 4096, step size 8192)
>>> Run completed in 19.616 seconds.
>>> $ filefrag ~/tmp/test.raw
>>> /home/kwolf/tmp/test.raw: 200 extents found
>>> $ time ./qemu-img map ~/tmp/test.raw
>>> Offset  Length  Mapped to   File
>>> 0   0x1e848 0   /home/kwolf/tmp/test.raw
>>>
>>> real0m1,279s
>>> user0m0,043s
>>> sys 0m1,226s
>>>
>>> With the new default extent size hint of 1 MB:
>>>
>>> $ ./qemu-img create -f raw -o extent_size_hint=1M ~/tmp/test.raw 10G
>>> Formatting '/home/kwolf/tmp/test.raw', fmt=raw size=10737418240 
>>> extent_size_hint=1048576
>>> $ ./qemu-img bench -f raw -t none -n -w ~/tmp/test.raw -c 100 -S 
>>> 8192 -o 0
>>> Sending 100 write requests, 4096 bytes each, 64 in parallel 
>>> (starting at offset 0, step size 8192)
>>> Run completed in 11.833 seconds.
>>> $ ./qemu-img bench -f raw -t none -n -w ~/tmp/test.raw -c 100 -S 
>>> 8192 -o 4096
>>> Sending 100 write requests, 4096 bytes each, 64 in parallel 
>>> (starting at offset 4096, step size 8192)
>>> Run completed in 10.155 seconds.
>>> $ filefrag ~/tmp/test.raw
>>> /home/kwolf/tmp/test.raw: 178 extents found
>>> $ time ./qemu-img map ~/tmp/test.raw
>>> Offset  Length  Mapped to   File
>>> 0   0x1e848 0   /home/kwolf/tmp/test.raw
>>>
>>> real0m0,061s
>>> user0m0,040s
>>> sys 0m0,014s
>>>
>>> Signed-off-by: Kevin Wolf 
>>
>> I also need to squash in a few trivial qemu-iotests updates, for which I
>> won't send a v2:
> 
> The additional specifications in 243 make it print a warning on tmpfs
> (because the option doesn’t work there).  I suppose the same may be true
> on other filesystems as well.  Should it be filtered out?

This patch also breaks 059, 106, and 175.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 1/3] hw/i386: Initialize topo_ids from CpuInstanceProperties

[Igor Mammedov]

On Wed, 01 Jul 2020 12:31:01 -0500
Babu Moger  wrote:

> This is in preparation to build the apic_id from user
> provided topology information.
> 
> Signed-off-by: Babu Moger 
> ---
>  include/hw/i386/topology.h |   19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index 07239f95f4..7cb21e9c82 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -40,6 +40,7 @@
>  
>  
>  #include "qemu/bitops.h"
> +#include "qapi/qapi-types-machine.h"
>  
>  /* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
>   */
> @@ -196,6 +197,24 @@ static inline void 
> x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
>  topo_ids->pkg_id = apicid >> apicid_pkg_offset_epyc(topo_info);
>  }
>  
> +
> +/*
> + * Initialize topo_ids from CpuInstanceProperties
> + * node_id in CpuInstanceProperties(or in CPU device) is a sequential
> + * number, but while building the topology 

>we need to separate it for
> + * each socket(mod nodes_per_pkg).
could you clarify a bit more on why this is necessary?


> + */
> +static inline void x86_init_topo_ids(X86CPUTopoInfo *topo_info,
> + CpuInstanceProperties props,
> + X86CPUTopoIDs *topo_ids)
> +{
> +topo_ids->smt_id = props.has_thread_id ? props.thread_id : 0;
> +topo_ids->core_id = props.has_core_id ? props.core_id : 0;
> +topo_ids->die_id = props.has_die_id ? props.die_id : 0;
> +topo_ids->node_id = props.has_node_id ?
> +props.node_id % MAX(topo_info->nodes_per_pkg, 1) : 0;
> +topo_ids->pkg_id = props.has_socket_id ? props.socket_id : 0;
> +}
>  /*
>   * Make APIC ID for the CPU 'cpu_index'
>   *
> 

Re: [PATCH 10/12] target/openrisc: Remove superfluous breaks

[Philippe Mathieu-Daudé]

On 7/13/20 11:05 AM, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove superfluous breaks, as there is a "return" before them.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé 

FYI the subject should say this is "PATCH v2", see:
https://wiki.qemu.org/Contribute/SubmitAPatch#When_resending_patches_add_a_version_tag

> ---
>  target/openrisc/sys_helper.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
> index d9fe6c5..d9691d0 100644
> --- a/target/openrisc/sys_helper.c
> +++ b/target/openrisc/sys_helper.c
> @@ -289,10 +289,8 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, 
> target_ulong rd,
>  
>  case TO_SPR(5, 1):  /* MACLO */
>  return (uint32_t)env->mac;
> -break;
>  case TO_SPR(5, 2):  /* MACHI */
>  return env->mac >> 32;
> -break;
>  
>  case TO_SPR(8, 0):  /* PMR */
>  return env->pmr;
> 

Re: migration: broken snapshot saves appear on s390 when small fields in migration stream removed

[Claudio Fontana]

Hi Paolo,

On 7/12/20 6:11 PM, Paolo Bonzini wrote:
> On 12/07/20 12:00, Claudio Fontana wrote:
>> Note: only the === -blockdev with a backing file === part of test 267 fails. 
>> -blockdev with NBD is ok, like all the rest.
>>
>>
>> Interesting facts about s390 in particular: its save/load code includes the 
>> transfer of "storage keys",
>> which include a buffer of 32768 bytes of keydata in the stream.
>>
>> The code (hw/s390x/s390-skeys.c),
>> is modeled similarly to RAM transfer (like in migration/ram.c), with an EOS 
>> (end of stream) marker.
>>
>> Countrary to RAM transfer code though, after qemu_put_be64(f, EOS), the s390 
>> code does not qemu_fflush(f).
> 
> 1) Are there unexpected differences in the migration stream?  That is,
> you could modify qcow2.c to fopen/fwrite/fclose the bytes as they're
> written and read, and see if something does not match.

I hooked qcow2_co_pwritev_task and qcow2_co_preadv_task to also write to an 
external file using your suggestion.

I am seeing very interesting differences with and without the reproducer patch 
(ie, forcing icount_state_needed to 0 or not):

* without the reproducer patch, everything past the s390-skeys data field is in 
order: there is the EOS, and then the next idstr follows ("cpu_common").

* with the reproducer patch, every single byte past the s390-skeys data field 
is ZERO. There is no EOS, there is no next idstr "cpu_common", there is 
absolutely nothing else than zeroes until the end of the file.


> 
> 2) If it matches, are there unexpected differences other than the lack
> of icount section when you apply the reproducer patch?

they do not match at all.


> 
> The fflush part makes me put more hope in the first, but both could help
> you debug it.
> 
> Thanks,
> 
> Paolo
> 

Thanks,

Claudio

Re: [PATCH RFC 2/5] s390x: implement diag260

[Heiko Carstens]

On Fri, Jul 10, 2020 at 05:24:07PM +0200, David Hildenbrand wrote:
> On 10.07.20 17:18, Heiko Carstens wrote:
> > On Fri, Jul 10, 2020 at 02:12:33PM +0200, David Hildenbrand wrote:
> >>> Note: Reading about diag260 subcode 0xc, we could modify Linux to query
> >>> the maximum possible pfn via diag260 0xc. Then, we maybe could avoid
> >>> indicating maxram size via SCLP, and keep diag260-unaware OSs keep
> >>> working as before. Thoughts?
> >>
> >> Implemented it, seems to work fine.
> > 
> > The returned value would not include standby/reserved memory within
> > z/VM. So this seems not to work.
> 
> Which value exactly are you referencing? diag 0xc returns two values.
> One of them seems to do exactly what we need.
> 
> See
> https://github.com/davidhildenbrand/linux/commit/a235f9fb20df7c04ae89bc0d134332d1a01842c7
> 
> for my current Linux approach.
> 
> > Also: why do you want to change this
> 
> Which change exactly do you mean?
> 
> If we limit the value returned via SCLP to initial memory, we cannot
> break any guest (e.g., Linux pre 4.2, kvm-unit-tests). diag260 is then
> purely optional.

Ok, now I see the context. Christian added my just to cc on this
specific patch.
So if I understand you correctly, then you want to use diag 260 in
order to figure out how much memory is _potentially_ available for a
guest?

This does not fit to the current semantics, since diag 260 returns the
address of the highest *currently* accessible address. That is: it
does explicitly *not* include standby memory or anything else that
might potentially be there.

So you would need a different interface to tell the guest about your
new hotplug memory interface. If sclp does not work, then maybe a new
diagnose(?).

Re: hw/misc/aspeed_scu: 5d971f9e breaks Supermicro AST2400

[Cédric Le Goater]

On 7/13/20 10:58 AM, Erik Smit wrote:
> On Mon, 13 Jul 2020 at 10:37, Cédric Le Goater  wrote:
>> On 7/13/20 10:06 AM, Erik Smit wrote:
>>> On Mon, 13 Jul 2020 at 09:52, Cédric Le Goater  wrote:

 With this patch, the supermicro firmware boots further but there is still
 an issue. It might be the flash definition I used. The machine is detected
 as an AST2300 SoC which is weird.
>>>
 BMC flash ID:0x19ba20
 Unable to handle kernel NULL pointer dereference at virtual address 
 
>>>
>>> The firmware is expecting the flash ID to repeat. The following makes it 
>>> boot.
>>
>> That doesn't work for me.
> 
> You'll probably also need
> https://github.com/qemu/qemu/commit/2d6c7194c230d334da70b88b1bee5e616595cabd

Nice. You should send that one.

It looks much better. the machine reaches a shell but there are still
some issues and it reboots (ipmi_kcs). 

I don't know what are these faults :

  Unhandled fault: external abort on non-linefetch (0x008) at 0xc50f

Thanks,

C.


U-Boot 2009.01 ( 4月 02 2019 - 15:29:02) ASPEED (v.0.21) 

I2C:   ready
DRAM:  128 MB
Flash: 32 MB
*** Warning - bad CRC, using default environment

In:serial
Out:   serial
Err:   serial
H/W:   AST2400 series chip
COM:   port1 and port2
PWM:   port[ABCDH]
Hit any key to stop autoboot:  0 
## Booting kernel from Legacy Image at 2140 ...
   Image Name:   2140
   Image Type:   ARM Linux Kernel Image (gzip compressed)
   Data Size:1536645 Bytes =  1.5 MB
   Load Address: 40008000
   Entry Point:  40008000
   Verifying Checksum ... cramfs size 15032320
cramfs size 7315456
OK
   Uncompressing Kernel Image ... OK

Starting kernel ...

Linux version 2.6.28.9 (root@localhost) (gcc version 4.9.1 (crosstool-NG 
1.20.0) ) #1 Wed Nov 20 18:20:38 CST 2019
CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
CPU: VIVT data cache, VIVT instruction cache
Machine: ASPEED-AST2300
Memory policy: ECC disabled, Data cache writeback
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 20066
Kernel command line: console=ttyS1,115200 root=/dev/mtdblock2 rootfstype=cramfs 
noinitrd rw mem=79M
PID hash table entries: 512 (order: 9, 2048 bytes)
Console: colour dummy device 80x30
console [ttyS1] enabled
Dentry cache hash table entries: 16384 (order: 4, 65536 bytes)
Inode-cache hash table entries: 8192 (order: 3, 32768 bytes)
Memory: 79MB = 79MB total
Memory: 76736KB available (2848K code, 358K data, 112K init)
SLUB: Genslabs=12, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
Calibrating delay loop... 653.72 BogoMIPS (lpj=3268608)
Mount-cache hash table entries: 512
CPU: Testing write buffer coherency: ok
net_namespace: 636 bytes
NET: Registered protocol family 16
SCSI subsystem initialized
NET: Registered protocol family 2
IP route cache hash table entries: 1024 (order: 0, 4096 bytes)
TCP established hash table entries: 4096 (order: 3, 32768 bytes)
TCP bind hash table entries: 4096 (order: 2, 16384 bytes)
TCP: Hash tables configured (established 4096 bind 4096)
TCP reno registered
NET: Registered protocol family 1
NetWinder Floating Point Emulator V0.97 (extended precision)
NTFS driver 2.1.29 [Flags: R/W].
JFFS2 version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red Hat, Inc.
fuse init (API version 7.10)
msgmni has been set to 149
alg: No test for stdrng (krng)
io scheduler noop registered
io scheduler anticipatory registered
io scheduler deadline registered
io scheduler cfq registered (default)
Non-volatile memory driver v1.2
ttyS0 at MMIO 0x1e783000 (irq = 9) is a ASPEED UART
ttyS1 at MMIO 0x1e784000 (irq = 10) is a ASPEED UART
brd: module loaded
loop: module loaded
nbd: registered device at major 43
Driver 'sd' needs updating - please use bus_type methods
Driver 'sr' needs updating - please use bus_type methods
BMC flash ID:0xc21920c2
 platform_flash: MX25L25635E (32768 Kbytes)
Creating 7 MTD partitions on "spi0.0":
0x-0x0010 : "bootloader"
0x0010-0x0040 : "nvram"
0x0040-0x0140 : "rootFS"
0x0140-0x0170 : "kernel"
0x0170-0x01f4 : "webpage"
0x-0x01fc : "all_part"
0x01fc-0x01fd : "uboot_env"
mice: PS/2 mouse device common for all mice
ip_tables: (C) 2000-2006 Netfilter Core Team
TCP cubic registered
NET: Registered protocol family 10
lo: Disabled Privacy Extensions
ip6_tables: (C) 2000-2006 Netfilter Core Team
IPv6 over IPv4 tunneling driver
sit0: Disabled Privacy Extensions
NET: Registered protocol family 17
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
802.1Q VLAN Support v1.8 Ben Greear 
All bugs added by David S. Miller 
VFS: Mounted root (cramfs filesystem) readonly.
Freeing init memory: 112K
starting pid 335, tty '': '/etc/init.d/rcS'
udevd[360]: specified group 'tty' unknown

udevd[360]: specified group 'kmem' unknown

populate devices node .
Block at 0x: free 0x, dirty 0x0001, unchecked 0x, 
used 0x, wasted 0x
JFFS2: Erase block at 0x00

Re: [PATCH v2 2/3] hw/i386: Build apic_id from CpuInstanceProperties

[Igor Mammedov]

On Wed, 01 Jul 2020 12:31:08 -0500
Babu Moger  wrote:

> Build apic_id from CpuInstanceProperties if numa configured.
> Use the node_id from user provided numa information. This
> will avoid conflicts between numa information and apic_id
> generated.
> 
> Re-arranged the code little bit to make sure CpuInstanceProperties
> is initialized before calling.
> 
> Signed-off-by: Babu Moger 
> ---
>  hw/i386/pc.c   |6 +-
>  hw/i386/x86.c  |   19 +--
>  include/hw/i386/topology.h |   14 +++---
>  include/hw/i386/x86.h  |6 --
>  tests/test-x86-cpuid.c |   39 ---
>  5 files changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d103b8c0ab..e613b2299f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -800,13 +800,17 @@ void pc_smp_parse(MachineState *ms, QemuOpts *opts)
>  void pc_hot_add_cpu(MachineState *ms, const int64_t id, Error **errp)
>  {
>  X86MachineState *x86ms = X86_MACHINE(ms);
> -int64_t apic_id = x86_cpu_apic_id_from_index(x86ms, id);
> +CpuInstanceProperties props;
> +int64_t apic_id;
>  Error *local_err = NULL;
>  
>  if (id < 0) {
>  error_setg(errp, "Invalid CPU id: %" PRIi64, id);
>  return;
>  }
> +props = ms->possible_cpus->cpus[id].props;
> +
> +apic_id = x86_cpu_apic_id_from_index(x86ms, id, props);
>  
>  if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) {
>  error_setg(errp, "Unable to add CPU: %" PRIi64
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 34229b45c7..7554416ae0 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -93,7 +93,8 @@ static void x86_set_epyc_topo_handlers(MachineState 
> *machine)
>   * all CPUs up to max_cpus.
>   */
>  uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
> -unsigned int cpu_index)
> +unsigned int cpu_index,
> +CpuInstanceProperties props)
>  {
>  X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
>  X86CPUTopoInfo topo_info;
> @@ -102,7 +103,7 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState 
> *x86ms,
>  
>  init_topo_info(&topo_info, x86ms);
>  
> -correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index);
> +correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index, props);
>  if (x86mc->compat_apic_id_mode) {
>  if (cpu_index != correct_id && !warned && !qtest_enabled()) {
>  error_report("APIC IDs set in compatibility mode, "
> @@ -136,6 +137,8 @@ void x86_cpus_init(X86MachineState *x86ms, int 
> default_cpu_version)
>  const CPUArchIdList *possible_cpus;
>  MachineState *ms = MACHINE(x86ms);
>  MachineClass *mc = MACHINE_GET_CLASS(x86ms);
> +CpuInstanceProperties props;
> +
>  
>  /* Check for apicid encoding */
>  if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
> @@ -144,6 +147,8 @@ void x86_cpus_init(X86MachineState *x86ms, int 
> default_cpu_version)
>  
>  x86_cpu_set_default_version(default_cpu_version);
>  
> +possible_cpus = mc->possible_cpu_arch_ids(ms);
> +
>  /*
>   * Calculates the limit to CPU APIC ID values
>   *
> @@ -152,13 +157,15 @@ void x86_cpus_init(X86MachineState *x86ms, int 
> default_cpu_version)
>   *
>   * This is used for FW_CFG_MAX_CPUS. See comments on 
> fw_cfg_arch_create().
>   */
> -x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
> -  ms->smp.max_cpus - 1) 
> + 1;
> -possible_cpus = mc->possible_cpu_arch_ids(ms);
> +props = ms->possible_cpus->cpus[ms->smp.max_cpus - 1].props;
>  
> +x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
> +  ms->smp.max_cpus - 1,
> +  props) + 1;
>  for (i = 0; i < ms->possible_cpus->len; i++) {
> +props = ms->possible_cpus->cpus[i].props;
>  ms->possible_cpus->cpus[i].arch_id =
> -x86_cpu_apic_id_from_index(x86ms, i);
> +x86_cpu_apic_id_from_index(x86ms, i, props);
>  }
>  
>  for (i = 0; i < ms->smp.cpus; i++) {
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index 7cb21e9c82..a800fc905f 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -221,10 +221,17 @@ static inline void x86_init_topo_ids(X86CPUTopoInfo 
> *topo_info,
>   * 'cpu_index' is a sequential, contiguous ID for the CPU.
>   */
>  static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo 
> *topo_info,
> - unsigned cpu_index)
> + unsigned cpu_index,
> + CpuInstanceProperties 
> props)
>  {
>  X86CPUTopoIDs topo_i

Re: [PATCH v6 01/10] qemu-img: Flush stdout before before potential stderr messages

[Max Reitz]

On 06.07.20 22:39, Eric Blake wrote:
> During 'qemu-img create ... 2>&1', if --quiet is not in force, we can
> end up with buffered I/O in stdout that was produced before failure,
> but which appears in output after failure.  This is confusing; the fix
> is to flush stdout prior to attempting anything that might produce an
> error message.  Several iotests demonstrate the resulting ordering
> change now that the merged outputs now reflect chronology.  (An even
> better fix would be to avoid printf from within block.c altogether,
> but that's much more invasive...)
> 
> Signed-off-by: Eric Blake 
> ---
>  block.c| 1 +
>  tests/qemu-iotests/049.out | 8 
>  tests/qemu-iotests/054.out | 2 +-
>  tests/qemu-iotests/079.out | 2 +-
>  tests/qemu-iotests/112.out | 4 ++--
>  tests/qemu-iotests/259.out | 2 +-
>  6 files changed, 10 insertions(+), 9 deletions(-)

282 also needs some treatment.

Max



signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name

[Markus Armbruster]

Eduardo Habkost  writes:

> On Fri, Jul 10, 2020 at 11:05:29AM +0200, Philippe Mathieu-Daudé wrote:
>> +Stefan for tracing PoV
>> 
>> On 7/9/20 9:48 PM, Eduardo Habkost wrote:
>> > On Fri, Jun 26, 2020 at 04:26:33PM +0200, Philippe Mathieu-Daudé 
>> > wrote:
>> >> On 6/26/20 1:00 PM, BALATON Zoltan wrote:
>> >>> On Fri, 26 Jun 2020, Philippe Mathieu-Daudé wrote:
>>  Suggested-by: Markus Armbruster 
>>  Signed-off-by: Philippe Mathieu-Daudé 
>>  ---
>>  hw/i2c/smbus_eeprom.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>>  diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>  index 879fd7c416..22ba7b20d4 100644
>>  --- a/hw/i2c/smbus_eeprom.c
>>  +++ b/hw/i2c/smbus_eeprom.c
>>  @@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice {
>>      uint8_t *init_data;
>>      uint8_t offset;
>>      bool accessed;
>>  +    char *description;
>>  } SMBusEEPROMDevice;
>> 
>>  static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>>  @@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev,
>>  Error **errp)
>>      smbus_eeprom_reset(dev);
>>      if (eeprom->init_data == NULL) {
>>          
>>  error_setg(errp, "init_data cannot be NULL");
>>  +        return;
>>      }
>>  +    eeprom->description =
>>  object_get_canonical_path_component(OBJECT(dev));
>>  }
>> 
>>  static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>> >>>
>> >>> What is this for? Shouldn't this description field be in some parent
>> >>> object and whatever wants to print it could use the
>> >>> object_get_canonical_path_component() as default value if it's not set
>> >>> instead of adding more boiler plate to every object?
>> >>
>> >> You are right, if we want to use this field generically, it should be
>> >> a static Object field. I'll defer that question to Eduardo/Markus.
>> > 
>> > I don't understand what's the question here.  What would be the
>> > purpose of a static Object field, and why it would be better than
>> > simply calling object_get_canonical_path_component() when you
>> > need it?
>> 
>> Because when reading a 8KB EEPROM with tracing enabled we end
>> up calling:
>> 
>> 8192 g_hash_table_iter_init()
>> 8192 g_hash_table_iter_next()
>> 8192 object_property_iter_init()
>> 8192 object_property_iter_next()
>> 8192 g_hash_table_add()
>> 8192 g_strdup()
>> 8192 g_free()
>> 
>> Which is why I added the SMBusEEPROMDeviceState::description
>> field, to call it once and cache it. But Zoltan realized this
>> could benefit all QOM objects, not this single one.
>> 
>> So the question is, is it OK to make this a cached field
>> in object_get_canonical_path_component()?
>
> That's what I was thinking of, but now I see that
> object_get_canonical_path_component() is an inconvenient API
> because it requires callers to g_free() the return value.

I agree.

> We could change object_get_canonical_path_component() to not
> require callers to call g_free(),

I'll look into that.  It would fix a memory leak I created because I
didn't expect object_get_canonical_path_component() to return a malloced
string.

>   or create a new mechanism to
> get the object name like you suggested (and then get rid of most
> of the existing uses of object_get_canonical_path_component()).

[...]

Re: [PATCH v2] docs/system/s390x: Improve the 3270 documentation

[Cornelia Huck]

On Mon, 13 Jul 2020 09:51:12 +0200
Thomas Huth  wrote:

> There is some additional information about the 3270 support in our Wiki
> at https://wiki.qemu.org/Features/3270 - so let's include this information
> into the main documentation now to have one single source of information
> (the Wiki page could later be removed).
> 
> While at it, I also shortened the lines of the first example a little bit.
> Otherwise they showed up with a horizontal scrollbar in my Firefox browser.
> 
> Signed-off-by: Thomas Huth 
> ---
>  v2:
>  - Added the changes that have been suggested by Cornelia
>  - Talk about "Linux kernel" instead of just saying "kernel", just in case.
> 
>  docs/system/s390x/3270.rst | 43 --
>  1 file changed, 37 insertions(+), 6 deletions(-)

(...)

> @@ -29,4 +39,25 @@ Example configuration
>  
>  systemctl start serial-getty@3270-tty1.service
>  
> -This should get you an addtional tty for logging into the guest.
> +  This should get you an addtional tty for logging into the guest.

While you're at it,

s/addtional/additional/

> +
> +* If you want to use the 3270 device as the Linux kernel console instead of
> +  an additional tty, you can also append ``conmode=3270 condev=000a`` to
> +  the guest's kernel command line. The kernel then should use the 3270 as
> +  console after the next boot.
> +
> +Restrictions
> +
> +
> +3270 support is very basic. In particular:
> +
> +* Only one 3270 device is supported.
> +
> +* It has only been tested with Linux guests and the x3270 emulator.
> +
> +* TLS/SSL is not supported.
> +
> +* Resizing on reattach is not supported.
> +
> +* Multiple commands in one inbound buffer (for example, when the reset key
> +  is pressed while the network is slow) are not supported.

Looks good.

Reviewed-by: Cornelia Huck 

(Feel free to send it yourself if you have some patches in your queue, I
don't have anything for s390x right now.)

Re: [PATCH v5 02/12] python/machine.py: Close QMP socket in cleanup

[Philippe Mathieu-Daudé]

On 7/10/20 7:06 AM, John Snow wrote:
> It's not important to do this before waiting for the process to exit, so
> it can be done during generic post-shutdown cleanup.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v5 05/12] python/machine.py: Prohibit multiple shutdown() calls

[Philippe Mathieu-Daudé]

On 7/10/20 7:06 AM, John Snow wrote:
> If the VM is not launched, don't try to shut it down. As a change,
> _post_shutdown now unconditionally also calls _early_cleanup in order to
> offer comprehensive object cleanup in failure cases.
> 
> As a courtesy, treat it as a NOP instead of rejecting it as an
> error. This is slightly nicer for acceptance tests where vm.shutdown()
> is issued unconditionally in tearDown callbacks.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v5 07/12] python/machine.py: Make wait() call shutdown()

[Philippe Mathieu-Daudé]

On 7/10/20 7:06 AM, John Snow wrote:
> At this point, shutdown(has_quit=True) and wait() do essentially the
> same thing; they perform cleanup without actually instructing QEMU to
> quit.
> 
> Define one in terms of the other.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v5 06/12] python/machine.py: Add a configurable timeout to shutdown()

[Philippe Mathieu-Daudé]

On 7/10/20 7:06 AM, John Snow wrote:
> Three seconds is hardcoded. Use it as a default parameter instead, and use 
> that
> value for both waits that may occur in the function.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v5 11/12] python/machine.py: re-add sigkill warning suppression

[Philippe Mathieu-Daudé]

On 7/10/20 7:06 AM, John Snow wrote:
> If the user kills QEMU on purpose, we don't need to warn them about that
> having happened: they know already.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 

Good idea :)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v5 12/12] python/machine.py: change default wait timeout to 3 seconds

[Philippe Mathieu-Daudé]

On 7/10/20 7:06 AM, John Snow wrote:
> Machine.wait() does not appear to be used except in the acceptance tests,
> and an infinite timeout by default in a test suite is not the most helpful.
> 
> Change it to 3 seconds, like the default shutdown timeout.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 02d66e3cff..d08a8e4a6e 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -472,12 +472,12 @@ def kill(self):
>  """
>  self.shutdown(hard=True)
>  
> -def wait(self, timeout: Optional[int] = None) -> None:
> +def wait(self, timeout: Optional[int] = 3) -> None:
>  """
>  Wait for the VM to power off and perform post-shutdown cleanup.
>  
>  :param timeout: Optional timeout in seconds.
> -Default None, an infinite wait.
> +Default 3 seconds, A value of None is an infinite 
> wait.
>  """
>  self.shutdown(has_quit=True, timeout=timeout)
>  
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 01/12] target/arm/kvm: Remove superfluous break

[Thomas Huth]

On 13/07/2020 11.03, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove superfluous break.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé  
> ---
>  target/arm/kvm64.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 1169237..ef1e960 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -330,7 +330,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
>  switch (type) {
>  case GDB_BREAKPOINT_HW:
>  return delete_hw_breakpoint(addr);
> -break;
>  case GDB_WATCHPOINT_READ:
>  case GDB_WATCHPOINT_WRITE:
>  case GDB_WATCHPOINT_ACCESS:
> 

Reviewed-by: Thomas Huth 

Re: [PATCH v5 13/16] python/machine.py: fix _popen access

[Philippe Mathieu-Daudé]

On 7/10/20 7:22 AM, John Snow wrote:
> As always, Optional[T] causes problems with unchecked access. Add a
> helper that asserts the pipe is present before we attempt to talk with
> it.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 16 +++-
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 04/12] scsi: Remove superfluous breaks

[Thomas Huth]

On 13/07/2020 11.04, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove superfluous breaks, as there is a "return" before them.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé  
> ---
>  scsi/utils.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/scsi/utils.c b/scsi/utils.c
> index c50e81f..b37c283 100644
> --- a/scsi/utils.c
> +++ b/scsi/utils.c
> @@ -32,17 +32,13 @@ uint32_t scsi_cdb_xfer(uint8_t *buf)
>  switch (buf[0] >> 5) {
>  case 0:
>  return buf[4];
> -break;
>  case 1:
>  case 2:
>  return lduw_be_p(&buf[7]);
> -break;
>  case 4:
>  return ldl_be_p(&buf[10]) & 0xULL;
> -break;
>  case 5:
>  return ldl_be_p(&buf[6]) & 0xULL;
> -break;
>  default:
>  return -1;
>  }
> 

Reviewed-by: Thomas Huth 

Re: [PATCH 02/12] target/ppc: Remove superfluous breaks

[Thomas Huth]

On 13/07/2020 11.03, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove superfluous breaks, as there is a "return" before them.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé  
> ---
>  target/ppc/misc_helper.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index 55b68d1..e43a3b4 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -234,25 +234,20 @@ target_ulong helper_clcs(CPUPPCState *env, uint32_t arg)
>  case 0x0CUL:
>  /* Instruction cache line size */
>  return env->icache_line_size;
> -break;
>  case 0x0DUL:
>  /* Data cache line size */
>  return env->dcache_line_size;
> -break;
>  case 0x0EUL:
>  /* Minimum cache line size */
>  return (env->icache_line_size < env->dcache_line_size) ?
>  env->icache_line_size : env->dcache_line_size;
> -break;
>  case 0x0FUL:
>  /* Maximum cache line size */
>  return (env->icache_line_size > env->dcache_line_size) ?
>  env->icache_line_size : env->dcache_line_size;
> -break;
>  default:
>  /* Undefined */
>  return 0;
> -break;
>  }
>  }
>  
> 

Reviewed-by: Thomas Huth 

Re: [PATCH 06/12] migration/migration.c: Remove superfluous breaks

[Thomas Huth]

On 13/07/2020 11.04, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove superfluous breaks, as there is a "return" before them.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  migration/migration.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 92e44e0..2fd5fbb 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -985,7 +985,6 @@ static void fill_source_migration_info(MigrationInfo 
> *info)
>  /* no migration has happened ever */
>  /* do not overwrite destination migration status */
>  return;
> -break;
>  case MIGRATION_STATUS_SETUP:
>  info->has_status = true;
>  info->has_total_time = false;
> @@ -1104,7 +1103,6 @@ static void 
> fill_destination_migration_info(MigrationInfo *info)
>  switch (mis->state) {
>  case MIGRATION_STATUS_NONE:
>  return;
> -break;
>  case MIGRATION_STATUS_SETUP:
>  case MIGRATION_STATUS_CANCELLING:
>  case MIGRATION_STATUS_CANCELLED:
> 

Reviewed-by: Thomas Huth 

Re: [PATCH 03/12] tcg/riscv: Remove superfluous breaks

[Thomas Huth]

On 13/07/2020 11.04, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove superfluous breaks, as there is a "return" before them.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé  
> ---
>  tcg/riscv/tcg-target.inc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/tcg/riscv/tcg-target.inc.c b/tcg/riscv/tcg-target.inc.c
> index 2bc0ba7..3c11ab8 100644
> --- a/tcg/riscv/tcg-target.inc.c
> +++ b/tcg/riscv/tcg-target.inc.c
> @@ -502,10 +502,8 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int 
> type,
>  break;
>  case R_RISCV_JAL:
>  return reloc_jimm20(code_ptr, (tcg_insn_unit *)value);
> -break;
>  case R_RISCV_CALL:
>  return reloc_call(code_ptr, (tcg_insn_unit *)value);
> -break;
>  default:
>  tcg_abort();
>  }
> 

Reviewed-by: Thomas Huth 

Re: [PATCH 07/12] vnc: Remove the superfluous break

[Thomas Huth]

On 13/07/2020 11.04, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove the superfluous break, as there is a "return" before.
> 
> Signed-off-by: Liao Pingfang a
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  ui/vnc-enc-tight.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
> index 1e08518..cebd358 100644
> --- a/ui/vnc-enc-tight.c
> +++ b/ui/vnc-enc-tight.c
> @@ -1125,7 +1125,6 @@ static int send_palette_rect(VncState *vs, int x, int y,
>  }
>  default:
>  return -1; /* No palette for 8bits colors */
> -break;
>  }
>  bytes = w * h;
>  vs->tight->tight.offset = bytes;
> 

Reviewed-by: Thomas Huth 

Re: [PATCH v5 16/16] python/qemu: Add mypy type annotations

[Philippe Mathieu-Daudé]

On 7/10/20 7:22 AM, John Snow wrote:
> These should all be purely annotations with no changes in behavior at
> all. You need to be in the python folder, but you should be able to
> confirm that these annotations are correct (or at least self-consistent)
> by running `mypy --strict qemu`.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/accel.py   |  8 ++--
>  python/qemu/machine.py | 94 --
>  python/qemu/qmp.py | 44 +++-
>  python/qemu/qtest.py   | 26 +++-
>  4 files changed, 98 insertions(+), 74 deletions(-)
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 08/12] block/vmdk: Remove superfluous breaks

[Thomas Huth]

On 13/07/2020 11.05, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove superfluous breaks, as there is a "return" before them.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  block/vmdk.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 28cec50..8f222e3 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1053,14 +1053,11 @@ static int vmdk_open_sparse(BlockDriverState *bs, 
> BdrvChild *file, int flags,
>  switch (magic) {
>  case VMDK3_MAGIC:
>  return vmdk_open_vmfs_sparse(bs, file, flags, errp);
> -break;
>  case VMDK4_MAGIC:
>  return vmdk_open_vmdk4(bs, file, flags, options, errp);
> -break;
>  default:
>  error_setg(errp, "Image not in VMDK format");
>  return -EINVAL;
> -break;
>  }
>  }
>  
> 

Reviewed-by: Thomas Huth 

Re: [PATCH v5 12/16] python/machine.py: Add _qmp access shim

[Philippe Mathieu-Daudé]

On 7/10/20 7:22 AM, John Snow wrote:
> Like many other Optional[] types, it's not always a given that this
> object will be set. Wrap it in a type-shim that raises a meaningful
> error and will always return a concrete type.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [RFC PATCH 1/5] hw/acpi/pcihp: Introduce find_host()

[Igor Mammedov]

On Thu,  9 Jul 2020 00:46:11 +0200
Julia Suvorova  wrote:

> Returns the current host bus with ACPI PCI hot-plug support: q35 or i440fx.
> 
> Signed-off-by: Julia Suvorova 
> ---
>  hw/i386/acpi-build.h |  2 ++
>  hw/acpi/pcihp.c  | 13 +
>  hw/i386/acpi-build.c |  2 +-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 74df5fc612..0696b4e48d 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -7,4 +7,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>  
>  void acpi_setup(void);
>  
> +Object *acpi_get_i386_pci_host(void);
> +
>  #endif
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index d42906ea19..3d4ee3af72 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -33,10 +33,12 @@
>  #include "hw/acpi/acpi.h"
>  #include "exec/address-spaces.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/pci/pci_host.h"
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "qom/qom-qobject.h"
>  #include "trace.h"
> +#include "hw/i386/acpi-build.h"
>  
>  #define ACPI_PCIHP_ADDR 0xae00
>  #define ACPI_PCIHP_SIZE 0x0014
> @@ -86,6 +88,17 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>  return bsel_alloc;
>  }
>  
> +static PCIBus *find_host(void)
> +{
> +Object *obj = acpi_get_i386_pci_host();
> +
> +if (obj) {
> +return PCI_HOST_BRIDGE(obj)->bus;
> +}
> +
> +return NULL;
> +}

My guess you are adding it for 5/5, with a function name a bit off
compared to what you are doing (probably you've tried to reuse find_i440fx() 
idea)

I'd just make acpi_get_i386_pci_host() public, drop find_host and use

 host = acpi_get_i386_pci_host()
 bus = PCI_HOST_BRIDGE(pci_host)->bus

like it's done elsewhere

>  static void acpi_set_pci_info(void)
>  {
>  static bool bsel_is_set;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 900f786d08..11c598f955 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -270,7 +270,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>   * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
>   * On i386 arch we only have two pci hosts, so we can look only for them.
>   */
> -static Object *acpi_get_i386_pci_host(void)
> +Object *acpi_get_i386_pci_host(void)
>  {
>  PCIHostState *host;
>  

Re: [PATCH 12/12] target/cris: Remove superfluous breaks

[Thomas Huth]

On 13/07/2020 11.05, Yi Wang wrote:
> From: Liao Pingfang 
> 
> Remove superfluous breaks, as there is a "return" before them.
> 
> Signed-off-by: Liao Pingfang 
> Signed-off-by: Yi Wang 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  target/cris/translate.c | 7 +++
>  target/cris/translate_v10.inc.c | 2 --
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index aaa46b5..64a478b 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -1178,12 +1178,11 @@ static inline void t_gen_zext(TCGv d, TCGv s, int 
> size)
>  static char memsize_char(int size)
>  {
>  switch (size) {
> -case 1: return 'b';  break;
> -case 2: return 'w';  break;
> -case 4: return 'd';  break;
> +case 1: return 'b';
> +case 2: return 'w';
> +case 4: return 'd';
>  default:
>  return 'x';
> -break;
>  }
>  }
>  #endif
> diff --git a/target/cris/translate_v10.inc.c b/target/cris/translate_v10.inc.c
> index ae34a0d..7f38fd2 100644
> --- a/target/cris/translate_v10.inc.c
> +++ b/target/cris/translate_v10.inc.c
> @@ -1026,10 +1026,8 @@ static unsigned int dec10_ind(CPUCRISState *env, 
> DisasContext *dc)
>  switch (dc->opcode) {
>  case CRISV10_IND_MOVE_M_R:
>  return dec10_ind_move_m_r(env, dc, size);
> -break;
>  case CRISV10_IND_MOVE_R_M:
>  return dec10_ind_move_r_m(dc, size);
> -break;
>  case CRISV10_IND_CMP:
>  LOG_DIS("cmp size=%d op=%d %d\n",  size, dc->src, dc->dst);
>  cris_cc_mask(dc, CC_MASK_NZVC);
> 

Reviewed-by: Thomas Huth