Re: [PATCH 1/3] qemu: implementation of transient option for qcow2 file

[Masayoshi Mizuma]

On Thu, Aug 06, 2020 at 12:09:20PM +0200, Peter Krempa wrote:
> On Sun, Jul 19, 2020 at 22:56:50 -0400, Masayoshi Mizuma wrote:
> > On Sat, Jul 18, 2020 at 08:06:00AM +0200, Peter Krempa wrote:
> > > On Thu, Jul 16, 2020 at 20:55:29 -0400, Masayoshi Mizuma wrote:
> > > > Thank you for your review.
> > > > 
> > > > On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
> > > > > On 7/7/20 2:12 AM, Peter Krempa wrote:
> > > > > 
> > > > > > You can install a qcow2 overlay on top of a raw file too. IMO the
> > > > > > implications of using  allow that.
> > > > > > 
> > > > > > As said above I'd strongly prefer if the overlay is created in qemu
> > > > > > using the blockdev-create blockjob (there is already infrastructure 
> > > > > > in
> > > > > > libvirt to achieve that).
> > > > > 
> > > > > Agreed.  At this point, any time we call out to qemu-img as a separate
> > > > > process, we are probably doing it wrong.
> > > > 
> > > > Got it. I'm thinking about the procedure such as followings.
> > > > Does that make sense?
> > > > 
> > > >   1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), 
> > > >  and connect it.
> > > 
> > > Starting a new qemu process just to format an image is extreme overkill
> > > and definitely not what we want to do.
> > 
> > I see, thanks.
> > 
> > > 
> > > >   2) Setup the transient disk with 
> > > > qemuDomainPrepareStorageSourceBlockdev(),
> > > >  qemuBlockStorageSourceAttachApplyStorage(), 
> > > > qemuBlockStorageSourceCreateGetFormatProps()
> > > >  and something...
> > > > 
> > > >   3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then
> > > >  close the monitor.
> > > 
> > > These two steps should be exectued in the qemu process which already
> > > will run the VM prior to starting the guest CPUs.
> > > 
> > > >   4) Switch the original disk to the transient disk.
> > > > 
> > > >   5) Build the blockdev argument for qemu.
> > > 
> > > And instead of this step, you use the external snapshot infrastructure
> > > to install the overlays via 'blockdev-snapshot' QMP command
> > 
> > OK. I suppose qemuDomainSnapshotDiskPrepare() and
> > qemuDomainSnapshotDiskUpdateSource() maybe helpful to implement the
> > setup steps of transient disk.
> > 
> > > 
> > > > 
> > > >   6) Run qemu
> > > 
> > > And instead of this the VM cpus will be started.
> > 
> > Got it, I think the appropriate place is after virCommandRun() at 
> > qemuProcessLaunch(),
> > and before qemuProcessFinishStartup().
> > 
> > > 
> > > 
> > > The above steps require factoring out snapshot code a bit. I have a few
> > > patches in that direction so I'll try posting them next week hopefully.
> 
> Sorry this took longer than expected, but we were dealing with the build
> system change.
> 
> The refactor is here:
> 
> https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html
> 
> You can now create an equivalent of 'qemuSnapshotDiskPrepare' which will
> go through the disks and find the 'transient' ones. It will then create
> snapshot data by a call to 'qemuSnapshotDiskPrepareOne' with a faked
> snapshot disk object.
> 
> 'qemuSnapshotDiskPrepareOne' ensures that the files are created and
> formatted properly.
> 
> You then use same algorithm as 'qemuSnapshotCreateDiskActive'
> (e.g. by extracting the common internals (basically everything except
> call to 'qemuSnapshotDiskPrepare') into a separate function) and reuse
> it when starting the VM as you've described above.
> 
> Note that all of the above can work only when QEMU_CAPS_BLOCKDEV is
> supported.
> 
> The caveats/limitations with blockjobs and snapshots still apply though.
> It depends on how you approach it. It's okay to limit/block the features
> if transient disk is used. Alternatively you can implement some form of
> private data to mark which image was transient and allow those
> operations.

Thank you for the helpful advice and the patches!
I'll try to implement the transient disk procedure with the refactor.

Thanks!
Masa

[PATCH 1/2] block/rbd: remove runtime_opts

[John Snow]

This saw its last use in 4bfb274165ba.

Signed-off-by: John Snow 
---
 block/rbd.c | 42 --
 1 file changed, 42 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 688074c64b7..171c67e3a01 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -341,48 +341,6 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
 }
 }
 
-static QemuOptsList runtime_opts = {
-.name = "rbd",
-.head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
-.desc = {
-{
-.name = "pool",
-.type = QEMU_OPT_STRING,
-.help = "Rados pool name",
-},
-{
-.name = "namespace",
-.type = QEMU_OPT_STRING,
-.help = "Rados namespace name in the pool",
-},
-{
-.name = "image",
-.type = QEMU_OPT_STRING,
-.help = "Image name in the pool",
-},
-{
-.name = "conf",
-.type = QEMU_OPT_STRING,
-.help = "Rados config file location",
-},
-{
-.name = "snapshot",
-.type = QEMU_OPT_STRING,
-.help = "Ceph snapshot name",
-},
-{
-/* maps to 'id' in rados_create() */
-.name = "user",
-.type = QEMU_OPT_STRING,
-.help = "Rados id name",
-},
-/*
- * server.* extracted manually, see qemu_rbd_mon_host()
- */
-{ /* end of list */ }
-},
-};
-
 /* FIXME Deprecate and remove keypairs or make it available in QMP. */
 static int qemu_rbd_do_create(BlockdevCreateOptions *options,
   const char *keypairs, const char 
*password_secret,
-- 
2.26.2

[PATCH 2/2] block/qcow: remove runtime opts

[John Snow]

Introduced by d85f4222b468,
These were seemingly never used at all.

Signed-off-by: John Snow 
---
 block/qcow.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index e514a86fe5e..f8919a44d19 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -105,15 +105,6 @@ static int qcow_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 return 0;
 }
 
-static QemuOptsList qcow_runtime_opts = {
-.name = "qcow",
-.head = QTAILQ_HEAD_INITIALIZER(qcow_runtime_opts.head),
-.desc = {
-BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET("encrypt."),
-{ /* end of list */ }
-},
-};
-
 static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
-- 
2.26.2

[PATCH 0/2] block: remove stale runtime_opts

[John Snow]

John Snow (2):
  block/rbd: remove runtime_opts
  block/qcow: remove runtime opts

 block/qcow.c |  9 -
 block/rbd.c  | 42 --
 2 files changed, 51 deletions(-)

-- 
2.26.2

[PATCH v13 04/11] 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 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 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 d4a9974..b447344 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 v13 05/11] qcow2_format.py: Dump bitmap directory information

[Andrey Shinkevich]

Read and dump entries from the bitmap directory of QCOW2 image.

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 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/303.out | 18 +++
 tests/qemu-iotests/qcow2_format.py | 47 ++
 2 files changed, 65 insertions(+)

diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index 8d7973c..038ba93 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -58,3 +58,21 @@ reserved320
 bitmap_directory_size 0x40
 bitmap_directory_offset   0x9d
 
+Bitmap name   bitmap-1
+bitmap_table_offset   0x9b
+bitmap_table_size 1
+flags 0x2 (['auto'])
+type  1
+granularity_bits  15
+name_size 8
+extra_data_size   0
+
+Bitmap name   bitmap-2
+bitmap_table_offset   0x9c
+bitmap_table_size 1
+flags 0x0 ([])
+type  1
+granularity_bits  16
+name_size 8
+extra_data_size   0
+
diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index b447344..05a8aa9 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -134,6 +134,53 @@ class Qcow2BitmapExt(Qcow2Struct):
 tail = struct.calcsize(self.fmt) % 8
 if tail:
 fd.seek(8 - tail, 1)
+position = fd.tell()
+self.read_bitmap_directory(fd)
+fd.seek(position)
+
+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 v13 08/11] 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 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/qcow2.py| 18 ++
 tests/qemu-iotests/qcow2_format.py |  4 ++--
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index 0910e6a..77ca59c 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -26,16 +26,19 @@ from qcow2_format import (
 )
 
 
+is_json = False
+
+
 def cmd_dump_header(fd):
 h = QcowHeader(fd)
-h.dump()
+h.dump(is_json)
 print()
-h.dump_extensions()
+h.dump_extensions(is_json)
 
 
 def cmd_dump_header_exts(fd):
 h = QcowHeader(fd)
-h.dump_extensions()
+h.dump_extensions(is_json)
 
 
 def cmd_set_header(fd, name, value):
@@ -151,11 +154,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__':
@@ -163,4 +169,8 @@ if __name__ == '__main__':
 usage()
 sys.exit(1)
 
+is_json = '-j' in sys.argv
+if is_json:
+sys.argv.remove('-j')
+
 main(sys.argv[1], sys.argv[2], sys.argv[3:])
diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 574249b..de0adcb 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, is_json=False):
 for f in self.fields:
 value = self.__dict__[f[2]]
 if isinstance(f[1], str):
@@ -408,7 +408,7 @@ class QcowHeader(Qcow2Struct):
 buf = buf[0:header_bytes-1]
 fd.write(buf)
 
-def dump_extensions(self):
+def dump_extensions(self, is_json=False):
 for ex in self.extensions:
 print('Header extension:')
 ex.dump()
-- 
1.8.3.1

[PATCH v13 00/11] iotests: Dump QCOW2 dirty bitmaps metadata

[Andrey Shinkevich]

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

v13:
  01: Bitmaps are added without launching VM (suggested by Eric).
  The code amendments suggested by Vladimir.
  07: Bitmap table entry size zeroed up for all types but serialized.
  09: The extra dict variables removed. to_dict() renamed to to_json().
  The to_json() added to the class Qcow2BitmapTable. (By Vladimir).


Andrey Shinkevich (11):
  iotests: add test for QCOW2 header dump
  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
  iotests: dump QCOW2 header in JSON in #303

 tests/qemu-iotests/303 |  63 +++
 tests/qemu-iotests/303.out | 158 +++
 tests/qemu-iotests/group   |   1 +
 tests/qemu-iotests/qcow2.py|  18 +++-
 tests/qemu-iotests/qcow2_format.py | 215 ++---
 5 files changed, 434 insertions(+), 21 deletions(-)
 create mode 100755 tests/qemu-iotests/303
 create mode 100644 tests/qemu-iotests/303.out

-- 
1.8.3.1

[PATCH v13 10/11] 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": [
{
"type": "serialized",
"offset": 655360
},
...

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

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 5a298b2..8adc995 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, 'to_json'):
+return obj.to_json()
+else:
+return json.JSONEncoder.default(self, obj)
 
 
 class Qcow2Field:
@@ -110,6 +119,10 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
  for i, field in enumerate(self.fields))
 
 def dump(self, is_json=False):
+if is_json:
+print(json.dumps(self.to_json(), indent=4, cls=ComplexEncoder))
+return
+
 for f in self.fields:
 value = self.__dict__[f[2]]
 if isinstance(f[1], str):
@@ -445,6 +458,10 @@ class QcowHeader(Qcow2Struct):
 fd.write(buf)
 
 def dump_extensions(self, is_json=False):
+if is_json:
+print(json.dumps(self.extensions, indent=4, cls=ComplexEncoder))
+return
+
 for ex in self.extensions:
 print('Header extension:')
 ex.dump()
-- 
1.8.3.1

[PATCH v13 09/11] 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, add the to_dict() method to classes that returns a dictionary
with desired fields and their values. Extend it in subclass when
necessary to print the final dictionary in the JSON output which
follows.

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

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index de0adcb..5a298b2 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -119,6 +119,9 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
 
 print('{:<25} {}'.format(f[2], value_str))
 
+def to_json(self):
+return dict((f[2], self.__dict__[f[2]]) for f in self.fields)
+
 
 class Qcow2BitmapExt(Qcow2Struct):
 
@@ -151,6 +154,11 @@ class Qcow2BitmapExt(Qcow2Struct):
 print()
 entry.dump()
 
+def to_json(self):
+fields_dict = super().to_json()
+fields_dict['bitmap_directory'] = self.bitmap_directory
+return fields_dict
+
 
 class Qcow2BitmapDirEntry(Qcow2Struct):
 
@@ -189,6 +197,14 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 super(Qcow2BitmapDirEntry, self).dump()
 self.bitmap_table.dump()
 
+def to_json(self):
+# Put the name ahead of the dict
+return {
+'name': self.name,
+**super().to_json(),
+'bitmap_table': self.bitmap_table
+}
+
 
 class Qcow2BitmapTableEntry(Qcow2Struct):
 
@@ -214,6 +230,10 @@ class Qcow2BitmapTableEntry(Qcow2Struct):
 else:
 self.type = 'all-zeroes'
 
+def to_json(self):
+return {'type': self.type, 'offset': self.offset,
+'reserved': self.reserved}
+
 
 class Qcow2BitmapTable:
 
@@ -234,6 +254,9 @@ class Qcow2BitmapTable:
 size = 0
 print(f'{i:<14} {entry.type:<15} {size:<12} {entry.offset}')
 
+def to_json(self):
+return self.entries
+
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
 
@@ -249,6 +272,9 @@ class QcowHeaderExtension(Qcow2Struct):
 0x44415441: 'Data file'
 }
 
+def to_json(self):
+return self.mapping.get(self.value, "")
+
 fields = (
 ('u32', Magic, 'magic'),
 ('u32', '{}', 'length')
@@ -311,6 +337,16 @@ class QcowHeaderExtension(Qcow2Struct):
 else:
 self.obj.dump()
 
+def to_json(self):
+# Put the name ahead of the dict
+res = {'name': self.Magic(self.magic), **super().to_json()}
+if self.obj is not None:
+res['data'] = self.obj
+else:
+res['data_str'] = self.data_str
+
+return res
+
 @classmethod
 def create(cls, magic, data):
 return QcowHeaderExtension(magic, len(data), data)
-- 
1.8.3.1

[PATCH v13 02/11] 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 v13 03/11] 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 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/qcow2_format.py | 36 ++--
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2f3681b..d4a9974 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -113,6 +113,11 @@ class Qcow2BitmapExt(Qcow2Struct):
 ('u64', '{:#x}', 'bitmap_directory_offset')
 )
 
+def __init__(self, fd):
+super().__init__(fd=fd)
+tail = struct.calcsize(self.fmt) % 8
+if tail:
+fd.seek(8 - tail, 1)
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
 
@@ -161,21 +166,24 @@ class QcowHeaderExtension(Qcow2Struct):
 else:
 assert all(v is None for v in (magic, length, data))
 super().__init__(fd=fd)
-padded = (self.length + 7) & ~7
-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(fd=fd)
+self.data = None
+else:
+padded = (self.length + 7) & ~7
+self.data = fd.read(padded)
+assert self.data is not None
+self.obj = None
+
+if 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:
-self.obj = None
 
 def dump(self):
 super().dump()
-- 
1.8.3.1

[PATCH v13 11/11] iotests: dump QCOW2 header in JSON in #303

[Andrey Shinkevich]

Extend the test case #303 by dumping QCOW2 image metadata in JSON
format.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/303 |  3 ++
 tests/qemu-iotests/303.out | 76 ++
 2 files changed, 79 insertions(+)

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index e9accdc..6c21774 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -58,3 +58,6 @@ add_bitmap(1, 0, 6, False)
 add_bitmap(2, 6, 8, True)
 dump = ['qcow2.py', disk, 'dump-header']
 subprocess.run(dump)
+# Dump the metadata in JSON format
+dump.append('-j')
+subprocess.run(dump)
diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index 70828e0..7fa1ede 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -80,3 +80,79 @@ extra_data_size   0
 Bitmap table   typesize offset
 0  all-zeroes  00
 
