[PULL 6/8] python: silence pylint raising-non-exception error
From: John Snow As of (at least) pylint 3.3.1, this code trips pylint up into believing we are raising something other than an Exception. We are not: the first two values may indeed be "None", but the last and final value must by definition be a SystemExit exception. Signed-off-by: John Snow Message-ID: <20241101173700.965776-5-js...@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- python/scripts/mkvenv.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/scripts/mkvenv.py b/python/scripts/mkvenv.py index f2526af0a0..8ac5b0b2a0 100644 --- a/python/scripts/mkvenv.py +++ b/python/scripts/mkvenv.py @@ -379,6 +379,9 @@ def make_venv( # pylint: disable=too-many-arguments try: builder.create(str(env_dir)) except SystemExit as exc: +# pylint 3.3 bug: +# pylint: disable=raising-non-exception, raise-missing-from + # Some versions of the venv module raise SystemExit; *nasty*! # We want the exception that prompted it. It might be a subprocess # error that has output we *really* want to see. -- 2.47.0
[PULL 2/8] parallels: fix possible int overflow
From: Dmitry Frolov
The sum "cluster_index + count" may overflow uint32_t.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Signed-off-by: Dmitry Frolov
Message-ID: <20241106080521.219255-2-fro...@swemel.ru>
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
block/parallels.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 9205a0864f..071b6dcaf8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -184,11 +184,11 @@ static int mark_used(BlockDriverState *bs, unsigned long
*bitmap,
BDRVParallelsState *s = bs->opaque;
uint32_t cluster_index = host_cluster_index(s, off);
unsigned long next_used;
-if (cluster_index + count > bitmap_size) {
+if ((uint64_t)cluster_index + count > bitmap_size) {
return -E2BIG;
}
next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
-if (next_used < cluster_index + count) {
+if (next_used < (uint64_t)cluster_index + count) {
return -EBUSY;
}
bitmap_set(bitmap, cluster_index, count);
--
2.47.0
[PULL 1/8] migration: Check current_migration in migration_is_running()
From: Peter Xu
Report shows that commit 34a8892dec broke iotest 055:
https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c...@linaro.org
When replacing migration_is_idle() with "!migration_is_running()", it was
overlooked that the idle helper also checks for current_migration being
available first.
The check would be there if the whole series was applied, but since the
last patches in the previous series rely on some other patches to land
first, we need to recover the behavior of migration_is_idle() first before
that whole set will be merged.
I left migration_is_active / migration_is_device alone, as I don't think
it's possible for them to hit his case (current_migration not initialized).
Also they're prone to removal soon from VFIO side.
Cc: Fabiano Rosas
Cc: Peter Maydell
Fixes: 34a8892dec ("migration: Drop migration_is_idle()")
Reported-by: Pierrick Bouvier
Tested-by: Pierrick Bouvier
Signed-off-by: Peter Xu
Message-ID: <20241105182725.2393425-1-pet...@redhat.com>
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
migration/migration.c | 4
1 file changed, 4 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index aedf7f0751..8c5bd0a75c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1117,6 +1117,10 @@ bool migration_is_running(void)
{
MigrationState *s = current_migration;
+if (!s) {
+return false;
+}
+
switch (s->state) {
case MIGRATION_STATUS_ACTIVE:
case MIGRATION_STATUS_POSTCOPY_ACTIVE:
--
2.47.0
[PULL 4/8] iotests: correct resultclass type in ReproducibleTestRunner
From: John Snow I have a vague memory that I suggested this base class to Vladimir and said "Maybe someday it will break, and I'll just fix it then." Guess that's today. Fixes various mypy errors in the "make check-tox" python test for at least Python3.8; seemingly requires a fairly modern mypy and/or Python base version to trigger. Signed-off-by: John Snow Message-ID: <20241101173700.965776-3-js...@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/iotests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 673bbcd356..19817c7353 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -1617,7 +1617,7 @@ class ReproducibleTestRunner(unittest.TextTestRunner): def __init__( self, stream: Optional[TextIO] = None, -resultclass: Type[unittest.TestResult] = +resultclass: Type[unittest.TextTestResult] = ReproducibleTestResult, **kwargs: Any ) -> None: -- 2.47.0
[PULL 7/8] qdev-monitor: avoid QemuOpts in QMP device_add
From: Stefan Hajnoczi
The QMP device_add monitor command converts the QDict arguments to
QemuOpts and then back again to QDict. This process only supports scalar
types. Device properties like virtio-blk-pci's iothread-vq-mapping (an
array of objects) are silently dropped by qemu_opts_from_qdict() during
the QemuOpts conversion even though QAPI is capable of validating them.
As a result, hotplugging virtio-blk-pci devices with the
iothread-vq-mapping property does not work as expected (the property is
ignored).
Get rid of the QemuOpts conversion in qmp_device_add() and call
qdev_device_add_from_qdict() with from_json=true. Using the QMP
command's QDict arguments directly allows non-scalar properties.
The HMP is also adjusted since qmp_device_add()'s now expects properly
typed JSON arguments and cannot be used from HMP anymore. Move the code
that was previously in qmp_device_add() (with QemuOpts conversion and
from_json=false) into hmp_device_add() so that its behavior is
unchanged.
This patch changes the behavior of QMP device_add but not HMP
device_add. QMP clients that sent incorrectly typed device_add QMP
commands no longer work. This is a breaking change but clients should be
using the correct types already. See the netdev_add QAPIfication in
commit db2a380c8457 for similar reasoning and object-add in commit
9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false
for the time being.
Markus helped me figure this out and even provided a draft patch. The
code ended up very close to what he suggested.
Suggested-by: Markus Armbruster
Cc: Daniel P. Berrangé
Signed-off-by: Stefan Hajnoczi
Message-ID: <20240827192751.948633-2-stefa...@redhat.com>
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
system/qdev-monitor.c | 42 --
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 4c09b38ffb..03ae610649 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -856,18 +856,9 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
{
-QemuOpts *opts;
DeviceState *dev;
-opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
-if (!opts) {
-return;
-}
-if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
-qemu_opts_del(opts);
-return;
-}
-dev = qdev_device_add(opts, errp);
+dev = qdev_device_add_from_qdict(qdict, true, errp);
if (!dev) {
/*
* Drain all pending RCU callbacks. This is done because
@@ -879,9 +870,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp)
* to the user
*/
drain_call_rcu();
-
-qemu_opts_del(opts);
-return;
}
object_unref(OBJECT(dev));
}
@@ -1018,8 +1006,34 @@ void qmp_device_sync_config(const char *id, Error **errp)
void hmp_device_add(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
+QemuOpts *opts;
+DeviceState *dev;
-qmp_device_add((QDict *)qdict, NULL, &err);
+opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &err);
+if (!opts) {
+goto out;
+}
+if (qdev_device_help(opts)) {
+qemu_opts_del(opts);
+return;
+}
+dev = qdev_device_add(opts, &err);
+if (!dev) {
+/*
+ * Drain all pending RCU callbacks. This is done because
+ * some bus related operations can delay a device removal
+ * (in this case this can happen if device is added and then
+ * removed due to a configuration error)
+ * to a RCU callback, but user might expect that this interface
+ * will finish its job completely once qmp command returns result
+ * to the user
+ */
+drain_call_rcu();
+
+qemu_opts_del(opts);
+}
+object_unref(dev);
+out:
hmp_handle_error(mon, err);
}
--
2.47.0
[PULL 0/8] Block layer patches
The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432: Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +) are available in the Git repository at: https://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934: vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100) Block layer patches - Fix qmp_device_add() to not throw non-scalar options away (fixes iothread-vq-mapping being silently ignored in device_add) - iotests: Fix mypy failure - parallels: Avoid potential integer overflow - Fix crash in migration_is_running() Dmitry Frolov (1): parallels: fix possible int overflow John Snow (4): iotests: reflow ReproducibleTestRunner arguments iotests: correct resultclass type in ReproducibleTestRunner python: disable too-many-positional-arguments warning python: silence pylint raising-non-exception error Peter Xu (1): migration: Check current_migration in migration_is_running() Stefan Hajnoczi (2): qdev-monitor: avoid QemuOpts in QMP device_add vl: use qmp_device_add() in qemu_create_cli_devices() block/parallels.c | 4 ++-- migration/migration.c | 4 system/qdev-monitor.c | 42 -- system/vl.c | 14 -- python/scripts/mkvenv.py | 3 +++ tests/qemu-iotests/iotests.py | 11 +++ python/setup.cfg | 1 + tests/qemu-iotests/pylintrc | 1 + 8 files changed, 50 insertions(+), 30 deletions(-)
[PULL 3/8] iotests: reflow ReproducibleTestRunner arguments
From: John Snow Trivial reflow to let the type names breathe. (I need to add a longer type name.) Signed-off-by: John Snow Message-ID: <20241101173700.965776-2-js...@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/iotests.py | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index ea48af4a7b..673bbcd356 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -1614,10 +1614,13 @@ def write(self, arg=None): self.stream.write(arg) class ReproducibleTestRunner(unittest.TextTestRunner): -def __init__(self, stream: Optional[TextIO] = None, - resultclass: Type[unittest.TestResult] = - ReproducibleTestResult, - **kwargs: Any) -> None: +def __init__( +self, +stream: Optional[TextIO] = None, +resultclass: Type[unittest.TestResult] = +ReproducibleTestResult, +**kwargs: Any +) -> None: rstream = ReproducibleStreamWrapper(stream or sys.stdout) super().__init__(stream=rstream, # type: ignore descriptions=True, -- 2.47.0
[PULL 5/8] python: disable too-many-positional-arguments warning
From: John Snow Newest versions of pylint complain about specifically positional arguments in addition to too many in general. We already disable the general case, so silence this new warning too. Signed-off-by: John Snow Message-ID: <20241101173700.965776-4-js...@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- python/setup.cfg| 1 + tests/qemu-iotests/pylintrc | 1 + 2 files changed, 2 insertions(+) diff --git a/python/setup.cfg b/python/setup.cfg index 3b4e2cc550..cf5af7e664 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -142,6 +142,7 @@ ignore_missing_imports = True disable=consider-using-f-string, consider-using-with, too-many-arguments, +too-many-positional-arguments, too-many-function-args, # mypy handles this with less false positives. too-many-instance-attributes, no-member, # mypy also handles this better. diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc index 05b75ee59b..c5f4833e45 100644 --- a/tests/qemu-iotests/pylintrc +++ b/tests/qemu-iotests/pylintrc @@ -13,6 +13,7 @@ disable=invalid-name, no-else-return, too-few-public-methods, too-many-arguments, +too-many-positional-arguments, too-many-branches, too-many-lines, too-many-locals, -- 2.47.0
[PULL 8/8] vl: use qmp_device_add() in qemu_create_cli_devices()
From: Stefan Hajnoczi
qemu_create_cli_devices() should use qmp_device_add() to match the
behavior of the QMP monitor. A comment explained that libvirt changes
implementing strict CLI syntax were needed.
Peter Krempa has confirmed that modern libvirt uses
the same JSON for -device (CLI) and device_add (QMP). Go ahead and use
qmp_device_add().
Cc: Peter Krempa
Reviewed-by: Markus Armbruster
Signed-off-by: Stefan Hajnoczi
Message-ID: <20240827192751.948633-3-stefa...@redhat.com>
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
system/vl.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index d217b3d64d..e3f7d3a156 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2653,17 +2653,11 @@ static void qemu_create_cli_devices(void)
qemu_opts_foreach(qemu_find_opts("device"),
device_init_func, NULL, &error_fatal);
QTAILQ_FOREACH(opt, &device_opts, next) {
-DeviceState *dev;
+QObject *ret_data = NULL;
+
loc_push_restore(&opt->loc);
-/*
- * TODO Eventually we should call qmp_device_add() here to make sure it
- * behaves the same, but QMP still has to accept incorrectly typed
- * options until libvirt is fixed and we want to be strict on the CLI
- * from the start, so call qdev_device_add_from_qdict() directly for
- * now.
- */
-dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
-object_unref(OBJECT(dev));
+qmp_device_add(opt->opts, &ret_data, &error_fatal);
+assert(ret_data == NULL); /* error_fatal aborts */
loc_pop(&opt->loc);
}
rom_reset_order_override();
--
2.47.0
Re: [PATCH ssh] ssh: Do not switch session to non-blocking mode
Am 14.11.2024 um 17:49 hat Jakub Jelen geschrieben: > Hello, > comments inline below. > > On Thu, Nov 14, 2024 at 4:21 PM Kevin Wolf wrote: > > [...] > > > > > I'll just note that I'm only forwarding on the patch from Jakub. > > > I didn't write it. > > > > > > I did lightly test it, and it seems to work. However it seems also > > > likely that it is causing qemu to block internally. Probably not > > > noticable for light use, but not something that we'd want for serious > > > use. However if libssh doesn't support non-blocking SFTP there's not > > > much we can do about that in qemu. > > > > ...just making it blocking is not acceptable. It will potentially make > > the guest hang while we're waiting for sftp responses. > > This is the limitation of the SFTP API we have (and a reason why we > have a new API below, but only in new 0.11.0 release so not solution > for older systems that wont get new libssh). > > > I see that there is an sftp_aio_*() API, but it looks weird. Instead of > > allowing you to just poll the next request that is ready, you have to > > call a (blocking) wait on a specific request. > > Its more "streaming" API allowing the request and response overlap in time > allowing better throughput on networks with large latency. > > To support fully non blocking API in SFTP, there is still way to go, but this > api should be more ready for that than the old one. Ok, so something to possibly look into later, but not for the time being. > > co_yield(), which is currently used when sftp_read() returns SSH_AGAIN, > > makes sure that we poll the socket fd, so we can know that _something_ > > new has arrived. However it's unclear to me how to know _which_ request > > received a reply and can be completed now. It seems you have to call > > sftp_aio_wait_*() in non-blocking mode on all requests to do that, which > > probably is affected by the libssh bug, too. > > Are you sure that with the old libssh versions you were getting SSH_AGAIN > in non-blocking mode? Michael in the following comment found the > change where the issue started to demonstrate: > > https://gitlab.com/libssh/libssh-mirror/-/issues/280#note_2204139954 > https://gitlab.com/libssh/libssh-mirror/-/commit/2d3b7e07af3675b9a0326bc5c6253a0bbbda567b > > And from what I read, it was just silently behaving as blocking > (potentially infinitely) instead of returning SSH_AGAIN deep in libssh > code. Hm, after looking some more at the code, I agree that it can't have worked, for the simple reason that sftp_read() never returns SSH_AGAIN, but turns it into 0. Which QEMU would have reported as an I/O error if we're not at EOF. What I don't understand yet where in the code it would have blocked before rather than returning an error. I tried to follow the code path and didn't see anything like it, but obviously I'm also not familiar with libssh code. I guess it also doesn't really matter as long as we know it has always been broken... The thing that maybe misled me is that sftp_recv_response_msg() calls ssh_channel_poll() first to make sure that there is even something to read. So I expected it should have been non-blocking at least in some cases, but if it had been, we would probably have seen I/O errors all the time? > > So I'm not sure if sftp_aio_*() can be combined with something else into > > a working solution, and I also don't know if it's affected by the same > > libssh bug. > > Right now, we do not have a full solution. But having SFTP working > completely in nonoblocking mode is one of the things we would like to have > in the long term. > > > Jakub, can you help with that? > > > > [...] > > > > As far as I can see, libssh sessions aren't thread safe, so we'll have > > to make sure to have only one request going at the same time, but I > > assume that calling ssh_read/write() from different threads sequentially > > isn't a problem? > > My understanding is that the thread safety of libssh is limited to not > sharing session between threads -- there is no synchronization if two > threads would send packets at the same time: > > https://api.libssh.org/master/ > > If you will make sure you will not call sftp_read()/sftp_write() at > the same time from different threads, it might work, but it is > untested. How do you feel about it? Do you think this is something libssh can support, or is it something that might accidentally work today, but not necessarily next year? We have a thread pool readily available that we could use, but then requests for the same session would come from different threads - just never at the same time. If we need a single long-lived thread per session instead, that might be a little more involved because we might have to implement all the communication and synchronisation from scratch. (Hmm... Or we abuse the IOThread object to create one internally and just move the request coroutine to it around libssh calls. That could be easy enough.) Kevin
Re: [PATCH ssh] ssh: Do not switch session to non-blocking mode
Hello, comments inline below. On Thu, Nov 14, 2024 at 4:21 PM Kevin Wolf wrote: > [...] > > > I'll just note that I'm only forwarding on the patch from Jakub. > > I didn't write it. > > > > I did lightly test it, and it seems to work. However it seems also > > likely that it is causing qemu to block internally. Probably not > > noticable for light use, but not something that we'd want for serious > > use. However if libssh doesn't support non-blocking SFTP there's not > > much we can do about that in qemu. > > ...just making it blocking is not acceptable. It will potentially make > the guest hang while we're waiting for sftp responses. This is the limitation of the SFTP API we have (and a reason why we have a new API below, but only in new 0.11.0 release so not solution for older systems that wont get new libssh). > I see that there is an sftp_aio_*() API, but it looks weird. Instead of > allowing you to just poll the next request that is ready, you have to > call a (blocking) wait on a specific request. Its more "streaming" API allowing the request and response overlap in time allowing better throughput on networks with large latency. To support fully non blocking API in SFTP, there is still way to go, but this api should be more ready for that than the old one. > co_yield(), which is currently used when sftp_read() returns SSH_AGAIN, > makes sure that we poll the socket fd, so we can know that _something_ > new has arrived. However it's unclear to me how to know _which_ request > received a reply and can be completed now. It seems you have to call > sftp_aio_wait_*() in non-blocking mode on all requests to do that, which > probably is affected by the libssh bug, too. Are you sure that with the old libssh versions you were getting SSH_AGAIN in non-blocking mode? Michael in the following comment found the change where the issue started to demonstrate: https://gitlab.com/libssh/libssh-mirror/-/issues/280#note_2204139954 https://gitlab.com/libssh/libssh-mirror/-/commit/2d3b7e07af3675b9a0326bc5c6253a0bbbda567b And from what I read, it was just silently behaving as blocking (potentially infinitely) instead of returning SSH_AGAIN deep in libssh code. > So I'm not sure if sftp_aio_*() can be combined with something else into > a working solution, and I also don't know if it's affected by the same > libssh bug. Right now, we do not have a full solution. But having SFTP working completely in nonoblocking mode is one of the things we would like to have in the long term. > Jakub, can you help with that? > > [...] > > As far as I can see, libssh sessions aren't thread safe, so we'll have > to make sure to have only one request going at the same time, but I > assume that calling ssh_read/write() from different threads sequentially > isn't a problem? My understanding is that the thread safety of libssh is limited to not sharing session between threads -- there is no synchronization if two threads would send packets at the same time: https://api.libssh.org/master/ If you will make sure you will not call sftp_read()/sftp_write() at the same time from different threads, it might work, but it is untested. (joined the ML so I hope this mail will make it there) Jakub
Re: [PATCH v10 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Thu, 14 Nov 2024 at 14:38, BALATON Zoltan wrote: > On Thu, 14 Nov 2024, Phil Dennis-Jordan wrote: > > On Wed, 13 Nov 2024 at 19:16, BALATON Zoltan wrote: > >> On Wed, 13 Nov 2024, Phil Dennis-Jordan wrote: > >>> macOS's Cocoa event handling must be done on the initial (main) thread > >>> of the process. Furthermore, if library or application code uses > >>> libdispatch, the main dispatch queue must be handling events on the > main > >>> thread as well. > >>> > >>> So far, this has affected Qemu in both the Cocoa and SDL UIs, although > >>> in different ways: the Cocoa UI replaces the default qemu_main function > >>> with one that spins Qemu's internal main event loop off onto a > >>> background thread. SDL (which uses Cocoa internally) on the other hand > >>> uses a polling approach within Qemu's main event loop. Events are > >>> polled during the SDL UI's dpy_refresh callback, which happens to run > >>> on the main thread by default. > >>> > >>> As UIs are mutually exclusive, this works OK as long as nothing else > >>> needs platform-native event handling. In the next patch, a new device > is > >>> introduced based on the ParavirtualizedGraphics.framework in macOS. > >>> This uses libdispatch internally, and only works when events are being > >>> handled on the main runloop. With the current system, it works when > >>> using either the Cocoa or the SDL UI. However, it does not when running > >>> headless. Moreover, any attempt to install a similar scheme to the > >>> Cocoa UI's main thread replacement fails when combined with the SDL > >>> UI. > >>> > >>> This change tidies up main thread management to be more flexible. > >>> > >>> * The qemu_main global function pointer is a custom function for the > >>> main thread, and it may now be NULL. When it is, the main thread > >>> runs the main Qemu loop. This represents the traditional setup. > >>> * When non-null, spawning the main Qemu event loop on a separate > >>> thread is now done centrally rather than inside the Cocoa UI code. > >>> * For most platforms, qemu_main is indeed NULL by default, but on > >>> Darwin, it defaults to a function that runs the CFRunLoop. > >>> * The Cocoa UI sets qemu_main to a function which runs the > >>> NSApplication event handling runloop, as is usual for a Cocoa app. > >>> * The SDL UI overrides the qemu_main function to NULL, thus > >>> specifying that Qemu's main loop must run on the main > >>> thread. > >>> * The GTK UI also overrides the qemu_main function to NULL. > >>> * For other UIs, or in the absence of UIs, the platform's default > >>> behaviour is followed. > >>> > >>> This means that on macOS, the platform's runloop events are always > >>> handled, regardless of chosen UI. The new PV graphics device will > >>> thus work in all configurations. There is no functional change on other > >>> operating systems. > >>> > >>> Signed-off-by: Phil Dennis-Jordan > >>> Reviewed-by: Akihiko Odaki > >>> --- > >>> > >>> v5: > >>> > >>> * Simplified the way of setting/clearing the main loop by going back > >>> to setting qemu_main directly, but narrowing the scope of what it > >>> needs to do, and it can now be NULL. > >>> > >>> v6: > >>> > >>> * Folded function qemu_run_default_main_on_new_thread's code into > >>> main() > >>> * Removed whitespace changes left over on lines near code removed > >>> between v4 and v5 > >>> > >>> v9: > >>> > >>> * Set qemu_main to NULL for GTK UI as well. > >>> > >>> v10: > >>> > >>> * Added comments clarifying the functionality and purpose of qemu_main. > >>> > >>> include/qemu-main.h | 21 ++-- > >>> include/qemu/typedefs.h | 1 + > >>> system/main.c | 50 ++ > >>> ui/cocoa.m | 54 ++--- > >>> ui/gtk.c| 8 ++ > >>> ui/sdl2.c | 4 +++ > >>> 6 files changed, 90 insertions(+), 48 deletions(-) > >>> > >>> diff --git a/include/qemu-main.h b/include/qemu-main.h > >>> index 940960a7dbc..fc70681c8b5 100644 > >>> --- a/include/qemu-main.h > >>> +++ b/include/qemu-main.h > >>> @@ -5,7 +5,24 @@ > >>> #ifndef QEMU_MAIN_H > >>> #define QEMU_MAIN_H > >>> > >>> -int qemu_default_main(void); > >>> -extern int (*qemu_main)(void); > >>> +/* > >>> + * The function to run on the main (initial) thread of the process. > >>> + * NULL means QEMU's main event loop. > >>> + * When non-NULL, QEMU's main event loop will run on a purposely > created > >>> + * thread, after which the provided function pointer will be invoked > on > >>> + * the initial thread. > >>> + * This is useful on platforms which treat the main thread as special > >>> + * (macOS/Darwin) and/or require all UI API calls to occur from a > >>> + * specific thread. > >>> + * Implementing this via a global function pointer variable is a bit > >>> + * ugly, but it's probably worth investigating the existing UI thread > >> rule > >>> + * violations in the SDL (e.g. #2537) and GTK+ back-end
Re: [PATCH v10 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Wed, 13 Nov 2024 at 17:38, Paolo Bonzini wrote:
> On 11/13/24 15:23, Phil Dennis-Jordan wrote:
> > macOS's Cocoa event handling must be done on the initial (main) thread
> > of the process. Furthermore, if library or application code uses
> > libdispatch, the main dispatch queue must be handling events on the main
> > thread as well.
> >
> > So far, this has affected Qemu in both the Cocoa and SDL UIs, although
> > in different ways: the Cocoa UI replaces the default qemu_main function
> > with one that spins Qemu's internal main event loop off onto a
> > background thread. SDL (which uses Cocoa internally) on the other hand
> > uses a polling approach within Qemu's main event loop. Events are
> > polled during the SDL UI's dpy_refresh callback, which happens to run
> > on the main thread by default.
> >
> > As UIs are mutually exclusive, this works OK as long as nothing else
> > needs platform-native event handling. In the next patch, a new device is
> > introduced based on the ParavirtualizedGraphics.framework in macOS.
> > This uses libdispatch internally, and only works when events are being
> > handled on the main runloop. With the current system, it works when
> > using either the Cocoa or the SDL UI. However, it does not when running
> > headless. Moreover, any attempt to install a similar scheme to the
> > Cocoa UI's main thread replacement fails when combined with the SDL
> > UI.
> >
> > This change tidies up main thread management to be more flexible.
> >
> > * The qemu_main global function pointer is a custom function for the
> > main thread, and it may now be NULL. When it is, the main thread
> > runs the main Qemu loop. This represents the traditional setup.
> > * When non-null, spawning the main Qemu event loop on a separate
> > thread is now done centrally rather than inside the Cocoa UI code.
> > * For most platforms, qemu_main is indeed NULL by default, but on
> > Darwin, it defaults to a function that runs the CFRunLoop.
> > * The Cocoa UI sets qemu_main to a function which runs the
> > NSApplication event handling runloop, as is usual for a Cocoa app.
> > * The SDL UI overrides the qemu_main function to NULL, thus
> > specifying that Qemu's main loop must run on the main
> > thread.
> > * The GTK UI also overrides the qemu_main function to NULL.
> > * For other UIs, or in the absence of UIs, the platform's default
> > behaviour is followed.
> >
> > This means that on macOS, the platform's runloop events are always
> > handled, regardless of chosen UI. The new PV graphics device will
> > thus work in all configurations. There is no functional change on other
> > operating systems.
> >
> > Signed-off-by: Phil Dennis-Jordan
> > Reviewed-by: Akihiko Odaki
>
> I checked what GTK+ does and, either way, you have to create another
> thread: timers are handled with a CFRunLoopTimer, but file descriptors
> are polled in a separate thread and sent to the main thread with a
> single CFRunLoopSource. It's a bit nicer that the main thread is in
> charge, but it's more complex and probably slower too.
>
Just to clarify: is this supposed to be happening inside the GTK+ library
itself? i.e. GTK should spawn its own thread to poll file descriptors that
are owned by GTK? (As opposed to the file descriptors used by QEMU's own
event loop - what on Linux are eventfds, but on macOS I think are just
pipes*.)
This doesn't describe what I'm seeing when I run with -display gtk on
macOS. There's no extra thread created. There's a dock icon, but it's
non-interactive ("Application not responding"), there aren't any menus, and
there's no window. QEMU's own simulation is running in the background - I
can reach a guest via the network. So I guess there's some function in GTK
we're supposed to be calling that will make it crank the native event loop
on macOS, but this isn't being done?
Or do you mean there exists a global "block here forever" function in GTK
we can call from the main thread and which will make everything spring into
life, but that brings with it the same requirement as with the Cocoa UI:
moving everything that was originally on the main thread onto a background
thread. (I've done some searching for GTK docs and other background info
but anything I've found has been rather thin on this topic. The ui/gtk*.c
source is also not particularly enlightening - it seems to imply that
something in the background ought to be handling events somewhere.)
[* The event loop in QEMU on macOS probably ought to use a kqueue, with
EVFILT_USER where eventfds are used on Linux, except the rough equivalent
of ioeventfds are Mach ports which can be handled via EVFILT_MACHPORT, but
I digress.]
As long as it's clear that any handlers that go through the CFRunLoop
> run outside the BQL, as is already the case for the Cocoa UI, I see no
> problem with this approach.
>
I'm not entirely sure what you're getting at here, to be honest. The UI
thread can definitely not assume t
[PATCH v3 0/3] Introduce a new Write Protected pin inverted property
change from v1: 1. Support RTC for AST2700. 2. Support SDHCI write protected pin inverted for AST2500 and AST2600. 3. Introduce Capabilities Register 2 for SD slot 0 and 1. 4. Support create flash devices via command line for AST1030. change from v2: replace wp-invert with wp-inverted and fix review issues. change from v3: 1. add reviewer suggestion about wp_inverted comment 2. AST2500 EVB does not need to set wp-inverted property of sdhci model https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm/boot/dts/aspeed/aspeed-ast2500-evb.dts#L110 Jamin Lin (3): hw/sd/sdhci: Fix coding style hw/sd/sdhci: Introduce a new Write Protected pin inverted property hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB hw/arm/aspeed.c | 7 + hw/sd/sdhci.c | 70 - include/hw/arm/aspeed.h | 1 + include/hw/sd/sdhci.h | 5 +++ 4 files changed, 61 insertions(+), 22 deletions(-) -- 2.34.1
[PATCH v3 1/3] hw/sd/sdhci: Fix coding style
Fix coding style issues from checkpatch.pl
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
---
hw/sd/sdhci.c | 64 +--
1 file changed, 42 insertions(+), 22 deletions(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index dbe5c2340c..37875c02c3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -233,7 +233,7 @@ static void sdhci_raise_insertion_irq(void *opaque)
if (s->norintsts & SDHC_NIS_REMOVE) {
timer_mod(s->insert_timer,
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
SDHC_INSERTION_DELAY);
+qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + SDHC_INSERTION_DELAY);
} else {
s->prnsts = 0x1ff;
if (s->norintstsen & SDHC_NISEN_INSERT) {
@@ -251,7 +251,7 @@ static void sdhci_set_inserted(DeviceState *dev, bool level)
if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
/* Give target some time to notice card ejection */
timer_mod(s->insert_timer,
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
SDHC_INSERTION_DELAY);
+qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + SDHC_INSERTION_DELAY);
} else {
if (level) {
s->prnsts = 0x1ff;
@@ -289,9 +289,11 @@ static void sdhci_reset(SDHCIState *s)
timer_del(s->insert_timer);
timer_del(s->transfer_timer);
-/* Set all registers to 0. Capabilities/Version registers are not cleared
+/*
+ * Set all registers to 0. Capabilities/Version registers are not cleared
* and assumed to always preserve their value, given to them during
- * initialization */
+ * initialization
+ */
memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg -
(uintptr_t)&s->sdmasysad);
/* Reset other state based on current card insertion/readonly status */
@@ -305,7 +307,8 @@ static void sdhci_reset(SDHCIState *s)
static void sdhci_poweron_reset(DeviceState *dev)
{
-/* QOM (ie power-on) reset. This is identical to reset
+/*
+ * QOM (ie power-on) reset. This is identical to reset
* commanded via device register apart from handling of the
* 'pending insert on powerup' quirk.
*/
@@ -445,8 +448,10 @@ static void sdhci_read_block_from_card(SDHCIState *s)
s->prnsts &= ~SDHC_DAT_LINE_ACTIVE;
}
-/* If stop at block gap request was set and it's not the last block of
- * data - generate Block Event interrupt */
+/*
+ * If stop at block gap request was set and it's not the last block of
+ * data - generate Block Event interrupt
+ */
if (s->stopped_state == sdhc_gap_read && (s->trnmod & SDHC_TRNS_MULTI) &&
s->blkcnt != 1){
s->prnsts &= ~SDHC_DAT_LINE_ACTIVE;
@@ -548,8 +553,10 @@ static void sdhci_write_block_to_card(SDHCIState *s)
sdhci_update_irq(s);
}
-/* Write @size bytes of @value data to host controller @s Buffer Data Port
- * register */
+/*
+ * Write @size bytes of @value data to host controller @s Buffer Data Port
+ * register
+ */
static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
{
unsigned i;
@@ -594,9 +601,11 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
return;
}
-/* XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
+/*
+ * XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
* possible stop at page boundary if initial address is not page aligned,
- * allow them to work properly */
+ * allow them to work properly
+ */
if ((s->sdmasysad % boundary_chk) == 0) {
page_aligned = true;
}
@@ -702,7 +711,8 @@ static void get_adma_description(SDHCIState *s, ADMADescr
*dscr)
dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
MEMTXATTRS_UNSPECIFIED);
adma2 = le64_to_cpu(adma2);
-/* The spec does not specify endianness of descriptor table.
+/*
+ * The spec does not specify endianness of descriptor table.
* We currently assume that it is LE.
*/
dscr->addr = (hwaddr)extract64(adma2, 32, 32) & ~0x3ull;
@@ -977,8 +987,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
return true;
}
-/* The Buffer Data Port register must be accessed in sequential and
- * continuous manner */
+/*
+ * The Buffer Data Port register must be accessed in sequential and
+ * continuous manner
+ */
static inline bool
sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
{
@@ -1206,8 +1218,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val,
unsigned size)
MASKED_WRITE(s->argument, mask, value);
break;
case SDHC_TRNMOD:
-/* DMA can be enabled only if it is supported as indicated by
- * capabilities register */
+/*
+ * DMA can be enabled only if it is supported as indicated by
+ * capabilities register
+ */
if (!(s->capareg &
[PATCH v3 2/3] hw/sd/sdhci: Introduce a new Write Protected pin inverted property
The Write Protect pin of SDHCI model is default active low to match the SDHCI
spec. So, write enable the bit 19 should be 1 and write protected the bit 19
should be 0 at the Present State Register (0x24). However, some boards are
design Write Protected pin active high. In other words, write enable the bit 19
should be 0 and write protected the bit 19 should be 1 at the
Present State Register (0x24). To support it, introduces a new "wp-inverted"
property and set it false by default.
Signed-off-by: Jamin Lin
Acked-by: Cédric Le Goater
---
hw/sd/sdhci.c | 6 ++
include/hw/sd/sdhci.h | 5 +
2 files changed, 11 insertions(+)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 37875c02c3..06d1e24086 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -274,6 +274,10 @@ static void sdhci_set_readonly(DeviceState *dev, bool
level)
{
SDHCIState *s = (SDHCIState *)dev;
+if (s->wp_inverted) {
+level = !level;
+}
+
if (level) {
s->prnsts &= ~SDHC_WRITE_PROTECT;
} else {
@@ -1550,6 +1554,8 @@ static Property sdhci_sysbus_properties[] = {
false),
DEFINE_PROP_LINK("dma", SDHCIState,
dma_mr, TYPE_MEMORY_REGION, MemoryRegion *),
+DEFINE_PROP_BOOL("wp-inverted", SDHCIState,
+ wp_inverted, false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 6cd2822f1d..38c08e2859 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -100,6 +100,11 @@ struct SDHCIState {
uint8_t sd_spec_version;
uint8_t uhs_mode;
uint8_t vendor;/* For vendor specific functionality */
+/*
+ * Write Protect pin default active low for detecting SD card
+ * to be protected. Set wp_inverted to invert the signal.
+ */
+bool wp_inverted;
};
typedef struct SDHCIState SDHCIState;
--
2.34.1
[PATCH v3 3/3] hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB
The Write Protect pin of SDHCI model is default active low to match the SDHCI
spec. So, write enable the bit 19 should be 1 and write protected the bit 19
should be 0 at the Present State Register (0x24).
According to the design of AST2600 EVB, the Write Protected pin is active
high by default. To support it, introduces a new "sdhci_wp_inverted"
property in ASPEED MACHINE State and set it true for AST2600 EVB
and set "wp_inverted" property true of sdhci-generic model.
Signed-off-by: Jamin Lin
Reviewed-by: Andrew Jeffery
---
hw/arm/aspeed.c | 7 +++
include/hw/arm/aspeed.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6ca145362c..a58b7160cd 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -413,6 +413,12 @@ static void aspeed_machine_init(MachineState *machine)
OBJECT(get_system_memory()), &error_abort);
object_property_set_link(OBJECT(bmc->soc), "dram",
OBJECT(machine->ram), &error_abort);
+if (amc->sdhci_wp_inverted) {
+for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
+object_property_set_bool(OBJECT(&bmc->soc->sdhci.slots[i]),
+ "wp-inverted", true, &error_abort);
+}
+}
if (machine->kernel_filename) {
/*
* When booting with a -kernel command line there is no u-boot
@@ -1419,6 +1425,7 @@ static void
aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
amc->num_cs= 1;
amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON | ASPEED_MAC2_ON |
ASPEED_MAC3_ON;
+amc->sdhci_wp_inverted = true;
amc->i2c_init = ast2600_evb_i2c_init;
mc->default_ram_size = 1 * GiB;
aspeed_machine_class_init_cpus_defaults(mc);
diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index cbeacb214c..9cae45a1c9 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -39,6 +39,7 @@ struct AspeedMachineClass {
uint32_t macs_mask;
void (*i2c_init)(AspeedMachineState *bmc);
uint32_t uart_default;
+bool sdhci_wp_inverted;
};
--
2.34.1
RE: [PATCH v2 2/3] hw/sd/sdhci: Introduce a new Write Protected pin inverted property
Hi Cedric,
>
> On 11/4/24 04:21, Jamin Lin wrote:
> > The Write Protect pin of SDHCI model is default active low to match
> > the SDHCI spec. So, write enable the bit 19 should be 1 and write
> > protected the bit 19 should be 0 at the Present State Register (0x24).
> > However, some boards are design Write Protected pin active high. In
> > other words, write enable the bit 19 should be 0 and write protected
> > the bit 19 should be 1 at the Present State Register (0x24). To support it,
> introduces a new "wp-inverted"
> > property and set it false by default.
> >
> > Signed-off-by: Jamin Lin
>
> Acked-by: Cédric Le Goater
>
> > ---
> > hw/sd/sdhci.c | 6 ++
> > include/hw/sd/sdhci.h | 5 +
> > 2 files changed, 11 insertions(+)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index
> > db7d547156..c675543873 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -275,6 +275,10 @@ static void sdhci_set_readonly(DeviceState *dev,
> bool level)
> > {
> > SDHCIState *s = (SDHCIState *)dev;
> >
> > +if (s->wp_inverted) {
> > +level = !level;
> > +}
> > +
> > if (level) {
> > s->prnsts &= ~SDHC_WRITE_PROTECT;
> > } else {
> > @@ -1551,6 +1555,8 @@ static Property sdhci_sysbus_properties[] = {
> >false),
> > DEFINE_PROP_LINK("dma", SDHCIState,
> >dma_mr, TYPE_MEMORY_REGION,
> MemoryRegion *),
> > +DEFINE_PROP_BOOL("wp-inverted", SDHCIState,
> > + wp_inverted, false),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h index
> > 6cd2822f1d..25ad9ed778 100644
> > --- a/include/hw/sd/sdhci.h
> > +++ b/include/hw/sd/sdhci.h
> > @@ -100,6 +100,11 @@ struct SDHCIState {
> > uint8_t sd_spec_version;
> > uint8_t uhs_mode;
> > uint8_t vendor;/* For vendor specific functionality */
> > +/*
> > + * Write Protect pin default active low for detecting SD card
> > + * to be protected. Set wp_inverted to true inverted the signal.
>
> In case you respin, may be you could change the last sentence to :
> "Set wp_inverted to invert the signal."
>
>
Thanks for your review.
Will update it.
Jamin
> Thanks,
>
> C.
>
>
> > + */
> > +bool wp_inverted;
> > };
> > typedef struct SDHCIState SDHCIState;
> >
RE: [PATCH v2 00/18] Fix write incorrect data into flash in user mode
Hi Cedric, > Subject: Re: [PATCH v2 00/18] Fix write incorrect data into flash in user mode > > Hello Jamin, > > On 11/14/24 06:30, Jamin Lin wrote: > > Hi Cedric, > > > >> Subject: RE: [PATCH v2 00/18] Fix write incorrect data into flash in > >> user mode > >> > >> Hi Cedric, > >> > >>> Subject: Re: [PATCH v2 00/18] Fix write incorrect data into flash in > >>> user mode > >>> > >>> Hello Jamin, > >>> > >>> On 10/22/24 11:40, Jamin Lin wrote: > change from v1: > 1. Fix write incorrect data into flash in user mode. > 2. Refactor aspeed smc qtest testcases to support AST2600, > AST2500 and AST1030. > 3. Add ast2700 smc qtest testcase to support AST2700. > > change from v2: > 1. Introduce a new aspeed-smc-utils.c to place common testcases. > 2. Fix hardcode attach flash model of spi controllers 3. Add > reviewers suggestion and fix review issue. > >>> I have applied 1-6,8 to aspeed-next and should send a PR with them. > >>> I kept the test extensions for later, to take a closer a look and > >>> also because I will be on PTO next week. Tests can be merged in the > >>> next PR if we have time in this cycle or in QEMU 10.0. > >>> > >> Got it and thanks for help. > >> Jamin > >>> Thanks, > > > > Could you please help to review patch 17 and 18 ? > > Do I need to re-send patch from 9 to 18 of this patch series? > > Not yet. I have some comments to send but I am busy on another topic. > Got it. Thanks for your kindly support. Jamin > We have some time before QEMU 10.0. They are in my aspeed-9.2 branch, so > that I don't forget about them. > > Thanks, > > C. >
RE: [PATCH v2 0/3] Introduce a new Write Protected pin inverted property
Hi Cedric, > > Hello Jamin, > > On 11/14/24 06:32, Jamin Lin wrote: > > Hi Cedric, Andrew > > > >> Subject: [PATCH v2 0/3] Introduce a new Write Protected pin inverted > >> property > >> > >> change from v1: > >> 1. Support RTC for AST2700. > >> 2. Support SDHCI write protected pin inverted for AST2500 and AST2600. > >> 3. Introduce Capabilities Register 2 for SD slot 0 and 1. > >> 4. Support create flash devices via command line for AST1030. > >> > >> change from v2: > >> replace wp-invert with wp-inverted and fix review issues. > >> > >> Jamin Lin (3): > >>hw/sd/sdhci: Fix coding style > >>hw/sd/sdhci: Introduce a new Write Protected pin inverted property > >>hw/arm/aspeed: Invert sdhci write protected pin for AST2600 and > >> AST2500 EVBs > >> > >> hw/arm/aspeed.c | 8 + > >> hw/sd/sdhci.c | 70 > - > >> include/hw/arm/aspeed.h | 1 + > >> include/hw/sd/sdhci.h | 5 +++ > >> 4 files changed, 62 insertions(+), 22 deletions(-) > >> > > > > Could you please help to review this patch series? > > We would need an Ack from the sd maintainer on patch 2. Then, I can apply on > the aspeed queue. That's material for QEMU 10.0. > Got it. Thanks for your kindly support. Jamin > Thanks, > > C.
Re: "relaxed" `-hda file.qcow2` equivalent ?
Le jeu. 14 nov. 2024 à 15:11, lacsaP Patatetom a écrit : > hi, > what is the "relaxed" equivalent of the `-hda file.qcow2` command line > argument to control file locking (`locking=off`) ? > the documentation says that `-hda a` can be replaced by `-drive file=a` > and that locking can be controlled with the `locking` option of the > `blockdev` argument : how to link `drive` and `blockdev` to obtain > "relaxed" `-hda file.qcow2` ? > regards, lacsaP. > a few (corrected) details to clarify my request... `-hda` is a (very) old way of specifying the disk to be used, but is extremely practical when there's nothing special to define, and is still supported by qemu. my image `file.qcow2` is mounted elsewhere (nbd) and running `qemu-system-x86_64 -snapshot -hda file.qcow2` fails with the error "qemu-system-x86_64: -hda file.qcow2: Failed to get shared “write” lock / Is another process using the image [file.qcow2] ?". I'm doing some tests and I don't want to unmount my mounts, so I'm trying to force qemu to start without taking into account the fact that the image is not free and keeping the disk order (I'm also using `-hdb file2.qcow2` but this image doesn't cause any problems, is not mounted and is totally free). what should I use to replace `-hda file.qcow2` (drive/device/blockdev) to force qemu to boot from this first disk ? regards, lacsaP.
Re: "relaxed" `-hda file.qcow2` equivalent ?
Le jeu. 14 nov. 2024 à 16:15, lacsaP Patatetom a écrit : > Le jeu. 14 nov. 2024 à 15:11, lacsaP Patatetom a > écrit : > >> hi, >> what is the "relaxed" equivalent of the `-hda file.qcow2` command line >> argument to control file locking (`locking=off`) ? >> the documentation says that `-hda a` can be replaced by `-drive file=a` >> and that locking can be controlled with the `locking` option of the >> `blockdev` argument : how to link `drive` and `blockdev` to obtain >> "relaxed" `-hda file.qcow2` ? >> regards, lacsaP. >> > > a few (corrected) details to clarify my request... > > `-hda` is a (very) old way of specifying the disk to be used, but is > extremely practical when there's nothing special to define, and is still > supported by qemu. > > my image `file.qcow2` is mounted elsewhere (nbd) and running > `qemu-system-x86_64 -snapshot -hda file.qcow2` fails with the error > "qemu-system-x86_64: -hda file.qcow2: Failed to get shared “write” lock / > Is another process using the image [file.qcow2] ?". > > I'm doing some tests and I don't want to unmount my mounts, so I'm trying > to force qemu to start without taking into account the fact that the image > is not free and keeping the disk order (I'm also using `-hdb file2.qcow2` > but this image doesn't cause any problems, is not mounted and is totally > free). > > what should I use to replace `-hda file.qcow2` (drive/device/blockdev) to > force qemu to boot from this first disk ? > > regards, lacsaP. > > `qemu-system-x86_64 -global file.locking=off -snapshot -hda file.qcow2 -hdb file2.qcow2` and `qemu-system-x86_64 -global driver=file,property=locking,value=off -snapshot -hda file.qcow2 -hdb file2.qcow2` do not force qemu to start up...
"relaxed" `-hda file.qcow2` equivalent ?
hi, what is the "relaxed" equivalent of the `-hda file.qcow2` command line argument to control file locking (`locking=off`) ? the documentation says that `-hda a` can be replaced by `-drive file=a` and that locking can be controlled with the `locking` option of the `blockdev` argument : how to link `drive` and `blockdev` to obtain "relaxed" `-hda file.qcow2` ? regards, lacsaP.
Re: [PATCH v10 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Thu, 14 Nov 2024 at 14:27, Paolo Bonzini wrote:
> On Thu, Nov 14, 2024 at 11:32 AM Phil Dennis-Jordan
> wrote:
> >> I checked what GTK+ does and, either way, you have to create another
> >> thread: timers are handled with a CFRunLoopTimer, but file descriptors
> >> are polled in a separate thread and sent to the main thread with a
> >> single CFRunLoopSource. It's a bit nicer that the main thread is in
> >> charge, but it's more complex and probably slower too.
> >
> >
> > Just to clarify: is this supposed to be happening inside the GTK+
> library itself? i.e. GTK should spawn its own thread to poll file
> descriptors that are owned by GTK? (As opposed to the file descriptors used
> by QEMU's own event loop - what on Linux are eventfds, but on macOS I think
> are just pipes*.)
>
> It's what I saw in the GTK+ source code.
>
>
> https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gdk/macos/gdkmacoseventsource.c?ref_type=heads
>
> > This doesn't describe what I'm seeing when I run with -display gtk on
> macOS. There's no extra thread created. There's a dock icon, but it's
> non-interactive ("Application not responding"), there aren't any menus, and
> there's no window. QEMU's own simulation is running in the background - I
> can reach a guest via the network. So I guess there's some function in GTK
> we're supposed to be calling that will make it crank the native event loop
> on macOS, but this isn't being done?
>
> In theory it should work, with the event source added as soon as GTK+
> is started... aha!
>
> We do not use the GMainContext's poll_func, we use qemu_poll_ns.
> That's the missing part: GDK replaces the poll_func with one that
> calls nextEventMatchingMask:
>
>
> https://gitlab.gnome.org/GNOME/gtk/-/blame/main/gdk/macos/gdkmacoseventsource.c?ref_type=heads#L767
>
Thanks for explaining. That makes sense - I was looking in the Qemu source
for where exactly it was polling the GLib/GTK event handler, now I know why
I couldn't find it. Tracing the poll_func setting, it looks like GTK
expects g_main_context_iteration() to be called regularly, or the main
thread needs to call and block inside g_main_loop_run.
But in QEMU it's not as easy a fix as just going ahead and doing one of
those two things because QEMU semi-replicates the GLib poll function, and
we can't use both at the same time. Right?
> There could be more issues, but I think for now it's better to block
> the GTK+ UI under macOS.
>
OK, I've created a new issue on GitLab where any further decision making
and action can be tracked:
https://gitlab.com/qemu-project/qemu/-/issues/2676
I'm not sure this patchset is the best place for a patch blocking GTK on
macOS, especially if you want it in 9.2.
> [...]
>
> >> As long as it's clear that any handlers that go through the CFRunLoop
> >> run outside the BQL, as is already the case for the Cocoa UI, I see no
> >> problem with this approach.
> >
> > I'm not entirely sure what you're getting at here, to be honest. The UI
> thread can definitely not assume to be holding the BQL all the time; we'd
> have to treat it more like an AIOContext. It could pave the way towards
> putting the display and UI subsystems on their own AIOContext or
> AIOContext-like-thing, rather than hogging the BQL for expensive image
> operations.
>
> Don't worry, I was mostly talking to myself... The UI thread, and more
> specifically any handlers that are called from CFRunLoop instead of
> QEMU's main loop, will have to use mutexes or bql_lock()/bql_unlock(),
> like ui/cocoa.m already does.
>
> In other words, code that interacts with Apple's paravirtualized
> graphics needs to know if it runs from the CFRunLoop or from
> qemu_main().
>
We already have extremely careful threading code in patch 02/15 for safely
interfacing the threading models of the PV Graphics/libdispatch world and
QEMU's world of BQL, AIOs, BHs, and so on.
> > (*By the sound of it, Win32 has an all-UI-calls-on-one-thread
> requirement as well which we may be violating to some degree via the GTK
> and/or SDL backends as well; my adventures with Win32 are almost 20 years
> back now so I'm a bit out of the loop there.)
>
> Hmm, no I don't remember anything like that for Windows but it's also
> been many years for me.
>
Sorry, this was in reference to Daniel Berrangé's comment on this related
bug: https://gitlab.com/qemu-project/qemu/-/issues/2537#note_2203183775
(QEMU's SDL UI also falls afoul of macOS's threading requirements in some
specific cases.)
Re: "relaxed" `-hda file.qcow2` equivalent ?
Le jeu. 14 nov. 2024 à 15:11, lacsaP Patatetom a écrit : > hi, > what is the "relaxed" equivalent of the `-hda file.qcow2` command line > argument to control file locking (`locking=off`) ? > the documentation says that `-hda a` can be replaced by `-drive file=a` > and that locking can be controlled with the `locking` option of the > `blockdev` argument : how to link `drive` and `blockdev` to obtain > "relaxed" `-hda file.qcow2` ? > regards, lacsaP. > a few details to clarify my request... `-hda` is a (very) old way of specifying the disk to be used, but is extremely practical when there's nothing special to define, and is still supported by qemu. my image `fie.qcow2` is mounted elsewhere (nbd) and running `qemu-system-x86_64 -snapshot -hda file.qcow2` fails with the error "qemu-system-x86_64: -hda usb1.disk: Failed to get shared “write” lock / Is another process using the image [usb1.disk] ?". I'm doing some tests and I don't want to unmount my mounts, so I'm trying to force qemu to start without taking into account the fact that the image is not free and keeping the disk order (I'm also using `-hdb file2.qcow2` but this image doesn't cause any problems, is not mounted and is totally free). what should I use to replace `-hda file.qcow2` (drive/device/blockdev) to force qemu to boot from this first disk ? regards, lacsaP.
Re: [PATCH ssh] ssh: Do not switch session to non-blocking mode
Am 13.11.2024 um 14:00 hat Richard W.M. Jones geschrieben: > On Wed, Nov 13, 2024 at 03:02:59PM +0300, Michael Tokarev wrote: > > Heh. I was creating a qemu bug report on gitlab when this email arrived :) > > > > 13.11.2024 14:49, Richard W.M. Jones wrote: > > >From: Jakub Jelen > > > > > >The libssh does not handle non-blocking mode in SFTP correctly. The > > >driver code already changes the mode to blocking for the SFTP > > >initialization, but for some reason changes to non-blocking mode. > > > > "changes to non-blocking mode LATER", I guess, - or else it's a bit > > difficult to read. But this works too. > > > > >This used to work accidentally until libssh in 0.11 branch merged > > >the patch to avoid infinite looping in case of network errors: > > > > > >https://gitlab.com/libssh/libssh-mirror/-/merge_requests/498 > > > > > >Since then, the ssh driver in qemu fails to read files over SFTP > > >as the first SFTP messages exchanged after switching the session > > >to non-blocking mode return SSH_AGAIN, but that message is lost > > >int the SFTP internals and interpretted as SSH_ERROR, which is > > >returned to the caller: > > > > > >https://gitlab.com/libssh/libssh-mirror/-/issues/280 > > > > > >This is indeed an issue in libssh that we should address in the > > >long term, but it will require more work on the internals. For > > >now, the SFTP is not supported in non-blocking mode. > > > > The comment at init where the code sets socket to blocking mode, says: > > > > /* > > * Make sure we are in blocking mode during the connection and > > * authentication phases. > > */ > > ssh_set_blocking(s->session, 1); > > > > > > There are a few other places where the code expect "some" blocking > > mode, changes it to blocking, and restores the mode later, - eg, > > see ssh_grow_file(). It looks all this has to be fixed too. I agree, if we're moving away from non-blocking sessions, then we should remove the switching everywhere. But obviously... > I'll just note that I'm only forwarding on the patch from Jakub. > I didn't write it. > > I did lightly test it, and it seems to work. However it seems also > likely that it is causing qemu to block internally. Probably not > noticable for light use, but not something that we'd want for serious > use. However if libssh doesn't support non-blocking SFTP there's not > much we can do about that in qemu. ...just making it blocking is not acceptable. It will potentially make the guest hang while we're waiting for sftp responses. I see that there is an sftp_aio_*() API, but it looks weird. Instead of allowing you to just poll the next request that is ready, you have to call a (blocking) wait on a specific request. co_yield(), which is currently used when sftp_read() returns SSH_AGAIN, makes sure that we poll the socket fd, so we can know that _something_ new has arrived. However it's unclear to me how to know _which_ request received a reply and can be completed now. It seems you have to call sftp_aio_wait_*() in non-blocking mode on all requests to do that, which probably is affected by the libssh bug, too. So I'm not sure if sftp_aio_*() can be combined with something else into a working solution, and I also don't know if it's affected by the same libssh bug. Jakub, can you help with that? > I would recommend using nbdkit-ssh-plugin instead anyway as it is much > more featureful and doesn't have this problem as we use real threads > instead of coroutines. Telling people to switch away from QEMU is not an appropriate fix for the problem. QEMU has all of the infrastructure with thread pools etc., that's not a unique thing of nbdkit. So if libssh can't provide working non-blocking connections, we'll have to use blocking sftp_read() in a worker thread. It's uglier than using a proper asynchronous interface, but we'll have to work with whatever we get from the library. As far as I can see, libssh sessions aren't thread safe, so we'll have to make sure to have only one request going at the same time, but I assume that calling ssh_read/write() from different threads sequentially isn't a problem? > > I wonder if qemu ssh driver needs to mess with blocking mode of this > > socket in the first place, ever. Is there a way qemu can get non-blocking > > socket in this context? I can only think of fd=NNN, but is it > > possible for this socket to be non-blocking? I'm not sure if this is actually related to blocking sockets specifically. It seems to me that it's more about blocking behaviour in libssh itself, while it internally uses poll() to avoid blocking. Kevin
Re: [PATCH v10 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Wed, 13 Nov 2024 at 19:16, BALATON Zoltan wrote: > On Wed, 13 Nov 2024, Phil Dennis-Jordan wrote: > > macOS's Cocoa event handling must be done on the initial (main) thread > > of the process. Furthermore, if library or application code uses > > libdispatch, the main dispatch queue must be handling events on the main > > thread as well. > > > > So far, this has affected Qemu in both the Cocoa and SDL UIs, although > > in different ways: the Cocoa UI replaces the default qemu_main function > > with one that spins Qemu's internal main event loop off onto a > > background thread. SDL (which uses Cocoa internally) on the other hand > > uses a polling approach within Qemu's main event loop. Events are > > polled during the SDL UI's dpy_refresh callback, which happens to run > > on the main thread by default. > > > > As UIs are mutually exclusive, this works OK as long as nothing else > > needs platform-native event handling. In the next patch, a new device is > > introduced based on the ParavirtualizedGraphics.framework in macOS. > > This uses libdispatch internally, and only works when events are being > > handled on the main runloop. With the current system, it works when > > using either the Cocoa or the SDL UI. However, it does not when running > > headless. Moreover, any attempt to install a similar scheme to the > > Cocoa UI's main thread replacement fails when combined with the SDL > > UI. > > > > This change tidies up main thread management to be more flexible. > > > > * The qemu_main global function pointer is a custom function for the > > main thread, and it may now be NULL. When it is, the main thread > > runs the main Qemu loop. This represents the traditional setup. > > * When non-null, spawning the main Qemu event loop on a separate > > thread is now done centrally rather than inside the Cocoa UI code. > > * For most platforms, qemu_main is indeed NULL by default, but on > > Darwin, it defaults to a function that runs the CFRunLoop. > > * The Cocoa UI sets qemu_main to a function which runs the > > NSApplication event handling runloop, as is usual for a Cocoa app. > > * The SDL UI overrides the qemu_main function to NULL, thus > > specifying that Qemu's main loop must run on the main > > thread. > > * The GTK UI also overrides the qemu_main function to NULL. > > * For other UIs, or in the absence of UIs, the platform's default > > behaviour is followed. > > > > This means that on macOS, the platform's runloop events are always > > handled, regardless of chosen UI. The new PV graphics device will > > thus work in all configurations. There is no functional change on other > > operating systems. > > > > Signed-off-by: Phil Dennis-Jordan > > Reviewed-by: Akihiko Odaki > > --- > > > > v5: > > > > * Simplified the way of setting/clearing the main loop by going back > > to setting qemu_main directly, but narrowing the scope of what it > > needs to do, and it can now be NULL. > > > > v6: > > > > * Folded function qemu_run_default_main_on_new_thread's code into > > main() > > * Removed whitespace changes left over on lines near code removed > > between v4 and v5 > > > > v9: > > > > * Set qemu_main to NULL for GTK UI as well. > > > > v10: > > > > * Added comments clarifying the functionality and purpose of qemu_main. > > > > include/qemu-main.h | 21 ++-- > > include/qemu/typedefs.h | 1 + > > system/main.c | 50 ++ > > ui/cocoa.m | 54 ++--- > > ui/gtk.c| 8 ++ > > ui/sdl2.c | 4 +++ > > 6 files changed, 90 insertions(+), 48 deletions(-) > > > > diff --git a/include/qemu-main.h b/include/qemu-main.h > > index 940960a7dbc..fc70681c8b5 100644 > > --- a/include/qemu-main.h > > +++ b/include/qemu-main.h > > @@ -5,7 +5,24 @@ > > #ifndef QEMU_MAIN_H > > #define QEMU_MAIN_H > > > > -int qemu_default_main(void); > > -extern int (*qemu_main)(void); > > +/* > > + * The function to run on the main (initial) thread of the process. > > + * NULL means QEMU's main event loop. > > + * When non-NULL, QEMU's main event loop will run on a purposely created > > + * thread, after which the provided function pointer will be invoked on > > + * the initial thread. > > + * This is useful on platforms which treat the main thread as special > > + * (macOS/Darwin) and/or require all UI API calls to occur from a > > + * specific thread. > > + * Implementing this via a global function pointer variable is a bit > > + * ugly, but it's probably worth investigating the existing UI thread > rule > > + * violations in the SDL (e.g. #2537) and GTK+ back-ends. Fixing those > > + * issues might precipitate requirements similar but not identical to > those > > + * of the Cocoa UI; hopefully we'll see some kind of pattern emerge, > which > > + * can then be used as a basis for an overhaul. (In fact, it may turn > > + * out to be simplest to split the UI/native platform event t
Re: [PATCH v10 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Thu, Nov 14, 2024 at 11:32 AM Phil Dennis-Jordan wrote:
>> I checked what GTK+ does and, either way, you have to create another
>> thread: timers are handled with a CFRunLoopTimer, but file descriptors
>> are polled in a separate thread and sent to the main thread with a
>> single CFRunLoopSource. It's a bit nicer that the main thread is in
>> charge, but it's more complex and probably slower too.
>
>
> Just to clarify: is this supposed to be happening inside the GTK+ library
> itself? i.e. GTK should spawn its own thread to poll file descriptors that
> are owned by GTK? (As opposed to the file descriptors used by QEMU's own
> event loop - what on Linux are eventfds, but on macOS I think are just
> pipes*.)
It's what I saw in the GTK+ source code.
https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gdk/macos/gdkmacoseventsource.c?ref_type=heads
> This doesn't describe what I'm seeing when I run with -display gtk on macOS.
> There's no extra thread created. There's a dock icon, but it's
> non-interactive ("Application not responding"), there aren't any menus, and
> there's no window. QEMU's own simulation is running in the background - I can
> reach a guest via the network. So I guess there's some function in GTK we're
> supposed to be calling that will make it crank the native event loop on
> macOS, but this isn't being done?
In theory it should work, with the event source added as soon as GTK+
is started... aha!
We do not use the GMainContext's poll_func, we use qemu_poll_ns.
That's the missing part: GDK replaces the poll_func with one that
calls nextEventMatchingMask:
https://gitlab.gnome.org/GNOME/gtk/-/blame/main/gdk/macos/gdkmacoseventsource.c?ref_type=heads#L767
There could be more issues, but I think for now it's better to block
the GTK+ UI under macOS.
[...]
>> As long as it's clear that any handlers that go through the CFRunLoop
>> run outside the BQL, as is already the case for the Cocoa UI, I see no
>> problem with this approach.
>
> I'm not entirely sure what you're getting at here, to be honest. The UI
> thread can definitely not assume to be holding the BQL all the time; we'd
> have to treat it more like an AIOContext. It could pave the way towards
> putting the display and UI subsystems on their own AIOContext or
> AIOContext-like-thing, rather than hogging the BQL for expensive image
> operations.
Don't worry, I was mostly talking to myself... The UI thread, and more
specifically any handlers that are called from CFRunLoop instead of
QEMU's main loop, will have to use mutexes or bql_lock()/bql_unlock(),
like ui/cocoa.m already does.
In other words, code that interacts with Apple's paravirtualized
graphics needs to know if it runs from the CFRunLoop or from
qemu_main().
> (*By the sound of it, Win32 has an all-UI-calls-on-one-thread requirement as
> well which we may be violating to some degree via the GTK and/or SDL backends
> as well; my adventures with Win32 are almost 20 years back now so I'm a bit
> out of the loop there.)
Hmm, no I don't remember anything like that for Windows but it's also
been many years for me.
Paolo
Re: [PATCH v10 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Thu, 14 Nov 2024, Phil Dennis-Jordan wrote: On Wed, 13 Nov 2024 at 19:16, BALATON Zoltan wrote: On Wed, 13 Nov 2024, Phil Dennis-Jordan wrote: macOS's Cocoa event handling must be done on the initial (main) thread of the process. Furthermore, if library or application code uses libdispatch, the main dispatch queue must be handling events on the main thread as well. So far, this has affected Qemu in both the Cocoa and SDL UIs, although in different ways: the Cocoa UI replaces the default qemu_main function with one that spins Qemu's internal main event loop off onto a background thread. SDL (which uses Cocoa internally) on the other hand uses a polling approach within Qemu's main event loop. Events are polled during the SDL UI's dpy_refresh callback, which happens to run on the main thread by default. As UIs are mutually exclusive, this works OK as long as nothing else needs platform-native event handling. In the next patch, a new device is introduced based on the ParavirtualizedGraphics.framework in macOS. This uses libdispatch internally, and only works when events are being handled on the main runloop. With the current system, it works when using either the Cocoa or the SDL UI. However, it does not when running headless. Moreover, any attempt to install a similar scheme to the Cocoa UI's main thread replacement fails when combined with the SDL UI. This change tidies up main thread management to be more flexible. * The qemu_main global function pointer is a custom function for the main thread, and it may now be NULL. When it is, the main thread runs the main Qemu loop. This represents the traditional setup. * When non-null, spawning the main Qemu event loop on a separate thread is now done centrally rather than inside the Cocoa UI code. * For most platforms, qemu_main is indeed NULL by default, but on Darwin, it defaults to a function that runs the CFRunLoop. * The Cocoa UI sets qemu_main to a function which runs the NSApplication event handling runloop, as is usual for a Cocoa app. * The SDL UI overrides the qemu_main function to NULL, thus specifying that Qemu's main loop must run on the main thread. * The GTK UI also overrides the qemu_main function to NULL. * For other UIs, or in the absence of UIs, the platform's default behaviour is followed. This means that on macOS, the platform's runloop events are always handled, regardless of chosen UI. The new PV graphics device will thus work in all configurations. There is no functional change on other operating systems. Signed-off-by: Phil Dennis-Jordan Reviewed-by: Akihiko Odaki --- v5: * Simplified the way of setting/clearing the main loop by going back to setting qemu_main directly, but narrowing the scope of what it needs to do, and it can now be NULL. v6: * Folded function qemu_run_default_main_on_new_thread's code into main() * Removed whitespace changes left over on lines near code removed between v4 and v5 v9: * Set qemu_main to NULL for GTK UI as well. v10: * Added comments clarifying the functionality and purpose of qemu_main. include/qemu-main.h | 21 ++-- include/qemu/typedefs.h | 1 + system/main.c | 50 ++ ui/cocoa.m | 54 ++--- ui/gtk.c| 8 ++ ui/sdl2.c | 4 +++ 6 files changed, 90 insertions(+), 48 deletions(-) diff --git a/include/qemu-main.h b/include/qemu-main.h index 940960a7dbc..fc70681c8b5 100644 --- a/include/qemu-main.h +++ b/include/qemu-main.h @@ -5,7 +5,24 @@ #ifndef QEMU_MAIN_H #define QEMU_MAIN_H -int qemu_default_main(void); -extern int (*qemu_main)(void); +/* + * The function to run on the main (initial) thread of the process. + * NULL means QEMU's main event loop. + * When non-NULL, QEMU's main event loop will run on a purposely created + * thread, after which the provided function pointer will be invoked on + * the initial thread. + * This is useful on platforms which treat the main thread as special + * (macOS/Darwin) and/or require all UI API calls to occur from a + * specific thread. + * Implementing this via a global function pointer variable is a bit + * ugly, but it's probably worth investigating the existing UI thread rule + * violations in the SDL (e.g. #2537) and GTK+ back-ends. Fixing those + * issues might precipitate requirements similar but not identical to those + * of the Cocoa UI; hopefully we'll see some kind of pattern emerge, which + * can then be used as a basis for an overhaul. (In fact, it may turn + * out to be simplest to split the UI/native platform event thread from the + * QEMU main event loop on all platforms, with any UI or even none at all.) + */ +extern qemu_main_fn qemu_main; qemu_main_fn is defined in typdefefs.h but not included here. Also coding style says typedefs should be CamelCase but maybe it's just not worth to define a type for this and you can just leave the existing line here only
[PATCH] vpc: Read images exported from Azure correctly
It was found that 'qemu-nbd' is not able to work with some disk images exported from Azure. Looking at the 512b footer (which contains VPC metadata): 63 6f 6e 65 63 74 69 78 00 00 00 02 00 01 00 00 |conectix| 0010 ff ff ff ff ff ff ff ff 2e c7 9b 96 77 61 00 00 |wa..| 0020 00 07 00 00 57 69 32 6b 00 00 00 01 40 00 00 00 |Wi2k@...| 0030 00 00 00 01 40 00 00 00 28 a2 10 3f 00 00 00 02 |@...(..?| 0040 ff ff e7 47 8c 54 df 94 bd 35 71 4c 94 5f e5 44 |...G.T...5qL._.D| 0050 44 53 92 1a 00 00 00 00 00 00 00 00 00 00 00 00 |DS..| 0060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 || we can see that Azure uses a different 'Creator application' -- 'wa\0\0' (offset 0x1c, likely reads as 'Windows Azure') and QEMU uses this field to determine how it can get image size. Apparently, Azure uses 'new' method, just like Hyper-V. Signed-off-by: Vitaly Kuznetsov --- Alternatively, we can probably make 'current_size' the default and only use CHS for 'vpc '/'qemu'. --- block/vpc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block/vpc.c b/block/vpc.c index d95a204612b7..da8662402d00 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -321,6 +321,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, * 'qemu' : CHS QEMU (uses disk geometry) * 'qem2' : current_size QEMU (uses current_size) * 'win ' : current_size Hyper-V + * 'wa\0\0': current_size Azure * 'd2v ' : current_size Disk2vhd * 'tap\0' : current_size XenServer * 'CTXS' : current_size XenConverter @@ -330,6 +331,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, * that have CHS geometry of the maximum size. */ use_chs = (!!strncmp(footer->creator_app, "win ", 4) && + !!strncmp(footer->creator_app, "wa", 4) && !!strncmp(footer->creator_app, "qem2", 4) && !!strncmp(footer->creator_app, "d2v ", 4) && !!strncmp(footer->creator_app, "CTXS", 4) && -- 2.47.0
