Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
marcandre.lur...@redhat.com writes: > From: Marc-André Lureau > > This is just moving qapi-gen.py and related subdir to qemu-common, to > ease review and proceed step by step. The following patches will move > related necessary code, tests etc. > > Signed-off-by: Marc-André Lureau As moved files tend to become low-level annoyances for a long time, I'd like to understand why you want to move them. The commit message says "to ease review", which I suspect isn't the real reason. Perhaps you explained all that elsewhere already, but I missed it.
Re: [PATCH v3] hw/i386: place setup_data at fixed place in memory
On 8/5/22 01:04, Jason A. Donenfeld wrote: +/* Nothing else uses this part of the hardware mapped region */ +setup_data_base = 0xf - 0x1000; Isn't this where the BIOS lives? I don't think this works. Does it work to place setup_data at the end of the cmdline file instead of having it at the end of the kernel file? This way the first item will be at 0x2 + cmdline_size. Paolo
Re: Regression in -readconfig [memory] size (was: [PULL 13/27] machine: add mem compound property)
On Fri, Aug 05, 2022 at 11:30:12AM +0200, Markus Armbruster wrote:
> Markus Armbruster writes:
>
> > Paolo Bonzini writes:
> >
> >> Make -m syntactic sugar for a compound property "-machine
> >> mem.{size,max-size,slots}". The new property does not have
> >> the magic conversion to megabytes of unsuffixed arguments,
> >> and also does not understand that "0" means the default size
> >> (you have to leave it out to get the default). This means
> >> that we need to convert the QemuOpts by hand to a QDict.
> >>
> >> Signed-off-by: Paolo Bonzini
> >> Message-Id: <20220414165300.555321-4-pbonz...@redhat.com>
> >> Signed-off-by: Paolo Bonzini
>
> [...]
>
> > This appears to change the meaning of
> >
> > [memory]
> > size = "1024"
> >
> > in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to
> > 8KiB silently).
>
> No reply so far.
>
> If we can't fix this, we better mention it in the release notes.
That is such a massive change in semantics of -readconfig that if we
can't fix it we might as well just go all out and fully drop all notion
of backcompat for -readconfig.
With 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] pc-bios/s390-ccw: Fix booting with logical block size < physical block size
For accessing single blocks during boot, it's the logical block size that
matters. (Physical block sizes are rather interesting e.g. for creating
file systems with the correct alignment for speed reasons etc.).
So the s390-ccw bios has to use the logical block size for calculating
sector numbers during the boot phase, the "physical_block_exp" shift
value must not be taken into account. This change fixes the boot process
when the guest hast been installed on a disk where the logical block size
differs from the physical one, e.g. if the guest has been installed
like this:
qemu-system-s390x -nographic -accel kvm -m 2G \
-drive if=none,id=d1,file=fedora.iso,format=raw,media=cdrom \
-device virtio-scsi -device scsi-cd,drive=d1 \
-drive if=none,id=d2,file=test.qcow2,format=qcow2
-device virtio-blk,drive=d2,physical_block_size=4096,logical_block_size=512
Linux correctly uses the logical block size of 512 for the installation,
but the s390-ccw bios tries to boot from a disk with 4096 block size so
far, as long as this patch has not been applied yet (well, it used to work
by accident in the past due to the virtio_assume_scsi() hack that used to
enforce 512 byte sectors on all virtio-block disks, but that hack has been
well removed in commit 5447de2619050a0a4d to fix other scenarios).
Fixes: 5447de2619 ("pc-bios/s390-ccw/virtio-blkdev: Remove
virtio_assume_scsi()")
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2112303
Signed-off-by: Thomas Huth
---
pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 8271c47296..794f99b42c 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -173,7 +173,7 @@ int virtio_get_block_size(void)
switch (vdev->senseid.cu_model) {
case VIRTIO_ID_BLOCK:
-return vdev->config.blk.blk_size <<
vdev->config.blk.physical_block_exp;
+return vdev->config.blk.blk_size;
case VIRTIO_ID_SCSI:
return vdev->scsi_block_size;
}
--
2.31.1
[PATCH 1/1] tests/qtest: add scenario for -readconfig handling
This test of -readconfig validates the last three regressions we
have fixed with -readconfig:
* Interpretation of memory size units as MiB not bytes
* Allow use of [spice]
* Allow use of [object]
Signed-off-by: Daniel P. Berrangé
---
tests/qtest/meson.build | 1 +
tests/qtest/readconfig-test.c | 195 ++
2 files changed, 196 insertions(+)
create mode 100644 tests/qtest/readconfig-test.c
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 3a474010e4..be4b30dea2 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -26,6 +26,7 @@ qtests_generic = [
'qom-test',
'test-hmp',
'qos-test',
+ 'readconfig-test',
]
if config_host.has_key('CONFIG_MODULES')
qtests_generic += [ 'modules-test' ]
diff --git a/tests/qtest/readconfig-test.c b/tests/qtest/readconfig-test.c
new file mode 100644
index 00..2e604d7c2d
--- /dev/null
+++ b/tests/qtest/readconfig-test.c
@@ -0,0 +1,195 @@
+/*
+ * Validate -readconfig
+ *
+ * Copyright (c) 2022 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qapi/error.h"
+#include "qapi/qapi-visit-machine.h"
+#include "qapi/qapi-visit-qom.h"
+#include "qapi/qapi-visit-ui.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qmp/qstring.h"
+#include "qemu/units.h"
+
+static QTestState *qtest_init_with_config(const char *cfgdata)
+{
+GError *error = NULL;
+g_autofree char *args = NULL;
+int cfgfd = -1;
+g_autofree char *cfgpath = NULL;
+QTestState *qts;
+ssize_t ret;
+
+cfgfd = g_file_open_tmp("readconfig-test-XX", , );
+g_assert_no_error(error);
+g_assert_cmpint(cfgfd, >=, 0);
+
+ret = qemu_write_full(cfgfd, cfgdata, strlen(cfgdata));
+if (ret < 0) {
+unlink(cfgpath);
+}
+g_assert_cmpint(ret, ==, strlen(cfgdata));
+
+close(cfgfd);
+
+args = g_strdup_printf("-nodefaults -machine none -readconfig %s",
cfgpath);
+
+qts = qtest_init(args);
+
+unlink(cfgpath);
+
+return qts;
+}
+
+static void test_x86_memdev_resp(QObject *res)
+{
+Visitor *v;
+g_autoptr(MemdevList) memdevs = NULL;
+Memdev *memdev;
+
+g_assert(res);
+v = qobject_input_visitor_new(res);
+visit_type_MemdevList(v, NULL, , _abort);
+
+g_assert(memdevs);
+g_assert(memdevs->value);
+g_assert(!memdevs->next);
+
+memdev = memdevs->value;
+g_assert_cmpstr(memdev->id, ==, "ram");
+g_assert_cmpint(memdev->size, ==, 200 * MiB);
+
+visit_free(v);
+}
+
+static void test_x86_memdev(void)
+{
+QDict *resp;
+QTestState *qts;
+const char *cfgdata =
+"[memory]\n"
+"size = \"200\"";
+
+qts = qtest_init_with_config(cfgdata);
+ /* Test valid command */
+resp = qtest_qmp(qts, "{ 'execute': 'query-memdev' }");
+test_x86_memdev_resp(qdict_get(resp, "return"));
+qobject_unref(resp);
+
+qtest_quit(qts);
+}
+
+
+#ifdef CONFIG_SPICE
+static void test_spice_resp(QObject *res)
+{
+Visitor *v;
+g_autoptr(SpiceInfo) spice = NULL;
+
+g_assert(res);
+v = qobject_input_visitor_new(res);
+visit_type_SpiceInfo(v, "spcie", , _abort);
+
+g_assert(spice);
+g_assert(spice->enabled);
+
+visit_free(v);
+}
+
+static void test_spice(void)
+{
+QDict *resp;
+QTestState *qts;
+const char *cfgdata =
+"[spice]\n"
+"disable-ticketing = \"on\"\n"
+"unix = \"on\"\n";
+
+qts = qtest_init_with_config(cfgdata);
+ /* Test valid command */
+resp = qtest_qmp(qts, "{ 'execute': 'query-spice' }");
+test_spice_resp(qdict_get(resp, "return"));
+qobject_unref(resp);
+
+qtest_quit(qts);
+}
+#endif
+
+static void test_object_rng_resp(QObject *res)
+{
+Visitor *v;
+g_autoptr(ObjectPropertyInfoList) objs = NULL;
+ObjectPropertyInfoList *tmp;
+ObjectPropertyInfo *obj;
+bool seen_rng = false;
+
+g_assert(res);
+v = qobject_input_visitor_new(res);
+visit_type_ObjectPropertyInfoList(v, NULL, , _abort);
+
+g_assert(objs);
+tmp = objs;
+while (tmp) {
+g_assert(tmp->value);
+
+obj = tmp->value;
+if (g_str_equal(obj->name, "rng0") &&
+g_str_equal(obj->type, "child")) {
+seen_rng = true;
+}
+
+tmp = tmp->next;
+}
+
+g_assert(seen_rng);
+
+visit_free(v);
+}
+
+static void test_object_rng(void)
+{
+QDict *resp;
+QTestState *qts;
+const char *cfgdata =
+"[object]\n"
+"qom-type = \"rng-builtin\"\n"
+"id = \"rng0\"\n";
+
+qts = qtest_init_with_config(cfgdata);
+ /* Test valid command */
+resp = qtest_qmp(qts,
+ "{ 'execute': 'qom-list',"
+ " 'arguments': {'path': '/objects' }}");
+
Re: [PATCH v2 12/20] ppc/ppc405: QOM'ify EBC
On Fri, 5 Aug 2022, Cédric Le Goater wrote:
On 8/4/22 21:26, BALATON Zoltan wrote:
On Thu, 4 Aug 2022, Peter Maydell wrote:
On Thu, 4 Aug 2022 at 19:03, BALATON Zoltan wrote:
I was trying to find out how to do it but I don't understand QOM enough
to
answer the simple question of how to get the cpu object from QOM. My
guesses are:
object_resolve_path_type("/machine", TYPE_POWERPC_CPU, NULL)
Out of curiosity would this work though to get the cpu or if not why not
and what would be a preferred way? I could not find this out from reading
the object.h comments, the docs/deve/qom.rst, nor searching the code.
You could scan the object topology using object_child_foreach_recursive()
and use object_dynamic_cast() to find a POWERPC CPU object. A link is
much faster !
Yes that sounds a lot slower, I hoped there's simple easy way to get to
the cpu, then we could simplify this a bit.
One more idea, you've introduced the Ppc405MachineState which you can get
to casting current_machine and so it's a convenient place to store a cpu
pointer or even just use PPC405_MACHINE(current_machine)->soc.cpu. This
works for these 405 machines you've changed in this series but other
PPC4xx machines don't have this machine and soc states yet. Could these
Ppc405MachineState and Ppc405SoCState be more generic as
Ppc4xxMachineState and Ppc4xxSoCState considering that these chips are
quite similar or maybe we need an abstract PPC4xxSoC class but machine
state could be shared? (If you say this is too much cahnges for you now I
may try to look at this later once your series is merged but going that
way now could save some churn.) If we had a Ppc4xxMachineState that has a
cpu pointer then we could easily add that to bamboo and sam460ex now and
these QOMified devices could then use PPC4XX_MACHINE(current_machine)->cpu
instead of a link property. What do you think?
If this is the preferred way then so be it, I just don't like it because I
think this is too many boilerplate code that could be avoided. This series:
9 files changed, 894 insertions(+), 652 deletions(-)
and that's including removing all of the taihu machine; the file where
the QOMification is done:
hw/ppc/ppc405_uc.c | 799 +++-
Yes. You should consider also that this code is > 15 years old and
serious shortcuts have been taken to "make things work". I think QOM
clarifies the models and represents better the HW topology. There is
a price in being explicit.
I know this is a mess curently but QOM is full of boilerplate code which
is confusing for new people and makes it hard to undestand the code. So
cutting down the boilerplate and making things simpler would help people
who want to get started with QEMU development. If adding a property was
3-4 additional lines I wouldn't care but if it makes the code
significantly more complex and thus harder to understand at a glance then
I'd rather avoid it if possible and stick to simple terms. Verbosity is
good if it explains things better but bad if it's hiding the important
details between unneeded complexity due to having to use constructs that
are not obvious. Also the property needs to be set which is additional
lines and possibility for mistakes so if there's a way to avoid that I
think that's better.
Ideally introducing QOM should make it simpler not more complex. Four of
the QOMified devices only have a property defined at all because of this
cpu link that's only used once in the realize method to register DCRs. This
is about 10 lines of code each. If there was a simple way to get the cpu
object from these realize methods then we could get rid of all these
properties and save about 40-50 lines and make these simpler.
I tried several approaches and found this one was the simplest and not
too verbose really.
The DCRs are accessed by software through the use of the mtdcr and mfdcr
instructions. These are converted in transactions on a side band bus,
the DCR bus, which connects the on-SoC devices to the CPU. The "cpu" link
should be considered as modeling this little piece of HW logic connecting
the device to the DCR bus.
I rather consider it an implementation detail and trying to find the
simplest way. If we don't find anything simpler I'm OK with link
properties but I'm not convinced yet there's no simpler solution to this
problem that could avoid some of the additional complexity.
Regards,
BALATON Zoltan
Re: [PATCH v10 21/21] job: remove unused functions
Am 25.07.2022 um 09:38 hat Emanuele Giuseppe Esposito geschrieben: > These public functions are not used anywhere, thus can be dropped. > Also, since this is the final job API that doesn't use AioContext > lock and replaces it with job_lock, adjust all remaining function > documentation to clearly specify if the job lock is taken or not. > > Signed-off-by: Emanuele Giuseppe Esposito > Reviewed-by: Stefan Hajnoczi You're also documenting the locking requirements for a few functions where you don't remove a second version. Reviewed-by: Kevin Wolf
Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions
Am 05/08/2022 um 15:22 schrieb Emanuele Giuseppe Esposito:
>
>
> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>>> The aim of this series is to reorganize bdrv_try_set_aio_context
>>> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in
>>> favour of a new one, ->change_aio_ctx.
>>>
>>> More informations in patch 3 (which is also RFC, due to the doubts
>>> I have with AioContext locks).
>>>
>>> Patch 1 just add assertions in the code, 2 extends the transactions
>>> API to be able to add also transactions in the tail
>>> of the list.
>>> Patch 3 is the core of this series, and introduces the new callback.
>>> It is marked as RFC and the reason is explained in the commit message.
>>> Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob
>>> and block-backend BDSes.
>>> Patch 7 substitutes ->change_aio_ctx with the old callbacks, and
>>> patch 8 takes care of deleting the old callbacks and unused code.
>>>
>>> This series is based on "job: replace AioContext lock with job_mutex",
>>> but just because it uses job_set_aio_context() introduced there.
>>>
>>> Suggested-by: Paolo Bonzini
>>> Based-on:<20220706201533.289775-1-eespo...@redhat.com>
>>
>>
>
> So, I read your email before going on PTO and at that point I got what
> your concerns were, but now after re-reading it I don't understand
> anymore what you mean :)
>
>> What I dislike here is that you refactor aio-context-change to use
>> transactions, but you use it "internally" for aio-context-change.
>> aio-context-change doesn't become a native part of block-graph
>> modifiction transaction system after the series.
>>
>> For example, bdrv_attach_child_common(..., tran) still calls
>> bdrv_try_change_aio_context() function which doesn't take @tran
>> argument. And we have to call bdrv_try_change_aio_context() again in
>> bdrv_attach_child_common_abort() with opposite arguments by hand. We
>> create in _try_ separate Transaction object which is unrelated to the
>> original block-graph-change transaction.
>>
>
> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
> transaction parameter" supports taking transaction as a parameter.
> bdrv_attach_child_common could simply call
> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
> it could work).
No actually I don't get how it can work in bdrv_attach_child_common.
We have the following:
parent_ctx = bdrv_child_get_parent_aio_context(new_child);
if (child_ctx != parent_ctx) {
int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL,
_err);
if (ret < 0 && child_class->change_aio_ctx) {
ret_child = child_class->change_aio_ctx(new_child, child_ctx,
visited, tran, NULL);
tran_finalize(tran, ret_child == true ? 0 : -1);
}
if (ret < 0) {
return ret;
}
}
bdrv_replace_child_noperm(_child, child_bs, true);
So bdrv_try_change_aio_context would be changed in
bdrv_try_change_aio_context_tran, but then how can we call
bdrv_replace_child_noperm if no aiocontext has been changed at all?
I don't think we can mix transactional operations with non-transactional.
Emanuele
>
> I think the main concern here is that during the prepare phase this
> serie doesn't change any aiocontext, so until we don't commit the rest
> of the code cannot assume that the aiocontext has been changed.
>
> But isn't it what happens also for permissions? Permission functions
> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
> bdrv_set_perm() in commit.
>
> Another important question is that if we actually want to put everything
> in a single transaction, because .prepare functions like the one
> proposed here perform drains, so the logic following prepare and
> preceding commit must take into account that everything is drained. Also
> prepare itself has to take into account that maybe other .prepare took
> locks or drained themselves...
>
>> With good refactoring we should get rid of these _try_ functions, and
>> have just bdrv_change_aio_context(..., tran) that can be natively used
>> with external tran object, where other block-graph change actions
>> participate. This way we'll not have to call reverse
>> aio_context_change() in .abort, it will be done automatically.
>>
>> Moreover, your *aio_context_change* functions that do have tran
>> parameter cannot be simply used in the block-graph change transaction,
>> as you don't follow the common paradigm, that in .prepare we do all
>> visible changes. That's why you have to still use _try_*() version that
>> creates seaparate transaction object and completes it: after that the
>> action is actually done and other graph-modifying actions can be done on
>> top.
>>
>> So, IMHO, we shouldn't go this way, as that adds transaction actions
>> that actually can't be used in
Re: [PATCH] hw/ppc: ppc440_uc: avoid multiply overflow in dcr_write_dma()
On Fri, 5 Aug 2022, Daniel Henrique Barboza wrote:
Coverity reports a OVERFLOW_BEFORE_WIDEN issue in dcr_write_dma(). When
handling the DMA0_CR switch we're doing a multiplication between two
integers (count and width), and the product is assigned to an uint64_t
(xferlen). The int32 product can be overflow before widened.
It can't in practice though as count is max 0x and width is max 8 so
this sounds like a Coverity false positive. Maybe you could avoid it by
changing the type of count or width to uint64_t instead of casting?
Fix it by casting the first operand to uint64_t to force the product to
be 64 bit.
Fixes: Coverity CID 1490852
Fixes: 3c409c1927ef ("ppc440_uc: Basic emulation of PPC440 DMA controller")
This line does not appear in 3c409c1927ef so this second Fixes line is
bogus. This was added to fix other Coverity issues in eda3f17bcd7b96 but
still did not make Coverity happy :-)
Regards,
BALATON Zoltan
Cc: BALATON Zoltan
Signed-off-by: Daniel Henrique Barboza
---
hw/ppc/ppc440_uc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 11fdb88c22..31eeffa946 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -908,7 +908,7 @@ static void dcr_write_dma(void *opaque, int dcrn, uint32_t
val)
sidx = didx = 0;
width = 1 << ((val & DMA0_CR_PW) >> 25);
-xferlen = count * width;
+xferlen = (uint64_t)count * width;
wlen = rlen = xferlen;
rptr = cpu_physical_memory_map(dma->ch[chnl].sa, ,
false);