+{
+"magic": 1363560955,
+"version": 3,
+"backing_file_offset": 0,
+"backing_file_size": 0,
+"cluster_bits": 16,
+"size": 10485760,
+"crypt_method": 0,
+"l1_size": 1,
+"l1_table_offset": 196608,
+"refcount_table_offset": 65536,
+"refcount_table_clusters": 1,
+"nb_snapshots": 0,
+"snapshot_offset": 0,
+"incompatible_features": 0,
+"compatible_features": 0,
+"autoclear_features": 1,
+"refcount_order": 4,
+"header_length": 112
+}
+
+[
+{
+"name": "Feature table",
+"magic": 1745090647,
+"length": 336,
+"data_str": ""
+},
+{
+"name": "Bitmaps",
+"magic": 595929205,
+"length": 24,
+"data": {
+"nb_bitmaps": 2,
+"reserved32": 0,
+"bitmap_directory_size": 64,
+"bitmap_directory_offset": 10289152,
+"bitmap_directory": [
+{
+"name": "bitmap-1",
+"bitmap_table_offset": 10158080,
+"bitmap_table_size": 1,
+"flags": 2,
+"type": 1,
+"granularity_bits": 15,
+"name_size": 8,
+"extra_data_size": 0,
+"bitmap_table": [
+{
+"type": "serialized",
+"offset": 10092544,
+"reserved": 0
+}
+]
+},
+{
+"name": "bitmap-2",
+"bitmap_table_offset": 10223616,
+"bitmap_table_size": 1,
+"flags": 0,
+"type": 1,
+"granularity_bits": 16,
+"name_size": 8,
+"extra_data_size": 0,
+"bitmap_table": [
+{
+"type": "all-zeroes",
+"offset": 0,
+"reserved": 0
+}
+]
+}
+]
+}
+}
+]
-- 
1.8.3.1

[PATCH v13 07/11] qcow2_format.py: Dump bitmap table serialized entries

[Andrey Shinkevich]

Add bitmap table information to the QCOW2 metadata dump.

Bitmap name   bitmap-1
...
Bitmap table   typesize offset
0  serialized  6553610092544
1  all-zeroes  00
2  all-zeroes  00

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/303.out |  4 +++
 tests/qemu-iotests/qcow2_format.py | 50 ++
 2 files changed, 54 insertions(+)

diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index 038ba93..70828e0 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -66,6 +66,8 @@ type  1
 granularity_bits  15
 name_size 8
 extra_data_size   0
+Bitmap table   typesize offset
+0  serialized  6553610092544
 
 Bitmap name   bitmap-2
 bitmap_table_offset   0x9c
@@ -75,4 +77,6 @@ type  1
 granularity_bits  16
 name_size 8
 extra_data_size   0
+Bitmap table   typesize offset
+0  all-zeroes  00
 
diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index ca0d350..574249b 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -175,6 +175,10 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 entry_raw_size = self.bitmap_dir_entry_raw_size()
 padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
 fd.seek(padding, 1)
+self.bitmap_table = Qcow2BitmapTable(fd=fd,
+ offset=self.bitmap_table_offset,
+ nb_entries=self.bitmap_table_size,
+ cluster_size=self.cluster_size)
 
 def bitmap_dir_entry_raw_size(self):
 return struct.calcsize(self.fmt) + self.name_size + \
@@ -183,6 +187,52 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 def dump(self):
 print(f'{"Bitmap name":<25} {self.name}')
 super(Qcow2BitmapDirEntry, self).dump()
+self.bitmap_table.dump()
+
+
+class Qcow2BitmapTableEntry(Qcow2Struct):
+
+fields = (
+('u64',  '{}', 'entry'),
+)
+
+BME_TABLE_ENTRY_RESERVED_MASK = 0xff0001fe
+BME_TABLE_ENTRY_OFFSET_MASK = 0x00fffe00
+BME_TABLE_ENTRY_FLAG_ALL_ONES = 1
+
+def __init__(self, fd):
+super().__init__(fd=fd)
+self.reserved = self.entry & self.BME_TABLE_ENTRY_RESERVED_MASK
+self.offset = self.entry & self.BME_TABLE_ENTRY_OFFSET_MASK
+if self.offset:
+if self.entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+self.type = 'invalid'
+else:
+self.type = 'serialized'
+elif self.entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+self.type = 'all-ones'
+else:
+self.type = 'all-zeroes'
+
+
+class Qcow2BitmapTable:
+
+def __init__(self, fd, offset, nb_entries, cluster_size):
+self.cluster_size = cluster_size
+position = fd.tell()
+fd.seek(offset)
+self.entries = [Qcow2BitmapTableEntry(fd) for _ in range(nb_entries)]
+fd.seek(position)
+
+def dump(self):
+bitmap_table = enumerate(self.entries)
+print(f'{"Bitmap table":<14} {"type":<15} {"size":<12} {"offset"}')
+for i, entry in bitmap_table:
+if entry.type == 'serialized':
+size = self.cluster_size
+else:
+size = 0
+print(f'{i:<14} {entry.type:<15} {size:<12} {entry.offset}')
 
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
-- 
1.8.3.1

[PATCH v13 01/11] iotests: add test for QCOW2 header dump

[Andrey Shinkevich]

The simple script creates a QCOW2 image and fills it with some data.
Two bitmaps are created as well. Then the script reads the image header
with extensions from the disk by running the script qcow2.py and dumps
the information to the output. Other entities, such as snapshots, may
be added to the test later.

Suggested-by: Eric Blake 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/303 | 60 ++
 tests/qemu-iotests/303.out | 60 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 121 insertions(+)
 create mode 100755 tests/qemu-iotests/303
 create mode 100644 tests/qemu-iotests/303.out

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
new file mode 100755
index 000..e9accdc
--- /dev/null
+++ b/tests/qemu-iotests/303
@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+#
+# Test for dumping of qcow2 image metadata
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+import subprocess
+from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+disk = file_path('disk')
+chunk = 1024 * 1024
+
+
+def create_bitmap(bitmap_number, disabled):
+granularity = 1 << (14 + bitmap_number)
+bitmap_name = 'bitmap-' + str(bitmap_number)
+args = ['bitmap', '--add', '-g', f'{granularity}', '-f', iotests.imgfmt,
+disk, bitmap_name]
+if disabled:
+args.append('--disable')
+
+iotests.qemu_img_pipe(*args)
+
+
+def write_to_disk(offset, size):
+write = f'write {offset} {size}'
+log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
+
+
+def add_bitmap(num, begin, end, disabled):
+log(f'Add bitmap {num}')
+create_bitmap(num, disabled)
+for i in range(begin, end):
+write_to_disk((i) * chunk, chunk)
+log('')
+
+
+qemu_img_create('-f', iotests.imgfmt, disk, '10M')
+
+add_bitmap(1, 0, 6, False)
+add_bitmap(2, 6, 8, True)
+dump = ['qcow2.py', disk, 'dump-header']
+subprocess.run(dump)
diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
new file mode 100644
index 000..8d7973c
--- /dev/null
+++ b/tests/qemu-iotests/303.out
@@ -0,0 +1,60 @@
+Add bitmap 1
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 4194304
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 5242880
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
+Add bitmap 2
+wrote 1048576/1048576 bytes at offset 6291456
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 7340032
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
+magic 0x514649fb
+version   3
+backing_file_offset   0x0
+backing_file_size 0x0
+cluster_bits  16
+size  10485760
+crypt_method  0
+l1_size   1
+l1_table_offset   0x3
+refcount_table_offset 0x1
+refcount_table_clusters   1
+nb_snapshots  0
+snapshot_offset   0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[0]
+refcount_order4
+header_length 112
+
+Header extension:
+magic 0x6803f857 (Feature table)
+length336
+data  
+
+Header extension:
+magic 0x23852875 (Bitmaps)
+length24
+nb_bitmaps2
+reserved320
+bitmap_directory_size 0x40
+bitmap_directory_offset   0x9d
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 025ed52..7e12e1f 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -309,3 +309,4 @@
 299 auto quick
 301 backing quick
 302 quick
+303 rw quick
-- 

Re: [PATCH for-5.1?] block/block-copy: always align copied region to cluster size

[Eric Blake]

On 8/6/20 8:57 AM, Stefan Reiter wrote:

Since commit 42ac214406e0 (block/block-copy: refactor task creation)
block_copy_task_create calculates the area to be copied via
bdrv_dirty_bitmap_next_dirty_area, but that can return an unaligned byte
count if the backing image's last cluster end is not aligned to the
bitmap's granularity.

Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy,
which requires the 'bytes' parameter to be aligned to cluster size.

Signed-off-by: Stefan Reiter 
---


As this is an assertion failure in a feature new to 5.1, this might be a 
candidate for inclusion if we have other reasons to go with -rc4.  But 
it's awfully late, I don't think this bug is sufficient on its own to 
delay the release.




This causes backups with unaligned image sizes to fail on the last block in my
testing (e.g. a backup job with 4k cluster size fails on a drive with 4097
bytes).

Alternatively one could remove the
   assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
from block_copy_do_copy, but I'd wager that's there for a reason?

  block/block-copy.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/block/block-copy.c b/block/block-copy.c
index f7428a7c08..023cb03200 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -142,6 +142,8 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState 
*s,
  return NULL;
  }
  
+bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);

+
  /* region is dirty, so no existent tasks possible in it */
  assert(!find_conflicting_task(s, offset, bytes));
  



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

Re: [PATCH] block/block-copy: always align copied region to cluster size

[Vladimir Sementsov-Ogievskiy]

06.08.2020 16:57, Stefan Reiter wrote:

Since commit 42ac214406e0 (block/block-copy: refactor task creation)
block_copy_task_create calculates the area to be copied via
bdrv_dirty_bitmap_next_dirty_area, but that can return an unaligned byte
count if the backing image's last cluster end is not aligned to the


Hmm, I assume you mean not "backing image" but just "image"? Backing seems 
unrelated, the problem is just unaligned image


bitmap's granularity.

Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy,
which requires the 'bytes' parameter to be aligned to cluster size.

Signed-off-by: Stefan Reiter 
---

This causes backups with unaligned image sizes to fail on the last block in my
testing (e.g. a backup job with 4k cluster size fails on a drive with 4097
bytes).


Ohh. Sorry for this :(

I think, we want this case covered by some iotest.. Are you going to add a 
test? Or I can do it..



Alternatively one could remove the
   assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
from block_copy_do_copy, but I'd wager that's there for a reason?


Looking at the code I think, that the reason is just a convention. It may be 
changed, but it will need an audit of the code. For me your fix looks the 
correct thing to do.



  block/block-copy.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/block/block-copy.c b/block/block-copy.c
index f7428a7c08..023cb03200 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -142,6 +142,8 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState 
*s,
  return NULL;
  }
  


For readability, I'd also add an assertion:

  assert(QEMU_IS_ALIGNED(offset, s->cluster_size));


+bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
+
  /* region is dirty, so no existent tasks possible in it */
  assert(!find_conflicting_task(s, offset, bytes));
  



Thanks for fixing!
Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Janosch Frank]

On 8/6/20 12:53 PM, Thomas Huth wrote:
> Let's add two new tests:
> 
> 1) Booting with "bootindex" is the architected default behavior on the
> s390x target, so we should have at least one test that is using the
> "bootindex" property.
> 
> 2) The s390-ccw bios used to fail when other unbootable devices have
> been specified before the bootable device (without "bootindex"). Now
> that the s390-ccw bios is a little bit smarter here, we should test
> this scenario, too, to avoid regressions.
> 
> Signed-off-by: Thomas Huth 

Nice!

Acked-by: Janosch Frank 

> ---
>  tests/qtest/cdrom-test.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> index 833a0508a1..13e22f57c1 100644
> --- a/tests/qtest/cdrom-test.c
> +++ b/tests/qtest/cdrom-test.c
> @@ -163,6 +163,18 @@ static void add_s390x_tests(void)
>  qtest_add_data_func("cdrom/boot/virtio-scsi",
>  "-device virtio-scsi -device scsi-cd,drive=cdr "
>  "-blockdev file,node-name=cdr,filename=", 
> test_cdboot);
> +qtest_add_data_func("cdrom/boot/with-bootindex",
> +"-device virtio-serial -device virtio-scsi "
> +"-device virtio-blk,drive=d1 "
> +"-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> +"-device virtio-blk,drive=d2,bootindex=1 "
> +"-drive if=none,id=d2,media=cdrom,file=", 
> test_cdboot);
> +qtest_add_data_func("cdrom/boot/without-bootindex",
> +"-device virtio-scsi -device virtio-serial "
> +"-device x-terminal3270 -device virtio-blk,drive=d1 "
> +"-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> +"-device virtio-blk,drive=d2 "
> +"-drive if=none,id=d2,media=cdrom,file=", 
> test_cdboot);
>  }
>  
>  int main(int argc, char **argv)
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function

[Janosch Frank]

On 8/6/20 12:53 PM, Thomas Huth wrote:
> Move the code to a separate function to be able to re-use it from a
> different spot later.
> 
> Reviewed-by: Claudio Imbrenda 
> Signed-off-by: Thomas Huth 

The new function name helps a lot :)

Reviewed-by: Janosch Frank 

