Re: [PATCH RFC 0/7] hostmem: NUMA-aware memory preallocation using ThreadContext
On 9/21/22 16:54, David Hildenbrand wrote: > On 21.09.22 16:44, Michal Prívozník wrote: >> On 7/21/22 14:07, David Hildenbrand wrote: >>> >> >> Ping? Is there any plan how to move forward? I have libvirt patches >> ready to consume this and I'd like to prune my old local branches :-) > > Heh, I was thinking about this series just today. I was distracted with > all other kind of stuff. > > I'll move forward with this series later this week/early next week. No rush, it's only that I don't want this to fall into void. Let me know if I can help somehow. Meanwhile, here's my aforementioned branch: https://gitlab.com/MichalPrivoznik/libvirt/-/tree/qemu_thread_context I've made it so that ThreadContext is generated whenever .prealloc-threads AND .host-nodes are used (i.e. no XML visible config knob). And I'm generating ThreadContext objects for each memory backend separately even though they can be reused, but IMO that's optimization that can be done later. Michal
Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one
Claudio Fontana writes:
> On 9/21/22 14:16, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> On 16/9/22 11:27, Markus Armbruster wrote:
Claudio Fontana writes:
> improve error handling during module load, by changing:
>
> bool module_load_one(const char *prefix, const char *lib_name);
> void module_load_qom_one(const char *type);
>
> to:
>
> bool module_load_one(const char *prefix, const char *name, Error **errp);
> bool module_load_qom_one(const char *type, Error **errp);
>
> module_load_qom_one has been introduced in:
>
> commit 28457744c345 ("module: qom module support"), which built on top of
> module_load_one, but discarded the bool return value. Restore it.
>
> Adapt all callers to emit errors, or ignore them, or fail hard,
> as appropriate in each context.
How exactly does behavior change? The commit message is mum on the
behavior before the patch, and vague on the behavior afterwards.
> Signed-off-by: Claudio Fontana
> ---
> audio/audio.c | 9 ++-
> block.c | 15 -
> block/dmg.c | 18 +-
> hw/core/qdev.c| 10 ++-
> include/qemu/module.h | 38 ++--
> qom/object.c | 18 +-
> softmmu/qtest.c | 6 +-
> ui/console.c | 18 +-
> util/module.c | 140 --
> 9 files changed, 194 insertions(+), 78 deletions(-)
>>>
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 8c012bbe03..78d4c4de96 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -61,16 +61,44 @@ typedef enum {
>>>
>
> void module_call_init(module_init_type type);
> -bool module_load_one(const char *prefix, const char *lib_name);
> -void module_load_qom_one(const char *type);
> +
> +/*
> + * module_load_one: attempt to load a module from a set of directories
> + *
> + * directories searched are:
> + * - getenv("QEMU_MODULE_DIR")
> + * - get_relocated_path(CONFIG_QEMU_MODDIR);
> + * - /var/run/qemu/${version_dir}
> + *
> + * prefix: a subsystem prefix, or the empty string ("audio-",
> ..., "")
> + * name: name of the module
> + * errp: error to set in case the module is found, but load
> failed.
> + *
> + * Return value: true on success (found and loaded);
> + * if module if found, but load failed, errp will be set.
> + * if module is not found, errp will not be set.
I understand you need to distingush two failure modes "found, but load
failed" and "not found".
Functions that set an error on some failures only tend to be awkward: in
addition to checking the return value for failure, you have to check
@errp for special failures. This is particularly cumbersome when it
requires a @local_err and an error_propagate() just for that. I
generally prefer to return an error code and always set an error.
>>>
>>> I notice the same issue, therefore would suggest this alternative
>>> prototype:
>>>
>>>bool module_load_one(const char *prefix, const char *name,
>>> bool ignore_if_missing, Error **errp);
>>> which always sets *errp when returning false:
>>>
>>> * Return value: if ignore_if_missing is true:
>>> * true on success (found or missing), false on
>>> * load failure.
>>> * if ignore_if_missing is false:
>>> * true on success (found and loaded); false if
>>> * not found or load failed.
>>> * errp will be set if the returned value is false.
>>> */
>>
>> I think this interface is less surprising.
>>
>> If having to pass a flag turns out to to be a legibility issue, we can
>> have wrapper functions.
>>
>
> To me it seems even more confusing tbh. Do we have more ideas? Richard?
>
> bool module_load_one(const char *prefix, const char *name, Error **errp);
>
> In my mind we should really say,
>
> RETURN VALUE: a bool with the meaning: "was a module loaded? true/false"
>
> ERROR: The Error parameter tells us: "was there an error loading the module?"
>
> Makes sense to me, but maybe someone has a better one.
>
> Btw, as an aside, for the general Error API pattern, if the bool return value
> and Error != NULL should be always related 1:1,
> It would have been better to design it with only one of those informing about
> the error (Error, as it carries the additional Error information, besides the
> information that Error != NULL),
> and remove the extra degree of freedom for a return value that at this point
> (in the current code) carries ZERO additional useful information.
>
> We could have designed the API to use the return value as... an actual
Re: [PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers
Hi, > > Why not just use virtio-gpu? > > Trying to run this command: > qemu-system-x86_64 -M microvm -m 2048 -device virtio-gpu '-device virtio-gpu-device' Might also need '-global virtio-mmio.force-legacy=false' to switch virtio-mmio into 1.0 mode. take care, Gerd
Re: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs
"Kasireddy, Vivek" writes:
> Hi Markus,
>
> Thank you for the review.
>
>> Vivek Kasireddy writes:
>>
>> > The new parameter named "connector" can be used to assign physical
>> > monitors/connectors to individual GFX VCs such that when the monitor
>> > is connected or hotplugged, the associated GTK window would be
>> > fullscreened on it. If the monitor is disconnected or unplugged,
>> > the associated GTK window would be destroyed and a relevant
>> > disconnect event would be sent to the Guest.
>> >
>> > Usage: -device
>> > virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080...
>> >-display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.
>> >
>> > Cc: Dongwon Kim
>> > Cc: Gerd Hoffmann
>> > Cc: Daniel P. Berrangé
>> > Cc: Markus Armbruster
>> > Cc: Philippe Mathieu-Daudé
>> > Cc: Marc-André Lureau
>> > Cc: Thomas Huth
>> > Signed-off-by: Vivek Kasireddy
>> > ---
>> > qapi/ui.json| 9 ++-
>> > qemu-options.hx | 1 +
>> > ui/gtk.c| 168
>> > 3 files changed, 177 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/qapi/ui.json b/qapi/ui.json
>> > index 286c5731d1..86787a4c95 100644
>> > --- a/qapi/ui.json
>> > +++ b/qapi/ui.json
>> > @@ -1199,13 +1199,20 @@
>> > # interfaces (e.g. VGA and virtual console character
>> > devices)
>> > # by default.
>> > # Since 7.1
>> > +# @connector: List of physical monitor/connector names where the GTK
>> > windows
>> > +# containing the respective graphics virtual consoles (VCs)
>> > are
>> > +# to be placed. If a mapping exists for a VC, it will be
>> > +# fullscreened on that specific monitor or else it would
>> > not be
>> > +# displayed anywhere and would appear disconnected to the
>> > guest.
>>
>> Let's see whether I understand this... We have VCs numbered 0, 1, ...
>> VC #i is mapped to the i-th element of @connector, counting from zero.
>> Correct?
>
> [Kasireddy, Vivek] Yes, correct.
>
>> Ignorant question: what's a "physical monitor/connector name"?
>
> [Kasireddy, Vivek] IIUC, while the HDMI/DP protocols refer to a receiver (RX)
> as a "sink", the DRM Graphics subsystem in the kernel uses the term
> "connector"
> and the GTK library prefers the term "monitor".
Awesome.
> All of these terms can be
> used interchangeably but I chose the term connector for the new parameter
> after debating between connector, monitor, output, etc.
Okay.
> The connector name (e.g. DP-1, HDMI-A-2, etc) uniquely identifies a connector
> or a monitor on any given system:
> https://elixir.bootlin.com/linux/v6.0-rc6/source/include/drm/drm_connector.h#L1328
> If you are on Intel hardware, you can find more info related to connectors by
> doing:
> cat /sys/kernel/debug/dri/0/i915_display_info
Thanks!
>> Would you mind breaking the lines a bit earlier, between column 70 and
>> 75?
>
> [Kasireddy, Vivek] Np, will do that.
>
>>
>> > +# Since 7.2
>> > #
>> > # Since: 2.12
>> > ##
>> > { 'struct' : 'DisplayGTK',
>> >'data': { '*grab-on-hover' : 'bool',
>> > '*zoom-to-fit' : 'bool',
>> > -'*show-tabs' : 'bool' } }
>> > +'*show-tabs' : 'bool',
>> > +'*connector' : ['str'] } }
>> >
>> > ##
>> > # @DisplayEGLHeadless:
Elsewhere in ui.json, names of list-valued members use plural: channels,
clients, keys, addresses. Let's name this one connectors for
consistency.
With that, QAPI schema
Acked-by: Markus Armbruster
>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index 31c04f7eea..576b65ef9f 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>> > #if defined(CONFIG_GTK)
>> > "-display
>> > gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
>> > "
>> > [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
>> > +"[,connector.=]\n"
>>
>> Is "" a VC number?
>
> [Kasireddy, Vivek] Yes.
An "id" is commonly a name. Suggest connector..
>> > #endif
>> > #if defined(CONFIG_VNC)
>> > "-display vnc=[,]\n"
A bit of documentation is missing:
``show-cursor=on|off`` : Force showing the mouse cursor
``window-close=on|off`` : Allow to quit qemu with window close
button
+``connector.`` :
``curses[,charset=]``
>>
>> Remainder of my review is on style only. Style suggestions are not
>> demands :)
>
> [Kasireddy, Vivek] No problem; most of your suggestions are sensible
> and I'll include them all in v2.
Thanks!
Re: [PATCH v1 01/10] monitor: expose monitor_puts to rest of code
Alex Bennée writes: > This helps us construct strings elsewhere before echoing to the > monitor. It avoids having to jump through hoops like: > > monitor_printf(mon, "%s", s->str); > > It will be useful in following patches but for now convert all > existing plain "%s" printfs to use the _puts api. > > Signed-off-by: Alex Bennée > Reviewed-by: Richard Henderson Reviewed-by: Markus Armbruster
Re: [PATCH v2 26/39] tests/qtest: migration-test: Make sure QEMU process "to" exited after migration is canceled
On Thu, Sep 22, 2022 at 5:54 AM Marc-André Lureau wrote: > > Hi > > On Tue, Sep 20, 2022 at 3:18 PM Bin Meng wrote: >> >> From: Xuzhou Cheng >> >> Make sure QEMU process "to" exited before launching another target >> for migration in the test_multifd_tcp_cancel case. >> >> Signed-off-by: Xuzhou Cheng >> Signed-off-by: Bin Meng >> Reviewed-by: Marc-André Lureau > > > fwiw, I didn't r-b the version with a busy wait > (https://patchew.org/QEMU/20220824094029.1634519-1-bmeng...@gmail.com/20220824094029.1634519-42-bmeng...@gmail.com/) > My mistake. The R-B tag was added before I changed the implementation and I forgot to remove the tag. Regards, Bin
Re: [PATCH v2 35/39] tests/qtest: migration-test: Skip running some TLS cases for win32
On Thu, Sep 22, 2022 at 1:23 AM Daniel P. Berrangé wrote:
>
> On Wed, Sep 21, 2022 at 05:51:33PM +0100, Dr. David Alan Gilbert wrote:
> > * Bin Meng (bmeng...@gmail.com) wrote:
> > > From: Bin Meng
> > >
> > > Some migration test cases use TLS to communicate, but they fail on
> > > Windows with the following error messages:
> > >
> > > qemu-system-x86_64: TLS handshake failed: Insufficient credentials for
> > > that request.
> > > qemu-system-x86_64: TLS handshake failed: Error in the pull function.
> > > query-migrate shows failed migration: TLS handshake failed: Error in
> > > the pull function.
> > >
> > > Disable them temporarily.
> > >
> > > Signed-off-by: Bin Meng
> > > ---
> > > I am not familar with the gnutls and simply enabling the gnutls debug
> > > output does not give me an immedidate hint on why it's failing on
> > > Windows. Disable these cases for now until someone or maintainers
> > > who may want to test this on Windows.
> >
> > Copying in Dan Berrange, he's our expert on weird TLS failures.
>
> Seems to match this:
>
>https://gnutls.org/faq.html#key-usage-violation2
>
> which suggests we have a configuration mis-match.
>
> I'm surprised to see you are only needing to disable the TLS PSK tests,
> not the TLS x509 tests.
The TLS x509 qtests all passed.
>
> I'd like to know if tests/unit/test-crypto-tlssession passes.
These unit tests currently are not built on Windows as they simply
don't build due to usage of socketpair().
>
> If so, it might suggest we are missing 'priority: NORMAL' property
> when configuring TLS creds for the migration test.
I did the following changes but the error is still the same:
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index dbee9b528a..c1e3f11873 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -788,7 +788,8 @@ test_migrate_tls_psk_start_common(QTestState *from,
" 'id': 'tlscredspsk0',"
" 'endpoint': 'client',"
" 'dir': %s,"
- " 'username': 'qemu'} }",
+ " 'username': 'qemu',"
+ " 'priority': 'NORMAL'} }",
data->workdir);
qobject_unref(rsp);
@@ -797,7 +798,8 @@ test_migrate_tls_psk_start_common(QTestState *from,
" 'arguments': { 'qom-type': 'tls-creds-psk',"
" 'id': 'tlscredspsk0',"
" 'endpoint': 'server',"
- " 'dir': %s } }",
+ " 'dir': %s,"
+ " 'priority': 'NORMAL'} }",
mismatch ? data->workdiralt : data->workdir);
qobject_unref(rsp);
I am not sure whether I did the right changes.
>
> > > (no changes since v1)
> > >
> > > tests/qtest/migration-test.c | 14 ++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > > index aedd9ddb72..dbee9b528a 100644
> > > --- a/tests/qtest/migration-test.c
> > > +++ b/tests/qtest/migration-test.c
> > > @@ -1403,6 +1403,7 @@ static void test_precopy_unix_dirty_ring(void)
> > > }
> > >
> > > #ifdef CONFIG_GNUTLS
> > > +#ifndef _WIN32
> > > static void test_precopy_unix_tls_psk(void)
> > > {
> > > g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> > > @@ -1415,6 +1416,7 @@ static void test_precopy_unix_tls_psk(void)
> > >
> > > test_precopy_common(&args);
> > > }
> > > +#endif /* _WIN32 */
> > >
> > > #ifdef CONFIG_TASN1
> > > static void test_precopy_unix_tls_x509_default_host(void)
> > > @@ -1523,6 +1525,7 @@ static void test_precopy_tcp_plain(void)
> > > }
> > >
> > > #ifdef CONFIG_GNUTLS
> > > +#ifndef _WIN32
> > > static void test_precopy_tcp_tls_psk_match(void)
> > > {
> > > MigrateCommon args = {
> > > @@ -1533,6 +1536,7 @@ static void test_precopy_tcp_tls_psk_match(void)
> > >
> > > test_precopy_common(&args);
> > > }
> > > +#endif /* _WIN32 */
> > >
> > > static void test_precopy_tcp_tls_psk_mismatch(void)
> > > {
> > > @@ -1930,6 +1934,7 @@ static void test_multifd_tcp_zstd(void)
> > > #endif
> > >
> > > #ifdef CONFIG_GNUTLS
> > > +#ifndef _WIN32
> > > static void *
> > > test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
> > > QTestState *to)
> > > @@ -1937,6 +1942,7 @@
> > > test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
> > > test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
> > > return test_migrate_tls_psk_start_match(from, to);
> > > }
> > > +#endif /* _WIN32 */
> > >
> > > static void *
> > > test_migrate_multifd_tcp_tls_psk_start_mismatch(QTestState *from,
> > > @@ -1988,6 +1994,7 @@
> > > test_migrate_multifd_tls_x509_start_reject_anon_client(QTestState *from,
> > > }
> > > #endif /* CONFIG_TASN1 */
> > >
> > > +#ifndef _WIN32
> > > static void test_multifd_tcp_tls_psk_match(void)
> > > {
> > > MigrateCommon args = {
> > > @@ -1997,6 +2004,7 @@ static void test_multifd_tcp_tls_psk_match(void)
> > > };
> > > test_precopy_common(&args);
> > > }
> > > +#endif /* _WIN32 */
> > >
> > > static void test_multifd_tcp_tls_psk_mismatch(void)
> > > {
> > > @@ -2497,8 +2505
Re: [PATCH v3 0/5] hw/arm/virt: Improve address assignment for high memory regions
[PATCH v3 0/5] hw/arm/virt: Improve address assignment for high memory regions
Author: Gavin Shan
Date: Thu Sep 22 07:13:45 2022 +0800
PATCH[1-3] preparatory work for the improvment
PATCH[4] improve high memory region address assignment
PATCH[5] adds 'highmem-compact' to enable or disable the optimization
Signed-off-by: Gavin Shan
The patchs works well on "IPA Size Limit: 40" FUJITSU machine.
I added some debug code to show high memory region address.
The test results are as expected.
1. virt-7.2 default
# /home/qemu/build/qemu-system-aarch64 -accel kvm -cpu host -machine
virt-7.2 -m 4G,maxmem=511G -monitor stdio
=> virt_set_high_memmap: pa_bits=40, base=0x80
[HIGH_GIC_REDIST2]: disabled, highest_gpa=0x7f[no] [yes]
[HIGH_PCIE_ECAM]: disabled, highest_gpa=0x7f[no] [yes]
[HIGH_PCIE_MMIO] enabled, highest_gpa=0xff
2. When virt-7.2,highmem-compact=off
# /home/qemu/build/qemu-system-aarch64 -accel kvm -cpu host -machine
virt-7.2,highmem-compact=off -m 4G,maxmem=511G -monitor stdio
=> virt_set_high_memmap: pa_bits=40, base=0x80
[HIGH_GIC_REDIST2]: disabled, highest_gpa=0x8003ff[no] [yes]
[HIGH_PCIE_ECAM]: disabled, highest_gpa=0x801fff[no] [yes]
[HIGH_PCIE_MMIO]: disabled, highest_gpa=0x801fff[no] [no]
3. virt-7.1 default
# /home/qemu/build/qemu-system-aarch64 -accel kvm -cpu host -machine
virt-7.1 -m 4G,maxmem=511G -monitor stdio
=> virt_set_high_memmap: pa_bits=40, base=0x80
[HIGH_GIC_REDIST2]: disabled, highest_gpa=0x8003ff[no] [yes]
[HIGH_PCIE_ECAM]: disabled, highest_gpa=0x801fff[no] [yes]
[HIGH_PCIE_MMIO]: disabled, highest_gpa=0x801fff[no] [no]
2. When virt-7.1,highmem-compact=on
# /home/qemu/build/qemu-system-aarch64 -accel kvm -cpu host -machine
virt-7.1,highmem-compact=on -m 4G,maxmem=511G -monitor stdio
=> virt_set_high_memmap: pa_bits=40, base=0x80
[HIGH_GIC_REDIST2]: disabled, highest_gpa=0x7f[no] [yes]
[HIGH_PCIE_ECAM]: disabled, highest_gpa=0x7f[no] [yes]
[HIGH_PCIE_MMIO] enabled, highest_gpa=0xff
Tested-by: Zhenyu Zhang
On Thu, Sep 22, 2022 at 7:13 AM Gavin Shan wrote:
>
> There are three high memory regions, which are VIRT_HIGH_REDIST2,
> VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
> are floating on highest RAM address. However, they can be disabled
> in several cases.
>
> (1) One specific high memory region is disabled by developer by
> toggling vms->highmem_{redists, ecam, mmio}.
>
> (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
> 'virt-2.12' or ealier than it.
>
> (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
> on 32-bits system.
>
> (4) One specific high memory region is disabled when it breaks the
> PA space limit.
>
> The current implementation of virt_set_memmap() isn't comprehensive
> because the space for one specific high memory region is always
> reserved from the PA space for case (1), (2) and (3). In the code,
> 'base' and 'vms->highest_gpa' are always increased for those three
> cases. It's unnecessary since the assigned space of the disabled
> high memory region won't be used afterwards.
>
> The series intends to improve the address assignment for these
> high memory regions.
>
> PATCH[1-3] preparatory work for the improvment
> PATCH[4] improve high memory region address assignment
> PATCH[5] adds 'highmem-compact' to enable or disable the optimization
>
> History
> ===
> v2: https://lore.kernel.org/all/20220815062958.100366-1-gs...@redhat.com/T/
> v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html
>
> Changelog
> ==
> v3:
> * Reorder the patches(Gavin)
> * Add 'highmem-compact' property for backwards compatibility (Eric)
> v2:
> * Split the patches for easier review(Gavin)
> * Improved changelog (Marc)
> * Use 'bool fits' in virt_set_high_memmap() (Eric)
>
> Gavin Shan (5):
> hw/arm/virt: Introduce virt_set_high_memmap() helper
> hw/arm/virt: Rename variable size to region_size in
> virt_set_high_memmap()
> hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()
> hw/arm/virt: Improve high memory region address assignment
> hw/arm/virt: Add 'highmem-compact' property
>
> docs/system/arm/virt.rst | 4 ++
> hw/arm/virt.c| 116 ---
> include/hw/arm/virt.h| 2 +
> 3 files changed, 89 insertions(+), 33 deletions(-)
>
> --
> 2.23.0
>
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
wrote:
>
> If I read your response on the other thread correctly, this change is intended
>
> to prioritize the MAC address exposed by DPDK over the one provided by the
>
> QEMU command line? Sounds reasonable in principle, but I would get
> confirmation
>
> from vDPA/vhost-net maintainers.
I think the best way is to (and it seems easier)
1) have the management layer to make sure the mac came from cli
matches the underlayer vDPA
2) having a sanity check and fail the device initialization if they don't match
Thanks
>
>
>
> That said the way you’re hacking the vhost-user code breaks a valuable check
> for
>
> bad vhost-user-blk backends. I would suggest finding another implementation
> which
>
> does not regress functionality for other device types.
>
>
>
>
>
> >From: Hao Chen
>
> >
>
> >When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
>
> >start the virtual machine through libvirt or qemu, but now, the libvirt or
>
> >qemu can call dpdk vdpa vendor driver's ops .get_config through
> >vhost_net_get_config
>
> >to get the mac address of the vdpa hardware without manual configuration.
>
> >
>
> >v1->v2:
>
> >Only copy ETH_ALEN data of netcfg for some vdpa device such as
>
> >NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
>
> >We only need the mac address and don't care about the status field.
>
> >
>
> >Signed-off-by: Hao Chen
>
> >---
>
> > hw/block/vhost-user-blk.c | 1 -
>
> > hw/net/virtio-net.c | 7 +++
>
> > hw/virtio/vhost-user.c| 19 ---
>
> > 3 files changed, 7 insertions(+), 20 deletions(-)
>
> >
>
> >diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>
> >index 9117222456..5dca4eab09 100644
>
> >--- a/hw/block/vhost-user-blk.c
>
> >+++ b/hw/block/vhost-user-blk.c
>
> >@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
> >Error **errp)
>
> >
>
> > vhost_dev_set_config_notifier(&s->dev, &blk_ops);
>
> >
>
> >-s->vhost_user.supports_config = true;
>
> > ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER,
> > 0,
>
> > errp);
>
> > if (ret < 0) {
>
> >diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>
> >index dd0d056fde..90405083b1 100644
>
> >--- a/hw/net/virtio-net.c
>
> >+++ b/hw/net/virtio-net.c
>
> >@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice *vdev,
> >uint8_t *config)
>
> > }
>
> > memcpy(config, &netcfg, n->config_size);
>
> > }
>
> >+} else if (nc->peer && nc->peer->info->type ==
> >NET_CLIENT_DRIVER_VHOST_USER) {
>
> >+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
> >*)&netcfg,
>
> >+ n->config_size);
>
> >+if (ret != -1) {
>
> >+ /* Automatically obtain the mac address of the vdpa device
>
> >+* when using the dpdk vdpa */
>
> >+memcpy(config, &netcfg, ETH_ALEN);
>
> > }
>
> > }
>
> >
>
> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>
> >index bd24741be8..8b01078249 100644
>
> >--- a/hw/virtio/vhost-user.c
>
> >+++ b/hw/virtio/vhost-user.c
>
> >@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
> >*dev, void *opaque,
>
> > }
>
> >
>
> > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>
> >-bool supports_f_config = vus->supports_config ||
>
> >-(dev->config_ops && dev->config_ops->vhost_dev_config_notifier);
>
> > uint64_t protocol_features;
>
> >
>
> > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>
> >@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
> >*dev, void *opaque,
>
> > */
>
> > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
>
> >
>
> >-if (supports_f_config) {
>
> >-if (!virtio_has_feature(protocol_features,
>
> >-VHOST_USER_PROTOCOL_F_CONFIG)) {
>
> >-error_setg(errp, "vhost-user device expecting "
>
> >- "VHOST_USER_PROTOCOL_F_CONFIG but the vhost-user
> >backend does "
>
> >- "not support it.");
>
> >-return -EPROTO;
>
> >-}
>
> >-} else {
>
> >-if (virtio_has_feature(protocol_features,
>
> >- VHOST_USER_PROTOCOL_F_CONFIG)) {
>
> >-warn_reportf_err(*errp, "vhost-user backend supports "
>
> >- "VHOST_USER_PROTOCOL_F_CONFIG but QEMU
> >does not.");
>
> >-protocol_features &= ~(1ULL <<
> >VHOST_USER_PROTOCOL_F_CONFIG);
>
> >-}
>
> >-}
>
> >-
>
> > /* final set of protocol features */
>
> > dev->protocol_features = protocol_features;
>
> > err = vhost_user_set_protocol_features(dev, dev->protocol_fea
Re: [PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"
On Thu, Sep 22, 2022 at 12:12 AM Peter Xu wrote:
>
> It's true that when vcpus<=255 we don't require the length of 32bit APIC
> IDs. However here since we already have EIM=ON it means the hypervisor
> will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
> EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
> even if vcpus<=255. In short, commit 77250171bdc breaks any simple cmdline
> that wants to boot a VM with >=9 but <=255 vcpus with:
>
> -device intel-iommu,intremap=on
>
> For anyone who does not want to enable x2apic, we can use eim=off in the
> intel-iommu parameters to skip enabling KVM x2apic.
>
> This partly reverts commit 77250171bdc02aee106083fd2a068147befa1a38, while
> keeping the valid bit on checking split irqchip, but revert the other change.
>
> Cc: David Woodhouse
> Cc: Claudio Fontana
> Cc: Igor Mammedov
> Signed-off-by: Peter Xu
> ---
> hw/i386/intel_iommu.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 05d53a1aa9..6524c2ee32 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3818,6 +3818,11 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> Error **errp)
> error_setg(errp, "eim=on requires
> accel=kvm,kernel-irqchip=split");
> return false;
> }
> +if (!kvm_enable_x2apic()) {
> +error_setg(errp, "eim=on requires support on the KVM side"
> + "(X2APIC_API, first shipped in v4.7)");
> +return false;
> +}
I wonder if we need some work on the migration compatibility here
(though it could be tricky).
Thanks
> }
>
> /* Currently only address widths supported are 39 and 48 bits */
> --
> 2.32.0
>
Re: [PATCH v3 4/5] acpi/nvdimm: Implement ACPI NVDIMM Label Methods
On Wed, 2022-09-21 at 15:29 +0200, Igor Mammedov wrote:
> On Tue, 20 Sep 2022 20:28:31 +0800
> Robert Hoo wrote:
>
> > On Tue, 2022-09-20 at 11:13 +0200, Igor Mammedov wrote:
> > > On Fri, 16 Sep 2022 21:15:35 +0800
> > > Robert Hoo wrote:
> > >
> > > > On Fri, 2022-09-16 at 09:37 +0200, Igor Mammedov wrote:
> > > >
> > > > > > Fine, get your point now.
> > > > > > In ASL it will look like this:
> > > > > > Local1 = Package (0x3) {STTS, SLSA,
> > > > > > MAXT}
> > > > > > Return (Local1)
> > > > >
> > > > >
> > > > > >
> > > > > > But as for
> > > > > > CreateDWordField (Local0, Zero,
> > > > > > STTS) //
> > > > > > Status
> > > > > > CreateDWordField (Local0, 0x04,
> > > > > > SLSA) //
> > > > > > SizeofLSA
> > > > > > CreateDWordField (Local0, 0x08,
> > > > > > MAXT) //
> > > > > > Max
> > > > > > Trans
> > > > > >
> > > > > > I cannot figure out how to substitute with LocalX. Can you
> > > > > > shed
> > > > > > more
> > > > > > light?
> > > > >
> > > > > Leave this as is, there is no way to make it anonymous/local
> > > > > with
> > > > > FooField.
> > > > >
> > > > > (well one might try to use Index and copy field's bytes into
> > > > > a
> > > > > buffer
> > > > > and
> > > > > then explicitly convert to Integer, but that's a rather
> > > > > convoluted
> > > > > way
> > > > > around limitation so I'd not go this route)
> > > > >
> > > >
> > > > OK, pls. take a look, how about this?
> > > >
> > > > Method (_LSI, 0, Serialized) // _LSI: Label Storage
> > > > Information
> > > > {
> > > > Local0 = NCAL (ToUUID("4309ac30-0d11-11e4-9191-
> > > > 0800200c9a66"),
> > > > 0x02, 0x04, Zero, One)// Buffer
> > > > CreateDWordField (Local0, Zero, STTS) // Status
> > > > CreateDWordField (Local0, 0x04, SLSA) // Size of LSA
> > > > CreateDWordField (Local0, 0x08, MAXT) // Max Transfer Size
> > > > Local1 = Package (0x3) {STTS, SLSA, MAXT}
> > > > Return (Local1)
> > > > }
> > > >
> > > > Method (_LSR, 2, Serialized) // _LSR: Label Storage Read
> > > > {
> > > > Name (INPT, Buffer(8) {})
> > > > CreateDWordField (INPT, Zero, OFST);
> > > > CreateDWordField (INPT, 4, LEN);
> > >
> > > why do you have to create and use INPT, wouldn't local be enough
> > > to
> > > keep the buffer?
> >
> > If substitute INPT with LocalX, then later
> > Local0 = Package (0x01) {LocalX} isn't accepted.
> >
> > PackageElement :=
> > DataObject | NameString
>
> ok, then respin series and lets get it some testing.
Sure. I'd also like it to go through your tests, though I had done some
ordinary tests like guest create/delete/init-labels on vNVDIMM.
>
> BTW:
> it looks like Windows Server starting from v2019 has support for
> NVDIMM-P devices which came with 'Optane DC Persistent Memory
> Modules'
> but it fails to recognize NVDIMMs in QEMU (complaining something
> about
> wrong target). Perhaps you can reach someone with Optane/ACPI
> expertise within your org and try to fix QEMU side.
Yes, it's a known gap there. I will try that once I had some time and
resource.
>
> > >
> > > > OFST = Arg0
> > > > LEN = Arg1
> > > > Local0 = Package (0x01) {INPT}
> > > > Local3 = NCAL (ToUUID("4309ac30-0d11-11e4-9191-
> > > > 0800200c9a66"),
> > > > 0x02, 0x05, Local0, One)
> > > > CreateDWordField (Local3, Zero, STTS)
> > > > CreateField (Local3, 32, LEN << 3, LDAT)
> > > > Local1 = Package (0x2) {STTS, toBuffer(LDAT)}
> > > > Return (Local1)
> > > > }
> > > >
> > > > Method (_LSW, 3, Serialized) // _LSW: Label Storage Write
> > > > {
> > > > Local2 = Arg2
> > > > Name (INPT, Buffer(8) {})
> > >
> > > ditto
> > >
> > > > CreateDWordField (INPT, Zero, OFST);
> > > > CreateDWordField (INPT, 4, TLEN);
> > > > OFST = Arg0
> > > > TLEN = Arg1
> > > > Concatenate(INPT, Local2, INPT)
> > > > Local0 = Package (0x01)
> > > > {
> > > > INPT
> > > > }
> > > > Local3 = NCAL (ToUUID ("4309ac30-0d11-11e4-9191-
> > > > 0800200c9a66"),
> > > > 0x02, 0x06, Local0, One)
> > > > CreateDWordField (Local3, 0, STTS)
> > > > Return (STTS)
> > > > }
> >
> >
>
>
[PATCH] hw/net: npcm7xx_emc: set MAC in register space
The MAC address set from Qemu wasn't being saved into the register space.
Reviewed-by: Hao Wu
Signed-off-by: Patrick Venture
---
hw/net/npcm7xx_emc.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
index 7c86bb52e5..6be1008529 100644
--- a/hw/net/npcm7xx_emc.c
+++ b/hw/net/npcm7xx_emc.c
@@ -96,6 +96,9 @@ static const char *emc_reg_name(int regno)
#undef REG
}
+static void npcm7xx_emc_write(void *opaque, hwaddr offset,
+ uint64_t v, unsigned size);
+
static void emc_reset(NPCM7xxEMCState *emc)
{
trace_npcm7xx_emc_reset(emc->emc_num);
@@ -112,6 +115,18 @@ static void emc_reset(NPCM7xxEMCState *emc)
emc->tx_active = false;
emc->rx_active = false;
+
+/* Set the MAC address in the register space. */
+uint32_t value = (emc->conf.macaddr.a[0] << 24) |
+(emc->conf.macaddr.a[1] << 16) |
+(emc->conf.macaddr.a[2] << 8) |
+emc->conf.macaddr.a[3];
+npcm7xx_emc_write(emc, REG_CAMM_BASE * sizeof(uint32_t), value,
+ sizeof(uint32_t));
+
+value = (emc->conf.macaddr.a[4] << 24) | (emc->conf.macaddr.a[5] << 16);
+npcm7xx_emc_write(emc, REG_CAML_BASE * sizeof(uint32_t), value,
+ sizeof(uint32_t));
}
static void npcm7xx_emc_reset(DeviceState *dev)
--
2.37.3.998.g577e59143f-goog
Re: [PATCH v2 13/23] target/i386: Introduce DISAS_JUMP
On 9/21/22 12:28, Paolo Bonzini wrote:
On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson
wrote:
Drop the unused dest argument to gen_jr().
Remove most of the calls to gen_jr, and use DISAS_JUMP.
Remove some unused loads of eip for lcall and ljmp.
The only use outside i386_tr_tb_stop is here:
static void gen_goto_tb(DisasContext *s, int tb_num, target_ulong eip)
{
target_ulong pc = s->cs_base + eip;
if (translator_use_goto_tb(&s->base, pc)) {
/* jump to same page: we can use a direct jump */
tcg_gen_goto_tb(tb_num);
gen_jmp_im(s, eip);
tcg_gen_exit_tb(s->base.tb, tb_num);
s->base.is_jmp = DISAS_NORETURN;
} else {
/* jump to another page */
gen_jmp_im(s, eip);
gen_jr(s);
}
}
Should it set s->base.is_jmp = DISAS_JUMP instead, so that gen_jr() can be
inlined into i386_tr_tb_stop() and removed completely? If not,
It can't, because of conditional branches which do
brcond something, L1
gen_goto_tb
L1
gen_goto_tb
The first gen_goto_tb can't just fall through, it needs to exit.
r~
[PATCH v3 3/5] hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()
This introduces variable 'region_base' for the base address of the
specific high memory region. It's the preparatory work to optimize
high memory region address assignment.
No functional change intended.
Signed-off-by: Gavin Shan
---
hw/arm/virt.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 187b3ee0e2..b0b679d1f4 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1692,15 +1692,15 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState
*vms, int idx)
static void virt_set_high_memmap(VirtMachineState *vms,
hwaddr base, int pa_bits)
{
-hwaddr region_size;
+hwaddr region_base, region_size;
bool fits;
int i;
for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
+region_base = ROUND_UP(base, extended_memmap[i].size);
region_size = extended_memmap[i].size;
-base = ROUND_UP(base, region_size);
-vms->memmap[i].base = base;
+vms->memmap[i].base = region_base;
vms->memmap[i].size = region_size;
/*
@@ -1709,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
*
* For each device that doesn't fit, disable it.
*/
-fits = (base + region_size) <= BIT_ULL(pa_bits);
+fits = (region_base + region_size) <= BIT_ULL(pa_bits);
if (fits) {
-vms->highest_gpa = base + region_size - 1;
+vms->highest_gpa = region_base + region_size - 1;
}
switch (i) {
@@ -1726,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
break;
}
-base += region_size;
+base = region_base + region_size;
}
}
--
2.23.0
[PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property
After the improvement to high memory region address assignment is
applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
memory region is enabled when the improvement is applied, but it's
disabled if the improvement isn't applied.
pa_bits = 40;
vms->highmem_redists = false;
vms->highmem_ecam= false;
vms->highmem_mmio= true;
# qemu-system-aarch64 -accel kvm -cpu host \
-machine virt-7.2 -m 4G,maxmem=511G \
-monitor stdio
In order to keep backwords compatibility, we need to disable the
optimization on machines, which is virt-7.1 or ealier than it. It
means the optimization is enabled by default from virt-7.2. Besides,
'highmem-compact' property is added so that the optimization can be
explicitly enabled or disabled on all machine types by users.
Signed-off-by: Gavin Shan
---
docs/system/arm/virt.rst | 4
hw/arm/virt.c| 33 +
include/hw/arm/virt.h| 2 ++
3 files changed, 39 insertions(+)
diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 20442ea2c1..f05ec2253b 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -94,6 +94,10 @@ highmem
address space above 32 bits. The default is ``on`` for machine types
later than ``virt-2.12``.
+highmem-compact
+ Set ``on``/``off`` to enable/disable compact space for high memory regions.
+ The default is ``on`` for machine types later than ``virt-7.2``
+
gic-version
Specify the version of the Generic Interrupt Controller (GIC) to provide.
Valid values are:
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b702f8f2b5..a4fbdaef91 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1734,6 +1734,13 @@ static void virt_set_high_memmap(VirtMachineState *vms,
base = region_base + region_size;
} else {
*region_enabled = false;
+
+if (!vms->highmem_compact) {
+base = region_base + region_size;
+if (fits) {
+vms->highest_gpa = region_base + region_size - 1;
+}
+}
}
}
}
@@ -2348,6 +2355,20 @@ static void virt_set_highmem(Object *obj, bool value,
Error **errp)
vms->highmem = value;
}
+static bool virt_get_highmem_compact(Object *obj, Error **errp)
+{
+VirtMachineState *vms = VIRT_MACHINE(obj);
+
+return vms->highmem_compact;
+}
+
+static void virt_set_highmem_compact(Object *obj, bool value, Error **errp)
+{
+VirtMachineState *vms = VIRT_MACHINE(obj);
+
+vms->highmem_compact = value;
+}
+
static bool virt_get_its(Object *obj, Error **errp)
{
VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -2966,6 +2987,13 @@ static void virt_machine_class_init(ObjectClass *oc,
void *data)
"Set on/off to enable/disable using "
"physical address space above 32
bits");
+object_class_property_add_bool(oc, "highmem-compact",
+ virt_get_highmem_compact,
+ virt_set_highmem_compact);
+object_class_property_set_description(oc, "highmem-compact",
+ "Set on/off to enable/disable
compact "
+ "space for high memory regions");
+
object_class_property_add_str(oc, "gic-version", virt_get_gic_version,
virt_set_gic_version);
object_class_property_set_description(oc, "gic-version",
@@ -3050,6 +3078,7 @@ static void virt_instance_init(Object *obj)
/* High memory is enabled by default */
vms->highmem = true;
+vms->highmem_compact = !vmc->no_highmem_compact;
vms->gic_version = VIRT_GIC_VERSION_NOSEL;
vms->highmem_ecam = !vmc->no_highmem_ecam;
@@ -3119,8 +3148,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
static void virt_machine_7_1_options(MachineClass *mc)
{
+VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
virt_machine_7_2_options(mc);
compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
+/* Compact space for high memory regions was introduced with 7.2 */
+vmc->no_highmem_compact = true;
}
DEFINE_VIRT_MACHINE(7, 1)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 6ec479ca2b..c7dd59d7f1 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -125,6 +125,7 @@ struct VirtMachineClass {
bool no_pmu;
bool claim_edge_triggered_timers;
bool smbios_old_sys_ver;
+bool no_highmem_compact;
bool no_highmem_ecam;
bool no_ged; /* Machines < 4.2 have no support for ACPI GED device */
bool kvm_no_adjvtime;
@@ -144,6 +145,7 @@ struct VirtMachineState {
PFlashCFI01 *flash[2];
bool secure;
bool highmem;
+bool highmem_compact;
bool highmem_ecam;
bool highmem_mmio;
bool highmem_redists;
--
2.23.0
[PATCH v3 1/5] hw/arm/virt: Introduce virt_set_high_memmap() helper
This introduces virt_set_high_memmap() helper. The logic of high
memory region address assignment is moved to the helper. The intention
is to make the subsequent optimization for high memory region address
assignment easier.
No functional change intended.
Signed-off-by: Gavin Shan
---
hw/arm/virt.c | 74 ---
1 file changed, 41 insertions(+), 33 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0961e053e5..4dab528b82 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1689,6 +1689,46 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState
*vms, int idx)
return arm_cpu_mp_affinity(idx, clustersz);
}
+static void virt_set_high_memmap(VirtMachineState *vms,
+ hwaddr base, int pa_bits)
+{
+int i;
+
+for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
+hwaddr size = extended_memmap[i].size;
+bool fits;
+
+base = ROUND_UP(base, size);
+vms->memmap[i].base = base;
+vms->memmap[i].size = size;
+
+/*
+ * Check each device to see if they fit in the PA space,
+ * moving highest_gpa as we go.
+ *
+ * For each device that doesn't fit, disable it.
+ */
+fits = (base + size) <= BIT_ULL(pa_bits);
+if (fits) {
+vms->highest_gpa = base + size - 1;
+}
+
+switch (i) {
+case VIRT_HIGH_GIC_REDIST2:
+vms->highmem_redists &= fits;
+break;
+case VIRT_HIGH_PCIE_ECAM:
+vms->highmem_ecam &= fits;
+break;
+case VIRT_HIGH_PCIE_MMIO:
+vms->highmem_mmio &= fits;
+break;
+}
+
+base += size;
+}
+}
+
static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
{
MachineState *ms = MACHINE(vms);
@@ -1744,39 +1784,7 @@ static void virt_set_memmap(VirtMachineState *vms, int
pa_bits)
/* We know for sure that at least the memory fits in the PA space */
vms->highest_gpa = memtop - 1;
-for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
-hwaddr size = extended_memmap[i].size;
-bool fits;
-
-base = ROUND_UP(base, size);
-vms->memmap[i].base = base;
-vms->memmap[i].size = size;
-
-/*
- * Check each device to see if they fit in the PA space,
- * moving highest_gpa as we go.
- *
- * For each device that doesn't fit, disable it.
- */
-fits = (base + size) <= BIT_ULL(pa_bits);
-if (fits) {
-vms->highest_gpa = base + size - 1;
-}
-
-switch (i) {
-case VIRT_HIGH_GIC_REDIST2:
-vms->highmem_redists &= fits;
-break;
-case VIRT_HIGH_PCIE_ECAM:
-vms->highmem_ecam &= fits;
-break;
-case VIRT_HIGH_PCIE_MMIO:
-vms->highmem_mmio &= fits;
-break;
-}
-
-base += size;
-}
+virt_set_high_memmap(vms, base, pa_bits);
if (device_memory_size > 0) {
ms->device_memory = g_malloc0(sizeof(*ms->device_memory));
--
2.23.0
[PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment
There are three high memory regions, which are VIRT_HIGH_REDIST2,
VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
are floating on highest RAM address. However, they can be disabled
in several cases.
(1) One specific high memory region is disabled by developer by
toggling vms->highmem_{redists, ecam, mmio}.
(2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
'virt-2.12' or ealier than it.
(3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
on 32-bits system.
(4) One specific high memory region is disabled when it breaks the
PA space limit.
The current implementation of virt_set_memmap() isn't comprehensive
because the space for one specific high memory region is always
reserved from the PA space for case (1), (2) and (3). In the code,
'base' and 'vms->highest_gpa' are always increased for those three
cases. It's unnecessary since the assigned space of the disabled
high memory region won't be used afterwards.
This improves the address assignment for those three high memory
region by skipping the address assignment for one specific high
memory region if it has been disabled in case (1), (2) and (3).
Signed-off-by: Gavin Shan
---
hw/arm/virt.c | 44 ++--
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b0b679d1f4..b702f8f2b5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1693,15 +1693,31 @@ static void virt_set_high_memmap(VirtMachineState *vms,
hwaddr base, int pa_bits)
{
hwaddr region_base, region_size;
-bool fits;
+bool *region_enabled, fits;
int i;
for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
region_base = ROUND_UP(base, extended_memmap[i].size);
region_size = extended_memmap[i].size;
-vms->memmap[i].base = region_base;
-vms->memmap[i].size = region_size;
+switch (i) {
+case VIRT_HIGH_GIC_REDIST2:
+region_enabled = &vms->highmem_redists;
+break;
+case VIRT_HIGH_PCIE_ECAM:
+region_enabled = &vms->highmem_ecam;
+break;
+case VIRT_HIGH_PCIE_MMIO:
+region_enabled = &vms->highmem_mmio;
+break;
+default:
+region_enabled = NULL;
+}
+
+/* Skip unknown region */
+if (!region_enabled) {
+continue;
+}
/*
* Check each device to see if they fit in the PA space,
@@ -1710,23 +1726,15 @@ static void virt_set_high_memmap(VirtMachineState *vms,
* For each device that doesn't fit, disable it.
*/
fits = (region_base + region_size) <= BIT_ULL(pa_bits);
-if (fits) {
-vms->highest_gpa = region_base + region_size - 1;
-}
+if (*region_enabled && fits) {
+vms->memmap[i].base = region_base;
+vms->memmap[i].size = region_size;
-switch (i) {
-case VIRT_HIGH_GIC_REDIST2:
-vms->highmem_redists &= fits;
-break;
-case VIRT_HIGH_PCIE_ECAM:
-vms->highmem_ecam &= fits;
-break;
-case VIRT_HIGH_PCIE_MMIO:
-vms->highmem_mmio &= fits;
-break;
+vms->highest_gpa = region_base + region_size - 1;
+base = region_base + region_size;
+} else {
+*region_enabled = false;
}
-
-base = region_base + region_size;
}
}
--
2.23.0
[PATCH v3 2/5] hw/arm/virt: Rename variable size to region_size in virt_set_high_memmap()
This renames variable 'size' to 'region_size' in virt_set_high_memmap().
Its counterpart ('region_base') will be introduced in next patch.
No functional change intended.
Signed-off-by: Gavin Shan
---
hw/arm/virt.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4dab528b82..187b3ee0e2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1692,15 +1692,16 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState
*vms, int idx)
static void virt_set_high_memmap(VirtMachineState *vms,
hwaddr base, int pa_bits)
{
+hwaddr region_size;
+bool fits;
int i;
for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
-hwaddr size = extended_memmap[i].size;
-bool fits;
+region_size = extended_memmap[i].size;
-base = ROUND_UP(base, size);
+base = ROUND_UP(base, region_size);
vms->memmap[i].base = base;
-vms->memmap[i].size = size;
+vms->memmap[i].size = region_size;
/*
* Check each device to see if they fit in the PA space,
@@ -1708,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
*
* For each device that doesn't fit, disable it.
*/
-fits = (base + size) <= BIT_ULL(pa_bits);
+fits = (base + region_size) <= BIT_ULL(pa_bits);
if (fits) {
-vms->highest_gpa = base + size - 1;
+vms->highest_gpa = base + region_size - 1;
}
switch (i) {
@@ -1725,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
break;
}
-base += size;
+base += region_size;
}
}
--
2.23.0
[PATCH v3 0/5] hw/arm/virt: Improve address assignment for high memory regions
There are three high memory regions, which are VIRT_HIGH_REDIST2,
VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
are floating on highest RAM address. However, they can be disabled
in several cases.
(1) One specific high memory region is disabled by developer by
toggling vms->highmem_{redists, ecam, mmio}.
(2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
'virt-2.12' or ealier than it.
(3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
on 32-bits system.
(4) One specific high memory region is disabled when it breaks the
PA space limit.
The current implementation of virt_set_memmap() isn't comprehensive
because the space for one specific high memory region is always
reserved from the PA space for case (1), (2) and (3). In the code,
'base' and 'vms->highest_gpa' are always increased for those three
cases. It's unnecessary since the assigned space of the disabled
high memory region won't be used afterwards.
The series intends to improve the address assignment for these
high memory regions.
PATCH[1-3] preparatory work for the improvment
PATCH[4] improve high memory region address assignment
PATCH[5] adds 'highmem-compact' to enable or disable the optimization
History
===
v2: https://lore.kernel.org/all/20220815062958.100366-1-gs...@redhat.com/T/
v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html
Changelog
==
v3:
* Reorder the patches(Gavin)
* Add 'highmem-compact' property for backwards compatibility (Eric)
v2:
* Split the patches for easier review(Gavin)
* Improved changelog (Marc)
* Use 'bool fits' in virt_set_high_memmap() (Eric)
Gavin Shan (5):
hw/arm/virt: Introduce virt_set_high_memmap() helper
hw/arm/virt: Rename variable size to region_size in
virt_set_high_memmap()
hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()
hw/arm/virt: Improve high memory region address assignment
hw/arm/virt: Add 'highmem-compact' property
docs/system/arm/virt.rst | 4 ++
hw/arm/virt.c| 116 ---
include/hw/arm/virt.h| 2 +
3 files changed, 89 insertions(+), 33 deletions(-)
--
2.23.0
Re: [PATCH 2/3] vdpa: load vlan configuration at NIC startup
On 9/16/2022 6:45 AM, Eugenio Perez Martin wrote:
On Wed, Sep 14, 2022 at 5:44 PM Si-Wei Liu wrote:
On 9/14/2022 2:57 PM, Eugenio Perez Martin wrote:
On Wed, Sep 14, 2022 at 1:33 PM Si-Wei Liu wrote:
On 9/14/2022 3:20 AM, Jason Wang wrote:
On Fri, Sep 9, 2022 at 4:02 PM Eugenio Perez Martin wrote:
On Fri, Sep 9, 2022 at 8:40 AM Jason Wang wrote:
On Fri, Sep 9, 2022 at 2:38 PM Jason Wang wrote:
On Wed, Sep 7, 2022 at 12:36 AM Eugenio Pérez wrote:
To have enabled vlans at device startup may happen in the destination of
a live migration, so this configuration must be restored.
At this moment the code is not accessible, since SVQ refuses to start if
vlan feature is exposed by the device.
Signed-off-by: Eugenio Pérez
---
net/vhost-vdpa.c | 46 --
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 4bc3fd01a8..ecbfd08eb9 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -100,6 +100,8 @@ static const uint64_t vdpa_svq_device_features =
BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
BIT_ULL(VIRTIO_NET_F_STANDBY);
+#define MAX_VLAN(1 << 12) /* Per 802.1Q definition */
+
VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
{
VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -423,6 +425,47 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
return *s->status != VIRTIO_NET_OK;
}
+static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
+ const VirtIONet *n,
+ uint16_t vid)
+{
+ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_VLAN,
+ VIRTIO_NET_CTRL_VLAN_ADD,
+ &vid, sizeof(vid));
+if (unlikely(dev_written < 0)) {
+return dev_written;
+}
+
+if (unlikely(*s->status != VIRTIO_NET_OK)) {
+return -EINVAL;
+}
+
+return 0;
+}
+
+static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
+const VirtIONet *n)
+{
+uint64_t features = n->parent_obj.guest_features;
+
+if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VLAN))) {
+return 0;
+}
+
+for (int i = 0; i < MAX_VLAN >> 5; i++) {
+for (int j = 0; n->vlans[i] && j <= 0x1f; j++) {
+if (n->vlans[i] & (1U << j)) {
+int r = vhost_vdpa_net_load_single_vlan(s, n, (i << 5) + j);
This seems to cause a lot of latency if the driver has a lot of vlans.
I wonder if it's simply to let all vlan traffic go by disabling
CTRL_VLAN feature at vDPA layer.
The guest will not be able to recover that vlan configuration.
Spec said it's best effort and we actually don't do this for
vhost-net/user for years and manage to survive.
I thought there's a border line between best effort and do nothing. ;-)
I also think it is worth at least trying to migrate it for sure. For
MAC there may be too many entries, but VLAN should be totally
manageable (and the information is already there, the bitmap is
actually being migrated).
The vlan bitmap is migrated, though you'd still need to add vlan filter
one by one through ctrl vq commands, correct? AFAIK you can add one
filter for one single vlan ID at a time via VIRTIO_NET_CTRL_VLAN_ADD.
I think that the reason this could survive is really software
implementation specific - say if the backend doesn't start with VLAN
filtering disabled (meaning allow all VLAN traffic to pass) then it
would become a problem. This won't be a problem for almost PF NICs but
may be problematic for vDPA e.g. VF backed VDPAs.
I'd say the driver would expect all vlan filters to be cleared after a
reset, isn't it?
If I understand the intended behavior (from QEMU implementation)
correctly, when VIRTIO_NET_F_CTRL_VLAN is negotiated it would start with
all VLANs filtered (meaning only untagged traffic can be received and
traffic with VLAN tag would get dropped). However, if
VIRTIO_NET_F_CTRL_VLAN is not negotiated, all traffic including untagged
and tagged can be received.
The spec doesn't explicitly say anything about that
as far as I see.
Here the spec is totally ruled by the (software artifact of)
implementation rather than what a real device is expected to work with
VLAN rx filters. Are we sure we'd stick to this flawed device
implementation? The guest driver seems to be agnostic with this broken
spec behavior so far, and I am afraid it's an overkill to add another
feature bit or ctrl command to VLAN filter in clean way.
I agree with all of the above. So, double checking, all vlan should be
allowed by default at device start?
That is true only when VIRTIO_NET_F_CTRL_VLAN is not negotiated. If the
guest already negotiated VIRTIO_NET_F_CTRL_VLAN before being migrated,
device should resume with all VLANs filtered/disallowed.
Maybe the spec needs to be more
c
RE: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs
Hi Markus,
Thank you for the review.
> Vivek Kasireddy writes:
>
> > The new parameter named "connector" can be used to assign physical
> > monitors/connectors to individual GFX VCs such that when the monitor
> > is connected or hotplugged, the associated GTK window would be
> > fullscreened on it. If the monitor is disconnected or unplugged,
> > the associated GTK window would be destroyed and a relevant
> > disconnect event would be sent to the Guest.
> >
> > Usage: -device virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080...
> >-display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.
> >
> > Cc: Dongwon Kim
> > Cc: Gerd Hoffmann
> > Cc: Daniel P. Berrangé
> > Cc: Markus Armbruster
> > Cc: Philippe Mathieu-Daudé
> > Cc: Marc-André Lureau
> > Cc: Thomas Huth
> > Signed-off-by: Vivek Kasireddy
> > ---
> > qapi/ui.json| 9 ++-
> > qemu-options.hx | 1 +
> > ui/gtk.c| 168
> > 3 files changed, 177 insertions(+), 1 deletion(-)
> >
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 286c5731d1..86787a4c95 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1199,13 +1199,20 @@
> > # interfaces (e.g. VGA and virtual console character devices)
> > # by default.
> > # Since 7.1
> > +# @connector: List of physical monitor/connector names where the GTK
> > windows
> > +# containing the respective graphics virtual consoles (VCs)
> > are
> > +# to be placed. If a mapping exists for a VC, it will be
> > +# fullscreened on that specific monitor or else it would not
> > be
> > +# displayed anywhere and would appear disconnected to the
> > guest.
>
> Let's see whether I understand this... We have VCs numbered 0, 1, ...
> VC #i is mapped to the i-th element of @connector, counting from zero.
> Correct?
[Kasireddy, Vivek] Yes, correct.
>
> Ignorant question: what's a "physical monitor/connector name"?
[Kasireddy, Vivek] IIUC, while the HDMI/DP protocols refer to a receiver (RX)
as a "sink", the DRM Graphics subsystem in the kernel uses the term "connector"
and the GTK library prefers the term "monitor". All of these terms can be
used interchangeably but I chose the term connector for the new parameter
after debating between connector, monitor, output, etc.
The connector name (e.g. DP-1, HDMI-A-2, etc) uniquely identifies a connector
or a monitor on any given system:
https://elixir.bootlin.com/linux/v6.0-rc6/source/include/drm/drm_connector.h#L1328
If you are on Intel hardware, you can find more info related to connectors by
doing:
cat /sys/kernel/debug/dri/0/i915_display_info
>
> Would you mind breaking the lines a bit earlier, between column 70 and
> 75?
[Kasireddy, Vivek] Np, will do that.
>
> > +# Since 7.2
> > #
> > # Since: 2.12
> > ##
> > { 'struct' : 'DisplayGTK',
> >'data': { '*grab-on-hover' : 'bool',
> > '*zoom-to-fit' : 'bool',
> > -'*show-tabs' : 'bool' } }
> > +'*show-tabs' : 'bool',
> > +'*connector' : ['str'] } }
> >
> > ##
> > # @DisplayEGLHeadless:
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 31c04f7eea..576b65ef9f 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
> > #if defined(CONFIG_GTK)
> > "-display
> > gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
> > "
> > [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
> > +"[,connector.=]\n"
>
> Is "" a VC number?
[Kasireddy, Vivek] Yes.
>
> > #endif
> > #if defined(CONFIG_VNC)
> > "-display vnc=[,]\n"
>
> Remainder of my review is on style only. Style suggestions are not
> demands :)
[Kasireddy, Vivek] No problem; most of your suggestions are sensible
and I'll include them all in v2.
>
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 945c550909..651aaaf174 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -37,6 +37,7 @@
> > #include "qapi/qapi-commands-misc.h"
> > #include "qemu/cutils.h"
> > #include "qemu/main-loop.h"
> > +#include "qemu/option.h"
> >
> > #include "ui/console.h"
> > #include "ui/gtk.h"
> > @@ -115,6 +116,7 @@
> > #endif
> >
> > #define HOTKEY_MODIFIERS(GDK_CONTROL_MASK | GDK_MOD1_MASK)
> > +#define MAX_NUM_ATTEMPTS 5
>
> Could use a comment, and maybe a more telling name (this one doesn't
> tell me what is being attempted).
[Kasireddy, Vivek] How about MAX_NUM_RETRIES?
gdk_monitor_get_model() can return NULL but if I wait for sometime
(few milliseconds) and try again it may give a valid value. So, I need a
macro to limit the number of attempts or retries.
>
> >
> > static const guint16 *keycode_map;
> > static size_t keycode_maplen;
> > @@ -126,6 +128,15 @@ struct VCChardev {
> > };
> > typedef struct VCChar
Re: [PATCH v2 26/39] tests/qtest: migration-test: Make sure QEMU process "to" exited after migration is canceled
Hi
On Tue, Sep 20, 2022 at 3:18 PM Bin Meng wrote:
> From: Xuzhou Cheng
>
> Make sure QEMU process "to" exited before launching another target
> for migration in the test_multifd_tcp_cancel case.
>
> Signed-off-by: Xuzhou Cheng
> Signed-off-by: Bin Meng
> Reviewed-by: Marc-André Lureau
>
fwiw, I didn't r-b the version with a busy wait
(
https://patchew.org/QEMU/20220824094029.1634519-1-bmeng...@gmail.com/20220824094029.1634519-42-bmeng...@gmail.com/
)
---
>
> Changes in v2:
> - Change to a busy wait after migration is canceled
>
> tests/qtest/migration-test.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index c87afad9e8..aedd9ddb72 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2133,6 +2133,10 @@ static void test_multifd_tcp_cancel(void)
> wait_for_migration_pass(from);
>
> migrate_cancel(from);
> +/* Make sure QEMU process "to" exited */
> +while (qtest_probe_child(to)) {
> +;
> +}
>
> args = (MigrateStart){
> .only_target = true,
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v8 1/8] mm/memfd: Introduce userspace inaccessible memfd
(please excuse any formatting disasters. my internet went out as I was composing this, and i did my best to rescue it.) On Mon, Sep 19, 2022, at 12:10 PM, Sean Christopherson wrote: > +Will, Marc and Fuad (apologies if I missed other pKVM folks) > > On Mon, Sep 19, 2022, David Hildenbrand wrote: >> On 15.09.22 16:29, Chao Peng wrote: >> > From: "Kirill A. Shutemov" >> > >> > KVM can use memfd-provided memory for guest memory. For normal userspace >> > accessible memory, KVM userspace (e.g. QEMU) mmaps the memfd into its >> > virtual address space and then tells KVM to use the virtual address to >> > setup the mapping in the secondary page table (e.g. EPT). >> > >> > With confidential computing technologies like Intel TDX, the >> > memfd-provided memory may be encrypted with special key for special >> > software domain (e.g. KVM guest) and is not expected to be directly >> > accessed by userspace. Precisely, userspace access to such encrypted >> > memory may lead to host crash so it should be prevented. >> >> Initially my thaught was that this whole inaccessible thing is TDX specific >> and there is no need to force that on other mechanisms. That's why I >> suggested to not expose this to user space but handle the notifier >> requirements internally. >> >> IIUC now, protected KVM has similar demands. Either access (read/write) of >> guest RAM would result in a fault and possibly crash the hypervisor (at >> least not the whole machine IIUC). > > Yep. The missing piece for pKVM is the ability to convert from shared > to private > while preserving the contents, e.g. to hand off a large buffer > (hundreds of MiB) > for processing in the protected VM. Thoughts on this at the bottom. > >> > This patch introduces userspace inaccessible memfd (created with >> > MFD_INACCESSIBLE). Its memory is inaccessible from userspace through >> > ordinary MMU access (e.g. read/write/mmap) but can be accessed via >> > in-kernel interface so KVM can directly interact with core-mm without >> > the need to map the memory into KVM userspace. >> >> With secretmem we decided to not add such "concept switch" flags and instead >> use a dedicated syscall. >> > > I have no personal preference whatsoever between a flag and a dedicated > syscall, > but a dedicated syscall does seem like it would give the kernel a bit more > flexibility. The third option is a device node, e.g. /dev/kvm_secretmem or /dev/kvm_tdxmem or similar. But if we need flags or other details in the future, maybe this isn't ideal. > >> What about memfd_inaccessible()? Especially, sealing and hugetlb are not >> even supported and it might take a while to support either. > > Don't know about sealing, but hugetlb support for "inaccessible" memory > needs to > come sooner than later. "inaccessible" in quotes because we might want > to choose > a less binary name, e.g. "restricted"?. > > Regarding pKVM's use case, with the shim approach I believe this can be done > by > allowing userspace mmap() the "hidden" memfd, but with a ton of restrictions > piled on top. > > My first thought was to make the uAPI a set of KVM ioctls so that KVM > could tightly > tightly control usage without taking on too much complexity in the > kernel, but > working through things, routing the behavior through the shim itself > might not be > all that horrific. > > IIRC, we discarded the idea of allowing userspace to map the "private" > fd because > things got too complex, but with the shim it doesn't seem _that_ bad. What's the exact use case? Is it just to pre-populate the memory? > > E.g. on the memfd side: > > 1. The entire memfd must be mapped, and at most one mapping is allowed, i.e. > mapping is all or nothing. > > 2. Acquiring a reference via get_pfn() is disallowed if there's a mapping > for > the restricted memfd. > > 3. Add notifier hooks to allow downstream users to further restrict things. > > 4. Disallow splitting VMAs, e.g. to force userspace to munmap() everything > in > one shot. > > 5. Require that there are no outstanding references at munmap(). Or if this > can't be guaranteed by userspace, maybe add some way for userspace to > wait > until it's ok to convert to private? E.g. so that get_pfn() doesn't need > to do an expensive check every time. Hmm. I haven't looked at the code to see if this would really work, but I think this could be done more in line with how the rest of the kernel works by using the rmap infrastructure. When the pKVM memfd is in not-yet-private mode, just let it be mmapped as usual (but don't allow any form of GUP or pinning). Then have an ioctl to switch to to shared mode that takes locks or sets flags so that no new faults can be serviced and does unmap_mapping_range. As long as the shim arranges to have its own vm_ops, I don't immediately see any reason this can't work.
Re: [PATCH RESEND] hw/microblaze: pass random seed to fdt
On Wed, Sep 21, 2022 at 12:32:37PM +0200, Jason A. Donenfeld wrote:
> On Thu, Sep 8, 2022 at 11:40 AM Jason A. Donenfeld wrote:
> >
> > If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
> > initialize early. Set this using the usual guest random number
> > generation function. This FDT node is part of the DT specification.
> >
> > Reviewed-by: Edgar E. Iglesias
> > Signed-off-by: Jason A. Donenfeld
> > ---
> > hw/microblaze/boot.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> > index 8b92a9801a..25ad54754e 100644
> > --- a/hw/microblaze/boot.c
> > +++ b/hw/microblaze/boot.c
> > @@ -30,6 +30,7 @@
> > #include "qemu/option.h"
> > #include "qemu/config-file.h"
> > #include "qemu/error-report.h"
> > +#include "qemu/guest-random.h"
> > #include "sysemu/device_tree.h"
> > #include "sysemu/reset.h"
> > #include "hw/boards.h"
> > @@ -75,6 +76,7 @@ static int microblaze_load_dtb(hwaddr addr,
> > int fdt_size;
> > void *fdt = NULL;
> > int r;
> > +uint8_t rng_seed[32];
> >
> > if (dtb_filename) {
> > fdt = load_device_tree(dtb_filename, &fdt_size);
> > @@ -83,6 +85,9 @@ static int microblaze_load_dtb(hwaddr addr,
> > return 0;
> > }
> >
> > +qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
> > +qemu_fdt_setprop(fdt, "/chosen", "rng-seed", rng_seed,
> > sizeof(rng_seed));
> > +
> > if (kernel_cmdline) {
> > r = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
> > kernel_cmdline);
> > --
> > 2.37.3
> >
>
> Bumping this patch. Could somebody queue this up?
Hi Jason,
I've just sent a pull-request with this patch.
Thanks,
Edgar
[PULL v1 0/1] Xilinx queue
From: "Edgar E. Iglesias" The following changes since commit 2906f933dd1de6d94c54881cc16ea7390a6ba300: Merge tag 'pull-request-2022-09-20' of https://gitlab.com/thuth/qemu into staging (2022-09-20 16:24:07 -0400) are available in the Git repository at: g...@github.com:edgarigl/qemu.git tags/edgar/xilinx-next-2022-09-21.for-upstream for you to fetch changes up to b91b6b5a2cd83a096116929dfc8e016091080adc: hw/microblaze: pass random seed to fdt (2022-09-21 19:59:56 +0200) Xilinx queue Jason A. Donenfeld (1): hw/microblaze: pass random seed to fdt hw/microblaze/boot.c | 5 + 1 file changed, 5 insertions(+) Jason A. Donenfeld (1): hw/microblaze: pass random seed to fdt hw/microblaze/boot.c | 5 + 1 file changed, 5 insertions(+) -- 2.25.1
[PULL v1 1/1] hw/microblaze: pass random seed to fdt
From: "Jason A. Donenfeld"
If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
initialize early. Set this using the usual guest random number
generation function. This FDT node is part of the DT specification.
Reviewed-by: Edgar E. Iglesias
Signed-off-by: Jason A. Donenfeld
Signed-off-by: Edgar E. Iglesias
---
hw/microblaze/boot.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 8b92a9801a..25ad54754e 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -30,6 +30,7 @@
#include "qemu/option.h"
#include "qemu/config-file.h"
#include "qemu/error-report.h"
+#include "qemu/guest-random.h"
#include "sysemu/device_tree.h"
#include "sysemu/reset.h"
#include "hw/boards.h"
@@ -75,6 +76,7 @@ static int microblaze_load_dtb(hwaddr addr,
int fdt_size;
void *fdt = NULL;
int r;
+uint8_t rng_seed[32];
if (dtb_filename) {
fdt = load_device_tree(dtb_filename, &fdt_size);
@@ -83,6 +85,9 @@ static int microblaze_load_dtb(hwaddr addr,
return 0;
}
+qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
+qemu_fdt_setprop(fdt, "/chosen", "rng-seed", rng_seed, sizeof(rng_seed));
+
if (kernel_cmdline) {
r = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
kernel_cmdline);
--
2.25.1
Re: [PATCH v4 2/7] accel/tcg: Use DisasContextBase in plugin_gen_tb_start
Richard Henderson writes: > Use the pc coming from db->pc_first rather than the TB. > > Use the cached host_addr rather than re-computing for the > first page. We still need a separate lookup for the second > page because it won't be computed for DisasContextBase until > the translator actually performs a read from the page. > > Signed-off-by: Richard Henderson Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PATCH v4 1/7] accel/tcg: Use bool for page_find_alloc
Richard Henderson writes: > Bool is more appropriate type for the alloc parameter. > > Signed-off-by: Richard Henderson Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PULL 00/17] ppc queue
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/7.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PULL 0/5] M68k for 7.2 patches
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/7.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Wed, Sep 21, 2022 at 07:23:12PM +0100, Alex Bennée wrote:
>
> chenh writes:
>
> > From: Hao Chen
> >
> > When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
> > start the virtual machine through libvirt or qemu, but now, the libvirt or
> > qemu can call dpdk vdpa vendor driver's ops .get_config through
> > vhost_net_get_config
> > to get the mac address of the vdpa hardware without manual configuration.
> >
> > v1->v2:
> > Only copy ETH_ALEN data of netcfg for some vdpa device such as
> > NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
> > We only need the mac address and don't care about the status field.
> >
> > Signed-off-by: Hao Chen
> > ---
> > hw/block/vhost-user-blk.c | 1 -
> > hw/net/virtio-net.c | 7 +++
> > hw/virtio/vhost-user.c| 19 ---
> > 3 files changed, 7 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 9117222456..5dca4eab09 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
> > Error **errp)
> >
> > vhost_dev_set_config_notifier(&s->dev, &blk_ops);
> >
> > -s->vhost_user.supports_config = true;
>
>
>
> NACK from me. The supports_config flag is there for a reason.
Alex please, do not send NACKs. If you feel compelled to stress
your point, provide extra justification instead. Thanks!
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index bd24741be8..8b01078249 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
> > *dev, void *opaque,
> > }
> >
> > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> > -bool supports_f_config = vus->supports_config ||
> > -(dev->config_ops &&
> > dev->config_ops->vhost_dev_config_notifier);
> > uint64_t protocol_features;
> >
> > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> > @@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
> > *dev, void *opaque,
> > */
> > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
> >
> > -if (supports_f_config) {
> > -if (!virtio_has_feature(protocol_features,
> > -VHOST_USER_PROTOCOL_F_CONFIG)) {
> > -error_setg(errp, "vhost-user device expecting "
> > - "VHOST_USER_PROTOCOL_F_CONFIG but the
> > vhost-user backend does "
> > - "not support it.");
> > -return -EPROTO;
> > -}
> > -} else {
> > -if (virtio_has_feature(protocol_features,
> > - VHOST_USER_PROTOCOL_F_CONFIG)) {
> > -warn_reportf_err(*errp, "vhost-user backend supports "
> > - "VHOST_USER_PROTOCOL_F_CONFIG but QEMU
> > does not.");
> > -protocol_features &= ~(1ULL <<
> > VHOST_USER_PROTOCOL_F_CONFIG);
> > -}
> > -}
> > -
> > /* final set of protocol features */
> > dev->protocol_features = protocol_features;
> > err = vhost_user_set_protocol_features(dev,
> > dev->protocol_features);
>
>
> --
> Alex Bennée
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
chenh writes:
> From: Hao Chen
>
> When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
> start the virtual machine through libvirt or qemu, but now, the libvirt or
> qemu can call dpdk vdpa vendor driver's ops .get_config through
> vhost_net_get_config
> to get the mac address of the vdpa hardware without manual configuration.
>
> v1->v2:
> Only copy ETH_ALEN data of netcfg for some vdpa device such as
> NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
> We only need the mac address and don't care about the status field.
>
> Signed-off-by: Hao Chen
> ---
> hw/block/vhost-user-blk.c | 1 -
> hw/net/virtio-net.c | 7 +++
> hw/virtio/vhost-user.c| 19 ---
> 3 files changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..5dca4eab09 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev, Error
> **errp)
>
> vhost_dev_set_config_notifier(&s->dev, &blk_ops);
>
> -s->vhost_user.supports_config = true;
NACK from me. The supports_config flag is there for a reason.
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index bd24741be8..8b01078249 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
> *dev, void *opaque,
> }
>
> if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> -bool supports_f_config = vus->supports_config ||
> -(dev->config_ops && dev->config_ops->vhost_dev_config_notifier);
> uint64_t protocol_features;
>
> dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> @@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
> *dev, void *opaque,
> */
> protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
>
> -if (supports_f_config) {
> -if (!virtio_has_feature(protocol_features,
> -VHOST_USER_PROTOCOL_F_CONFIG)) {
> -error_setg(errp, "vhost-user device expecting "
> - "VHOST_USER_PROTOCOL_F_CONFIG but the vhost-user
> backend does "
> - "not support it.");
> -return -EPROTO;
> -}
> -} else {
> -if (virtio_has_feature(protocol_features,
> - VHOST_USER_PROTOCOL_F_CONFIG)) {
> -warn_reportf_err(*errp, "vhost-user backend supports "
> - "VHOST_USER_PROTOCOL_F_CONFIG but QEMU does
> not.");
> -protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
> -}
> -}
> -
> /* final set of protocol features */
> dev->protocol_features = protocol_features;
> err = vhost_user_set_protocol_features(dev, dev->protocol_features);
--
Alex Bennée
Re: [PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers
On 9/21/22 09:14, Gerd Hoffmann wrote: Nope. Even if you fix the framebuffer address conflict you still have the io address conflict. Yeah, that is why I explicitly said that this is needed to be fixed as well in later patches. Yep. That's why isa-pc is pretty much unused these days. Well, I can't say I use it frequently, but I do test it with the SerenityOS kernel and it works pretty well. The SerenityOS kernel is able to drive an isa-vga device just fine after my patches were merged yesterday (in the GitHub pull request I provided a link to), so I do see that machine type as a valuable test platform. When you want build a sysbus variant of the bochs-display device and make that discoverable by the guest somehow (dt or acpi) you can expose both io ports and framebuffer address that way. No need to touch the bochs dispi interface for that. This is an interesting idea. A sysbus-bochs-display device might work well as you suggested, instead of using an isa-vga device. This idea is quite neat in my opinion, because it could speed up the boot of such VM while still providing sufficient display capabilities for those we need them. It could help developers to test their OSes on such hardware setups to ensure multi-monitor configuration works reliably when there's no PCI bus at all but many framebuffer devices being used in one VM. Why not just use virtio-gpu? Trying to run this command: qemu-system-x86_64 -M microvm -m 2048 -device virtio-gpu Results in: qemu-system-x86_64: -device virtio-gpu: No 'PCI' bus found for device 'virtio-gpu-pci' The idea was to not use PCI at all but still to have multiple framebuffer devices. So clearly virtio-gpu is not usable in this situation. 2. This is more related to the SerenityOS project - I prefer to not hardcode physical addresses at all wherever I can do so. This makes the code cleaner and more understandable as far as I observe this from the angle of the person which is not me, that tries to make sense from the code flow. Yea, fully agree, but why continue to use non-discoverable isa bus devices then? On the ISA-PC machine, I still want to be able to boot into a graphical environment with the SerenityOS kernel. The only option is to use the isa-vga device, which works OK. On the microvm machine, it is really not that important if I use the isa-vga device or a sysbus-bochs-display device (when I implement that device). I just want to support as many x86 platform configurations as possible - modern non-PCI machines, ISA-PC machines and regular i440fx/Q35 machines. 3. The costs of adding this feature are pretty negligible compared to the possible value of this, especially if we apply the idea of running multiple ISA-VGA devices on one microvm machine. Still, the only major "issue" that one can point to is the fact that I bump up the Bochs VBE version number, which could be questionable with how the feature might be insignificant for many guest OSes out there. Touching isa-vga at this point doesn't make sense at all. We simply can't move around the framebuffer without screwing up users. That's an issue I didn't consider, but this is not a real problem on the microvm machine if you use the device tree approach to expose the resources of the device, which in some sense makes it unnecessary to use the bochs dispi interface to expose the framebuffer physical address. Also I don't see any actual value in this. Even considering the multiple devices case the patch is a partial solution only (handles the framebuffer but not the ioports) which is pointless. Considering the possibility of exposing the framebuffer address within the device tree blob, it is indeed not making more value to go with this approach. I'm still fond of the idea to create a sysbus variant of the bochs-display device, so I might work on that instead :) Best regards, Liav
Re: [PULL 00/12] Publish1 patches
Hi Helge, On 20/9/22 19:31, Helge Deller wrote: The following changes since commit 621da7789083b80d6f1ff1c0fb499334007b4f51: Update version for v7.1.0 release (2022-08-30 09:40:11 -0700) are available in the Git repository at: https://github.com/hdeller/qemu-hppa.git tags/publish1-pull-request for you to fetch changes up to 7f8674a61a908592bb4e8e698f5bef84d0eeb8cc: linux-user: Add parameters of getrandom() syscall for strace (2022-09-18 21:35:27 +0200) linux-user: Add more syscalls, enhance tracing & logging enhancements Here is a bunch of patches for linux-user. Most of them add missing syscalls and enhance the tracing/logging. Some of the patches are target-hppa specific. I've tested those on productive hppa debian buildd servers (running qemu-user). Thanks! Helge Changes to v2: - Fix build of close_range() and pidfd_*() patches on older Linux distributions (noticed by Stefan Hajnoczi) Changes to v1: - Dropped the faccessat2() syscall patch in favour of Richard's patch - Various changes to the "missing signals in strace output" patch based on Richard's feedback, e.g. static arrays, fixed usage of _NSIG, fix build when TARGET_SIGIOT does not exist - Use FUTEX_CMD_MASK in "Show timespec on strace for futex" patch unconditionally and turn into a switch statement - as suggested by Richard Helge Deller (12): linux-user: Add missing signals in strace output linux-user: Add missing clock_gettime64() syscall strace linux-user: Add pidfd_open(), pidfd_send_signal() and pidfd_getfd() syscalls linux-user: Log failing executable in EXCP_DUMP() linux-user/hppa: Use EXCP_DUMP() to show enhanced debug info linux-user/hppa: Dump IIR on register dump linux-user: Fix strace of chmod() if mode == 0 linux-user/hppa: Set TASK_UNMAPPED_BASE to 0xfa00 for hppa arch linux-user: Add strace for clock_nanosleep() linux-user: Show timespec on strace for futex() linux-user: Add close_range() syscall linux-user: Add parameters of getrandom() syscall for strace It seems you missed my review comments: - https://lore.kernel.org/qemu-devel/569161db-c8cf-9ae5-9ae6-161de7f22...@amsat.org/ - https://lore.kernel.org/qemu-devel/d1668b24-9c04-0e54-2a82-7174f0d46...@amsat.org/ - https://lore.kernel.org/qemu-devel/e8bfd1ba-cec7-7c29-9319-eb013c14a...@amsat.org/#t - https://lore.kernel.org/qemu-devel/02090880-0db6-0a6b-60b0-b3313566b...@amsat.org/
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
If I read your response on the other thread correctly, this change is intended
to prioritize the MAC address exposed by DPDK over the one provided by the
QEMU command line? Sounds reasonable in principle, but I would get confirmation
from vDPA/vhost-net maintainers.
That said the way you’re hacking the vhost-user code breaks a valuable check for
bad vhost-user-blk backends. I would suggest finding another implementation
which
does not regress functionality for other device types.
>From: Hao Chen
>
>When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
>start the virtual machine through libvirt or qemu, but now, the libvirt or
>qemu can call dpdk vdpa vendor driver's ops .get_config through
>vhost_net_get_config
>to get the mac address of the vdpa hardware without manual configuration.
>
>v1->v2:
>Only copy ETH_ALEN data of netcfg for some vdpa device such as
>NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
>We only need the mac address and don't care about the status field.
>
>Signed-off-by: Hao Chen
>---
> hw/block/vhost-user-blk.c | 1 -
> hw/net/virtio-net.c | 7 +++
> hw/virtio/vhost-user.c| 19 ---
> 3 files changed, 7 insertions(+), 20 deletions(-)
>
>diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>index 9117222456..5dca4eab09 100644
>--- a/hw/block/vhost-user-blk.c
>+++ b/hw/block/vhost-user-blk.c
>@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev, Error
>**errp)
>
> vhost_dev_set_config_notifier(&s->dev, &blk_ops);
>
>-s->vhost_user.supports_config = true;
> ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
> errp);
> if (ret < 0) {
>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>index dd0d056fde..90405083b1 100644
>--- a/hw/net/virtio-net.c
>+++ b/hw/net/virtio-net.c
>@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice *vdev,
>uint8_t *config)
> }
> memcpy(config, &netcfg, n->config_size);
> }
>+} else if (nc->peer && nc->peer->info->type ==
>NET_CLIENT_DRIVER_VHOST_USER) {
>+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
>*)&netcfg,
>+ n->config_size);
>+if (ret != -1) {
>+ /* Automatically obtain the mac address of the vdpa device
>+* when using the dpdk vdpa */
>+memcpy(config, &netcfg, ETH_ALEN);
> }
> }
>
>diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>index bd24741be8..8b01078249 100644
>--- a/hw/virtio/vhost-user.c
>+++ b/hw/virtio/vhost-user.c
>@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
>*dev, void *opaque,
> }
>
> if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>-bool supports_f_config = vus->supports_config ||
>-(dev->config_ops && dev->config_ops->vhost_dev_config_notifier);
> uint64_t protocol_features;
>
> dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
>*dev, void *opaque,
> */
> protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
>
>-if (supports_f_config) {
>-if (!virtio_has_feature(protocol_features,
>-VHOST_USER_PROTOCOL_F_CONFIG)) {
>-error_setg(errp, "vhost-user device expecting "
>- "VHOST_USER_PROTOCOL_F_CONFIG but the vhost-user
>backend does "
>- "not support it.");
>-return -EPROTO;
>-}
>-} else {
>-if (virtio_has_feature(protocol_features,
>- VHOST_USER_PROTOCOL_F_CONFIG)) {
>-warn_reportf_err(*errp, "vhost-user backend supports "
>- "VHOST_USER_PROTOCOL_F_CONFIG but QEMU does
>not.");
>-protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
>-}
>-}
>-
> /* final set of protocol features */
> dev->protocol_features = protocol_features;
> err = vhost_user_set_protocol_features(dev, dev->protocol_features);
>--
>2.27.0
>
Re: [PATCH v2 02/23] target/i386: Return bool from disas_insn
On 8/9/22 14:14, Richard Henderson wrote:
On 9/6/22 15:42, Philippe Mathieu-Daudé wrote:
On 6/9/22 12:09, Richard Henderson wrote:
Instead of returning the new pc, which is present in
DisasContext, return true if an insn was translated.
This is false when we detect a page crossing and must
undo the insn under translation.
Signed-off-by: Richard Henderson
---
target/i386/tcg/translate.c | 42 +++--
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 1e24bb2985..46300ffd91 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -4665,7 +4665,7 @@ static void gen_sse(CPUX86State *env,
DisasContext *s, int b)
/* convert one instruction. s->base.is_jmp is set if the
translation must
be stopped. Return the next pc value */
-static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
+static bool disas_insn(DisasContext *s, CPUState *cpu)
{
CPUX86State *env = cpu->env_ptr;
int b, prefixes;
@@ -4695,12 +4695,13 @@ static target_ulong disas_insn(DisasContext
*s, CPUState *cpu)
return s->pc;
Shouldn't we return 'true' here?
Whoops, yes.
Returning 'true':
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v1 06/10] plugins: Assert mmu_idx in range before use in qemu_plugin_get_hwaddr
On 21/9/22 18:07, Alex Bennée wrote: From: Richard Henderson Coverity reports out-of-bound accesses here. This should be a false positive due to how the index is decoded from MemOpIdx. Fixes: Coverity CID 1487201 Signed-off-by: Richard Henderson Reviewed-by: Damien Hedde Message-Id: <20220401190233.329360-1-richard.hender...@linaro.org> Signed-off-by: Alex Bennée --- plugins/api.c | 2 ++ 1 file changed, 2 insertions(+) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v1 07/10] docs/devel: clean-up qemu invocations in tcg-plugins
On 21/9/22 18:07, Alex Bennée wrote: We currently have the final binaries in the root of the build dir so the build prefix is superfluous. Additionally add a shell prompt to be more in line with the rest of the code. Signed-off-by: Alex Bennée --- docs/devel/tcg-plugins.rst | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v1 08/10] docs/devel: move API to end of tcg-plugins.rst
On 21/9/22 18:07, Alex Bennée wrote: The API documentation is quite dry and doesn't flow nicely with the rest of the document. Move it to its own section at the bottom along with a little leader text to remind people to update it. Signed-off-by: Alex Bennée --- docs/devel/tcg-plugins.rst | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v1 09/10] contrib/plugins: reset skip when matching in execlog
On 21/9/22 18:08, Alex Bennée wrote: The purpose of the matches was to only track the execution of instructions we care about. Without resetting skip to the value at the start of the block we end up dumping all instructions after the match with the consequent load on the instrumentation. Signed-off-by: Alex Bennée Cc: Alexandre Iooss --- contrib/plugins/execlog.c | 3 +++ 1 file changed, 3 insertions(+) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v1 03/10] disas: use result of ->read_memory_func
On 21/9/22 18:07, Alex Bennée wrote: This gets especially confusing if you start plugging in host addresses from a trace and you wonder why the output keeps changing. Report when read_memory_func fails instead of blindly disassembling the buffer contents. Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson --- disas.c | 20 ++--- disas/capstone.c | 73 2 files changed, 53 insertions(+), 40 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PULL 00/15] Testing, qga and misc patches
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/7.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH v1 01/10] monitor: expose monitor_puts to rest of code
On 21/9/22 18:07, Alex Bennée wrote: This helps us construct strings elsewhere before echoing to the monitor. It avoids having to jump through hoops like: monitor_printf(mon, "%s", s->str); It will be useful in following patches but for now convert all existing plain "%s" printfs to use the _puts api. Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson --- v2 - s/monitor_printf(mon, "%s"/monitor_puts(mon, / --- docs/devel/writing-monitor-commands.rst | 2 +- include/monitor/monitor.h | 1 + monitor/monitor-internal.h | 1 - block/monitor/block-hmp-cmds.c | 10 +- hw/misc/mos6522.c | 2 +- monitor/hmp-cmds.c | 8 monitor/hmp.c | 2 +- target/i386/helper.c| 2 +- 8 files changed, 14 insertions(+), 14 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PULL 00/21] Misc patches for 2022-09-19
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/7.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH v2 35/39] tests/qtest: migration-test: Skip running some TLS cases for win32
On Wed, Sep 21, 2022 at 05:51:33PM +0100, Dr. David Alan Gilbert wrote:
> * Bin Meng (bmeng...@gmail.com) wrote:
> > From: Bin Meng
> >
> > Some migration test cases use TLS to communicate, but they fail on
> > Windows with the following error messages:
> >
> > qemu-system-x86_64: TLS handshake failed: Insufficient credentials for
> > that request.
> > qemu-system-x86_64: TLS handshake failed: Error in the pull function.
> > query-migrate shows failed migration: TLS handshake failed: Error in the
> > pull function.
> >
> > Disable them temporarily.
> >
> > Signed-off-by: Bin Meng
> > ---
> > I am not familar with the gnutls and simply enabling the gnutls debug
> > output does not give me an immedidate hint on why it's failing on
> > Windows. Disable these cases for now until someone or maintainers
> > who may want to test this on Windows.
>
> Copying in Dan Berrange, he's our expert on weird TLS failures.
Seems to match this:
https://gnutls.org/faq.html#key-usage-violation2
which suggests we have a configuration mis-match.
I'm surprised to see you are only needing to disable the TLS PSK tests,
not the TLS x509 tests.
I'd like to know if tests/unit/test-crypto-tlssession passes.
If so, it might suggest we are missing 'priority: NORMAL' property
when configuring TLS creds for the migration test.
> > (no changes since v1)
> >
> > tests/qtest/migration-test.c | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index aedd9ddb72..dbee9b528a 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -1403,6 +1403,7 @@ static void test_precopy_unix_dirty_ring(void)
> > }
> >
> > #ifdef CONFIG_GNUTLS
> > +#ifndef _WIN32
> > static void test_precopy_unix_tls_psk(void)
> > {
> > g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> > @@ -1415,6 +1416,7 @@ static void test_precopy_unix_tls_psk(void)
> >
> > test_precopy_common(&args);
> > }
> > +#endif /* _WIN32 */
> >
> > #ifdef CONFIG_TASN1
> > static void test_precopy_unix_tls_x509_default_host(void)
> > @@ -1523,6 +1525,7 @@ static void test_precopy_tcp_plain(void)
> > }
> >
> > #ifdef CONFIG_GNUTLS
> > +#ifndef _WIN32
> > static void test_precopy_tcp_tls_psk_match(void)
> > {
> > MigrateCommon args = {
> > @@ -1533,6 +1536,7 @@ static void test_precopy_tcp_tls_psk_match(void)
> >
> > test_precopy_common(&args);
> > }
> > +#endif /* _WIN32 */
> >
> > static void test_precopy_tcp_tls_psk_mismatch(void)
> > {
> > @@ -1930,6 +1934,7 @@ static void test_multifd_tcp_zstd(void)
> > #endif
> >
> > #ifdef CONFIG_GNUTLS
> > +#ifndef _WIN32
> > static void *
> > test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
> > QTestState *to)
> > @@ -1937,6 +1942,7 @@
> > test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
> > test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
> > return test_migrate_tls_psk_start_match(from, to);
> > }
> > +#endif /* _WIN32 */
> >
> > static void *
> > test_migrate_multifd_tcp_tls_psk_start_mismatch(QTestState *from,
> > @@ -1988,6 +1994,7 @@
> > test_migrate_multifd_tls_x509_start_reject_anon_client(QTestState *from,
> > }
> > #endif /* CONFIG_TASN1 */
> >
> > +#ifndef _WIN32
> > static void test_multifd_tcp_tls_psk_match(void)
> > {
> > MigrateCommon args = {
> > @@ -1997,6 +2004,7 @@ static void test_multifd_tcp_tls_psk_match(void)
> > };
> > test_precopy_common(&args);
> > }
> > +#endif /* _WIN32 */
> >
> > static void test_multifd_tcp_tls_psk_mismatch(void)
> > {
> > @@ -2497,8 +2505,10 @@ int main(int argc, char **argv)
> > qtest_add_func("/migration/precopy/unix/plain",
> > test_precopy_unix_plain);
> > qtest_add_func("/migration/precopy/unix/xbzrle",
> > test_precopy_unix_xbzrle);
> > #ifdef CONFIG_GNUTLS
> > +#ifndef _WIN32
> > qtest_add_func("/migration/precopy/unix/tls/psk",
> > test_precopy_unix_tls_psk);
> > +#endif
> >
> > if (has_uffd) {
> > /*
> > @@ -2524,8 +2534,10 @@ int main(int argc, char **argv)
> >
> > qtest_add_func("/migration/precopy/tcp/plain", test_precopy_tcp_plain);
> > #ifdef CONFIG_GNUTLS
> > +#ifndef _WIN32
> > qtest_add_func("/migration/precopy/tcp/tls/psk/match",
> > test_precopy_tcp_tls_psk_match);
> > +#endif
> > qtest_add_func("/migration/precopy/tcp/tls/psk/mismatch",
> > test_precopy_tcp_tls_psk_mismatch);
> > #ifdef CONFIG_TASN1
> > @@ -2569,8 +2581,10 @@ int main(int argc, char **argv)
> > test_multifd_tcp_zstd);
> > #endif
> > #ifdef CONFIG_GNUTLS
> > +#ifndef _WIN32
> > qtest_add_func("/migration/multifd/tcp/tls/psk/match",
> > test_multifd_tcp_tls_psk_match);
> > +#endif
> > qtest_add_func("/migration/mul
Re: [PATCH v2 35/39] tests/qtest: migration-test: Skip running some TLS cases for win32
* Bin Meng (bmeng...@gmail.com) wrote:
> From: Bin Meng
>
> Some migration test cases use TLS to communicate, but they fail on
> Windows with the following error messages:
>
> qemu-system-x86_64: TLS handshake failed: Insufficient credentials for that
> request.
> qemu-system-x86_64: TLS handshake failed: Error in the pull function.
> query-migrate shows failed migration: TLS handshake failed: Error in the
> pull function.
>
> Disable them temporarily.
>
> Signed-off-by: Bin Meng
> ---
> I am not familar with the gnutls and simply enabling the gnutls debug
> output does not give me an immedidate hint on why it's failing on
> Windows. Disable these cases for now until someone or maintainers
> who may want to test this on Windows.
Copying in Dan Berrange, he's our expert on weird TLS failures.
Dave
>
> (no changes since v1)
>
> tests/qtest/migration-test.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index aedd9ddb72..dbee9b528a 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1403,6 +1403,7 @@ static void test_precopy_unix_dirty_ring(void)
> }
>
> #ifdef CONFIG_GNUTLS
> +#ifndef _WIN32
> static void test_precopy_unix_tls_psk(void)
> {
> g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> @@ -1415,6 +1416,7 @@ static void test_precopy_unix_tls_psk(void)
>
> test_precopy_common(&args);
> }
> +#endif /* _WIN32 */
>
> #ifdef CONFIG_TASN1
> static void test_precopy_unix_tls_x509_default_host(void)
> @@ -1523,6 +1525,7 @@ static void test_precopy_tcp_plain(void)
> }
>
> #ifdef CONFIG_GNUTLS
> +#ifndef _WIN32
> static void test_precopy_tcp_tls_psk_match(void)
> {
> MigrateCommon args = {
> @@ -1533,6 +1536,7 @@ static void test_precopy_tcp_tls_psk_match(void)
>
> test_precopy_common(&args);
> }
> +#endif /* _WIN32 */
>
> static void test_precopy_tcp_tls_psk_mismatch(void)
> {
> @@ -1930,6 +1934,7 @@ static void test_multifd_tcp_zstd(void)
> #endif
>
> #ifdef CONFIG_GNUTLS
> +#ifndef _WIN32
> static void *
> test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
> QTestState *to)
> @@ -1937,6 +1942,7 @@ test_migrate_multifd_tcp_tls_psk_start_match(QTestState
> *from,
> test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
> return test_migrate_tls_psk_start_match(from, to);
> }
> +#endif /* _WIN32 */
>
> static void *
> test_migrate_multifd_tcp_tls_psk_start_mismatch(QTestState *from,
> @@ -1988,6 +1994,7 @@
> test_migrate_multifd_tls_x509_start_reject_anon_client(QTestState *from,
> }
> #endif /* CONFIG_TASN1 */
>
> +#ifndef _WIN32
> static void test_multifd_tcp_tls_psk_match(void)
> {
> MigrateCommon args = {
> @@ -1997,6 +2004,7 @@ static void test_multifd_tcp_tls_psk_match(void)
> };
> test_precopy_common(&args);
> }
> +#endif /* _WIN32 */
>
> static void test_multifd_tcp_tls_psk_mismatch(void)
> {
> @@ -2497,8 +2505,10 @@ int main(int argc, char **argv)
> qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
> qtest_add_func("/migration/precopy/unix/xbzrle",
> test_precopy_unix_xbzrle);
> #ifdef CONFIG_GNUTLS
> +#ifndef _WIN32
> qtest_add_func("/migration/precopy/unix/tls/psk",
> test_precopy_unix_tls_psk);
> +#endif
>
> if (has_uffd) {
> /*
> @@ -2524,8 +2534,10 @@ int main(int argc, char **argv)
>
> qtest_add_func("/migration/precopy/tcp/plain", test_precopy_tcp_plain);
> #ifdef CONFIG_GNUTLS
> +#ifndef _WIN32
> qtest_add_func("/migration/precopy/tcp/tls/psk/match",
> test_precopy_tcp_tls_psk_match);
> +#endif
> qtest_add_func("/migration/precopy/tcp/tls/psk/mismatch",
> test_precopy_tcp_tls_psk_mismatch);
> #ifdef CONFIG_TASN1
> @@ -2569,8 +2581,10 @@ int main(int argc, char **argv)
> test_multifd_tcp_zstd);
> #endif
> #ifdef CONFIG_GNUTLS
> +#ifndef _WIN32
> qtest_add_func("/migration/multifd/tcp/tls/psk/match",
> test_multifd_tcp_tls_psk_match);
> +#endif
> qtest_add_func("/migration/multifd/tcp/tls/psk/mismatch",
> test_multifd_tcp_tls_psk_mismatch);
> #ifdef CONFIG_TASN1
> --
> 2.34.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2 26/39] tests/qtest: migration-test: Make sure QEMU process "to" exited after migration is canceled
On Wed, Sep 21, 2022 at 05:29:55PM +0100, Dr. David Alan Gilbert wrote: > * Bin Meng (bmeng...@gmail.com) wrote: > > From: Xuzhou Cheng > > > > Make sure QEMU process "to" exited before launching another target > > for migration in the test_multifd_tcp_cancel case. > > > > Signed-off-by: Xuzhou Cheng > > Signed-off-by: Bin Meng > > Reviewed-by: Marc-André Lureau > > Hmm you might want to put a small usleep in that loop; otherwise > it'll burn CPU. > > There is a slim risk with this that another, entirely unrelated, process > will start up with the same PID between the end of migrate_cancel > and then you'll be spinning on it rather than the 'to' qemu. > > I wonder if there's a better way to check for it dieing; e.g. an error > on it's qmp interface or something? Both the qtest and qmp sockets should give EOF. So if there's an API that can call g_poll() on the FD with POLL_HUP event, it would be the reliable way to detect it, without busy-looping. 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 v1 09/10] contrib/plugins: reset skip when matching in execlog
The purpose of the matches was to only track the execution of instructions we care about. Without resetting skip to the value at the start of the block we end up dumping all instructions after the match with the consequent load on the instrumentation. Signed-off-by: Alex Bennée Cc: Alexandre Iooss --- contrib/plugins/execlog.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c index e659ac9cbb..b5360f2c8e 100644 --- a/contrib/plugins/execlog.c +++ b/contrib/plugins/execlog.c @@ -147,6 +147,9 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) /* Register callback on instruction */ qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec, QEMU_PLUGIN_CB_NO_REGS, output); + +/* reset skip */ +skip = (imatches || amatches) ? true : false; } } -- 2.34.1
Re: [PATCH v2 26/39] tests/qtest: migration-test: Make sure QEMU process "to" exited after migration is canceled
* Bin Meng (bmeng...@gmail.com) wrote:
> From: Xuzhou Cheng
>
> Make sure QEMU process "to" exited before launching another target
> for migration in the test_multifd_tcp_cancel case.
>
> Signed-off-by: Xuzhou Cheng
> Signed-off-by: Bin Meng
> Reviewed-by: Marc-André Lureau
Hmm you might want to put a small usleep in that loop; otherwise
it'll burn CPU.
There is a slim risk with this that another, entirely unrelated, process
will start up with the same PID between the end of migrate_cancel
and then you'll be spinning on it rather than the 'to' qemu.
I wonder if there's a better way to check for it dieing; e.g. an error
on it's qmp interface or something?
Dave
> ---
>
> Changes in v2:
> - Change to a busy wait after migration is canceled
>
> tests/qtest/migration-test.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index c87afad9e8..aedd9ddb72 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2133,6 +2133,10 @@ static void test_multifd_tcp_cancel(void)
> wait_for_migration_pass(from);
>
> migrate_cancel(from);
> +/* Make sure QEMU process "to" exited */
> +while (qtest_probe_child(to)) {
> +;
> +}
>
> args = (MigrateStart){
> .only_target = true,
> --
> 2.34.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH] Revert "intel_iommu: Fix irqchip / X2APIC configuration checks"
It's true that when vcpus<=255 we don't require the length of 32bit APIC
IDs. However here since we already have EIM=ON it means the hypervisor
will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
even if vcpus<=255. In short, commit 77250171bdc breaks any simple cmdline
that wants to boot a VM with >=9 but <=255 vcpus with:
-device intel-iommu,intremap=on
For anyone who does not want to enable x2apic, we can use eim=off in the
intel-iommu parameters to skip enabling KVM x2apic.
This partly reverts commit 77250171bdc02aee106083fd2a068147befa1a38, while
keeping the valid bit on checking split irqchip, but revert the other change.
Cc: David Woodhouse
Cc: Claudio Fontana
Cc: Igor Mammedov
Signed-off-by: Peter Xu
---
hw/i386/intel_iommu.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 05d53a1aa9..6524c2ee32 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3818,6 +3818,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error
**errp)
error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
return false;
}
+if (!kvm_enable_x2apic()) {
+error_setg(errp, "eim=on requires support on the KVM side"
+ "(X2APIC_API, first shipped in v4.7)");
+return false;
+}
}
/* Currently only address widths supported are 39 and 48 bits */
--
2.32.0
[PATCH v1 10/10] docs/devel: document the test plugins
Although the test plugins are fairly basic they are still useful for some things so we should document their existence. Signed-off-by: Alex Bennée --- docs/devel/tcg-plugins.rst | 137 +++-- 1 file changed, 133 insertions(+), 4 deletions(-) diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst index 8b40b2a606..9740a70406 100644 --- a/docs/devel/tcg-plugins.rst +++ b/docs/devel/tcg-plugins.rst @@ -145,12 +145,141 @@ Example Plugins There are a number of plugins included with QEMU and you are encouraged to contribute your own plugins plugins upstream. There is a -``contrib/plugins`` directory where they can go. +``contrib/plugins`` directory where they can go. There are also some +basic plugins that are used to test and exercise the API during the +``make check-tcg`` target in ``tests\plugins``. -- tests/plugins +- tests/plugins/empty.c -These are some basic plugins that are used to test and exercise the -API during the ``make check-tcg`` target. +Purely a test plugin for measuring the overhead of the plugins system +itself. Does no instrumentation. + +- tests/plugins/bb.c + +A very basic plugin which will measure execution in course terms as +each basic block is executed. By default the results are shown once +execution finishes:: + + $ qemu-aarch64 -plugin tests/plugin/libbb.so \ + -d plugin ./tests/tcg/aarch64-linux-user/sha1 + SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6 + bb's: 2277338, insns: 158483046 + +Behaviour can be tweaked with the following arguments: + + * inline=true|false + + Use faster inline addition of a single counter. Not per-cpu and not + thread safe. + + * idle=true|false + + Dump the current execution stats whenever the guest vCPU idles + +- tests/plugins/insn.c + +This is a basic instruction level instrumentation which can count the +number of instructions executed on each core/thread:: + + $ qemu-aarch64 -plugin tests/plugin/libinsn.so \ + -d plugin ./tests/tcg/aarch64-linux-user/threadcount + Created 10 threads + Done + cpu 0 insns: 46765 + cpu 1 insns: 3694 + cpu 2 insns: 3694 + cpu 3 insns: 2994 + cpu 4 insns: 1497 + cpu 5 insns: 1497 + cpu 6 insns: 1497 + cpu 7 insns: 1497 + total insns: 63135 + +Behaviour can be tweaked with the following arguments: + + * inline=true|false + + Use faster inline addition of a single counter. Not per-cpu and not + thread safe. + + * sizes=true|false + + Give a summary of the instruction sizes for the execution + + * match= + + Only instrument instructions matching the string prefix. Will show + some basic stats including how many instructions have executed since + the last execution. For example:: + + $ qemu-aarch64 -plugin tests/plugin/libinsn.so,match=bl \ + -d plugin ./tests/tcg/aarch64-linux-user/sha512-vector + ... + 0x40069c, 'bl #0x4002b0', 10 hits, 1093 match hits, Δ+1257 since last match, 98 avg insns/match + 0x4006ac, 'bl #0x403690', 10 hits, 1094 match hits, Δ+47 since last match, 98 avg insns/match + 0x4037fc, 'bl #0x4002b0', 18 hits, 1095 match hits, Δ+22 since last match, 98 avg insns/match + 0x400720, 'bl #0x403690', 10 hits, 1096 match hits, Δ+58 since last match, 98 avg insns/match + 0x4037fc, 'bl #0x4002b0', 19 hits, 1097 match hits, Δ+22 since last match, 98 avg insns/match + 0x400730, 'bl #0x403690', 10 hits, 1098 match hits, Δ+33 since last match, 98 avg insns/match + 0x4037ac, 'bl #0x4002b0', 12 hits, 1099 match hits, Δ+20 since last match, 98 avg insns/match + ... + +For more detailed execution tracing see the ``execlog`` plugin for +other options. + +- tests/plugins/mem.c + +Basic instruction level memory instrumentation:: + + $ qemu-aarch64 -plugin tests/plugin/libmem.so,inline=true \ + -d plugin ./tests/tcg/aarch64-linux-user/sha1 + SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6 + inline mem accesses: 79525013 + +Behaviour can be tweaked with the following arguments: + + * inline=true|false + + Use faster inline addition of a single counter. Not per-cpu and not + thread safe. + + * callback=true|false + + Use callbacks on each memory instrumentation. + + * hwaddr=true|false + + Count IO accesses (only for system emulation) + +- tests/plugins/syscall.c + +A basic syscall tracing plugin. This only works for user-mode. By +default it will give a summary of syscall stats at the end of the +run:: + + $ qemu-aarch64 -plugin tests/plugin/libsyscall \ + -d plugin ./tests/tcg/aarch64-linux-user/threadcount + Created 10 threads + Done + syscall no. calls errors + 226 12 0 + 99 11 11 + 115 11 0 + 222 11 0 + 93 10 0 + 220 10 0 + 233 10 0 + 215 8 0 + 214 4 0 + 134 2 0 + 64 2 0 + 96 1 0 + 94 1 0 + 80 1 0 + 261 1 0 + 78 1 0 + 160 1 0 + 135
[PATCH v1 07/10] docs/devel: clean-up qemu invocations in tcg-plugins
We currently have the final binaries in the root of the build dir so the build prefix is superfluous. Additionally add a shell prompt to be more in line with the rest of the code. Signed-off-by: Alex Bennée --- docs/devel/tcg-plugins.rst | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst index a503d44cee..a6fdde01f8 100644 --- a/docs/devel/tcg-plugins.rst +++ b/docs/devel/tcg-plugins.rst @@ -172,7 +172,7 @@ slightly faster (but not thread safe) counters. Example:: - ./aarch64-linux-user/qemu-aarch64 \ + $ qemu-aarch64 \ -plugin contrib/plugins/libhotblocks.so -d plugin \ ./tests/tcg/aarch64-linux-user/sha1 SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6 @@ -186,7 +186,7 @@ Example:: Similar to hotblocks but this time tracks memory accesses:: - ./aarch64-linux-user/qemu-aarch64 \ + $ qemu-aarch64 \ -plugin contrib/plugins/libhotpages.so -d plugin \ ./tests/tcg/aarch64-linux-user/sha1 SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6 @@ -220,7 +220,7 @@ counted. You can give a value to the ``count`` argument for a class of instructions to break it down fully, so for example to see all the system registers accesses:: - ./aarch64-softmmu/qemu-system-aarch64 $(QEMU_ARGS) \ + $ qemu-system-aarch64 $(QEMU_ARGS) \ -append "root=/dev/sda2 systemd.unit=benchmark.service" \ -smp 4 -plugin ./contrib/plugins/libhowvec.so,count=sreg -d plugin @@ -288,10 +288,10 @@ for the plugin is a path for the socket the two instances will communicate over:: - ./sparc-softmmu/qemu-system-sparc -monitor none -parallel none \ + $ qemu-system-sparc -monitor none -parallel none \ -net none -M SS-20 -m 256 -kernel day11/zImage.elf \ -plugin ./contrib/plugins/liblockstep.so,sockpath=lockstep-sparc.sock \ - -d plugin,nochain +-d plugin,nochain which will eventually report:: @@ -348,7 +348,7 @@ Please be aware that this will generate a lot of output. The plugin needs default argument:: - qemu-system-arm $(QEMU_ARGS) \ + $ qemu-system-arm $(QEMU_ARGS) \ -plugin ./contrib/plugins/libexeclog.so -d plugin which will output an execution trace following this structure:: @@ -365,10 +365,10 @@ which will output an execution trace following this structure:: 0, 0x10c8, 0xfff96c43, "ldr r3, [r0, #0x44]", load, 0x20e4, RAM the output can be filtered to only track certain instructions or -addresses using the `ifilter` or `afilter` options. You can stack the +addresses using the ``ifilter`` or ``afilter`` options. You can stack the arguments if required:: - qemu-system-arm $(QEMU_ARGS) \ + $ qemu-system-arm $(QEMU_ARGS) \ -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin - contrib/plugins/cache.c @@ -377,7 +377,7 @@ Cache modelling plugin that measures the performance of a given L1 cache configuration, and optionally a unified L2 per-core cache when a given working set is run:: -qemu-x86_64 -plugin ./contrib/plugins/libcache.so \ + $ qemu-x86_64 -plugin ./contrib/plugins/libcache.so \ -d plugin -D cache.log ./tests/tcg/x86_64-linux-user/float_convs will report the following:: -- 2.34.1
[PATCH v1 06/10] plugins: Assert mmu_idx in range before use in qemu_plugin_get_hwaddr
From: Richard Henderson
Coverity reports out-of-bound accesses here. This should be a
false positive due to how the index is decoded from MemOpIdx.
Fixes: Coverity CID 1487201
Signed-off-by: Richard Henderson
Reviewed-by: Damien Hedde
Message-Id: <20220401190233.329360-1-richard.hender...@linaro.org>
Signed-off-by: Alex Bennée
---
plugins/api.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/plugins/api.c b/plugins/api.c
index 7bf71b189d..2078b16edb 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -289,6 +289,8 @@ struct qemu_plugin_hwaddr
*qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
enum qemu_plugin_mem_rw rw = get_plugin_meminfo_rw(info);
hwaddr_info.is_store = (rw & QEMU_PLUGIN_MEM_W) != 0;
+assert(mmu_idx < NB_MMU_MODES);
+
if (!tlb_plugin_lookup(cpu, vaddr, mmu_idx,
hwaddr_info.is_store, &hwaddr_info)) {
error_report("invalid use of qemu_plugin_get_hwaddr");
--
2.34.1
[PATCH v1 05/10] plugins: extend execlog to filter matches
Sometimes the whole execlog is just two much so add the ability to
filter by instruction opcode or address.
[AJB: this shows for example
.qemu-system-aarch64 -display none -serial mon:stdio \
-M virt -cpu max \
-semihosting-config enable=on \
-kernel ./tests/tcg/aarch64-softmmu/memory-sve \
-plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d
plugin -D plugin.out
the st1w SVE instruction is not instrumenting its stores.]
Signed-off-by: Alex Bennée
Reviewed-by: Alexandre Iooss
Cc: Robert Henry
Cc: Aaron Lindsay
---
docs/devel/tcg-plugins.rst | 9 +++-
contrib/plugins/execlog.c | 96 --
2 files changed, 90 insertions(+), 15 deletions(-)
diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index a7cc44aa20..a503d44cee 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -346,7 +346,7 @@ The execlog tool traces executed instructions with memory
access. It can be used
for debugging and security analysis purposes.
Please be aware that this will generate a lot of output.
-The plugin takes no argument::
+The plugin needs default argument::
qemu-system-arm $(QEMU_ARGS) \
-plugin ./contrib/plugins/libexeclog.so -d plugin
@@ -364,6 +364,13 @@ which will output an execution trace following this
structure::
0, 0xd34, 0xf9c8f000, "bl #0x10c8"
0, 0x10c8, 0xfff96c43, "ldr r3, [r0, #0x44]", load, 0x20e4, RAM
+the output can be filtered to only track certain instructions or
+addresses using the `ifilter` or `afilter` options. You can stack the
+arguments if required::
+
+ qemu-system-arm $(QEMU_ARGS) \
+-plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d
plugin
+
- contrib/plugins/cache.c
Cache modelling plugin that measures the performance of a given L1 cache
diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
index a5275dcc15..e659ac9cbb 100644
--- a/contrib/plugins/execlog.c
+++ b/contrib/plugins/execlog.c
@@ -20,6 +20,9 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_version =
QEMU_PLUGIN_VERSION;
/* Store last executed instruction on each vCPU as a GString */
GArray *last_exec;
+static GPtrArray *imatches;
+static GArray *amatches;
+
/**
* Add memory read or write information to current instruction log
*/
@@ -85,12 +88,13 @@ static void vcpu_insn_exec(unsigned int cpu_index, void
*udata)
static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
{
struct qemu_plugin_insn *insn;
-uint64_t insn_vaddr;
-uint32_t insn_opcode;
-char *insn_disas;
+bool skip = (imatches || amatches) ? true : false;
size_t n = qemu_plugin_tb_n_insns(tb);
for (size_t i = 0; i < n; i++) {
+char *insn_disas;
+uint64_t insn_vaddr;
+
/*
* `insn` is shared between translations in QEMU, copy needed data
here.
* `output` is never freed as it might be used multiple times during
@@ -99,20 +103,52 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct
qemu_plugin_tb *tb)
* a limitation for CISC architectures.
*/
insn = qemu_plugin_tb_get_insn(tb, i);
-insn_vaddr = qemu_plugin_insn_vaddr(insn);
-insn_opcode = *((uint32_t *)qemu_plugin_insn_data(insn));
insn_disas = qemu_plugin_insn_disas(insn);
-char *output = g_strdup_printf("0x%"PRIx64", 0x%"PRIx32", \"%s\"",
- insn_vaddr, insn_opcode, insn_disas);
+insn_vaddr = qemu_plugin_insn_vaddr(insn);
+
+/*
+ * If we are filtering we better check out if we have any
+ * hits. The skip "latches" so we can track memory accesses
+ * after the instruction we care about.
+ */
+if (skip && imatches) {
+int j;
+for (j = 0; j < imatches->len && skip; j++) {
+char *m = g_ptr_array_index(imatches, j);
+if (g_str_has_prefix(insn_disas, m)) {
+skip = false;
+}
+}
+}
+
+if (skip && amatches) {
+int j;
+for (j = 0; j < amatches->len && skip; j++) {
+uint64_t v = g_array_index(amatches, uint64_t, j);
+if (v == insn_vaddr) {
+skip = false;
+}
+}
+}
-/* Register callback on memory read or write */
-qemu_plugin_register_vcpu_mem_cb(insn, vcpu_mem,
- QEMU_PLUGIN_CB_NO_REGS,
- QEMU_PLUGIN_MEM_RW, NULL);
+if (skip) {
+g_free(insn_disas);
+} else {
+uint32_t insn_opcode;
+insn_opcode = *((uint32_t *)qemu_plugin_insn_data(insn));
+char *output = g_strdup_printf("0x%"PRIx64", 0x%"PRIx32", \"%s\"",
+ insn_vaddr, insn_opcode,
insn_disas);
+
+/* Register
[PATCH v1 02/10] disas: generalise plugin_printf and use for monitor_disas
Rather than assembling our output piecemeal lets use the same approach
as the plugin disas interface to build the disassembly string before
printing it.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
---
disas.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/disas.c b/disas.c
index e31438f349..f07b6e760b 100644
--- a/disas.c
+++ b/disas.c
@@ -239,7 +239,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong
code,
}
}
-static int plugin_printf(FILE *stream, const char *fmt, ...)
+static int gstring_printf(FILE *stream, const char *fmt, ...)
{
/* We abuse the FILE parameter to pass a GString. */
GString *s = (GString *)stream;
@@ -270,7 +270,7 @@ char *plugin_disas(CPUState *cpu, uint64_t addr, size_t
size)
GString *ds = g_string_new(NULL);
initialize_debug_target(&s, cpu);
-s.info.fprintf_func = plugin_printf;
+s.info.fprintf_func = gstring_printf;
s.info.stream = (FILE *)ds; /* abuse this slot */
s.info.buffer_vma = addr;
s.info.buffer_length = size;
@@ -358,15 +358,19 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
{
int count, i;
CPUDebug s;
+g_autoptr(GString) ds = g_string_new("");
initialize_debug_target(&s, cpu);
-s.info.fprintf_func = qemu_fprintf;
+s.info.fprintf_func = gstring_printf;
+s.info.stream = (FILE *)ds; /* abuse this slot */
+
if (is_physical) {
s.info.read_memory_func = physical_read_memory;
}
s.info.buffer_vma = pc;
if (s.info.cap_arch >= 0 && cap_disas_monitor(&s.info, pc, nb_insn)) {
+monitor_puts(mon, ds->str);
return;
}
@@ -376,13 +380,16 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
return;
}
-for(i = 0; i < nb_insn; i++) {
- monitor_printf(mon, "0x" TARGET_FMT_lx ": ", pc);
+for (i = 0; i < nb_insn; i++) {
+g_string_append_printf(ds, "0x" TARGET_FMT_lx ": ", pc);
count = s.info.print_insn(pc, &s.info);
- monitor_printf(mon, "\n");
- if (count < 0)
- break;
+g_string_append_c(ds, '\n');
+if (count < 0) {
+break;
+}
pc += count;
}
+
+monitor_puts(mon, ds->str);
}
#endif
--
2.34.1
[PATCH v1 08/10] docs/devel: move API to end of tcg-plugins.rst
The API documentation is quite dry and doesn't flow nicely with the rest of the document. Move it to its own section at the bottom along with a little leader text to remind people to update it. Signed-off-by: Alex Bennée --- docs/devel/tcg-plugins.rst | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst index a6fdde01f8..8b40b2a606 100644 --- a/docs/devel/tcg-plugins.rst +++ b/docs/devel/tcg-plugins.rst @@ -110,11 +110,6 @@ details are opaque to plugins. The plugin is able to query select details of instructions and system configuration only through the exported *qemu_plugin* functions. -API -~~~ - -.. kernel-doc:: include/qemu/qemu-plugin.h - Internals - @@ -448,3 +443,13 @@ The plugin has a number of arguments, all of them are optional: associativity of the L2 cache, respectively. Setting any of the L2 configuration arguments implies ``l2=on``. (default: N = 2097152 (2MB), B = 64, A = 16) + +API +--- + +The following API is generated from the inline documentation in +``include/qemu/qemu-plugin.h``. Please ensure any updates to the API +include the full kernel-doc annotations. + +.. kernel-doc:: include/qemu/qemu-plugin.h + -- 2.34.1
Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one
On 9/21/22 14:16, Markus Armbruster wrote:
> Philippe Mathieu-Daudé writes:
>
>> On 16/9/22 11:27, Markus Armbruster wrote:
>>> Claudio Fontana writes:
>>>
improve error handling during module load, by changing:
bool module_load_one(const char *prefix, const char *lib_name);
void module_load_qom_one(const char *type);
to:
bool module_load_one(const char *prefix, const char *name, Error **errp);
bool module_load_qom_one(const char *type, Error **errp);
module_load_qom_one has been introduced in:
commit 28457744c345 ("module: qom module support"), which built on top of
module_load_one, but discarded the bool return value. Restore it.
Adapt all callers to emit errors, or ignore them, or fail hard,
as appropriate in each context.
>>>
>>> How exactly does behavior change? The commit message is mum on the
>>> behavior before the patch, and vague on the behavior afterwards.
>>>
Signed-off-by: Claudio Fontana
---
audio/audio.c | 9 ++-
block.c | 15 -
block/dmg.c | 18 +-
hw/core/qdev.c| 10 ++-
include/qemu/module.h | 38 ++--
qom/object.c | 18 +-
softmmu/qtest.c | 6 +-
ui/console.c | 18 +-
util/module.c | 140 --
9 files changed, 194 insertions(+), 78 deletions(-)
>>
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 8c012bbe03..78d4c4de96 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -61,16 +61,44 @@ typedef enum {
>>
void module_call_init(module_init_type type);
-bool module_load_one(const char *prefix, const char *lib_name);
-void module_load_qom_one(const char *type);
+
+/*
+ * module_load_one: attempt to load a module from a set of directories
+ *
+ * directories searched are:
+ * - getenv("QEMU_MODULE_DIR")
+ * - get_relocated_path(CONFIG_QEMU_MODDIR);
+ * - /var/run/qemu/${version_dir}
+ *
+ * prefix: a subsystem prefix, or the empty string ("audio-",
..., "")
+ * name: name of the module
+ * errp: error to set in case the module is found, but load
failed.
+ *
+ * Return value: true on success (found and loaded);
+ * if module if found, but load failed, errp will be set.
+ * if module is not found, errp will not be set.
>>>
>>> I understand you need to distingush two failure modes "found, but load
>>> failed" and "not found".
>>>
>>> Functions that set an error on some failures only tend to be awkward: in
>>> addition to checking the return value for failure, you have to check
>>> @errp for special failures. This is particularly cumbersome when it
>>> requires a @local_err and an error_propagate() just for that. I
>>> generally prefer to return an error code and always set an error.
>>
>> I notice the same issue, therefore would suggest this alternative
>> prototype:
>>
>>bool module_load_one(const char *prefix, const char *name,
>> bool ignore_if_missing, Error **errp);
>> which always sets *errp when returning false:
>>
>> * Return value: if ignore_if_missing is true:
>> * true on success (found or missing), false on
>> * load failure.
>> * if ignore_if_missing is false:
>> * true on success (found and loaded); false if
>> * not found or load failed.
>> * errp will be set if the returned value is false.
>> */
>
> I think this interface is less surprising.
>
> If having to pass a flag turns out to to be a legibility issue, we can
> have wrapper functions.
>
To me it seems even more confusing tbh. Do we have more ideas? Richard?
bool module_load_one(const char *prefix, const char *name, Error **errp);
In my mind we should really say,
RETURN VALUE: a bool with the meaning: "was a module loaded? true/false"
ERROR: The Error parameter tells us: "was there an error loading the module?"
Makes sense to me, but maybe someone has a better one.
Btw, as an aside, for the general Error API pattern, if the bool return value
and Error != NULL should be always related 1:1,
It would have been better to design it with only one of those informing about
the error (Error, as it carries the additional Error information, besides the
information that Error != NULL),
and remove the extra degree of freedom for a return value that at this point
(in the current code) carries ZERO additional useful information.
We could have designed the API to use the return value as... an actual return
value for solving the domain problem at hand,
and use only the Error parameter for the error path.
Ie the API standard pattern could have been, instead o
[PATCH v1 04/10] tests/tcg: add memory-sve test for aarch64
This will be helpful in debugging problems with tracking SVE memory accesses via the TCG plugins system. Signed-off-by: Alex Bennée Cc: Robert Henry Cc: Aaron Lindsay --- tests/tcg/aarch64/Makefile.softmmu-target | 7 +++ tests/tcg/aarch64/system/boot.S | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target index f6fcd4829e..26701b718c 100644 --- a/tests/tcg/aarch64/Makefile.softmmu-target +++ b/tests/tcg/aarch64/Makefile.softmmu-target @@ -31,6 +31,13 @@ LDFLAGS+=-static -nostdlib $(CRT_OBJS) $(MINILIB_OBJS) -lgcc memory: CFLAGS+=-DCHECK_UNALIGNED=1 +memory-sve: memory.c $(LINK_SCRIPT) $(CRT_OBJS) $(MINILIB_OBJS) + $(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS) + +memory-sve: CFLAGS+=-DCHECK_UNALIGNED=1 -march=armv8.1-a+sve -O3 -fno-tree-loop-distribute-patterns + +TESTS+=memory-sve + # Running QEMU_BASE_MACHINE=-M virt -cpu max -display none QEMU_OPTS+=$(QEMU_BASE_MACHINE) -semihosting-config enable=on,target=native,chardev=output -kernel diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S index e190b1efa6..f136363d2a 100644 --- a/tests/tcg/aarch64/system/boot.S +++ b/tests/tcg/aarch64/system/boot.S @@ -179,12 +179,13 @@ __start: isb /* -* Enable FP registers. The standard C pre-amble will be +* Enable FP/SVE registers. The standard C pre-amble will be * saving these and A-profile compilers will use AdvSIMD * registers unless we tell it not to. */ mrs x0, cpacr_el1 orr x0, x0, #(3 << 20) + orr x0, x0, #(3 << 16) msr cpacr_el1, x0 /* Setup some stack space and enter the test code. -- 2.34.1
[PATCH v1 03/10] disas: use result of ->read_memory_func
This gets especially confusing if you start plugging in host addresses
from a trace and you wonder why the output keeps changing. Report when
read_memory_func fails instead of blindly disassembling the buffer
contents.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
---
disas.c | 20 ++---
disas/capstone.c | 73
2 files changed, 53 insertions(+), 40 deletions(-)
diff --git a/disas.c b/disas.c
index f07b6e760b..94d3b45042 100644
--- a/disas.c
+++ b/disas.c
@@ -83,18 +83,18 @@ static int print_insn_objdump(bfd_vma pc, disassemble_info
*info,
const char *prefix)
{
int i, n = info->buffer_length;
-uint8_t *buf = g_malloc(n);
-
-info->read_memory_func(pc, buf, n, info);
-
-for (i = 0; i < n; ++i) {
-if (i % 32 == 0) {
-info->fprintf_func(info->stream, "\n%s: ", prefix);
+g_autofree uint8_t *buf = g_malloc(n);
+
+if (info->read_memory_func(pc, buf, n, info) == 0) {
+for (i = 0; i < n; ++i) {
+if (i % 32 == 0) {
+info->fprintf_func(info->stream, "\n%s: ", prefix);
+}
+info->fprintf_func(info->stream, "%02x", buf[i]);
}
-info->fprintf_func(info->stream, "%02x", buf[i]);
+} else {
+info->fprintf_func(info->stream, "unable to read memory");
}
-
-g_free(buf);
return n;
}
diff --git a/disas/capstone.c b/disas/capstone.c
index 20bc8f9669..fe3efb0d3c 100644
--- a/disas/capstone.c
+++ b/disas/capstone.c
@@ -191,37 +191,43 @@ bool cap_disas_target(disassemble_info *info, uint64_t
pc, size_t size)
size_t tsize = MIN(sizeof(cap_buf) - csize, size);
const uint8_t *cbuf = cap_buf;
-info->read_memory_func(pc + csize, cap_buf + csize, tsize, info);
-csize += tsize;
-size -= tsize;
+if (info->read_memory_func(pc + csize, cap_buf + csize, tsize, info)
== 0) {
+csize += tsize;
+size -= tsize;
-while (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
-cap_dump_insn(info, insn);
-}
+while (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
+cap_dump_insn(info, insn);
+}
+
+/* If the target memory is not consumed, go back for more... */
+if (size != 0) {
+/*
+ * ... taking care to move any remaining fractional insn
+ * to the beginning of the buffer.
+ */
+if (csize != 0) {
+memmove(cap_buf, cbuf, csize);
+}
+continue;
+}
-/* If the target memory is not consumed, go back for more... */
-if (size != 0) {
/*
- * ... taking care to move any remaining fractional insn
- * to the beginning of the buffer.
+ * Since the target memory is consumed, we should not have
+ * a remaining fractional insn.
*/
if (csize != 0) {
-memmove(cap_buf, cbuf, csize);
+info->fprintf_func(info->stream,
+ "Disassembler disagrees with translator "
+ "over instruction decoding\n"
+ "Please report this to
qemu-devel@nongnu.org\n");
}
-continue;
-}
+break;
-/*
- * Since the target memory is consumed, we should not have
- * a remaining fractional insn.
- */
-if (csize != 0) {
+} else {
info->fprintf_func(info->stream,
-"Disassembler disagrees with translator "
-"over instruction decoding\n"
-"Please report this to qemu-devel@nongnu.org\n");
+ "0x%08" PRIx64 ": unable to read memory\n", pc);
+break;
}
-break;
}
cs_close(&handle);
@@ -286,16 +292,23 @@ bool cap_disas_monitor(disassemble_info *info, uint64_t
pc, int count)
/* Make certain that we can make progress. */
assert(tsize != 0);
-info->read_memory_func(pc + csize, cap_buf + csize, tsize, info);
-csize += tsize;
-
-if (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
-cap_dump_insn(info, insn);
-if (--count <= 0) {
-break;
+if (info->read_memory_func(pc + csize, cap_buf + csize,
+ tsize, info) == 0)
+{
+csize += tsize;
+
+if (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
+cap_dump_insn(info, insn);
+if (--count <= 0) {
+break;
+}
}
+memmove(cap_buf, cbuf, csize);
+} else {
+info->fprintf_func(info->strea
[PATCH v1 00/10] plugins/next (disas, monitor, docs, execlog)
Hi, It has been a while since I last posted the state of my plugins queue. These are mostly small cleanups and documentation tweaks. I also did a little bit of tidying up in the disas interface. The following still need review: - docs/devel: document the test plugins - contrib/plugins: reset skip when matching in execlog - docs/devel: move API to end of tcg-plugins.rst - docs/devel: clean-up qemu invocations in tcg-plugins - tests/tcg: add memory-sve test for aarch64 Alex Bennée (9): monitor: expose monitor_puts to rest of code disas: generalise plugin_printf and use for monitor_disas disas: use result of ->read_memory_func tests/tcg: add memory-sve test for aarch64 plugins: extend execlog to filter matches docs/devel: clean-up qemu invocations in tcg-plugins docs/devel: move API to end of tcg-plugins.rst contrib/plugins: reset skip when matching in execlog docs/devel: document the test plugins Richard Henderson (1): plugins: Assert mmu_idx in range before use in qemu_plugin_get_hwaddr docs/devel/tcg-plugins.rst| 175 +++--- docs/devel/writing-monitor-commands.rst | 2 +- include/monitor/monitor.h | 1 + monitor/monitor-internal.h| 1 - block/monitor/block-hmp-cmds.c| 10 +- contrib/plugins/execlog.c | 99 ++-- disas.c | 43 +++--- disas/capstone.c | 73 + hw/misc/mos6522.c | 2 +- monitor/hmp-cmds.c| 8 +- monitor/hmp.c | 2 +- plugins/api.c | 2 + target/i386/helper.c | 2 +- tests/tcg/aarch64/Makefile.softmmu-target | 7 + tests/tcg/aarch64/system/boot.S | 3 +- 15 files changed, 336 insertions(+), 94 deletions(-) -- 2.34.1
[PATCH v1 01/10] monitor: expose monitor_puts to rest of code
This helps us construct strings elsewhere before echoing to the
monitor. It avoids having to jump through hoops like:
monitor_printf(mon, "%s", s->str);
It will be useful in following patches but for now convert all
existing plain "%s" printfs to use the _puts api.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
---
v2
- s/monitor_printf(mon, "%s"/monitor_puts(mon, /
---
docs/devel/writing-monitor-commands.rst | 2 +-
include/monitor/monitor.h | 1 +
monitor/monitor-internal.h | 1 -
block/monitor/block-hmp-cmds.c | 10 +-
hw/misc/mos6522.c | 2 +-
monitor/hmp-cmds.c | 8
monitor/hmp.c | 2 +-
target/i386/helper.c| 2 +-
8 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/docs/devel/writing-monitor-commands.rst
b/docs/devel/writing-monitor-commands.rst
index 4aa2bb904d..2fefedcd98 100644
--- a/docs/devel/writing-monitor-commands.rst
+++ b/docs/devel/writing-monitor-commands.rst
@@ -716,7 +716,7 @@ message. Here's the implementation of the "info roms" HMP
command::
if (hmp_handle_error(mon, err)) {
return;
}
- monitor_printf(mon, "%s", info->human_readable_text);
+ monitor_puts(mon, info->human_readable_text);
}
Also, you have to add the function's prototype to the hmp.h file.
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a4b40e8391..737e750670 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -31,6 +31,7 @@ void monitor_resume(Monitor *mon);
int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp);
+int monitor_puts(Monitor *mon, const char *str);
int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
G_GNUC_PRINTF(2, 0);
int monitor_printf(Monitor *mon, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index caa2e90ef2..a2cdbbf646 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -174,7 +174,6 @@ extern int mon_refcount;
extern HMPCommand hmp_cmds[];
-int monitor_puts(Monitor *mon, const char *str);
void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
bool use_io_thread);
void monitor_data_destroy(Monitor *mon);
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index bfb3c043a0..939a520d17 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -638,16 +638,16 @@ static void print_block_info(Monitor *mon, BlockInfo
*info,
assert(!info || !info->has_inserted || info->inserted == inserted);
if (info && *info->device) {
-monitor_printf(mon, "%s", info->device);
+monitor_puts(mon, info->device);
if (inserted && inserted->has_node_name) {
monitor_printf(mon, " (%s)", inserted->node_name);
}
} else {
assert(info || inserted);
-monitor_printf(mon, "%s",
- inserted && inserted->has_node_name ?
inserted->node_name
- : info && info->has_qdev ? info->qdev
- : "");
+monitor_puts(mon,
+ inserted && inserted->has_node_name ? inserted->node_name
+ : info && info->has_qdev ? info->qdev
+ : "");
}
if (inserted) {
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index f9e646350e..fe38c44426 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -595,7 +595,7 @@ void hmp_info_via(Monitor *mon, const QDict *qdict)
if (hmp_handle_error(mon, err)) {
return;
}
-monitor_printf(mon, "%s", info->human_readable_text);
+monitor_puts(mon, info->human_readable_text);
}
static const MemoryRegionOps mos6522_ops = {
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index c6cd6f91dd..f90eea8d01 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -730,7 +730,7 @@ static void hmp_info_pci_device(Monitor *mon, const
PciDeviceInfo *dev)
monitor_printf(mon, "");
if (dev->class_info->has_desc) {
-monitor_printf(mon, "%s", dev->class_info->desc);
+monitor_puts(mon, dev->class_info->desc);
} else {
monitor_printf(mon, "Class %04" PRId64, dev->class_info->q_class);
}
@@ -2258,12 +2258,12 @@ static void print_stats_schema_value(Monitor *mon,
StatsSchemaValue *value)
if (unit && value->base == 10 &&
value->exponent >= -18 && value->exponent <= 18 &&
value->exponent % 3 == 0) {
-monitor_printf(mon, "%s", si_prefix(value->exponent));
+monitor_puts(mon, si_prefix(value->exponent));
} else if (unit && value->base == 2 &&
value->exponent >= 0 && value->exponent <= 60 &&
value->exponent % 10 == 0) {
-
[PULL 4/5] target/m68k: rename M68K_FEATURE_M68000 to M68K_FEATURE_M68K
From: Mark Cave-Ayland
The M68K_FEATURE_M68000 feature is misleading in that its name suggests the
feature
is defined just for Motorola 68000 CPUs, whilst in fact it is defined for all
Motorola 680X0 CPUs.
In order to avoid confusion with the other M68K_FEATURE_M680X0 constants which
define the features available for specific Motorola CPU models, rename
M68K_FEATURE_M68000 to M68K_FEATURE_M68K and add comments to clarify its usage.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20220917112515.83905-2-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Laurent Vivier
---
target/m68k/cpu.h | 5 +-
target/m68k/cpu.c | 2 +-
target/m68k/helper.c| 2 +-
target/m68k/op_helper.c | 2 +-
target/m68k/translate.c | 138
5 files changed, 75 insertions(+), 74 deletions(-)
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 4d8f48e8c747..67b6c12c2892 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -480,8 +480,9 @@ void do_m68k_semihosting(CPUM68KState *env, int nr);
*/
enum m68k_features {
-/* Base m68k instruction set */
-M68K_FEATURE_M68000,
+/* Base Motorola CPU set (not set for Coldfire CPUs) */
+M68K_FEATURE_M68K,
+/* Motorola CPU feature sets */
M68K_FEATURE_M68010,
M68K_FEATURE_M68020,
M68K_FEATURE_M68030,
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 5bbefda5752d..f681be3a2a58 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -110,7 +110,7 @@ static void m68000_cpu_initfn(Object *obj)
M68kCPU *cpu = M68K_CPU(obj);
CPUM68KState *env = &cpu->env;
-m68k_set_feature(env, M68K_FEATURE_M68000);
+m68k_set_feature(env, M68K_FEATURE_M68K);
m68k_set_feature(env, M68K_FEATURE_USP);
m68k_set_feature(env, M68K_FEATURE_WORD_INDEX);
m68k_set_feature(env, M68K_FEATURE_MOVEP);
diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 5728e48585fc..4621cf24027e 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -460,7 +460,7 @@ void m68k_switch_sp(CPUM68KState *env)
int new_sp;
env->sp[env->current_sp] = env->aregs[7];
-if (m68k_feature(env, M68K_FEATURE_M68000)) {
+if (m68k_feature(env, M68K_FEATURE_M68K)) {
if (env->sr & SR_S) {
/* SR:Master-Mode bit unimplemented then ISP is not available */
if (!m68k_feature(env, M68K_FEATURE_MSP) || env->sr & SR_M) {
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index a96a03405060..5da176d6425a 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -432,7 +432,7 @@ static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
static void do_interrupt_all(CPUM68KState *env, int is_hw)
{
-if (m68k_feature(env, M68K_FEATURE_M68000)) {
+if (m68k_feature(env, M68K_FEATURE_M68K)) {
m68k_interrupt_all(env, is_hw);
return;
}
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 4640eadf78e1..0b618e8eb2bd 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -471,7 +471,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext
*s, TCGv base)
if ((ext & 0x800) == 0 && !m68k_feature(s->env, M68K_FEATURE_WORD_INDEX))
return NULL_QREG;
-if (m68k_feature(s->env, M68K_FEATURE_M68000) &&
+if (m68k_feature(s->env, M68K_FEATURE_M68K) &&
!m68k_feature(s->env, M68K_FEATURE_SCALED_INDEX)) {
ext &= ~(3 << 9);
}
@@ -804,7 +804,7 @@ static TCGv gen_lea_mode(CPUM68KState *env, DisasContext *s,
reg = get_areg(s, reg0);
tmp = mark_to_release(s, tcg_temp_new());
if (reg0 == 7 && opsize == OS_BYTE &&
-m68k_feature(s->env, M68K_FEATURE_M68000)) {
+m68k_feature(s->env, M68K_FEATURE_M68K)) {
tcg_gen_subi_i32(tmp, reg, 2);
} else {
tcg_gen_subi_i32(tmp, reg, opsize_bytes(opsize));
@@ -888,7 +888,7 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext *s,
int mode, int reg0,
if (what == EA_STORE || !addrp) {
TCGv tmp = tcg_temp_new();
if (reg0 == 7 && opsize == OS_BYTE &&
-m68k_feature(s->env, M68K_FEATURE_M68000)) {
+m68k_feature(s->env, M68K_FEATURE_M68K)) {
tcg_gen_addi_i32(tmp, reg, 2);
} else {
tcg_gen_addi_i32(tmp, reg, opsize_bytes(opsize));
@@ -2210,7 +2210,7 @@ DISAS_INSN(bitop_im)
op = (insn >> 6) & 3;
bitnum = read_im16(env, s);
-if (m68k_feature(s->env, M68K_FEATURE_M68000)) {
+if (m68k_feature(s->env, M68K_FEATURE_M68K)) {
if (bitnum & 0xfe00) {
disas_undef(env, s, insn);
return;
@@ -2897,7 +2897,7 @@ DISAS_INSN(mull)
return;
}
SRC_EA(env, src1, OS_LONG, 0, NULL);
-if (m68k_feature(s->env, M68K_FEATURE_M68000)) {
+if (m68k_feature(s->env, M68K_FEATURE_M68K)) {
[PULL 5/5] target/m68k: always call gen_exit_tb() after writes to SR
From: Mark Cave-Ayland
Any write to SR can change the security state so always call gen_exit_tb() when
this occurs. In particular MacOS makes use of andiw/oriw in a few places to
handle the switch between user and supervisor mode.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20220917112515.83905-5-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 4
1 file changed, 4 insertions(+)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 0b618e8eb2bd..233b9d8e5783 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -2375,6 +2375,7 @@ DISAS_INSN(arith_im)
tcg_gen_or_i32(dest, src1, im);
if (with_SR) {
gen_set_sr(s, dest, opsize == OS_BYTE);
+gen_exit_tb(s);
} else {
DEST_EA(env, insn, opsize, dest, &addr);
gen_logic_cc(s, dest, opsize);
@@ -2384,6 +2385,7 @@ DISAS_INSN(arith_im)
tcg_gen_and_i32(dest, src1, im);
if (with_SR) {
gen_set_sr(s, dest, opsize == OS_BYTE);
+gen_exit_tb(s);
} else {
DEST_EA(env, insn, opsize, dest, &addr);
gen_logic_cc(s, dest, opsize);
@@ -2407,6 +2409,7 @@ DISAS_INSN(arith_im)
tcg_gen_xor_i32(dest, src1, im);
if (with_SR) {
gen_set_sr(s, dest, opsize == OS_BYTE);
+gen_exit_tb(s);
} else {
DEST_EA(env, insn, opsize, dest, &addr);
gen_logic_cc(s, dest, opsize);
@@ -4614,6 +4617,7 @@ DISAS_INSN(strldsr)
}
gen_push(s, gen_get_sr(s));
gen_set_sr_im(s, ext, 0);
+gen_exit_tb(s);
}
DISAS_INSN(move_from_sr)
--
2.37.3
[PULL 0/5] M68k for 7.2 patches
The following changes since commit 832e9e33bc51f52fc3ea667d48912e95af3e28f3: Merge tag 'pull-loongarch-20220920' of https://gitlab.com/gaosong/qemu into staging (2022-09-20 14:17:03 -0400) are available in the Git repository at: https://github.com/vivier/qemu-m68k.git tags/m68k-for-7.2-pull-request for you to fetch changes up to c7546abfaa1b1c2729eaddd41c6268a73cdae14f: target/m68k: always call gen_exit_tb() after writes to SR (2022-09-21 15:10:57 +0200) m68k pull request 20220921 - several fixes for SR - implement TAS - feature cleanup Mark Cave-Ayland (2): target/m68k: rename M68K_FEATURE_M68000 to M68K_FEATURE_M68K target/m68k: always call gen_exit_tb() after writes to SR Richard Henderson (3): target/m68k: Implement atomic test-and-set target/m68k: Fix MACSR to CCR target/m68k: Perform writback before modifying SR target/m68k/cpu.h | 5 +- target/m68k/cpu.c | 2 +- target/m68k/helper.c| 2 +- target/m68k/op_helper.c | 2 +- target/m68k/translate.c | 196 +++- 5 files changed, 118 insertions(+), 89 deletions(-) -- 2.37.3
[PULL 3/5] target/m68k: Perform writback before modifying SR
From: Richard Henderson
Writes to SR may change security state, which may involve
a swap of %ssp with %usp as reflected in %a7. Finish the
writeback of %sp@+ before swapping stack pointers.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1206
Signed-off-by: Richard Henderson
Reviewed-by: Laurent Vivier
Reviewed-by: Mark Cave-Ayland
Message-Id: <20220913142818.7802-3-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index c9bb05380323..4640eadf78e1 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -2285,9 +2285,9 @@ static void gen_set_sr_im(DisasContext *s, uint16_t val,
int ccr_only)
tcg_gen_movi_i32(QREG_CC_N, val & CCF_N ? -1 : 0);
tcg_gen_movi_i32(QREG_CC_X, val & CCF_X ? 1 : 0);
} else {
-TCGv sr = tcg_const_i32(val);
-gen_helper_set_sr(cpu_env, sr);
-tcg_temp_free(sr);
+/* Must writeback before changing security state. */
+do_writebacks(s);
+gen_helper_set_sr(cpu_env, tcg_constant_i32(val));
}
set_cc_op(s, CC_OP_FLAGS);
}
@@ -2297,6 +2297,8 @@ static void gen_set_sr(DisasContext *s, TCGv val, int
ccr_only)
if (ccr_only) {
gen_helper_set_ccr(cpu_env, val);
} else {
+/* Must writeback before changing security state. */
+do_writebacks(s);
gen_helper_set_sr(cpu_env, val);
}
set_cc_op(s, CC_OP_FLAGS);
--
2.37.3
[PULL 2/5] target/m68k: Fix MACSR to CCR
From: Richard Henderson
First, we were writing to the entire SR register, instead
of only the flags portion. Second, we were not clearing C
as per the documentation (X was cleared via the 0xf mask).
Signed-off-by: Richard Henderson
Reviewed-by: Laurent Vivier
Message-Id: <20220913142818.7802-2-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index ffcc761d6011..c9bb05380323 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -5912,8 +5912,10 @@ DISAS_INSN(from_mext)
DISAS_INSN(macsr_to_ccr)
{
TCGv tmp = tcg_temp_new();
-tcg_gen_andi_i32(tmp, QREG_MACSR, 0xf);
-gen_helper_set_sr(cpu_env, tmp);
+
+/* Note that X and C are always cleared. */
+tcg_gen_andi_i32(tmp, QREG_MACSR, CCF_N | CCF_Z | CCF_V);
+gen_helper_set_ccr(cpu_env, tmp);
tcg_temp_free(tmp);
set_cc_op(s, CC_OP_FLAGS);
}
--
2.37.3
[PULL 1/5] target/m68k: Implement atomic test-and-set
From: Richard Henderson
This is slightly more complicated than cas,
because tas is allowed on data registers.
Signed-off-by: Richard Henderson
Reviewed-by: Laurent Vivier
Message-Id: <20220829051746.227094-1-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 40 ++--
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 5098f7e570e0..ffcc761d6011 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -2825,19 +2825,39 @@ DISAS_INSN(illegal)
gen_exception(s, s->base.pc_next, EXCP_ILLEGAL);
}
-/* ??? This should be atomic. */
DISAS_INSN(tas)
{
-TCGv dest;
-TCGv src1;
-TCGv addr;
+int mode = extract32(insn, 3, 3);
+int reg0 = REG(insn, 0);
-dest = tcg_temp_new();
-SRC_EA(env, src1, OS_BYTE, 1, &addr);
-gen_logic_cc(s, src1, OS_BYTE);
-tcg_gen_ori_i32(dest, src1, 0x80);
-DEST_EA(env, insn, OS_BYTE, dest, &addr);
-tcg_temp_free(dest);
+if (mode == 0) {
+/* data register direct */
+TCGv dest = cpu_dregs[reg0];
+gen_logic_cc(s, dest, OS_BYTE);
+tcg_gen_ori_tl(dest, dest, 0x80);
+} else {
+TCGv src1, addr;
+
+addr = gen_lea_mode(env, s, mode, reg0, OS_BYTE);
+if (IS_NULL_QREG(addr)) {
+gen_addr_fault(s);
+return;
+}
+src1 = tcg_temp_new();
+tcg_gen_atomic_fetch_or_tl(src1, addr, tcg_constant_tl(0x80),
+ IS_USER(s), MO_SB);
+gen_logic_cc(s, src1, OS_BYTE);
+tcg_temp_free(src1);
+
+switch (mode) {
+case 3: /* Indirect postincrement. */
+tcg_gen_addi_i32(AREG(insn, 0), addr, 1);
+break;
+case 4: /* Indirect predecrememnt. */
+tcg_gen_mov_i32(AREG(insn, 0), addr);
+break;
+}
+}
}
DISAS_INSN(mull)
--
2.37.3
Re: [RFC 4/4] tcg/plugins: Add example pair of QPP plugins
Andrew Fasano writes: > The first plugin, qpp_srv exposes two functions and one callback that other > plugins can leverage. These functions are described in the corresponding > header file. > > The second plugin, qpp_client, imports this header file, registers its > own function to run on a qpp_srv-provided callback, and directly calls > into the two exposed functions in qpp_srv. > > Signed-off-by: Andrew Fasano > --- > contrib/plugins/Makefile | 2 ++ > contrib/plugins/qpp_client.c | 42 > contrib/plugins/qpp_client.h | 1 + > contrib/plugins/qpp_srv.c| 33 > contrib/plugins/qpp_srv.h| 17 +++ Oh and I forgot this toy case should probably be in test/plugins/qpp with an explicit test in tests/tcg/multiarch/Makefile to exercise it during "make check-tcg". This should hopefully avoid having to mess with PLUGINS in tests/tcg/Makefile.target. -- Alex Bennée
Re: [PATCH] add keepalive for qemu-nbd
On 9/21/22 10:36, songlinfeng wrote:
From: songlinfeng
we want to export a image with qemu-nbd as server, in client we use libnbd to
connect qemu-nbd,but when client power down,the server is still working.
qemu-nbd will exit when last client exit.so,we still want server exit when
client power down.maybe qmp can handle it,but i don't know how to do .
Signed-off-by: songlinfeng
---
qemu-nbd.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0cd5aa6..115ef2b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -20,7 +20,8 @@
#include
#include
#include
-
+#include
+#include
#include "qemu/help-texts.h"
#include "qapi/error.h"
#include "qemu/cutils.h"
@@ -365,6 +366,26 @@ static void nbd_accept(QIONetListener *listener,
QIOChannelSocket *cioc,
nb_fds++;
nbd_update_server_watch();
nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
+int tcp_keepalive_intvl = 5;
+int tcp_keepalive_probes = 5;
+int tcp_keepalive_time = 60;
+int tcp_keepalive_on = 1;
+if (setsockopt(cioc->fd, SOL_TCP, TCP_KEEPINTVL,
+ &tcp_keepalive_intvl, sizeof(tcp_keepalive_intvl)) < 0) {
+perror("setsockopt failed\n");
+}
+if (setsockopt(cioc->fd, SOL_TCP, TCP_KEEPCNT,
+ &tcp_keepalive_probes, sizeof(tcp_keepalive_probes)) < 0) {
+perror("setsockopt failed\n");
+}
+if (setsockopt(cioc->fd, SOL_TCP, TCP_KEEPIDLE,
+ &tcp_keepalive_time, sizeof(tcp_keepalive_time)) < 0) {
+perror("setsockopt failed\n");
+}
+if (setsockopt(cios->fd, SOL_SOCKET, SO_KEEPALIVE,
+ &tcp_keepalive_on, sizeof(tcp_keepalive_on)) < 0) {
+perror("setsockopt failed\n");
+}
}
static void nbd_update_server_watch(void)
I tend to so no. Not in this exact form.
In general we have NBD server which could be created and configured
in several different ways. Through qemu-nbd and through QMP.
At the moment we do set KEEP_ALIVE for outgoing connections
only and that is configurable, please refer to
int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
which I believe should be called for the any real configuration
setting with an IP address specified. This option will get
InetSocketAddress->keep_alive/has_keep_alive set.
Though this option has no effect for the listen socket and
this is wrong for you case. You are absolutely right,
as specified in
static int inet_listen_saddr(InetSocketAddress *saddr,
int port_offset,
int num,
Error **errp)
{
struct addrinfo ai,*res,*e;
char port[33];
char uaddr[INET6_ADDRSTRLEN+1];
char uport[33];
int rc, port_min, port_max, p;
int slisten = -1;
int saved_errno = 0;
bool socket_created = false;
Error *err = NULL;
if (saddr->keep_alive) {
error_setg(errp, "keep-alive option is not supported for passive "
"sockets");
return -1;
}
thus you should technically remove this pitfall and set keep_alive
in the generic accept code if you have it specified for the listen
socket.
This seems consistent to me. Adding Vladimir to the loop as
the submitter of the original keep-alive code (if my memory
does not fail me :).
Den
Re: [RFC 4/4] tcg/plugins: Add example pair of QPP plugins
Andrew Fasano writes:
> The first plugin, qpp_srv exposes two functions and one callback that other
> plugins can leverage. These functions are described in the corresponding
> header file.
>
> The second plugin, qpp_client, imports this header file, registers its
> own function to run on a qpp_srv-provided callback, and directly calls
> into the two exposed functions in qpp_srv.
I'll just sketch out how I would change the API in this example plugin:
>
> Signed-off-by: Andrew Fasano
> ---
> contrib/plugins/Makefile | 2 ++
> contrib/plugins/qpp_client.c | 42
> contrib/plugins/qpp_client.h | 1 +
> contrib/plugins/qpp_srv.c| 33
> contrib/plugins/qpp_srv.h| 17 +++
> 5 files changed, 95 insertions(+)
> create mode 100644 contrib/plugins/qpp_client.c
> create mode 100644 contrib/plugins/qpp_client.h
> create mode 100644 contrib/plugins/qpp_srv.c
> create mode 100644 contrib/plugins/qpp_srv.h
>
> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
> index b7720fea0f..b7510de89c 100644
> --- a/contrib/plugins/Makefile
> +++ b/contrib/plugins/Makefile
> @@ -21,6 +21,8 @@ NAMES += lockstep
> NAMES += hwprofile
> NAMES += cache
> NAMES += drcov
> +NAMES += qpp_srv
> +NAMES += qpp_client
>
> SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>
> diff --git a/contrib/plugins/qpp_client.c b/contrib/plugins/qpp_client.c
> new file mode 100644
> index 00..de3335e167
> --- /dev/null
> +++ b/contrib/plugins/qpp_client.c
> @@ -0,0 +1,42 @@
> +#include
> +#include
> +#include
> +#include
> +#include "qpp_srv.h"
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
QEMU_PLUGIN_EXPORT const char *qemu_plugin_name = "qpp_client";
QEMU_PLUGIN_EXPORT const char *qemu_plugin_uses = "qpp_server";
> +
> +void my_on_exit(int x, bool b)
void my_on_exit(gpointer evdata, gpointer udata)
{
struct qpp_exit_event *info = (struct qpp_exit_event *) evdata;
x = info->x;
b = info->b;
> +{
> + g_autoptr(GString) report = g_string_new("Client: on_exit runs with args:
> ");
> + g_string_append_printf(report, "%d, %d\n", x, b);
> + qemu_plugin_outs(report->str);
> +
> + g_string_printf(report, "Client: calls qpp_srv's do_add(1): %d\n",
> + qpp_srv_do_add(1));
> + qemu_plugin_outs(report->str);
> +
> + g_string_printf(report, "Client: calls qpp_srv's do_sub(1): %d\n",
> + qpp_srv_do_sub(1));
> + qemu_plugin_outs(report->str);
> +}
> +
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> + const qemu_info_t *info, int argc, char **argv) {
> +
> +/*
> + * Register our "my_on_exit" function to run on the on_exit QPP-callback
> + * exported by qpp_srv
> + */
> +QPP_REG_CB("qpp_srv", on_exit, my_on_exit);
qemu_plugin_register_event_listener("qpp_server", "exit", my_on_exit);
> +
> +g_autoptr(GString) report = g_string_new(CURRENT_PLUGIN ": Call "
> + "qpp_srv's do_add(0) => ");
> +g_string_append_printf(report, "%d\n", qpp_srv_do_add(0));
> +qemu_plugin_outs(report->str);
> +
> +g_string_printf(report, "Client: registered on_exit callback\n");
> +return 0;
> +}
> +
> diff --git a/contrib/plugins/qpp_client.h b/contrib/plugins/qpp_client.h
> new file mode 100644
> index 00..573923f580
> --- /dev/null
> +++ b/contrib/plugins/qpp_client.h
> @@ -0,0 +1 @@
> +void my_on_exit(int, bool);
> diff --git a/contrib/plugins/qpp_srv.c b/contrib/plugins/qpp_srv.c
> new file mode 100644
> index 00..61a6ab38ed
> --- /dev/null
> +++ b/contrib/plugins/qpp_srv.c
> @@ -0,0 +1,33 @@
> +#include
> +#include
> +#include
> +#include
> +#include "qpp_srv.h"
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
QEMU_PLUGIN_EXPORT const char *qemu_plugin_name = "qpp_server";
> +
> +QPP_CREATE_CB(on_exit);
void *on_exit;
> +
> +static void plugin_exit(qemu_plugin_id_t id, void *p)
> +{
> + qemu_plugin_outs(CURRENT_PLUGIN "exit triggered, running all registered"
> + " QPP callbacks\n");
> + QPP_RUN_CB(on_exit, 0, true);
struct qpp_exit_event *info = g_new0(qpp_exit_event, 1);
info->x = 0;
info->b = true;
qemu_plugin_trigger_event(on_exit, info);
> +}
> +
> +QEMU_PLUGIN_EXPORT int do_add(int x)
QEMU_PLUGIN_EXPORT int qpp_srv_do_add(int x)
> +{
> + return x + 1;
> +}
> +
> +QEMU_PLUGIN_EXPORT int do_sub(int x)
QEMU_PLUGIN_EXPORT int qpp_srv_do_sub(int x)
> +{
> + return x - 1;
> +}
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> + const qemu_info_t *info, int argc, char **argv) {
> +qemu_plugin_outs("qpp_srv loaded\n");
> +qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> +return 0;
> +}
> diff --git a/contrib/plugins/qpp_srv.h b/contrib/plugins/qpp_srv.h
> new file mode 100644
> in
Re: [PATCH v8 12/14] qemu-sockets: update socket_uri() and socket_parse() to be consistent
Laurent Vivier writes:
> To be consistent with socket_uri(), add 'tcp:' prefix for inet type in
> socket_parse(), by default socket_parse() use tcp when no prefix is
> provided (format is host:port).
>
> In socket_uri(), use 'vsock:' prefix for vsock type rather than 'tcp:'
> because it makes a vsock address look like an inet address with CID
> misinterpreted as host.
> Goes back to commit 9aca82ba31 "migration: Create socket-address parameter"
>
> Signed-off-by: Laurent Vivier
> ---
> util/qemu-sockets.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 9f6f655fd526..a9926af714c4 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1090,7 +1090,7 @@ char *socket_uri(SocketAddress *addr)
> case SOCKET_ADDRESS_TYPE_FD:
> return g_strdup_printf("fd:%s", addr->u.fd.str);
> case SOCKET_ADDRESS_TYPE_VSOCK:
> -return g_strdup_printf("tcp:%s:%s",
> +return g_strdup_printf("vsock:%s:%s",
> addr->u.vsock.cid,
> addr->u.vsock.port);
> default:
> @@ -1124,6 +1124,11 @@ SocketAddress *socket_parse(const char *str, Error
> **errp)
> if (vsock_parse(&addr->u.vsock, str + strlen("vsock:"), errp)) {
> goto fail;
> }
> +} else if (strstart(str, "tcp:", NULL)) {
> +addr->type = SOCKET_ADDRESS_TYPE_INET;
> +if (inet_parse(&addr->u.inet, str + strlen("tcp:"), errp)) {
> +goto fail;
> +}
> } else {
> addr->type = SOCKET_ADDRESS_TYPE_INET;
> if (inet_parse(&addr->u.inet, str, errp)) {
I'd be tempted to split this patch, but I'm a compulsive patch splitter
;)
Reviewed-by: Markus Armbruster
Re: [PATCH v8 13/14] net: stream: move to QIO
Laurent Vivier writes:
> Use QIOChannel, QIOChannelSocket and QIONetListener.
>
> Signed-off-by: Laurent Vivier
> ---
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ee2436ae14a7..a0b5b70c80cb 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2732,8 +2732,8 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
> "-netdev socket,id=str[,fd=h][,udp=host:port][,localaddr=host:port]\n"
> "configure a network backend to connect to another
> network\n"
> "using an UDP tunnel\n"
> -"-netdev
> stream,id=str[,server=on|off],addr.type=inet,addr.host=host,addr.port=port\n"
> -"-netdev stream,id=str[,server=on|off],addr.type=unix,addr.path=path\n"
> +"-netdev
> stream,id=str[,server=on|off],addr.type=inet,addr.host=host,addr.port=port[,to=maxport][,numeric=on|off][,keep-alive=on|off][,mptcp=on|off][,addr.ipv4=on|off][,addr.ipv6=on|off]\n"
> +"-netdev
> stream,id=str[,server=on|off],addr.type=unix,addr.path=path[,abstract=on|off][,tight=on|off]\n"
> "-netdev stream,id=str[,server=on|off],addr.type=fd,addr.str=h\n"
> "configure a network backend to connect to another
> network\n"
> "using a socket connection in stream mode.\n"
The commit message didn't prepare me for this change. Could you
explain?
Re: [RFC 3/4] tcg/plugins: Support for inter-plugin interactions
Andrew Fasano writes:
> Expand tcg-plugin system to allow for plugins to export functions
> and callbacks that can be used by other plugins. Exported functions
> can be called at runtime by other loaded plugins. Loaded plugins
> can register functions with exported callbacks and have these
> functions run whenever the callback is triggered.
Could you please split the callback and function handling in future
patches to aid review.
>
> Signed-off-by: Andrew Fasano
> ---
> include/qemu/plugin-qpp.h| 252 +++
> plugins/core.c | 11 ++
> plugins/loader.c | 24
> plugins/plugin.h | 5 +
> plugins/qemu-plugins.symbols | 1 +
> 5 files changed, 293 insertions(+)
> create mode 100644 include/qemu/plugin-qpp.h
>
> diff --git a/include/qemu/plugin-qpp.h b/include/qemu/plugin-qpp.h
> new file mode 100644
> index 00..befa4f9b8b
> --- /dev/null
> +++ b/include/qemu/plugin-qpp.h
> @@ -0,0 +1,252 @@
> +#ifndef PLUGIN_QPP_H
> +#define PLUGIN_QPP_H
> +
> +/*
> + * Facilities for "QEMU plugin to plugin" (QPP) interactions between tcg
> + * plugins. These allow for an inter-plugin callback system as well
> + * as direct function calls between loaded plugins. For more details see
> + * docs/devel/plugin.rst.
> + */
> +
> +#include
> +#include
> +#include
> +extern GModule * qemu_plugin_name_to_handle(const char *);
Plugin API functions should have /** */ kernel-doc annotations for the
benefit of the autogenerated API docs. Moreover handles to plugins are
usually anonymous pointer types to discourage them fishing about in the
contents.
The fact we expose GModule to the plugin to do the linking makes me
think that maybe the linking should be pushed into loader and be done on
behalf of the plugin.
> +
> +#define PLUGIN_CONCAT(x, y) _PLUGIN_CONCAT(x, y)
> +#define _PLUGIN_CONCAT(x, y) x##y
> +#define QPP_NAME(plugin, fn) PLUGIN_CONCAT(plugin, PLUGIN_CONCAT(_, fn))
> +#define QPP_MAX_CB 256
> +#define _QPP_SETUP_NAME(plugin, fn) PLUGIN_CONCAT(_qpp_setup_, \
> +QPP_NAME(plugin, fn))
> +
> +/*
> + **
> + * The QPP_CREATE_CB and QPP_RUN_CB macros are to be used by a plugin that
> + * wishs to create and later trigger QPP-based callback events. These are
> + * events that the plugin can detect (i.e., through analysis of guest state)
> + * that may be of interest to other plugins.
> + **
> + */
> +
> +/*
> + * QPP_CREATE_CB(name) will create a callback by defining necessary internal
> + * functions and variables based off the provided name. It must be run before
> + * triggering the callback event (with QPP_RUN_CB). This macro will create
> the
> + * following variables and functions, based off the provided name:
> + *
> + * 1) qpp_[name]_cb is an array of function pointers storing the
> + *registered callbacks.
> + * 2) qpp_[name]_num_cb stores the number of functions stored with this
> + *callback.
> + * 3) qpp_add_cb_[name] is a function to add a pointer into the qpp_[name]_cb
> + *array and increment qpp_[name]_num_cb.
> + * 4) qpp_remove_cb_[name] finds a registered callback, deletes it,
> decrements
> + *_num_cb and shifts the _cb array appropriately to fill the gap.
> + *
> + * Important notes:
> + *
> + * 1) Multiple callbacks can be registered for the same event, however
> consumers
> + *can not control the order in which they are called and this order may
> + *change in the future.
> + *
> + * 2) If this macro is incorrectly used multiple times in the same plugin
> with
> + *the same callback name set, it will raise a compilation error since
> + *these variables would then be defined multiple times. The same callback
> + *name can, however, be created in distrinct plugins without issue.
> + */
> +#define QPP_CREATE_CB(cb_name) \
> +void qpp_add_cb_##cb_name(cb_name##_t fptr);\
> +bool qpp_remove_cb_##cb_name(cb_name##_t fptr); \
> +cb_name##_t * qpp_##cb_name##_cb[QPP_MAX_CB]; \
> +int qpp_##cb_name##_num_cb; \
> +\
> +void qpp_add_cb_##cb_name(cb_name##_t fptr) \
> +{ \
> + assert(qpp_##cb_name##_num_cb < QPP_MAX_CB); \
> + qpp_##cb_name##_cb[qpp_##cb_name##_num_cb] = fptr;\
> + qpp_##cb_name##_num_cb += 1; \
> +} \
> +\
> +bool qpp_remove_cb_##cb_name(cb_name##_t fptr) \
> +{ \
> + int i = 0;
Re: QEMU's FreeBSD 13 CI job is failing
On Wed, Sep 21, 2022 at 1:13 AM Daniel P. Berrangé wrote: > On Tue, Sep 20, 2022 at 02:21:46PM -0600, Warner Losh wrote: > > On Tue, Sep 20, 2022 at 2:57 AM Daniel P. Berrangé > > wrote: > > > > > On Tue, Sep 20, 2022 at 10:23:56AM +0200, Thomas Huth wrote: > > > > On 20/09/2022 10.21, Daniel P. Berrangé wrote: > > > > > On Tue, Sep 20, 2022 at 08:44:27AM +0200, Thomas Huth wrote: > > > > > > > > > > > > Seen here for example: > > > > > > > > > > > > https://gitlab.com/qemu-project/qemu/-/jobs/3050165356#L2543 > > > > > > > > > > > > ld-elf.so.1: /lib/libc.so.7: version FBSD_1.7 required by > > > > > > /usr/local/lib/libpython3.9.so.1.0 not found > > > > > > ERROR: Cannot use '/usr/local/bin/python3', Python >= 3.6 is > > > required. > > > > > > > > > > > > ... looks like the Python binary is not working anymore? Does > > > anybody know > > > > > > what happened here? > > > > > > > > > > FreeBSD ports is only guaranteed to work with latest minor release > > > > > base image. The python binary recently started relying on symbols > > > > > in the 13.1 base image, and we're using 13.0. > > > > > > > > > > I updated lcitool last week to pick 13.1, so we just need a refresh > > > > > on the QEMU side to pick this up. > > > > > > > > OK ... Alex, IIRC you have a patch queued to update the files that > are > > > > refreshed by lcitool ... does that already contain the update for > > > FreeBSD, > > > > too? > > > > > > Oh actually, I'm forgetting that QEMU doesn't use the 'lcitool > manifest' > > > command for auto-generating the gitlab-ci.yml file. In QEMU's case just > > > manually edit .gitlab-ci.d/cirrus.yml to change > > > > > > CIRRUS_VM_IMAGE_NAME: freebsd-13-0 > > > > > > > FreeBSD's support policy is that we EOL minor dot releases a few months > > after > > the next minor release is final. Part of that process involves moving the > > build > > of packages to that new minor version (which is what's not guaranteed to > > work > > on older versions... only old binaries on new versions is guaranteed)... > > And that's > > the problem that was hit here. > > It would be nice if something in the ports tool / packages was > able to report the incompatibility at time of install, rather > than leaving a later runtime failed. > Indeed. I've suggested it to the authors... There's some technical issues around this and the package format they need to work out. > > I'll try to submit changes after the next minor release in that 'few > month' > > window > > to update this in the future. In general, doing so would be the best fit > > with FreeBSD's > > support model... It's one of those things I didn't think of at the time, > > but is obvious in > > hindsight. > > Note, we're reliant on Cirrus CI actually publishing the new images > for use. I've not previously checked before how quickly they publish > them after FreeBSD does the upstream release, but anyway I go by what > they list here: > > https://cirrus-ci.org/guide/FreeBSD/ Yea. They have been pretty good in the past about getting new images up quickly after the release. Warner
Re: [PATCH v3 1/1] monitor/hmp: print trace as option in help for log command
Dongli Zhang writes:
> Hi Markus,
>
> On 9/17/22 2:44 PM, Philippe Mathieu-Daudé via wrote:
>> Hi Markus,
>>
>> On 2/9/22 14:24, Markus Armbruster wrote:
>>> Dongli Zhang writes:
>>>
The below is printed when printing help information in qemu-system-x86_64
command line, and when CONFIG_TRACE_LOG is enabled:
$ qemu-system-x86_64 -d help
... ...
trace:PATTERN enable trace events
Use "-d trace:help" to get a list of trace events.
However, the options of "trace:PATTERN" are only printed by
"qemu-system-x86_64 -d help", but missing in hmp "help log" command.
Fixes: c84ea00dc2 ("log: add "-d trace:PATTERN"")
Cc: Joe Jin
Signed-off-by: Dongli Zhang
---
Changed since v1:
- change format for "none" as well.
Changed since v2:
- use "log trace:help" in help message.
- add more clarification in commit message.
- add 'Fixes' tag.
---
monitor/hmp.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
>>
>>> Not this patch's fault:
>>>
>>> 1. "-d help" terminates with exit status 1, "-d trace:help" with 0. The
>>> former is wrong.
>
> May I assume it is expected to have exit status 1 when "-d help"?
Non-zero exit status means error. Asking for and receiving help is not
an error. Therefore, "-d help" should exit with status 0, just like
"-help", "-device help", "-machine help", ...
> According to the output of "-d", there is even not a "help" option, but only a
> "-d trace:help" option. That is, "-d help" is not officially supported.
It *is* documented:
$ qemu-system-x86_64 -help | grep -- '^-d '
-d item1,...enable logging of specified items (use '-d help' for a list
of log items)
> The below example use "-d hellworld" but not "help".
>
> # qemu-system-x86_64 -d helloworld
> Log items (comma separated):
> out_asm show generated host assembly code for each compiled TB
> in_asm show target assembly code for each compiled TB
> op show micro ops for each compiled TB
> op_opt show micro ops after optimization
> op_ind show micro ops before indirect lowering
> int show interrupts/exceptions in short format
> execshow trace before each executed TB (lots of logs)
> cpu show CPU registers before entering a TB (lots of logs)
> fpu include FPU registers in the 'cpu' logging
> mmu log MMU-related activities
> pcall x86 only: show protected mode far calls/returns/exceptions
> cpu_reset show CPU state before CPU resets
> unimp log unimplemented functionality
> guest_errorslog when the guest OS does something invalid (eg accessing a
> non-existent register)
> pagedump pages at beginning of user mode emulation
> nochain do not chain compiled TBs so that "exec" and "cpu" show
> complete traces
> plugin output from TCG plugins
>
> strace log every user-mode syscall, its input, and its result
> tid open a separate log file per thread; filename must contain
> '%d'
> trace:PATTERN enable trace events
>
> Use "-d trace:help" to get a list of trace events.
>
>
> According to the source code, the qemu_str_to_log_mask() expects either log
> items or "trace". For any other inputs (e.g., "help" or "helloworld"),
> qemu_str_to_log_mask() returns 0 (no bit set in the mask).
You're right.
>That indicates the
> input (e.g., "help") is not an expected input.
No, it indicates laziness :)
> Therefore, can I assume this is not a bug? I do not think something like below
> is very helpful.
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 263f029a8e..54c8e624bf 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2389,6 +2389,8 @@ static void qemu_process_early_options(void)
> mask = qemu_str_to_log_mask(log_mask);
> if (!mask) {
> qemu_print_log_usage(stdout);
> +if (g_str_equal(log_mask, "help"))
> +exit(0)
> exit(1);
> }
> }
Let's make "-d help" print help to stdout and terminate successfully,
and "-d crap" report an error and terminate unsuccessfully. Just like
other options, such as -device and -machine.
> Thank you very much!
You're welcome!
Re: [PATCH] i386: Add new CPU model SapphireRapids
* Daniel P. Berrangé (berra...@redhat.com) wrote: > On Wed, Sep 21, 2022 at 03:51:42PM +0100, Dr. David Alan Gilbert wrote: > > * Wang, Lei (lei4.w...@intel.com) wrote: > > > The new CPU model mostly inherits features from Icelake-Server, while > > > adding new features: > > > - AMX (Advance Matrix eXtensions) > > > - Bus Lock Debug Exception > > > and new instructions: > > > - AVX VNNI (Vector Neural Network Instruction): > > > - VPDPBUS: Multiply and Add Unsigned and Signed Bytes > > > - VPDPBUSDS: Multiply and Add Unsigned and Signed Bytes with > > > Saturation > > > - VPDPWSSD: Multiply and Add Signed Word Integers > > > - VPDPWSSDS: Multiply and Add Signed Integers with Saturation > > > - FP16: Replicates existing AVX512 computational SP (FP32) instructions > > >using FP16 instead of FP32 for ~2X performance gain > > > - SERIALIZE: Provide software with a simple way to force the processor to > > >complete all modifications, faster, allowed in all privilege levels and > > >not causing an unconditional VM exit > > > - TSX Suspend Load Address Tracking: Allows programmers to choose which > > >memory accesses do not need to be tracked in the TSX read set > > > - AVX512_BF16: Vector Neural Network Instructions supporting BFLOAT16 > > >inputs and conversion instructions from IEEE single precision > > > > > > Features may be added in future versions: > > > - CET (virtualization support hasn't been merged) > > > Instructions may be added in future versions: > > > - fast zero-length MOVSB (KVM doesn't support yet) > > > - fast short STOSB (KVM doesn't support yet) > > > - fast short CMPSB, SCASB (KVM doesn't support yet) > > > > > > Signed-off-by: Wang, Lei > > > Reviewed-by: Robert Hoo > > > > Hi, > >What fills in the AMX tile and tmul information leafs > > (0x1D, 0x1E)? > > In particular, how would we make sure when we migrate between two > > generations of AMX/Tile/Tmul capable devices with different > > register/palette/tmul limits that the migration is tied to the CPU type > > correctly? > > Would you expect all devices called a 'SappireRapids' to have the same > > sizes? > > We shouldn't assume this will only be used on 'SappireRapids' host > silicon. Thi named CPU model is likely to be used by a guest running > on any host silicon generations that follow SappireRapids too. Indeed, but I wanted to check the opposite question first; whether all SappireRapids had the same sizes; I think you're asking the opposite question. Dave > 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 :| > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] i386: Add new CPU model SapphireRapids
On Wed, Sep 21, 2022 at 03:51:42PM +0100, Dr. David Alan Gilbert wrote: > * Wang, Lei (lei4.w...@intel.com) wrote: > > The new CPU model mostly inherits features from Icelake-Server, while > > adding new features: > > - AMX (Advance Matrix eXtensions) > > - Bus Lock Debug Exception > > and new instructions: > > - AVX VNNI (Vector Neural Network Instruction): > > - VPDPBUS: Multiply and Add Unsigned and Signed Bytes > > - VPDPBUSDS: Multiply and Add Unsigned and Signed Bytes with Saturation > > - VPDPWSSD: Multiply and Add Signed Word Integers > > - VPDPWSSDS: Multiply and Add Signed Integers with Saturation > > - FP16: Replicates existing AVX512 computational SP (FP32) instructions > >using FP16 instead of FP32 for ~2X performance gain > > - SERIALIZE: Provide software with a simple way to force the processor to > >complete all modifications, faster, allowed in all privilege levels and > >not causing an unconditional VM exit > > - TSX Suspend Load Address Tracking: Allows programmers to choose which > >memory accesses do not need to be tracked in the TSX read set > > - AVX512_BF16: Vector Neural Network Instructions supporting BFLOAT16 > >inputs and conversion instructions from IEEE single precision > > > > Features may be added in future versions: > > - CET (virtualization support hasn't been merged) > > Instructions may be added in future versions: > > - fast zero-length MOVSB (KVM doesn't support yet) > > - fast short STOSB (KVM doesn't support yet) > > - fast short CMPSB, SCASB (KVM doesn't support yet) > > > > Signed-off-by: Wang, Lei > > Reviewed-by: Robert Hoo > > Hi, >What fills in the AMX tile and tmul information leafs > (0x1D, 0x1E)? > In particular, how would we make sure when we migrate between two > generations of AMX/Tile/Tmul capable devices with different > register/palette/tmul limits that the migration is tied to the CPU type > correctly? > Would you expect all devices called a 'SappireRapids' to have the same > sizes? We shouldn't assume this will only be used on 'SappireRapids' host silicon. Thi named CPU model is likely to be used by a guest running on any host silicon generations that follow SappireRapids too. 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 :|
Re: [PATCH] qga: fix possible memory leak
luzhipeng writes:
> From: lu zhipeng
>
> Signed-off-by: lu zhipeng
> ---
> qga/main.c | 19 ++-
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 5f1efa2333..73ea1aae65 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1287,7 +1287,7 @@ static GAState *initialize_agent(GAConfig *config, int
> socket_activation)
> if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
> g_critical("unable to create (an ancestor of) the state directory"
> " '%s': %s", config->state_dir, strerror(errno));
> -return NULL;
> +goto failed;
> }
> #endif
>
> @@ -1312,7 +1312,7 @@ static GAState *initialize_agent(GAConfig *config, int
> socket_activation)
> if (!log_file) {
> g_critical("unable to open specified log file: %s",
> strerror(errno));
> -return NULL;
> +goto failed;
> }
> s->log_file = log_file;
> }
> @@ -1323,7 +1323,7 @@ static GAState *initialize_agent(GAConfig *config, int
> socket_activation)
> s->pstate_filepath,
> ga_is_frozen(s))) {
> g_critical("failed to load persistent state");
> -return NULL;
> +goto failed;
> }
>
> config->blacklist = ga_command_blacklist_init(config->blacklist);
> @@ -1344,7 +1344,7 @@ static GAState *initialize_agent(GAConfig *config, int
> socket_activation)
> #ifndef _WIN32
> if (!register_signal_handlers()) {
> g_critical("failed to register signal handlers");
> -return NULL;
> +goto failed;
> }
> #endif
>
> @@ -1357,12 +1357,21 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
> s->wakeup_event = CreateEvent(NULL, TRUE, FALSE, TEXT("WakeUp"));
> if (s->wakeup_event == NULL) {
> g_critical("CreateEvent failed");
> -return NULL;
> +goto failed;
> }
> #endif
>
> ga_state = s;
> return s;
> +
> +failed:
> +g_free(s->pstate_filepath);
> +g_free(s->state_filepath_isfrozen);
> +if (s->log_file && s->log_file != stderr) {
> +fclose(s->log_file);
> +}
> +g_free(s);
> +return NULL;
> }
>
> static void cleanup_agent(GAState *s)
The function's only caller is main():
int main(int argc, char **argv)
{
int ret = EXIT_SUCCESS;
...
s = initialize_agent(config, socket_activation);
if (!s) {
g_critical("error initializing guest agent");
goto end;
}
...
end:
if (config->daemonize) {
unlink(config->pid_filepath);
}
config_free(config);
return ret;
}
When initialize_agent() fails, the process terminates immediately.
There is no memory leak.
We might want to free in initialize_agent() anyway. Up to the
maintainer.
Bug (not related to this patch): when initialize_agent() fails, we still
terminate successfully. We should ret = EXIT_FAILURE before goto end,
like we do elsewhere in main().
Re: [PATCH RFC 0/7] hostmem: NUMA-aware memory preallocation using ThreadContext
On 21.09.22 16:44, Michal Prívozník wrote: On 7/21/22 14:07, David Hildenbrand wrote: Ping? Is there any plan how to move forward? I have libvirt patches ready to consume this and I'd like to prune my old local branches :-) Heh, I was thinking about this series just today. I was distracted with all other kind of stuff. I'll move forward with this series later this week/early next week. Thanks! -- Thanks, David / dhildenb
[PATCH] qcow2: fix memory leak in qcow2_read_extensions
From: lu zhipeng
Free feature_table if it is failed in bdrv_pread.
Signed-off-by: lu zhipeng
---
block/qcow2.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index c6c6692fb7..c8fc3a6160 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -275,6 +275,7 @@ static int qcow2_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
if (ret < 0) {
error_setg_errno(errp, -ret, "ERROR: ext_feature_table: "
"Could not read table");
+g_free(feature_table);
return ret;
}
--
2.31.1
Re: [PATCH] i386: Add new CPU model SapphireRapids
* Wang, Lei (lei4.w...@intel.com) wrote:
> The new CPU model mostly inherits features from Icelake-Server, while
> adding new features:
> - AMX (Advance Matrix eXtensions)
> - Bus Lock Debug Exception
> and new instructions:
> - AVX VNNI (Vector Neural Network Instruction):
> - VPDPBUS: Multiply and Add Unsigned and Signed Bytes
> - VPDPBUSDS: Multiply and Add Unsigned and Signed Bytes with Saturation
> - VPDPWSSD: Multiply and Add Signed Word Integers
> - VPDPWSSDS: Multiply and Add Signed Integers with Saturation
> - FP16: Replicates existing AVX512 computational SP (FP32) instructions
>using FP16 instead of FP32 for ~2X performance gain
> - SERIALIZE: Provide software with a simple way to force the processor to
>complete all modifications, faster, allowed in all privilege levels and
>not causing an unconditional VM exit
> - TSX Suspend Load Address Tracking: Allows programmers to choose which
>memory accesses do not need to be tracked in the TSX read set
> - AVX512_BF16: Vector Neural Network Instructions supporting BFLOAT16
>inputs and conversion instructions from IEEE single precision
>
> Features may be added in future versions:
> - CET (virtualization support hasn't been merged)
> Instructions may be added in future versions:
> - fast zero-length MOVSB (KVM doesn't support yet)
> - fast short STOSB (KVM doesn't support yet)
> - fast short CMPSB, SCASB (KVM doesn't support yet)
>
> Signed-off-by: Wang, Lei
> Reviewed-by: Robert Hoo
Hi,
What fills in the AMX tile and tmul information leafs
(0x1D, 0x1E)?
In particular, how would we make sure when we migrate between two
generations of AMX/Tile/Tmul capable devices with different
register/palette/tmul limits that the migration is tied to the CPU type
correctly?
Would you expect all devices called a 'SappireRapids' to have the same
sizes?
Dave
> ---
> target/i386/cpu.c | 128 ++
> target/i386/cpu.h | 4 ++
> 2 files changed, 132 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1db1278a59..abb43853d4 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3467,6 +3467,134 @@ static const X86CPUDefinition builtin_x86_defs[] = {
> { /* end of list */ }
> }
> },
> +{
> +.name = "SapphireRapids",
> +.level = 0x20,
> +.vendor = CPUID_VENDOR_INTEL,
> +.family = 6,
> +.model = 143,
> +.stepping = 4,
> +/*
> + * please keep the ascending order so that we can have a clear view
> of
> + * bit position of each feature.
> + */
> +.features[FEAT_1_EDX] =
> +CPUID_FP87 | CPUID_VME | CPUID_DE | CPUID_PSE | CPUID_TSC |
> +CPUID_MSR | CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC |
> +CPUID_SEP | CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV |
> +CPUID_PAT | CPUID_PSE36 | CPUID_CLFLUSH | CPUID_MMX | CPUID_FXSR
> |
> +CPUID_SSE | CPUID_SSE2,
> +.features[FEAT_1_ECX] =
> +CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSSE3 |
> +CPUID_EXT_FMA | CPUID_EXT_CX16 | CPUID_EXT_PCID |
> CPUID_EXT_SSE41 |
> +CPUID_EXT_SSE42 | CPUID_EXT_X2APIC | CPUID_EXT_MOVBE |
> +CPUID_EXT_POPCNT | CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_AES |
> +CPUID_EXT_XSAVE | CPUID_EXT_AVX | CPUID_EXT_F16C |
> CPUID_EXT_RDRAND,
> +.features[FEAT_8000_0001_EDX] =
> +CPUID_EXT2_SYSCALL | CPUID_EXT2_NX | CPUID_EXT2_PDPE1GB |
> +CPUID_EXT2_RDTSCP | CPUID_EXT2_LM,
> +.features[FEAT_8000_0001_ECX] =
> +CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM | CPUID_EXT3_3DNOWPREFETCH,
> +.features[FEAT_8000_0008_EBX] =
> +CPUID_8000_0008_EBX_WBNOINVD,
> +.features[FEAT_7_0_EBX] =
> +CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_HLE |
> +CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 |
> +CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | CPUID_7_0_EBX_RTM |
> +CPUID_7_0_EBX_AVX512F | CPUID_7_0_EBX_AVX512DQ |
> +CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | CPUID_7_0_EBX_SMAP |
> +CPUID_7_0_EBX_AVX512IFMA | CPUID_7_0_EBX_CLFLUSHOPT |
> +CPUID_7_0_EBX_CLWB | CPUID_7_0_EBX_AVX512CD |
> CPUID_7_0_EBX_SHA_NI |
> +CPUID_7_0_EBX_AVX512BW | CPUID_7_0_EBX_AVX512VL,
> +.features[FEAT_7_0_ECX] =
> +CPUID_7_0_ECX_AVX512_VBMI | CPUID_7_0_ECX_UMIP |
> CPUID_7_0_ECX_PKU |
> +CPUID_7_0_ECX_AVX512_VBMI2 | CPUID_7_0_ECX_GFNI |
> +CPUID_7_0_ECX_VAES | CPUID_7_0_ECX_VPCLMULQDQ |
> +CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
> +CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57 |
> +CPUID_7_0_ECX_RDPID | CPUID_7_0_ECX_BUS_LOCK_DETECT,
> +.features[FEAT_7_0_EDX] =
>
Re: [PATCH RFC 0/7] hostmem: NUMA-aware memory preallocation using ThreadContext
On 7/21/22 14:07, David Hildenbrand wrote: > Ping? Is there any plan how to move forward? I have libvirt patches ready to consume this and I'd like to prune my old local branches :-) Michal
[PATCH] add keepalive for qemu-nbd
From: songlinfeng
we want to export a image with qemu-nbd as server, in client we use libnbd to
connect qemu-nbd,but when client power down,the server is still working.
qemu-nbd will exit when last client exit.so,we still want server exit when
client power down.maybe qmp can handle it,but i don't know how to do .
Signed-off-by: songlinfeng
---
qemu-nbd.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0cd5aa6..115ef2b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -20,7 +20,8 @@
#include
#include
#include
-
+#include
+#include
#include "qemu/help-texts.h"
#include "qapi/error.h"
#include "qemu/cutils.h"
@@ -365,6 +366,26 @@ static void nbd_accept(QIONetListener *listener,
QIOChannelSocket *cioc,
nb_fds++;
nbd_update_server_watch();
nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
+int tcp_keepalive_intvl = 5;
+int tcp_keepalive_probes = 5;
+int tcp_keepalive_time = 60;
+int tcp_keepalive_on = 1;
+if (setsockopt(cioc->fd, SOL_TCP, TCP_KEEPINTVL,
+ &tcp_keepalive_intvl, sizeof(tcp_keepalive_intvl)) < 0) {
+perror("setsockopt failed\n");
+}
+if (setsockopt(cioc->fd, SOL_TCP, TCP_KEEPCNT,
+ &tcp_keepalive_probes, sizeof(tcp_keepalive_probes)) < 0) {
+perror("setsockopt failed\n");
+}
+if (setsockopt(cioc->fd, SOL_TCP, TCP_KEEPIDLE,
+ &tcp_keepalive_time, sizeof(tcp_keepalive_time)) < 0) {
+perror("setsockopt failed\n");
+}
+if (setsockopt(cios->fd, SOL_SOCKET, SO_KEEPALIVE,
+ &tcp_keepalive_on, sizeof(tcp_keepalive_on)) < 0) {
+perror("setsockopt failed\n");
+}
}
static void nbd_update_server_watch(void)
--
1.8.3.1
Re: [PATCH v2] virtio-scsi: Send "REPORTED LUNS CHANGED" sense data upon disk hotplug events.
On 2022-09-21 16:33:35 +0200, Paolo Bonzini wrote:
> On Fri, Sep 16, 2022 at 3:44 AM Venu Busireddy
> wrote:
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index 41f2a5630173..69194c7ae23c 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -608,7 +608,19 @@ static void virtio_scsi_command_complete(SCSIRequest
> > *r, size_t resid)
> >
> > req->resp.cmd.response = VIRTIO_SCSI_S_OK;
> > req->resp.cmd.status = r->status;
> > -if (req->resp.cmd.status == GOOD) {
> > +if (req->dev->reported_luns_changed &&
> > +(req->req.cmd.cdb[0] != INQUIRY) &&
> > +(req->req.cmd.cdb[0] != REPORT_LUNS) &&
> > +(req->req.cmd.cdb[0] != REQUEST_SENSE)) {
> > +req->dev->reported_luns_changed = false;
> > +req->resp.cmd.resid = 0;
> > +req->resp.cmd.status_qualifier = 0;
> > +req->resp.cmd.status = CHECK_CONDITION;
> > +sense_len = scsi_build_sense(sense,
> > SENSE_CODE(REPORTED_LUNS_CHANGED));
> > +qemu_iovec_from_buf(&req->resp_iov, sizeof(req->resp.cmd),
> > +sense, sense_len);
> > +req->resp.cmd.sense_len = virtio_tswap32(vdev, sense_len);
> > +} else if (req->resp.cmd.status == GOOD) {
> > req->resp.cmd.resid = virtio_tswap32(vdev, resid);
> > } else {
> > req->resp.cmd.resid = 0;
>
> Hi,
>
> a unit attention sense must be sent _instead_ of executing the command.
>
> QEMU already has a function scsi_device_set_ua() that handles
> everything; you have to call it, if reported_luns_changed is true,
> from virtio_scsi_handle_cmd_req_prepare() before scsi_req_new().
>
> It will also skip GET_CONFIGURATION and GET_EVENT_STATUS_NOTIFICATION
> commands which are further special-cased in 4.1.6.1 of the MMC
> specification.
Thanks, Paolo. I will test your suggestion (as soon as I finish what I
am working currently), and get back with either more questions, or with
a v3 post.
Venu
> > @@ -956,6 +968,7 @@ static void virtio_scsi_hotplug(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > virtio_scsi_push_event(s, sd,
> > VIRTIO_SCSI_T_TRANSPORT_RESET,
> > VIRTIO_SCSI_EVT_RESET_RESCAN);
> > +s->reported_luns_changed = true;
> > virtio_scsi_release(s);
> > }
> > }
> > @@ -973,6 +986,7 @@ static void virtio_scsi_hotunplug(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > virtio_scsi_push_event(s, sd,
> > VIRTIO_SCSI_T_TRANSPORT_RESET,
> > VIRTIO_SCSI_EVT_RESET_REMOVED);
> > +s->reported_luns_changed = true;
> > virtio_scsi_release(s);
> > }
> >
> > diff --git a/include/hw/virtio/virtio-scsi.h
> > b/include/hw/virtio/virtio-scsi.h
> > index a36aad9c8695..efbcf9ba069a 100644
> > --- a/include/hw/virtio/virtio-scsi.h
> > +++ b/include/hw/virtio/virtio-scsi.h
> > @@ -81,6 +81,7 @@ struct VirtIOSCSI {
> > SCSIBus bus;
> > int resetting;
> > bool events_dropped;
> > +bool reported_luns_changed;
> >
> > /* Fields for dataplane below */
> > AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
> >
>
Re: [PATCH v2] virtio-scsi: Send "REPORTED LUNS CHANGED" sense data upon disk hotplug events.
On Fri, Sep 16, 2022 at 3:44 AM Venu Busireddy
wrote:
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 41f2a5630173..69194c7ae23c 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -608,7 +608,19 @@ static void virtio_scsi_command_complete(SCSIRequest *r,
> size_t resid)
>
> req->resp.cmd.response = VIRTIO_SCSI_S_OK;
> req->resp.cmd.status = r->status;
> -if (req->resp.cmd.status == GOOD) {
> +if (req->dev->reported_luns_changed &&
> +(req->req.cmd.cdb[0] != INQUIRY) &&
> +(req->req.cmd.cdb[0] != REPORT_LUNS) &&
> +(req->req.cmd.cdb[0] != REQUEST_SENSE)) {
> +req->dev->reported_luns_changed = false;
> +req->resp.cmd.resid = 0;
> +req->resp.cmd.status_qualifier = 0;
> +req->resp.cmd.status = CHECK_CONDITION;
> +sense_len = scsi_build_sense(sense,
> SENSE_CODE(REPORTED_LUNS_CHANGED));
> +qemu_iovec_from_buf(&req->resp_iov, sizeof(req->resp.cmd),
> +sense, sense_len);
> +req->resp.cmd.sense_len = virtio_tswap32(vdev, sense_len);
> +} else if (req->resp.cmd.status == GOOD) {
> req->resp.cmd.resid = virtio_tswap32(vdev, resid);
> } else {
> req->resp.cmd.resid = 0;
Hi,
a unit attention sense must be sent _instead_ of executing the command.
QEMU already has a function scsi_device_set_ua() that handles
everything; you have to call it, if reported_luns_changed is true,
from virtio_scsi_handle_cmd_req_prepare() before scsi_req_new().
It will also skip GET_CONFIGURATION and GET_EVENT_STATUS_NOTIFICATION
commands which are further special-cased in 4.1.6.1 of the MMC
specification.
Thanks,
Paolo
> @@ -956,6 +968,7 @@ static void virtio_scsi_hotplug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> virtio_scsi_push_event(s, sd,
> VIRTIO_SCSI_T_TRANSPORT_RESET,
> VIRTIO_SCSI_EVT_RESET_RESCAN);
> +s->reported_luns_changed = true;
> virtio_scsi_release(s);
> }
> }
> @@ -973,6 +986,7 @@ static void virtio_scsi_hotunplug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> virtio_scsi_push_event(s, sd,
> VIRTIO_SCSI_T_TRANSPORT_RESET,
> VIRTIO_SCSI_EVT_RESET_REMOVED);
> +s->reported_luns_changed = true;
> virtio_scsi_release(s);
> }
>
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index a36aad9c8695..efbcf9ba069a 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -81,6 +81,7 @@ struct VirtIOSCSI {
> SCSIBus bus;
> int resetting;
> bool events_dropped;
> +bool reported_luns_changed;
>
> /* Fields for dataplane below */
> AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
>
Re: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs
Vivek Kasireddy writes:
> The new parameter named "connector" can be used to assign physical
> monitors/connectors to individual GFX VCs such that when the monitor
> is connected or hotplugged, the associated GTK window would be
> fullscreened on it. If the monitor is disconnected or unplugged,
> the associated GTK window would be destroyed and a relevant
> disconnect event would be sent to the Guest.
>
> Usage: -device virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080...
>-display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.
>
> Cc: Dongwon Kim
> Cc: Gerd Hoffmann
> Cc: Daniel P. Berrangé
> Cc: Markus Armbruster
> Cc: Philippe Mathieu-Daudé
> Cc: Marc-André Lureau
> Cc: Thomas Huth
> Signed-off-by: Vivek Kasireddy
> ---
> qapi/ui.json| 9 ++-
> qemu-options.hx | 1 +
> ui/gtk.c| 168
> 3 files changed, 177 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 286c5731d1..86787a4c95 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1199,13 +1199,20 @@
> # interfaces (e.g. VGA and virtual console character devices)
> # by default.
> # Since 7.1
> +# @connector: List of physical monitor/connector names where the GTK
> windows
> +# containing the respective graphics virtual consoles (VCs) are
> +# to be placed. If a mapping exists for a VC, it will be
> +# fullscreened on that specific monitor or else it would not be
> +# displayed anywhere and would appear disconnected to the
> guest.
Let's see whether I understand this... We have VCs numbered 0, 1, ...
VC #i is mapped to the i-th element of @connector, counting from zero.
Correct?
Ignorant question: what's a "physical monitor/connector name"?
Would you mind breaking the lines a bit earlier, between column 70 and
75?
> +# Since 7.2
> #
> # Since: 2.12
> ##
> { 'struct' : 'DisplayGTK',
>'data': { '*grab-on-hover' : 'bool',
> '*zoom-to-fit' : 'bool',
> -'*show-tabs' : 'bool' } }
> +'*show-tabs' : 'bool',
> +'*connector' : ['str'] } }
>
> ##
> # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 31c04f7eea..576b65ef9f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
> #if defined(CONFIG_GTK)
> "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
> "
> [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
> +"[,connector.=]\n"
Is "" a VC number?
> #endif
> #if defined(CONFIG_VNC)
> "-display vnc=[,]\n"
Remainder of my review is on style only. Style suggestions are not
demands :)
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 945c550909..651aaaf174 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -37,6 +37,7 @@
> #include "qapi/qapi-commands-misc.h"
> #include "qemu/cutils.h"
> #include "qemu/main-loop.h"
> +#include "qemu/option.h"
>
> #include "ui/console.h"
> #include "ui/gtk.h"
> @@ -115,6 +116,7 @@
> #endif
>
> #define HOTKEY_MODIFIERS(GDK_CONTROL_MASK | GDK_MOD1_MASK)
> +#define MAX_NUM_ATTEMPTS 5
Could use a comment, and maybe a more telling name (this one doesn't
tell me what is being attempted).
>
> static const guint16 *keycode_map;
> static size_t keycode_maplen;
> @@ -126,6 +128,15 @@ struct VCChardev {
> };
> typedef struct VCChardev VCChardev;
>
> +struct gd_monitor_data {
> +GtkDisplayState *s;
> +GdkDisplay *dpy;
> +GdkMonitor *monitor;
> +QEMUTimer *hp_timer;
> +int attempt;
> +};
> +typedef struct gd_monitor_data gd_monitor_data;
We usually contract these like
typedef struct gd_monitor_data {
...
} gd_monitor_data;
> +
> #define TYPE_CHARDEV_VC "chardev-vc"
> DECLARE_INSTANCE_CHECKER(VCChardev, VC_CHARDEV,
> TYPE_CHARDEV_VC)
> @@ -1385,6 +1396,147 @@ static void gd_menu_untabify(GtkMenuItem *item, void
> *opaque)
> }
> }
>
> +static void gd_monitor_fullscreen(GdkDisplay *dpy, VirtualConsole *vc,
> + gint monitor_num)
> +{
> +GtkDisplayState *s = vc->s;
> +
> +if (!vc->window) {
> +gd_tab_window_create(vc);
> +}
> +gtk_window_fullscreen_on_monitor(GTK_WINDOW(vc->window),
> + gdk_display_get_default_screen(dpy),
> + monitor_num);
> +s->full_screen = TRUE;
> +gd_update_cursor(vc);
> +}
> +
> +static int gd_monitor_lookup(GdkDisplay *dpy, char *label)
> +{
> +GdkMonitor *monitor;
> +const char *monitor_name;
> +int i, total_monitors;
> +
> +total_monitors = gdk_display_get_n_monitors(dpy);
> +for (i = 0; i < total_monitors; i++) {
Suggest to format like this:
int to
[PATCH] try to find out which cluster allocated in qcow2
In our project,we want to full backup a disk only allocated area,but qmp
block-dity-block-add can create a bitmap with all zero,so we can't find out
which cluster is allocated.in qcow2,I think l2_table can help me find out which
cluster should be backup.
Signed-off-by: songlinfeng
---
block/qcow2.c | 49 +
block/qcow2.h | 1 +
2 files changed, 50 insertions(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index c6c6692..944cf4f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4194,6 +4194,55 @@ fail:
return ret;
}
+void qcow2_get_cluster(BlockDriverState *bs, uint64_t size)
+{
+BDRVQcow2State *s = bs->opaque;
+int l1_size = s->l1_size;
+int cluster_size = s->cluster_size;
+int i;
+int j;
+uint64_t *l2_table = (uint64_t *)malloc(cluster_size);
+int l2_entries = cluster_size / sizeof(uint64_t);
+int total = (size + cluster_size + 1) / cluster_size;
+for (i = 0; i < l1_size; i++) {
+uint64_t l1_entry = s->l1_table[i];
+uint64_t l2_offset = l1_entry & L1E_OFFSET_MASK;
+if (l2_offset == 0) {
+if (l2_entries < total) {
+char *buf = (char *)malloc(l2_entries * sizeof(char));
+memset(buf, '0', l2_entries);
+printf("%s", buf);
+free(buf);
+total -= l2_entries;
+} else {
+char *buf = (char *)malloc(total * sizeof(char));
+memset(buf, '0', total);
+printf("%s", buf);
+free(buf);
+total -= total;
+}
+continue;
+}
+int ret = bdrv_pread(bs->file, l2_offset, l2_table, cluster_size);
+if (ret < 0) {
+error_report("can't get l2_table");
+abort();
+}
+for (j = 0; j < l2_entries; j++) {
+if (total) {
+if (l2_table[j] == 0) {
+printf("0");
+} else {
+printf("1");
+}
+total--;
+}
+}
+}
+free(l2_table);
+printf("\n");
+}
+
static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
bool exact, PreallocMode prealloc,
BdrvRequestFlags flags, Error **errp)
diff --git a/block/qcow2.h b/block/qcow2.h
index ba436a8..7079916 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -998,6 +998,7 @@ int
qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp);
bool qcow2_supports_persistent_dirty_bitmap(BlockDriverState *bs);
+void qcow2_get_cluster(BlockDriverState *bs, uint64_t size);
uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *bs,
uint32_t cluster_size);
--
1.8.3.1
Re: [PATCH v2 00/23] target/i386: pc-relative translation blocks
Looks good! Just a couple weird parts of the architecture where I need
some more explanation.
Paolo
On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson
wrote:
>
> This is the x86 specific changes required to reduce the
> amount of translation for address space randomization.
> This is a re-base, with no other significant changes over v1.
>
>
> r~
>
>
> Based-on: 20220906091126.298041-1-richard.hender...@linaro.org
> ("[PATCH v4 0/7] tcg: pc-relative translation blocks")
>
> branch: https://gitlab.com/rth7680/qemu/-/tree/tgt-x86-pcrel
>
>
> Richard Henderson (23):
> target/i386: Remove pc_start
> target/i386: Return bool from disas_insn
> target/i386: Remove cur_eip argument to gen_exception
> target/i386: Remove cur_eip, next_eip arguments to gen_interrupt
> target/i386: Create gen_update_eip_cur
> target/i386: Create gen_update_eip_next
> target/i386: Introduce DISAS_EOB*
> target/i386: Use DISAS_EOB* in gen_movl_seg_T0
> target/i386: Use DISAS_EOB_NEXT
> target/i386: USe DISAS_EOB_ONLY
> target/i386: Create cur_insn_len, cur_insn_len_i32
> target/i386: Remove cur_eip, next_eip arguments to gen_repz*
> target/i386: Introduce DISAS_JUMP
> target/i386: Truncate values for lcall_real to i32
> target/i386: Create eip_next_*
> target/i386: Use DISAS_TOO_MANY to exit after gen_io_start
> target/i386: Create gen_jmp_rel
> target/i386: Use gen_jmp_rel for loop and jecxz insns
> target/i386: Use gen_jmp_rel for gen_jcc
> target/i386: Use gen_jmp_rel for gen_repz*
> target/i386: Use gen_jmp_rel for DISAS_TOO_MANY
> target/i386: Create gen_eip_cur
> target/i386: Enable TARGET_TB_PCREL
>
> target/i386/cpu-param.h | 1 +
> target/i386/helper.h | 2 +-
> target/i386/tcg/seg_helper.c | 6 +-
> target/i386/tcg/tcg-cpu.c| 8 +-
> target/i386/tcg/translate.c | 712 ++-
> 5 files changed, 369 insertions(+), 360 deletions(-)
>
> --
> 2.34.1
>
[PATCH] qga: fix possible memory leak
From: lu zhipeng
Signed-off-by: lu zhipeng
---
qga/main.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/qga/main.c b/qga/main.c
index 5f1efa2333..73ea1aae65 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1287,7 +1287,7 @@ static GAState *initialize_agent(GAConfig *config, int
socket_activation)
if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
g_critical("unable to create (an ancestor of) the state directory"
" '%s': %s", config->state_dir, strerror(errno));
-return NULL;
+goto failed;
}
#endif
@@ -1312,7 +1312,7 @@ static GAState *initialize_agent(GAConfig *config, int
socket_activation)
if (!log_file) {
g_critical("unable to open specified log file: %s",
strerror(errno));
-return NULL;
+goto failed;
}
s->log_file = log_file;
}
@@ -1323,7 +1323,7 @@ static GAState *initialize_agent(GAConfig *config, int
socket_activation)
s->pstate_filepath,
ga_is_frozen(s))) {
g_critical("failed to load persistent state");
-return NULL;
+goto failed;
}
config->blacklist = ga_command_blacklist_init(config->blacklist);
@@ -1344,7 +1344,7 @@ static GAState *initialize_agent(GAConfig *config, int
socket_activation)
#ifndef _WIN32
if (!register_signal_handlers()) {
g_critical("failed to register signal handlers");
-return NULL;
+goto failed;
}
#endif
@@ -1357,12 +1357,21 @@ static GAState *initialize_agent(GAConfig *config, int
socket_activation)
s->wakeup_event = CreateEvent(NULL, TRUE, FALSE, TEXT("WakeUp"));
if (s->wakeup_event == NULL) {
g_critical("CreateEvent failed");
-return NULL;
+goto failed;
}
#endif
ga_state = s;
return s;
+
+failed:
+g_free(s->pstate_filepath);
+g_free(s->state_filepath_isfrozen);
+if (s->log_file && s->log_file != stderr) {
+fclose(s->log_file);
+}
+g_free(s);
+return NULL;
}
static void cleanup_agent(GAState *s)
--
2.31.1
Re: [PATCH v2 23/23] target/i386: Enable TARGET_TB_PCREL
On Tue, Sep 6, 2022 at 12:10 PM Richard Henderson
wrote:
> static void gen_update_eip_cur(DisasContext *s)
> {
> gen_jmp_im(s, s->base.pc_next - s->cs_base);
> +s->pc_save = s->base.pc_next;
s->pc_save is not valid after all gen_jmp_im() calls. Is it worth
noting after each call to gen_jmp_im() why this is not a problem?
> }
>
> static void gen_update_eip_next(DisasContext *s)
> {
> gen_jmp_im(s, s->pc - s->cs_base);
> +s->pc_save = s->pc;
> +}
> +
> +static TCGv gen_eip_cur(DisasContext *s)
> +{
> +if (TARGET_TB_PCREL) {
> +gen_update_eip_cur(s);
> +return cpu_eip;
> +} else {
> +return tcg_constant_tl(s->base.pc_next - s->cs_base);
> +}
Ok, now I see why you called it gen_eip_cur(), but it's still a bit
disconcerting to see the difference in behavior between the
TARGET_TB_PCREL and !TARGET_TB_PCREL cases, one of them updating
cpu_eip and other not.
Perhaps gen_jmp_im() and gen_update_eip_cur() could be rewritten to
return the destination instead:
static TCGv gen_jmp_im(DisasContext *s, target_ulong eip)
{
if (TARGET_TB_PCREL) {
target_ulong eip_save = s->pc_save - s->cs_base;
tcg_gen_addi_tl(cpu_eip, cpu_eip, eip - eip_save);
return cpu_eip;
} else {
TCGv dest = tcg_constant_tl(eip);
tcg_gen_mov_tl(cpu_eip, dest);
return dest;
}
}
static TCGv gen_update_eip_cur(DisasContext *s)
{
TCGv dest = gen_jmp_im(s, s->base.pc_next - s->cs_base);
s->pc_save = s->base.pc_next;
return dest;
}
and the "if (update_ip)" case would use the return value?
This change would basically replace the previous patch, with just the
"if (TARGET_TB_PCREL)" added here.
Paolo
Re: [RFC 2/4] tcg/plugins: Automatically define CURRENT_PLUGIN
Andrew Fasano writes: > Use plugin filenames to set the preprocessor variable CURRENT_PLUGIN > as a string during plugin compilation. > > Signed-off-by: Andrew Fasano > --- > contrib/plugins/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile > index df3499f4f2..b7720fea0f 100644 > --- a/contrib/plugins/Makefile > +++ b/contrib/plugins/Makefile > @@ -34,7 +34,7 @@ CFLAGS += -I$(SRC_PATH)/include/qemu > all: $(SONAMES) > > %.o: %.c > - $(CC) $(CFLAGS) -c -o $@ $< > + $(CC) $(CFLAGS) -DCURRENT_PLUGIN=\"$(basename $@)\" -c -o $@ $< While all plugins are currently single files this seems a little clumsy. We can already check exported plugin symbols in loader.c (see qemu_plugin_version) so maybe it would be better to declare an API update and mandate any plugin object also needs to define a qemu_plugin_name with a null terminated string? > > lib%.so: %.o > $(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS) -- Alex Bennée
Re: [RFC 0/4] Support interactions between TCG plugins
Andrew Fasano writes: > Hello, > > I'm requesting comments on the following series of patches expanding the > TCG plugin system to add the "QEMU Plugin-to-Plugin (QPP)" interface > that allows for interactions between TCG plugins. The goal of this > interface is to enable plugins to expand on other plugins and reduce > code duplication. This patch series includes documentation and > significant comments, but a high-level summary is below along with a > discussion of the current implementation as well as the benefits and > drawbacks of these changes. Thanks for a very detailed cover letter. My initial thoughts are if we are trying to reduce code duplication what about simply using a library and linking it to the final plugin. I guess it depends on how computational effort has been spent in calculating a particular piece of state and if that is avoided by having this IPC mechanism instead of just repeating the calculation. > **Summary** > > The QPP interface allows two types of interactions between plugins: > > 1) Exported functions: A plugin may wish to allow other plugins to call > one of the functions it has defined. To do this, the plugin must mark > the function definition as publicly visible with the QEMU_PLUGIN_EXPORT > macro and place a definition in an included header file using the > QPP_FUN_PROTOTYPE macro. Other plugins can then include this header and > call the exported function by combining the name of the target plugin > with the name of the exported function. > > For example, consider a hypothetical plugin that collects a list of > cache misses. This plugin could export two functions using the QPP > interface: one to allow another plugin to query this list and another > to empty the list. This would enable the development of another plugin > that examines guest CPU state to identify process changes and reports > the cache misses per process. With the QPP interface, this second plugin > would not need to duplicate any logic from the first. Thinking of this concrete example I guess the process change detection is a fairly expensive operation that might be tuned to a particular architecture? Is this something Panda currently derives from plugins instead of the core QEMU code? > 2) Callbacks: Multiple plugins may wish to take some action when some > event of interest occurs inside a running guest. To support modularity > and reduce code duplication, the QPP callback system allows this logic > to be contained in single plugin that detects whenever a given event > occurs and exposes a callback with a given name. Another plugin can then > request to have one of its own functions run whenever this event occurs. > Additional plugins could also use this same callback to run additional > logic whenever this event occurs. > > For example, consider (again) a hypothetical plugin that detects when > the current guest process changes by analyzing the guest CPU state. This > plugin could define a callback named "on_process_change" and trigger > this callback event whenever it detects a process change. Other plugins > could then be developed that take various actions on process changes by > registering internal functions to run on this event. > > These patches and examples are inspired by the PANDA project > (https://panda.re and https://github.com/panda-re/panda), a fork of QEMU > modified to support dynamic program analysis and reverse engineering. > PANDA also includes a large plugin system with a similar interface for > interactions between plugins. I'm one of the maintainers of PANDA > and have seen how the ability for plugins to interact with > other plugins reduces code duplication and enables the creation of many > useful plugins. Would another use-case be to export the PANDA APIs so you could use the existing plugins on an upstream QEMU? > **Implementation Overview** > > These patches modify the TCG plugin build system to define the > preprocessor variable CURRENT_PLUGIN to the name of the current plugin > based off its filename. This can be useful for plugin developers in > general and is used internally in the QPP implementation to determine > if an exported plugin function is defined in the current plugin or > in another. > > These patches also add the function qemu_plugin_name_to_handle to the > core plugin API which uses the new internal function is_plugin_named. > The ability for plugins to get a handle to another plugin is necessary > for the inter-plugin interactions described below. > > The QPP implementation is contained inside a header file plugin-qpp.h > that adds the macros QPP_CREATE_CB, QPP_RUN_CB, QPP_REG_CB, > QPP_REMOVE_CB, and QPP_FUN_PROTOTYPE. The first 4 of these are related > to the callback system and the last one is for exported functions. > > The QPP_CREATE_CB macro is used by a plugin that wishes to create a > callback with a given name. The macro will create an array of function > pointers for every function that has been registered to run on this > callback ev
Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t
Le 20/09/2022 à 18:30, Mark Cave-Ayland a écrit :
On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:
On 17/9/22 14:09, BALATON Zoltan wrote:
On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
There are already 32 feature bits in use, so change the size of the m68k
CPU features to uint64_t (allong with the associated m68k_feature()
functions) to allow up to 64 feature bits to be used.
Signed-off-by: Mark Cave-Ayland
---
target/m68k/cpu.c | 4 ++--
target/m68k/cpu.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index f681be3a2a..7b4797e2f1 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
static void m68k_set_feature(CPUM68KState *env, int feature)
{
- env->features |= (1u << feature);
+ env->features |= (1ul << feature);
env->features = deposit64(env->features, feature, 1, 1);
}
static void m68k_unset_feature(CPUM68KState *env, int feature)
{
- env->features &= (-1u - (1u << feature));
+ env->features &= (-1ul - (1ul << feature));
env->features = deposit64(env->features, feature, 1, 0);
Should these be ull instead of ul?
Yes. Not needed if using the extract/deposit API.
I must admit I find the deposit64() variants not particularly easy to read: if we're considering
alterations rather than changing the constant suffix then I'd much rather go for:
env->features |= (1ULL << feature);
and:
env->features &= ~(1ULL << feature);
Laurent, what would be your preference?
I have no preference, do as you prefer.
}
static void m68k_cpu_reset(DeviceState *dev)
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 67b6c12c28..d3384e5d98 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -154,7 +154,7 @@ typedef struct CPUArchState {
struct {} end_reset_fields;
/* Fields from here on are preserved across CPU reset. */
- uint32_t features;
+ uint64_t features;
} CPUM68KState;
/*
@@ -539,9 +539,9 @@ enum m68k_features {
M68K_FEATURE_TRAPCC,
};
-static inline int m68k_feature(CPUM68KState *env, int feature)
+static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
Why uint64_t? Can we simplify using a boolean?
I don't really feel strongly either way here. Again I'm happy to go with whatever Laurent would
prefer as maintainer.
A boolean seems more logic, I think.
Thanks,
Laurent
Re: [PATCH v2 20/23] target/i386: Use gen_jmp_rel for gen_repz*
On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson
wrote:
>
> Subtract cur_insn_len to restart the current insn.
>
> Signed-off-by: Richard Henderson
I wouldn't mind squashing this with the jecxz/loop patch (and the
review comments there apply here too).
Paolo
> ---
> target/i386/tcg/translate.c | 10 ++
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index e27f36e4e9..7a9e533c6e 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -224,7 +224,6 @@ STUB_HELPER(wrmsr, TCGv_env env)
>
> static void gen_eob(DisasContext *s);
> static void gen_jr(DisasContext *s);
> -static void gen_jmp(DisasContext *s, target_ulong eip);
> static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num);
> static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num);
> static void gen_op(DisasContext *s1, int op, MemOp ot, int d);
> @@ -1277,7 +1276,7 @@ static void gen_repz(DisasContext *s, MemOp ot,
> if (s->repz_opt) {
> gen_op_jz_ecx(s, s->aflag, l2);
> }
> -gen_jmp(s, s->base.pc_next - s->cs_base);
> +gen_jmp_rel(s, MO_32, -cur_insn_len(s), 0);
> }
>
> #define GEN_REPZ(op) \
> @@ -1297,7 +1296,7 @@ static void gen_repz2(DisasContext *s, MemOp ot, int nz,
> if (s->repz_opt) {
> gen_op_jz_ecx(s, s->aflag, l2);
> }
> -gen_jmp(s, s->base.pc_next - s->cs_base);
> +gen_jmp_rel(s, MO_32, -cur_insn_len(s), 0);
> }
>
> #define GEN_REPZ2(op) \
> @@ -2751,11 +2750,6 @@ static void gen_jmp_rel(DisasContext *s, MemOp ot, int
> diff, int tb_num)
> gen_jmp_tb(s, dest, tb_num);
> }
>
> -static void gen_jmp(DisasContext *s, target_ulong eip)
> -{
> -gen_jmp_tb(s, eip, 0);
> -}
> -
> static inline void gen_ldq_env_A0(DisasContext *s, int offset)
> {
> tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0, s->mem_index, MO_LEUQ);
> --
> 2.34.1
>
Re: [RFC 1/4] docs/tcg-plugins: describe QPP API
Andrew Fasano writes:
> Describe how multiple TCG plugins can interact using the QEMU
> Plugin-to-Plugin API (QPP) with both callbacks and direct
> function calls.
Looks ok at first glance. I suspect it is quickly coming to the point we
need to split the examples and the API apart in the docs to stop things
getting too messy.
>
> Signed-off-by: Andrew Fasano
> ---
> docs/devel/tcg-plugins.rst | 76 ++
> 1 file changed, 76 insertions(+)
>
> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
> index a7cc44aa20..7985572027 100644
> --- a/docs/devel/tcg-plugins.rst
> +++ b/docs/devel/tcg-plugins.rst
> @@ -441,3 +441,79 @@ The plugin has a number of arguments, all of them are
> optional:
>associativity of the L2 cache, respectively. Setting any of the L2
>configuration arguments implies ``l2=on``.
>(default: N = 2097152 (2MB), B = 64, A = 16)
> +
> +Plugin-to-Plugin Interactions
> +-
> +
> +Plugins may interact with other plugins through the QEMU Plugin-to-Plugin
> +("QPP") API by including ``qemu/plugin-qpp.h``. This API supports direct
> +function calls between plugins as well as an inter-plugin callback system.
> +This API allows for composition of plugins: plugins can make use of logic in
> +other plugins without the need for code duplication.
> +
> +Plugin names
> +
> +Plugins are automatically given a name by removing the suffix from their
> +filename. These plugin names will be used during QPP interactions as
> +described below. A plugin can access its own name through the preprocessor
> +variable ``CURRENT_PLUGIN``.
> +
> +QPP function calls
> +~~
> +When a plugin (e.g. ``plugin_a``) wishes to make some of its functions (e.g.
> +``func_1``) available to other plugins, it must:
> +
> +1. Mark the function definition with the ``QEMU_PLUGIN_EXPORT`` macro. For
> +example : ``QEMU_PLUGIN_EXPORT int func_1(int x) {...}``.
> +2. Provide prototypes for exported functions in a header file (e.g.
> +``plugin_a.h``) using the macro ``QPP_FUN_PROTOTYPE`` with arguments of the
> +plugin's name, the function's return type, the function's name, and any
> +arguments the function takes. For example:
> +``QPP_FUN_PROTOTYPE(plugin_a, int, func_1, int);``.
> +3. Import this header from the plugin.
> +
> +When other plugins wish to use the functions exported by ``plugin_a``, they
> +must:
> +
> +1. Import the header file (e.g. ``plugin_a.h``) with the function
> prototype(s).
> +2. Call the function when desired by combining the target plugin name, an
> + underscore, and the target function name, e.g. ``plugin_a_func_1()``.
> +
> +QPP callbacks
> +~
> +
> +The QPP API also allows a plugin to define callback events and for other
> plugins
> +to request to be notified whenever these events happens. The plugin that
> defines
> +the callback is responsible for triggering the callback when it so wishes.
> Other
> +plugins that wish to be notified on these events must define a function of an
> +appropriate type and register it to run on this event.
> +
> +When a plugin (e.g. ``plugin_a``) wishes to define a callback (an event that
> +other plugins can request to be notified about), it must:
> +
> +1. Define the callback using the ``QPP_CREATE_CB`` macro with a single
> argument
> + of the callback's name. For example: ``QPP_CREATE_CB(on_some_event);``.
> +2. In a header file (e.g. ``plugin_a.h``) create a prototype for the callback
> + type with ``QPP_CB_PROTOTYPE`` with arguments of the callback's return
> type
> + (only ``void`` is currently supported), the name of the callback, and any
> + arguments the callback function will be called with. For example with a
> + callback named ``on_some_event`` that returns a void and takes an int and
> + a bool as an argument, you would use: ``QPP_CB_PROTOTYPE(void,
> + on_some_event, int, bool)``.
> +3. Import this header from the plugin.
> +4. When the plugin wishes to run any registered callback functions, it should
> + use the macro ``QPP_RUN_CB`` with the first argument set to the callback
> + name followed by the arguments as specified in the header. For example:
> + ``QPP_RUN_CB(on_some_event, 2, true);``.
> +
> +When other plugins wish to register a function to run on such an event, they
> +must:
> +
> +1. Import the header file with the callback prototype(s) (e.g.
> ``plugin_a.h``)
> +2. Define a function that matches the callback signature. For example:
> + ``void plugin_b_callback(int, bool) {...}``.
> +3. Register this function to be run on the callback using the ``QPP_REG_CB``
> + macro with the first argument being the name of the plugin that provides
> the
> + callback (as a string), the second being the callback name, and the third
> as
> + the function that should be run. For example: ``QPP_REG_CB("plugin_a",
> + on_some_event, plugin_b_callback);``
--
Alex Bennée
Re: [PATCH v3 4/5] acpi/nvdimm: Implement ACPI NVDIMM Label Methods
On Tue, 20 Sep 2022 20:28:31 +0800
Robert Hoo wrote:
> On Tue, 2022-09-20 at 11:13 +0200, Igor Mammedov wrote:
> > On Fri, 16 Sep 2022 21:15:35 +0800
> > Robert Hoo wrote:
> >
> > > On Fri, 2022-09-16 at 09:37 +0200, Igor Mammedov wrote:
> > >
> > > > > Fine, get your point now.
> > > > > In ASL it will look like this:
> > > > > Local1 = Package (0x3) {STTS, SLSA, MAXT}
> > > > > Return (Local1)
> > > >
> > > >
> > > > >
> > > > > But as for
> > > > > CreateDWordField (Local0, Zero, STTS) //
> > > > > Status
> > > > > CreateDWordField (Local0, 0x04, SLSA) //
> > > > > SizeofLSA
> > > > > CreateDWordField (Local0, 0x08, MAXT) //
> > > > > Max
> > > > > Trans
> > > > >
> > > > > I cannot figure out how to substitute with LocalX. Can you shed
> > > > > more
> > > > > light?
> > > >
> > > > Leave this as is, there is no way to make it anonymous/local with
> > > > FooField.
> > > >
> > > > (well one might try to use Index and copy field's bytes into a
> > > > buffer
> > > > and
> > > > then explicitly convert to Integer, but that's a rather
> > > > convoluted
> > > > way
> > > > around limitation so I'd not go this route)
> > > >
> > >
> > > OK, pls. take a look, how about this?
> > >
> > > Method (_LSI, 0, Serialized) // _LSI: Label Storage Information
> > > {
> > > Local0 = NCAL (ToUUID("4309ac30-0d11-11e4-9191-0800200c9a66"),
> > > 0x02, 0x04, Zero, One)// Buffer
> > > CreateDWordField (Local0, Zero, STTS) // Status
> > > CreateDWordField (Local0, 0x04, SLSA) // Size of LSA
> > > CreateDWordField (Local0, 0x08, MAXT) // Max Transfer Size
> > > Local1 = Package (0x3) {STTS, SLSA, MAXT}
> > > Return (Local1)
> > > }
> > >
> > > Method (_LSR, 2, Serialized) // _LSR: Label Storage Read
> > > {
> > > Name (INPT, Buffer(8) {})
> > > CreateDWordField (INPT, Zero, OFST);
> > > CreateDWordField (INPT, 4, LEN);
> >
> > why do you have to create and use INPT, wouldn't local be enough to
> > keep the buffer?
>
> If substitute INPT with LocalX, then later
> Local0 = Package (0x01) {LocalX} isn't accepted.
>
> PackageElement :=
> DataObject | NameString
ok, then respin series and lets get it some testing.
BTW:
it looks like Windows Server starting from v2019 has support for
NVDIMM-P devices which came with 'Optane DC Persistent Memory Modules'
but it fails to recognize NVDIMMs in QEMU (complaining something about
wrong target). Perhaps you can reach someone with Optane/ACPI
expertise within your org and try to fix QEMU side.
> >
> > > OFST = Arg0
> > > LEN = Arg1
> > > Local0 = Package (0x01) {INPT}
> > > Local3 = NCAL (ToUUID("4309ac30-0d11-11e4-9191-0800200c9a66"),
> > > 0x02, 0x05, Local0, One)
> > > CreateDWordField (Local3, Zero, STTS)
> > > CreateField (Local3, 32, LEN << 3, LDAT)
> > > Local1 = Package (0x2) {STTS, toBuffer(LDAT)}
> > > Return (Local1)
> > > }
> > >
> > > Method (_LSW, 3, Serialized) // _LSW: Label Storage Write
> > > {
> > > Local2 = Arg2
> > > Name (INPT, Buffer(8) {})
> >
> > ditto
> >
> > > CreateDWordField (INPT, Zero, OFST);
> > > CreateDWordField (INPT, 4, TLEN);
> > > OFST = Arg0
> > > TLEN = Arg1
> > > Concatenate(INPT, Local2, INPT)
> > > Local0 = Package (0x01)
> > > {
> > > INPT
> > > }
> > > Local3 = NCAL (ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66"),
> > > 0x02, 0x06, Local0, One)
> > > CreateDWordField (Local3, 0, STTS)
> > > Return (STTS)
> > > }
>
>
Re: [PATCH v2 18/23] target/i386: Use gen_jmp_rel for loop and jecxz insns
On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson
wrote:
>
> With gen_jmp_rel, we may chain to the next tb
> instead of merely writing to eip and exiting.
>
> Signed-off-by: Richard Henderson
See comment on the previous patch.
Paolo
> ---
> target/i386/tcg/translate.c | 21 ++---
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index 07c7764649..fdd17c3cf3 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -7355,24 +7355,18 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
> case 0xe2: /* loop */
> case 0xe3: /* jecxz */
> {
> -TCGLabel *l1, *l2, *l3;
> -
> -tval = (int8_t)insn_get(env, s, MO_8);
> -tval += s->pc - s->cs_base;
> -if (dflag == MO_16) {
> -tval &= 0x;
> -}
> +TCGLabel *l1, *l2;
> +int diff = (int8_t)insn_get(env, s, MO_8);
>
> l1 = gen_new_label();
> l2 = gen_new_label();
> -l3 = gen_new_label();
> gen_update_cc_op(s);
> b &= 3;
> switch(b) {
> case 0: /* loopnz */
> case 1: /* loopz */
> gen_op_add_reg_im(s, s->aflag, R_ECX, -1);
> -gen_op_jz_ecx(s, s->aflag, l3);
> +gen_op_jz_ecx(s, s->aflag, l2);
> gen_jcc1(s, (JCC_Z << 1) | (b ^ 1), l1);
> break;
> case 2: /* loop */
> @@ -7385,14 +7379,11 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
> break;
> }
>
> -gen_set_label(l3);
> -gen_update_eip_next(s);
> -tcg_gen_br(l2);
> +gen_set_label(l2);
> +gen_jmp_rel(s, MO_32, 0, 1);
>
> gen_set_label(l1);
> -gen_jmp_im(s, tval);
> -gen_set_label(l2);
> -s->base.is_jmp = DISAS_EOB_ONLY;
> +gen_jmp_rel(s, dflag, diff, 0);
> }
> break;
> case 0x130: /* wrmsr */
> --
> 2.34.1
>
Re: [PATCH 13/14] migration: Remove old preempt code around state maintainance
On Tue, Sep 20, 2022 at 08:47:20PM -0400, Peter Xu wrote:
> On Tue, Sep 20, 2022 at 06:52:27PM -0400, Peter Xu wrote:
> > With the new code to send pages in rp-return thread, there's little help to
> > keep lots of the old code on maintaining the preempt state in migration
> > thread, because the new way should always be faster..
> >
> > Then if we'll always send pages in the rp-return thread anyway, we don't
> > need those logic to maintain preempt state anymore because now we serialize
> > things using the mutex directly instead of using those fields.
> >
> > It's very unfortunate to have those code for a short period, but that's
> > still one intermediate step that we noticed the next bottleneck on the
> > migration thread. Now what we can do best is to drop unnecessary code as
> > long as the new code is stable to reduce the burden. It's actually a good
> > thing because the new "sending page in rp-return thread" model is (IMHO)
> > even cleaner and with better performance.
> >
> > Remove the old code that was responsible for maintaining preempt states, at
> > the meantime also remove x-postcopy-preempt-break-huge parameter because
> > with concurrent sender threads we don't really need to break-huge anymore.
> >
> > Signed-off-by: Peter Xu
> > ---
> > migration/migration.c | 2 -
> > migration/ram.c | 258 +-
> > 2 files changed, 3 insertions(+), 257 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index fae8fd378b..698fd94591 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -4399,8 +4399,6 @@ static Property migration_properties[] = {
> > DEFINE_PROP_SIZE("announce-step", MigrationState,
> >parameters.announce_step,
> >DEFAULT_MIGRATE_ANNOUNCE_STEP),
> > -DEFINE_PROP_BOOL("x-postcopy-preempt-break-huge", MigrationState,
> > - postcopy_preempt_break_huge, true),
>
> Forgot to drop the variable altogether:
>
> diff --git a/migration/migration.h b/migration/migration.h
> index cdad8aceaa..ae4ffd3454 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -340,13 +340,6 @@ struct MigrationState {
> bool send_configuration;
> /* Whether we send section footer during migration */
> bool send_section_footer;
> -/*
> - * Whether we allow break sending huge pages when postcopy preempt is
> - * enabled. When disabled, we won't interrupt precopy within sending a
> - * host huge page, which is the old behavior of vanilla postcopy.
> - * NOTE: this parameter is ignored if postcopy preempt is not enabled.
> - */
> -bool postcopy_preempt_break_huge;
>
> /* Needed by postcopy-pause state */
> QemuSemaphore postcopy_pause_sem;
>
> Will squash this in in next version.
Two more varialbes to drop, as attached..
--
Peter Xu
>From b3308e34398e21c19bd36ec21aae9c7f9f623d75 Mon Sep 17 00:00:00 2001
From: Peter Xu
Date: Wed, 21 Sep 2022 09:51:55 -0400
Subject: [PATCH] fixup! migration: Remove old preempt code around state
maintainance
Content-type: text/plain
Signed-off-by: Peter Xu
---
migration/ram.c | 33 -
1 file changed, 33 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 03bf2324ab..2599eee070 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -97,28 +97,6 @@ struct PageSearchStatus {
unsigned long page;
/* Set once we wrap around */
bool complete_round;
-/*
- * [POSTCOPY-ONLY] Whether current page is explicitly requested by
- * postcopy. When set, the request is "urgent" because the dest QEMU
- * threads are waiting for us.
- */
-bool postcopy_requested;
-/*
- * [POSTCOPY-ONLY] The target channel to use to send current page.
- *
- * Note: This may _not_ match with the value in postcopy_requested
- * above. Let's imagine the case where the postcopy request is exactly
- * the page that we're sending in progress during precopy. In this case
- * we'll have postcopy_requested set to true but the target channel
- * will be the precopy channel (so that we don't split brain on that
- * specific page since the precopy channel already contains partial of
- * that page data).
- *
- * Besides that specific use case, postcopy_target_channel should
- * always be equal to postcopy_requested, because by default we send
- * postcopy pages via postcopy preempt channel.
- */
-bool postcopy_target_channel;
/* Whether we're sending a host page */
bool host_page_sending;
/* The start/end of current host page. Invalid if
host_page_sending==false */
@@ -1573,13 +1551,6 @@ retry:
*/
static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
{
-/*
- * This is not a postcopy requested page, mark it "not urgent", and use
- * preco
Re: [PATCH v2] hw/acpi: Add ospm_status hook implementation for acpi-ged
On Tue, 20 Sep 2022 14:15:36 +0100
Peter Maydell wrote:
> On Wed, 24 Aug 2022 at 16:04, Igor Mammedov wrote:
> >
> > On Tue, 16 Aug 2022 17:49:57 +0800
> > Keqian Zhu wrote:
> >
> > > Setup an ARM virtual machine of machine virt and execute qmp
> > > "query-acpi-ospm-status"
> > > causes segmentation fault with following dumpstack:
> > > #1 0xab64235c in qmp_query_acpi_ospm_status
> > > (errp=errp@entry=0xf030) at ../monitor/qmp-cmds.c:312
> > > #2 0xabfc4e20 in qmp_marshal_query_acpi_ospm_status
> > > (args=, ret=0xea4ffe90, errp=0xea4ffe88) at
> > > qapi/qapi-commands-acpi.c:63
> > > #3 0xabff8ba0 in do_qmp_dispatch_bh (opaque=0xea4ffe98) at
> > > ../qapi/qmp-dispatch.c:128
> > > #4 0xac02e594 in aio_bh_call (bh=0xe0004d80) at
> > > ../util/async.c:150
> > > #5 aio_bh_poll (ctx=ctx@entry=0xad0f6040) at ../util/async.c:178
> > > #6 0xac00bd40 in aio_dispatch (ctx=ctx@entry=0xad0f6040) at
> > > ../util/aio-posix.c:421
> > > #7 0xac02e010 in aio_ctx_dispatch (source=0xad0f6040,
> > > callback=, user_data=) at
> > > ../util/async.c:320
> > > #8 0xf76f6884 in g_main_context_dispatch () at
> > > /usr/lib64/libglib-2.0.so.0
> > > #9 0xac0452d4 in glib_pollfds_poll () at ../util/main-loop.c:297
> > > #10 os_host_main_loop_wait (timeout=0) at ../util/main-loop.c:320
> > > #11 main_loop_wait (nonblocking=nonblocking@entry=0) at
> > > ../util/main-loop.c:596
> > > #12 0xab5c9e50 in qemu_main_loop () at ../softmmu/runstate.c:734
> > > #13 0xab185370 in qemu_main (argc=argc@entry=47,
> > > argv=argv@entry=0xf518, envp=envp@entry=0x0) at
> > > ../softmmu/main.c:38
> > > #14 0xab16f99c in main (argc=47, argv=0xf518) at
> > > ../softmmu/main.c:47
> > >
> > > Fixes: ebb62075021a ("hw/acpi: Add ACPI Generic Event Device Support")
> > > Signed-off-by: Keqian Zhu
> >
> > Reviewed-by: Igor Mammedov
>
> I notice this doesn't seem to have gone in yet -- whose tree is it
> going to go via?
I'd guess ARM tree (due to almost sole user virt-arm).
(there are toy users like microvm and new loongarch)
>
> thanks
> -- PMM
>
Re: [PATCH 1/2] target/m68k: Fix MACSR to CCR
Le 13/09/2022 à 16:28, Richard Henderson a écrit :
First, we were writing to the entire SR register, instead
of only the flags portion. Second, we were not clearing C
as per the documentation (X was cleared via the 0xf mask).
Signed-off-by: Richard Henderson
---
target/m68k/translate.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 5098f7e570..87044382c3 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -5892,8 +5892,10 @@ DISAS_INSN(from_mext)
DISAS_INSN(macsr_to_ccr)
{
TCGv tmp = tcg_temp_new();
-tcg_gen_andi_i32(tmp, QREG_MACSR, 0xf);
-gen_helper_set_sr(cpu_env, tmp);
+
+/* Note that X and C are always cleared. */
+tcg_gen_andi_i32(tmp, QREG_MACSR, CCF_N | CCF_Z | CCF_V);
+gen_helper_set_ccr(cpu_env, tmp);
tcg_temp_free(tmp);
set_cc_op(s, CC_OP_FLAGS);
}
Applied to my m68k-for-7.2 branch
Thanks,
Laurent
