Re: [PATCH] build-sys: fix meson project version usage
On Thu, Nov 2, 2023 at 6:11 PM wrote:
>
> From: Marc-André Lureau
>
> Program wixl found: YES (/usr/bin/wixl)
>
> ../qga/meson.build:149:16: ERROR: Unknown variable "project".
>
> Fixes: e20d68aa0b9 ("configure, meson: use command line options to configure
> qemu-ga")
> Signed-off-by: Marc-André Lureau
ping
> ---
> qga/meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qga/meson.build b/qga/meson.build
> index 940a51d55d..ff7a8496e4 100644
> --- a/qga/meson.build
> +++ b/qga/meson.build
> @@ -146,7 +146,7 @@ if targetos == 'windows'
>libpcre = 'libpcre2'
> endif
> qga_msi_version = get_option('qemu_ga_version') == '' \
> - ? project.version() \
> + ? meson.project_version() \
>: get_option('qemu_ga_version')
> qga_msi = custom_target('QGA MSI',
> input: files('installer/qemu-ga.wxs'),
> --
> 2.41.0
>
>
--
Marc-André Lureau
Re: [PATCH] ui/console: fix default VC when there are no display
Hi
On Wed, Nov 8, 2023 at 5:37 PM wrote:
>
> From: Marc-André Lureau
>
> When display is "none", we may still have remote displays (I think it
> would be simpler if VNC/Spice were regular display btw). Return the
> default VC then, and set them up to fix a regression when using remote
> display and it used the TTY instead.
>
> Fixes: commit 1bec1cc0d ("ui/console: allow to override the default VC")
> Reported-by: German Maglione
> Signed-off-by: Marc-André Lureau
German, did you file an issue on gitlab? Could you check/test this patch?
thanks
> ---
> system/vl.c | 4 +++-
> ui/console.c | 14 --
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index bd7fad770b..8c522a07da 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -1359,6 +1359,7 @@ static void qemu_setup_display(void)
> dpy.type = DISPLAY_TYPE_NONE;
> #if defined(CONFIG_VNC)
> vnc_parse("localhost:0,to=99,id=default");
> +display_remote++;
> #endif
> }
> }
> @@ -1391,7 +1392,8 @@ static void qemu_create_default_devices(void)
> }
> }
>
> -if (nographic || (!vc && !is_daemonized() && isatty(STDOUT_FILENO))) {
> +if (nographic ||
> +((!display_remote || !vc) && !is_daemonized() &&
> isatty(STDOUT_FILENO))) {
> if (default_parallel) {
> add_device_config(DEV_PARALLEL, "null");
> }
> diff --git a/ui/console.c b/ui/console.c
> index 8e688d3569..f08c8365b0 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1679,19 +1679,21 @@ void qemu_display_init(DisplayState *ds,
> DisplayOptions *opts)
>
> const char *qemu_display_get_vc(DisplayOptions *opts)
> {
> +#ifdef CONFIG_PIXMAN
> +const char *vc = "vc:80Cx24C";
> +#else
> +const char *vc = NULL;
> +#endif
> +
> assert(opts->type < DISPLAY_TYPE__MAX);
> if (opts->type == DISPLAY_TYPE_NONE) {
> -return NULL;
> +return vc;
> }
> assert(dpys[opts->type] != NULL);
> if (dpys[opts->type]->vc) {
> return dpys[opts->type]->vc;
> -} else {
> -#ifdef CONFIG_PIXMAN
> -return "vc:80Cx24C";
> -#endif
> }
> -return NULL;
> +return vc;
> }
>
> void qemu_display_help(void)
> --
> 2.41.0
>
Re: [PATCH v2 1/2] buildsys: Bump minimal meson version required to v1.2.3
On Thu, Nov 9, 2023 at 11:00 PM BALATON Zoltan wrote: > So these are packaged with the source. I did not know that. Now I get what > you mean by s/downolads/installs/ in your reply. But then this is disabled > by --disable-download but actually downloading avocado isn't? Confusing. Yes, that's mostly historical because avocado didn't use mkvenv.py. I'll fix it. Paolo
Re: [SeaBIOS] [PATCH v5] limit physical address space size
On 11/8/23 19:35, Kevin O'Connor wrote:
> On Tue, Nov 07, 2023 at 02:03:09PM +0100, Gerd Hoffmann wrote:
>> For better compatibility with old linux kernels,
>> see source code comment.
>>
>> Related (same problem in ovmf):
>> https://github.com/tianocore/edk2/commit/c1e853769046
>
> Thanks. I'll defer to your judgement on this. It does seem a little
> odd to alter the CPUPhysBits variable itself instead of adding
> additional checking to the pciinit.c code that uses CPUPhysBits.
> (That is, if there are old Linux kernels that can't handle a very high
> PCI space, then a workaround in the PCI code might make it more clear
> what is occurring.)
>
> Cheers,
> -Kevin
>
>
>>
>> Cc: Claudio Fontana
>> Signed-off-by: Gerd Hoffmann
Hi,
I thought about this, and I am not sure it's the right call though.
The change breaking old guests (CentOS7, Ubuntu 18.04 are the known ones, but
presumably others as well) in QEMU is:
commit 784155cdcb02ffaae44afecab93861070e7d652d
Author: Gerd Hoffmann
Date: Mon Sep 11 17:20:26 2023 +0200
seabios: update submodule to git snapshot
git shortlog
Gerd Hoffmann (7):
disable array bounds warning
better kvm detection
detect physical address space size
move 64bit pci window to end of address space
be less conservative with the 64bit pci io window
qemu: log reservations in fw_cfg e820 table
check for e820 conflict
The reasoning for the change is according to:
https://mail.coreboot.org/hyperkitty/list/seab...@seabios.org/message/BHK67ULKJTLJT676J4D5C2ND53CFEL3Q/
"It makes seabios follow a common pattern. real mode address space
has io resources mapped high (below 1M). 32-bit address space has
io resources mapped high too (below 4G). This does the same for
64-bit resources."
So it seems to be an almost aesthetic choice, the way I read the "common
pattern", one that ends up actually breaking existing workloads though.
Now the correction to that that you propose in SeaBIOS is:
>> ---
>> src/fw/paravirt.c | 8
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
>> index e5d4eca0cb5a..a2c9c64d5e78 100644
>> --- a/src/fw/paravirt.c
>> +++ b/src/fw/paravirt.c
>> @@ -182,6 +182,14 @@ static void physbits(int qemu_quirk)
>> __func__, signature, pae ? "yes" : "no", CPULongMode ? "yes" :
>> "no",
>> physbits, valid ? "yes" : "no");
>>
>> +if (valid && physbits > 46) {
>> +// Old linux kernels have trouble dealing with more than 46
>> +// phys-bits, so avoid that for now. Seems to be a bug in the
>> +// virtio-pci driver. Reported: centos-7, ubuntu-18.04
>> +dprintf(1, "%s: using phys-bits=46 (old linux kernel
>> compatibility)\n", __func__);
>> +physbits = 46;
>> +}
>> +
>> if (valid)
>> CPUPhysBits = physbits;
>> }
>> --
>> 2.41.0
but to me this is potentially breaking the situation for another set of users,
those that are passing through 48 physical bits from their host cpu to the
guest,
and expect it to actually happen.
Would it not be a better solution to instead either revert the original change,
or to patch it to find a new range that better satisfies code
consistency/aesthetic requirements,
but also does not break any running workload?
I could help with the testing for this.
Or we could add some extra workarounds in the stack elsewhere in the management
tools to
try to detect older guests (not ideal either), but this seems like adding
breakage on top of breakage to me.
Might be wrong though, looking forward for opinions across Seabios and QEMU.
Thanks,
Claudio
[PATCH for-8.2] test-resv-mem: Fix CID 1523911
Coverity complains about passing "&expected" to "run_range_inverse_array",
which dereferences null "expected". I guess the problem is that the
compare_ranges() loop dereferences 'e' without testing it. However the
loop condition is based on 'ranges' which is garanteed to have
the same length as 'expected' given the g_assert_cmpint() just
before the loop. So the code looks safe to me.
Nevertheless adding a test on expected before the loop to get rid of the
warning.
Fixes: CID 1523901
Signed-off-by: Eric Auger
Reported-by: Coverity (CID 1523901)
---
Hope this fixes the Coverity warning as I cannot test.
---
tests/unit/test-resv-mem.c | 4
1 file changed, 4 insertions(+)
diff --git a/tests/unit/test-resv-mem.c b/tests/unit/test-resv-mem.c
index 5963274e2c..cd8f7318cc 100644
--- a/tests/unit/test-resv-mem.c
+++ b/tests/unit/test-resv-mem.c
@@ -44,6 +44,10 @@ static void compare_ranges(const char *prefix, GList *ranges,
print_ranges("out", ranges);
print_ranges("expected", expected);
#endif
+if (!expected) {
+g_assert_true(!ranges);
+return;
+}
g_assert_cmpint(g_list_length(ranges), ==, g_list_length(expected));
for (l = ranges, e = expected; l ; l = l->next, e = e->next) {
Range *r = (Range *)l->data;
--
2.41.0
Re: [PATCH v5 11/20] vfio/platform: Allow the selection of a given iommu backend
On 11/9/23 12:45, Zhenzhong Duan wrote:
Now we support two types of iommu backends, let's add the capability
to select one of them. This depends on whether an iommufd object has
been linked with the vfio-platform device:
If the user wants to use the legacy backend, it shall not
link the vfio-platform device with any iommufd object:
-device vfio-platform,host=XXX
This is called the legacy mode/backend.
If the user wants to use the iommufd backend (/dev/iommu) it
shall pass an iommufd object id in the vfio-platform device options:
-object iommufd,id=iommufd0
-device vfio-platform,host=XXX,iommufd=iommufd0
Suggested-by: Alex Williamson
Signed-off-by: Zhenzhong Duan
---
include/hw/vfio/vfio-platform.h | 1 +
hw/vfio/platform.c | 5 +
2 files changed, 6 insertions(+)
diff --git a/include/hw/vfio/vfio-platform.h b/include/hw/vfio/vfio-platform.h
index c414c3dffc..f57f4276f2 100644
--- a/include/hw/vfio/vfio-platform.h
+++ b/include/hw/vfio/vfio-platform.h
@@ -18,6 +18,7 @@
#include "hw/sysbus.h"
#include "hw/vfio/vfio-common.h"
+#include "sysemu/iommufd.h"
#include "qemu/event_notifier.h"
#include "qemu/queue.h"
#include "qom/object.h"
I think we can move this change including "sysemu/iommufd.h" to file
"hw/vfio/platform.c"
Thanks,
C.
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 8e3d4ac458..86e176ee97 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -15,6 +15,7 @@
*/
#include "qemu/osdep.h"
+#include CONFIG_DEVICES /* CONFIG_IOMMUFD */
#include "qapi/error.h"
#include
#include
@@ -649,6 +650,10 @@ static Property vfio_platform_dev_properties[] = {
DEFINE_PROP_UINT32("mmap-timeout-ms", VFIOPlatformDevice,
mmap_timeout, 1100),
DEFINE_PROP_BOOL("x-irqfd", VFIOPlatformDevice, irqfd_allowed, true),
+#ifdef CONFIG_IOMMUFD
+DEFINE_PROP_LINK("iommufd", VFIOPlatformDevice, vbasedev.iommufd,
+ TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
+#endif
DEFINE_PROP_END_OF_LIST(),
};
[PATCH] hw/arm/virt: fix GIC maintenance IRQ registration
Since commit 9036e917f8 ("{include/}hw/arm: refactor virt PPI logic"),
GIC maintenance IRQ registration fails on arm64:
[0.979743] kvm [1]: Cannot register interrupt 9
That commit re-defined VIRTUAL_PMU_IRQ to be a INTID but missed a case
where the maintenance IRQ is actually referred by its PPI index. Just
like commit fa68ecb330db ("hw/arm/virt: fix PMU IRQ registration"), use
INITID_TO_PPI(). A search of "GIC_FDT_IRQ_TYPE_PPI" indicates that there
shouldn't be more similar issues.
Fixes: 9036e917f8 ("{include/}hw/arm: refactor virt PPI logic")
Signed-off-by: Jean-Philippe Brucker
---
hw/arm/virt.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 783d71a1b3..f5e685b060 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -591,7 +591,8 @@ static void fdt_add_gic_node(VirtMachineState *vms)
if (vms->virt) {
qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
- GIC_FDT_IRQ_TYPE_PPI, ARCH_GIC_MAINT_IRQ,
+ GIC_FDT_IRQ_TYPE_PPI,
+ INTID_TO_PPI(ARCH_GIC_MAINT_IRQ),
GIC_FDT_IRQ_FLAGS_LEVEL_HI);
}
} else {
@@ -615,7 +616,8 @@ static void fdt_add_gic_node(VirtMachineState *vms)
2, vms->memmap[VIRT_GIC_VCPU].base,
2, vms->memmap[VIRT_GIC_VCPU].size);
qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
- GIC_FDT_IRQ_TYPE_PPI, ARCH_GIC_MAINT_IRQ,
+ GIC_FDT_IRQ_TYPE_PPI,
+ INTID_TO_PPI(ARCH_GIC_MAINT_IRQ),
GIC_FDT_IRQ_FLAGS_LEVEL_HI);
}
}
--
2.42.0
RE: [PATCH v5 11/20] vfio/platform: Allow the selection of a given iommu backend
>-Original Message- >From: Cédric Le Goater >Sent: Friday, November 10, 2023 4:50 PM >Subject: Re: [PATCH v5 11/20] vfio/platform: Allow the selection of a given >iommu backend > >On 11/9/23 12:45, Zhenzhong Duan wrote: >> Now we support two types of iommu backends, let's add the capability >> to select one of them. This depends on whether an iommufd object has >> been linked with the vfio-platform device: >> >> If the user wants to use the legacy backend, it shall not >> link the vfio-platform device with any iommufd object: >> >> -device vfio-platform,host=XXX >> >> This is called the legacy mode/backend. >> >> If the user wants to use the iommufd backend (/dev/iommu) it >> shall pass an iommufd object id in the vfio-platform device options: >> >> -object iommufd,id=iommufd0 >> -device vfio-platform,host=XXX,iommufd=iommufd0 >> >> Suggested-by: Alex Williamson >> Signed-off-by: Zhenzhong Duan >> --- >> include/hw/vfio/vfio-platform.h | 1 + >> hw/vfio/platform.c | 5 + >> 2 files changed, 6 insertions(+) >> >> diff --git a/include/hw/vfio/vfio-platform.h >> b/include/hw/vfio/vfio-platform.h >> index c414c3dffc..f57f4276f2 100644 >> --- a/include/hw/vfio/vfio-platform.h >> +++ b/include/hw/vfio/vfio-platform.h >> @@ -18,6 +18,7 @@ >> >> #include "hw/sysbus.h" >> #include "hw/vfio/vfio-common.h" >> +#include "sysemu/iommufd.h" >> #include "qemu/event_notifier.h" >> #include "qemu/queue.h" >> #include "qom/object.h" >I think we can move this change including "sysemu/iommufd.h" to file >"hw/vfio/platform.c" Make sense, will do. Thanks Zhenzhong
Re: [PATCH] docs: document what configure does with virtual environments
On Thu, Nov 9, 2023 at 10:35 PM John Snow wrote: > > > +The venv resides in the ``pyvenv`` directory in the build tree, > > > +and provides consistency in how the build process runs Python code. > > > +In particular it avoids a potential mismatch, where Meson and Sphinx > > > > I think you can drop the comma. This is so pedantic that if you left > > it in to spite me, I'd not blame you. :) I'll keep it then. :) > > should we say ``--sphinx-build``? Yes, typo. > > I also might say "does not ^necessarily pick the ..." because they > > could be the same, it just isn't the criteria it uses to choose them. I'll replace "pick" with "look for". > > > +If QEMU does not find a dependency, check that it was installed in the > > > +right ``site-packages`` directory or with the right ``pip`` program. > > > > I don't actually know what this means. >_> It's meant to explain what happened with homebrew. Rephrased in v2: This avoids a potential mismatch, where Meson and Sphinx binaries on the PATH might operate in a different Python environment than the one chosen by the user during the build process. On the other hand, it introduces a potential source of confusion where the user installs a dependency but ``configure`` is not able to find it. When this happens, the dependency was installed in the ``site-packages`` directory of another interpreter, or with the wrong ``pip`` program. Paolo
Re: [PATCH] include/hw/xen: Use more inclusive language in comment
On Thu, 2023-11-09 at 18:40 +0100, Thomas Huth wrote: > Let's improve the wording here. > > Signed-off-by: Thomas Huth Absolutely, but please can we change it in Xen first because these headers are a direct import. Acked-by: David Woodhouse > --- > include/hw/xen/interface/hvm/params.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/hw/xen/interface/hvm/params.h > b/include/hw/xen/interface/hvm/params.h > index a22b4ed45d..9bcb40284c 100644 > --- a/include/hw/xen/interface/hvm/params.h > +++ b/include/hw/xen/interface/hvm/params.h > @@ -255,7 +255,7 @@ > * Note that 'mixed' mode has not been evaluated for safety from a > * security perspective. Before using this mode in a > * security-critical environment, each subop should be evaluated for > - * safety, with unsafe subops blacklisted in XSM. > + * safety, with unsafe subops blocked in XSM. > */ > #define HVM_PARAM_ALTP2M 35 > #define XEN_ALTP2M_disabled 0 smime.p7s Description: S/MIME cryptographic signature
[PATCH 0/2] Add QEMU_WARN_UNUSED_RESULT function attribute
Functions that return a value to indicate success or failure can be decorated with the warn_unused_result attribute. GCC will stop compilation if a caller does not check the return value after calling such a function. This was not possible to spot statically before, but Coverity detects this kind of bug. I was prompted by https://lore.kernel.org/qemu-devel/cafeaca_ts-b0gc-duyt6baknm8uauhsx3rw2dmvnugttovj...@mail.gmail.com/ to prevent this from happening in the future. This patch series depends on <20231109162034.2108018-1-manos.pitsidiana...@linaro.org> https://lore.kernel.org/qemu-devel/20231109162034.2108018-1-manos.pitsidiana...@linaro.org/ Manos Pitsidianakis (2): Add QEMU_WARN_UNUSED_RESULT attribute Add warn_unused_result attr to AUD_register_card audio/audio.h | 2 +- hw/arm/omap2.c | 8 +++- hw/input/tsc210x.c | 8 +++- include/qemu/compiler.h | 14 ++ 4 files changed, 29 insertions(+), 3 deletions(-) base-commit: ad6ef0a42e314a8c6ac6c96d5f6e607a1e5644b5 prerequisite-patch-id: 484ec9f7f6109c10d4be0484fe8e3c2550c415f4 -- 2.39.2
[PATCH 2/2] Add warn_unused_result attr to AUD_register_card
Ignoring the return value by accident is easy to miss as a bug. Such a
bug was spotted by Coverity CID 1523899. Now, future instances of this
type of bug will produce a warning when using GCC.
Signed-off-by: Manos Pitsidianakis
---
audio/audio.h | 2 +-
hw/arm/omap2.c | 8 +++-
hw/input/tsc210x.c | 8 +++-
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/audio/audio.h b/audio/audio.h
index fcc22307be..b78c75962e 100644
--- a/audio/audio.h
+++ b/audio/audio.h
@@ -94,7 +94,7 @@ typedef struct QEMUAudioTimeStamp {
void AUD_vlog (const char *cap, const char *fmt, va_list ap) G_GNUC_PRINTF(2,
0);
void AUD_log (const char *cap, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
-bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp);
+bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp)
QEMU_WARN_UNUSED_RESULT;
void AUD_remove_card (QEMUSoundCard *card);
CaptureVoiceOut *AUD_add_capture(
AudioState *s,
diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
index f170728e7e..59fc061120 100644
--- a/hw/arm/omap2.c
+++ b/hw/arm/omap2.c
@@ -614,7 +614,13 @@ static struct omap_eac_s *omap_eac_init(struct
omap_target_agent_s *ta,
s->codec.card.name = g_strdup(current_machine->audiodev);
s->codec.card.state = audio_state_by_name(s->codec.card.name,
&error_fatal);
}
-AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal);
+/*
+ * We pass error_fatal so on error QEMU will exit(). But we check the
+ * return value to make the warn_unused_result compiler warning go away.
+ */
+if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
+exit(1);
+}
memory_region_init_io(&s->iomem, NULL, &omap_eac_ops, s, "omap.eac",
omap_l4_region_size(ta, 0));
diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
index 950506fb38..003c664b56 100644
--- a/hw/input/tsc210x.c
+++ b/hw/input/tsc210x.c
@@ -1102,7 +1102,13 @@ static void tsc210x_init(TSC210xState *s,
s->card.name = g_strdup(current_machine->audiodev);
s->card.state = audio_state_by_name(s->card.name, &error_fatal);
}
-AUD_register_card(s->name, &s->card, &error_fatal);
+/*
+ * We pass error_fatal so on error QEMU will exit(). But we check the
+ * return value to make the warn_unused_result compiler warning go away.
+ */
+if (!AUD_register_card(s->name, &s->card, &error_fatal)) {
+return;
+}
qemu_register_reset((void *) tsc210x_reset, s);
vmstate_register(NULL, 0, vmsd, s);
--
2.39.2
[PATCH 1/2] Add QEMU_WARN_UNUSED_RESULT attribute
This commit adds QEMU_WARN_UNUSED_RESULT, a macro for the gcc function attribute `warn_unused_result`. The utility of this attribute is to ensure functions that return values that need to be inspected are not ignored by the caller. Signed-off-by: Manos Pitsidianakis --- include/qemu/compiler.h | 14 ++ 1 file changed, 14 insertions(+) diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index c797f0d457..7ddbf1f1cf 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -212,6 +212,20 @@ # define QEMU_USED #endif +/* + * From GCC documentation: + * + * The warn_unused_result attribute causes a warning to be emitted if a + * caller of the function with this attribute does not use its return value. + * This is useful for functions where not checking the result is either a + * security problem or always a bug, such as realloc. + */ +#if __has_attribute(warn_unused_result) +# define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result)) +#else +# define QEMU_WARN_UNUSED_RESULT +#endif + /* * Ugly CPP trick that is like "defined FOO", but also works in C * code. Useful to replace #ifdef with "if" statements; assumes -- 2.39.2
Re: [RFC PATCH-for-8.2] .gitlab-ci.d/cirrus.yml: Promote NetBSD job as gating
On Thu, Nov 09, 2023 at 06:15:51PM +0100, Thomas Huth wrote:
> On 09/11/2023 17.58, Daniel P. Berrangé wrote:
> > On Thu, Nov 09, 2023 at 04:35:56PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 9/11/23 16:35, Philippe Mathieu-Daudé wrote:
> > > > This Cirrus-CI based job takes ~12min, similarly to macOS job.
> > > >
> > > > Signed-off-by: Philippe Mathieu-Daudé
> > > > ---
> > > > Based-on: <20231109150900.91186-1-phi...@linaro.org>
> > > > "tests/vm/netbsd: Use Python v3.11"
> > > > ---
> > > >.gitlab-ci.d/cirrus.yml | 3 +--
> > > >1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/.gitlab-ci.d/cirrus.yml b/.gitlab-ci.d/cirrus.yml
> > > > index e7f1f83c2c..7b01acb104 100644
> > > > --- a/.gitlab-ci.d/cirrus.yml
> > > > +++ b/.gitlab-ci.d/cirrus.yml
> > > > @@ -94,8 +94,6 @@ aarch64-macos-12-base-build:
> > > >- cirrus-run -v --show-build-log always
> > > > .gitlab-ci.d/cirrus/$NAME.yml
> > > > variables:
> > > >QEMU_JOB_CIRRUS: 1
> > > > -QEMU_JOB_OPTIONAL: 1
> > > > -
> > > >x86-netbsd:
> > > > extends: .cirrus_kvm_job
> > > > @@ -110,3 +108,4 @@ x86-openbsd:
> > > >NAME: openbsd
> > > >CONFIGURE_ARGS:
> > > > --target-list=i386-softmmu,riscv64-softmmu,mips64-softmmu
> > > >TEST_TARGETS: check
> > > > +QEMU_JOB_OPTIONAL: 1
> > >
> > > BTW OpenBSD works for me, but takes ~20min (similar to the FreeBSD job).
> ...
> > I could have sworn our cirrus jobs were much slower in the past (around
> > the 40 min mark), as we had to cut down what we were running to avoid
> > frequent timeouts.
>
> You're right, Daniel. Seems like both, the Cirrus netbsd and the openbsd job
> are currently broken and only output some help text instead of compiling
> QEMU:
>
> https://gitlab.com/philmd/qemu/-/jobs/5497861511#L6834
>
> ... that's why the finish so fast.
>
> IIRC last time I've seen them "working", they were running into the 80
> minute timeout again.
>
> So the netbsd and openbsd job are indeed not very useful anymore. I think we
> should rather remove them and add a proper job via our own custom
> KVM-capable runners instead.
The CI job isn't the issue though - it is merely a sign of brokeness
elsewhere. Either tests/vm/{netbsd,openbsd} are broken, or our entire
build process for those platforms is broken.
We need to root cause this, rather than hide it further by dropping
the CI jobs.
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 for-8.3 v2 04/46] hw/pci: add pci_init_nic_devices(), pci_init_nic_in_slot()
On Fri, 2023-11-10 at 08:31 +0100, Philippe Mathieu-Daudé wrote: > > > + pci_dev = pci_new(devfn, model); > > + qdev_set_nic_properties(&pci_dev->qdev, nd); > > + pci_realize_and_unref(pci_dev, bus, &error_fatal); > > Could these functions be used with hotplug devices? > > If so we should propagate the error, not make it fatal. Hm, not sure. Mostly, I'm trying not to change existing behaviour with this series (except in carefully noted cases where the minutiæ of the existing behaviour appear to be both unintended and unimportant, and it would be unnecessarily complex to preserve the gratuitous differences between the way that platforms have open-coded things). I don't think it makes much sense *even* to use the new qemu_configure_nic_device() with hotplug devices. The user might create a *netdev* at startup, for later hotplug devices to use. But they wouldn't use `-nic` for that, and any devices explicitly added through hotplug will have an explicitly specified netdev, won't they? I don't think we want to change that model and allow hotplug devices to magically get config from -nic on the command line... do we? We even have a warning for the case where NIC configurations are provided with -nic but aren't consumed by the time the platform is instantiated (although that doesn't *prevent* them from being used later by hotplug). But that's answering your question which was about "these functions". For *this* particular function, pci_init_nic_in_slot(), it makes even less sense to consider hotplug. This is for platforms to handle the special cases, like "this board had an RTL8139 in slot 3", and make that NIC appear in the appropriate slot. It's done as a special case before processing the rest of the NICs which land in dynamically assigned slots — which may even be on a *different* bus, in the case of sun4u. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH 1/2] Add QEMU_WARN_UNUSED_RESULT attribute
On Fri, Nov 10, 2023 at 11:16:38AM +0200, Manos Pitsidianakis wrote: > This commit adds QEMU_WARN_UNUSED_RESULT, a macro for the gcc function > attribute `warn_unused_result`. The utility of this attribute is to > ensure functions that return values that need to be inspected are not > ignored by the caller. > > Signed-off-by: Manos Pitsidianakis > --- > include/qemu/compiler.h | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > index c797f0d457..7ddbf1f1cf 100644 > --- a/include/qemu/compiler.h > +++ b/include/qemu/compiler.h > @@ -212,6 +212,20 @@ > # define QEMU_USED > #endif > > +/* > + * From GCC documentation: > + * > + * The warn_unused_result attribute causes a warning to be emitted if a > + * caller of the function with this attribute does not use its return > value. > + * This is useful for functions where not checking the result is either a > + * security problem or always a bug, such as realloc. > + */ > +#if __has_attribute(warn_unused_result) > +# define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result)) > +#else > +# define QEMU_WARN_UNUSED_RESULT > +#endif GLib already provides us G_GNUC_WARN_UNUSED_RESULT so don't add this. 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 for-8.3 v2 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
On Fri, 2023-11-10 at 08:40 +0100, Philippe Mathieu-Daudé wrote:
> Hi David,
>
> +Markus/Bernhard
>
> On 6/11/23 20:49, David Woodhouse wrote:
> > From: David Woodhouse
> >
> > Eliminate direct access to nd_table[] and nb_nics by processing the the
> > Xen and ISA NICs first and then calling pci_init_nic_devices() for the
> > rest.
> >
> > Signed-off-by: David Woodhouse
> > Reviewed-by: Paul Durrant
> > ---
> > hw/i386/pc.c | 26 --
> > include/hw/net/ne2000-isa.h | 2 --
> > 2 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index c2bc3fa52d..4078d2d231 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -652,8 +652,11 @@ static void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
> > {
> > static int nb_ne2k = 0;
> >
> > - if (nb_ne2k == NE2000_NB_MAX)
> > + if (nb_ne2k == NE2000_NB_MAX) {
> > + error_setg(&error_fatal,
>
> In the context of dynamically created machines I'd rather have
> this function,
>
> > + "maximum number of ISA NE2000 devices exceeded");
> > return;
> > + }
> > isa_ne2000_init(bus, ne2000_io[nb_ne2k],
> > ne2000_irq[nb_ne2k], nd);
> > nb_ne2k++;
> > @@ -1291,23 +1294,26 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus
> > *isa_bus, PCIBus *pci_bus,
> > BusState *xen_bus)
> > {
> > MachineClass *mc = MACHINE_CLASS(pcmc);
> > - int i;
> > + bool default_is_ne2k = g_str_equal(mc->default_nic, TYPE_ISA_NE2000);
> > + NICInfo *nd;
> >
> > rom_set_order_override(FW_CFG_ORDER_OVERRIDE_NIC);
> > - for (i = 0; i < nb_nics; i++) {
> > - NICInfo *nd = &nd_table[i];
> > - const char *model = nd->model ? nd->model : mc->default_nic;
> >
> > - if (xen_bus && (!nd->model || g_str_equal(model,
> > "xen-net-device"))) {
> > + if (xen_bus) {
> > + while (nc = qemu_find_nic_info("xen-net-device", true, NULL)) {
> > DeviceState *dev = qdev_new("xen-net-device");
> > qdev_set_nic_properties(dev, nd);
> > qdev_realize_and_unref(dev, xen_bus, &error_fatal);
>
> and this one non-fatal (primarily for API example). But this is pending
> on a discussion on another thread, see:
> https://lore.kernel.org/qemu-devel/c1322f3b-2ae2-4ca7-9a76-a2a434dc8...@linaro.org/
> so no changed requested so far.
Thanks for the reference.
I'm happy to make pc_init_ne2k_isa() and even pc_nic_init() take an
'Error **' argument and use that instead of &error_fatal... and for the
*caller* to pass &error_fatal for now until/unless that discussion is
resolved? Not sure it helps much?
Then again... I do not favour the "my caller cannot *currently*
handle/propagate an error therefore I won't bother to return one
coherently" approach. That leads to someone else thinking "my callee
does not return an error coherently therefore I won't bother to handle
it", and nothing ever gets fixed.
I'll go fix it to take an errp argument.
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH] include/hw/xen: Use more inclusive language in comment
On 09.11.2023 18:40, Thomas Huth wrote: > --- a/include/hw/xen/interface/hvm/params.h > +++ b/include/hw/xen/interface/hvm/params.h > @@ -255,7 +255,7 @@ > * Note that 'mixed' mode has not been evaluated for safety from a > * security perspective. Before using this mode in a > * security-critical environment, each subop should be evaluated for > - * safety, with unsafe subops blacklisted in XSM. > + * safety, with unsafe subops blocked in XSM. To avoid another round trip when you send the patch against xen.git, as already asked for by others, I'd like to point out that the wording change isn't describing things sufficiently similarly: "blocked" reads as if XSM would do so all by itself, whereas "blacklisted" has an indication that something needs to be done for XSM to behave in the intended way. Minimally I'd suggest "suitably blocked via", but perhaps yet better wording can be thought of. Jan PS: Personally I'm against such avoiding of certain words. Them being misused is not really a justification. New wording (perhaps not specifically here, but considering the underlying wider theme) is going to be misused as well, leading to the need to come up with yet different wording, and so on.
Re: [PATCH] virtio-snd: check AUD_register_card return value
On Fri, 10 Nov 2023 01:23, "Michael S. Tsirkin" wrote: On Thu, Nov 09, 2023 at 06:03:15PM +, Peter Maydell wrote: On Thu, 9 Nov 2023 at 17:53, Michael S. Tsirkin wrote: > > On Thu, Nov 09, 2023 at 04:25:04PM +, Peter Maydell wrote: > > On Thu, 9 Nov 2023 at 16:21, Manos Pitsidianakis > > wrote: > > > > > > AUD_register_card might fail. Even though errp was passed as an > > > argument, the call's return value was not checked for failure. > > > > For whoever picks up this patch: we can add > > "Fixes Coverity CID 1523899" to the commit message. > > > Better: > > Fixes: Coverity CID 1523899 I thought "Fixes:" as a header-line like that was for the commit hash/subject of the commit the patch is fixing? thanks -- PMM This works for many other things e.g. gitlab issues (closes them). Fixes without : is much harder to distinguish from just general english text. qemu uses a mix of Fixes: Resolves: and Closes: . I don't see a real need for distinct tags for commit versus gitlab issue link: one can look at the contents to figure that out. The "Fixes:" trailer is for commits. In the kernel they use "Addresses-Coverity-ID: ..." but I can't find out if it's part of some automated workflow or just convention. Example commit in torvalds/linux: 5ad2e46030ad97de7fdbdaf63bb1af45c7caf3dd
Re: [RFC PATCH-for-8.2] .gitlab-ci.d/cirrus.yml: Promote NetBSD job as gating
On 10/11/2023 10.22, Daniel P. Berrangé wrote:
On Thu, Nov 09, 2023 at 06:15:51PM +0100, Thomas Huth wrote:
On 09/11/2023 17.58, Daniel P. Berrangé wrote:
On Thu, Nov 09, 2023 at 04:35:56PM +0100, Philippe Mathieu-Daudé wrote:
On 9/11/23 16:35, Philippe Mathieu-Daudé wrote:
This Cirrus-CI based job takes ~12min, similarly to macOS job.
Signed-off-by: Philippe Mathieu-Daudé
---
Based-on: <20231109150900.91186-1-phi...@linaro.org>
"tests/vm/netbsd: Use Python v3.11"
---
.gitlab-ci.d/cirrus.yml | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/.gitlab-ci.d/cirrus.yml b/.gitlab-ci.d/cirrus.yml
index e7f1f83c2c..7b01acb104 100644
--- a/.gitlab-ci.d/cirrus.yml
+++ b/.gitlab-ci.d/cirrus.yml
@@ -94,8 +94,6 @@ aarch64-macos-12-base-build:
- cirrus-run -v --show-build-log always .gitlab-ci.d/cirrus/$NAME.yml
variables:
QEMU_JOB_CIRRUS: 1
-QEMU_JOB_OPTIONAL: 1
-
x86-netbsd:
extends: .cirrus_kvm_job
@@ -110,3 +108,4 @@ x86-openbsd:
NAME: openbsd
CONFIGURE_ARGS:
--target-list=i386-softmmu,riscv64-softmmu,mips64-softmmu
TEST_TARGETS: check
+QEMU_JOB_OPTIONAL: 1
BTW OpenBSD works for me, but takes ~20min (similar to the FreeBSD job).
...
I could have sworn our cirrus jobs were much slower in the past (around
the 40 min mark), as we had to cut down what we were running to avoid
frequent timeouts.
You're right, Daniel. Seems like both, the Cirrus netbsd and the openbsd job
are currently broken and only output some help text instead of compiling
QEMU:
https://gitlab.com/philmd/qemu/-/jobs/5497861511#L6834
... that's why the finish so fast.
IIRC last time I've seen them "working", they were running into the 80
minute timeout again.
So the netbsd and openbsd job are indeed not very useful anymore. I think we
should rather remove them and add a proper job via our own custom
KVM-capable runners instead.
The CI job isn't the issue though - it is merely a sign of brokeness
elsewhere. Either tests/vm/{netbsd,openbsd} are broken, or our entire
build process for those platforms is broken.
We need to root cause this, rather than hide it further by dropping
the CI jobs.
"make vm-build-netbsd" locally just works fine (as soon as Philippe's python
fix gets merged). I just had another try with the cirrus-ci job, but it
indeeds run into timeout issues again:
https://gitlab.com/thuth/qemu/-/jobs/5501021556
I guess we could cut it down again by e.g. removing aarch64-softmmu from the
target list ... but we then still have the problem that we can not run it by
default due to the limitations of cirrus-ci only allowing to run 2 jobs in
parallel. And as long as we don't run things by default, they apparently
tend to bit-rot quite fast...
Thomas
[PATCH] tests: respect --enable/--disable-download for Avocado
Pass the content of $mkvenv_flags (which is either "--online"
or empty) down to tests/Makefile.include.
Signed-off-by: Paolo Bonzini
---
configure | 9 +
tests/Makefile.include | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/configure b/configure
index abcb199aa87..2da3c278db6 100755
--- a/configure
+++ b/configure
@@ -968,14 +968,14 @@ meson="$(cd pyvenv/bin; pwd)/meson"
# Conditionally ensure Sphinx is installed.
-mkvenv_flags=""
-if test "$download" = "enabled" -a "$docs" = "enabled" ; then
-mkvenv_flags="--online"
+mkvenv_online_flag=""
+if test "$download" = "enabled" ; then
+mkvenv_online_flag=" --online"
fi
if test "$docs" != "disabled" ; then
if ! $mkvenv ensuregroup \
- $mkvenv_flags \
+ $(test "$docs" = "enabled" && echo "$mkvenv_online_flag") \
${source_path}/pythondeps.toml docs;
then
if test "$docs" = "enabled" ; then
@@ -1631,6 +1631,7 @@ if test "$container" != no; then
fi
echo "SUBDIRS=$subdirs" >> $config_host_mak
echo "PYTHON=$python" >> $config_host_mak
+echo "MKVENV_ENSUREGROUP=$mkvenv ensuregroup $mkvenv_online_flag" >>
$config_host_mak
echo "GENISOIMAGE=$genisoimage" >> $config_host_mak
echo "MESON=$meson" >> $config_host_mak
echo "NINJA=$ninja" >> $config_host_mak
diff --git a/tests/Makefile.include b/tests/Makefile.include
index dab1989a071..c9d1674bd07 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -111,7 +111,7 @@ quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
$(TESTS_VENV_TOKEN): $(SRC_PATH)/pythondeps.toml
$(call quiet-venv-pip,install -e "$(SRC_PATH)/python/")
- $(PYTHON) python/scripts/mkvenv.py ensuregroup --online $< avocado
+ $(MKVENV_ENSUREGROUP) $< avocado
$(call quiet-command, touch $@)
$(TESTS_RESULTS_DIR):
--
2.41.0
Re: [RFC PATCH-for-8.2] .gitlab-ci.d/cirrus.yml: Promote NetBSD job as gating
On Fri, Nov 10, 2023 at 10:30:26AM +0100, Thomas Huth wrote:
> On 10/11/2023 10.22, Daniel P. Berrangé wrote:
> > On Thu, Nov 09, 2023 at 06:15:51PM +0100, Thomas Huth wrote:
> > > On 09/11/2023 17.58, Daniel P. Berrangé wrote:
> > > > On Thu, Nov 09, 2023 at 04:35:56PM +0100, Philippe Mathieu-Daudé wrote:
> > > > > On 9/11/23 16:35, Philippe Mathieu-Daudé wrote:
> > > > > > This Cirrus-CI based job takes ~12min, similarly to macOS job.
> > > > > >
> > > > > > Signed-off-by: Philippe Mathieu-Daudé
> > > > > > ---
> > > > > > Based-on: <20231109150900.91186-1-phi...@linaro.org>
> > > > > > "tests/vm/netbsd: Use Python v3.11"
> > > > > > ---
> > > > > > .gitlab-ci.d/cirrus.yml | 3 +--
> > > > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/.gitlab-ci.d/cirrus.yml b/.gitlab-ci.d/cirrus.yml
> > > > > > index e7f1f83c2c..7b01acb104 100644
> > > > > > --- a/.gitlab-ci.d/cirrus.yml
> > > > > > +++ b/.gitlab-ci.d/cirrus.yml
> > > > > > @@ -94,8 +94,6 @@ aarch64-macos-12-base-build:
> > > > > > - cirrus-run -v --show-build-log always
> > > > > > .gitlab-ci.d/cirrus/$NAME.yml
> > > > > > variables:
> > > > > > QEMU_JOB_CIRRUS: 1
> > > > > > -QEMU_JOB_OPTIONAL: 1
> > > > > > -
> > > > > > x86-netbsd:
> > > > > > extends: .cirrus_kvm_job
> > > > > > @@ -110,3 +108,4 @@ x86-openbsd:
> > > > > > NAME: openbsd
> > > > > > CONFIGURE_ARGS:
> > > > > > --target-list=i386-softmmu,riscv64-softmmu,mips64-softmmu
> > > > > > TEST_TARGETS: check
> > > > > > +QEMU_JOB_OPTIONAL: 1
> > > > >
> > > > > BTW OpenBSD works for me, but takes ~20min (similar to the FreeBSD
> > > > > job).
> > > ...
> > > > I could have sworn our cirrus jobs were much slower in the past (around
> > > > the 40 min mark), as we had to cut down what we were running to avoid
> > > > frequent timeouts.
> > >
> > > You're right, Daniel. Seems like both, the Cirrus netbsd and the openbsd
> > > job
> > > are currently broken and only output some help text instead of compiling
> > > QEMU:
> > >
> > > https://gitlab.com/philmd/qemu/-/jobs/5497861511#L6834
> > >
> > > ... that's why the finish so fast.
> > >
> > > IIRC last time I've seen them "working", they were running into the 80
> > > minute timeout again.
> > >
> > > So the netbsd and openbsd job are indeed not very useful anymore. I think
> > > we
> > > should rather remove them and add a proper job via our own custom
> > > KVM-capable runners instead.
> >
> > The CI job isn't the issue though - it is merely a sign of brokeness
> > elsewhere. Either tests/vm/{netbsd,openbsd} are broken, or our entire
> > build process for those platforms is broken.
> >
> > We need to root cause this, rather than hide it further by dropping
> > the CI jobs.
>
> "make vm-build-netbsd" locally just works fine (as soon as Philippe's python
> fix gets merged). I just had another try with the cirrus-ci job, but it
> indeeds run into timeout issues again:
>
> https://gitlab.com/thuth/qemu/-/jobs/5501021556
>
> I guess we could cut it down again by e.g. removing aarch64-softmmu from the
> target list ... but we then still have the problem that we can not run it by
> default due to the limitations of cirrus-ci only allowing to run 2 jobs in
> parallel. And as long as we don't run things by default, they apparently
> tend to bit-rot quite fast...
Right, even if we drop 1 target, with the other jobs we need to run in
Cirrus, it is still going to be too long. We would need it to be in the
30 min range maximum, to be viable running it by default I tink.
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] include/hw/xen: Use more inclusive language in comment
On 10/11/2023 10.30, Jan Beulich wrote: On 09.11.2023 18:40, Thomas Huth wrote: --- a/include/hw/xen/interface/hvm/params.h +++ b/include/hw/xen/interface/hvm/params.h @@ -255,7 +255,7 @@ * Note that 'mixed' mode has not been evaluated for safety from a * security perspective. Before using this mode in a * security-critical environment, each subop should be evaluated for - * safety, with unsafe subops blacklisted in XSM. + * safety, with unsafe subops blocked in XSM. To avoid another round trip when you send the patch against xen.git, as already asked for by others, I'd like to point out that the wording change isn't describing things sufficiently similarly: "blocked" reads as if XSM would do so all by itself, whereas "blacklisted" has an indication that something needs to be done for XSM to behave in the intended way. Minimally I'd suggest "suitably blocked via", but perhaps yet better wording can be thought of. Ok, could then please someone from you Xen guys get this fixed with appropriate wording in the xen.git repo? I never checked out that repo before and before I now spend hours and hours to figure out how to contribute a patch to Xen, just to replace a single word, it's way easier if someone with pre-existing Xen experience is taking care of this. Thanks, Thomas
Re: [PATCH v5 03/20] vfio/iommufd: Implement the iommufd backend
On 11/9/23 12:45, Zhenzhong Duan wrote:
From: Yi Liu
Add the iommufd backend. The IOMMUFD container class is implemented
based on the new /dev/iommu user API. This backend obviously depends
on CONFIG_IOMMUFD.
So far, the iommufd backend doesn't support dirty page sync yet due
to missing support in the host kernel.
Co-authored-by: Eric Auger
Signed-off-by: Yi Liu
Signed-off-by: Zhenzhong Duan
---
v5: Switch to IOAS attach/detach and hide hwpt
include/hw/vfio/vfio-common.h | 11 +
hw/vfio/common.c | 20 +-
hw/vfio/iommufd.c | 429 ++
hw/vfio/meson.build | 3 +
hw/vfio/trace-events | 10 +
5 files changed, 469 insertions(+), 4 deletions(-)
create mode 100644 hw/vfio/iommufd.c
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 24ecc0e7ee..3dac5c167e 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -89,6 +89,14 @@ typedef struct VFIOHostDMAWindow {
QLIST_ENTRY(VFIOHostDMAWindow) hostwin_next;
} VFIOHostDMAWindow;
+typedef struct IOMMUFDBackend IOMMUFDBackend;
+
+typedef struct VFIOIOMMUFDContainer {
+VFIOContainerBase bcontainer;
+IOMMUFDBackend *be;
+uint32_t ioas_id;
+} VFIOIOMMUFDContainer;
+
typedef struct VFIODeviceOps VFIODeviceOps;
typedef struct VFIODevice {
@@ -116,6 +124,8 @@ typedef struct VFIODevice {
OnOffAuto pre_copy_dirty_page_tracking;
bool dirty_pages_supported;
bool dirty_tracking;
+int devid;
+IOMMUFDBackend *iommufd;
} VFIODevice;
struct VFIODeviceOps {
@@ -201,6 +211,7 @@ typedef QLIST_HEAD(VFIODeviceList, VFIODevice)
VFIODeviceList;
extern VFIOGroupList vfio_group_list;
extern VFIODeviceList vfio_device_list;
extern const VFIOIOMMUOps vfio_legacy_ops;
+extern const VFIOIOMMUOps vfio_iommufd_ops;
extern const MemoryListener vfio_memory_listener;
extern int vfio_kvm_device_fd;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 572ae7c934..3b7e11158f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -19,6 +19,7 @@
*/
#include "qemu/osdep.h"
+#include CONFIG_DEVICES /* CONFIG_IOMMUFD */
#include
#ifdef CONFIG_KVM
#include
@@ -1462,10 +1463,13 @@ VFIOAddressSpace *vfio_get_address_space(AddressSpace
*as)
void vfio_put_address_space(VFIOAddressSpace *space)
{
-if (QLIST_EMPTY(&space->containers)) {
-QLIST_REMOVE(space, list);
-g_free(space);
+if (!QLIST_EMPTY(&space->containers)) {
+return;
I think this change deserves to be in a separate patch, even if simple.
Is there some relation with iommufd ? This is not clear.
}
+
+QLIST_REMOVE(space, list);
+g_free(space);
+
if (QLIST_EMPTY(&vfio_address_spaces)) {
qemu_unregister_reset(vfio_reset_handler, NULL);
}
@@ -1498,8 +1502,16 @@ retry:
int vfio_attach_device(char *name, VFIODevice *vbasedev,
AddressSpace *as, Error **errp)
{
-const VFIOIOMMUOps *ops = &vfio_legacy_ops;
+const VFIOIOMMUOps *ops;
+#ifdef CONFIG_IOMMUFD
+if (vbasedev->iommufd) {
+ops = &vfio_iommufd_ops;
+} else
+#endif
+{
+ops = &vfio_legacy_ops;
+}
Simply adding :
+#ifdef CONFIG_IOMMUFD
+if (vbasedev->iommufd) {
+ops = &vfio_iommufd_ops;
+}
+#endif
would have the same effect with less change.
That said, it would also be nice to find a way to avoid the use of
CONFIG_IOMMUFD in hw/vfio/common.c. May be with a helper returning
'const VFIOIOMMUOps *'. This is minor. Still, I find some redundancy
with vfio_container_init() and I don't a good alternative yet :)
return ops->attach_device(name, vbasedev, as, errp);
}
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
new file mode 100644
index 00..ea4e23f4ec
--- /dev/null
+++ b/hw/vfio/iommufd.c
@@ -0,0 +1,429 @@
+/*
+ * iommufd container backend
+ *
+ * Copyright (C) 2023 Intel Corporation.
+ * Copyright Red Hat, Inc. 2023
+ *
+ * Authors: Yi Liu
+ * Eric Auger
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include
+#include
+#include
+
+#include "hw/vfio/vfio-common.h"
+#include "qemu/error-report.h"
+#include "trace.h"
+#include "qapi/error.h"
+#include "sysemu/iommufd.h"
+#include "hw/qdev-core.h"
+#include "sysemu/reset.h"
+#include "qemu/cutils.h"
+#include "qemu/chardev_open.h"
+
+static int iommufd_map(VFIOContainerBase *bcontainer, hwaddr iova,
+ ram_addr_t size, void *vaddr, bool readonly)
+{
+VFIOIOMMUFDContainer *container =
+container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
+
+return iommufd_backend_map_dma(container->be,
+ container->ioas_id,
+ iova, size, vaddr, readonly);
+}
+
+static int iommufd_unmap(VFIOContainerBase *bcontainer,
+ hwaddr iova, ram_addr_t size,
+
Re: [PATCH v5 04/20] vfio/iommufd: Relax assert check for iommufd backend
On 11/9/23 12:45, Zhenzhong Duan wrote:
Currently iommufd doesn't support dirty page sync yet,
but it will not block us doing live migration if VFIO
migration is force enabled.
So in this case we allow set_dirty_page_tracking to be NULL.
Note we don't need same change for query_dirty_bitmap because
when dirty page sync isn't supported, query_dirty_bitmap will
never be called.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/container-base.c | 4
hw/vfio/container.c | 4
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 71f7274973..eee2dcfe76 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -55,6 +55,10 @@ void vfio_container_del_section_window(VFIOContainerBase
*bcontainer,
int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
bool start)
{
+if (!bcontainer->dirty_pages_supported) {
+return 0;
+}
+
g_assert(bcontainer->ops->set_dirty_page_tracking);
return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
}
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 6bacf38222..ed2d721b2b 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -216,10 +216,6 @@ static int
vfio_legacy_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
.argsz = sizeof(dirty),
};
-if (!bcontainer->dirty_pages_supported) {
-return 0;
-}
-
if (start) {
dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_START;
} else {
Re: [PATCH v5 05/20] vfio/iommufd: Add support for iova_ranges and pgsizes
On 11/9/23 12:45, Zhenzhong Duan wrote:
Some vIOMMU such as virtio-iommu use iova ranges from host side to
setup reserved ranges for passthrough device, so that guest will not
use an iova range beyond host support.
Use an uAPI of IOMMUFD to get iova ranges of host side and pass to
vIOMMU just like the legacy backend.
Also use out_iova_alignment returned from uAPI as pgsizes instead of
qemu_real_host_page_size() as a fallback.
Signed-off-by: Zhenzhong Duan
---
v5: Add missed pgsizes initialization in vfio_get_info_iova_range
hw/vfio/iommufd.c | 48 +++
1 file changed, 48 insertions(+)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index ea4e23f4ec..958c3e794f 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -267,6 +267,53 @@ static int iommufd_ram_block_discard_disable(bool state)
return ram_block_uncoordinated_discard_disable(state);
}
+static int vfio_get_info_iova_range(VFIOIOMMUFDContainer *container,
+uint32_t ioas_id)
+{
+VFIOContainerBase *bcontainer = &container->bcontainer;
+struct iommu_ioas_iova_ranges *info;
+struct iommu_iova_range *iova_ranges;
+int ret, sz, fd = container->be->fd;
+
+info = g_malloc0(sizeof(*info));
+info->size = sizeof(*info);
+info->ioas_id = ioas_id;
+
+ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info);
+if (ret && errno != EMSGSIZE) {
+goto error;
+}
+
+sz = info->num_iovas * sizeof(struct iommu_iova_range);
+info = g_realloc(info, sizeof(*info) + sz);
+info->allowed_iovas = (uintptr_t)(info + 1);
+
+ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info);
+if (ret) {
+goto error;
+}
+
+iova_ranges = (struct iommu_iova_range *)(uintptr_t)info->allowed_iovas;
+
+for (int i = 0; i < info->num_iovas; i++) {
+Range *range = g_new(Range, 1);
+
+range_set_bounds(range, iova_ranges[i].start, iova_ranges[i].last);
+bcontainer->iova_ranges =
+range_list_insert(bcontainer->iova_ranges, range);
+}
+bcontainer->pgsizes = info->out_iova_alignment;
+
+g_free(info);
+return 0;
+
+error:
+ret = -errno;
+g_free(info);
+error_report("vfio/iommufd: Cannot get iova ranges: %m");
Can we propagate the error ?
Thanks,
C.
+return ret;
+}
+
static int iommufd_attach_device(const char *name, VFIODevice *vbasedev,
AddressSpace *as, Error **errp)
{
@@ -343,6 +390,7 @@ static int iommufd_attach_device(const char *name,
VFIODevice *vbasedev,
}
bcontainer->pgsizes = qemu_real_host_page_size();
+vfio_get_info_iova_range(container, ioas_id);
bcontainer->listener = vfio_memory_listener;
memory_listener_register(&bcontainer->listener, bcontainer->space->as);
Re: [PATCH v5 19/20] kconfig: Activate IOMMUFD for s390x machines
On 11/10/23 00:00, Matthew Rosato wrote: On 11/9/23 6:45 AM, Zhenzhong Duan wrote: From: Cédric Le Goater Signed-off-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan --- hw/s390x/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig index 4c068d7960..26ad104485 100644 --- a/hw/s390x/Kconfig +++ b/hw/s390x/Kconfig @@ -6,6 +6,7 @@ config S390_CCW_VIRTIO imply VFIO_CCW imply WDT_DIAG288 imply PCIE_DEVICES +imply IOMMUFD select PCI_EXPRESS select S390_FLIC select S390_FLIC_KVM if KVM Reviewed-by: Matthew Rosato I also ran tests against vfio-pci (mlx, ism, nvme), vfio-ap and vfio-ccw on s390x with an iommufd-enabled host kernel + this series. Testing included having qemu open both fds, passing in one fd and letting qemu open the other, and passing in both fds. Thanks ! C.
[PATCH v2] docs: document what configure does with virtual environments
Given the recent confusion around how QEMU detects the system Meson installation, and/or decides to install its own, it is time to fill in the "Python virtual environments and the QEMU build system" section of the documentation. As a curiosity, a first and partial draft of the text was generated by an LLM[1]. It required quite a bit of editing and probably did not save much time, but some expressions do remain in the finished text. [1] https://chat.openai.com/share/42c1500d-71c1-480b-bab9-7ccc2c155365 Signed-off-by: Paolo Bonzini --- docs/devel/build-system.rst | 88 +++-- pythondeps.toml | 3 +- 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst index 21f78da7d1d..43d6005881e 100644 --- a/docs/devel/build-system.rst +++ b/docs/devel/build-system.rst @@ -122,10 +122,78 @@ functioning. These are performed using a few more helper functions: indicated by $TMPC. -Python virtual environments and the QEMU build system -- +Python virtual environments and the build process +- + +An important step in ``configure`` is to create a Python virtual +environment (venv) during the configuration phase. The Python interpreter +comes from the ``--python`` command line option, the ``$PYTHON`` variable +from the environment, or the system PATH, in this order. The venv resides +in the ``pyvenv`` directory in the build tree, and provides consistency +in how the build process runs Python code. + +At this stage, ``configure`` also queries the chosen Python interpreter +about QEMU's build dependencies. Note that the build process does *not* +look for ``meson``, ``sphinx-build`` or ``avocado`` binaries in the PATH; +likewise, there are no options such as ``--meson`` or ``--sphinx-build``. +This avoids a potential mismatch, where Meson and Sphinx binaries on the +PATH might operate in a different Python environment than the one chosen +by the user during the build process. On the other hand, it introduces +a potential source of confusion where the user installs a dependency but +``configure`` is not able to find it. When this happens, the dependency +was installed in the ``site-packages`` directory of another interpreter, +or with the wrong ``pip`` program. + +If a package is available for the chosen interpreter, ``configure`` +prepares a small script that invokes it from the venv itself[#distlib]_. +If not, ``configure`` can also optionally install dependencies in the +virtual environment with ``pip``, either from wheels in ``python/wheels`` +or by downloading the package with PyPI. Downloading can be disabled with +``--disable-download``; and anyway, it only happens when a ``configure`` +option (currently, only ``--enable-docs``) is explicitly enabled but +the dependencies are not present[#pip]_. + +.. [#distlib] The scripts are created based on the package's metadata, + specifically the ``console_script`` entry points. This is the + same mechanism that ``pip`` uses when installing a package. + Currently, in all cases it would be possible to use ``python -m`` + instead of an entry point script, which makes this approach a + bit overkill. On the other hand, creating the scripts is + future proof and it makes the contents of the ``pyvenv/bin`` + directory more informative. Portability is also not an issue, + because the Python Packaging Authority provides a package + ``distlib.scripts`` to perform this task. + +.. [#pip] ``pip`` might also be used when running ``make check-avocado`` + if downloading is enabled, to ensure that Avocado is + available. + +The required versions of the packages are stored in a configuration file +``pythondeps.toml``. The format is custom to QEMU, but it is documented +at the top of the file itself and it should be easy to understand. The +requirements should make it possible to use the version that is packaged +that is provided by supported distros. + +When dependencies are downloaded, instead, ``configure`` uses a "known +good" version that is also listed in ``pythondeps.toml``. In this +scenario, ``pythondeps.toml`` behaves like the "lock file" used by +``cargo``, ``poetry`` or other dependency management systems. + + +Bundled Python packages +--- + +Python packages that are **mandatory** dependencies to build QEMU, +but are not available in all supported distros, are bundled with the +QEMU sources. Currently this includes Meson (outdated in CentOS 8 +and derivatives, Ubuntu 20.04 and 22.04, and openSUSE Leap) and tomli +(absent in Ubuntu 20.04). + +If you need to update these, please do so by modifying and rerunning +``python/scripts/vendor.py``. This script embeds the sha256 hash of +package sources and c
Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
On Thu, Nov 09, 2023 at 08:31:01PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 10:24:20AM -0800, Andrea Bolognani wrote:
> > On Mon, Oct 16, 2023 at 05:27:03PM +0200, Victor Toso wrote:
> > > Example:
> > > qapi:
> > > | { 'command': 'query-sev', 'returns': 'SevInfo',
> > > | 'if': 'TARGET_I386' }
> > >
> > > go:
> > > | type QuerySevCommandReturn struct {
> > > | MessageId string `json:"id,omitempty"`
> > > | Result*SevInfo `json:"return"`
> > > | Error *QapiError `json:"error,omitempty"`
> > > | }
> > >
> > > usage:
> > > | // One can use QuerySevCommandReturn directly or
> > > | // command's interface GetReturnType() instead.
> >
> > I'm not convinced this function is particularly useful. I know
> > that I've suggested something similar for events, but the usage
> > scenarios are different.
>
> I think that I wanted to expose knowledge we had in the parser,
> not necessarily useful or needed indeed. At the very least, I
> agree that at this layer, we just want Command and ComandReturn
> types to be generated and properly (un)mashalled.
>
> One downside is for testing.
>
> If we have a list of commands, I can just iterate over them
> Unmarshal to a command interface variable, fetch the return type
> and do some comparisons to see if all is what we expected. See:
>
> https://gitlab.com/victortoso/qapi-go/-/blob/main/test/examples_test.go#L61
>
> Not saying we should keep it for tests, but it is useful :)
That code is quite dense and I'm not going to dig into it now :)
Anyway, I don't have a problem with keeping this type-safe
introspection feature around. Maybe call it GetCommandReturnType
though.
> > This produces
> >
> > type QueryAudiodevsCommandReturn struct {
> > MessageId string `json:"id,omitempty"`
> > Error *QAPIError `json:"error,omitempty"`
> > Result[]Audiodev `json:"return"`
> > }
> >
> > when the return type is an array. Is that the correct behavior? I
> > haven't thought too hard about it, but it seems odd so I though I'd
> > bring it up.
>
> Hm, the schema for it is
>
> ##
> # @query-audiodevs:
> #
> # Returns information about audiodev configuration
> #
> # Returns: array of @Audiodev
> #
> # Since: 8.0
> ##
> { 'command': 'query-audiodevs',
> 'returns': ['Audiodev'] }
>
> So, I think it is correct. Would you expect it to be an object
> wrapping the array or I'm missing what you find odd.
It works as expected if there is a result, but in the case of error:
in := `{ "error": {"class": "errorClass", "desc": "errorDesc" }}`
out := qapi.QueryAudiodevsCommandReturn{}
err := json.Unmarshal([]byte(in), &out)
if err != nil {
panic(err)
}
fmt.Printf("result=%v error=%v\n", out.Result, out.Error)
the output will be
result=[] error=errorDesc
Note how result is an empty array instead of a nil pointer. If we
unmarshal the same JSON into QueryReplayCommandReturn instead, the
output becomes
result= error=errorDesc
The latter behavior seems more correct to me, based on how we deal
with unions and alternates.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3] hw/ide/ahci: fix legacy software reset
Am 08.11.2023 um 23:26 hat Niklas Cassel geschrieben:
> From: Niklas Cassel
>
> Legacy software contains a standard mechanism for generating a reset to a
> Serial ATA device - setting the SRST (software reset) bit in the Device
> Control register.
>
> Serial ATA has a more robust mechanism called COMRESET, also referred to
> as port reset. A port reset is the preferred mechanism for error
> recovery and should be used in place of software reset.
>
> Commit e2a5d9b3d9c3 ("hw/ide/ahci: simplify and document PxCI handling")
> improved the handling of PxCI, such that PxCI gets cleared after handling
> a non-NCQ, or NCQ command (instead of incorrectly clearing PxCI after
> receiving anything - even a FIS that failed to parse, which should NOT
> clear PxCI, so that you can see which command slot that caused an error).
>
> However, simply clearing PxCI after a non-NCQ, or NCQ command, is not
> enough, we also need to clear PxCI when receiving a SRST in the Device
> Control register.
>
> A legacy software reset is performed by the host sending two H2D FISes,
> the first H2D FIS asserts SRST, and the second H2D FIS deasserts SRST.
>
> The first H2D FIS will not get a D2H reply, and requires the FIS to have
> the C bit set to one, such that the HBA itself will clear the bit in PxCI.
>
> The second H2D FIS will get a D2H reply once the diagnostic is completed.
> The clearing of the bit in PxCI for this command should ideally be done
> in ahci_init_d2h() (if it was a legacy software reset that caused the
> reset (a COMRESET does not use a command slot)). However, since the reset
> value for PxCI is 0, modify ahci_reset_port() to actually clear PxCI to 0,
> that way we can avoid complex logic in ahci_init_d2h().
>
> This fixes an issue for FreeBSD where the device would fail to reset.
> The problem was not noticed in Linux, because Linux uses a COMRESET
> instead of a legacy software reset by default.
>
> Fixes: e2a5d9b3d9c3 ("hw/ide/ahci: simplify and document PxCI handling")
> Reported-by: Marcin Juszkiewicz
> Signed-off-by: Niklas Cassel
Thanks, applied to the block branch.
Kevin
Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
On Thu, Nov 09, 2023 at 08:13:38PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 09:59:50AM -0800, Andrea Bolognani wrote:
> > Now, I'm not sure I would go as far as suggesting that the
> > GetName() function should be completely removed, but maybe we
> > can try leaving it out from the initial version and see if
> > people start screaming?
>
> It might be useful for debugging too. I would rather log
> e.GetName() than the string version of the type but if that's the
> only reason we needed, I agree on removing for now.
I think the upside is too small considering the potential for abuse.
> > API-wise, I'm not a fan of the fact that we're forcing users to call
> > (Un)MarshalEvent instead of the standard (Un)MarshalJSON. If we add
> > something like
> >
> > func GetEventType(data []byte) (Event, error) {
> >
> > it becomes feasible to stick with standard functions. We can of
> > course keep the (Un)MarshalEvent functions around for convenience,
> > but I don't think they should be the only available API.
>
> I agree. I'll change it. Perhaps we shouldn't use
> (Un)MarshalEvent at this layer at all. Probably the same for
> (Un)MarshalCommand.
Yeah, what I wrote for events applies 1:1 to commands as well.
Up to you whether or not you want to keep around the convenience
functions. It might indeed be fine to drop them for now and consider
reintroducing them later if it turns out that it really helps making
client code less clunky.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
On Thu, Nov 09, 2023 at 07:35:04PM +0100, Victor Toso wrote: > On Thu, Nov 09, 2023 at 09:29:28AM -0800, Andrea Bolognani wrote: > > Additionally, this would allow client code that *looks* at the > > union to keep working even if actual data is later added to the > > branch; client code that *creates* the union would need to be > > updated, of course, but that would be the case regardless. > > I think it is better to not have code that is working to keep > working in this case where Spice is implemented. > > Implementing Spice here would mean that a struct type > SetPasswordOptionsSpice was created but because the code handling > it before was using struct type Empty, it will not handle the new > struct, leading to possible runtime errors (e.g: not handling > username/password) > > A bool would be simpler, triggering compile time errors. You've convinced me :) Let's leave it like this for now. Once we start seriously thinking about compatibility across versions then we might have to reconsider this, but it's going to be part of a much bigger, much more fun conversation ;) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
On Thu, Nov 09, 2023 at 08:01:48PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 09:34:56AM -0800, Andrea Bolognani wrote:
> > We should call this Null instead of IsNull, and also make it a
> > pointer similarly to what I just suggested for union branches
> > that don't have additional data attached to them.
>
> I don't have a strong argument here against what you suggest, I
> just think that the usage of bool is simpler.
>
> The test I have here [0] would have code changing to something
> like:
>
> When is null:
>
> - val := &StrOrNull{IsNull: true}
> + myNull := JSONNull{}
> + val := &StrOrNull{Null: &myNull}
>
> So you need a instance to get a pointer.
>
> When is absent (no fields set), no changes as both bool defaults
> to false and pointer to nil.
>
> The code handling this would change from:
>
> - if u.IsNull {
> + if u.Null != nil {
> ...
> }
>
> We don't have the same issues as Union, under the hood we Marshal
> to/Unmarshal from "null" and that would not change.
>
> [0]
> https://gitlab.com/victortoso/qapi-go/-/blob/main/test/type_or_null_test.go
>
> I can change this in the next iteration.
No, leave the type alone. But I still think the name should probably
be Null instead of IsNull.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] tests: respect --enable/--disable-download for Avocado
On Fri, 10 Nov 2023, Paolo Bonzini wrote:
Pass the content of $mkvenv_flags (which is either "--online"
or empty) down to tests/Makefile.include.
Signed-off-by: Paolo Bonzini
---
configure | 9 +
tests/Makefile.include | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/configure b/configure
index abcb199aa87..2da3c278db6 100755
--- a/configure
+++ b/configure
@@ -968,14 +968,14 @@ meson="$(cd pyvenv/bin; pwd)/meson"
# Conditionally ensure Sphinx is installed.
-mkvenv_flags=""
-if test "$download" = "enabled" -a "$docs" = "enabled" ; then
-mkvenv_flags="--online"
+mkvenv_online_flag=""
+if test "$download" = "enabled" ; then
+mkvenv_online_flag=" --online"
Is leading space before -- intended? It does not seem to matter at usees
below.
Regards,
BALATON Zolatn
fi
if test "$docs" != "disabled" ; then
if ! $mkvenv ensuregroup \
- $mkvenv_flags \
+ $(test "$docs" = "enabled" && echo "$mkvenv_online_flag") \
${source_path}/pythondeps.toml docs;
then
if test "$docs" = "enabled" ; then
@@ -1631,6 +1631,7 @@ if test "$container" != no; then
fi
echo "SUBDIRS=$subdirs" >> $config_host_mak
echo "PYTHON=$python" >> $config_host_mak
+echo "MKVENV_ENSUREGROUP=$mkvenv ensuregroup $mkvenv_online_flag" >>
$config_host_mak
echo "GENISOIMAGE=$genisoimage" >> $config_host_mak
echo "MESON=$meson" >> $config_host_mak
echo "NINJA=$ninja" >> $config_host_mak
diff --git a/tests/Makefile.include b/tests/Makefile.include
index dab1989a071..c9d1674bd07 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -111,7 +111,7 @@ quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
$(TESTS_VENV_TOKEN): $(SRC_PATH)/pythondeps.toml
$(call quiet-venv-pip,install -e "$(SRC_PATH)/python/")
- $(PYTHON) python/scripts/mkvenv.py ensuregroup --online $< avocado
+ $(MKVENV_ENSUREGROUP) $< avocado
$(call quiet-command, touch $@)
$(TESTS_RESULTS_DIR):
RE: [PATCH v5 05/20] vfio/iommufd: Add support for iova_ranges and pgsizes
Hi Cédric,
>-Original Message-
>From: Cédric Le Goater
>Sent: Friday, November 10, 2023 5:36 PM
>Subject: Re: [PATCH v5 05/20] vfio/iommufd: Add support for iova_ranges and
>pgsizes
>
>On 11/9/23 12:45, Zhenzhong Duan wrote:
>> Some vIOMMU such as virtio-iommu use iova ranges from host side to
>> setup reserved ranges for passthrough device, so that guest will not
>> use an iova range beyond host support.
>>
>> Use an uAPI of IOMMUFD to get iova ranges of host side and pass to
>> vIOMMU just like the legacy backend.
>>
>> Also use out_iova_alignment returned from uAPI as pgsizes instead of
>> qemu_real_host_page_size() as a fallback.
>>
>> Signed-off-by: Zhenzhong Duan
>> ---
>> v5: Add missed pgsizes initialization in vfio_get_info_iova_range
>>
>> hw/vfio/iommufd.c | 48
>+++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index ea4e23f4ec..958c3e794f 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -267,6 +267,53 @@ static int iommufd_ram_block_discard_disable(bool
>state)
>> return ram_block_uncoordinated_discard_disable(state);
>> }
>>
>> +static int vfio_get_info_iova_range(VFIOIOMMUFDContainer *container,
>> +uint32_t ioas_id)
>> +{
>> +VFIOContainerBase *bcontainer = &container->bcontainer;
>> +struct iommu_ioas_iova_ranges *info;
>> +struct iommu_iova_range *iova_ranges;
>> +int ret, sz, fd = container->be->fd;
>> +
>> +info = g_malloc0(sizeof(*info));
>> +info->size = sizeof(*info);
>> +info->ioas_id = ioas_id;
>> +
>> +ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info);
>> +if (ret && errno != EMSGSIZE) {
>> +goto error;
>> +}
>> +
>> +sz = info->num_iovas * sizeof(struct iommu_iova_range);
>> +info = g_realloc(info, sizeof(*info) + sz);
>> +info->allowed_iovas = (uintptr_t)(info + 1);
>> +
>> +ret = ioctl(fd, IOMMU_IOAS_IOVA_RANGES, info);
>> +if (ret) {
>> +goto error;
>> +}
>> +
>> +iova_ranges = (struct iommu_iova_range *)(uintptr_t)info->allowed_iovas;
>> +
>> +for (int i = 0; i < info->num_iovas; i++) {
>> +Range *range = g_new(Range, 1);
>> +
>> +range_set_bounds(range, iova_ranges[i].start, iova_ranges[i].last);
>> +bcontainer->iova_ranges =
>> +range_list_insert(bcontainer->iova_ranges, range);
>> +}
>> +bcontainer->pgsizes = info->out_iova_alignment;
>> +
>> +g_free(info);
>> +return 0;
>> +
>> +error:
>> +ret = -errno;
>> +g_free(info);
>> +error_report("vfio/iommufd: Cannot get iova ranges: %m");
>
>Can we propagate the error ?
Do you mean propagating the error to call site and call error_report?
In fact, getting iova range from host is a better to have, not a must.
If fails we fallback to 64bit range.
Thanks
Zhenzhong
Re: [PATCH] hw/arm/virt: fix GIC maintenance IRQ registration
On Fri, 10 Nov 2023 at 09:07, Jean-Philippe Brucker
wrote:
>
> Since commit 9036e917f8 ("{include/}hw/arm: refactor virt PPI logic"),
> GIC maintenance IRQ registration fails on arm64:
>
> [0.979743] kvm [1]: Cannot register interrupt 9
>
> That commit re-defined VIRTUAL_PMU_IRQ to be a INTID but missed a case
> where the maintenance IRQ is actually referred by its PPI index. Just
> like commit fa68ecb330db ("hw/arm/virt: fix PMU IRQ registration"), use
> INITID_TO_PPI(). A search of "GIC_FDT_IRQ_TYPE_PPI" indicates that there
> shouldn't be more similar issues.
>
> Fixes: 9036e917f8 ("{include/}hw/arm: refactor virt PPI logic")
> Signed-off-by: Jean-Philippe Brucker
Isn't this already fixed by commit fa68ecb330dbd ?
thanks
-- PMM
RE: [PATCH v5 03/20] vfio/iommufd: Implement the iommufd backend
>-Original Message-
>From: Cédric Le Goater
>Sent: Friday, November 10, 2023 5:34 PM
>Subject: Re: [PATCH v5 03/20] vfio/iommufd: Implement the iommufd backend
>
>On 11/9/23 12:45, Zhenzhong Duan wrote:
>> From: Yi Liu
>>
>> Add the iommufd backend. The IOMMUFD container class is implemented
>> based on the new /dev/iommu user API. This backend obviously depends
>> on CONFIG_IOMMUFD.
>>
>> So far, the iommufd backend doesn't support dirty page sync yet due
>> to missing support in the host kernel.
>>
>> Co-authored-by: Eric Auger
>> Signed-off-by: Yi Liu
>> Signed-off-by: Zhenzhong Duan
>> ---
>> v5: Switch to IOAS attach/detach and hide hwpt
>>
>> include/hw/vfio/vfio-common.h | 11 +
>> hw/vfio/common.c | 20 +-
>> hw/vfio/iommufd.c | 429 ++
>> hw/vfio/meson.build | 3 +
>> hw/vfio/trace-events | 10 +
>> 5 files changed, 469 insertions(+), 4 deletions(-)
>> create mode 100644 hw/vfio/iommufd.c
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 24ecc0e7ee..3dac5c167e 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -89,6 +89,14 @@ typedef struct VFIOHostDMAWindow {
>> QLIST_ENTRY(VFIOHostDMAWindow) hostwin_next;
>> } VFIOHostDMAWindow;
>>
>> +typedef struct IOMMUFDBackend IOMMUFDBackend;
>> +
>> +typedef struct VFIOIOMMUFDContainer {
>> +VFIOContainerBase bcontainer;
>> +IOMMUFDBackend *be;
>> +uint32_t ioas_id;
>> +} VFIOIOMMUFDContainer;
>> +
>> typedef struct VFIODeviceOps VFIODeviceOps;
>>
>> typedef struct VFIODevice {
>> @@ -116,6 +124,8 @@ typedef struct VFIODevice {
>> OnOffAuto pre_copy_dirty_page_tracking;
>> bool dirty_pages_supported;
>> bool dirty_tracking;
>> +int devid;
>> +IOMMUFDBackend *iommufd;
>> } VFIODevice;
>>
>> struct VFIODeviceOps {
>> @@ -201,6 +211,7 @@ typedef QLIST_HEAD(VFIODeviceList, VFIODevice)
>VFIODeviceList;
>> extern VFIOGroupList vfio_group_list;
>> extern VFIODeviceList vfio_device_list;
>> extern const VFIOIOMMUOps vfio_legacy_ops;
>> +extern const VFIOIOMMUOps vfio_iommufd_ops;
>> extern const MemoryListener vfio_memory_listener;
>> extern int vfio_kvm_device_fd;
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 572ae7c934..3b7e11158f 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -19,6 +19,7 @@
>>*/
>>
>> #include "qemu/osdep.h"
>> +#include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>> #include
>> #ifdef CONFIG_KVM
>> #include
>> @@ -1462,10 +1463,13 @@ VFIOAddressSpace
>*vfio_get_address_space(AddressSpace *as)
>>
>> void vfio_put_address_space(VFIOAddressSpace *space)
>> {
>> -if (QLIST_EMPTY(&space->containers)) {
>> -QLIST_REMOVE(space, list);
>> -g_free(space);
>> +if (!QLIST_EMPTY(&space->containers)) {
>> +return;
>
>I think this change deserves to be in a separate patch, even if simple.
>Is there some relation with iommufd ? This is not clear.
OK, will do. It's unrelated to iommufd, just avoid unnecessary check below.
>
>> }
>> +
>> +QLIST_REMOVE(space, list);
>> +g_free(space);
>> +
>> if (QLIST_EMPTY(&vfio_address_spaces)) {
>> qemu_unregister_reset(vfio_reset_handler, NULL);
>> }
>> @@ -1498,8 +1502,16 @@ retry:
>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>> AddressSpace *as, Error **errp)
>> {
>> -const VFIOIOMMUOps *ops = &vfio_legacy_ops;
>> +const VFIOIOMMUOps *ops;
>>
>> +#ifdef CONFIG_IOMMUFD
>> +if (vbasedev->iommufd) {
>> +ops = &vfio_iommufd_ops;
>> +} else
>> +#endif
>> +{
>> +ops = &vfio_legacy_ops;
>> +}
>
>Simply adding :
>
> +#ifdef CONFIG_IOMMUFD
> +if (vbasedev->iommufd) {
> +ops = &vfio_iommufd_ops;
> +}
> +#endif
>
>would have the same effect with less change.
Indeed, will do.
>
>That said, it would also be nice to find a way to avoid the use of
>CONFIG_IOMMUFD in hw/vfio/common.c. May be with a helper returning
>'const VFIOIOMMUOps *'. This is minor. Still, I find some redundancy
>with vfio_container_init() and I don't a good alternative yet :)
Sure, will do, guess you mean a helper function in hw/vfio/helpers.c with
CONFIG_IOMMUFD check?
>
>
>> return ops->attach_device(name, vbasedev, as, errp);
>> }
>>
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> new file mode 100644
>> index 00..ea4e23f4ec
>> --- /dev/null
>> +++ b/hw/vfio/iommufd.c
>> @@ -0,0 +1,429 @@
>> +/*
>> + * iommufd container backend
>> + *
>> + * Copyright (C) 2023 Intel Corporation.
>> + * Copyright Red Hat, Inc. 2023
>> + *
>> + * Authors: Yi Liu
>> + * Eric Auger
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include
>> +#include
>> +#include
>> +
>> +#include "hw/vfio/vfio-common.h"
>> +#include "qemu/er
Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, 10 Nov 2023 at 09:16, Manos Pitsidianakis
wrote:
>
> Ignoring the return value by accident is easy to miss as a bug. Such a
> bug was spotted by Coverity CID 1523899. Now, future instances of this
> type of bug will produce a warning when using GCC.
>
> Signed-off-by: Manos Pitsidianakis
> ---
> audio/audio.h | 2 +-
> hw/arm/omap2.c | 8 +++-
> hw/input/tsc210x.c | 8 +++-
> 3 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/audio/audio.h b/audio/audio.h
> index fcc22307be..b78c75962e 100644
> --- a/audio/audio.h
> +++ b/audio/audio.h
> @@ -94,7 +94,7 @@ typedef struct QEMUAudioTimeStamp {
> void AUD_vlog (const char *cap, const char *fmt, va_list ap)
> G_GNUC_PRINTF(2, 0);
> void AUD_log (const char *cap, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
>
> -bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp);
> +bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp)
> QEMU_WARN_UNUSED_RESULT;
> void AUD_remove_card (QEMUSoundCard *card);
> CaptureVoiceOut *AUD_add_capture(
> AudioState *s,
> diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
> index f170728e7e..59fc061120 100644
> --- a/hw/arm/omap2.c
> +++ b/hw/arm/omap2.c
> @@ -614,7 +614,13 @@ static struct omap_eac_s *omap_eac_init(struct
> omap_target_agent_s *ta,
> s->codec.card.name = g_strdup(current_machine->audiodev);
> s->codec.card.state = audio_state_by_name(s->codec.card.name,
> &error_fatal);
> }
> -AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal);
> +/*
> + * We pass error_fatal so on error QEMU will exit(). But we check the
> + * return value to make the warn_unused_result compiler warning go away.
> + */
> +if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
> +exit(1);
> +}
This kind of thing is why Coverity's unused-result warning has a
lot of false positives. We shouldn't introduce extra code like
this to work around the fact that the tooling doesn't understand
our error-handling convention (i.e. error_fatal, and the way
that some functions report errors both via the return value and
also via the Error** argument).
thanks
-- PMM
Re: [PATCH] include/hw/xen: Use more inclusive language in comment
On Fri, 2023-11-10 at 10:30 +0100, Jan Beulich wrote: > On 09.11.2023 18:40, Thomas Huth wrote: > > --- a/include/hw/xen/interface/hvm/params.h > > +++ b/include/hw/xen/interface/hvm/params.h > > @@ -255,7 +255,7 @@ > > * Note that 'mixed' mode has not been evaluated for safety from a > > * security perspective. Before using this mode in a > > * security-critical environment, each subop should be evaluated for > > - * safety, with unsafe subops blacklisted in XSM. > > + * safety, with unsafe subops blocked in XSM. > > To avoid another round trip when you send the patch against xen.git, as > already asked for by others, I'd like to point out that the wording > change isn't describing things sufficiently similarly: "blocked" reads > as if XSM would do so all by itself, whereas "blacklisted" has an > indication that something needs to be done for XSM to behave in the > intended way. Minimally I'd suggest "suitably blocked via", but perhaps > yet better wording can be thought of. "denylist" is often used and works as a suitable replacement in most use cases. smime.p7s Description: S/MIME cryptographic signature
Re: [SeaBIOS] [PATCH v5] limit physical address space size
On Fri, Nov 10, 2023 at 09:36:02AM +0100, Claudio Fontana wrote:
> On 11/8/23 19:35, Kevin O'Connor wrote:
> > On Tue, Nov 07, 2023 at 02:03:09PM +0100, Gerd Hoffmann wrote:
> >> For better compatibility with old linux kernels,
> >> see source code comment.
> >>
> >> Related (same problem in ovmf):
> >> https://github.com/tianocore/edk2/commit/c1e853769046
> >
> > Thanks. I'll defer to your judgement on this. It does seem a little
> > odd to alter the CPUPhysBits variable itself instead of adding
> > additional checking to the pciinit.c code that uses CPUPhysBits.
> > (That is, if there are old Linux kernels that can't handle a very high
> > PCI space, then a workaround in the PCI code might make it more clear
> > what is occurring.)
> >
> > Cheers,
> > -Kevin
> >
> >
> >>
> >> Cc: Claudio Fontana
> >> Signed-off-by: Gerd Hoffmann
>
> Hi,
>
> I thought about this, and I am not sure it's the right call though.
>
> The change breaking old guests (CentOS7, Ubuntu 18.04 are the known ones, but
> presumably others as well) in QEMU is:
>
> commit 784155cdcb02ffaae44afecab93861070e7d652d
> Author: Gerd Hoffmann
> Date: Mon Sep 11 17:20:26 2023 +0200
>
> seabios: update submodule to git snapshot
>
> git shortlog
>
>
> Gerd Hoffmann (7):
> disable array bounds warning
> better kvm detection
> detect physical address space size
> move 64bit pci window to end of address space
> be less conservative with the 64bit pci io window
> qemu: log reservations in fw_cfg e820 table
> check for e820 conflict
>
> The reasoning for the change is according to:
>
> https://mail.coreboot.org/hyperkitty/list/seab...@seabios.org/message/BHK67ULKJTLJT676J4D5C2ND53CFEL3Q/
>
> "It makes seabios follow a common pattern. real mode address space
> has io resources mapped high (below 1M). 32-bit address space has
> io resources mapped high too (below 4G). This does the same for
> 64-bit resources."
>
> So it seems to be an almost aesthetic choice, the way I read the
> "common pattern", one that ends up actually breaking existing
> workloads though.
It's not about aesthetics. It's about handing out more resources to the
64bit mmio window and also to the pci bridge windows if we have enough
address space for that. This is important because not only main memory
sizes grow, but also device memory grows. Especially GPUs and AI
accelerators can have rather big PCI memory bars these days. If you
want support hotplugging them you need pcie root ports with big enough
bridge windows.
The "common pattern" refers to OVMF doing the same for the same reason.
> Now the correction to that that you propose in SeaBIOS is:
>
> >> +if (valid && physbits > 46) {
> >> +// Old linux kernels have trouble dealing with more than 46
> >> +// phys-bits, so avoid that for now. Seems to be a bug in the
> >> +// virtio-pci driver. Reported: centos-7, ubuntu-18.04
> >> +dprintf(1, "%s: using phys-bits=46 (old linux kernel
> >> compatibility)\n", __func__);
> >> +physbits = 46;
> >> +}
> but to me this is potentially breaking the situation for another set
> of users, those that are passing through 48 physical bits from their
> host cpu to the guest, and expect it to actually happen.
This doesn't change the address space size the guest does see. seabios
does not have the power to change the information returned by the cpuid
instruction.
This only changes the placement of the PCI bars. The pci setup code is
the only consumer of that variable, guess it makes sense to move the
quirk to the pci code (as suggested by Kevin) to clarify this.
> Would it not be a better solution to instead either revert the
> original change,
No (see above).
> or to patch it to find a new range that better satisfies code
> consistency/aesthetic requirements, but also does not break any
> running workload?
Upstream OVMF does the same thing for roughly one year, the old linux
kernel issue is the only one which showed up so far. OVMF wouldn't see
*really* old guests (without UEFI support) though.
The pci setup code already has a bunch of conditions for activating the
64bit mmio window, to avoid breaking really old guests. It wouldn't
happen on CPUs without long mode. It also wouldn't happen if guests
don't have memory above 4G.
I'm open to suggestions to refine this.
> Or we could add some extra workarounds in the stack elsewhere in the
> management tools to try to detect older guests (not ideal either), but
> this seems like adding breakage on top of breakage to me.
We already have libosinfo which records guest capabilities, which exists
to solve exactly that problem: Allow new guests use new features by
default, without breaking old guests. Extending libosinfo looks like a
perfectly valid approach to me, especially considering that we probably
want make the 46 phys-bits limit conditional some d
Re: [PATCH v5 10/20] vfio/pci: Make vfio cdev pre-openable by passing a file handle
On 11/9/23 12:45, Zhenzhong Duan wrote:
This gives management tools like libvirt a chance to open the vfio
cdev with privilege and pass FD to qemu. This way qemu never needs
to have privilege to open a VFIO or iommu cdev node.
Together with the earlier support of pre-opening /dev/iommu device,
now we have full support of passing a vfio device to unprivileged
qemu by management tool. This mode is no more considered for the
legacy backend. So let's remove the "TODO" comment.
Add a helper function vfio_device_get_name() to check fd and get
device name, it will also be used by other vfio devices.
There is no easy way to check if a device is mdev with FD passing,
so fail the x-balloon-allowed check unconditionally in this case.
There is also no easy way to get BDF as name with FD passing, so
we fake a name by VFIO_FD[fd].
Signed-off-by: Zhenzhong Duan
---
include/hw/vfio/vfio-common.h | 1 +
hw/vfio/helpers.c | 34 +
hw/vfio/iommufd.c | 12 +++
hw/vfio/pci.c | 40 ---
4 files changed, 71 insertions(+), 16 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 3dac5c167e..960a14e8d8 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -238,6 +238,7 @@ struct vfio_info_cap_header *
vfio_get_device_info_cap(struct vfio_device_info *info, uint16_t id);
struct vfio_info_cap_header *
vfio_get_cap(void *ptr, uint32_t cap_offset, uint16_t id);
+int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
#endif
bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 168847e7c5..d80aa58719 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -20,6 +20,7 @@
*/
#include "qemu/osdep.h"
+#include CONFIG_DEVICES /* CONFIG_IOMMUFD */
#include
#include "hw/vfio/vfio-common.h"
@@ -609,3 +610,36 @@ bool vfio_has_region_cap(VFIODevice *vbasedev, int region,
uint16_t cap_type)
return ret;
}
+
+int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
+{
+struct stat st;
+
+if (vbasedev->fd < 0) {
+if (stat(vbasedev->sysfsdev, &st) < 0) {
+error_setg_errno(errp, errno, "no such host device");
+error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->sysfsdev);
+return -errno;
+}
+/* User may specify a name, e.g: VFIO platform device */
+if (!vbasedev->name) {
+vbasedev->name = g_path_get_basename(vbasedev->sysfsdev);
+}
+}
+#ifdef CONFIG_IOMMUFD
+else {
+if (!vbasedev->iommufd) {
Can we handle with this case without CONFIG_IOMMUFD, simply by
testing vbasedev->iommufd ?
+error_setg(errp, "Use FD passing only with iommufd backend");
+return -EINVAL;
+}
+/*
+ * Give a name with fd so any function printing out vbasedev->name
+ * will not break.
+ */
+if (!vbasedev->name) {
+vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
+}
+}
+#endif
+return 0;
+}
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 44dc6848bf..fd30477275 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -326,11 +326,15 @@ static int iommufd_attach_device(const char *name,
VFIODevice *vbasedev,
uint32_t ioas_id;
Error *err = NULL;
-devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp);
-if (devfd < 0) {
-return devfd;
+if (vbasedev->fd < 0) {
+devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp);
+if (devfd < 0) {
+return devfd;
+}
+vbasedev->fd = devfd;
+} else {
+devfd = vbasedev->fd;
}
-vbasedev->fd = devfd;
ret = iommufd_connect_and_bind(vbasedev, errp);
if (ret) {
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e9a426200b..f95725ed16 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -44,6 +44,7 @@
#include "migration/blocker.h"
#include "migration/qemu-file.h"
#include "sysemu/iommufd.h"
+#include "monitor/monitor.h"
#define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
@@ -2934,18 +2935,23 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
VFIODevice *vbasedev = &vdev->vbasedev;
char *tmp, *subsys;
Error *err = NULL;
-struct stat st;
int i, ret;
bool is_mdev;
char uuid[UUID_STR_LEN];
char *name;
-if (!vbasedev->sysfsdev) {
+if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
if (!(~vdev->host.domain || ~vdev->host.bus ||
~vdev->host.slot || ~vdev->host.function)) {
error_setg(errp, "No provided host device");
+#ifdef CONFIG_IOMMUFD
+error_append_hint(errp, "Use -device vfio-pci,host=:BB:DD.F, "
+ "-device vfio-pci,sysfsdev=PATH_TO_DEVICE "
+
Re: [PATCH] tests: respect --enable/--disable-download for Avocado
On Fri, Nov 10, 2023 at 11:00 AM BALATON Zoltan wrote:
> > +if test "$download" = "enabled" ; then
> > +mkvenv_online_flag=" --online"
>
> Is leading space before -- intended? It does not seem to matter at usees
> below.
Maybe it's paranoia but I was worried that some shells would mess up
"echo --foo". They did in the past, but it was many years ago. It would
be useful to add a
print() {
printf '%s\n' "$*"
}
shell function but this was not the right patch to do it.
Paolo
> Regards,
> BALATON Zolatn
>
> > fi
> >
> > if test "$docs" != "disabled" ; then
> > if ! $mkvenv ensuregroup \
> > - $mkvenv_flags \
> > + $(test "$docs" = "enabled" && echo "$mkvenv_online_flag") \
> > ${source_path}/pythondeps.toml docs;
> > then
> > if test "$docs" = "enabled" ; then
> > @@ -1631,6 +1631,7 @@ if test "$container" != no; then
> > fi
> > echo "SUBDIRS=$subdirs" >> $config_host_mak
> > echo "PYTHON=$python" >> $config_host_mak
> > +echo "MKVENV_ENSUREGROUP=$mkvenv ensuregroup $mkvenv_online_flag" >>
> > $config_host_mak
> > echo "GENISOIMAGE=$genisoimage" >> $config_host_mak
> > echo "MESON=$meson" >> $config_host_mak
> > echo "NINJA=$ninja" >> $config_host_mak
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index dab1989a071..c9d1674bd07 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -111,7 +111,7 @@ quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
> >
> > $(TESTS_VENV_TOKEN): $(SRC_PATH)/pythondeps.toml
> > $(call quiet-venv-pip,install -e "$(SRC_PATH)/python/")
> > - $(PYTHON) python/scripts/mkvenv.py ensuregroup --online $< avocado
> > + $(MKVENV_ENSUREGROUP) $< avocado
> > $(call quiet-command, touch $@)
> >
> > $(TESTS_RESULTS_DIR):
> >
>
Re: [RFC PATCH 1/2] migration: Report error in incoming migration
Peter Xu writes:
> On Thu, Nov 09, 2023 at 01:58:55PM -0300, Fabiano Rosas wrote:
>> We're not currently reporting the errors set with migrate_set_error()
>> when incoming migration fails.
>>
>> Signed-off-by: Fabiano Rosas
>> ---
>> migration/migration.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 28a34c9068..cca32c553c 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -698,6 +698,13 @@ process_incoming_migration_co(void *opaque)
>> }
>>
>> if (ret < 0) {
>> +MigrationState *s = migrate_get_current();
>> +
>> +if (migrate_has_error(s)) {
>> +WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
>> +error_report_err(s->error);
>> +}
>> +}
>
> What's the major benefit of dumping this explicitly?
This is incoming migration, so there's no centralized error reporting
aside from the useless "load of migration failed: -5". If the code has
not called error_report we just never see the error message.
> And this is not relevant to the multifd problem, correct?
Yes, I'm being sneaky.
Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, 10 Nov 2023 12:21, Peter Maydell wrote: This kind of thing is why Coverity's unused-result warning has a lot of false positives. We shouldn't introduce extra code like this to work around the fact that the tooling doesn't understand our error-handling convention (i.e. error_fatal, and the way that some functions report errors both via the return value and also via the Error** argument). I respect that :). But I personally believe that clinging to C's inadequacies, instead of preventing bugs statically just because it adds some lines of code, is misguided. Proper code should strive to make bugs impossible in the first place. At least that is my perspective and I would like there to be constructive discussions about different approaches in the mailing list. Perhaps something good might come out of it!
Re: [SeaBIOS] [PATCH v5] limit physical address space size
Hi,
> This only changes the placement of the PCI bars. The pci setup code is
> the only consumer of that variable, guess it makes sense to move the
> quirk to the pci code (as suggested by Kevin) to clarify this.
i.e. like this:
>From d538dc7d4316e557ae302464252444d09de0681d Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann
Date: Tue, 7 Nov 2023 13:49:31 +0100
Subject: [PATCH] limit physical address space size
For better compatibility with old linux kernels,
see source code comment.
Related (same problem in ovmf):
https://github.com/tianocore/edk2/commit/c1e853769046
Reported-by: Claudio Fontana
Signed-off-by: Gerd Hoffmann
---
src/fw/pciinit.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c7084f5e397e..7aeea61bfd05 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -52,6 +52,7 @@ u64 pcimem64_start = BUILD_PCIMEM64_START;
u64 pcimem64_end = BUILD_PCIMEM64_END;
u64 pci_io_low_end = 0xa000;
u32 pci_use_64bit = 0;
+u32 pci_phys_bits = 0;
struct pci_region_entry {
struct pci_device *dev;
@@ -967,7 +968,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
if (hotplug_support == HOTPLUG_PCIE)
resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
if (hotplug_support && pci_use_64bit && is64 && (type ==
PCI_REGION_TYPE_PREFMEM))
-align = (u64)1 << (CPUPhysBits - 11);
+align = (u64)1 << (pci_phys_bits - 11);
if (align > sum && hotplug_support && !resource_optional)
sum = align; /* reserve min size for hot-plug */
if (size > sum) {
@@ -1131,12 +1132,12 @@ static void pci_bios_map_devices(struct pci_bus *busses)
r64_mem.base = le64_to_cpu(romfile_loadint("etc/reserved-memory-end",
0));
if (r64_mem.base < 0x1LL + RamSizeOver4G)
r64_mem.base = 0x1LL + RamSizeOver4G;
-if (CPUPhysBits) {
-u64 top = 1LL << CPUPhysBits;
+if (pci_phys_bits) {
+u64 top = 1LL << pci_phys_bits;
u64 size = (ALIGN(sum_mem, (1LL<<30)) +
ALIGN(sum_pref, (1LL<<30)));
if (pci_use_64bit)
-size = ALIGN(size, (1LL<<(CPUPhysBits-3)));
+size = ALIGN(size, (1LL<<(pci_phys_bits-3)));
if (r64_mem.base < top - size) {
r64_mem.base = top - size;
}
@@ -1181,8 +1182,16 @@ pci_setup(void)
dprintf(3, "pci setup\n");
-if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G)
+if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G) {
pci_use_64bit = 1;
+pci_phys_bits = CPUPhysBits;
+if (pci_phys_bits > 46) {
+// Old linux kernels have trouble dealing with more than 46
+// phys-bits, so avoid that for now. Seems to be a bug in the
+// virtio-pci driver. Reported: centos-7, ubuntu-18.04
+pci_phys_bits = 46;
+}
+}
dprintf(1, "=== PCI bus & bridge init ===\n");
if (pci_probe_host() != 0) {
--
2.41.0
Re: [PATCH 1/2] Add QEMU_WARN_UNUSED_RESULT attribute
On 10/11/23 10:16, Manos Pitsidianakis wrote: This commit adds QEMU_WARN_UNUSED_RESULT, a macro for the gcc function attribute `warn_unused_result`. The utility of this attribute is to ensure functions that return values that need to be inspected are not ignored by the caller. Signed-off-by: Manos Pitsidianakis --- include/qemu/compiler.h | 14 ++ 1 file changed, 14 insertions(+) +/* + * From GCC documentation: + * + * The warn_unused_result attribute causes a warning to be emitted if a + * caller of the function with this attribute does not use its return value. + * This is useful for functions where not checking the result is either a + * security problem or always a bug, such as realloc. + */ +#if __has_attribute(warn_unused_result) +# define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result)) +#else +# define QEMU_WARN_UNUSED_RESULT +#endif FWIW I sometimes use: +#if __has_attribute(nonnull) +# define QEMU_NONNULL(indexes...) __attribute__((nonnull((indexes +#else +# define QEMU_NONNULL(indexes...) +#endif when doing tree-wide refactors, then remove the attribute because prototypes become really ugly.
Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis wrote: > > On Fri, 10 Nov 2023 12:21, Peter Maydell wrote: > >This kind of thing is why Coverity's unused-result warning has a > >lot of false positives. We shouldn't introduce extra code like > >this to work around the fact that the tooling doesn't understand > >our error-handling convention (i.e. error_fatal, and the way > >that some functions report errors both via the return value and > >also via the Error** argument). > > I respect that :). But I personally believe that clinging to C's > inadequacies, instead of preventing bugs statically just because it adds > some lines of code, is misguided. Proper code should strive to make bugs > impossible in the first place. I generally agree. The problem here really is that we've ended up with this odd API convention that reports errors in two ways. In an ideal world we'd tidy up our APIs to report errors exactly in one way (presumably via the Error). -- PMM
Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote:
> On Fri, 10 Nov 2023 12:21, Peter Maydell wrote:
> > This kind of thing is why Coverity's unused-result warning has a
> > lot of false positives. We shouldn't introduce extra code like
> > this to work around the fact that the tooling doesn't understand
> > our error-handling convention (i.e. error_fatal, and the way
> > that some functions report errors both via the return value and
> > also via the Error** argument).
>
> I respect that :). But I personally believe that clinging to C's
> inadequacies, instead of preventing bugs statically just because it adds
> some lines of code, is misguided. Proper code should strive to make bugs
> impossible in the first place. At least that is my perspective and I would
> like there to be constructive discussions about different approaches in the
> mailing list. Perhaps something good might come out of it!
Your approach to the problem:
if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
exit(1);
}
is adding dead-code because the exit(1) will never be reachable. So while
it lets you squelch the unused result warning, it is verbose and misleading
to anyone who sees it.
Perhaps a more viable option is to pull in gnulib's ignore_value macro
#define ignore_value(x) \
(__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
and then we would have just this:
ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal));
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 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, Nov 10, 2023 at 11:18:39AM +, Peter Maydell wrote: > On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis > wrote: > > > > On Fri, 10 Nov 2023 12:21, Peter Maydell wrote: > > >This kind of thing is why Coverity's unused-result warning has a > > >lot of false positives. We shouldn't introduce extra code like > > >this to work around the fact that the tooling doesn't understand > > >our error-handling convention (i.e. error_fatal, and the way > > >that some functions report errors both via the return value and > > >also via the Error** argument). > > > > I respect that :). But I personally believe that clinging to C's > > inadequacies, instead of preventing bugs statically just because it adds > > some lines of code, is misguided. Proper code should strive to make bugs > > impossible in the first place. > > I generally agree. The problem here really is that we've ended > up with this odd API convention that reports errors in two > ways. In an ideal world we'd tidy up our APIs to report errors > exactly in one way (presumably via the Error). The compelling thing about our slightly odd &error_fatal/error_abort approach is that we can directly get stack traces showing exactly where the error happened. If we just propagate the error up the stack to finally exit/abort, the origin context is essentially lost, and when it does abort, we don't get a snapshot of all threads at the time the error hit, as we've moved on in time. 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 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, 10 Nov 2023 13:20, "Daniel P. Berrangé" wrote:
Your approach to the problem:
if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
exit(1);
}
is adding dead-code because the exit(1) will never be reachable. So while
it lets you squelch the unused result warning, it is verbose and misleading
to anyone who sees it.
Perhaps a more viable option is to pull in gnulib's ignore_value macro
#define ignore_value(x) \
(__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
and then we would have just this:
ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal));
Good suggestion, thanks!
And to be fair, I did put a comment directly above that line to explain
the unreachable code path. :)
+/*
+ * We pass error_fatal so on error QEMU will exit(). But we check
the
+ * return value to make the warn_unused_result compiler warning go
away.
+ */
Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, 10 Nov 2023 13:23, "Daniel P. Berrangé" wrote: On Fri, Nov 10, 2023 at 11:18:39AM +, Peter Maydell wrote: On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis wrote: > > On Fri, 10 Nov 2023 12:21, Peter Maydell wrote: > >This kind of thing is why Coverity's unused-result warning has a > >lot of false positives. We shouldn't introduce extra code like > >this to work around the fact that the tooling doesn't understand > >our error-handling convention (i.e. error_fatal, and the way > >that some functions report errors both via the return value and > >also via the Error** argument). > > I respect that :). But I personally believe that clinging to C's > inadequacies, instead of preventing bugs statically just because it adds > some lines of code, is misguided. Proper code should strive to make bugs > impossible in the first place. I generally agree. The problem here really is that we've ended up with this odd API convention that reports errors in two ways. In an ideal world we'd tidy up our APIs to report errors exactly in one way (presumably via the Error). The compelling thing about our slightly odd &error_fatal/error_abort approach is that we can directly get stack traces showing exactly where the error happened. If we just propagate the error up the stack to finally exit/abort, the origin context is essentially lost, and when it does abort, we don't get a snapshot of all threads at the time the error hit, as we've moved on in time. FYI: It is possible to get a stack trace using gnu libunwind which is portable, dependency-free and runs on many targets: https://github.com/libunwind/libunwind#libunwind Or llvm's libunwind which is also of great quality: https://bcain-llvm.readthedocs.io/projects/libunwind/en/latest/ They are also compatible with co-routines and JIT code (At least, the GNU one is, not sure about llvm).
Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, 10 Nov 2023, Daniel P. Berrangé wrote:
On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote:
On Fri, 10 Nov 2023 12:21, Peter Maydell wrote:
This kind of thing is why Coverity's unused-result warning has a
lot of false positives. We shouldn't introduce extra code like
this to work around the fact that the tooling doesn't understand
our error-handling convention (i.e. error_fatal, and the way
that some functions report errors both via the return value and
also via the Error** argument).
I respect that :). But I personally believe that clinging to C's
inadequacies, instead of preventing bugs statically just because it adds
some lines of code, is misguided. Proper code should strive to make bugs
impossible in the first place. At least that is my perspective and I would
like there to be constructive discussions about different approaches in the
mailing list. Perhaps something good might come out of it!
Your approach to the problem:
if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
exit(1);
}
is adding dead-code because the exit(1) will never be reachable. So while
it lets you squelch the unused result warning, it is verbose and misleading
to anyone who sees it.
Perhaps a more viable option is to pull in gnulib's ignore_value macro
#define ignore_value(x) \
(__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
and then we would have just this:
ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal));
I wonder if just casting to (void) without assigning to a value could
avoid the warning? In that case you would not need a macro just
(void)AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal);
Not sure it's clearer than a macro which states what it does but the
definition is a bit cryptic while this cast is simpler but may also puzzle
somebody not familiar with it.
Regards,
BALATON Zoltan
Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card
On Fri, Nov 10, 2023 at 12:35:56PM +0100, BALATON Zoltan wrote:
> On Fri, 10 Nov 2023, Daniel P. Berrangé wrote:
> > On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote:
> > > On Fri, 10 Nov 2023 12:21, Peter Maydell wrote:
> > > > This kind of thing is why Coverity's unused-result warning has a
> > > > lot of false positives. We shouldn't introduce extra code like
> > > > this to work around the fact that the tooling doesn't understand
> > > > our error-handling convention (i.e. error_fatal, and the way
> > > > that some functions report errors both via the return value and
> > > > also via the Error** argument).
> > >
> > > I respect that :). But I personally believe that clinging to C's
> > > inadequacies, instead of preventing bugs statically just because it adds
> > > some lines of code, is misguided. Proper code should strive to make bugs
> > > impossible in the first place. At least that is my perspective and I would
> > > like there to be constructive discussions about different approaches in
> > > the
> > > mailing list. Perhaps something good might come out of it!
> >
> > Your approach to the problem:
> >
> > if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
> >exit(1);
> > }
> >
> > is adding dead-code because the exit(1) will never be reachable. So while
> > it lets you squelch the unused result warning, it is verbose and misleading
> > to anyone who sees it.
> >
> > Perhaps a more viable option is to pull in gnulib's ignore_value macro
> >
> > #define ignore_value(x) \
> >(__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
> >
> > and then we would have just this:
> >
> > ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal));
>
> I wonder if just casting to (void) without assigning to a value could avoid
> the warning? In that case you would not need a macro just
>
> (void)AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal);
Casting to void is not sufficient, which is why I suggested the
ignore_value macro, which does enough to fool the compiler into
thinking the return value is used.
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] hw/arm/virt: fix GIC maintenance IRQ registration
On Fri, Nov 10, 2023 at 10:19:30AM +, Peter Maydell wrote:
> On Fri, 10 Nov 2023 at 09:07, Jean-Philippe Brucker
> wrote:
> >
> > Since commit 9036e917f8 ("{include/}hw/arm: refactor virt PPI logic"),
> > GIC maintenance IRQ registration fails on arm64:
> >
> > [0.979743] kvm [1]: Cannot register interrupt 9
> >
> > That commit re-defined VIRTUAL_PMU_IRQ to be a INTID but missed a case
> > where the maintenance IRQ is actually referred by its PPI index. Just
> > like commit fa68ecb330db ("hw/arm/virt: fix PMU IRQ registration"), use
> > INITID_TO_PPI(). A search of "GIC_FDT_IRQ_TYPE_PPI" indicates that there
> > shouldn't be more similar issues.
> >
> > Fixes: 9036e917f8 ("{include/}hw/arm: refactor virt PPI logic")
> > Signed-off-by: Jean-Philippe Brucker
>
> Isn't this already fixed by commit fa68ecb330dbd ?
No, that commit fixed the PMU interrupt (I copied most of its commit
message and referenced it), but the GIC maintenance interrupt still needed
to be fixed.
Thanks,
Jean
Re: [RFC PATCH 2/2] migration/multifd: Move semaphore release into main thread
Peter Xu writes:
> On Thu, Nov 09, 2023 at 01:58:56PM -0300, Fabiano Rosas wrote:
>> We cannot operate on the multifd semaphores outside of the multifd
>> channel thread
>> because multifd_save_cleanup() can run in parallel and
>> attempt to destroy the mutexes, which causes an assert.
>>
>> Looking at the places where we use the semaphores aside from the
>> migration thread, there's only the TLS handshake thread and the
>> initial multifd_channel_connect() in the main thread. These are places
>> where creating the multifd channel cannot fail, so releasing the
>> semaphores at these places only serves the purpose of avoiding a
>> deadlock when an error happens before the channel(s) have started, but
>> after the migration thread has already called
>> multifd_send_sync_main().
>>
>> Instead of attempting to release the semaphores at those places, move
>> the release into multifd_save_cleanup(). This puts the semaphore usage
>> in the same thread that does the cleanup, eliminating the race.
>>
>> Move the call to multifd_save_cleanup() before joining for the
>> migration thread so we release the semaphores before.
>>
>> Signed-off-by: Fabiano Rosas
>> ---
>> migration/migration.c | 4 +++-
>> migration/multifd.c | 29 +++--
>> 2 files changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index cca32c553c..52be20561b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1300,6 +1300,9 @@ static void migrate_fd_cleanup(MigrationState *s)
>> QEMUFile *tmp;
>>
>> trace_migrate_fd_cleanup();
>> +
>> +multifd_save_cleanup();
>> +
>> qemu_mutex_unlock_iothread();
>> if (s->migration_thread_running) {
>> qemu_thread_join(&s->thread);
>> @@ -1307,7 +1310,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>> }
>> qemu_mutex_lock_iothread();
>>
>> -multifd_save_cleanup();
>> qemu_mutex_lock(&s->qemu_file_lock);
>> tmp = s->to_dst_file;
>> s->to_dst_file = NULL;
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index ec58c58082..9ca482cfbe 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -527,6 +527,9 @@ void multifd_save_cleanup(void)
>>
>> if (p->running) {
>> qemu_thread_join(&p->thread);
>> +} else {
>> +qemu_sem_post(&p->sem_sync);
>> +qemu_sem_post(&multifd_send_state->channels_ready);
>
> I think relying on p->running to join the thread is already problematic.
>
Absolutely. I've already complained about this in another thread.
> Now all threads are created with JOINABLE, so we must join them to release
> the thread resources. Clearing "running" at the end of the thread is
> already wrong to me, because it means if the thread quits before the main
> thread reaching here, we will not join the thread, and the thread resource
> will be leaked.
>
> Here IMHO we should set p->running=true right before thread created, and
> never clear it. We may even want to rename it to p->thread_created?
>
Ok, let me do that. I already have some patches touching
multifd_new_send_channel_async() due to fixed-ram.
>> }
>> }
>> for (i = 0; i < migrate_multifd_channels(); i++) {
>> @@ -751,8 +754,6 @@ out:
>> assert(local_err);
>> trace_multifd_send_error(p->id);
>> multifd_send_terminate_threads(local_err);
>> -qemu_sem_post(&p->sem_sync);
>> -qemu_sem_post(&multifd_send_state->channels_ready);
>> error_free(local_err);
>> }
>>
>> @@ -780,20 +781,15 @@ static void multifd_tls_outgoing_handshake(QIOTask
>> *task,
>>
>> if (!qio_task_propagate_error(task, &err)) {
>> trace_multifd_tls_outgoing_handshake_complete(ioc);
>> -if (multifd_channel_connect(p, ioc, &err)) {
>> -return;
>> -}
>> +multifd_channel_connect(p, ioc, NULL);
>
> Ignoring Error** is not good..
>
> I think you meant to say "it should never fail", then we should put
> &error_abort. Another cleaner way to do this is split the current
> multifd_channel_connect() into tls and non-tls helpers, then we can call
> the non-tls helpers here (which may not need an Error**).
I attempted the split and it looked awkward, so I let go. But I agree
about the Error.
>> +} else {
>> +/*
>> + * The multifd client could already be waiting to queue data,
>> + * so let it know that we didn't even start.
>> + */
>> +p->quit = true;
>> +trace_multifd_tls_outgoing_handshake_error(ioc,
>> error_get_pretty(err));
>> }
>> -
>> -trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
>> -
>> -/*
>> - * Error happen, mark multifd_send_thread status as 'quit' although it
>> - * is not created, and then tell who pay attention to me.
>> - */
>> -p->quit =
Re: [RFC PATCH] Hexagon (target/hexagon) Make generators object oriented
Taylor Simpson wrote: > > RFC - This patch handles gen_tcg_funcs.py. I'd like to get comments > on the general approach before working on the other Python scripts. > > The generators are generally a bunch of Python if-then-else > statements based on the regtype and regid. Encapsulate regtype/regid > into a class hierarchy. Clients lookup the register and invoke > methods. > > This has several advantages for making the code easier to read, > understand, and maintain > - The class name makes it more clear what the operand does > - All the methods for a given type of operand are together > - Don't need as many calls to hex_common.bad_register > - We can remove the functions in hex_common that use regtype/regid > (e.g., is_read) > > Signed-off-by: Taylor Simpson Really nice! I personally think it's a great separation and it improves the code readability.
Re: [RFC PATCH 2/2] migration/multifd: Move semaphore release into main thread
Fabiano Rosas writes:
> Peter Xu writes:
>
>> On Thu, Nov 09, 2023 at 01:58:56PM -0300, Fabiano Rosas wrote:
>>> We cannot operate on the multifd semaphores outside of the multifd
>>> channel thread
>>> because multifd_save_cleanup() can run in parallel and
>>> attempt to destroy the mutexes, which causes an assert.
>>>
>>> Looking at the places where we use the semaphores aside from the
>>> migration thread, there's only the TLS handshake thread and the
>>> initial multifd_channel_connect() in the main thread. These are places
>>> where creating the multifd channel cannot fail, so releasing the
>>> semaphores at these places only serves the purpose of avoiding a
>>> deadlock when an error happens before the channel(s) have started, but
>>> after the migration thread has already called
>>> multifd_send_sync_main().
>>>
>>> Instead of attempting to release the semaphores at those places, move
>>> the release into multifd_save_cleanup(). This puts the semaphore usage
>>> in the same thread that does the cleanup, eliminating the race.
>>>
>>> Move the call to multifd_save_cleanup() before joining for the
>>> migration thread so we release the semaphores before.
>>>
>>> Signed-off-by: Fabiano Rosas
>>> ---
>>> migration/migration.c | 4 +++-
>>> migration/multifd.c | 29 +++--
>>> 2 files changed, 14 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index cca32c553c..52be20561b 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -1300,6 +1300,9 @@ static void migrate_fd_cleanup(MigrationState *s)
>>> QEMUFile *tmp;
>>>
>>> trace_migrate_fd_cleanup();
>>> +
>>> +multifd_save_cleanup();
>>> +
>>> qemu_mutex_unlock_iothread();
>>> if (s->migration_thread_running) {
>>> qemu_thread_join(&s->thread);
>>> @@ -1307,7 +1310,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>>> }
>>> qemu_mutex_lock_iothread();
>>>
>>> -multifd_save_cleanup();
>>> qemu_mutex_lock(&s->qemu_file_lock);
>>> tmp = s->to_dst_file;
>>> s->to_dst_file = NULL;
>>> diff --git a/migration/multifd.c b/migration/multifd.c
>>> index ec58c58082..9ca482cfbe 100644
>>> --- a/migration/multifd.c
>>> +++ b/migration/multifd.c
>>> @@ -527,6 +527,9 @@ void multifd_save_cleanup(void)
>>>
>>> if (p->running) {
>>> qemu_thread_join(&p->thread);
>>> +} else {
>>> +qemu_sem_post(&p->sem_sync);
>>> +qemu_sem_post(&multifd_send_state->channels_ready);
>>
>> I think relying on p->running to join the thread is already problematic.
>>
>
> Absolutely. I've already complained about this in another thread.
>
>> Now all threads are created with JOINABLE, so we must join them to release
>> the thread resources. Clearing "running" at the end of the thread is
>> already wrong to me, because it means if the thread quits before the main
>> thread reaching here, we will not join the thread, and the thread resource
>> will be leaked.
>>
>> Here IMHO we should set p->running=true right before thread created, and
>> never clear it. We may even want to rename it to p->thread_created?
>>
>
> Ok, let me do that. I already have some patches touching
> multifd_new_send_channel_async() due to fixed-ram.
>
>>> }
>>> }
>>> for (i = 0; i < migrate_multifd_channels(); i++) {
>>> @@ -751,8 +754,6 @@ out:
>>> assert(local_err);
>>> trace_multifd_send_error(p->id);
>>> multifd_send_terminate_threads(local_err);
>>> -qemu_sem_post(&p->sem_sync);
>>> -qemu_sem_post(&multifd_send_state->channels_ready);
>>> error_free(local_err);
>>> }
>>>
>>> @@ -780,20 +781,15 @@ static void multifd_tls_outgoing_handshake(QIOTask
>>> *task,
>>>
>>> if (!qio_task_propagate_error(task, &err)) {
>>> trace_multifd_tls_outgoing_handshake_complete(ioc);
>>> -if (multifd_channel_connect(p, ioc, &err)) {
>>> -return;
>>> -}
>>> +multifd_channel_connect(p, ioc, NULL);
>>
>> Ignoring Error** is not good..
>>
>> I think you meant to say "it should never fail", then we should put
>> &error_abort. Another cleaner way to do this is split the current
>> multifd_channel_connect() into tls and non-tls helpers, then we can call
>> the non-tls helpers here (which may not need an Error**).
>
> I attempted the split and it looked awkward, so I let go. But I agree
> about the Error.
>
>>> +} else {
>>> +/*
>>> + * The multifd client could already be waiting to queue data,
>>> + * so let it know that we didn't even start.
>>> + */
>>> +p->quit = true;
>>> +trace_multifd_tls_outgoing_handshake_error(ioc,
>>> error_get_pretty(err));
>>> }
>>> -
>>> -trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
>>> -
>>> -/*
>>> - * Error
Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card
On 10/11/23 12:20, Daniel P. Berrangé wrote:
On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote:
On Fri, 10 Nov 2023 12:21, Peter Maydell wrote:
This kind of thing is why Coverity's unused-result warning has a
lot of false positives. We shouldn't introduce extra code like
this to work around the fact that the tooling doesn't understand
our error-handling convention (i.e. error_fatal, and the way
that some functions report errors both via the return value and
also via the Error** argument).
I respect that :). But I personally believe that clinging to C's
inadequacies, instead of preventing bugs statically just because it adds
some lines of code, is misguided. Proper code should strive to make bugs
impossible in the first place. At least that is my perspective and I would
like there to be constructive discussions about different approaches in the
mailing list. Perhaps something good might come out of it!
Your approach to the problem:
if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
exit(1);
Rather:
g_assert_not_reached();
}
is adding dead-code because the exit(1) will never be reachable. So while
it lets you squelch the unused result warning, it is verbose and misleading
to anyone who sees it.
Re: [PATCH v4 28/41] vfio/iommufd: Implement the iommufd backend
On 10/11/2023 03:15, Duan, Zhenzhong wrote: > Hi Jason, Joao, > >> -Original Message- >> From: Jason Gunthorpe >> Sent: Thursday, November 9, 2023 10:35 PM >> Subject: Re: [PATCH v4 28/41] vfio/iommufd: Implement the iommufd backend >> >> On Thu, Nov 09, 2023 at 01:21:59PM +, Joao Martins wrote: >>> On 09/11/2023 13:09, Jason Gunthorpe wrote: On Thu, Nov 09, 2023 at 01:03:02PM +, Joao Martins wrote: >> I am not talking about mdevs; but rather the regular (non mdev) case not >> being >> able to use dirty tracking with autodomains hwpt allocation. > > ... without any vIOMMU. Ah, well, that is troublesome isn't it.. So do we teach autodomains to be more featured in the kernel or do we teach the generic qemu code to effectively implement autodomains in userspace? >>> >>> The latter is actually what we have been doing. Well I wouldn't call >> autodomains >>> in qemu, but rather just allocate a hwpt, instead of attaching the IOAS >>> directly. But well mdevs don't have domains and we overlooked that. I would >> turn >>> the exception into an exception rather than making the norm, doesn't look to >> be >>> much complexity added? >> >> Autodomains are complex because of things like mdev and iommu >> non-uniformity's. Qemu can't just allocate a single HWPT, it needs to >> be annoyingly managed. >> >>> What I last re-collect is that autodomains represents the 'simple users' >>> that >>> don't care much beyond the basics of IOMMU features (I recall the example >> was >>> DPDK apps and the like). You could say that for current needs IOMMU >> autodomains >>> suffices for qemu. >> >> Yes, that was my intention. Aside from that it primarily exists to >> support vfio compatibility >> >>> Connecting autodomains to this enforcing on the hwpt is relatively simple >>> btw, >>> it just needs to connect the dirty tracking flag with same semantic of >>> hwpt-alloc equivalent and pass the hwpt flags into the domain allocation. >> >> Yes >> >>> It's more of what of a question should be the expectations to the user when >>> using ATTACH_HWPT with an IOAS_ID versus direct manipulation of HWPT. I am >>> wondering if dirty tracking is alone here or whether there's more features >>> that >>> start to mud the simplicity of autodomains that would approximate of hwpt- >> alloc. >> >> This is why I had been thinking of a pure HWPT based scheme >> >> So it seems we cannot have a simple model where the generic qmeu layer >> just works in IOAS :( It might as well always work in HWPT and >> understand all the auto domains complexity itself. > > Let me know if there is anything I can do in this series to facilitate > future qemu dirty tracking support of iommufd. Not clear if I should > restore to the manual HWPT_ALLOC method in v4. If we want to have the closest support as type1-iommu, from what we have been discussing... it sounds like IOAS is the easiest first step to get barebones iommufd support. Which sort of makes sense since this is the introduction of iommufd and it already requires a lot of churn & refactoring to get there. For the new iommufd-only features (nesting/dirty-tracking) we will need the auto domains done by Qemu IIUC -- unless nesting is meant to coexist with autodomains with its own hwpts somehow (?) Right now I don't have the autodomains QEMU equivalent structure in mind to suggest a path in alternative to v5; Looking at the kernel autodomains path, aside from mdev I am not sure yet what annoyances the autodomains path in qemu is going to generate: more worringly whether we have enough information to tackle the non-uniformity e.g. if we are talking about features or whether different devices are behind different IOMMUs.
Re: [PATCH 5/7] qcow2: zero_l2_subclusters: fall through to discard operation when requested
On 11/3/23 17:19, Hanna Czenczek wrote:
> On 20.10.23 23:56, Andrey Drobyshev wrote:
>> When zeroizing subclusters within single cluster, detect usage of the
>> BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard
>> operation, much like it's done with the cluster-based discards. That
>> way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will
>> lead to actual unmap.
>
> Ever since the introduction of discard-no-unref, I wonder whether that
> is actually an advantage or not. I can’t think of a reason why it’d be
> advantageous to drop the allocation.
You mean omitting the UNallocation on the discard path?
>
> On the other hand, zero_in_l2_slice() does it indeed, so consistency is
> a reasonable argument.
>
>> Signed-off-by: Andrey Drobyshev
>> ---
>> block/qcow2-cluster.c | 17 ++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index cf40f2dc12..040251f2c3 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -2242,7 +2242,7 @@ zero_l2_subclusters(BlockDriverState *bs,
>> uint64_t offset,
>> unsigned nb_subclusters, int flags)
>> {
>> BDRVQcow2State *s = bs->opaque;
>> - uint64_t new_l2_bitmap;
>> + uint64_t new_l2_bitmap, l2_bitmap_mask;
>> int ret, sc = offset_to_sc_index(s, offset);
>> SubClusterRangeInfo scri = { 0 };
>> @@ -2251,9 +2251,10 @@ zero_l2_subclusters(BlockDriverState *bs,
>> uint64_t offset,
>> goto out;
>> }
>> + l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc +
>> nb_subclusters);
>
> “l2_bitmap_mask” already wasn’t a great name in patch 4 because it
> doesn’t say what mask it is, but in patch 4, there was at least only one
> relevant mask. Here, we have two (ALLOC_RANGE and ZERO_RANGE), so the
> name should reflect what kind of mask it is.
>
Noted.
>> new_l2_bitmap = scri.l2_bitmap;
>> - new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc +
>> nb_subclusters);
>> - new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc +
>> nb_subclusters);
>> + new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
>> + new_l2_bitmap &= ~l2_bitmap_mask;
>> /*
>> * If there're no non-zero subclusters left, we might as well
>> zeroize
>> @@ -2266,6 +2267,16 @@ zero_l2_subclusters(BlockDriverState *bs,
>> uint64_t offset,
>> 1, flags);
>> }
>> + /*
>> + * If the request allows discarding subclusters and they're actually
>> + * allocated, we go down the discard path since after the discard
>> + * operation the subclusters are going to be read as zeroes anyway.
>> + */
>> + if ((flags & BDRV_REQ_MAY_UNMAP) && (scri.l2_bitmap &
>> l2_bitmap_mask)) {
>> + return discard_l2_subclusters(bs, offset, nb_subclusters,
>> + QCOW2_DISCARD_REQUEST, false,
>> &scri);
>> + }
>
> I wonder why it matters whether any subclusters are allocated, i.e. why
> can’t we just immediately use discard_l2_subclusters() whenever
> BDRV_REQ_MAY_UNMAP is set, without even fetching SCRI (that would also
> save us passing SCRI here, which I’ve already said on patch 4, I don’t
> find great).
>
Yes, this way we'd indeed be able to get rid of passing an additional
argument.
> Doesn’t discard_l2_subclusters() guarantee the clusters read as zero
> when full_discard=false? There is this case where it won’t mark the
> subclusters as zero if there is no backing file, and none of the
> subclusters is allocated. But still, (A) those subclusters will read as
> zero, and (B) if there is a problem there, why don’t we just always mark
> those subclusters as zero? This optimization only has effect when the
> guest discards/zeroes subclusters (not whole clusters) that were already
> discarded, so sounds miniscule.
>
> Hanna
>
Good question. I also can't think of any issues when
zeroizing/discarding already unallocated clusters.
Essentially you're saying that zeroizing subclusters is equivalent to
discard_l2_subclusters(full_discard=false). Then in
discard_l2_subclusters() implementation we may mark the subclusters as
zeroes regardless of their allocation status in case of
full_discard=false. But when dealing with the whole clusters this logic
isn't quite applicable cause if the l2 entry isn't allocated at all
there's no point marking it as zero. Correct?
> [...]
Re: [PATCH 4/7] qcow2: make subclusters discardable
On 10/31/23 18:33, Hanna Czenczek wrote:
> (Sorry, opened another reply window, forgot I already had one open...)
>
> On 20.10.23 23:56, Andrey Drobyshev wrote:
>> This commit makes the discard operation work on the subcluster level
>> rather than cluster level. It introduces discard_l2_subclusters()
>> function and makes use of it in qcow2 discard implementation, much like
>> it's done with zero_in_l2_slice() / zero_l2_subclusters(). It also
>> changes the qcow2 driver pdiscard_alignment to subcluster_size. That
>> way subcluster-aligned discards lead to actual fallocate(PUNCH_HOLE)
>> operation and free host disk space.
>>
>> This feature will let us gain additional disk space on guest
>> TRIM/discard requests, especially when using large enough clusters
>> (1M, 2M) with subclusters enabled.
>>
>> Signed-off-by: Andrey Drobyshev
>> ---
>> block/qcow2-cluster.c | 100 --
>> block/qcow2.c | 8 ++--
>> 2 files changed, 101 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 7c6fa5524c..cf40f2dc12 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -2042,6 +2042,74 @@ discard_in_l2_slice(BlockDriverState *bs,
>> uint64_t offset, uint64_t nb_clusters,
>> return nb_clusters;
>> }
>> +static int coroutine_fn GRAPH_RDLOCK
>> +discard_l2_subclusters(BlockDriverState *bs, uint64_t offset,
>> + uint64_t nb_subclusters,
>> + enum qcow2_discard_type type,
>> + bool full_discard,
>> + SubClusterRangeInfo *pscri)
>> +{
>> + BDRVQcow2State *s = bs->opaque;
>> + uint64_t new_l2_bitmap, l2_bitmap_mask;
>> + int ret, sc = offset_to_sc_index(s, offset);
>> + SubClusterRangeInfo scri = { 0 };
>> +
>> + if (!pscri) {
>> + ret = get_sc_range_info(bs, offset, nb_subclusters, &scri);
>> + if (ret < 0) {
>> + goto out;
>> + }
>> + } else {
>> + scri = *pscri;
>
> Allowing to takes this from the caller sounds dangerous, considering we
> need to track who takes care of freeing scri.l2_slice.
>
>> + }
>> +
>> + l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc +
>> nb_subclusters);
>> + new_l2_bitmap = scri.l2_bitmap;
>> + new_l2_bitmap &= ~l2_bitmap_mask;
>> +
>> + /*
>> + * If there're no allocated subclusters left, we might as well
>> discard
>> + * the entire cluster. That way we'd also update the refcount
>> table.
>> + */
>> + if (!(new_l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC)) {
>
> What if there are subclusters in the cluster that are marked as zero,
> outside of the discarded range? It sounds wrong to apply a discard with
> either full_discard set or cleared to them.
>
Hmmm, then I think before falling to this path we should either:
1. Make sure full_discard=false. That is directly implied from what you
said in your other mail: discarding with full_discard=false is
equivalent to zeroizing;
2. Check that no subclusters within this cluster are explicitly marked
as zeroes.
3. Check that either 1) or 2) is true (if ((1) || (2))).
For now I like option 3) better.
>> + return discard_in_l2_slice(bs,
>> + QEMU_ALIGN_DOWN(offset,
>> s->cluster_size),
>> + 1, type, full_discard);
>> + }
>> +
>> + /*
>> + * Full discard means we fall through to the backing file, thus
>> we only
>> + * need to mark the subclusters as deallocated.
>
> I think it also means we need to clear the zero bits.
>
Yes, that seems right.
> Hanna
>
> [...]
Re: [PATCH v2 0/3] dump: Arch info function pointer addition and cleanup
Hi On Thu, Nov 9, 2023 at 4:05 PM Janosch Frank wrote: > > Small cleanups/fixes to the dump info function pointer assignments as > well as a new function pointer for cleanup of residual state. > > This has come up because test managed to dump a s390 PV vm onto a disk > that was too small for the dump. After the dump failed, the vm wasn't > able to resume running since KVM was still in dump mode which blocks > vcpu entry. > > The new function pointer allows cleanup of such a situation. > > v2: > - Usage of g_autofree > - Dropped explicit NULLing of function pointers > > Janosch Frank (3): > target/s390x/dump: Remove unneeded dump info function pointer init > dump: Add arch cleanup function > target/s390x/arch_dump: Add arch cleanup function for PV dumps > > dump/dump.c| 4 > include/sysemu/dump-arch.h | 1 + > target/s390x/arch_dump.c | 21 + > 3 files changed, 22 insertions(+), 4 deletions(-) > Series: Reviewed-by: Marc-André Lureau -- Marc-André Lureau
[PATCH 0/2] Implementation of resource_query_layout
This add implementation of resource_query_layout to get the information of how the host has actually allocated the buffer. This function is now used to query the stride for guest linear resource for dGPU prime on guest VMs. Related mesa mr: https: //gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23896 virglrenderer mr: https: //gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1268 Daniel Stone (1): virgl: Implement resource_query_layout Julia Zhang (1): virgl: Modify resource_query_layout hw/display/virtio-gpu-base.c| 4 +++ hw/display/virtio-gpu-virgl.c | 35 + include/hw/virtio/virtio-gpu-bswap.h| 7 + include/standard-headers/linux/virtio_gpu.h | 29 + meson.build | 4 +++ 5 files changed, 79 insertions(+) -- 2.34.1
[PATCH 1/2] virgl: Implement resource_query_layout
From: Daniel Stone
A new ioctl to shuttle information between host and guest about the
actual buffer allocation, which can be used for interop between GL and
Vulkan when supporting standard window systems.
---
hw/display/virtio-gpu-base.c| 4 ++
hw/display/virtio-gpu-virgl.c | 49 +
include/hw/virtio/virtio-gpu-bswap.h| 7 +++
include/standard-headers/linux/virtio_gpu.h | 29
meson.build | 4 ++
5 files changed, 93 insertions(+)
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 8d8aa2c88f..efaa681e54 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -246,6 +246,10 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t
features,
features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
features |= (1 << VIRTIO_GPU_F_CONTEXT_FENCE_WAIT);
+#ifdef HAVE_VIRGL_RESOURCE_QUERY_LAYOUT
+features |= (1 << VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT);
+#endif /* HAVE_VIRGL_RESOURCE_QUERY_LAYOUT */
+
return features;
}
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 2ac9a8b864..9c5a07cef1 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -715,6 +715,55 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
virtio_gpu_virgl_resource_unmap(g, cmd->cmd_hdr.ctx_id, res);
}
+#ifdef HAVE_VIRGL_RESOURCE_QUERY_LAYOUT
+static void virgl_cmd_resource_query_layout(VirtIOGPU *g,
+ struct virtio_gpu_ctrl_command *cmd)
+{
+struct virtio_gpu_simple_resource *res;
+struct virtio_gpu_resource_query_layout qlayout;
+struct virtio_gpu_resp_resource_layout resp;
+struct virgl_renderer_resource_layout rlayout;
+int ret;
+int i;
+VIRTIO_GPU_FILL_CMD(qlayout);
+virtio_gpu_resource_query_layout_bswap(&qlayout);
+
+if (qlayout.resource_id == 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
+ __func__);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}
+
+res = virtio_gpu_find_resource(g, qlayout.resource_id);
+if (!res) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
+ __func__, qlayout.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}
+
+ret = virgl_renderer_resource_query_layout(qlayout.resource_id, &rlayout);
+if (ret != 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource %d is not
externally-allocated\n",
+ __func__, qlayout.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}
+
+memset(&resp, 0, sizeof(resp));
+resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT;
+resp.num_planes = rlayout.num_planes;
+resp.modifier = rlayout.modifier;
+for (i = 0; i < resp.num_planes; i++) {
+resp.planes[i].offset = rlayout.planes[i].offset;
+resp.planes[i].stride = rlayout.planes[i].stride;
+ resp.planes[i].size = rlayout.planes[i].size;
+}
+virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
+}
+#endif /* HAVE_VIRGL_RESOURCE_QUERY_LAYOUT */
+
void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd)
{
diff --git a/include/hw/virtio/virtio-gpu-bswap.h
b/include/hw/virtio/virtio-gpu-bswap.h
index 8bbca1e751..49316758cc 100644
--- a/include/hw/virtio/virtio-gpu-bswap.h
+++ b/include/hw/virtio/virtio-gpu-bswap.h
@@ -99,4 +99,11 @@ virtio_gpu_host_wait_bswap(struct virtio_gpu_cmd_host_wait
*host_wait)
le64_to_cpus(&host_wait->fence_id);
}
+static inline void
+virtio_gpu_resource_query_layout_bswap(struct virtio_gpu_resource_query_layout
*rql)
+{
+virtio_gpu_ctrl_hdr_bswap(&rql->hdr);
+le32_to_cpus(&rql->resource_id);
+}
+
#endif
diff --git a/include/standard-headers/linux/virtio_gpu.h
b/include/standard-headers/linux/virtio_gpu.h
index a04419757f..734fdb6beb 100644
--- a/include/standard-headers/linux/virtio_gpu.h
+++ b/include/standard-headers/linux/virtio_gpu.h
@@ -70,6 +70,11 @@
*/
#define VIRTIO_GPU_F_CONTEXT_FENCE_WAIT 5
+/*
+ * VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT
+ */
+#define VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT 6
+
enum virtio_gpu_ctrl_type {
VIRTIO_GPU_UNDEFINED = 0,
@@ -102,6 +107,7 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB,
VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB,
VIRTIO_GPU_CMD_WAIT_FENCE,
+ VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT,
/* cursor commands */
VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
@@ -115,6 +121,7 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_RESP_OK_EDID,
VIRTIO_GPU_RESP_OK_RESOURCE_UUID,
VIRTIO_GPU_RESP_OK_MAP_INFO,
+ VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT,
/* error respon
[PATCH 2/2] virgl: Modify resource_query_layout
Modify resource_query_layout to handle the use case that need to query
correct stride for guest linear resource before it is created.
Signed-off-by: Julia Zhang
---
hw/display/virtio-gpu-virgl.c | 20 +++-
include/standard-headers/linux/virtio_gpu.h | 8
2 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 9c5a07cef1..ae146a68cb 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -728,22 +728,9 @@ static void virgl_cmd_resource_query_layout(VirtIOGPU *g,
VIRTIO_GPU_FILL_CMD(qlayout);
virtio_gpu_resource_query_layout_bswap(&qlayout);
-if (qlayout.resource_id == 0) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
- __func__);
-cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
-return;
-}
-
-res = virtio_gpu_find_resource(g, qlayout.resource_id);
-if (!res) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
- __func__, qlayout.resource_id);
-cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
-return;
-}
-
-ret = virgl_renderer_resource_query_layout(qlayout.resource_id, &rlayout);
+ret = virgl_renderer_resource_query_layout(qlayout.resource_id, &rlayout,
+ qlayout.width, qlayout.height,
+ qlayout.format, qlayout.bind);
if (ret != 0) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: resource %d is not
externally-allocated\n",
__func__, qlayout.resource_id);
@@ -758,7 +745,6 @@ static void virgl_cmd_resource_query_layout(VirtIOGPU *g,
for (i = 0; i < resp.num_planes; i++) {
resp.planes[i].offset = rlayout.planes[i].offset;
resp.planes[i].stride = rlayout.planes[i].stride;
- resp.planes[i].size = rlayout.planes[i].size;
}
virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
}
diff --git a/include/standard-headers/linux/virtio_gpu.h
b/include/standard-headers/linux/virtio_gpu.h
index 734fdb6beb..6fb0f711c8 100644
--- a/include/standard-headers/linux/virtio_gpu.h
+++ b/include/standard-headers/linux/virtio_gpu.h
@@ -506,7 +506,10 @@ struct virtio_gpu_status_freezing {
struct virtio_gpu_resource_query_layout {
struct virtio_gpu_ctrl_hdr hdr;
uint32_t resource_id;
- uint32_t padding;
+ uint32_t width;
+ uint32_t height;
+ uint32_t format;
+ uint32_t bind;
};
/* VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT */
@@ -515,12 +518,9 @@ struct virtio_gpu_resp_resource_layout {
struct virtio_gpu_ctrl_hdr hdr;
uint64_t modifier;
uint32_t num_planes;
- uint32_t padding;
struct virtio_gpu_resource_plane {
uint64_t offset;
- uint64_t size;
uint32_t stride;
- uint32_t padding;
} planes[VIRTIO_GPU_RES_MAX_PLANES];
};
--
2.34.1
[PATCH v4] target/ppc: Fix bugs in VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros
The patch below fixes a bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2
macros in target/ppc/fpu_helper.c where a non-NaN floating point value from the
source vector is incorrectly converted to 0, 0x8000, or 0x8000
instead of the expected value if a preceding source floating point value from
the same source vector was a NaN.
The bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros in
target/ppc/fpu_helper.c was introduced with commit c3f24257e3c0.
This patch also adds the vsx_f2i_nan test program that checks that the VSX
xvcvspsxws, xvcvspuxws, xvcvspsxds, xvcvspuxds, xvcvdpsxws, xvcvdpuxws,
xvcvdpsxds, and xvcvdpuxds instructions correctly convert non-NaN floating
values to integer values if the source vector contains NaN floating point
values.
Fixes: c3f24257e3c0 ("target/ppc: Clear fpstatus flags on helpers missing it")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1941
Signed-off-by: John Platts
---
target/ppc/fpu_helper.c | 12 +-
tests/tcg/ppc64/Makefile.target | 2 +-
tests/tcg/ppc64/vsx_f2i_nan.c | 304
3 files changed, 313 insertions(+), 5 deletions(-)
create mode 100644 tests/tcg/ppc64/vsx_f2i_nan.c
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 03150a0f10..4b3dcad5d1 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2880,20 +2880,22 @@ uint64_t helper_XSCVSPDPN(uint64_t xb)
#define VSX_CVT_FP_TO_INT(op, nels, stp, ttp, sfld, tfld, sfi, rnan) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.tfld = stp##_to_##ttp##_round_to_zero(xb->sfld, &env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.tfld = float_invalid_cvt(env, flags, t.tfld, rnan, 0, GETPC());\
}\
}\
\
*xt = t; \
+env->fp_status.float_exception_flags = all_flags;\
do_float_check_status(env, sfi, GETPC());\
}
@@ -2945,15 +2947,16 @@ VSX_CVT_FP_TO_INT128(XSCVQPSQZ, int128,
0x8000ULL);
#define VSX_CVT_FP_TO_INT2(op, nels, stp, ttp, sfi, rnan)\
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.VsrW(2 * i) = stp##_to_##ttp##_round_to_zero(xb->VsrD(i), \
&env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.VsrW(2 * i) = float_invalid_cvt(env, flags, t.VsrW(2 * i), \
rnan, 0, GETPC()); \
@@ -2962,6 +2965,7 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,
ppc_vsr_t *xb) \
}\
Re: [PATCH v5] target/ppc: Fix bugs in VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros
The patch below fixes a bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2
macros in target/ppc/fpu_helper.c where a non-NaN floating point value from the
source vector is incorrectly converted to 0, 0x8000, or 0x8000
instead of the expected value if a preceding source floating point value from
the same source vector was a NaN.
The bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros in
target/ppc/fpu_helper.c was introduced with commit c3f24257e3c0.
This patch also adds a new vsx_f2i_nan test in tests/tcg/ppc64 that checks that
the VSX xvcvspsxws, xvcvspuxws, xvcvspsxds, xvcvspuxds, xvcvdpsxws, xvcvdpuxws,
xvcvdpsxds, and xvcvdpuxds instructions correctly convert non-NaN floating point
values to integer values if the source vector contains NaN floating point
values.
Fixes: c3f24257e3c0 ("target/ppc: Clear fpstatus flags on helpers missing it")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1941
Signed-off-by: John Platts
---
target/ppc/fpu_helper.c | 12 +-
tests/tcg/ppc64/Makefile.target | 2 +-
tests/tcg/ppc64/vsx_f2i_nan.c | 304
3 files changed, 313 insertions(+), 5 deletions(-)
create mode 100644 tests/tcg/ppc64/vsx_f2i_nan.c
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 03150a0f10..4b3dcad5d1 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2880,20 +2880,22 @@ uint64_t helper_XSCVSPDPN(uint64_t xb)
#define VSX_CVT_FP_TO_INT(op, nels, stp, ttp, sfld, tfld, sfi, rnan) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.tfld = stp##_to_##ttp##_round_to_zero(xb->sfld, &env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.tfld = float_invalid_cvt(env, flags, t.tfld, rnan, 0, GETPC());\
}\
}\
\
*xt = t; \
+env->fp_status.float_exception_flags = all_flags;\
do_float_check_status(env, sfi, GETPC());\
}
@@ -2945,15 +2947,16 @@ VSX_CVT_FP_TO_INT128(XSCVQPSQZ, int128,
0x8000ULL);
#define VSX_CVT_FP_TO_INT2(op, nels, stp, ttp, sfi, rnan)\
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.VsrW(2 * i) = stp##_to_##ttp##_round_to_zero(xb->VsrD(i), \
&env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.VsrW(2 * i) = float_invalid_cvt(env, flags, t.VsrW(2 * i), \
rnan, 0, GETPC()); \
@@ -2962,6 +2965,7 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,
ppc_vsr_t *xb) \
}\
[PATCH v5] target/ppc: Fix bugs in VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros
The patch below fixes a bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2
macros in target/ppc/fpu_helper.c where a non-NaN floating point value from the
source vector is incorrectly converted to 0, 0x8000, or 0x8000
instead of the expected value if a preceding source floating point value from
the same source vector was a NaN.
The bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros in
target/ppc/fpu_helper.c was introduced with commit c3f24257e3c0.
This patch also adds a new vsx_f2i_nan test in tests/tcg/ppc64 that checks that
the VSX xvcvspsxws, xvcvspuxws, xvcvspsxds, xvcvspuxds, xvcvdpsxws, xvcvdpuxws,
xvcvdpsxds, and xvcvdpuxds instructions correctly convert non-NaN floating point
values to integer values if the source vector contains NaN floating point
values.
Fixes: c3f24257e3c0 ("target/ppc: Clear fpstatus flags on helpers missing it")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1941
Signed-off-by: John Platts
---
target/ppc/fpu_helper.c | 12 +-
tests/tcg/ppc64/Makefile.target | 2 +-
tests/tcg/ppc64/vsx_f2i_nan.c | 304
3 files changed, 313 insertions(+), 5 deletions(-)
create mode 100644 tests/tcg/ppc64/vsx_f2i_nan.c
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 03150a0f10..4b3dcad5d1 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2880,20 +2880,22 @@ uint64_t helper_XSCVSPDPN(uint64_t xb)
#define VSX_CVT_FP_TO_INT(op, nels, stp, ttp, sfld, tfld, sfi, rnan) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.tfld = stp##_to_##ttp##_round_to_zero(xb->sfld, &env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.tfld = float_invalid_cvt(env, flags, t.tfld, rnan, 0, GETPC());\
}\
}\
\
*xt = t; \
+env->fp_status.float_exception_flags = all_flags;\
do_float_check_status(env, sfi, GETPC());\
}
@@ -2945,15 +2947,16 @@ VSX_CVT_FP_TO_INT128(XSCVQPSQZ, int128,
0x8000ULL);
#define VSX_CVT_FP_TO_INT2(op, nels, stp, ttp, sfi, rnan)\
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.VsrW(2 * i) = stp##_to_##ttp##_round_to_zero(xb->VsrD(i), \
&env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.VsrW(2 * i) = float_invalid_cvt(env, flags, t.VsrW(2 * i), \
rnan, 0, GETPC()); \
@@ -2962,6 +2965,7 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,
ppc_vsr_t *xb) \
}\
Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
Hi,
On Fri, Nov 10, 2023 at 01:58:20AM -0800, Andrea Bolognani wrote:
> On Thu, Nov 09, 2023 at 08:01:48PM +0100, Victor Toso wrote:
> > On Thu, Nov 09, 2023 at 09:34:56AM -0800, Andrea Bolognani wrote:
> > > We should call this Null instead of IsNull, and also make it a
> > > pointer similarly to what I just suggested for union branches
> > > that don't have additional data attached to them.
> >
> > I don't have a strong argument here against what you suggest, I
> > just think that the usage of bool is simpler.
> >
> > The test I have here [0] would have code changing to something
> > like:
> >
> > When is null:
> >
> > - val := &StrOrNull{IsNull: true}
> > + myNull := JSONNull{}
> > + val := &StrOrNull{Null: &myNull}
> >
> > So you need a instance to get a pointer.
> >
> > When is absent (no fields set), no changes as both bool defaults
> > to false and pointer to nil.
> >
> > The code handling this would change from:
> >
> > - if u.IsNull {
> > + if u.Null != nil {
> > ...
> > }
> >
> > We don't have the same issues as Union, under the hood we Marshal
> > to/Unmarshal from "null" and that would not change.
> >
> > [0]
> > https://gitlab.com/victortoso/qapi-go/-/blob/main/test/type_or_null_test.go
> >
> > I can change this in the next iteration.
>
> No, leave the type alone. But I still think the name should probably
> be Null instead of IsNull.
Ok, keeping bool, rename to Null. Deal.
Cheers,
Victor
signature.asc
Description: PGP signature
Re: [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
Hi, On Fri, Nov 10, 2023 at 01:54:50AM -0800, Andrea Bolognani wrote: > On Thu, Nov 09, 2023 at 07:35:04PM +0100, Victor Toso wrote: > > On Thu, Nov 09, 2023 at 09:29:28AM -0800, Andrea Bolognani wrote: > > > Additionally, this would allow client code that *looks* at the > > > union to keep working even if actual data is later added to the > > > branch; client code that *creates* the union would need to be > > > updated, of course, but that would be the case regardless. > > > > I think it is better to not have code that is working to keep > > working in this case where Spice is implemented. > > > > Implementing Spice here would mean that a struct type > > SetPasswordOptionsSpice was created but because the code handling > > it before was using struct type Empty, it will not handle the new > > struct, leading to possible runtime errors (e.g: not handling > > username/password) > > > > A bool would be simpler, triggering compile time errors. > > You've convinced me :) Let's leave it like this for now. Once > we start seriously thinking about compatibility across versions > then we might have to reconsider this, but it's going to be > part of a much bigger, much more fun conversation ;) Yes! I'm looking forward to a 'unstable' version where we can agree on building things on top. Thanks, Victor signature.asc Description: PGP signature
[PATCH 1/1] ui/cocoa: add zoom-interpolation display option
From: Carwyn Ellis
Provides a new display option, zoom-interpolation, that enables
interpolation of the scaled display when zoom-to-fit is enabled.
Also provides a corresponding view menu item to allow this to be toggled
as required.
Signed-off-by: Carwyn Ellis
---
qapi/ui.json | 6 +-
ui/cocoa.m | 21 -
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/qapi/ui.json b/qapi/ui.json
index a0158baf23..e27e57f69c 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1414,6 +1414,9 @@
# turned off the host window will be resized instead. Defaults to
# "off". (Since 8.2)
#
+# @zoom-interpolation: Apply interpolation to smooth output when
+# zoom-to-fit is enabled. Defaults to "off". (Since 8.2)
+#
# Since: 7.0
##
{ 'struct': 'DisplayCocoa',
@@ -1421,7 +1424,8 @@
'*left-command-key': 'bool',
'*full-grab': 'bool',
'*swap-opt-cmd': 'bool',
- '*zoom-to-fit': 'bool'
+ '*zoom-to-fit': 'bool',
+ '*zoom-interpolation': 'bool'
} }
##
diff --git a/ui/cocoa.m b/ui/cocoa.m
index cd069da696..35df8d6ce2 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -105,6 +105,7 @@ static void cocoa_switch(DisplayChangeListener *dcl,
static bool swap_opt_cmd;
static bool stretch_video;
+static CGInterpolationQuality zoom_interpolation = kCGInterpolationNone;
static NSTextField *pauseLabel;
static bool allow_events;
@@ -455,7 +456,7 @@ - (void) drawRect:(NSRect) rect
// get CoreGraphic context
CGContextRef viewContextRef = [[NSGraphicsContext currentContext]
CGContext];
-CGContextSetInterpolationQuality (viewContextRef, kCGInterpolationNone);
+CGContextSetInterpolationQuality (viewContextRef, zoom_interpolation);
CGContextSetShouldAntialias (viewContextRef, NO);
// draw screen bitmap directly to Core Graphics context
@@ -1411,6 +1412,17 @@ - (void)zoomToFit:(id) sender
}
}
+- (void)toggleZoomInterpolation:(id) sender
+{
+if (zoom_interpolation == kCGInterpolationNone) {
+zoom_interpolation = kCGInterpolationLow;
+[sender setState: NSControlStateValueOn];
+} else {
+zoom_interpolation = kCGInterpolationNone;
+[sender setState: NSControlStateValueOff];
+}
+}
+
/* Displays the console on the screen */
- (void)displayConsole:(id)sender
{
@@ -1673,6 +1685,9 @@ static void create_initial_menus(void)
menuItem = [[[NSMenuItem alloc] initWithTitle:@"Zoom To Fit"
action:@selector(zoomToFit:) keyEquivalent:@""] autorelease];
[menuItem setState: stretch_video ? NSControlStateValueOn :
NSControlStateValueOff];
[menu addItem: menuItem];
+menuItem = [[[NSMenuItem alloc] initWithTitle:@"Zoom Interpolation"
action:@selector(toggleZoomInterpolation:) keyEquivalent:@""] autorelease];
+[menuItem setState: zoom_interpolation == kCGInterpolationLow ?
NSControlStateValueOn : NSControlStateValueOff];
+[menu addItem: menuItem];
menuItem = [[[NSMenuItem alloc] initWithTitle:@"View" action:nil
keyEquivalent:@""] autorelease];
[menuItem setSubmenu:menu];
[[NSApp mainMenu] addItem:menuItem];
@@ -2070,6 +2085,10 @@ static void cocoa_display_init(DisplayState *ds,
DisplayOptions *opts)
stretch_video = true;
}
+if (opts->u.cocoa.has_zoom_interpolation &&
opts->u.cocoa.zoom_interpolation) {
+zoom_interpolation = kCGInterpolationLow;
+}
+
create_initial_menus();
/*
* Create the menu entries which depend on QEMU state (for consoles
--
2.42.1
[PATCH 0/1] ui/cocoa: add zoom-interpolation display option
From: Carwyn Ellis Provides a new display option, zoom-interpolation, that enables or disables smoothing of the scaled display output when the zoom-to-fit option is enabled. A 'Zoom Interpolation' item has also been added to the view menu to allow interpolation to be toggled on and off as required. Signed-off-by: Carwyn Ellis Carwyn Ellis (1): ui/cocoa: add zoom-interpolation display option qapi/ui.json | 6 +- ui/cocoa.m | 21 - 2 files changed, 25 insertions(+), 2 deletions(-) -- 2.42.1
[PATCH] target/arm: Correct MTE tag checking for reverse-copy MOPS
When we are doing a FEAT_MOPS copy that must be performed backwards,
we call mte_mops_probe_rev(), passing it the address of the last byte
in the region we are probing. However, allocation_tag_mem_probe()
wants the address of the first byte to get the tag memory for.
Because we passed it (ptr, size) we could incorrectly trip the
allocation_tag_mem_probe() check for "does this access run across to
the following page", and if that following page happened not to be
valid then we would assert.
We know we will always be only dealing with a single page because the
code that calls mte_mops_probe_rev() ensures that. We could make
mte_mops_probe_rev() pass 'ptr - (size - 1)' to
allocation_tag_mem_probe(), but then we would have to adjust the
returned 'mem' pointer to get back to the tag RAM for the last byte
of the region. It's simpler to just pass in a size of 1 byte,
because we know that allocation_tag_mem_probe() in pure-probe
single-page mode doesn't care about the size.
Fixes: 69c51dc3723b ("target/arm: Implement MTE tag-checking functions for
FEAT_MOPS copies")
Signed-off-by: Peter Maydell
---
target/arm/tcg/mte_helper.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/target/arm/tcg/mte_helper.c b/target/arm/tcg/mte_helper.c
index 70ac876105f..ffb8ea1c349 100644
--- a/target/arm/tcg/mte_helper.c
+++ b/target/arm/tcg/mte_helper.c
@@ -1101,10 +1101,18 @@ uint64_t mte_mops_probe_rev(CPUARMState *env, uint64_t
ptr, uint64_t size,
uint32_t n;
mmu_idx = FIELD_EX32(desc, MTEDESC, MIDX);
-/* True probe; this will never fault */
+/*
+ * True probe; this will never fault. Note that our caller passes
+ * us a pointer to the end of the region, but allocation_tag_mem_probe()
+ * wants a pointer to the start. Because we know we don't span a page
+ * boundary and that allocation_tag_mem_probe() doesn't otherwise care
+ * about the size, pass in a size of 1 byte. This is simpler than
+ * adjusting the ptr to point to the start of the region and then having
+ * to adjust the returned 'mem' to get the end of the tag memory.
+ */
mem = allocation_tag_mem_probe(env, mmu_idx, ptr,
w ? MMU_DATA_STORE : MMU_DATA_LOAD,
- size, MMU_DATA_LOAD, true, 0);
+ 1, MMU_DATA_LOAD, true, 0);
if (!mem) {
return size;
}
--
2.34.1
Re: [PATCH v5] target/ppc: Fix bugs in VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros
Hello John,
On 11/10/23 16:37, John Platts wrote:
The patch below fixes a bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2
macros in target/ppc/fpu_helper.c where a non-NaN floating point value from the
source vector is incorrectly converted to 0, 0x8000, or 0x8000
instead of the expected value if a preceding source floating point value from
the same source vector was a NaN.
The bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros in
target/ppc/fpu_helper.c was introduced with commit c3f24257e3c0.
This patch also adds a new vsx_f2i_nan test in tests/tcg/ppc64 that checks that
the VSX xvcvspsxws, xvcvspuxws, xvcvspsxds, xvcvspuxds, xvcvdpsxws, xvcvdpuxws,
xvcvdpsxds, and xvcvdpuxds instructions correctly convert non-NaN floating point
values to integer values if the source vector contains NaN floating point
values.
Fixes: c3f24257e3c0 ("target/ppc: Clear fpstatus flags on helpers missing it")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1941
Signed-off-by: John Platts
---
target/ppc/fpu_helper.c | 12 +-
tests/tcg/ppc64/Makefile.target | 2 +-
tests/tcg/ppc64/vsx_f2i_nan.c | 304
3 files changed, 313 insertions(+), 5 deletions(-)
create mode 100644 tests/tcg/ppc64/vsx_f2i_nan.c
Please avoid email threading when sending a new version of the patch.
It gets burried in a thread and people (like me) might miss it.
Could you please run ./script/checkpatch.pl also and address this issue :
https://gitlab.com/legoater/qemu/-/jobs/5510354902
Thanks,
C.
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 03150a0f10..4b3dcad5d1 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2880,20 +2880,22 @@ uint64_t helper_XSCVSPDPN(uint64_t xb)
#define VSX_CVT_FP_TO_INT(op, nels, stp, ttp, sfld, tfld, sfi, rnan) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.tfld = stp##_to_##ttp##_round_to_zero(xb->sfld, &env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.tfld = float_invalid_cvt(env, flags, t.tfld, rnan, 0, GETPC());\
}\
}\
\
*xt = t; \
+env->fp_status.float_exception_flags = all_flags;\
do_float_check_status(env, sfi, GETPC());\
}
@@ -2945,15 +2947,16 @@ VSX_CVT_FP_TO_INT128(XSCVQPSQZ, int128, 0x8000ULL);
#define VSX_CVT_FP_TO_INT2(op, nels, stp, ttp, sfi, rnan)\
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.VsrW(2 * i) = stp##_to_##ttp##_round_to_zero(xb->VsrD(i), \
&env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.VsrW(2 * i) = float_inva
[PATCH] hw/audio/es1370: Clean up comment
Replace a sweary comment with one that's a bit more helpful to
future readers of the code.
Signed-off-by: Peter Maydell
---
hw/audio/es1370.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c
index 91c47330ad3..fad55412119 100644
--- a/hw/audio/es1370.c
+++ b/hw/audio/es1370.c
@@ -670,8 +670,13 @@ static void es1370_transfer_audio (ES1370State *s, struct
chan *d, int loop_sel,
cnt += (transferred + d->leftover) >> 2;
if (s->sctl & loop_sel) {
-/* Bah, how stupid is that having a 0 represent true value?
- i just spent few hours on this shit */
+/*
+ * loop_sel tells us which bit in the SCTL register to look at
+ * (either P1_LOOP_SEL, P2_LOOP_SEL or R1_LOOP_SEL). The sense
+ * of these bits is 0 for loop mode (set interrupt and keep recording
+ * when the sample count reaches zero) or 1 for stop mode (set
+ * interrupt and stop recording).
+ */
AUD_log ("es1370: warning", "non looping mode\n");
} else {
d->frame_cnt = size;
--
2.34.1
Re: [SeaBIOS] [PATCH v5] limit physical address space size
On Fri, Nov 10, 2023 at 12:04:24PM +0100, Gerd Hoffmann wrote:
> Hi,
>
> > This only changes the placement of the PCI bars. The pci setup code is
> > the only consumer of that variable, guess it makes sense to move the
> > quirk to the pci code (as suggested by Kevin) to clarify this.
>
> i.e. like this:
>
> From d538dc7d4316e557ae302464252444d09de0681d Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann
> Date: Tue, 7 Nov 2023 13:49:31 +0100
> Subject: [PATCH] limit physical address space size
>
> For better compatibility with old linux kernels,
> see source code comment.
>
> Related (same problem in ovmf):
> https://github.com/tianocore/edk2/commit/c1e853769046
>
> Reported-by: Claudio Fontana
> Signed-off-by: Gerd Hoffmann
> ---
> src/fw/pciinit.c | 19 ++-
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index c7084f5e397e..7aeea61bfd05 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -52,6 +52,7 @@ u64 pcimem64_start = BUILD_PCIMEM64_START;
> u64 pcimem64_end = BUILD_PCIMEM64_END;
> u64 pci_io_low_end = 0xa000;
> u32 pci_use_64bit = 0;
> +u32 pci_phys_bits = 0;
FWIW, all these flags are getting a bit confusing. Maybe do something
like the patch below.
>
> struct pci_region_entry {
> struct pci_device *dev;
> @@ -967,7 +968,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> if (hotplug_support == HOTPLUG_PCIE)
> resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
> if (hotplug_support && pci_use_64bit && is64 && (type ==
> PCI_REGION_TYPE_PREFMEM))
> -align = (u64)1 << (CPUPhysBits - 11);
> +align = (u64)1 << (pci_phys_bits - 11);
> if (align > sum && hotplug_support && !resource_optional)
> sum = align; /* reserve min size for hot-plug */
> if (size > sum) {
> @@ -1131,12 +1132,12 @@ static void pci_bios_map_devices(struct pci_bus
> *busses)
> r64_mem.base =
> le64_to_cpu(romfile_loadint("etc/reserved-memory-end", 0));
> if (r64_mem.base < 0x1LL + RamSizeOver4G)
> r64_mem.base = 0x1LL + RamSizeOver4G;
> -if (CPUPhysBits) {
> -u64 top = 1LL << CPUPhysBits;
> +if (pci_phys_bits) {
FYI, this is a change in behavior - previously this condition would
have been taken even if CPULongMode or RamSizeOver4G is false. I'm
not sure if this change is intentional. (My example patch below
follows your lead here.)
> +u64 top = 1LL << pci_phys_bits;
> u64 size = (ALIGN(sum_mem, (1LL<<30)) +
> ALIGN(sum_pref, (1LL<<30)));
> if (pci_use_64bit)
> -size = ALIGN(size, (1LL<<(CPUPhysBits-3)));
> +size = ALIGN(size, (1LL<<(pci_phys_bits-3)));
> if (r64_mem.base < top - size) {
> r64_mem.base = top - size;
> }
> @@ -1181,8 +1182,16 @@ pci_setup(void)
>
> dprintf(3, "pci setup\n");
>
> -if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G)
> +if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G) {
> pci_use_64bit = 1;
> +pci_phys_bits = CPUPhysBits;
> +if (pci_phys_bits > 46) {
> +// Old linux kernels have trouble dealing with more than 46
> +// phys-bits, so avoid that for now. Seems to be a bug in the
> +// virtio-pci driver. Reported: centos-7, ubuntu-18.04
> +pci_phys_bits = 46;
> +}
> +}
>
> dprintf(1, "=== PCI bus & bridge init ===\n");
> if (pci_probe_host() != 0) {
> --
> 2.41.0
>
Possible alternate variable naming:
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c7084f5..cd64d6e 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -46,12 +46,16 @@ static const char *region_type_name[] = {
[ PCI_REGION_TYPE_PREFMEM ] = "prefmem",
};
+// Memory ranges exported to legacy ACPI type table generation
u64 pcimem_start = BUILD_PCIMEM_START;
u64 pcimem_end = BUILD_PCIMEM_END;
u64 pcimem64_start = BUILD_PCIMEM64_START;
u64 pcimem64_end = BUILD_PCIMEM64_END;
-u64 pci_io_low_end = 0xa000;
-u32 pci_use_64bit = 0;
+
+// Memory resource allocation tracking
+static u64 pci_io_low_end = 0xa000;
+static u32 pci_use_64bit = 0;
+static u64 pci_mem64_top = 0;
struct pci_region_entry {
struct pci_device *dev;
@@ -966,8 +970,9 @@ static int pci_bios_check_devices(struct pci_bus *busses)
int resource_optional = 0;
if (hotplug_support == HOTPLUG_PCIE)
resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
-if (hotplug_support && pci_use_64bit && is64 && (type ==
PCI_REGION_TYPE_PREFMEM))
-align = (u64)1 << (CPUPhysBits - 11);
+if (hotplug_support && pci_use_64bit && is64
+&& (type == PCI_REGION_TYPE_PREFMEM))
+
Re: [SeaBIOS] [PATCH v5] limit physical address space size
On Fri, Nov 10, 2023 at 11:44:48AM -0500, Kevin O'Connor wrote:
> On Fri, Nov 10, 2023 at 12:04:24PM +0100, Gerd Hoffmann wrote:
> > -if (CPUPhysBits) {
> > -u64 top = 1LL << CPUPhysBits;
> > +if (pci_phys_bits) {
>
> FYI, this is a change in behavior - previously this condition would
> have been taken even if CPULongMode or RamSizeOver4G is false. I'm
> not sure if this change is intentional. (My example patch below
> follows your lead here.)
>
On closer inspection, I think this change in behavior was not
intended. How about variable names like the below instead.
-Kevin
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c7084f5..6b13cd5 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -46,12 +46,16 @@ static const char *region_type_name[] = {
[ PCI_REGION_TYPE_PREFMEM ] = "prefmem",
};
+// Memory ranges exported to legacy ACPI type table generation
u64 pcimem_start = BUILD_PCIMEM_START;
u64 pcimem_end = BUILD_PCIMEM_END;
u64 pcimem64_start = BUILD_PCIMEM64_START;
u64 pcimem64_end = BUILD_PCIMEM64_END;
-u64 pci_io_low_end = 0xa000;
-u32 pci_use_64bit = 0;
+
+// Resource allocation limits
+static u64 pci_io_low_end = 0xa000;
+static u64 pci_mem64_top = 0;
+static u32 pci_pad_mem64 = 0;
struct pci_region_entry {
struct pci_device *dev;
@@ -966,8 +970,9 @@ static int pci_bios_check_devices(struct pci_bus *busses)
int resource_optional = 0;
if (hotplug_support == HOTPLUG_PCIE)
resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
-if (hotplug_support && pci_use_64bit && is64 && (type ==
PCI_REGION_TYPE_PREFMEM))
-align = (u64)1 << (CPUPhysBits - 11);
+if (hotplug_support && pci_pad_mem64 && is64
+&& (type == PCI_REGION_TYPE_PREFMEM))
+align = pci_mem64_top >> 11;
if (align > sum && hotplug_support && !resource_optional)
sum = align; /* reserve min size for hot-plug */
if (size > sum) {
@@ -,7 +1116,7 @@ static void pci_bios_map_devices(struct pci_bus *busses)
panic("PCI: out of I/O address space\n");
dprintf(1, "PCI: 32: %016llx - %016llx\n", pcimem_start, pcimem_end);
-if (pci_use_64bit || pci_bios_init_root_regions_mem(busses)) {
+if (pci_pad_mem64 || pci_bios_init_root_regions_mem(busses)) {
struct pci_region r64_mem, r64_pref;
r64_mem.list.first = NULL;
r64_pref.list.first = NULL;
@@ -1131,14 +1136,13 @@ static void pci_bios_map_devices(struct pci_bus *busses)
r64_mem.base = le64_to_cpu(romfile_loadint("etc/reserved-memory-end",
0));
if (r64_mem.base < 0x1LL + RamSizeOver4G)
r64_mem.base = 0x1LL + RamSizeOver4G;
-if (CPUPhysBits) {
-u64 top = 1LL << CPUPhysBits;
+if (pci_mem64_top) {
u64 size = (ALIGN(sum_mem, (1LL<<30)) +
ALIGN(sum_pref, (1LL<<30)));
-if (pci_use_64bit)
-size = ALIGN(size, (1LL<<(CPUPhysBits-3)));
-if (r64_mem.base < top - size) {
-r64_mem.base = top - size;
+if (pci_pad_mem64)
+size = ALIGN(size, pci_mem64_top >> 3);
+if (r64_mem.base < pci_mem64_top - size) {
+r64_mem.base = pci_mem64_top - size;
}
if (e820_is_used(r64_mem.base, size))
r64_mem.base -= size;
@@ -1181,8 +1185,18 @@ pci_setup(void)
dprintf(3, "pci setup\n");
+if (CPUPhysBits) {
+pci_mem64_top = 1LL << CPUPhysBits;
+if (CPUPhysBits > 46) {
+// Old linux kernels have trouble dealing with more than 46
+// phys-bits, so avoid that for now. Seems to be a bug in the
+// virtio-pci driver. Reported: centos-7, ubuntu-18.04
+pci_mem64_top = 1LL << 46;
+}
+}
+
if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G)
-pci_use_64bit = 1;
+pci_pad_mem64 = 1;
dprintf(1, "=== PCI bus & bridge init ===\n");
if (pci_probe_host() != 0) {
[PATCH for-8.2] accel/tcg: Remove CF_LAST_IO
In cpu_exec_step_atomic, we did not set CF_LAST_IO, which can
lead to a loop with cpu_io_recompile.
But since 18a536f1f8 ("Always require can_do_io") we no longer need
a flag to indicate when the last insn should have can_do_io set, so
remove the flag entirely.
Reported-by: Clément Chigot
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1961
Signed-off-by: Richard Henderson
---
docs/devel/tcg-icount.rst| 6 --
include/exec/translation-block.h | 13 ++---
accel/tcg/cpu-exec.c | 2 +-
accel/tcg/tb-maint.c | 6 ++
accel/tcg/translate-all.c| 4 ++--
accel/tcg/translator.c | 22 +-
system/watchpoint.c | 6 ++
7 files changed, 22 insertions(+), 37 deletions(-)
diff --git a/docs/devel/tcg-icount.rst b/docs/devel/tcg-icount.rst
index 50c8e8dabc..7df883446a 100644
--- a/docs/devel/tcg-icount.rst
+++ b/docs/devel/tcg-icount.rst
@@ -62,12 +62,6 @@ To deal with this case, when an I/O access is made we:
- re-compile a single [1]_ instruction block for the current PC
- exit the cpu loop and execute the re-compiled block
-The new block is created with the CF_LAST_IO compile flag which
-ensures the final instruction translation starts with a call to
-gen_io_start() so we don't enter a perpetual loop constantly
-recompiling a single instruction block. For translators using the
-common translator_loop this is done automatically.
-
.. [1] sometimes two instructions if dealing with delay slots
Other I/O operations
diff --git a/include/exec/translation-block.h b/include/exec/translation-block.h
index b785751774..e2b26e16da 100644
--- a/include/exec/translation-block.h
+++ b/include/exec/translation-block.h
@@ -71,13 +71,12 @@ struct TranslationBlock {
#define CF_NO_GOTO_TB0x0200 /* Do not chain with goto_tb */
#define CF_NO_GOTO_PTR 0x0400 /* Do not chain with goto_ptr */
#define CF_SINGLE_STEP 0x0800 /* gdbstub single-step in effect */
-#define CF_LAST_IO 0x8000 /* Last insn may be an IO access. */
-#define CF_MEMI_ONLY 0x0001 /* Only instrument memory ops */
-#define CF_USE_ICOUNT0x0002
-#define CF_INVALID 0x0004 /* TB is stale. Set with @jmp_lock held */
-#define CF_PARALLEL 0x0008 /* Generate code for a parallel context */
-#define CF_NOIRQ 0x0010 /* Generate an uninterruptible TB */
-#define CF_PCREL 0x0020 /* Opcodes in TB are PC-relative */
+#define CF_MEMI_ONLY 0x1000 /* Only instrument memory ops */
+#define CF_USE_ICOUNT0x2000
+#define CF_INVALID 0x4000 /* TB is stale. Set with @jmp_lock held */
+#define CF_PARALLEL 0x8000 /* Generate code for a parallel context */
+#define CF_NOIRQ 0x0001 /* Generate an uninterruptible TB */
+#define CF_PCREL 0x0002 /* Opcodes in TB are PC-relative */
#define CF_CLUSTER_MASK 0xff00 /* Top 8 bits are cluster ID */
#define CF_CLUSTER_SHIFT 24
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 1a5bc90220..c938eb96f8 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -721,7 +721,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int
*ret)
&& cpu->neg.icount_decr.u16.low + cpu->icount_extra == 0) {
/* Execute just one insn to trigger exception pending in the log */
cpu->cflags_next_tb = (curr_cflags(cpu) & ~CF_USE_ICOUNT)
-| CF_LAST_IO | CF_NOIRQ | 1;
+| CF_NOIRQ | 1;
}
#endif
return false;
diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index e678d20dc2..3d2a896220 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -1083,8 +1083,7 @@ bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr,
uintptr_t pc)
if (current_tb_modified) {
/* Force execution of one insn next time. */
CPUState *cpu = current_cpu;
-cpu->cflags_next_tb =
-1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(current_cpu);
+cpu->cflags_next_tb = 1 | CF_NOIRQ | curr_cflags(current_cpu);
return true;
}
return false;
@@ -1154,8 +1153,7 @@ tb_invalidate_phys_page_range__locked(struct
page_collection *pages,
if (current_tb_modified) {
page_collection_unlock(pages);
/* Force execution of one insn next time. */
-current_cpu->cflags_next_tb =
-1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(current_cpu);
+current_cpu->cflags_next_tb = 1 | CF_NOIRQ | curr_cflags(current_cpu);
mmap_unlock();
cpu_loop_exit_noexc(current_cpu);
}
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index b263857ecc..79a88f5fb7 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -304,7 +304,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
if (phys_pc == -1) {
/* Generate a one-shot TB with 1 insn in it */
-cflags = (cflags & ~C
[PATCH] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Observed the following failure while booting the SEV-SNP guest and the
guest fails to boot with the smp parameters:
"-smp 192,sockets=1,dies=12,cores=8,threads=2".
qemu-system-x86_64: sev_snp_launch_update: SNP_LAUNCH_UPDATE ret=-5 fw_error=22
'Invalid parameter'
qemu-system-x86_64: SEV-SNP: CPUID validation failed for function 0x801e,
index: 0x0.
provided: eax:0x, ebx: 0x0100, ecx: 0x0b00, edx: 0x
expected: eax:0x, ebx: 0x0100, ecx: 0x0300, edx: 0x
qemu-system-x86_64: SEV-SNP: failed update CPUID page
Reason for the failure is due to overflowing of bits used for "Node per
processor" in CPUID Fn801E_ECX. This field's width is 3 bits wide and
can hold maximum value 0x7. With dies=12 (0xB), it overflows and spills
over into the reserved bits. In the case of SEV-SNP, this causes CPUID
enforcement failure and guest fails to boot.
The PPR documentation for CPUID_Fn801E_ECX [Node Identifiers]
=
BitsDescription
31:11 Reserved.
10:8NodesPerProcessor: Node per processor. Read-only.
ValidValues:
Value Description
0h 1 node per processor.
7h-1h Reserved.
7:0 NodeId: Node ID. Read-only. Reset: Fixed,XXh.
=
As in the spec, the valid value for "node per processor" is 0 and rest
are reserved.
Looking back at the history of decoding of CPUID_Fn801E_ECX, noticed
that there were cases where "node per processor" can be more than 1. It
is valid only for pre-F17h (pre-EPYC) architectures. For EPYC or later
CPUs, the linux kernel does not use this information to build the L3
topology.
Also noted that the CPUID Function 0x801E_ECX is available only when
TOPOEXT feature is enabled. This feature is enabled only for EPYC(F17h)
or later processors. So, previous generation of processors do not not
enumerate 0x801E_ECX leaf.
There could be some corner cases where the older guests could enable the
TOPOEXT feature by running with -cpu host, in which case legacy guests
might notice the topology change. To address those cases introduced a
new CPU property "legacy-multi-node". It will be true for older machine
types to maintain compatibility. By default, it will be false, so new
decoding will be used going forward.
The documentation is taken from Preliminary Processor Programming
Reference (PPR) for AMD Family 19h Model 11h, Revision B1 Processors 55901
Rev 0.25 - Oct 6, 2022.
Cc: qemu-sta...@nongnu.org
Fixes: 31ada106d891 ("Simplify CPUID_8000_001E for AMD")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger
---
hw/i386/pc.c | 4 +++-
target/i386/cpu.c | 18 ++
target/i386/cpu.h | 1 +
3 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 188bc9d0f8..624d5da146 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -77,7 +77,9 @@
{ "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },\
{ "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
-GlobalProperty pc_compat_8_1[] = {};
+GlobalProperty pc_compat_8_1[] = {
+{ TYPE_X86_CPU, "legacy-multi-node", "on" },
+};
const size_t pc_compat_8_1_len = G_N_ELEMENTS(pc_compat_8_1);
GlobalProperty pc_compat_8_0[] = {
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 358d9c0a65..baee9394a1 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -398,12 +398,9 @@ static void encode_topo_cpuid801e(X86CPU *cpu,
X86CPUTopoInfo *topo_info,
* 31:11 Reserved.
* 10:8 NodesPerProcessor: Node per processor. Read-only. Reset: XXXb.
* ValidValues:
- * Value Description
- * 000b 1 node per processor.
- * 001b 2 nodes per processor.
- * 010b Reserved.
- * 011b 4 nodes per processor.
- * 111b-100b Reserved.
+ * Value Description
+ * 0h 1 node per processor.
+ * 7h-1h Reserved.
* 7:0 NodeId: Node ID. Read-only. Reset: XXh.
*
* NOTE: Hardware reserves 3 bits for number of nodes per processor.
@@ -412,8 +409,12 @@ static void encode_topo_cpuid801e(X86CPU *cpu,
X86CPUTopoInfo *topo_info,
* NodeId is combination of node and socket_id which is already decoded
* in apic_id. Just use it by shifting.
*/
-*ecx = ((topo_info->dies_per_pkg - 1) << 8) |
- ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
+if (cpu->legacy_multi_node) {
+*ecx = ((topo_info->dies_per_pkg - 1) << 8) |
+ ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
+} else {
+*ecx = (cpu->apic_id >> apicid_pkg_offset(topo_info)) & 0xFF;
+}
*edx = 0;
}
@@ -7894,6 +7895,7 @@ static Property x86_cpu_properties[] = {
* own cache information (see x86_cpu_load_def()).
*/
DEFINE_PROP_BOOL("legacy-cac
[PATCH 0/8] Add powernv10 I2C devices and tests
This series of patches includes support, tests and fixes for adding PCA9552 and PCA9554 I2C devices to the powernv10 chip. The PCA9552 device is used for PCIe slot hotplug power control and monitoring, while the PCA9554 device is used for presence detection of IBM CableCard devices. Both devices are required by the Power Hypervisor Firmware on Power10 platforms. Glenn Miles (8): ppc/pnv: Add pca9552 to powernv10 for PCIe hotplug power control ppc/pnv: Wire up pca9552 GPIO pins for PCIe hotplug power control ppc/pnv: PNV I2C engines assigned incorrect XSCOM addresses ppc/pnv: Fix PNV I2C invalid status after reset ppc/pnv: Use resettable interface to reset child I2C buses misc: Add a pca9554 GPIO device model ppc/pnv: Add a pca9554 I2C device to powernv10 ppc/pnv: Test pnv i2c master and connected devices MAINTAINERS | 2 + hw/misc/Kconfig | 4 + hw/misc/meson.build | 1 + hw/misc/pca9554.c | 328 hw/ppc/Kconfig | 2 + hw/ppc/pnv.c| 35 +- hw/ppc/pnv_i2c.c| 47 ++- include/hw/misc/pca9554.h | 36 ++ include/hw/misc/pca9554_regs.h | 19 + tests/qtest/meson.build | 1 + tests/qtest/pnv-host-i2c-test.c | 653 11 files changed, 1106 insertions(+), 22 deletions(-) create mode 100644 hw/misc/pca9554.c create mode 100644 include/hw/misc/pca9554.h create mode 100644 include/hw/misc/pca9554_regs.h create mode 100644 tests/qtest/pnv-host-i2c-test.c -- 2.31.1
[PULL v2 00/11] qdev: Make array properties user accessible again
The following changes since commit ea10c3817814b8be75be22c78ea91d633b0d2532: Merge tag 'tracing-pull-request' of https://gitlab.com/stefanha/qemu into staging (2023-11-10 08:10:43 +0800) are available in the Git repository at: https://repo.or.cz/qemu/kevin.git tags/qdev-array-prop for you to fetch changes up to b06f8b500da2a5a73dfb15de17cd96ad2385fdc7: qdev: Rework array properties based on list visitor (2023-11-10 18:19:19 +0100) qdev: Make array properties user accessible again Kevin Wolf (11): hw/i386/pc: Use qdev_prop_set_array() hw/arm/mps2-tz: Use qdev_prop_set_array() hw/arm/mps2: Use qdev_prop_set_array() hw/arm/sbsa-ref: Use qdev_prop_set_array() hw/arm/vexpress: Use qdev_prop_set_array() hw/arm/virt: Use qdev_prop_set_array() hw/arm/xlnx-versal: Use qdev_prop_set_array() hw/rx/rx62n: Use qdev_prop_set_array() qom: Add object_property_set_default_list() qdev: Make netdev properties work as list elements qdev: Rework array properties based on list visitor include/hw/qdev-properties.h | 35 +++--- include/qom/object.h | 8 ++ hw/arm/mps2-tz.c | 10 +- hw/arm/mps2.c| 12 +- hw/arm/sbsa-ref.c| 7 +- hw/arm/vexpress.c| 21 ++-- hw/arm/virt.c| 31 ++--- hw/arm/xlnx-versal.c | 9 +- hw/core/qdev-properties-system.c | 2 +- hw/core/qdev-properties.c| 237 ++- hw/i386/pc.c | 8 +- hw/rx/rx62n.c| 19 ++-- qom/object.c | 6 + 13 files changed, 257 insertions(+), 148 deletions(-)
[PATCH 4/8] ppc/pnv: Fix PNV I2C invalid status after reset
The PNV I2C Controller was clearing the status register
after a reset without repopulating the "upper threshold
for I2C ports", "Command Complete" and the SCL/SDA input
level fields.
Fixed this for resets caused by a system reset as well
as from writing to the "Immediate Reset" register.
Signed-off-by: Glenn Miles
---
hw/ppc/pnv_i2c.c | 42 ++
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c
index b2c738da50..f80589157b 100644
--- a/hw/ppc/pnv_i2c.c
+++ b/hw/ppc/pnv_i2c.c
@@ -462,6 +462,23 @@ static uint64_t pnv_i2c_xscom_read(void *opaque, hwaddr
addr,
return val;
}
+static void pnv_i2c_reset(void *dev)
+{
+PnvI2C *i2c = PNV_I2C(dev);
+
+memset(i2c->regs, 0, sizeof(i2c->regs));
+
+i2c->regs[I2C_STAT_REG] =
+SETFIELD(I2C_STAT_UPPER_THRS, 0ull, i2c->num_busses - 1) |
+I2C_STAT_CMD_COMP | I2C_STAT_SCL_INPUT_LEVEL |
+I2C_STAT_SDA_INPUT_LEVEL;
+i2c->regs[I2C_EXTD_STAT_REG] =
+SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) |
+SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last version */
+
+fifo8_reset(&i2c->fifo);
+}
+
static void pnv_i2c_xscom_write(void *opaque, hwaddr addr,
uint64_t val, unsigned size)
{
@@ -499,16 +516,7 @@ static void pnv_i2c_xscom_write(void *opaque, hwaddr addr,
break;
case I2C_RESET_I2C_REG:
-i2c->regs[I2C_MODE_REG] = 0;
-i2c->regs[I2C_CMD_REG] = 0;
-i2c->regs[I2C_WATERMARK_REG] = 0;
-i2c->regs[I2C_INTR_MASK_REG] = 0;
-i2c->regs[I2C_INTR_COND_REG] = 0;
-i2c->regs[I2C_INTR_RAW_COND_REG] = 0;
-i2c->regs[I2C_STAT_REG] = 0;
-i2c->regs[I2C_RESIDUAL_LEN_REG] = 0;
-i2c->regs[I2C_EXTD_STAT_REG] &=
-(I2C_EXTD_STAT_FIFO_SIZE | I2C_EXTD_STAT_I2C_VERSION);
+pnv_i2c_reset(i2c);
break;
case I2C_RESET_ERRORS:
@@ -620,20 +628,6 @@ static int pnv_i2c_dt_xscom(PnvXScomInterface *dev, void
*fdt,
return 0;
}
-static void pnv_i2c_reset(void *dev)
-{
-PnvI2C *i2c = PNV_I2C(dev);
-
-memset(i2c->regs, 0, sizeof(i2c->regs));
-
-i2c->regs[I2C_STAT_REG] = I2C_STAT_CMD_COMP;
-i2c->regs[I2C_EXTD_STAT_REG] =
-SETFIELD(I2C_EXTD_STAT_FIFO_SIZE, 0ull, PNV_I2C_FIFO_SIZE) |
-SETFIELD(I2C_EXTD_STAT_I2C_VERSION, 0ull, 23); /* last version */
-
-fifo8_reset(&i2c->fifo);
-}
-
static void pnv_i2c_realize(DeviceState *dev, Error **errp)
{
PnvI2C *i2c = PNV_I2C(dev);
--
2.31.1
[PATCH 8/8] ppc/pnv: Test pnv i2c master and connected devices
Tests the following for both P9 and P10:
- I2C master POR status
- I2C master status after immediate reset
Tests the following for P10 only:
- Config pca9552 hotplug device pins as inputs then
Read the INPUT0/1 registers to verify all pins are high
- Connected GPIO pin tests of P10 PCA9552 device. Tests
output of pins 0-4 affect input of pins 5-9 respectively.
- PCA9554 GPIO pins test. Tests input and ouput functionality.
Signed-off-by: Glenn Miles
---
tests/qtest/meson.build | 1 +
tests/qtest/pnv-host-i2c-test.c | 653
2 files changed, 654 insertions(+)
create mode 100644 tests/qtest/pnv-host-i2c-test.c
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 47dabf91d0..fbb0bd204c 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -163,6 +163,7 @@ qtests_ppc64 = \
qtests_ppc + \
(config_all_devices.has_key('CONFIG_PSERIES') ? ['device-plug-test'] : []) +
\
(config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-xscom-test'] : []) +
\
+ (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-host-i2c-test'] : [])
+ \
(config_all_devices.has_key('CONFIG_PSERIES') ? ['rtas-test'] : []) +
\
(slirp.found() ? ['pxe-test'] : []) + \
(config_all_devices.has_key('CONFIG_USB_UHCI') ? ['usb-hcd-uhci-test'] : [])
+ \
diff --git a/tests/qtest/pnv-host-i2c-test.c b/tests/qtest/pnv-host-i2c-test.c
new file mode 100644
index 00..8002dfcc0c
--- /dev/null
+++ b/tests/qtest/pnv-host-i2c-test.c
@@ -0,0 +1,653 @@
+/*
+ * QTest testcase for PowerNV 10 Host I2C Communications
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "hw/misc/pca9554_regs.h"
+#include "hw/misc/pca9552_regs.h"
+
+#define PPC_BIT(bit)(0x8000ULL >> (bit))
+#define PPC_BIT32(bit) (0x8000 >> (bit))
+#define PPC_BIT8(bit) (0x80 >> (bit))
+#define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
+#define PPC_BITMASK32(bs, be) ((PPC_BIT32(bs) - PPC_BIT32(be)) | \
+ PPC_BIT32(bs))
+
+#define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1)
+#define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m))
+#define SETFIELD(m, v, val) \
+(((v) & ~(m)) | typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
+
+#define P10_XSCOM_BASE 0x000603fcull
+#define PNV10_CHIP_MAX_I2C 5
+#define PNV10_XSCOM_I2CM_BASE 0xa
+#define PNV10_XSCOM_I2CM_SIZE 0x1000
+
+/* I2C FIFO register */
+#define I2C_FIFO_REG0x4
+#define I2C_FIFOPPC_BITMASK(0, 7)
+
+/* I2C command register */
+#define I2C_CMD_REG 0x5
+#define I2C_CMD_WITH_START PPC_BIT(0)
+#define I2C_CMD_WITH_ADDR PPC_BIT(1)
+#define I2C_CMD_READ_CONT PPC_BIT(2)
+#define I2C_CMD_WITH_STOP PPC_BIT(3)
+#define I2C_CMD_INTR_STEERING PPC_BITMASK(6, 7) /* P9 */
+#define I2C_CMD_INTR_STEER_HOST 1
+#define I2C_CMD_INTR_STEER_OCC2
+#define I2C_CMD_DEV_ADDRPPC_BITMASK(8, 14)
+#define I2C_CMD_READ_NOT_WRITE PPC_BIT(15)
+#define I2C_CMD_LEN_BYTES PPC_BITMASK(16, 31)
+#define I2C_MAX_TFR_LEN 0xfff0ull
+
+/* I2C mode register */
+#define I2C_MODE_REG0x6
+#define I2C_MODE_BIT_RATE_DIV PPC_BITMASK(0, 15)
+#define I2C_MODE_PORT_NUM PPC_BITMASK(16, 21)
+#define I2C_MODE_ENHANCED PPC_BIT(28)
+#define I2C_MODE_DIAGNOSTIC PPC_BIT(29)
+#define I2C_MODE_PACING_ALLOW PPC_BIT(30)
+#define I2C_MODE_WRAP PPC_BIT(31)
+
+/* I2C watermark register */
+#define I2C_WATERMARK_REG 0x7
+#define I2C_WATERMARK_HIGH PPC_BITMASK(16, 19)
+#define I2C_WATERMARK_LOW PPC_BITMASK(24, 27)
+
+/*
+ * I2C interrupt mask and condition registers
+ *
+ * NB: The function of 0x9 and 0xa changes depending on whether you're reading
+ * or writing to them. When read they return the interrupt condition bits
+ * and on writes they update the interrupt mask register.
+ *
+ * The bit definitions are the same for all the interrupt registers.
+ */
+#define I2C_INTR_MASK_REG 0x8
+
+#define I2C_INTR_RAW_COND_REG 0x9 /* read */
+#define I2C_INTR_MASK_OR_REG0x9 /* write*/
+
+#define I2C_INTR_COND_REG 0xa /* read */
+#define I2C_INTR_MASK_AND_REG 0xa /* write */
+
+#define I2C_INTR_ALLPPC_BITMASK(16, 31)
+#define I2C_INTR_INVALID_CMDPPC_BIT(16)
+#define I2C_INTR_LBUS_PARITY_ERRPPC_BIT(17)
+#define I2C_INTR_BKEND_OVERR
[PATCH 6/8] misc: Add a pca9554 GPIO device model
Specs are available here:
https://www.nxp.com/docs/en/data-sheet/PCA9554_9554A.pdf
This is a simple model supporting the basic registers for GPIO
mode. The device also supports an interrupt output line but the
model does not yet support this.
Signed-off-by: Glenn Miles
---
MAINTAINERS| 2 +
hw/misc/pca9554.c | 328 +
include/hw/misc/pca9554.h | 36
include/hw/misc/pca9554_regs.h | 19 ++
4 files changed, 385 insertions(+)
create mode 100644 hw/misc/pca9554.c
create mode 100644 include/hw/misc/pca9554.h
create mode 100644 include/hw/misc/pca9554_regs.h
diff --git a/MAINTAINERS b/MAINTAINERS
index e73a3ff544..209080e1a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1504,8 +1504,10 @@ F: hw/ppc/pnv*
F: hw/intc/pnv*
F: hw/intc/xics_pnv.c
F: hw/pci-host/pnv*
+F: hw/misc/pca9554.c
F: include/hw/ppc/pnv*
F: include/hw/pci-host/pnv*
+F: include/hw/misc/pca9554*.h
F: pc-bios/skiboot.lid
F: tests/qtest/pnv*
diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
new file mode 100644
index 00..778b32e443
--- /dev/null
+++ b/hw/misc/pca9554.c
@@ -0,0 +1,328 @@
+/*
+ * PCA9554 I/O port
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/bitops.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/pca9554.h"
+#include "hw/misc/pca9554_regs.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "trace.h"
+#include "qom/object.h"
+
+struct PCA9554Class {
+/*< private >*/
+I2CSlaveClass parent_class;
+/*< public >*/
+};
+typedef struct PCA9554Class PCA9554Class;
+
+DECLARE_CLASS_CHECKERS(PCA9554Class, PCA9554,
+ TYPE_PCA9554)
+
+#define PCA9554_PIN_LOW 0x0
+#define PCA9554_PIN_HIZ 0x1
+
+static const char *pin_state[] = {"low", "high"};
+
+static void pca9554_update_pin_input(PCA9554State *s)
+{
+int i;
+uint8_t config = s->regs[PCA9554_CONFIG];
+uint8_t output = s->regs[PCA9554_OUTPUT];
+uint8_t internal_state = config | output;
+
+for (i = 0; i < PCA9554_PIN_COUNT; i++) {
+uint8_t bit_mask = 1 << i;
+uint8_t internal_pin_state = (internal_state >> i) & 0x1;
+uint8_t old_value = s->regs[PCA9554_INPUT] & bit_mask;
+uint8_t new_value;
+
+switch (internal_pin_state) {
+case PCA9554_PIN_LOW:
+s->regs[PCA9554_INPUT] &= ~bit_mask;
+break;
+case PCA9554_PIN_HIZ:
+/*
+ * pullup sets it to a logical 1 unless
+ * external device drives it low.
+ */
+if (s->ext_state[i] == PCA9554_PIN_LOW) {
+s->regs[PCA9554_INPUT] &= ~bit_mask;
+} else {
+s->regs[PCA9554_INPUT] |= bit_mask;
+}
+break;
+default:
+break;
+}
+
+/* update irq state only if pin state changed */
+new_value = s->regs[PCA9554_INPUT] & bit_mask;
+if (new_value != old_value) {
+if (new_value) {
+/* changed from 0 to 1 */
+qemu_set_irq(s->gpio_out[i], 1);
+} else {
+/* changed from 1 to 0 */
+qemu_set_irq(s->gpio_out[i], 0);
+}
+}
+}
+}
+
+static uint8_t pca9554_read(PCA9554State *s, uint8_t reg)
+{
+switch (reg) {
+case PCA9554_INPUT:
+return s->regs[PCA9554_INPUT] ^ s->regs[PCA9554_POLARITY];
+case PCA9554_OUTPUT:
+case PCA9554_POLARITY:
+case PCA9554_CONFIG:
+return s->regs[reg];
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to register %d\n",
+ __func__, reg);
+return 0xFF;
+}
+}
+
+static void pca9554_write(PCA9554State *s, uint8_t reg, uint8_t data)
+{
+switch (reg) {
+case PCA9554_OUTPUT:
+case PCA9554_CONFIG:
+s->regs[reg] = data;
+pca9554_update_pin_input(s);
+break;
+case PCA9554_POLARITY:
+s->regs[reg] = data;
+break;
+case PCA9554_INPUT:
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to register %d\n",
+ __func__, reg);
+}
+}
+
+static uint8_t pca9554_recv(I2CSlave *i2c)
+{
+PCA9554State *s = PCA9554(i2c);
+uint8_t ret;
+
+ret = pca9554_read(s, s->pointer & 0x3);
+
+return ret;
+}
+
+static int pca9554_send(I2CSlave *i2c, uint8_t data)
+{
+PCA9554State *s = PCA9554(i2c);
+
+/* First byte sent by is the register address */
+if (s->len == 0) {
+s->pointer = data;
+s->len++;
+} else {
+pca9554_write(s, s->pointer & 0x3, data);
+}
+
+return 0;
+}
+
+static int pca9554_event(I2CSlave *i2c, enum i2c_event event)
+{
+PCA9554State *s = PCA9554(i2c);
+
+s->len = 0;
+
[PATCH 1/8] ppc/pnv: Add pca9552 to powernv10 for PCIe hotplug power control
The Power Hypervisor code expects to see a pca9552 device connected to the 3rd PNV I2C engine on port 1 at I2C address 0x63 (or left- justified address of 0xC6). This is used by hypervisor code to control PCIe slot power during hotplug events. Signed-off-by: Glenn Miles --- hw/ppc/Kconfig | 1 + hw/ppc/pnv.c | 7 +++ 2 files changed, 8 insertions(+) diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig index 56f0475a8e..f77ca773cf 100644 --- a/hw/ppc/Kconfig +++ b/hw/ppc/Kconfig @@ -32,6 +32,7 @@ config POWERNV select XIVE select FDT_PPC select PCI_POWERNV +select PCA9552 config PPC405 bool diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 9c29727337..7afaf1008f 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -1877,6 +1877,13 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp) qdev_get_gpio_in(DEVICE(&chip10->psi), PSIHB9_IRQ_SBE_I2C)); } + +/* + * Add a PCA9552 I2C device for PCIe hotplug control + * to engine 2, bus 1, address 0x63 + */ +i2c_slave_create_simple(chip10->i2c[2].busses[1], "pca9552", 0x63); + } static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr) -- 2.31.1
[PATCH 3/8] ppc/pnv: PNV I2C engines assigned incorrect XSCOM addresses
The PNV I2C engines for power9 and power10 were being assigned a base
XSCOM address that was off by one I2C engine's address range such
that engine 0 had engine 1's address and so on. The xscom address
assignment was being based on the device tree engine numbering, which
starts at 1. Rather than changing the device tree numbering to start
with 0, the addressing was changed to be based on the existing device
tree numbers minus one.
Signed-off-by: Glenn Miles
---
hw/ppc/pnv.c | 6 --
hw/ppc/pnv_i2c.c | 2 +-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0b24d7d8ed..516434bc9c 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1623,7 +1623,8 @@ static void pnv_chip_power9_realize(DeviceState *dev,
Error **errp)
return;
}
pnv_xscom_add_subregion(chip, PNV9_XSCOM_I2CM_BASE +
- chip9->i2c[i].engine * PNV9_XSCOM_I2CM_SIZE,
+(chip9->i2c[i].engine - 1) *
+PNV9_XSCOM_I2CM_SIZE,
&chip9->i2c[i].xscom_regs);
qdev_connect_gpio_out(DEVICE(&chip9->i2c[i]), 0,
qdev_get_gpio_in(DEVICE(&chip9->psi),
@@ -1871,7 +1872,8 @@ static void pnv_chip_power10_realize(DeviceState *dev,
Error **errp)
return;
}
pnv_xscom_add_subregion(chip, PNV10_XSCOM_I2CM_BASE +
-chip10->i2c[i].engine * PNV10_XSCOM_I2CM_SIZE,
+(chip10->i2c[i].engine - 1) *
+PNV10_XSCOM_I2CM_SIZE,
&chip10->i2c[i].xscom_regs);
qdev_connect_gpio_out(DEVICE(&chip10->i2c[i]), 0,
qdev_get_gpio_in(DEVICE(&chip10->psi),
diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c
index f75e59e709..b2c738da50 100644
--- a/hw/ppc/pnv_i2c.c
+++ b/hw/ppc/pnv_i2c.c
@@ -593,7 +593,7 @@ static int pnv_i2c_dt_xscom(PnvXScomInterface *dev, void
*fdt,
int i2c_offset;
const char i2c_compat[] = "ibm,power8-i2cm\0ibm,power9-i2cm";
uint32_t i2c_pcba = PNV9_XSCOM_I2CM_BASE +
-i2c->engine * PNV9_XSCOM_I2CM_SIZE;
+(i2c->engine - 1) * PNV9_XSCOM_I2CM_SIZE;
uint32_t reg[2] = {
cpu_to_be32(i2c_pcba),
cpu_to_be32(PNV9_XSCOM_I2CM_SIZE)
--
2.31.1
[PATCH 5/8] ppc/pnv: Use resettable interface to reset child I2C buses
The QEMU I2C buses and devices use the resettable
interface for resetting while the PNV I2C controller
and parent buses and devices have not yet transitioned
to this new interface and use the old reset strategy.
This was preventing the I2C buses and devices wired
to the PNV I2C controller from being reset.
The short term fix for this is to have the PNV I2C
Controller's reset function explicitly call the resettable
interface function, bus_cold_reset(), on all child
I2C buses.
The long term fix should be to transition all PNV parent
devices and buses to use the resettable interface so that
all child buses and devices are automatically reset.
Signed-off-by: Glenn Miles
---
hw/ppc/pnv_i2c.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c
index f80589157b..9ced596b98 100644
--- a/hw/ppc/pnv_i2c.c
+++ b/hw/ppc/pnv_i2c.c
@@ -628,6 +628,19 @@ static int pnv_i2c_dt_xscom(PnvXScomInterface *dev, void
*fdt,
return 0;
}
+static void pnv_i2c_sys_reset(void *dev)
+{
+int port;
+PnvI2C *i2c = PNV_I2C(dev);
+
+pnv_i2c_reset(dev);
+
+/* reset all buses connected to this i2c controller */
+for (port = 0; port < i2c->num_busses; port++) {
+bus_cold_reset(BUS(i2c->busses[port]));
+}
+}
+
static void pnv_i2c_realize(DeviceState *dev, Error **errp)
{
PnvI2C *i2c = PNV_I2C(dev);
@@ -648,7 +661,7 @@ static void pnv_i2c_realize(DeviceState *dev, Error **errp)
fifo8_create(&i2c->fifo, PNV_I2C_FIFO_SIZE);
-qemu_register_reset(pnv_i2c_reset, dev);
+qemu_register_reset(pnv_i2c_sys_reset, dev);
qdev_init_gpio_out(DEVICE(dev), &i2c->psi_irq, 1);
}
--
2.31.1
[PATCH 7/8] ppc/pnv: Add a pca9554 I2C device to powernv10
The Power Hypervisor code expects to see a pca9554 device connected
to the 3rd PNV I2C engine on port 1 at I2C address 0x25 (or left-
justified address of 0x4A). This is used by the hypervisor code to
detect if a "Cable Card" is present.
Signed-off-by: Glenn Miles
---
hw/misc/Kconfig | 4
hw/misc/meson.build | 1 +
hw/ppc/Kconfig | 1 +
hw/ppc/pnv.c| 5 +
4 files changed, 11 insertions(+)
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index cc8a8c1418..c347a132c2 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -34,6 +34,10 @@ config PCA9552
bool
depends on I2C
+config PCA9554
+bool
+depends on I2C
+
config I2C_ECHO
bool
default y if TEST_DEVICES
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 36c20d5637..c39410e4a7 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -4,6 +4,7 @@ system_ss.add(when: 'CONFIG_FW_CFG_DMA', if_true:
files('vmcoreinfo.c'))
system_ss.add(when: 'CONFIG_ISA_DEBUG', if_true: files('debugexit.c'))
system_ss.add(when: 'CONFIG_ISA_TESTDEV', if_true: files('pc-testdev.c'))
system_ss.add(when: 'CONFIG_PCA9552', if_true: files('pca9552.c'))
+system_ss.add(when: 'CONFIG_PCA9554', if_true: files('pca9554.c'))
system_ss.add(when: 'CONFIG_PCI_TESTDEV', if_true: files('pci-testdev.c'))
system_ss.add(when: 'CONFIG_UNIMP', if_true: files('unimp.c'))
system_ss.add(when: 'CONFIG_EMPTY_SLOT', if_true: files('empty_slot.c'))
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index f77ca773cf..2302778265 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -33,6 +33,7 @@ config POWERNV
select FDT_PPC
select PCI_POWERNV
select PCA9552
+select PCA9554
config PPC405
bool
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 516434bc9c..b6b4164d78 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1903,6 +1903,11 @@ static void pnv_chip_power10_realize(DeviceState *dev,
Error **errp)
qdev_connect_gpio_out(DEVICE(hotplug), 4,
qdev_get_gpio_in(DEVICE(hotplug), 9));
+/*
+ * Add a PCA9554 I2C device for cable card presence detection
+ * to engine 2, bus 1, address 0x25
+ */
+i2c_slave_create_simple(chip10->i2c[2].busses[1], "pca9554", 0x25);
}
static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr)
--
2.31.1
[PATCH 2/8] ppc/pnv: Wire up pca9552 GPIO pins for PCIe hotplug power control
For power10, a pca9552 device is used for PCIe slot hotplug power control by the Power Hypervisor code. The code expects that some time after it enables power to a PCIe slot by asserting one of the pca9552 GPIO pins 0-4, it should see a "power good" signal asserted on one of pca9552 GPIO pins 5-9. To simulate this behavior, we simply connect the GPIO outputs for pins 0-4 to the GPIO inputs for pins 5-9. Each PCIe slot is assigned 3 GPIO pins on the pca9552 device, for control of up to 5 PCIe slots. The per-slot signal names are: SLOTx_EN...PHYP uses this as an output to enable slot power. We connect this to the SLOTx_PG pin to simulate a PGOOD signal. SLOTx_PG...PHYP uses this as in input to detect PGOOD for the slot. For our purposes we just connect this to the SLOTx_EN output. SLOTx_Control..PHYP uses this as an output to prevent a race condition in the real hotplug circuitry, but we can ignore this output for simulation. Signed-off-by: Glenn Miles --- hw/ppc/pnv.c | 19 ++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 7afaf1008f..0b24d7d8ed 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -1882,7 +1882,24 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp) * Add a PCA9552 I2C device for PCIe hotplug control * to engine 2, bus 1, address 0x63 */ -i2c_slave_create_simple(chip10->i2c[2].busses[1], "pca9552", 0x63); +I2CSlave* hotplug = i2c_slave_create_simple(chip10->i2c[2].busses[1], +"pca9552", 0x63); + +/* + * Connect PCA9552 GPIO pins 0-4 (SLOTx_EN) outputs to GPIO pins 5-9 + * (SLOTx_PG) inputs in order to fake the pgood state of PCIe slots after + * hypervisor code sets a SLOTx_EN pin high. + */ +qdev_connect_gpio_out(DEVICE(hotplug), 0, + qdev_get_gpio_in(DEVICE(hotplug), 5)); +qdev_connect_gpio_out(DEVICE(hotplug), 1, + qdev_get_gpio_in(DEVICE(hotplug), 6)); +qdev_connect_gpio_out(DEVICE(hotplug), 2, + qdev_get_gpio_in(DEVICE(hotplug), 7)); +qdev_connect_gpio_out(DEVICE(hotplug), 3, + qdev_get_gpio_in(DEVICE(hotplug), 8)); +qdev_connect_gpio_out(DEVICE(hotplug), 4, + qdev_get_gpio_in(DEVICE(hotplug), 9)); } -- 2.31.1
[PATCH for-8.2] hw/riscv/virt.c: do create_fdt() earlier, add finalize_fdt()
Commit 49554856f0 fixed a problem, where TPM devices were not appearing
in the FDT, by delaying the FDT creation up until virt_machine_done().
This create a side effect (see gitlab #1925) - devices that need access
to the '/chosen' FDT node during realize() stopped working because, at
that point, we don't have a FDT.
This happens because our FDT creation is monolithic, but it doesn't need
to be. We can add the needed FDT components for realize() time and, at
the same time, do another FDT round where we account for dynamic sysbus
devices. In other words, the problem fixed by 49554856f0 could also be
fixed by postponing only create_fdt_sockets() and its dependencies,
leaving everything else from create_fdt() to be done during init().
Split the FDT creation in two parts:
- create_fdt(), now moved back to virt_machine_init(), will create FDT
nodes that doesn't depend on additional (dynamic) devices from the
sysbus;
- a new finalize_fdt() step is added, where create_fdt_sockets() and
friends is executed, accounting for the dynamic sysbus devices that
were added during realize().
This will make both use cases happy: TPM devices are still working as
intended, and devices such as 'guest-loader' have a FDT to work on
during realize().
Fixes: 49554856f0 ("riscv: Generate devicetree only after machine
initialization is complete")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1925
Signed-off-by: Daniel Henrique Barboza
---
hw/riscv/virt.c | 71 +
1 file changed, 42 insertions(+), 29 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index c7fc97e273..d2eac24156 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -962,7 +962,6 @@ static void create_fdt_uart(RISCVVirtState *s, const
MemMapEntry *memmap,
qemu_fdt_setprop_cells(ms->fdt, name, "interrupts", UART0_IRQ, 0x4);
}
-qemu_fdt_add_subnode(ms->fdt, "/chosen");
qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", name);
g_free(name);
}
@@ -1023,11 +1022,29 @@ static void create_fdt_fw_cfg(RISCVVirtState *s, const
MemMapEntry *memmap)
g_free(nodename);
}
-static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
+static void finalize_fdt(RISCVVirtState *s)
{
-MachineState *ms = MACHINE(s);
uint32_t phandle = 1, irq_mmio_phandle = 1, msi_pcie_phandle = 1;
uint32_t irq_pcie_phandle = 1, irq_virtio_phandle = 1;
+
+create_fdt_sockets(s, virt_memmap, &phandle, &irq_mmio_phandle,
+ &irq_pcie_phandle, &irq_virtio_phandle,
+ &msi_pcie_phandle);
+
+create_fdt_virtio(s, virt_memmap, irq_virtio_phandle);
+
+create_fdt_pcie(s, virt_memmap, irq_pcie_phandle, msi_pcie_phandle);
+
+create_fdt_reset(s, virt_memmap, &phandle);
+
+create_fdt_uart(s, virt_memmap, irq_mmio_phandle);
+
+create_fdt_rtc(s, virt_memmap, irq_mmio_phandle);
+}
+
+static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
+{
+MachineState *ms = MACHINE(s);
uint8_t rng_seed[32];
ms->fdt = create_device_tree(&s->fdt_size);
@@ -1047,28 +1064,16 @@ static void create_fdt(RISCVVirtState *s, const
MemMapEntry *memmap)
qemu_fdt_setprop_cell(ms->fdt, "/soc", "#size-cells", 0x2);
qemu_fdt_setprop_cell(ms->fdt, "/soc", "#address-cells", 0x2);
-create_fdt_sockets(s, memmap, &phandle, &irq_mmio_phandle,
- &irq_pcie_phandle, &irq_virtio_phandle,
- &msi_pcie_phandle);
-
-create_fdt_virtio(s, memmap, irq_virtio_phandle);
-
-create_fdt_pcie(s, memmap, irq_pcie_phandle, msi_pcie_phandle);
-
-create_fdt_reset(s, memmap, &phandle);
-
-create_fdt_uart(s, memmap, irq_mmio_phandle);
-
-create_fdt_rtc(s, memmap, irq_mmio_phandle);
-
-create_fdt_flash(s, memmap);
-create_fdt_fw_cfg(s, memmap);
-create_fdt_pmu(s);
+qemu_fdt_add_subnode(ms->fdt, "/chosen");
/* Pass seed to RNG */
qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
qemu_fdt_setprop(ms->fdt, "/chosen", "rng-seed",
rng_seed, sizeof(rng_seed));
+
+create_fdt_flash(s, memmap);
+create_fdt_fw_cfg(s, memmap);
+create_fdt_pmu(s);
}
static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
@@ -1257,15 +1262,12 @@ static void virt_machine_done(Notifier *notifier, void
*data)
uint64_t kernel_entry = 0;
BlockBackend *pflash_blk0;
-/* load/create device tree */
-if (machine->dtb) {
-machine->fdt = load_device_tree(machine->dtb, &s->fdt_size);
-if (!machine->fdt) {
-error_report("load_device_tree() failed");
-exit(1);
-}
-} else {
-create_fdt(s, memmap);
+/*
+ * An user provided dtb must include everything, including
+ * dynamic sysbus devices. Our FDT needs to be finalized.
+ */
+if (machine->dtb == NULL) {
+finalize_fdt(s);
}
/*
@@ -1541,6 +1543,17 @@
Re: [PATCH] target/arm: Correct MTE tag checking for reverse-copy MOPS
On 10/11/23 17:25, Peter Maydell wrote:
When we are doing a FEAT_MOPS copy that must be performed backwards,
we call mte_mops_probe_rev(), passing it the address of the last byte
in the region we are probing. However, allocation_tag_mem_probe()
wants the address of the first byte to get the tag memory for.
Because we passed it (ptr, size) we could incorrectly trip the
allocation_tag_mem_probe() check for "does this access run across to
the following page", and if that following page happened not to be
valid then we would assert.
We know we will always be only dealing with a single page because the
code that calls mte_mops_probe_rev() ensures that. We could make
mte_mops_probe_rev() pass 'ptr - (size - 1)' to
allocation_tag_mem_probe(), but then we would have to adjust the
returned 'mem' pointer to get back to the tag RAM for the last byte
of the region. It's simpler to just pass in a size of 1 byte,
because we know that allocation_tag_mem_probe() in pure-probe
single-page mode doesn't care about the size.
Fixes: 69c51dc3723b ("target/arm: Implement MTE tag-checking functions for FEAT_MOPS
copies")
Signed-off-by: Peter Maydell
---
target/arm/tcg/mte_helper.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 1/1] plugins: Move the windows linking function to qemu
> Previously, a plugin author needed an implementation of the > __pfnDliFailureHook2 or __pfnDliNotifyHook2 hook in the plugin. Now all > they need is a null exported pointer with the right name (as in > win32_linker.c). If QEMU finds this, it will set it to the hook > function, which has now moved into qemu (os-win32.c). I have a new idea for this. We've made the qemu_plugin_api.lib file which is a delaylib with all the symbol names of all the api functions, so windows can do the whole delay-linking thing. We could also put into that archive the object win32_linker.o: ar -q qemu_plugin.api.lib ../whatever/win32_linker.o Then hopefully when a plugin links to this, it gets the __pfnDliFailureHook2 symbol defined and set up and everything would work. Except gcc strips out any unreferenced symbols from static libs when linking. So the plugin would have to be linked thusly: gcc -shared -o my_plugin.dll -Wl,-u,__pfnDliFailureHook2 my_plugin.o qemu_plugin_api.lib But no other qemu-fiddling-with-things or extra code in plugins required. Hmm. Feels like half a solution. I wonder if there's a way to mark symbols as "always required despite what dead code analysis says". Greg.
[PATCH v2 0/2] s390x/pci: small set of fixes
The following set of changes are associated with issues exposed by testing of the 'vfio: Adopt iommufd' series. The first patch fixes an existing assumption that a vfio device will always have a group fd (which is no longer true if cdev is used, which can only happen once the iommufd backend is used). This patch really only needs to go into 8.2 if the 'vfio: Adopt iommufd' series does (but would be fine to go into 8.2 without it too). The second patch fixes an issue where we do not detect that a vfio DMA limit was never read from vfio. This is actually an existing bug as it's possible for an older host kernel to be missing this support today; so ideally this one should be targeted for 8.2 regardless. Changes for v2: - minor style changes (Phil, Thomas) Matthew Rosato (2): s390x/pci: bypass vfio DMA counting when using cdev s390x/pci: only limit DMA aperture if vfio DMA limit reported hw/s390x/s390-pci-vfio.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) -- 2.41.0
[PATCH v2 2/2] s390x/pci: only limit DMA aperture if vfio DMA limit reported
If the host kernel lacks vfio DMA limit reporting, do not attempt
to shrink the guest DMA aperture.
Fixes: df202e3ff3 ("s390x/pci: shrink DMA aperture to be bound by vfio DMA
limit")
Signed-off-by: Matthew Rosato
---
hw/s390x/s390-pci-vfio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index e28573b593..7dbbc76823 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -136,7 +136,7 @@ static void s390_pci_read_base(S390PCIBusDevice *pbdev,
* to the guest based upon the vfio DMA limit.
*/
vfio_size = pbdev->iommu->max_dma_limit << TARGET_PAGE_BITS;
-if (vfio_size < (cap->end_dma - cap->start_dma + 1)) {
+if (vfio_size > 0 && vfio_size < cap->end_dma - cap->start_dma + 1) {
pbdev->zpci_fn.edma = cap->start_dma + vfio_size - 1;
}
}
--
2.41.0
[PATCH v2 1/2] s390x/pci: bypass vfio DMA counting when using cdev
The current code assumes that there is always a vfio group, but
that's no longer guaranteed with the iommufd backend when using
cdev. In this case, we don't need to track the vfio dma limit
anyway.
Signed-off-by: Matthew Rosato
---
hw/s390x/s390-pci-vfio.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index 59a2e03873..e28573b593 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -66,6 +66,10 @@ S390PCIDMACount *s390_pci_start_dma_count(S390pciState *s,
assert(vpdev);
+if (!vpdev->vbasedev.group) {
+return NULL;
+}
+
id = vpdev->vbasedev.group->container->fd;
if (!s390_pci_update_dma_avail(id, &avail)) {
--
2.41.0
Re: [PATCH 0/8] Add powernv10 I2C devices and tests
On Fri, 2023-11-10 at 11:22 -0600, Glenn Miles wrote: > This series of patches includes support, tests and fixes for > adding PCA9552 and PCA9554 I2C devices to the powernv10 chip. > > The PCA9552 device is used for PCIe slot hotplug power control > and monitoring, while the PCA9554 device is used for presence > detection of IBM CableCard devices. Both devices are required > by the Power Hypervisor Firmware on Power10 platforms. > Sorry folks, I got a ahead of myself and forgot to test this on the master branch. Looks like it's currently failing 'make check' there so I'll be coming out with a v2 once I get it working. Regards, Glenn > Glenn Miles (8): > ppc/pnv: Add pca9552 to powernv10 for PCIe hotplug power control > ppc/pnv: Wire up pca9552 GPIO pins for PCIe hotplug power control > ppc/pnv: PNV I2C engines assigned incorrect XSCOM addresses > ppc/pnv: Fix PNV I2C invalid status after reset > ppc/pnv: Use resettable interface to reset child I2C buses > misc: Add a pca9554 GPIO device model > ppc/pnv: Add a pca9554 I2C device to powernv10 > ppc/pnv: Test pnv i2c master and connected devices > > MAINTAINERS | 2 + > hw/misc/Kconfig | 4 + > hw/misc/meson.build | 1 + > hw/misc/pca9554.c | 328 > hw/ppc/Kconfig | 2 + > hw/ppc/pnv.c| 35 +- > hw/ppc/pnv_i2c.c| 47 ++- > include/hw/misc/pca9554.h | 36 ++ > include/hw/misc/pca9554_regs.h | 19 + > tests/qtest/meson.build | 1 + > tests/qtest/pnv-host-i2c-test.c | 653 > > 11 files changed, 1106 insertions(+), 22 deletions(-) > create mode 100644 hw/misc/pca9554.c > create mode 100644 include/hw/misc/pca9554.h > create mode 100644 include/hw/misc/pca9554_regs.h > create mode 100644 tests/qtest/pnv-host-i2c-test.c >
[PATCH] linux-user/riscv: Add Zicboz block size to hwprobe
Support for probing the Zicboz block size landed in Linux 6.6, which was
released a few weeks ago. This provides the user-configured block size
when Zicboz is enabled.
Signed-off-by: Palmer Dabbelt
---
linux-user/syscall.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 65ac3ac796..7caacf43d6 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8808,6 +8808,8 @@ static int do_getdents64(abi_long dirfd, abi_long arg2,
abi_long count)
#define RISCV_HWPROBE_MISALIGNED_UNSUPPORTED (4 << 0)
#define RISCV_HWPROBE_MISALIGNED_MASK(7 << 0)
+#define RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE 6
+
struct riscv_hwprobe {
abi_llong key;
abi_ullong value;
@@ -8860,6 +8862,10 @@ static void risc_hwprobe_fill_pairs(CPURISCVState *env,
case RISCV_HWPROBE_KEY_CPUPERF_0:
__put_user(RISCV_HWPROBE_MISALIGNED_FAST, &pair->value);
break;
+case RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE:
+value = cfg->ext_zicboz ? cfg->cboz_blocksize : 0;
+__put_user(value, &pair->value);
+break;
default:
__put_user(-1, &pair->key);
break;
--
2.42.1
[PATCH v6] target/ppc: Fix bugs in VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros
The patch below fixes a bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2
macros in target/ppc/fpu_helper.c where a non-NaN floating point value from the
source vector is incorrectly converted to 0, 0x8000, or 0x8000
instead of the expected value if a preceding source floating point value from
the same source vector was a NaN.
The bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros in
target/ppc/fpu_helper.c was introduced with commit c3f24257e3c0.
This patch also adds a new vsx_f2i_nan test in tests/tcg/ppc64 that checks that
the VSX xvcvspsxws, xvcvspuxws, xvcvspsxds, xvcvspuxds, xvcvdpsxws, xvcvdpuxws,
xvcvdpsxds, and xvcvdpuxds instructions correctly convert non-NaN floating point
values to integer values if the source vector contains NaN floating point
values.
Fixes: c3f24257e3c0 ("target/ppc: Clear fpstatus flags on helpers missing it")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1941
Signed-off-by: John Platts
---
target/ppc/fpu_helper.c | 12 +-
tests/tcg/ppc64/Makefile.target | 5 +
tests/tcg/ppc64/vsx_f2i_nan.c | 303
3 files changed, 316 insertions(+), 4 deletions(-)
create mode 100644 tests/tcg/ppc64/vsx_f2i_nan.c
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 03150a0f10..4b3dcad5d1 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2880,20 +2880,22 @@ uint64_t helper_XSCVSPDPN(uint64_t xb)
#define VSX_CVT_FP_TO_INT(op, nels, stp, ttp, sfld, tfld, sfi, rnan) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.tfld = stp##_to_##ttp##_round_to_zero(xb->sfld, &env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.tfld = float_invalid_cvt(env, flags, t.tfld, rnan, 0, GETPC());\
}\
}\
\
*xt = t; \
+env->fp_status.float_exception_flags = all_flags;\
do_float_check_status(env, sfi, GETPC());\
}
@@ -2945,15 +2947,16 @@ VSX_CVT_FP_TO_INT128(XSCVQPSQZ, int128,
0x8000ULL);
#define VSX_CVT_FP_TO_INT2(op, nels, stp, ttp, sfi, rnan)\
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.VsrW(2 * i) = stp##_to_##ttp##_round_to_zero(xb->VsrD(i), \
&env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.VsrW(2 * i) = float_invalid_cvt(env, flags, t.VsrW(2 * i), \
rnan, 0, GETPC()); \
@@ -2962,6 +2965,7 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,
ppc_vsr_t *xb) \
}\
[PATCH v7] target/ppc: Fix bugs in VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros
The patch below fixes a bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2
macros in target/ppc/fpu_helper.c where a non-NaN floating point value from the
source vector is incorrectly converted to 0, 0x8000, or 0x8000
instead of the expected value if a preceding source floating point value from
the same source vector was a NaN.
The bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros in
target/ppc/fpu_helper.c was introduced with commit c3f24257e3c0.
This patch also adds a new vsx_f2i_nan test in tests/tcg/ppc64 that checks that
the VSX xvcvspsxws, xvcvspuxws, xvcvspsxds, xvcvspuxds, xvcvdpsxws, xvcvdpuxws,
xvcvdpsxds, and xvcvdpuxds instructions correctly convert non-NaN floating point
values to integer values if the source vector contains NaN floating point
values.
Fixes: c3f24257e3c0 ("target/ppc: Clear fpstatus flags on helpers missing it")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1941
Signed-off-by: John Platts
---
target/ppc/fpu_helper.c | 12 +-
tests/tcg/ppc64/Makefile.target | 5 +
tests/tcg/ppc64/vsx_f2i_nan.c | 300
3 files changed, 313 insertions(+), 4 deletions(-)
create mode 100644 tests/tcg/ppc64/vsx_f2i_nan.c
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 03150a0f10..4b3dcad5d1 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2880,20 +2880,22 @@ uint64_t helper_XSCVSPDPN(uint64_t xb)
#define VSX_CVT_FP_TO_INT(op, nels, stp, ttp, sfld, tfld, sfi, rnan) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.tfld = stp##_to_##ttp##_round_to_zero(xb->sfld, &env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.tfld = float_invalid_cvt(env, flags, t.tfld, rnan, 0, GETPC());\
}\
}\
\
*xt = t; \
+env->fp_status.float_exception_flags = all_flags;\
do_float_check_status(env, sfi, GETPC());\
}
@@ -2945,15 +2947,16 @@ VSX_CVT_FP_TO_INT128(XSCVQPSQZ, int128,
0x8000ULL);
#define VSX_CVT_FP_TO_INT2(op, nels, stp, ttp, sfi, rnan)\
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.VsrW(2 * i) = stp##_to_##ttp##_round_to_zero(xb->VsrD(i), \
&env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.VsrW(2 * i) = float_invalid_cvt(env, flags, t.VsrW(2 * i), \
rnan, 0, GETPC()); \
@@ -2962,6 +2965,7 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,
ppc_vsr_t *xb) \
}\
[PATCH v8] target/ppc: Fix bugs in VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros
The patch below fixes a bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2
macros in target/ppc/fpu_helper.c where a non-NaN floating point value from the
source vector is incorrectly converted to 0, 0x8000, or 0x8000
instead of the expected value if a preceding source floating point value from
the same source vector was a NaN.
The bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros in
target/ppc/fpu_helper.c was introduced with commit c3f24257e3c0.
This patch also adds a new vsx_f2i_nan test in tests/tcg/ppc64 that checks that
the VSX xvcvspsxws, xvcvspuxws, xvcvspsxds, xvcvspuxds, xvcvdpsxws, xvcvdpuxws,
xvcvdpsxds, and xvcvdpuxds instructions correctly convert non-NaN floating point
values to integer values if the source vector contains NaN floating point
values.
Fixes: c3f24257e3c0 ("target/ppc: Clear fpstatus flags on helpers missing it")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1941
Signed-off-by: John Platts
---
target/ppc/fpu_helper.c | 12 +-
tests/tcg/ppc64/Makefile.target | 5 +
tests/tcg/ppc64/vsx_f2i_nan.c | 300
3 files changed, 313 insertions(+), 4 deletions(-)
create mode 100644 tests/tcg/ppc64/vsx_f2i_nan.c
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 03150a0f10..4b3dcad5d1 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2880,20 +2880,22 @@ uint64_t helper_XSCVSPDPN(uint64_t xb)
#define VSX_CVT_FP_TO_INT(op, nels, stp, ttp, sfld, tfld, sfi, rnan) \
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.tfld = stp##_to_##ttp##_round_to_zero(xb->sfld, &env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.tfld = float_invalid_cvt(env, flags, t.tfld, rnan, 0, GETPC());\
}\
}\
\
*xt = t; \
+env->fp_status.float_exception_flags = all_flags;\
do_float_check_status(env, sfi, GETPC());\
}
@@ -2945,15 +2947,16 @@ VSX_CVT_FP_TO_INT128(XSCVQPSQZ, int128,
0x8000ULL);
#define VSX_CVT_FP_TO_INT2(op, nels, stp, ttp, sfi, rnan)\
void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb) \
{\
+int all_flags = 0; \
ppc_vsr_t t = { }; \
int i, flags;\
\
-helper_reset_fpstatus(env); \
- \
for (i = 0; i < nels; i++) { \
+helper_reset_fpstatus(env); \
t.VsrW(2 * i) = stp##_to_##ttp##_round_to_zero(xb->VsrD(i), \
&env->fp_status); \
flags = env->fp_status.float_exception_flags;\
+all_flags |= flags; \
if (unlikely(flags & float_flag_invalid)) { \
t.VsrW(2 * i) = float_invalid_cvt(env, flags, t.VsrW(2 * i), \
rnan, 0, GETPC()); \
@@ -2962,6 +2965,7 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,
ppc_vsr_t *xb) \
}\