> ---
>  pc-bios/s390-ccw/main.c | 99 -
>  1 file changed, 57 insertions(+), 42 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 9b64eb0c24..0d2aabbc58 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -51,6 +51,60 @@ unsigned int get_loadparm_index(void)
>  return atoui(loadparm_str);
>  }
>  
> +static int is_dev_possibly_bootable(int dev_no, int sch_no)
> +{
> +bool is_virtio;
> +Schib schib;
> +int r;
> +
> +blk_schid.sch_no = sch_no;
> +r = stsch_err(blk_schid, );
> +if (r == 3 || r == -EIO) {
> +return -ENODEV;
> +}
> +if (!schib.pmcw.dnv) {
> +return false;
> +}
> +
> +enable_subchannel(blk_schid);
> +cutype = cu_type(blk_schid);
> +
> +/*
> + * Note: we always have to run virtio_is_supported() here to make
> + * sure that the vdev.senseid data gets pre-initialized correctly
> + */
> +is_virtio = virtio_is_supported(blk_schid);
> +
> +/* No specific devno given, just return whether the device is possibly 
> bootable */
> +if (dev_no < 0) {
> +switch (cutype) {
> +case CU_TYPE_VIRTIO:
> +if (is_virtio) {
> +/*
> + * Skip net devices since no IPLB is created and therefore
> + * no network bootloader has been loaded
> + */
> +if (virtio_get_device_type() != VIRTIO_ID_NET) {
> +return true;
> +}
> +}
> +return false;
> +case CU_TYPE_DASD_3990:
> +case CU_TYPE_DASD_2107:
> +return true;
> +default:
> +return false;
> +}
> +}
> +
> +/* Caller asked for a specific devno */
> +if (schib.pmcw.dev == dev_no) {
> +return true;
> +}
> +
> +return false;
> +}
> +
>  /*
>   * Find the subchannel connected to the given device (dev_no) and fill in the
>   * subchannel information block (schib) with the connected subchannel's info.
> @@ -62,53 +116,14 @@ unsigned int get_loadparm_index(void)
>   */
>  static bool find_subch(int dev_no)
>  {
> -Schib schib;
>  int i, r;
> -bool is_virtio;
>  
>  for (i = 0; i < 0x1; i++) {
> -blk_schid.sch_no = i;
> -r = stsch_err(blk_schid, );
> -if ((r == 3) || (r == -EIO)) {
> +r = is_dev_possibly_bootable(dev_no, i);
> +if (r < 0) {
>  break;
>  }
> -if (!schib.pmcw.dnv) {
> -continue;
> -}
> -
> -enable_subchannel(blk_schid);
> -cutype = cu_type(blk_schid);
> -
> -/*
> - * Note: we always have to run virtio_is_supported() here to make
> - * sure that the vdev.senseid data gets pre-initialized correctly
> - */
> -is_virtio = virtio_is_supported(blk_schid);
> -
> -/* No specific devno given, just return 1st possibly bootable device 
> */
> -if (dev_no < 0) {
> -switch (cutype) {
> -case CU_TYPE_VIRTIO:
> -if (is_virtio) {
> -/*
> - * Skip net devices since no IPLB is created and 
> therefore
> - * no network bootloader has been loaded
> - */
> -if (virtio_get_device_type() != VIRTIO_ID_NET) {
> -return true;
> -}
> -}
> -continue;
> -case CU_TYPE_DASD_3990:
> -case CU_TYPE_DASD_2107:
> -return true;
> -default:
> -continue;
> -}
> -}
> -
> -/* Caller asked for a specific devno */
> -if (schib.pmcw.dev == dev_no) {
> +if (r == true) {
>  return true;
>  }
>  }
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others

[Janosch Frank]

On 8/6/20 12:53 PM, Thomas Huth wrote:
> Remove the "#ifndef E..." guards from the defines here - the header
> guard S390_CCW_H at the top of the file should avoid double definition,
> and if the error code is defined in a different file already, we're in
> trouble anyway, then it's better to see the error at compile time instead
> of hunting weird behavior during runtime later.
> Also define ENODEV - we will use this in a later patch.
> 
> Signed-off-by: Thomas Huth 

Would it make sense to use the errno.h numbers for the defines?

Reviewed-by: Janosch Frank 

> ---
>  pc-bios/s390-ccw/s390-ccw.h | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 36b884cced..dbc4c64851 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -27,12 +27,10 @@ typedef unsigned long long __u64;
>  #define false 0
>  #define PAGE_SIZE 4096
>  
> -#ifndef EIO
>  #define EIO 1
> -#endif
> -#ifndef EBUSY
>  #define EBUSY   2
> -#endif
> +#define ENODEV  3
> +
>  #ifndef NULL
>  #define NULL0
>  #endif
> 




signature.asc
Description: OpenPGP digital signature

[PATCH] block/block-copy: always align copied region to cluster size

[Stefan Reiter]

Since commit 42ac214406e0 (block/block-copy: refactor task creation)
block_copy_task_create calculates the area to be copied via
bdrv_dirty_bitmap_next_dirty_area, but that can return an unaligned byte
count if the backing image's last cluster end is not aligned to the
bitmap's granularity.

Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy,
which requires the 'bytes' parameter to be aligned to cluster size.

Signed-off-by: Stefan Reiter 
---

This causes backups with unaligned image sizes to fail on the last block in my
testing (e.g. a backup job with 4k cluster size fails on a drive with 4097
bytes).

Alternatively one could remove the
  assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
from block_copy_do_copy, but I'd wager that's there for a reason?

 block/block-copy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/block-copy.c b/block/block-copy.c
index f7428a7c08..023cb03200 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -142,6 +142,8 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState 
*s,
 return NULL;
 }
 
+bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
+
 /* region is dirty, so no existent tasks possible in it */
 assert(!find_conflicting_task(s, offset, bytes));
 
-- 
2.20.1

Re: [PATCH v3 0/3] aio-posix: keep aio_notify_me disabled during polling

[Paolo Bonzini]

On 06/08/20 15:17, Stefan Hajnoczi wrote:
> v3:
>  * Use smp_wmb() in aio_notify_accept() [Paolo]
>  * Flatten if statement in aio_poll() [Paolo]
> 
> v2:
>  * Added smp_mb() in aio_notify_accept() [Paolo]
>  * Added comments about memory barrier pairing [Paolo]
>  * Eliminated extra aio_compute_timeout() before calling ppoll()
> 
> This patch series eliminates ctx->notifier EventNotifier activity when
> aio_poll() is in polling mode. There is no need to use the EventNotifier since
> a polling handler can detect that aio_notify() has been called by monitoring a
> field in memory instead.
> 
> Optimizing out the EventNotifier calls improves null-co random read 4KB
> iodepth=1 IOPS by 18%.
> 
> I have not modified docs/spin/aio_notify*.promela because I'm not familiar 
> with
> the SPIN model checker.
> 
> Stefan Hajnoczi (3):
>   async: rename event_notifier_dummy_cb/poll()
>   async: always set ctx->notified in aio_notify()
>   aio-posix: keep aio_notify_me disabled during polling
> 
>  util/aio-posix.c | 47 +--
>  util/async.c | 36 +++-
>  2 files changed, 48 insertions(+), 35 deletions(-)
> 

Reviewed-by: Paolo Bonzini 

apart from a nit on patch 2.

Paolo

Re: [PATCH v3 2/3] async: always set ctx->notified in aio_notify()

[Paolo Bonzini]

On 06/08/20 15:18, Stefan Hajnoczi wrote:
> +atomic_set(>notified, false);
> +
> +/*
> + * Write ctx->notified before reading e.g. bh->flags.  Pairs with smp_mb 
> in
> + * aio_notify.
> + */
> +smp_wmb();

Sorry I was not clear: the memory barrier has to be smp_mb(), but the
comment has to say smp_wmb().  No need to repost for this.

Paolo

[PATCH v3 3/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

Polling only monitors the ctx->notified field and does not need the
ctx->notifier EventNotifier to be signalled. Keep ctx->aio_notify_me
disabled while polling to avoid unnecessary EventNotifier syscalls.

This optimization improves virtio-blk 4KB random read performance by
18%. The following results are with an IOThread and the null-co block
driver:

Test IOPS   Error
Before  244518.62 ± 1.20%
After   290706.11 ± 0.44%

Signed-off-by: Stefan Hajnoczi 
---
 util/aio-posix.c | 47 +--
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index 1b2a3af65b..f7f13ebfc2 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -464,9 +464,6 @@ static bool remove_idle_poll_handlers(AioContext *ctx, 
int64_t now)
  *
  * Polls for a given time.
  *
- * Note that ctx->notify_me must be non-zero so this function can detect
- * aio_notify().
- *
  * Note that the caller must have incremented ctx->list_lock.
  *
  * Returns: true if progress was made, false otherwise
@@ -476,7 +473,6 @@ static bool run_poll_handlers(AioContext *ctx, int64_t 
max_ns, int64_t *timeout)
 bool progress;
 int64_t start_time, elapsed_time;
 
-assert(ctx->notify_me);
 assert(qemu_lockcnt_count(>list_lock) > 0);
 
 trace_run_poll_handlers_begin(ctx, max_ns, *timeout);
@@ -520,8 +516,6 @@ static bool run_poll_handlers(AioContext *ctx, int64_t 
max_ns, int64_t *timeout)
  * @timeout: timeout for blocking wait, computed by the caller and updated if
  *polling succeeds.
  *
- * ctx->notify_me must be non-zero so this function can detect aio_notify().
- *
  * Note that the caller must have incremented ctx->list_lock.
  *
  * Returns: true if progress was made, false otherwise
@@ -556,6 +550,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
 AioHandlerList ready_list = QLIST_HEAD_INITIALIZER(ready_list);
 int ret = 0;
 bool progress;
+bool use_notify_me;
 int64_t timeout;
 int64_t start = 0;
 
@@ -566,33 +561,39 @@ bool aio_poll(AioContext *ctx, bool blocking)
  */
 assert(in_aio_context_home_thread(ctx));
 
-/* aio_notify can avoid the expensive event_notifier_set if
+qemu_lockcnt_inc(>list_lock);
+
+if (ctx->poll_max_ns) {
+start = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+}
+
+timeout = blocking ? aio_compute_timeout(ctx) : 0;
+progress = try_poll_mode(ctx, );
+assert(!(timeout && progress));
+
+/*
+ * aio_notify can avoid the expensive event_notifier_set if
  * everything (file descriptors, bottom halves, timers) will
  * be re-evaluated before the next blocking poll().  This is
  * already true when aio_poll is called with blocking == false;
  * if blocking == true, it is only true after poll() returns,
  * so disable the optimization now.
  */
-if (blocking) {
+use_notify_me = timeout != 0;
+if (use_notify_me) {
 atomic_set(>notify_me, atomic_read(>notify_me) + 2);
 /*
- * Write ctx->notify_me before computing the timeout
- * (reading bottom half flags, etc.).  Pairs with
+ * Write ctx->notify_me before reading ctx->notified.  Pairs with
  * smp_mb in aio_notify().
  */
 smp_mb();
-}
-
-qemu_lockcnt_inc(>list_lock);
 
-if (ctx->poll_max_ns) {
-start = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+/* Don't block if aio_notify() was called */
+if (atomic_read(>notified)) {
+timeout = 0;
+}
 }
 
-timeout = blocking ? aio_compute_timeout(ctx) : 0;
-progress = try_poll_mode(ctx, );
-assert(!(timeout && progress));
-
 /* If polling is allowed, non-blocking aio_poll does not need the
  * system call---a single round of run_poll_handlers_once suffices.
  */
@@ -600,12 +601,14 @@ bool aio_poll(AioContext *ctx, bool blocking)
 ret = ctx->fdmon_ops->wait(ctx, _list, timeout);
 }
 
-if (blocking) {
+if (use_notify_me) {
 /* Finish the poll before clearing the flag.  */
-atomic_store_release(>notify_me, atomic_read(>notify_me) - 
2);
-aio_notify_accept(ctx);
+atomic_store_release(>notify_me,
+ atomic_read(>notify_me) - 2);
 }
 
+aio_notify_accept(ctx);
+
 /* Adjust polling time */
 if (ctx->poll_max_ns) {
 int64_t block_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start;
-- 
2.26.2

[PATCH v3 1/3] async: rename event_notifier_dummy_cb/poll()

[Stefan Hajnoczi]

The event_notifier_*() prefix can be confused with the EventNotifier
APIs that are also called event_notifier_*().

Rename the functions to aio_context_notifier_*() to make it clear that
they relate to the AioContext::notifier field.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Philippe Mathieu-Daudé 
---
 util/async.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/async.c b/util/async.c
index 1319eee3bc..d9f987e133 100644
--- a/util/async.c
+++ b/util/async.c
@@ -445,12 +445,12 @@ static void aio_timerlist_notify(void *opaque, 
QEMUClockType type)
 aio_notify(opaque);
 }
 
-static void event_notifier_dummy_cb(EventNotifier *e)
+static void aio_context_notifier_dummy_cb(EventNotifier *e)
 {
 }
 
 /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
-static bool event_notifier_poll(void *opaque)
+static bool aio_context_notifier_poll(void *opaque)
 {
 EventNotifier *e = opaque;
 AioContext *ctx = container_of(e, AioContext, notifier);
@@ -508,8 +508,8 @@ AioContext *aio_context_new(Error **errp)
 
 aio_set_event_notifier(ctx, >notifier,
false,
-   event_notifier_dummy_cb,
-   event_notifier_poll);
+   aio_context_notifier_dummy_cb,
+   aio_context_notifier_poll);
 #ifdef CONFIG_LINUX_AIO
 ctx->linux_aio = NULL;
 #endif
-- 
2.26.2

[PATCH v3 0/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

v3:
 * Use smp_wmb() in aio_notify_accept() [Paolo]
 * Flatten if statement in aio_poll() [Paolo]

v2:
 * Added smp_mb() in aio_notify_accept() [Paolo]
 * Added comments about memory barrier pairing [Paolo]
 * Eliminated extra aio_compute_timeout() before calling ppoll()

This patch series eliminates ctx->notifier EventNotifier activity when
aio_poll() is in polling mode. There is no need to use the EventNotifier since
a polling handler can detect that aio_notify() has been called by monitoring a
field in memory instead.

Optimizing out the EventNotifier calls improves null-co random read 4KB
iodepth=1 IOPS by 18%.

I have not modified docs/spin/aio_notify*.promela because I'm not familiar with
the SPIN model checker.

Stefan Hajnoczi (3):
  async: rename event_notifier_dummy_cb/poll()
  async: always set ctx->notified in aio_notify()
  aio-posix: keep aio_notify_me disabled during polling

 util/aio-posix.c | 47 +--
 util/async.c | 36 +++-
 2 files changed, 48 insertions(+), 35 deletions(-)

-- 
2.26.2

[PATCH v3 2/3] async: always set ctx->notified in aio_notify()

[Stefan Hajnoczi]

aio_notify() does not set ctx->notified when called with
ctx->aio_notify_me disabled. Therefore aio_notify_me needs to be enabled
during polling.

This is suboptimal since expensive event_notifier_set(>notifier)
and event_notifier_test_and_clear(>notifier) calls are required
when ctx->aio_notify_me is enabled.

Change aio_notify() so that aio->notified is always set, regardless of
ctx->aio_notify_me. This will make polling cheaper since
ctx->aio_notify_me can remain disabled. Move the
event_notifier_test_and_clear() to the fd handler function (which is now
no longer an empty function so "dummy" has been dropped from its name).

The next patch takes advantage of this by optimizing polling in
util/aio-posix.c.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Paolo Bonzini 
---
 util/async.c | 32 +---
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/util/async.c b/util/async.c
index d9f987e133..99e15644d9 100644
--- a/util/async.c
+++ b/util/async.c
@@ -419,25 +419,32 @@ LuringState *aio_get_linux_io_uring(AioContext *ctx)
 
 void aio_notify(AioContext *ctx)
 {
-/* Write e.g. bh->scheduled before reading ctx->notify_me.  Pairs
+/*
+ * Write e.g. bh->flags before writing ctx->notified.  Pairs with smp_mb in
+ * aio_notify_accept.
+ */
+smp_wmb();
+atomic_set(>notified, true);
+
+/*
+ * Write ctx->notified before reading ctx->notify_me.  Pairs
  * with smp_mb in aio_ctx_prepare or aio_poll.
  */
 smp_mb();
 if (atomic_read(>notify_me)) {
 event_notifier_set(>notifier);
-atomic_mb_set(>notified, true);
 }
 }
 
 void aio_notify_accept(AioContext *ctx)
 {
-if (atomic_xchg(>notified, false)
-#ifdef WIN32
-|| true
-#endif
-) {
-event_notifier_test_and_clear(>notifier);
-}
+atomic_set(>notified, false);
+
+/*
+ * Write ctx->notified before reading e.g. bh->flags.  Pairs with smp_mb in
+ * aio_notify.
+ */
+smp_wmb();
 }
 
 static void aio_timerlist_notify(void *opaque, QEMUClockType type)
@@ -445,8 +452,11 @@ static void aio_timerlist_notify(void *opaque, 
QEMUClockType type)
 aio_notify(opaque);
 }
 
-static void aio_context_notifier_dummy_cb(EventNotifier *e)
+static void aio_context_notifier_cb(EventNotifier *e)
 {
+AioContext *ctx = container_of(e, AioContext, notifier);
+
+event_notifier_test_and_clear(>notifier);
 }
 
 /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
@@ -508,7 +518,7 @@ AioContext *aio_context_new(Error **errp)
 
 aio_set_event_notifier(ctx, >notifier,
false,
-   aio_context_notifier_dummy_cb,
+   aio_context_notifier_cb,
aio_context_notifier_poll);
 #ifdef CONFIG_LINUX_AIO
 ctx->linux_aio = NULL;
-- 
2.26.2

Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Cornelia Huck]

On Thu, 6 Aug 2020 13:58:47 +0200
Thomas Huth  wrote:

> On 06/08/2020 13.23, Cornelia Huck wrote:
> > On Thu,  6 Aug 2020 12:53:49 +0200
> > Thomas Huth  wrote:
> >   
> >> Let's add two new tests:
> >>
> >> 1) Booting with "bootindex" is the architected default behavior on the
> >> s390x target, so we should have at least one test that is using the
> >> "bootindex" property.
> >>
> >> 2) The s390-ccw bios used to fail when other unbootable devices have
> >> been specified before the bootable device (without "bootindex"). Now
> >> that the s390-ccw bios is a little bit smarter here, we should test
> >> this scenario, too, to avoid regressions.
> >>
> >> Signed-off-by: Thomas Huth 
> >> ---
> >>  tests/qtest/cdrom-test.c | 12 
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> >> index 833a0508a1..13e22f57c1 100644
> >> --- a/tests/qtest/cdrom-test.c
> >> +++ b/tests/qtest/cdrom-test.c
> >> @@ -163,6 +163,18 @@ static void add_s390x_tests(void)
> >>  qtest_add_data_func("cdrom/boot/virtio-scsi",
> >>  "-device virtio-scsi -device scsi-cd,drive=cdr "
> >>  "-blockdev file,node-name=cdr,filename=", 
> >> test_cdboot);
> >> +qtest_add_data_func("cdrom/boot/with-bootindex",
> >> +"-device virtio-serial -device virtio-scsi "
> >> +"-device virtio-blk,drive=d1 "
> >> +"-drive 
> >> driver=null-co,read-zeroes=on,if=none,id=d1 "
> >> +"-device virtio-blk,drive=d2,bootindex=1 "
> >> +"-drive if=none,id=d2,media=cdrom,file=", 
> >> test_cdboot);
> >> +qtest_add_data_func("cdrom/boot/without-bootindex",
> >> +"-device virtio-scsi -device virtio-serial "
> >> +"-device x-terminal3270 -device 
> >> virtio-blk,drive=d1 "  
> > 
> > Any special reason for that 3270 device here? Or just to add more
> > variety? :)  
> 
> Yes, there is a reason:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07231.html
> 
> ... so this is a check that this does not happen again.

Ah, thanks for reminding me. Adding 3270 is a good idea.

Reviewed-by: Cornelia Huck 

Re: [PATCH-for-5.2 7/7] hw/block/fdc: Add ASCII art schema of QOM relations

[Kevin Wolf]

Am 06.08.2020 um 12:33 hat Philippe Mathieu-Daudé geschrieben:
> On 8/6/20 10:57 AM, Kevin Wolf wrote:
> > Am 06.08.2020 um 10:08 hat Philippe Mathieu-Daudé geschrieben:
> >> Without knowing the QEMU history, it is hard to relate QEMU objects
> >> with the hardware datasheet.
> >>
> >> For example, one naively expects:
> >>
> >> * a floppy disk is plugged / unplugged on the bus
> >>
> >>   Wrong! QEMU floppy disks always sit on the bus. The block drives
> >>   are plugged / unplugged on the disks, and the disks magically
> >>   re-adapt their proprieties to match the block drive.
> > 
> > This is because what sits on the bus is not a floppy disk, but a floppy
> > drive. FloppyDrive is also what the type is called.
> > 
> > The disk is represented by the BlockDriverState (the actual image file)
> > that is inserted in the BlockBackend (which is logically part of the
> > drive).
> > 
> >> * a floppy controller has a fixed number of disks pluggable on the bus
> >>
> >>   Wrong! QEMU floppy controllers have as much slots as the number of
> >>   floppy drive provided when a machine is created. Then the ACPI table
> >>   are generated and the number of slots can not be modified. So if you
> >>   expect a dual slot controller being created with slot A and B, if
> >>   the machine is created with a single drive attached, the controller
> >>   will only have slot A created, and you will never be able to plug
> >>   drive B without risking a mismatch in the ACPI tables.
> > 
> > Hm... I guess hotplugging floppy drives might actually have worked,
> > though I have never tried it on real hardware. I'm pretty sure it wasn't
> > an official feature, though, and ACPI tables certainly won't magically
> > change if you do this because (apart from polling, I guess) software has
> > no way to detect that you tinkered with the floppy cable. :-)
> > 
> >> * a floppy controller supporting 4 disks uses 2 buses
> >>
> >>   Wrong! QEMU uses a single bus to plug the 4 disks.
> > 
> > But we don't even emulate floppy controllers that can have more than two
> > floppy drives:
> > 
> > $ x86_64-softmmu/qemu-system-x86_64 -device floppy -device floppy 
> > -device floppy
> > qemu-system-x86_64: -device floppy: Can't create floppy unit 2, bus 
> > supports only 2 units
> 
> This comment is for developers, the warning is for user.
> 
> It comes from:
> 
> if (dev->unit >= MAX_FD) {
> error_setg(errp, "Can't create floppy unit %d, bus supports "
>"only %d units", dev->unit, MAX_FD);
> return;
> }
> 
> But you can compile QEMU with MAX_FD=4:
> [...]

Ah, I wasn't aware that this was supposed to be changed.

I don't even have the spec here for a floppy controller that supports
four drives. My copy has "reserved" for those bits that apparently refer
to the additional drives (if the QEMU code is right).

> > This is checked in floppy_drive_realize(), so it applies to all
> > variants of the controller.
> > 
> > If you want more floppy drives, you have to create a second controller
> > (with a different iobase). Though I don't think I actually got this
> > working when I tried. I wasn't sure if the problem was the emulation or
> > the guest OSes (or SeaBIOS actually for DOS).
> > 
> >> As all these false assumptions are not obvious (we don't plug a disk,
> >> we plug a block drive into a disk, etc...), start documenting the QOM
> >> relationships with a simple ASCII schema.
> > 
> > Maybe be more specific to have: "floppy (drive)" and "blk (disk)".
> > Because the ASCII schema is actually true, though you seem to have
> > misunderstood what each item in it is supposed to represent.
> > 
> > Actually "blk (disk)" is not 100% accurate either because the drive
> > always has a BlockBackend present. It's really the BlockDriverState
> > inserted into the BlockBackend that is the disk.
> > 
> > In summary, to be honest, I believe since its qdevification, floppy is
> > one of the block devices that is modelled the best on the QOM + block
> > backend level. Only SCSI might be comparable, but IDE, virtio-blk and
> > usb-storage are a mess in comparison.
> 
> I'm sorry I didn't want to criticize the model or hurt you, I just want
> to note the differences between how the controller is described in the
> Intel 82078 datasheet and how the QEMU model works. Maybe I'm wrong
> assuming there would be a 1:1 match.

Sorry for not being clear. This isn't about criticism at all. I just
wanted to suggest that if you're trying to find out how to model new
devices, looking at floppy is probably a better start than looking at
most other block devices.

I don't know the floppy emulation well enough to say much about the
MAX_FD == 4 case, but the basic separation into controller and drive,
and that removable media are not represented in qdev, but just as block
nodes inserted into the BlockBackend of the drive, feels like the right
approach.

> I'll repost with the name updated in the schema and removing my
> 

Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Thomas Huth]

On 06/08/2020 13.23, Cornelia Huck wrote:
> On Thu,  6 Aug 2020 12:53:49 +0200
> Thomas Huth  wrote:
> 
>> Let's add two new tests:
>>
>> 1) Booting with "bootindex" is the architected default behavior on the
>> s390x target, so we should have at least one test that is using the
>> "bootindex" property.
>>
>> 2) The s390-ccw bios used to fail when other unbootable devices have
>> been specified before the bootable device (without "bootindex"). Now
>> that the s390-ccw bios is a little bit smarter here, we should test
>> this scenario, too, to avoid regressions.
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  tests/qtest/cdrom-test.c | 12 
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
>> index 833a0508a1..13e22f57c1 100644
>> --- a/tests/qtest/cdrom-test.c
>> +++ b/tests/qtest/cdrom-test.c
>> @@ -163,6 +163,18 @@ static void add_s390x_tests(void)
>>  qtest_add_data_func("cdrom/boot/virtio-scsi",
>>  "-device virtio-scsi -device scsi-cd,drive=cdr "
>>  "-blockdev file,node-name=cdr,filename=", 
>> test_cdboot);
>> +qtest_add_data_func("cdrom/boot/with-bootindex",
>> +"-device virtio-serial -device virtio-scsi "
>> +"-device virtio-blk,drive=d1 "
>> +"-drive driver=null-co,read-zeroes=on,if=none,id=d1 
>> "
>> +"-device virtio-blk,drive=d2,bootindex=1 "
>> +"-drive if=none,id=d2,media=cdrom,file=", 
>> test_cdboot);
>> +qtest_add_data_func("cdrom/boot/without-bootindex",
>> +"-device virtio-scsi -device virtio-serial "
>> +"-device x-terminal3270 -device virtio-blk,drive=d1 
>> "
> 
> Any special reason for that 3270 device here? Or just to add more
> variety? :)

Yes, there is a reason:

 https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07231.html

... so this is a check that this does not happen again.

 Thomas

[PATCH for-5.2 v2 6/9] pc-bios/s390-ccw: Scan through all devices if no boot device specified

[Thomas Huth]

If no boot device has been specified (via "bootindex=..."), the s390-ccw
bios scans through all devices to find a bootable device. But so far, it
stops at the very first block device (including virtio-scsi controllers
without attached devices) that it finds, no matter whether it is bootable
or not. That leads to some weird situatation where it is e.g. possible
to boot via:

 qemu-system-s390x -hda /path/to/disk.qcow2

but not if there is e.g. a virtio-scsi controller specified before:

 qemu-system-s390x -device virtio-scsi -hda /path/to/disk.qcow2

While using "bootindex=..." is clearly the preferred way of booting
on s390x, we still can make the life for the users at least a little
bit easier if we look at all available devices to find a bootable one.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1846975
Reviewed-by: Cornelia Huck 
Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/main.c | 46 +++--
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 7bdd12ab2e..9b581074a1 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -182,20 +182,8 @@ static void boot_setup(void)
 static void find_boot_device(void)
 {
 VDev *vdev = virtio_get_device();
-int ssid;
 bool found;
 
-if (!have_iplb) {
-for (ssid = 0; ssid < 0x3; ssid++) {
-blk_schid.ssid = ssid;
-found = find_subch(-1);
-if (found) {
-return;
-}
-}
-panic("Could not find a suitable boot device (none specified)\n");
-}
-
 switch (iplb.pbt) {
 case S390_IPL_TYPE_CCW:
 debug_print_int("device no. ", iplb.ccw.devno);
@@ -261,14 +249,42 @@ static void ipl_boot_device(void)
 }
 }
 
+/*
+ * No boot device has been specified, so we have to scan through the
+ * channels to find one.
+ */
+static void probe_boot_device(void)
+{
+int ssid, sch_no, ret;
+
+for (ssid = 0; ssid < 0x3; ssid++) {
+blk_schid.ssid = ssid;
+for (sch_no = 0; sch_no < 0x1; sch_no++) {
+ret = is_dev_possibly_bootable(-1, sch_no);
+if (ret < 0) {
+break;
+}
+if (ret == true) {
+ipl_boot_device();  /* Only returns if unsuccessful */
+}
+}
+}
+
+sclp_print("Could not find a suitable boot device (none specified)\n");
+}
+
 int main(void)
 {
 sclp_setup();
 css_setup();
 boot_setup();
-find_boot_device();
-enable_subchannel(blk_schid);
-ipl_boot_device();
+if (have_iplb) {
+find_boot_device();
+enable_subchannel(blk_schid);
+ipl_boot_device();
+} else {
+probe_boot_device();
+}
 
 panic("Failed to load OS from hard disk\n");
 return 0; /* make compiler happy */
-- 
2.18.1

Re: [PATCH v2 3/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

On Wed, Aug 05, 2020 at 06:37:45PM +0200, Paolo Bonzini wrote:
> On 05/08/20 12:00, Stefan Hajnoczi wrote:
> > +
> > +/*
> > + * aio_notify can avoid the expensive event_notifier_set if
> > + * everything (file descriptors, bottom halves, timers) will
> > + * be re-evaluated before the next blocking poll().  This is
> > + * already true when aio_poll is called with blocking == false;
> > + * if blocking == true, it is only true after poll() returns,
> > + * so disable the optimization now.
> > + */
> > +if (use_notify_me) {
> > +atomic_set(>notify_me, atomic_read(>notify_me) + 2);
> > +/*
> > + * Write ctx->notify_me before reading ctx->notified.  Pairs 
> > with
> > + * smp_mb in aio_notify().
> > + */
> > +smp_mb();
> > +
> > +/* Don't block if aio_notify() was called */
> > +if (atomic_read(>notified)) {
> > +timeout = 0;
> > +}
> 
> Aha, this is the trick: "timeout = 0" also applies if a timer was moved 
> early.  In this case you uselessly keep notify_me set for a bit, but 
> it's okay. Nice!
> 
> The code can be simplified a bit more, since the use_notify_me variable 
> is just "timeout":

Good point. I'll send another revision.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others

[Cornelia Huck]

On Thu,  6 Aug 2020 12:53:43 +0200
Thomas Huth  wrote:

> Remove the "#ifndef E..." guards from the defines here - the header
> guard S390_CCW_H at the top of the file should avoid double definition,
> and if the error code is defined in a different file already, we're in
> trouble anyway, then it's better to see the error at compile time instead
> of hunting weird behavior during runtime later.
> Also define ENODEV - we will use this in a later patch.
> 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/s390-ccw.h | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck 

[PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Thomas Huth]

Let's add two new tests:

1) Booting with "bootindex" is the architected default behavior on the
s390x target, so we should have at least one test that is using the
"bootindex" property.

2) The s390-ccw bios used to fail when other unbootable devices have
been specified before the bootable device (without "bootindex"). Now
that the s390-ccw bios is a little bit smarter here, we should test
this scenario, too, to avoid regressions.

Signed-off-by: Thomas Huth 
---
 tests/qtest/cdrom-test.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index 833a0508a1..13e22f57c1 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -163,6 +163,18 @@ static void add_s390x_tests(void)
 qtest_add_data_func("cdrom/boot/virtio-scsi",
 "-device virtio-scsi -device scsi-cd,drive=cdr "
 "-blockdev file,node-name=cdr,filename=", test_cdboot);
+qtest_add_data_func("cdrom/boot/with-bootindex",
+"-device virtio-serial -device virtio-scsi "
+"-device virtio-blk,drive=d1 "
+"-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
+"-device virtio-blk,drive=d2,bootindex=1 "
+"-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
+qtest_add_data_func("cdrom/boot/without-bootindex",
+"-device virtio-scsi -device virtio-serial "
+"-device x-terminal3270 -device virtio-blk,drive=d1 "
+"-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
+"-device virtio-blk,drive=d2 "
+"-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
 }
 
 int main(int argc, char **argv)
-- 
2.18.1

Re: [PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk

[Cornelia Huck]

On Thu,  6 Aug 2020 12:53:45 +0200
Thomas Huth  wrote:

> In case the user did not specify a boot device, we want to continue
> looking for other devices if there are no valid SCSI disks on a virtio-
> scsi controller. As a first step, do not panic in this case and let
> the control flow carry the error to the upper functions instead.
> 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/main.c  | 14 ++
>  pc-bios/s390-ccw/s390-ccw.h  |  2 +-
>  pc-bios/s390-ccw/virtio-blkdev.c |  7 +--
>  pc-bios/s390-ccw/virtio-scsi.c   | 28 
>  pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
>  5 files changed, 37 insertions(+), 16 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel()

[Cornelia Huck]

On Thu,  6 Aug 2020 12:53:48 +0200
Thomas Huth  wrote:

> enable_subchannel() is already done during is_dev_possibly_bootable()
> (which is called from find_boot_device() -> find_subch()), so there
> is no need to do this again in the main() function.
> 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/main.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Cornelia Huck]

On Thu,  6 Aug 2020 12:53:49 +0200
Thomas Huth  wrote:

> Let's add two new tests:
> 
> 1) Booting with "bootindex" is the architected default behavior on the
> s390x target, so we should have at least one test that is using the
> "bootindex" property.
> 
> 2) The s390-ccw bios used to fail when other unbootable devices have
> been specified before the bootable device (without "bootindex"). Now
> that the s390-ccw bios is a little bit smarter here, we should test
> this scenario, too, to avoid regressions.
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/qtest/cdrom-test.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> index 833a0508a1..13e22f57c1 100644
> --- a/tests/qtest/cdrom-test.c
> +++ b/tests/qtest/cdrom-test.c
> @@ -163,6 +163,18 @@ static void add_s390x_tests(void)
>  qtest_add_data_func("cdrom/boot/virtio-scsi",
>  "-device virtio-scsi -device scsi-cd,drive=cdr "
>  "-blockdev file,node-name=cdr,filename=", 
> test_cdboot);
> +qtest_add_data_func("cdrom/boot/with-bootindex",
> +"-device virtio-serial -device virtio-scsi "
> +"-device virtio-blk,drive=d1 "
> +"-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> +"-device virtio-blk,drive=d2,bootindex=1 "
> +"-drive if=none,id=d2,media=cdrom,file=", 
> test_cdboot);
> +qtest_add_data_func("cdrom/boot/without-bootindex",
> +"-device virtio-scsi -device virtio-serial "
> +"-device x-terminal3270 -device virtio-blk,drive=d1 "

Any special reason for that 3270 device here? Or just to add more
variety? :)

> +"-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> +"-device virtio-blk,drive=d2 "
> +"-drive if=none,id=d2,media=cdrom,file=", 
> test_cdboot);
>  }
>  
>  int main(int argc, char **argv)

Re: [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function

[Cornelia Huck]

On Thu,  6 Aug 2020 12:53:44 +0200
Thomas Huth  wrote:

> Move the code to a separate function to be able to re-use it from a
> different spot later.
> 
> Reviewed-by: Claudio Imbrenda 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/main.c | 99 -
>  1 file changed, 57 insertions(+), 42 deletions(-)

(...)

> @@ -62,53 +116,14 @@ unsigned int get_loadparm_index(void)
>   */
>  static bool find_subch(int dev_no)
>  {
> -Schib schib;
>  int i, r;
> -bool is_virtio;
>  
>  for (i = 0; i < 0x1; i++) {
> -blk_schid.sch_no = i;
> -r = stsch_err(blk_schid, );
> -if ((r == 3) || (r == -EIO)) {
> +r = is_dev_possibly_bootable(dev_no, i);

Maybe explicitly check for -ENODEV here? But no strong opinion.

> +if (r < 0) {
>  break;
>  }

(...)

Reviewed-by: Cornelia Huck 

[PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel()

[Thomas Huth]

enable_subchannel() is already done during is_dev_possibly_bootable()
(which is called from find_boot_device() -> find_subch()), so there
is no need to do this again in the main() function.

Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index fc17e6ab83..43c792cf95 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -280,7 +280,6 @@ int main(void)
 boot_setup();
 if (have_iplb) {
 find_boot_device();
-enable_subchannel(blk_schid);
 ipl_boot_device();
 } else {
 probe_boot_device();
-- 
2.18.1

[PATCH for-5.2 v2 7/9] pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is bad

[Thomas Huth]

If you try to boot with two virtio-blk disks (without bootindex), and
only the second one is bootable, the s390-ccw bios currently stops at
the first disk and does not continue booting from the second one. This
is annoying - and all other major QEMU firmwares succeed to boot from
the second disk in this case, so we should do the same in the s390-ccw
bios, too.

Reviewed-by: Cornelia Huck 
Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/bootmap.c | 34 +++---
 pc-bios/s390-ccw/main.c|  2 +-
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 97205674e5..0ef6b851f3 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -289,11 +289,18 @@ static void ipl_eckd_cdl(void)
 read_block(1, ipl2, "Cannot read IPL2 record at block 1");
 
 mbr = >mbr;
-IPL_assert(magic_match(mbr, ZIPL_MAGIC), "No zIPL section in IPL2 
record.");
-IPL_assert(block_size_ok(mbr->blockptr.xeckd.bptr.size),
-   "Bad block size in zIPL section of IPL2 record.");
-IPL_assert(mbr->dev_type == DEV_TYPE_ECKD,
-   "Non-ECKD device type in zIPL section of IPL2 record.");
+if (!magic_match(mbr, ZIPL_MAGIC)) {
+sclp_print("No zIPL section in IPL2 record.\n");
+return;
+}
+if (!block_size_ok(mbr->blockptr.xeckd.bptr.size)) {
+sclp_print("Bad block size in zIPL section of IPL2 record.\n");
+return;
+}
+if (!mbr->dev_type == DEV_TYPE_ECKD) {
+sclp_print("Non-ECKD device type in zIPL section of IPL2 record.\n");
+return;
+}
 
 /* save pointer to Boot Map Table */
 bmt_block_nr = eckd_block_num(>blockptr.xeckd.bptr.chs);
@@ -303,10 +310,14 @@ static void ipl_eckd_cdl(void)
 
 memset(sec, FREE_SPACE_FILLER, sizeof(sec));
 read_block(2, vlbl, "Cannot read Volume Label at block 2");
-IPL_assert(magic_match(vlbl->key, VOL1_MAGIC),
-   "Invalid magic of volume label block");
-IPL_assert(magic_match(vlbl->f.key, VOL1_MAGIC),
-   "Invalid magic of volser block");
+if (!magic_match(vlbl->key, VOL1_MAGIC)) {
+sclp_print("Invalid magic of volume label block.\n");
+return;
+}
+if (!magic_match(vlbl->f.key, VOL1_MAGIC)) {
+sclp_print("Invalid magic of volser block.\n");
+return;
+}
 print_volser(vlbl->f.volser);
 
 run_eckd_boot_script(bmt_block_nr, s1b_block_nr);
@@ -398,7 +409,8 @@ static void ipl_eckd(void)
 read_block(0, mbr, "Cannot read block 0 on DASD");
 
 if (magic_match(mbr->magic, IPL1_MAGIC)) {
-ipl_eckd_cdl(); /* no return */
+ipl_eckd_cdl(); /* only returns in case of error */
+return;
 }
 
 /* LDL/CMS? */
@@ -825,5 +837,5 @@ void zipl_load(void)
 panic("\n! Unknown IPL device type !\n");
 }
 
-panic("\n* this can never happen *\n");
+sclp_print("zIPL load failed.\n");
 }
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 9b581074a1..fc17e6ab83 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -240,7 +240,7 @@ static void ipl_boot_device(void)
 break;
 case CU_TYPE_VIRTIO:
 if (virtio_setup() == 0) {
-zipl_load(); /* no return */
+zipl_load(); /* Only returns in case of errors */
 }
 break;
 default:
-- 
2.18.1

[PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function

[Thomas Huth]

Move the code to a separate function to be able to re-use it from a
different spot later.

Reviewed-by: Claudio Imbrenda 
Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/main.c | 99 -
 1 file changed, 57 insertions(+), 42 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 9b64eb0c24..0d2aabbc58 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -51,6 +51,60 @@ unsigned int get_loadparm_index(void)
 return atoui(loadparm_str);
 }
 
+static int is_dev_possibly_bootable(int dev_no, int sch_no)
+{
+bool is_virtio;
+Schib schib;
+int r;
+
+blk_schid.sch_no = sch_no;
+r = stsch_err(blk_schid, );
+if (r == 3 || r == -EIO) {
+return -ENODEV;
+}
+if (!schib.pmcw.dnv) {
+return false;
+}
+
+enable_subchannel(blk_schid);
+cutype = cu_type(blk_schid);
+
+/*
+ * Note: we always have to run virtio_is_supported() here to make
+ * sure that the vdev.senseid data gets pre-initialized correctly
+ */
+is_virtio = virtio_is_supported(blk_schid);
+
+/* No specific devno given, just return whether the device is possibly 
bootable */
+if (dev_no < 0) {
+switch (cutype) {
+case CU_TYPE_VIRTIO:
+if (is_virtio) {
+/*
+ * Skip net devices since no IPLB is created and therefore
+ * no network bootloader has been loaded
+ */
+if (virtio_get_device_type() != VIRTIO_ID_NET) {
+return true;
+}
+}
+return false;
+case CU_TYPE_DASD_3990:
+case CU_TYPE_DASD_2107:
+return true;
+default:
+return false;
+}
+}
+
+/* Caller asked for a specific devno */
+if (schib.pmcw.dev == dev_no) {
+return true;
+}
+
+return false;
+}
+
 /*
  * Find the subchannel connected to the given device (dev_no) and fill in the
  * subchannel information block (schib) with the connected subchannel's info.
@@ -62,53 +116,14 @@ unsigned int get_loadparm_index(void)
  */
 static bool find_subch(int dev_no)
 {
-Schib schib;
 int i, r;
-bool is_virtio;
 
 for (i = 0; i < 0x1; i++) {
-blk_schid.sch_no = i;
-r = stsch_err(blk_schid, );
-if ((r == 3) || (r == -EIO)) {
+r = is_dev_possibly_bootable(dev_no, i);
+if (r < 0) {
 break;
 }
-if (!schib.pmcw.dnv) {
-continue;
-}
-
-enable_subchannel(blk_schid);
-cutype = cu_type(blk_schid);
-
-/*
- * Note: we always have to run virtio_is_supported() here to make
- * sure that the vdev.senseid data gets pre-initialized correctly
- */
-is_virtio = virtio_is_supported(blk_schid);
-
-/* No specific devno given, just return 1st possibly bootable device */
-if (dev_no < 0) {
-switch (cutype) {
-case CU_TYPE_VIRTIO:
-if (is_virtio) {
-/*
- * Skip net devices since no IPLB is created and therefore
- * no network bootloader has been loaded
- */
-if (virtio_get_device_type() != VIRTIO_ID_NET) {
-return true;
-}
-}
-continue;
-case CU_TYPE_DASD_3990:
-case CU_TYPE_DASD_2107:
-return true;
-default:
-continue;
-}
-}
-
-/* Caller asked for a specific devno */
-if (schib.pmcw.dev == dev_no) {
+if (r == true) {
 return true;
 }
 }
-- 
2.18.1

[PATCH for-5.2 v2 1/9] pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and -fno-common

[Thomas Huth]

The main QEMU code is compiled with -std=gnu99, -fwrapv and -fno-common.
We should use the same flags for the s390-ccw bios, too, to avoid that
we get different behavior with different compiler versions that changed
their default settings in the course of time (it happened at least with
-std=... and -fno-common in the past already).

While we're at it, also group the other flags here in a little bit nicer
fashion: Move the two "-m" flags out of the "-f" area and specify them on
a separate line.

Reviewed-by: Claudio Imbrenda 
Acked-by: Cornelia Huck 
Acked-by: Janosch Frank 
Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/Makefile | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 50bc880272..9abb0ea4c0 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -13,10 +13,11 @@ OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o 
\
  virtio.o virtio-scsi.o virtio-blkdev.o libc.o cio.o dasd-ipl.o
 
 QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS))
-QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -msoft-float
-QEMU_CFLAGS += -march=z900 -fPIE -fno-strict-aliasing
-QEMU_CFLAGS += -fno-asynchronous-unwind-tables
+QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -fno-common -fPIE
+QEMU_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables
 QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector)
+QEMU_CFLAGS += -msoft-float -march=z900
+QEMU_CFLAGS += -std=gnu99
 LDFLAGS += -Wl,-pie -nostdlib
 
 build-all: s390-ccw.img s390-netboot.img
-- 
2.18.1

[PATCH for-5.2 v2 2/9] pc-bios/s390-ccw: Move ipl-related code from main() into a separate function

[Thomas Huth]

Let's move this part of the code into a separate function to be able
to use it from multiple spots later.

Reviewed-by: Claudio Imbrenda 
Reviewed-by: Cornelia Huck 
Reviewed-by: Janosch Frank 
Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/main.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 146a50760b..9b64eb0c24 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -223,14 +223,8 @@ static void virtio_setup(void)
 }
 }
 
-int main(void)
+static void ipl_boot_device(void)
 {
-sclp_setup();
-css_setup();
-boot_setup();
-find_boot_device();
-enable_subchannel(blk_schid);
-
 switch (cutype) {
 case CU_TYPE_DASD_3990:
 case CU_TYPE_DASD_2107:
@@ -242,8 +236,18 @@ int main(void)
 break;
 default:
 print_int("Attempting to boot from unexpected device type", cutype);
-panic("");
+panic("\nBoot failed.\n");
 }
+}
+
+int main(void)
+{
+sclp_setup();
+css_setup();
+boot_setup();
+find_boot_device();
+enable_subchannel(blk_schid);
+ipl_boot_device();
 
 panic("Failed to load OS from hard disk\n");
 return 0; /* make compiler happy */
-- 
2.18.1

[PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk

[Thomas Huth]

In case the user did not specify a boot device, we want to continue
looking for other devices if there are no valid SCSI disks on a virtio-
scsi controller. As a first step, do not panic in this case and let
the control flow carry the error to the upper functions instead.

Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/main.c  | 14 ++
 pc-bios/s390-ccw/s390-ccw.h  |  2 +-
 pc-bios/s390-ccw/virtio-blkdev.c |  7 +--
 pc-bios/s390-ccw/virtio-scsi.c   | 28 
 pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
 5 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 0d2aabbc58..7bdd12ab2e 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -218,7 +218,7 @@ static void find_boot_device(void)
 IPL_assert(found, "Boot device not found\n");
 }
 
-static void virtio_setup(void)
+static int virtio_setup(void)
 {
 VDev *vdev = virtio_get_device();
 QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
@@ -233,9 +233,14 @@ static void virtio_setup(void)
 sclp_print("Network boot device detected\n");
 vdev->netboot_start_addr = qipl.netboot_start_addr;
 } else {
-virtio_blk_setup_device(blk_schid);
+int ret = virtio_blk_setup_device(blk_schid);
+if (ret) {
+return ret;
+}
 IPL_assert(virtio_ipl_disk_is_valid(), "No valid IPL device detected");
 }
+
+return 0;
 }
 
 static void ipl_boot_device(void)
@@ -246,8 +251,9 @@ static void ipl_boot_device(void)
 dasd_ipl(blk_schid, cutype); /* no return */
 break;
 case CU_TYPE_VIRTIO:
-virtio_setup();
-zipl_load(); /* no return */
+if (virtio_setup() == 0) {
+zipl_load(); /* no return */
+}
 break;
 default:
 print_int("Attempting to boot from unexpected device type", cutype);
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index dbc4c64851..9b86c120b4 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -69,7 +69,7 @@ int sclp_read(char *str, size_t count);
 unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
  ulong subchan_id, void *load_addr);
 bool virtio_is_supported(SubChannelId schid);
-void virtio_blk_setup_device(SubChannelId schid);
+int virtio_blk_setup_device(SubChannelId schid);
 int virtio_read(ulong sector, void *load_addr);
 
 /* bootmap.c */
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 11c56261ca..7d35050292 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -263,9 +263,10 @@ uint64_t virtio_get_blocks(void)
 return 0;
 }
 
-void virtio_blk_setup_device(SubChannelId schid)
+int virtio_blk_setup_device(SubChannelId schid)
 {
 VDev *vdev = virtio_get_device();
+int ret = 0;
 
 vdev->schid = schid;
 virtio_setup_ccw(vdev);
@@ -288,9 +289,11 @@ void virtio_blk_setup_device(SubChannelId schid)
 "Config: CDB size mismatch");
 
 sclp_print("Using virtio-scsi.\n");
-virtio_scsi_setup(vdev);
+ret = virtio_scsi_setup(vdev);
 break;
 default:
 panic("\n! No IPL device available !\n");
 }
+
+return ret;
 }
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index eddfb8a7ad..2c8d0f3097 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -194,7 +194,12 @@ static bool scsi_read_capacity(VDev *vdev,
 
 /* virtio-scsi routines */
 
-static void virtio_scsi_locate_device(VDev *vdev)
+/*
+ * Tries to locate a SCSI device and and adds the information for the found
+ * device to the vdev->scsi_device structure.
+ * Returns 0 if SCSI device could be located, or a error code < 0 otherwise
+ */
+static int virtio_scsi_locate_device(VDev *vdev)
 {
 const uint16_t channel = 0; /* again, it's what QEMU does */
 uint16_t target;
@@ -220,7 +225,7 @@ static void virtio_scsi_locate_device(VDev *vdev)
 IPL_check(sdev->channel == 0, "non-zero channel requested");
 IPL_check(sdev->target <= vdev->config.scsi.max_target, "target# 
high");
 IPL_check(sdev->lun <= vdev->config.scsi.max_lun, "LUN# high");
-return;
+return 0;
 }
 
 for (target = 0; target <= vdev->config.scsi.max_target; target++) {
@@ -247,18 +252,20 @@ static void virtio_scsi_locate_device(VDev *vdev)
  */
 sdev->lun = r->lun[0].v16[0]; /* it's returned this way */
 debug_print_int("Have to use LUN", sdev->lun);
-return; /* we have to use this device */
+return 0; /* we have to use this device */
 }
 for (i = 0; i < luns; i++) {
 if (r->lun[i].v64) {
 /* Look for non-zero LUN - we have where to choose from */
 sdev->lun = 

[PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others

[Thomas Huth]

Remove the "#ifndef E..." guards from the defines here - the header
guard S390_CCW_H at the top of the file should avoid double definition,
and if the error code is defined in a different file already, we're in
trouble anyway, then it's better to see the error at compile time instead
of hunting weird behavior during runtime later.
Also define ENODEV - we will use this in a later patch.

Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/s390-ccw.h | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 36b884cced..dbc4c64851 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -27,12 +27,10 @@ typedef unsigned long long __u64;
 #define false 0
 #define PAGE_SIZE 4096
 
-#ifndef EIO
 #define EIO 1
-#endif
-#ifndef EBUSY
 #define EBUSY   2
-#endif
+#define ENODEV  3
+
 #ifndef NULL
 #define NULL0
 #endif
-- 
2.18.1

[PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable

[Thomas Huth]

The traditional / architected way of booting on s390x is to always
specify the device where the guest should be booted from - that means
s390x guests should be always started with a device that has the
"bootindex" property set.

For the users that are used to a firmware from a different CPU archi-
tecture (or the lazy s390x folks like myself), the s390-ccw bios also
tries to find a bootable device on its own in case the user did not
specify a "bootindex" property. Unfortunately, it always stops at the
very first device that it can find, no matter whether it's bootable or
not. That causes some weird behavior, for example while

 qemu-system-s390x -hda bootable.qcow2

boots perfectly fine, the bios refuses to work if you just specify
a virtio-scsi controller in front of it:

 qemu-system-s390x -device virtio-scsi -hda bootable.qcow2

While this is perfectly fine from the Z architecture point of view, it
still could be a little bit uncomfortable and confusing for the lazy
or ignorant users who did not specify a "bootindex". And since all major
firmwares on other architectures correctly boot in such cases, too,
let's also try to teach the s390-ccw bios how to boot in such cases.

For this, we have to get rid of the various panic()s and IPL_assert()
statements at the "low-level" function and let the main code handle
the decision instead whether a boot from a device should fail or not,
so that the main code can continue searching in case it wants to.

 Thomas

v2:
 - Add patch to remove superfluous call to enable_subchannel()
 - Add patch to test the new behavior in the tests/qtest/cdrom-test
 - Added Reviewed-bys from v1
 - Renamed check_sch_no() to is_dev_possibly_bootable()
 - Reworked the return codes to use 0/-ENODEV instead of true/false

Thomas Huth (9):
  pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and
-fno-common
  pc-bios/s390-ccw: Move ipl-related code from main() into a separate
function
  pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others
  pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate
function
  pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk
  pc-bios/s390-ccw: Scan through all devices if no boot device specified
  pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is
bad
  pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel()
  tests/qtest/cdrom: Add more s390x-related boot tests

 pc-bios/s390-ccw/Makefile|   7 +-
 pc-bios/s390-ccw/bootmap.c   |  34 --
 pc-bios/s390-ccw/main.c  | 172 +++
 pc-bios/s390-ccw/s390-ccw.h  |   8 +-
 pc-bios/s390-ccw/virtio-blkdev.c |   7 +-
 pc-bios/s390-ccw/virtio-scsi.c   |  28 +++--
 pc-bios/s390-ccw/virtio-scsi.h   |   2 +-
 tests/qtest/cdrom-test.c |  12 +++
 8 files changed, 174 insertions(+), 96 deletions(-)

-- 
2.18.1

Re: [PATCH-for-5.2 7/7] hw/block/fdc: Add ASCII art schema of QOM relations

[Philippe Mathieu-Daudé]

On 8/6/20 10:57 AM, Kevin Wolf wrote:
> Am 06.08.2020 um 10:08 hat Philippe Mathieu-Daudé geschrieben:
>> Without knowing the QEMU history, it is hard to relate QEMU objects
>> with the hardware datasheet.
>>
>> For example, one naively expects:
>>
>> * a floppy disk is plugged / unplugged on the bus
>>
>>   Wrong! QEMU floppy disks always sit on the bus. The block drives
>>   are plugged / unplugged on the disks, and the disks magically
>>   re-adapt their proprieties to match the block drive.
> 
> This is because what sits on the bus is not a floppy disk, but a floppy
> drive. FloppyDrive is also what the type is called.
> 
> The disk is represented by the BlockDriverState (the actual image file)
> that is inserted in the BlockBackend (which is logically part of the
> drive).
> 
>> * a floppy controller has a fixed number of disks pluggable on the bus
>>
>>   Wrong! QEMU floppy controllers have as much slots as the number of
>>   floppy drive provided when a machine is created. Then the ACPI table
>>   are generated and the number of slots can not be modified. So if you
>>   expect a dual slot controller being created with slot A and B, if
>>   the machine is created with a single drive attached, the controller
>>   will only have slot A created, and you will never be able to plug
>>   drive B without risking a mismatch in the ACPI tables.
> 
> Hm... I guess hotplugging floppy drives might actually have worked,
> though I have never tried it on real hardware. I'm pretty sure it wasn't
> an official feature, though, and ACPI tables certainly won't magically
> change if you do this because (apart from polling, I guess) software has
> no way to detect that you tinkered with the floppy cable. :-)
> 
>> * a floppy controller supporting 4 disks uses 2 buses
>>
>>   Wrong! QEMU uses a single bus to plug the 4 disks.
> 
> But we don't even emulate floppy controllers that can have more than two
> floppy drives:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -device floppy -device floppy -device 
> floppy
> qemu-system-x86_64: -device floppy: Can't create floppy unit 2, bus 
> supports only 2 units

This comment is for developers, the warning is for user.

It comes from:

if (dev->unit >= MAX_FD) {
error_setg(errp, "Can't create floppy unit %d, bus supports "
   "only %d units", dev->unit, MAX_FD);
return;
}

But you can compile QEMU with MAX_FD=4:

static FDrive *get_drv(FDCtrl *fdctrl, int unit)
{
switch (unit) {
case 0: return drv0(fdctrl);
case 1: return drv1(fdctrl);
#if MAX_FD == 4
case 2: return drv2(fdctrl);
case 3: return drv3(fdctrl);
#endif
default: return NULL;
}
}

ACPI also handles 4 slots:

static void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
{
Aml *dev;
Aml *crs;
int i;

#define ACPI_FDE_MAX_FD 4
uint32_t fde_buf[5] = {
0, 0, 0, 0, /* presence of floppy drives #0 - #3 */
cpu_to_le32(2)  /* tape presence (2 == never present) */
};

crs = aml_resource_template();
aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
aml_append(crs, aml_irq_no_flags(6));
aml_append(crs,
aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));

dev = aml_device("FDC0");
aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
aml_append(dev, aml_name_decl("_CRS", crs));

for (i = 0; i < MIN(MAX_FD, ACPI_FDE_MAX_FD); i++) {
FloppyDriveType type = isa_fdc_get_drive_type(isadev, i);

if (type < FLOPPY_DRIVE_TYPE_NONE) {
fde_buf[i] = cpu_to_le32(1);  /* drive present */
aml_append(dev, build_fdinfo_aml(i, type));
}
}
aml_append(dev, aml_name_decl("_FDE",
   aml_buffer(sizeof(fde_buf), (uint8_t *)fde_buf)));

aml_append(scope, dev);
}


> 
> This is checked in floppy_drive_realize(), so it applies to all
> variants of the controller.
> 
> If you want more floppy drives, you have to create a second controller
> (with a different iobase). Though I don't think I actually got this
> working when I tried. I wasn't sure if the problem was the emulation or
> the guest OSes (or SeaBIOS actually for DOS).
> 
>> As all these false assumptions are not obvious (we don't plug a disk,
>> we plug a block drive into a disk, etc...), start documenting the QOM
>> relationships with a simple ASCII schema.
> 
> Maybe be more specific to have: "floppy (drive)" and "blk (disk)".
> Because the ASCII schema is actually true, though you seem to have
> misunderstood what each item in it is supposed to represent.
> 
> Actually "blk (disk)" is not 100% accurate either because the drive
> always has a BlockBackend present. It's really the BlockDriverState
> inserted into the BlockBackend that is the disk.
> 
> In summary, to be honest, I believe since its qdevification, floppy is
> one of the block 

Re: [PATCH 1/3] qemu: implementation of transient option for qcow2 file

[Peter Krempa]

On Sun, Jul 19, 2020 at 22:56:50 -0400, Masayoshi Mizuma wrote:
> On Sat, Jul 18, 2020 at 08:06:00AM +0200, Peter Krempa wrote:
> > On Thu, Jul 16, 2020 at 20:55:29 -0400, Masayoshi Mizuma wrote:
> > > Thank you for your review.
> > > 
> > > On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
> > > > On 7/7/20 2:12 AM, Peter Krempa wrote:
> > > > 
> > > > > You can install a qcow2 overlay on top of a raw file too. IMO the
> > > > > implications of using  allow that.
> > > > > 
> > > > > As said above I'd strongly prefer if the overlay is created in qemu
> > > > > using the blockdev-create blockjob (there is already infrastructure in
> > > > > libvirt to achieve that).
> > > > 
> > > > Agreed.  At this point, any time we call out to qemu-img as a separate
> > > > process, we are probably doing it wrong.
> > > 
> > > Got it. I'm thinking about the procedure such as followings.
> > > Does that make sense?
> > > 
> > >   1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), 
> > >  and connect it.
> > 
> > Starting a new qemu process just to format an image is extreme overkill
> > and definitely not what we want to do.
> 
> I see, thanks.
> 
> > 
> > >   2) Setup the transient disk with 
> > > qemuDomainPrepareStorageSourceBlockdev(),
> > >  qemuBlockStorageSourceAttachApplyStorage(), 
> > > qemuBlockStorageSourceCreateGetFormatProps()
> > >  and something...
> > > 
> > >   3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then
> > >  close the monitor.
> > 
> > These two steps should be exectued in the qemu process which already
> > will run the VM prior to starting the guest CPUs.
> > 
> > >   4) Switch the original disk to the transient disk.
> > > 
> > >   5) Build the blockdev argument for qemu.
> > 
> > And instead of this step, you use the external snapshot infrastructure
> > to install the overlays via 'blockdev-snapshot' QMP command
> 
> OK. I suppose qemuDomainSnapshotDiskPrepare() and
> qemuDomainSnapshotDiskUpdateSource() maybe helpful to implement the
> setup steps of transient disk.
> 
> > 
> > > 
> > >   6) Run qemu
> > 
> > And instead of this the VM cpus will be started.
> 
> Got it, I think the appropriate place is after virCommandRun() at 
> qemuProcessLaunch(),
> and before qemuProcessFinishStartup().
> 
> > 
> > 
> > The above steps require factoring out snapshot code a bit. I have a few
> > patches in that direction so I'll try posting them next week hopefully.

Sorry this took longer than expected, but we were dealing with the build
system change.

The refactor is here:

https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html

You can now create an equivalent of 'qemuSnapshotDiskPrepare' which will
go through the disks and find the 'transient' ones. It will then create
snapshot data by a call to 'qemuSnapshotDiskPrepareOne' with a faked
snapshot disk object.

'qemuSnapshotDiskPrepareOne' ensures that the files are created and
formatted properly.

You then use same algorithm as 'qemuSnapshotCreateDiskActive'
(e.g. by extracting the common internals (basically everything except
call to 'qemuSnapshotDiskPrepare') into a separate function) and reuse
it when starting the VM as you've described above.

Note that all of the above can work only when QEMU_CAPS_BLOCKDEV is
supported.

The caveats/limitations with blockjobs and snapshots still apply though.
It depends on how you approach it. It's okay to limit/block the features
if transient disk is used. Alternatively you can implement some form of
private data to mark which image was transient and allow those
operations.

Re: [PATCH v12 09/11] qcow2_format.py: collect fields to dump in JSON format

[Vladimir Sementsov-Ogievskiy]

06.08.2020 12:08, Andrey Shinkevich wrote:

On 05.08.2020 19:58, Vladimir Sementsov-Ogievskiy wrote:

30.07.2020 17:15, Andrey Shinkevich wrote:

As __dict__ is being extended with class members we do not want to
print, add the to_dict() method to classes that returns a dictionary
with desired fields and their values. Extend it in subclass when
necessary to print the final dictionary in the JSON output which
follows.

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

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2000de3..a4114cb 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -119,6 +119,9 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
    print('{:<25} {}'.format(f[2], value_str))
  +    def to_dict(self):
+    return dict((f[2], self.__dict__[f[2]]) for f in self.fields)
+
    class Qcow2BitmapExt(Qcow2Struct):
  @@ -151,6 +154,11 @@ class Qcow2BitmapExt(Qcow2Struct):
  print()
  entry.dump()
  +    def to_dict(self):
+    fields_dict = super().to_dict()
+    fields_dict['bitmap_directory'] = self.bitmap_directory
+    return fields_dict
+
    class Qcow2BitmapDirEntry(Qcow2Struct):
  @@ -189,6 +197,14 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
  super(Qcow2BitmapDirEntry, self).dump()
  self.bitmap_table.dump()
  +    def to_dict(self):
+    fields_dict = super().to_dict()
+    fields_dict['bitmap_table'] = self.bitmap_table.entries


the fact that we have to access internals of bitmap_table is not nice, but 
let's refactor it later.


+    bmp_name = dict(name=self.name)
+    # Put the name ahead of the dict


Aha. I don't think that ordering is necessary for json, but why not to order it 
a bit.


+    bme_dict = {**bmp_name, **fields_dict}



strange to create bmp_name dict just to unpack it. You may as well do

bme_dict = {'name': self.name, **fields_dict}


+    return bme_dict


bme_dict is extra variable: it's created just to return it, so, how about just

return {'name': self.name, **fields_dict}


or, maybe

return {
   'name': self.name,
   **super().to_dict(),
   'bitmap_table': self.bitmap_table.entries
   }


+
    class Qcow2BitmapTableEntry(Qcow2Struct):
  @@ -214,6 +230,9 @@ class Qcow2BitmapTableEntry(Qcow2Struct):
  else:
  self.type = 'all-zeroes'
  +    def to_dict(self):
+    return dict(type=self.type, offset=self.offset, reserved=self.reserved)
+


Python has a special syntax for creating dicts :), let's use:

return {'type': self.type, 'offset': self.offset, 'reserved': self.reserved}



    class Qcow2BitmapTable:
  @@ -246,6 +265,9 @@ class QcowHeaderExtension(Qcow2Struct):
  0x44415441: 'Data file'
  }
  +    def to_dict(self):
+    return self.mapping.get(self.value, "")


aha, so, actually, to_dict() returns not dict, but string. So it should better 
be named to_json() (and return any json-dumpable object, like string or dict)

and then, we can add to_json() method to Qcow2BitmapTable as well, to return 
list.



So, should I refactor it now?



Up to you. Better yes if not difficult









+
  fields = (
  ('u32', Magic, 'magic'),
  ('u32', '{}', 'length')
@@ -308,6 +330,18 @@ class QcowHeaderExtension(Qcow2Struct):
  else:
  self.obj.dump()
  +    def to_dict(self):
+    fields_dict = super().to_dict()
+    ext_name = dict(name=self.Magic(self.magic))


strange that we have to create Magic object here... Ok, let's refactor it later


+    # Put the name ahead of the dict
+    he_dict = {**ext_name, **fields_dict}
+    if self.obj is not None:
+    he_dict['data'] = self.obj
+    else:
+    he_dict['data_str'] = self.data_str
+
+    return he_dict


again, let's avoid extra dict variables:

res = {'name': self.Magic(self.magic), **super().to_dict()}
if ...



+
  @classmethod
  def create(cls, magic, data):
  return QcowHeaderExtension(magic, len(data), data)







--
Best regards,
Vladimir

Re: [PATCH] qcow2: flush qcow2 l2 meta for new allocated clusters

[Kevin Wolf]

Am 05.08.2020 um 04:38 hat Ying Fang geschrieben:
> From: fangying 
> 
> When qemu or qemu-nbd process uses a qcow2 image and configured with
> 'cache = none', it will write to the qcow2 image with a cache to cache
> L2 tables, however the process will not use L2 tables without explicitly
> calling the flush command or closing the mirror flash into the disk.
> Which may cause the disk data inconsistent with the written data for
> a long time. If an abnormal process exit occurs here, the issued written
> data will be lost.
> 
> Therefore, in order to keep data consistency we need to flush the changes
> to the L2 entry to the disk in time for the newly allocated cluster.
> 
> Signed-off-by: Ying Fang 

If you want to have data safely written to the disk after each write
request, you need to use cache=writethrough/directsync (in other words,
aliases that are equivalent to setting -device ...,write-cache=off).
Note that this will have a major impact on write performance.

cache=none means bypassing the kernel page cache (O_DIRECT), but not
flushing after each write request.

Kevin

Re: [PATCH v12 09/11] qcow2_format.py: collect fields to dump in JSON format

[Andrey Shinkevich]

On 05.08.2020 19:58, Vladimir Sementsov-Ogievskiy wrote:

30.07.2020 17:15, Andrey Shinkevich wrote:

As __dict__ is being extended with class members we do not want to
print, add the to_dict() method to classes that returns a dictionary
with desired fields and their values. Extend it in subclass when
necessary to print the final dictionary in the JSON output which
follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 34 
++

  1 file changed, 34 insertions(+)

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

index 2000de3..a4114cb 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -119,6 +119,9 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
    print('{:<25} {}'.format(f[2], value_str))
  +    def to_dict(self):
+    return dict((f[2], self.__dict__[f[2]]) for f in self.fields)
+
    class Qcow2BitmapExt(Qcow2Struct):
  @@ -151,6 +154,11 @@ class Qcow2BitmapExt(Qcow2Struct):
  print()
  entry.dump()
  +    def to_dict(self):
+    fields_dict = super().to_dict()
+    fields_dict['bitmap_directory'] = self.bitmap_directory
+    return fields_dict
+
    class Qcow2BitmapDirEntry(Qcow2Struct):
  @@ -189,6 +197,14 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
  super(Qcow2BitmapDirEntry, self).dump()
  self.bitmap_table.dump()
  +    def to_dict(self):
+    fields_dict = super().to_dict()
+    fields_dict['bitmap_table'] = self.bitmap_table.entries


the fact that we have to access internals of bitmap_table is not nice, 
but let's refactor it later.



+    bmp_name = dict(name=self.name)
+    # Put the name ahead of the dict


Aha. I don't think that ordering is necessary for json, but why not to 
order it a bit.



+    bme_dict = {**bmp_name, **fields_dict}



strange to create bmp_name dict just to unpack it. You may as well do

bme_dict = {'name': self.name, **fields_dict}


+    return bme_dict


bme_dict is extra variable: it's created just to return it, so, how 
about just


return {'name': self.name, **fields_dict}


or, maybe

return {
   'name': self.name,
   **super().to_dict(),
   'bitmap_table': self.bitmap_table.entries
   }


+
    class Qcow2BitmapTableEntry(Qcow2Struct):
  @@ -214,6 +230,9 @@ class Qcow2BitmapTableEntry(Qcow2Struct):
  else:
  self.type = 'all-zeroes'
  +    def to_dict(self):
+    return dict(type=self.type, offset=self.offset, 
reserved=self.reserved)

+


Python has a special syntax for creating dicts :), let's use:

return {'type': self.type, 'offset': self.offset, 'reserved': 
self.reserved}




    class Qcow2BitmapTable:
  @@ -246,6 +265,9 @@ class QcowHeaderExtension(Qcow2Struct):
  0x44415441: 'Data file'
  }
  +    def to_dict(self):
+    return self.mapping.get(self.value, "")


aha, so, actually, to_dict() returns not dict, but string. So it 
should better be named to_json() (and return any json-dumpable object, 
like string or dict)


and then, we can add to_json() method to Qcow2BitmapTable as well, to 
return list.



So, should I refactor it now?

Andrey






+
  fields = (
  ('u32', Magic, 'magic'),
  ('u32', '{}', 'length')
@@ -308,6 +330,18 @@ class QcowHeaderExtension(Qcow2Struct):
  else:
  self.obj.dump()
  +    def to_dict(self):
+    fields_dict = super().to_dict()
+    ext_name = dict(name=self.Magic(self.magic))


strange that we have to create Magic object here... Ok, let's refactor 
it later



+    # Put the name ahead of the dict
+    he_dict = {**ext_name, **fields_dict}
+    if self.obj is not None:
+    he_dict['data'] = self.obj
+    else:
+    he_dict['data_str'] = self.data_str
+
+    return he_dict


again, let's avoid extra dict variables:

res = {'name': self.Magic(self.magic), **super().to_dict()}
if ...



+
  @classmethod
  def create(cls, magic, data):
  return QcowHeaderExtension(magic, len(data), data)

Re: [PATCH] qcow2: flush qcow2 l2 meta for new allocated clusters

[Daniel P . Berrangé]

On Thu, Aug 06, 2020 at 05:01:51PM +0800, Ying Fang wrote:
> 
> 
> On 8/5/2020 10:43 AM, no-re...@patchew.org wrote:
> > Patchew URL: 
> > https://patchew.org/QEMU/20200805023826.184-1-fangyi...@huawei.com/
> > 
> > 
> > 
> > Hi,
> > 
> > This series failed the docker-quick@centos7 build test. Please find the 
> > testing commands and
> > their output below. If you have Docker installed, you can probably 
> > reproduce it
> > locally.
> > I see some error message which says ** No space left on device **
> However I do not know what is wrong with this build test.
> Could you give me some help here?

It isn't your fault - this is just QEMU's  patchew CI that is broken yet again
due to lack of disk space. Just ignore the error report here.

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] qcow2: flush qcow2 l2 meta for new allocated clusters

[Ying Fang]

On 8/5/2020 10:43 AM, no-re...@patchew.org wrote:

Patchew URL: https://patchew.org/QEMU/20200805023826.184-1-fangyi...@huawei.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
I see some error message which says ** No space left on device **

However I do not know what is wrong with this build test.
Could you give me some help here?

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
error: copy-fd: write returned No space left on device
fatal: failed to copy file to 
'/var/tmp/patchew-tester-tmp-wtnwtuq5/src/.git/objects/pack/pack-518a8ad92e3ce11d2627a7221e2d360b337cb27d.pack': 
No space left on device

fatal: The remote end hung up unexpectedly
Traceback (most recent call last):
  File "patchew-tester/src/patchew-cli", line 521, in test_one
git_clone_repo(clone, r["repo"], r["head"], logf, True)
  File "patchew-tester/src/patchew-cli", line 53, in git_clone_repo
subprocess.check_call(clone_cmd, stderr=logf, stdout=logf)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", 
line 291, in check_call

raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'clone', '-q', 
'/home/patchew/.cache/patchew-git-cache/httpsgithubcompatchewprojectqemu-3c8cf5a9c21ff8782164d1def7f44bd888713384', 
'/var/tmp/patchew-tester-tmp-wtnwtuq5/src']' returned non-zero exit 
status 128.








The full log is available at
http://patchew.org/logs/20200805023826.184-1-fangyi...@huawei.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH] ide: Get rid of IDEDrive struct

[Peter Maydell]

On Wed, 5 Aug 2020 at 23:14, Eduardo Habkost  wrote:
> On Wed, Aug 05, 2020 at 09:41:25PM +0100, Peter Maydell wrote:
> > This is one of those areas where this change works and reduces
> > amount of code, but on the other hand it means the QOM type
> > doesn't follow the common pattern for a leaf type of:
> >  * it has a struct
> >  * it has cast macros that cast to that struct
> >  * the typeinfo instance_size is the size of that struct
> > (it wasn't exactly following this pattern before, of course).
>
> Is this really a pattern that exists and we want to follow?
> I don't see why that pattern would be useful for simple leaf
> types.

Most leaf types need this. Consider a simple device type
like TYPE_CMSDK_APB_UART. It has a TYPE_* name so that
users of it can instantiate it; it has a CMSDKAPBUART struct
that holds all the device state; it has the CMSDK_APB_UART()
cast macro so that code that gets a Device* or Object* can
get at the struct. Leaf types like ide-hd which have no
actual state of their own are I think the less common case:
most leaf types do have at least some member variables.

As Markus says, we can have a couple of standard patterns
if we want to (as we do for the class-macro conventions);
I just wanted to explain that lots of leaf types work the
way I outline above.

thanks
-- PMM

Re: [PATCH-for-5.2 7/7] hw/block/fdc: Add ASCII art schema of QOM relations

[Kevin Wolf]

Am 06.08.2020 um 10:08 hat Philippe Mathieu-Daudé geschrieben:
> Without knowing the QEMU history, it is hard to relate QEMU objects
> with the hardware datasheet.
> 
> For example, one naively expects:
> 
> * a floppy disk is plugged / unplugged on the bus
> 
>   Wrong! QEMU floppy disks always sit on the bus. The block drives
>   are plugged / unplugged on the disks, and the disks magically
>   re-adapt their proprieties to match the block drive.

This is because what sits on the bus is not a floppy disk, but a floppy
drive. FloppyDrive is also what the type is called.

The disk is represented by the BlockDriverState (the actual image file)
that is inserted in the BlockBackend (which is logically part of the
drive).

> * a floppy controller has a fixed number of disks pluggable on the bus
> 
>   Wrong! QEMU floppy controllers have as much slots as the number of
>   floppy drive provided when a machine is created. Then the ACPI table
>   are generated and the number of slots can not be modified. So if you
>   expect a dual slot controller being created with slot A and B, if
>   the machine is created with a single drive attached, the controller
>   will only have slot A created, and you will never be able to plug
>   drive B without risking a mismatch in the ACPI tables.

Hm... I guess hotplugging floppy drives might actually have worked,
though I have never tried it on real hardware. I'm pretty sure it wasn't
an official feature, though, and ACPI tables certainly won't magically
change if you do this because (apart from polling, I guess) software has
no way to detect that you tinkered with the floppy cable. :-)

> * a floppy controller supporting 4 disks uses 2 buses
> 
>   Wrong! QEMU uses a single bus to plug the 4 disks.

But we don't even emulate floppy controllers that can have more than two
floppy drives:

$ x86_64-softmmu/qemu-system-x86_64 -device floppy -device floppy -device 
floppy
qemu-system-x86_64: -device floppy: Can't create floppy unit 2, bus 
supports only 2 units

This is checked in floppy_drive_realize(), so it applies to all
variants of the controller.

If you want more floppy drives, you have to create a second controller
(with a different iobase). Though I don't think I actually got this
working when I tried. I wasn't sure if the problem was the emulation or
the guest OSes (or SeaBIOS actually for DOS).

> As all these false assumptions are not obvious (we don't plug a disk,
> we plug a block drive into a disk, etc...), start documenting the QOM
> relationships with a simple ASCII schema.

Maybe be more specific to have: "floppy (drive)" and "blk (disk)".
Because the ASCII schema is actually true, though you seem to have
misunderstood what each item in it is supposed to represent.

Actually "blk (disk)" is not 100% accurate either because the drive
always has a BlockBackend present. It's really the BlockDriverState
inserted into the BlockBackend that is the disk.

In summary, to be honest, I believe since its qdevification, floppy is
one of the block devices that is modelled the best on the QOM + block
backend level. Only SCSI might be comparable, but IDE, virtio-blk and
usb-storage are a mess in comparison.

Kevin

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/block/fdc.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 6944b06e4b..b109f37050 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -47,6 +47,28 @@
>  #include "qemu/module.h"
>  #include "trace.h"
>  
> +/*
> + * QOM relationship:
> + * =
> + *
> + *  +---+
> + *  |   |
> + * isa/sysbus  <--->|   |
> + *  |   |
> + *  irq/dma<|fdc|
> + *  |
> + *  clk>|   |+-+--+-++-+--+-+
> + *  |   || | blk  | || | blk  | |
> + *  ++--+| |  | || |  | |
> + *   |   | +--+ || +--+ |
> + *   |   |  ||  |
> + *   |   |  floppy  ||  floppy  |
> + *   |   ++-+++-+
> + *   |   floppy-bus   |   |
> + *   +v---v---
> + *
> + */
> +
>  //
>  /* debug Floppy devices */
>  
> -- 
> 2.21.3
> 

Re: [PATCH] ide: Get rid of IDEDrive struct

[Daniel P . Berrangé]

On Thu, Aug 06, 2020 at 07:58:06AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > On Wed, Aug 05, 2020 at 09:41:25PM +0100, Peter Maydell wrote:
> >> On Wed, 5 Aug 2020 at 20:49, Eduardo Habkost  wrote:
> >> >
> >> > The struct had a single field (IDEDevice dev), and is only used
> >> > in the QOM type declarations and property lists.  We can simply
> >> > use the IDEDevice struct directly instead.
> >> >
> >> > Signed-off-by: Eduardo Habkost 
> >> > @@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, 
> >> > void *data)
> >> >  static const TypeInfo ide_hd_info = {
> >> >  .name  = "ide-hd",
> >> >  .parent= TYPE_IDE_DEVICE,
> >> > -.instance_size = sizeof(IDEDrive),
> >> >  .class_init= ide_hd_class_init,
> >> >  };
> >> 
> >> This is one of those areas where this change works and reduces
> >> amount of code, but on the other hand it means the QOM type
> >> doesn't follow the common pattern for a leaf type of:
> >>  * it has a struct
> >>  * it has cast macros that cast to that struct
> >>  * the typeinfo instance_size is the size of that struct
> >> (it wasn't exactly following this pattern before, of course).
> >
> > Is this really a pattern that exists and we want to follow?
> > I don't see why that pattern would be useful for simple leaf
> > types.
> 
> I think the pattern exists, but we deviate from it in quite a few
> places, probably just because it's so much boilerplate.
> 
> Related: Daniel's "[PATCH 0/4] qom: reduce boilerplate required for
> declaring and defining objects".  Perhaps Daniel has an opinion on
> taking shortcuts with leaf types.

I think following a consistent pattern everywhere is important,
because people look at existing code to guide their new code.
The boilerplate pain is very real, but I think my patch series
you point to will reduce the burden sufficiently that the kind
of optimization proposed here is not required.

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 :|

[PATCH-for-5.2 3/7] hw/block/fdc: Use warn_report() instead of debug FLOPPY_DPRINTF() calls

[Philippe Mathieu-Daudé]

Use warn_report() instead of debug FLOPPY_DPRINTF() calls.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/fdc.c | 32 +++-
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index c91ed7ee2d..ee45ec0b27 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -395,12 +395,10 @@ static int pick_geometry(FDrive *drv)
 if (match == -1) {
 if (size_match != -1) {
 parse = _formats[size_match];
-FLOPPY_DPRINTF("User requested floppy drive type '%s', "
-   "but inserted medium appears to be a "
-   "%"PRId64" sector '%s' type\n",
-   FloppyDriveType_str(drv->drive),
-   nb_sectors,
-   FloppyDriveType_str(parse->drive));
+warn_report("User requested floppy drive type '%s', but inserted "
+"medium appears to be a %"PRId64" sector '%s' type",
+FloppyDriveType_str(drv->drive), nb_sectors,
+FloppyDriveType_str(parse->drive));
 }
 assert(type_match != -1 && "misconfigured fd_format");
 match = type_match;
@@ -1805,8 +1803,8 @@ static int fdctrl_transfer_handler (void *opaque, int 
nchan,
 /* READ & SCAN commands and realign to a sector for WRITE */
 if (blk_pread(cur_drv->blk, fd_offset(cur_drv),
   fdctrl->fifo, BDRV_SECTOR_SIZE) < 0) {
-FLOPPY_DPRINTF("Floppy: error getting sector %d\n",
-   fd_sector(cur_drv));
+warn_report("Floppy: error getting sector %" PRIu32,
+fd_sector(cur_drv));
 /* Sure, image size is too small... */
 memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
 }
@@ -1833,8 +1831,8 @@ static int fdctrl_transfer_handler (void *opaque, int 
nchan,
fdctrl->data_pos, len);
 if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
fdctrl->fifo, BDRV_SECTOR_SIZE, 0) < 0) {
-FLOPPY_DPRINTF("error writing sector %d\n",
-   fd_sector(cur_drv));
+warn_report("error writing sector %" PRIu32,
+fd_sector(cur_drv));
 fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 
0x00, 0x00);
 goto transfer_error;
 }
@@ -1911,8 +1909,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
 if (pos == 0) {
 if (fdctrl->data_pos != 0)
 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
-FLOPPY_DPRINTF("error seeking to next sector %d\n",
-   fd_sector(cur_drv));
+warn_report("error seeking to next sector %" PRIu32,
+fd_sector(cur_drv));
 return 0;
 }
 if (blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
@@ -1997,7 +1995,7 @@ static void fdctrl_format_sector(FDCtrl *fdctrl)
 if (cur_drv->blk == NULL ||
 blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
BDRV_SECTOR_SIZE, 0) < 0) {
-FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
+warn_report("error formatting sector %" PRIu32, fd_sector(cur_drv));
 fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
 } else {
 if (cur_drv->sect == cur_drv->last_sect) {
@@ -2421,13 +2419,13 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t 
value)
 cur_drv = get_cur_drv(fdctrl);
 if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
BDRV_SECTOR_SIZE, 0) < 0) {
-FLOPPY_DPRINTF("error writing sector %d\n",
-   fd_sector(cur_drv));
+warn_report("error writing sector %" PRIu32,
+fd_sector(cur_drv));
 break;
 }
 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
-FLOPPY_DPRINTF("error seeking to next sector %d\n",
-   fd_sector(cur_drv));
+warn_report("error seeking to next sector %" PRIu32,
+fd_sector(cur_drv));
 break;
 }
 }
-- 
2.21.3

[PATCH-for-5.2 6/7] hw/block/fdc: Use more descriptive TypeInfo names

[Philippe Mathieu-Daudé]

Better name TypeInfo structures:

- ISA bus
- Common floppy controller
- Intel 82078 floppy controller
- SUN floppy controller

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/fdc.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 278220ed29..6944b06e4b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2930,7 +2930,7 @@ static void isabus_fdc_instance_init(Object *obj)
   DEVICE(obj));
 }
 
-static const TypeInfo isa_fdc_info = {
+static const TypeInfo isabus_fdc_info = {
 .name  = TYPE_ISA_FDC,
 .parent= TYPE_ISA_DEVICE,
 .instance_size = sizeof(FDCtrlISABus),
@@ -2971,7 +2971,7 @@ static void sysbus_fdc_class_init(ObjectClass *klass, 
void *data)
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
-static const TypeInfo sysbus_fdc_info = {
+static const TypeInfo sysbus_fdc_i82078_info = {
 .name  = "sysbus-fdc",
 .parent= TYPE_SYSBUS_FDC,
 .instance_init = sysbus_fdc_initfn,
@@ -2997,7 +2997,7 @@ static void sun4m_fdc_class_init(ObjectClass *klass, void 
*data)
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
-static const TypeInfo sun4m_fdc_info = {
+static const TypeInfo sysbus_fdc_sun4m_info = {
 .name  = "SUNW,fdtwo",
 .parent= TYPE_SYSBUS_FDC,
 .instance_init = sun4m_fdc_initfn,
@@ -3013,7 +3013,7 @@ static void sysbus_fdc_common_class_init(ObjectClass 
*klass, void *data)
 dc->vmsd = _sysbus_fdc;
 }
 
-static const TypeInfo sysbus_fdc_type_info = {
+static const TypeInfo sysbus_fdc_common_info = {
 .name  = TYPE_SYSBUS_FDC,
 .parent= TYPE_SYS_BUS_DEVICE,
 .instance_size = sizeof(FDCtrlSysBus),
@@ -3024,10 +3024,12 @@ static const TypeInfo sysbus_fdc_type_info = {
 
 static void fdc_register_types(void)
 {
-type_register_static(_fdc_info);
-type_register_static(_fdc_type_info);
-type_register_static(_fdc_info);
-type_register_static(_fdc_info);
+type_register_static(_fdc_info);
+
+type_register_static(_fdc_common_info);
+type_register_static(_fdc_i82078_info);
+type_register_static(_fdc_sun4m_info);
+
 type_register_static(_bus_info);
 type_register_static(_drive_info);
 }
-- 
2.21.3

[PATCH-for-5.2 5/7] hw/block/fdc: Drop pointless FLOPPY_DPRINTF() call

[Philippe Mathieu-Daudé]

Remove not very helpful debug call.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/fdc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f9f3f3c079..278220ed29 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2636,7 +2636,6 @@ static void fdctrl_realize_common(DeviceState *dev, 
FDCtrl *fdctrl,
 }
 }
 
-FLOPPY_DPRINTF("init controller\n");
 fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
 memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
 fdctrl->fifo_size = 512;
-- 
2.21.3

[PATCH-for-5.2 1/7] hw/block/fdc: Let sector count be unsigned

[Philippe Mathieu-Daudé]

Sectors count can not be negative, make it unsigned.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/fdc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e9ed3eef45..2cec7568c1 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -245,14 +245,14 @@ static void fd_init(FDrive *drv)
 
 #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
 
-static int fd_sector_calc(uint8_t head, uint8_t track, uint8_t sect,
-  uint8_t last_sect, uint8_t num_sides)
+static uint32_t fd_sector_calc(uint8_t head, uint8_t track, uint8_t sect,
+   uint8_t last_sect, uint8_t num_sides)
 {
 return (((track * num_sides) + head) * last_sect) + sect - 1;
 }
 
 /* Returns current position, in sectors, for given drive */
-static int fd_sector(FDrive *drv)
+static uint32_t fd_sector(FDrive *drv)
 {
 return fd_sector_calc(drv->head, drv->track, drv->sect, drv->last_sect,
   NUM_SIDES(drv));
-- 
2.21.3

[PATCH-for-5.2 7/7] hw/block/fdc: Add ASCII art schema of QOM relations

[Philippe Mathieu-Daudé]

Without knowing the QEMU history, it is hard to relate QEMU objects
with the hardware datasheet.

For example, one naively expects:

* a floppy disk is plugged / unplugged on the bus

  Wrong! QEMU floppy disks always sit on the bus. The block drives
  are plugged / unplugged on the disks, and the disks magically
  re-adapt their proprieties to match the block drive.

* a floppy controller has a fixed number of disks pluggable on the bus

  Wrong! QEMU floppy controllers have as much slots as the number of
  floppy drive provided when a machine is created. Then the ACPI table
  are generated and the number of slots can not be modified. So if you
  expect a dual slot controller being created with slot A and B, if
  the machine is created with a single drive attached, the controller
  will only have slot A created, and you will never be able to plug
  drive B without risking a mismatch in the ACPI tables.

* a floppy controller supporting 4 disks uses 2 buses

  Wrong! QEMU uses a single bus to plug the 4 disks.

As all these false assumptions are not obvious (we don't plug a disk,
we plug a block drive into a disk, etc...), start documenting the QOM
relationships with a simple ASCII schema.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/fdc.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 6944b06e4b..b109f37050 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -47,6 +47,28 @@
 #include "qemu/module.h"
 #include "trace.h"
 
+/*
+ * QOM relationship:
+ * =
+ *
+ *  +---+
+ *  |   |
+ * isa/sysbus  <--->|   |
+ *  |   |
+ *  irq/dma<|fdc|
+ *  |
+ *  clk>|   |+-+--+-++-+--+-+
+ *  |   || | blk  | || | blk  | |
+ *  ++--+| |  | || |  | |
+ *   |   | +--+ || +--+ |
+ *   |   |  ||  |
+ *   |   |  floppy  ||  floppy  |
+ *   |   ++-+++-+
+ *   |   floppy-bus   |   |
+ *   +v---v---
+ *
+ */
+
 //
 /* debug Floppy devices */
 
-- 
2.21.3

[PATCH-for-5.2 4/7] hw/block/fdc: Convert debug FLOPPY_DPRINTF() to trace events

[Philippe Mathieu-Daudé]

Convert debug FLOPPY_DPRINTF() to trace events.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/fdc.c| 6 +++---
 hw/block/trace-events | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index ee45ec0b27..f9f3f3c079 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -326,7 +326,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t 
track, uint8_t sect,
 /* Set drive back to track 0 */
 static void fd_recalibrate(FDrive *drv)
 {
-FLOPPY_DPRINTF("recalibrate\n");
+trace_floppy_recalibrate();
 fd_seek(drv, 0, 0, 1, 1);
 }
 
@@ -438,7 +438,7 @@ static void fd_revalidate(FDrive *drv)
 {
 int rc;
 
-FLOPPY_DPRINTF("revalidate\n");
+trace_floppy_revalidate();
 if (drv->blk != NULL) {
 drv->ro = blk_is_read_only(drv->blk);
 if (!blk_is_inserted(drv->blk)) {
@@ -1283,7 +1283,7 @@ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
 {
 int i;
 
-FLOPPY_DPRINTF("reset controller\n");
+trace_fdc_reset();
 fdctrl_reset_irq(fdctrl);
 /* Initialise controller */
 fdctrl->sra = 0;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 958fcc5508..9f7caf9b17 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -1,8 +1,11 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # fdc.c
+fdc_reset(void) ""
 fdc_ioport_read(uint8_t reg, uint8_t value) "read reg 0x%02x val 0x%02x"
 fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
+floppy_recalibrate(void) ""
+floppy_revalidate(void) ""
 
 # pflash_cfi02.c
 # pflash_cfi01.c
-- 
2.21.3

[PATCH-for-5.2 2/7] hw/block/fdc: Let sector offset be unsigned

[Philippe Mathieu-Daudé]

Sector offset can not be negative, make it unsigned.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/fdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2cec7568c1..c91ed7ee2d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -259,7 +259,7 @@ static uint32_t fd_sector(FDrive *drv)
 }
 
 /* Returns current position, in bytes, for given drive */
-static int fd_offset(FDrive *drv)
+static uint64_t fd_offset(FDrive *drv)
 {
 g_assert(fd_sector(drv) < INT_MAX >> BDRV_SECTOR_BITS);
 return fd_sector(drv) << BDRV_SECTOR_BITS;
-- 
2.21.3

[PATCH-for-5.2 0/7] hw/block/fdc: Cleanups trying to make sense of the floppy controllers

[Philippe Mathieu-Daudé]

Trivial patches while trying to make sense of the floppy
controllers code.

Philippe Mathieu-Daudé (7):
  hw/block/fdc: Let sector count be unsigned
  hw/block/fdc: Let sector offset be unsigned
  hw/block/fdc: Use warn_report() instead of debug FLOPPY_DPRINTF()
calls
  hw/block/fdc: Convert debug FLOPPY_DPRINTF() to trace events
  hw/block/fdc: Drop pointless FLOPPY_DPRINTF() call
  hw/block/fdc: Use more descriptive TypeInfo names
  hw/block/fdc: Add ASCII art schema of QOM relations

 hw/block/fdc.c| 87 +++
 hw/block/trace-events |  3 ++
 2 files changed, 57 insertions(+), 33 deletions(-)

-- 
2.21.3

Re: [PATCH v2 1/3] async: rename event_notifier_dummy_cb/poll()

[Philippe Mathieu-Daudé]

On 8/5/20 12:00 PM, Stefan Hajnoczi wrote:
> The event_notifier_*() prefix can be confused with the EventNotifier
> APIs that are also called event_notifier_*().
> 
> Rename the functions to aio_context_notifier_*() to make it clear that
> they relate to the AioContext::notifier field.
> 
> Signed-off-by: Stefan Hajnoczi 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  util/async.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/util/async.c b/util/async.c
> index 1319eee3bc..d9f987e133 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -445,12 +445,12 @@ static void aio_timerlist_notify(void *opaque, 
> QEMUClockType type)
>  aio_notify(opaque);
>  }
>  
> -static void event_notifier_dummy_cb(EventNotifier *e)
> +static void aio_context_notifier_dummy_cb(EventNotifier *e)
>  {
>  }
>  
>  /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
> -static bool event_notifier_poll(void *opaque)
> +static bool aio_context_notifier_poll(void *opaque)
>  {
>  EventNotifier *e = opaque;
>  AioContext *ctx = container_of(e, AioContext, notifier);
> @@ -508,8 +508,8 @@ AioContext *aio_context_new(Error **errp)
>  
>  aio_set_event_notifier(ctx, >notifier,
> false,
> -   event_notifier_dummy_cb,
> -   event_notifier_poll);
> +   aio_context_notifier_dummy_cb,
> +   aio_context_notifier_poll);
>  #ifdef CONFIG_LINUX_AIO
>  ctx->linux_aio = NULL;
>  #endif
> 

Re: [PATCH] block/vhdx: Support vhdx image only with 512 bytes logical sector size

[Philippe Mathieu-Daudé]

On 8/5/20 11:30 PM, Swapnil Ingle wrote:
> block/vhdx uses qemu block layer where sector size is always 512 byte.

"bytes".

> This may have issues  with 4K logical sector sized vhdx image.
> 
> For e.g qemu-img convert on such images fails with following assert:
> 
> $qemu-img convert -f vhdx -O raw 4KTest1.vhdx test.raw
> qemu-img: util/iov.c:388: qiov_slice: Assertion `offset + len <=
> qiov->size' failed.
> Aborted
> 
> This patch adds an check to return ENOTSUP for vhdx images which
> has logical sector size other than 512 bytes.

Probably "which have".

> 
> Signed-off-by: Swapnil Ingle 
> ---
>  block/vhdx.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 791eb90..356ec4c 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -816,9 +816,9 @@ static int vhdx_parse_metadata(BlockDriverState *bs, 
> BDRVVHDXState *s)
>  goto exit;
>  }
>  
> -/* only 2 supported sector sizes */
> -if (s->logical_sector_size != 512 && s->logical_sector_size != 4096) {
> -ret = -EINVAL;
> +/* Currently we only support 512 */
> +if (s->logical_sector_size != 512) {
> +ret = -ENOTSUP;
>  goto exit;
>  }
>  
>