Re: [PATCH] include/hw/xen: Use more inclusive language in comment
On Thu, Nov 09, 2023 at 06:40:34PM +0100, Thomas Huth wrote: > Let's improve the wording here. > > Signed-off-by: Thomas Huth > --- > include/hw/xen/interface/hvm/params.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé 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: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer
On Tue, Oct 10, 2023 at 02:41:22PM +0300, Dmitry Osipenko wrote:
> On 9/15/23 14:11, Huang Rui wrote:
> > Configure context init feature flag for virglrenderer.
> >
> > Originally-by: Antonio Caggiano
> > Signed-off-by: Huang Rui
> > ---
> >
> > V4 -> V5:
> > - Inverted patch 5 and 6 because we should configure
> > HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> >
> > meson.build | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/meson.build b/meson.build
> > index 98e68ef0b1..ff20d3c249 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or
> > have_system or have_vhost_user_gpu
> > prefix: '#include
> > ',
> > dependencies: virgl))
> >endif
> > + config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> > +
> > cc.has_function('virgl_renderer_context_create_with_flags',
> > + prefix: '#include
> > ',
> > + dependencies: virgl))
> The "cc.has_function" doesn't work properly with PKG_CONFIG_PATH. It ignores
> the the given pkg and uses system includes. Antonio was aware about that
> problem [1].
>
> [1]
> https://gitlab.freedesktop.org/Fahien/qemu/-/commit/ea1c252a707940983ccce71e92a292b49496bfcd
>
> Given that virglrenderer 1.0 has been released couple weeks ago,
> can we make the v1.0 a mandatory requirement for qemu and remove
> all the ifdefs? I doubt that anyone is going to test newer qemu
> using older libviglrenderer, all that ifdef code will be bitrotting.
We cannot do that. If is is only weeks old, no distro will
have virglrenderer 1.0 present. QEMU has a defined set of
platforms that it targets compatibility with:
https://www.qemu.org/docs/master/about/build-platforms.html
For newly added functionality we can set the min version to
something newer than the oldest QEMU target platform.
For existing functionality though, we must not regress wrt
the currently supported set of target platforms. So we can
only bump the min version to that present in the oldest
platform we target.
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 v1 01/23] pc/xen: Xen Q35 support: provide IRQ handling for PCI devices
On Tue, Jun 20, 2023 at 01:24:34PM -0400, Joel Upham wrote:
>
> Signed-off-by: Alexey Gerasimenko
This isn't a valid email address for Alexey - I presume you grabbed
these patches from the xen-devel mail archives, which have mangled
the addresses for anti-spam reasons.
Fortunately there are alternative archives which don't mangle the
patches:
https://lore.kernel.org/xen-devel/6067bc3c91c9ee629a35723dfb474ef168ff4ebf.1520867955.git.x19...@gmail.com/
Signed-off-by: Alexey Gerasimenko
This affects all patches in the series, but I won't repeat my
comment on each one.
> Signed-off-by: Joel Upham
> ---
> hw/i386/pc_piix.c | 3 +-
> hw/i386/xen/xen-hvm.c | 7 +++--
> hw/isa/lpc_ich9.c | 53 ---
> hw/isa/piix3.c| 2 +-
> include/hw/southbridge/ich9.h | 1 +
> include/hw/xen/xen.h | 4 +--
> stubs/xen-hw-stub.c | 4 +--
> 7 files changed, 61 insertions(+), 13 deletions(-)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index d5b0dcd1fe..8c1b20f3bc 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -62,6 +62,7 @@
> #endif
> #include "hw/xen/xen-x86.h"
> #include "hw/xen/xen.h"
> +#include "sysemu/xen.h"
> #include "migration/global_state.h"
> #include "migration/misc.h"
> #include "sysemu/numa.h"
> @@ -233,7 +234,7 @@ static void pc_init1(MachineState *machine,
>x86ms->above_4g_mem_size,
>pci_memory, ram_memory);
> pci_bus_map_irqs(pci_bus,
> - xen_enabled() ? xen_pci_slot_get_pirq
> + xen_enabled() ? xen_cmn_pci_slot_get_pirq
> : pc_pci_slot_get_pirq);
> pcms->bus = pci_bus;
>
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index 56641a550e..540ac46639 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -15,6 +15,7 @@
> #include "hw/pci/pci.h"
> #include "hw/pci/pci_host.h"
> #include "hw/i386/pc.h"
> +#include "hw/southbridge/ich9.h"
> #include "hw/irq.h"
> #include "hw/hw.h"
> #include "hw/i386/apic-msidef.h"
> @@ -136,14 +137,14 @@ typedef struct XenIOState {
> Notifier wakeup;
> } XenIOState;
>
> -/* Xen specific function for piix pci */
> +/* Xen-specific functions for pci dev IRQ handling */
>
> -int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> +int xen_cmn_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> {
> return irq_num + (PCI_SLOT(pci_dev->devfn) << 2);
> }
>
> -void xen_piix3_set_irq(void *opaque, int irq_num, int level)
> +void xen_cmn_set_irq(void *opaque, int irq_num, int level)
> {
> xen_set_pci_intx_level(xen_domid, 0, 0, irq_num >> 2,
> irq_num & 3, level);
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 9c47a2f6c7..733a99d443 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -51,6 +51,9 @@
> #include "hw/core/cpu.h"
> #include "hw/nvram/fw_cfg.h"
> #include "qemu/cutils.h"
> +#include "hw/xen/xen.h"
> +#include "sysemu/xen.h"
> +#include "hw/southbridge/piix.h"
> #include "hw/acpi/acpi_aml_interface.h"
> #include "trace.h"
>
> @@ -535,11 +538,49 @@ static int ich9_lpc_post_load(void *opaque, int
> version_id)
> return 0;
> }
>
> +static void ich9_lpc_config_write_xen(PCIDevice *d,
> + uint32_t addr, uint32_t val, int len)
> +{
> +static bool pirqe_f_warned = false;
> +if (ranges_overlap(addr, len, ICH9_LPC_PIRQA_ROUT, 4)) {
> +/* handle PIRQA..PIRQD routing */
> +/* Scan for updates to PCI link routes (0x60-0x63). */
> +int i;
> +for (i = 0; i < len; i++) {
> +uint8_t v = (val >> (8 * i)) & 0xff;
> +if (v & 0x80) {
> +v = 0;
> +}
> +v &= 0xf;
> +if (((addr + i) >= PIIX_PIRQCA) && ((addr + i) <= PIIX_PIRQCD)) {
> +xen_set_pci_link_route(addr + i - PIIX_PIRQCA, v);
> +}
> +}
> +} else if (ranges_overlap(addr, len, ICH9_LPC_PIRQE_ROUT, 4)) {
> +while (len--) {
> +if (range_covers_byte(ICH9_LPC_PIRQE_ROUT, 4, addr) &&
> +(val & 0x80) == 0) {
> +/* print warning only once */
> +if (!pirqe_f_warned) {
> +pirqe_f_warned = true;
> +fprintf(stderr, "WARNING: guest domain attempted to use
> PIRQ%c "
> +"routing which is not supported for Xen/Q35
> currently\n",
> +(char)(addr - ICH9_LPC_PIRQE_ROUT + 'E'));
> +break;
> +}
> +}
> +addr++, val >>= 8;
> +}
> +}
> +}
> +
> static void ich9_lpc_config_write(PCIDevice *d,
>uint32_t addr, uint32_t val, int len)
> {
> ICH9LPCState *lpc =
Re: [PATCH-for-8.1 4/5] bulk: Do not declare function prototypes using extern keyword
On Mon, Mar 20, 2023 at 02:42:18PM +0100, Philippe Mathieu-Daudé wrote:
> By default, C function prototypes declared in headers are visible,
> so there is no need to declare them as 'extern' functions.
> Remove this redundancy in a single bulk commit; do not modify:
>
> - meson.build (used to check function availability at runtime)
> - pc-bios
> - libdecnumber
> - *.c
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> block/dmg.h| 8 +++
> bsd-user/bsd-file.h| 6 ++---
> crypto/hmacpriv.h | 13 +--
> hw/xen/xen_pt.h| 8 +++
> include/crypto/secret_common.h | 14 +---
> include/exec/page-vary.h | 4 ++--
> include/hw/misc/aspeed_scu.h | 2 +-
> include/hw/nvram/npcm7xx_otp.h | 4 ++--
> include/hw/qdev-core.h | 4 ++--
> include/qemu/crc-ccitt.h | 4 ++--
> include/qemu/osdep.h | 2 +-
> include/qemu/rcu.h | 14 ++--
> include/qemu/sys_membarrier.h | 4 ++--
> include/qemu/uri.h | 6 ++---
> include/sysemu/accel-blocker.h | 14 ++--
> include/sysemu/os-win32.h | 4 ++--
> include/user/safe-syscall.h| 4 ++--
> target/i386/sev.h | 6 ++---
> target/mips/cpu.h | 4 ++--
> tcg/tcg-internal.h | 4 ++--
> tests/tcg/minilib/minilib.h| 2 +-
> include/exec/memory_ldst.h.inc | 42 +-
> roms/seabios | 2 +-
Accidental submodule commit.,
> 23 files changed, 84 insertions(+), 91 deletions(-)
>
> diff --git a/block/dmg.h b/block/dmg.h
> index e488601b62..ed209b5dec 100644
> --- a/block/dmg.h
> +++ b/block/dmg.h
> @@ -51,10 +51,10 @@ typedef struct BDRVDMGState {
> z_stream zstream;
> } BDRVDMGState;
>
> -extern int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
> - char *next_out, unsigned int avail_out);
> +int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
> + char *next_out, unsigned int avail_out);
>
> -extern int (*dmg_uncompress_lzfse)(char *next_in, unsigned int avail_in,
> - char *next_out, unsigned int avail_out);
> +int (*dmg_uncompress_lzfse)(char *next_in, unsigned int avail_in,
> +char *next_out, unsigned int avail_out);
These are variable declarations, so with this change you'll get multiple
copies of the variable if this header is included from multiple source
files. IOW, the 'extern' usage is correct.
> diff --git a/roms/seabios b/roms/seabios
> index ea1b7a0733..3208b098f5 16
> --- a/roms/seabios
> +++ b/roms/seabios
> @@ -1 +1 @@
> -Subproject commit ea1b7a0733906b8425d948ae94fba63c32b1d425
> +Subproject commit 3208b098f51a9ef96d0dfa71d5ec3a3eaec88f0a
Nope !
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 v4 2/5] docs/about/deprecated: Deprecate the qemu-system-i386 binary
On Mon, Mar 06, 2023 at 02:25:46PM +, Daniel P. Berrangé wrote: > On Mon, Mar 06, 2023 at 03:18:23PM +0100, Thomas Huth wrote: > > On 06/03/2023 15.06, Daniel P. Berrangé wrote: > > > On Mon, Mar 06, 2023 at 02:48:16PM +0100, Thomas Huth wrote: > > > > On 06/03/2023 10.27, Daniel P. Berrangé wrote: > > > > > On Mon, Mar 06, 2023 at 09:46:55AM +0100, Thomas Huth wrote: > > > > > > [...] If a 32-bit CPU guest > > > > > > +environment should be enforced, you can switch off the "long mode" > > > > > > CPU > > > > > > +flag, e.g. with ``-cpu max,lm=off``. > > > > > > > > > > I had the idea to check this today and this is not quite sufficient, > > > > [...] > > > > > A further difference is that qemy-system-i686 does not appear to > > > > > enable > > > > > the 'syscall' flag, but I've not figured out where that difference is > > > > > coming from in the code. > > > > > > > > I think I just spotted this by accident in target/i386/cpu.c > > > > around line 637: > > > > > > > > #ifdef TARGET_X86_64 > > > > #define TCG_EXT2_X86_64_FEATURES (CPUID_EXT2_SYSCALL | CPUID_EXT2_LM) > > > > #else > > > > #define TCG_EXT2_X86_64_FEATURES 0 > > > > #endif > > > > > > Hmm, so right now the difference between qemu-system-i386 and > > > qemu-system-x86_64 is based on compile time conditionals. So we > > > have the burden of building everything twice and also a burden > > > of testing everything twice. > > > > > > If we eliminate qemu-system-i386 we get rid of our own burden, > > > but users/mgmt apps need to adapt to force qemu-system-x86_64 > > > to present a 32-bit system. > > > > > > What about if we had qemu-system-i386 be a hardlink to > > > qemu-system-x86_64, and then changed behaviour based off the > > > executed binary name ? > > > > We could also simply provide a shell script that runs: > > > > qemu-system-x86_64 -cpu qemu32 $* > > > > ... that'd sounds like the simplest solution to me. > > That woudn't do the right thing if the user ran 'qemu-system-i386 -cpu max' > because their '-cpu max' would override the -cpu arg in the shell script > that forced 32-bit mode. It would also fail to work with SELinux, because policy restrictions doesn't allow for an intermediate wrapper script to exec binaries. 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 v4 2/5] docs/about/deprecated: Deprecate the qemu-system-i386 binary
On Mon, Mar 06, 2023 at 03:18:23PM +0100, Thomas Huth wrote: > On 06/03/2023 15.06, Daniel P. Berrangé wrote: > > On Mon, Mar 06, 2023 at 02:48:16PM +0100, Thomas Huth wrote: > > > On 06/03/2023 10.27, Daniel P. Berrangé wrote: > > > > On Mon, Mar 06, 2023 at 09:46:55AM +0100, Thomas Huth wrote: > > > > > [...] If a 32-bit CPU guest > > > > > +environment should be enforced, you can switch off the "long mode" > > > > > CPU > > > > > +flag, e.g. with ``-cpu max,lm=off``. > > > > > > > > I had the idea to check this today and this is not quite sufficient, > > > [...] > > > > A further difference is that qemy-system-i686 does not appear to enable > > > > the 'syscall' flag, but I've not figured out where that difference is > > > > coming from in the code. > > > > > > I think I just spotted this by accident in target/i386/cpu.c > > > around line 637: > > > > > > #ifdef TARGET_X86_64 > > > #define TCG_EXT2_X86_64_FEATURES (CPUID_EXT2_SYSCALL | CPUID_EXT2_LM) > > > #else > > > #define TCG_EXT2_X86_64_FEATURES 0 > > > #endif > > > > Hmm, so right now the difference between qemu-system-i386 and > > qemu-system-x86_64 is based on compile time conditionals. So we > > have the burden of building everything twice and also a burden > > of testing everything twice. > > > > If we eliminate qemu-system-i386 we get rid of our own burden, > > but users/mgmt apps need to adapt to force qemu-system-x86_64 > > to present a 32-bit system. > > > > What about if we had qemu-system-i386 be a hardlink to > > qemu-system-x86_64, and then changed behaviour based off the > > executed binary name ? > > We could also simply provide a shell script that runs: > > qemu-system-x86_64 -cpu qemu32 $* > > ... that'd sounds like the simplest solution to me. That woudn't do the right thing if the user ran 'qemu-system-i386 -cpu max' because their '-cpu max' would override the -cpu arg in the shell script that forced 32-bit mode. 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 v4 2/5] docs/about/deprecated: Deprecate the qemu-system-i386 binary
On Mon, Mar 06, 2023 at 02:48:16PM +0100, Thomas Huth wrote:
> On 06/03/2023 10.27, Daniel P. Berrangé wrote:
> > On Mon, Mar 06, 2023 at 09:46:55AM +0100, Thomas Huth wrote:
> > > [...] If a 32-bit CPU guest
> > > +environment should be enforced, you can switch off the "long mode" CPU
> > > +flag, e.g. with ``-cpu max,lm=off``.
> >
> > I had the idea to check this today and this is not quite sufficient,
> [...]
> > A further difference is that qemy-system-i686 does not appear to enable
> > the 'syscall' flag, but I've not figured out where that difference is
> > coming from in the code.
>
> I think I just spotted this by accident in target/i386/cpu.c
> around line 637:
>
> #ifdef TARGET_X86_64
> #define TCG_EXT2_X86_64_FEATURES (CPUID_EXT2_SYSCALL | CPUID_EXT2_LM)
> #else
> #define TCG_EXT2_X86_64_FEATURES 0
> #endif
Hmm, so right now the difference between qemu-system-i386 and
qemu-system-x86_64 is based on compile time conditionals. So we
have the burden of building everything twice and also a burden
of testing everything twice.
If we eliminate qemu-system-i386 we get rid of our own burden,
but users/mgmt apps need to adapt to force qemu-system-x86_64
to present a 32-bit system.
What about if we had qemu-system-i386 be a hardlink to
qemu-system-x86_64, and then changed behaviour based off the
executed binary name ?
ie if running qemu-system-i386, we could present a 32-bit CPU by
default. We eliminate all of our double compilation burden still.
We still have extra testing burden, but it is in a fairly narrow
area, so does not imply x2 the testing burden just $small-percentage
extra testing ? That would means apps/users would not need to change
at all, but we still get most of the win we're after on the
QEMU side
Essentially #ifdef TARGET_X86_64 would be change 'if (is_64bit) {...}'
in a handful of places, with 'bool is_64bit' initialized in main() from
argv[0] ?
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 v4 2/5] docs/about/deprecated: Deprecate the qemu-system-i386 binary
On Mon, Mar 06, 2023 at 10:54:15AM +0100, Thomas Huth wrote: > On 06/03/2023 10.27, Daniel P. Berrangé wrote: > > On Mon, Mar 06, 2023 at 09:46:55AM +0100, Thomas Huth wrote: > > > Aside from not supporting KVM on 32-bit hosts, the qemu-system-x86_64 > > > binary is a proper superset of the qemu-system-i386 binary. With the > > > 32-bit host support being deprecated, it is now also possible to > > > deprecate the qemu-system-i386 binary. > > > > > > With regards to 32-bit KVM support in the x86 Linux kernel, > > > the developers confirmed that they do not need a recent > > > qemu-system-i386 binary here: > > > > > > https://lore.kernel.org/kvm/y%2ffkts5ajfy0h...@google.com/ > > > > > > Reviewed-by: Daniel P. Berrangé > > > Reviewed-by: Wilfred Mallawa > > > Signed-off-by: Thomas Huth > > > --- > > > docs/about/deprecated.rst | 14 ++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > > > index 1ca9dc33d6..c4fcc6b33c 100644 > > > --- a/docs/about/deprecated.rst > > > +++ b/docs/about/deprecated.rst > > > @@ -34,6 +34,20 @@ deprecating the build option and no longer defend it > > > in CI. The > > > ``--enable-gcov`` build option remains for analysis test case > > > coverage. > > > +``qemu-system-i386`` binary (since 8.0) > > > +''''''''''''''''''''''''''''''''''''''' > > > + > > > +The ``qemu-system-i386`` binary was mainly useful for running with KVM > > > +on 32-bit x86 hosts, but most Linux distributions already removed their > > > +support for 32-bit x86 kernels, so hardly anybody still needs this. The > > > +``qemu-system-x86_64`` binary is a proper superset and can be used to > > > +run 32-bit guests by selecting a 32-bit CPU model, including KVM support > > > +on x86_64 hosts. Thus users are recommended to reconfigure their systems > > > +to use the ``qemu-system-x86_64`` binary instead. If a 32-bit CPU guest > > > +environment should be enforced, you can switch off the "long mode" CPU > > > +flag, e.g. with ``-cpu max,lm=off``. > > > > I had the idea to check this today and this is not quite sufficient, > > because we have code that changes the family/model/stepping for > > 'max' which is target dependent: > > > > #ifdef TARGET_X86_64 > > object_property_set_int(OBJECT(cpu), "family", 15, &error_abort); > > object_property_set_int(OBJECT(cpu), "model", 107, &error_abort); > > object_property_set_int(OBJECT(cpu), "stepping", 1, &error_abort); > > #else > > object_property_set_int(OBJECT(cpu), "family", 6, &error_abort); > > object_property_set_int(OBJECT(cpu), "model", 6, &error_abort); > > object_property_set_int(OBJECT(cpu), "stepping", 3, &error_abort); > > #endif > > > > The former is a 64-bit AMD model and the latter is a 32-bit model. > > > > Seems LLVM was sensitive to this distinction to some extent: > > > > https://gitlab.com/qemu-project/qemu/-/issues/191 > > > > A further difference is that qemy-system-i686 does not appear to enable > > the 'syscall' flag, but I've not figured out where that difference is > > coming from in the code. > > Ugh, ok. I gave it a quick try with a patch like this: > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -4344,15 +4344,15 @@ static void max_x86_cpu_initfn(Object *obj) > */ > object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD, > &error_abort); > -#ifdef TARGET_X86_64 > -object_property_set_int(OBJECT(cpu), "family", 15, &error_abort); > -object_property_set_int(OBJECT(cpu), "model", 107, &error_abort); > -object_property_set_int(OBJECT(cpu), "stepping", 1, &error_abort); > -#else > -object_property_set_int(OBJECT(cpu), "family", 6, &error_abort); > -object_property_set_int(OBJECT(cpu), "model", 6, &error_abort); > -object_property_set_int(OBJECT(cpu), "stepping", 3, &error_abort); > -#endif > +if (object_property_get_bool(o
Re: [PATCH v4 2/5] docs/about/deprecated: Deprecate the qemu-system-i386 binary
On Mon, Mar 06, 2023 at 09:46:55AM +0100, Thomas Huth wrote: > Aside from not supporting KVM on 32-bit hosts, the qemu-system-x86_64 > binary is a proper superset of the qemu-system-i386 binary. With the > 32-bit host support being deprecated, it is now also possible to > deprecate the qemu-system-i386 binary. > > With regards to 32-bit KVM support in the x86 Linux kernel, > the developers confirmed that they do not need a recent > qemu-system-i386 binary here: > > https://lore.kernel.org/kvm/y%2ffkts5ajfy0h...@google.com/ > > Reviewed-by: Daniel P. Berrangé > Reviewed-by: Wilfred Mallawa > Signed-off-by: Thomas Huth > --- > docs/about/deprecated.rst | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 1ca9dc33d6..c4fcc6b33c 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -34,6 +34,20 @@ deprecating the build option and no longer defend it in > CI. The > ``--enable-gcov`` build option remains for analysis test case > coverage. > > +``qemu-system-i386`` binary (since 8.0) > +''''''''''''''''''''''''''''''''''''''' > + > +The ``qemu-system-i386`` binary was mainly useful for running with KVM > +on 32-bit x86 hosts, but most Linux distributions already removed their > +support for 32-bit x86 kernels, so hardly anybody still needs this. The > +``qemu-system-x86_64`` binary is a proper superset and can be used to > +run 32-bit guests by selecting a 32-bit CPU model, including KVM support > +on x86_64 hosts. Thus users are recommended to reconfigure their systems > +to use the ``qemu-system-x86_64`` binary instead. If a 32-bit CPU guest > +environment should be enforced, you can switch off the "long mode" CPU > +flag, e.g. with ``-cpu max,lm=off``. I had the idea to check this today and this is not quite sufficient, because we have code that changes the family/model/stepping for 'max' which is target dependent: #ifdef TARGET_X86_64 object_property_set_int(OBJECT(cpu), "family", 15, &error_abort); object_property_set_int(OBJECT(cpu), "model", 107, &error_abort); object_property_set_int(OBJECT(cpu), "stepping", 1, &error_abort); #else object_property_set_int(OBJECT(cpu), "family", 6, &error_abort); object_property_set_int(OBJECT(cpu), "model", 6, &error_abort); object_property_set_int(OBJECT(cpu), "stepping", 3, &error_abort); #endif The former is a 64-bit AMD model and the latter is a 32-bit model. Seems LLVM was sensitive to this distinction to some extent: https://gitlab.com/qemu-project/qemu/-/issues/191 A further difference is that qemy-system-i686 does not appear to enable the 'syscall' flag, but I've not figured out where that difference is coming from in the code. 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 v2 4/6] docs/about/deprecated: Deprecate the qemu-system-arm binary
On Fri, Mar 03, 2023 at 12:31:42PM +0100, Thomas Huth wrote: > On 03/03/2023 12.16, Peter Maydell wrote: > > On Thu, 2 Mar 2023 at 16:31, Thomas Huth wrote: > > > > > > qemu-system-aarch64 is a proper superset of qemu-system-arm, > > > and the latter was mainly still required for 32-bit KVM support. > > > But this 32-bit KVM arm support has been dropped in the Linux > > > kernel a couple of years ago already, so we don't really need > > > qemu-system-arm anymore, thus deprecated it now. > > > > > > Signed-off-by: Thomas Huth > > > --- > > > docs/about/deprecated.rst | 10 ++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > > > index a30aa8dfdf..21ce70b5c9 100644 > > > --- a/docs/about/deprecated.rst > > > +++ b/docs/about/deprecated.rst > > > @@ -45,6 +45,16 @@ run 32-bit guests by selecting a 32-bit CPU model, > > > including KVM support > > > on x86_64 hosts. Thus users are recommended to reconfigure their systems > > > to use the ``qemu-system-x86_64`` binary instead. > > > > > > +``qemu-system-arm`` binary (since 8.0) > > > +'' > > > + > > > +``qemu-system-aarch64`` is a proper superset of ``qemu-system-arm``. > > > > I think this is not quite true -- at the moment if you want > > "every feature we implement, 32-bit" the only way to get > > that is 'qemu-system-arm -cpu max'. The '-cpu max' on > > qemu-system-aarch64 is 64-bit, and we don't implement for TCG > > the "-cpu max,aarch64=off" syntax that we do for KVM that would > > let the user say "no 64-bit support". > > Ok ... so what does that mean now? ... can we continue with this patch, e.g. > after rephrasing the text a little bit, or do we need to implement "-cpu > max,aarch64=off" for TCG first? I think we need to have a way to request the max 32-bit CPU before we deprecate, because deprecation has to tell people what they should use instead. For qemu-system-i686 -cpu max, I guess we have lm=off to hide the 64-bit support, so that's OK from QEMU POV, but will need libvirt enhancement as I don't think we've taken that into account. 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 v2 6/6] gitlab-ci.d/crossbuilds: Drop the 32-bit arm system emulation jobs
On Thu, Mar 02, 2023 at 05:31:06PM +0100, Thomas Huth wrote: > Hardly anybody still uses 32-bit arm environments for running QEMU, > so let's stop wasting our scarce CI minutes with these jobs. > > Signed-off-by: Thomas Huth > --- > .gitlab-ci.d/crossbuilds.yml | 14 -- > 1 file changed, 14 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 5/6] docs/about/deprecated: Deprecate 32-bit arm hosts
On Thu, Mar 02, 2023 at 05:31:05PM +0100, Thomas Huth wrote: > For running QEMU in system emulation mode, the user needs a rather > strong host system, i.e. not only an embedded low-frequency controller. > All recent beefy arm host machines should support 64-bit now, it's > unlikely that anybody is still seriously using QEMU on a 32-bit arm > CPU, so we deprecate the 32-bit arm hosts here to finally save use > some time and precious CI minutes. > > Signed-off-by: Thomas Huth > --- > docs/about/deprecated.rst | 9 + > 1 file changed, 9 insertions(+) Reviewed-by: Daniel P. Berrangé > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 21ce70b5c9..c7113a7510 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -229,6 +229,15 @@ discontinue it. Since all recent x86 hardware from the > past >10 years > is capable of the 64-bit x86 extensions, a corresponding 64-bit OS > should be used instead. > > +System emulation on 32-bit arm hosts (since 8.0) > +'''''''''''''''''''''''''''''''''''''''''''''''' > + > +Since QEMU needs a strong host machine for running full system emulation, and > +all recent powerful arm hosts support 64-bit, the QEMU project deprecates the > +support for running any system emulation on 32-bit arm hosts in general. Use > +64-bit arm hosts for system emulation instead. (Note: "user" mode emulation > +continuous to be supported on 32-bit arm hosts, too) s/continuous/continues/ s/,too/, as well as command line tools like qemu-img, qemu-nbd, etc/ > + > > QEMU API (QAPI) events > -- > -- > 2.31.1 > 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 v2 4/6] docs/about/deprecated: Deprecate the qemu-system-arm binary
On Thu, Mar 02, 2023 at 05:31:04PM +0100, Thomas Huth wrote: > qemu-system-aarch64 is a proper superset of qemu-system-arm, > and the latter was mainly still required for 32-bit KVM support. > But this 32-bit KVM arm support has been dropped in the Linux > kernel a couple of years ago already, so we don't really need > qemu-system-arm anymore, thus deprecated it now. > > Signed-off-by: Thomas Huth > --- > docs/about/deprecated.rst | 10 ++ > 1 file changed, 10 insertions(+) Reviewed-by: Daniel P. Berrangé 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 v2 3/6] gitlab-ci.d/crossbuilds: Drop the i386 jobs
On Thu, Mar 02, 2023 at 05:31:03PM +0100, Thomas Huth wrote: > Hardly anybody still uses 32-bit x86 environments for running QEMU, > so let's stop wasting our scarce CI minutes with these jobs. > > Signed-off-by: Thomas Huth > --- > .gitlab-ci.d/crossbuilds.yml | 16 > 1 file changed, 16 deletions(-) Reviewed-by: Daniel P. Berrangé There's still the mingw 32-bit x86 build, but probably wolrth keeping that until we actually stop 32-bit from a technical POV, because Stefan still publishes the 32-bit windows installers currently Similarly the dockerfile can stay in case someone wants to reproduce a flaw locally Reviewed-by: Daniel P. Berrangé > diff --git a/.gitlab-ci.d/crossbuilds.yml b/.gitlab-ci.d/crossbuilds.yml > index 101416080c..3ce51adf77 100644 > --- a/.gitlab-ci.d/crossbuilds.yml > +++ b/.gitlab-ci.d/crossbuilds.yml > @@ -43,22 +43,6 @@ cross-arm64-user: >variables: > IMAGE: debian-arm64-cross > > -cross-i386-system: > - extends: .cross_system_build_job > - needs: > -job: i386-fedora-cross-container > - variables: > -IMAGE: fedora-i386-cross > -MAKE_CHECK_ARGS: check-qtest > - > -cross-i386-user: > - extends: .cross_user_build_job > - needs: > -job: i386-fedora-cross-container > - variables: > -IMAGE: fedora-i386-cross > -MAKE_CHECK_ARGS: check > - > cross-i386-tci: >extends: .cross_accel_build_job >timeout: 60m > -- > 2.31.1 > 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 v2 2/6] docs/about/deprecated: Deprecate 32-bit x86 hosts
On Thu, Mar 02, 2023 at 05:31:02PM +0100, Thomas Huth wrote: > Hardly anybody still uses 32-bit x86 hosts today, so we should start > deprecating them to stop wasting our time and CI minutes here. > For example, there are also still some unresolved problems with these: > When emulating 64-bit binaries in user mode, TCG does not honor atomicity > for 64-bit accesses, which is "perhaps worse than not working at all" > (quoting Richard). Let's simply make it clear that people should use > 64-bit x86 hosts nowadays and we do not intend to fix/maintain the old > 32-bit stuff. > > Signed-off-by: Thomas Huth > --- > docs/about/deprecated.rst | 12 > 1 file changed, 12 insertions(+) Reviewed-by: Daniel P. Berrangé > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 11700adac9..a30aa8dfdf 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -208,6 +208,18 @@ CI coverage support may bitrot away before the > deprecation process > completes. The little endian variants of MIPS (both 32 and 64 bit) are > still a supported host architecture. > > +32-bit x86 hosts (since 8.0) > +'''''''''''''''''''''''''''' > + > +Support for 32-bit x86 host deployments is increasingly uncommon in > +mainstream OS distributions given the widespread availability of 64-bit > +x86 hardware. The QEMU project no longer considers 32-bit x86 support > +to be an effective use of its limited resources, and thus intends to > +discontinue it. Since all recent x86 hardware from the past >10 years > +is capable of the 64-bit x86 extensions, a corresponding 64-bit OS > +should be used instead. > + > + > QEMU API (QAPI) events > -- > > -- > 2.31.1 > 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 v2 1/6] docs/about/deprecated: Deprecate the qemu-system-i386 binary
On Thu, Mar 02, 2023 at 05:31:01PM +0100, Thomas Huth wrote: > Hardly anybody really requires the i386 binary anymore, since the > qemu-system-x86_64 binary is a proper superset. So let's deprecate > the 32-bit variant now, so that we can finally stop wasting our time > and CI minutes with this. The first sentence isn't quite true wrt to KVM. Change slightly to: Aside from not supporting KVM on 32-bit hosts, the qemu-system-x86_64 binary is a proper superset of the qemu-system-i386 binary. With the 32-bit host support being deprecated, it is now also possible to deprecate the qemu-system-i386 binary. > With regards to 32-bit KVM support in the x86 Linux kernel, > the developers confirmed that they do not need a recent > qemu-system-i386 binary here: > > https://lore.kernel.org/kvm/y%2ffkts5ajfy0h...@google.com/ > > Signed-off-by: Thomas Huth > --- > docs/about/deprecated.rst | 12 > 1 file changed, 12 insertions(+) Reviewed-by: Daniel P. Berrangé 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 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386
On Tue, Feb 28, 2023 at 06:24:02AM -0500, Michael S. Tsirkin wrote: > On Tue, Feb 28, 2023 at 12:12:22PM +0100, Thomas Huth wrote: > > On 28/02/2023 11.51, Michael S. Tsirkin wrote: > > > On Tue, Feb 28, 2023 at 11:39:39AM +0100, Markus Armbruster wrote: > > > > The question to answer: Is 32 bit x86 worth its upkeep? Two > > > > sub-questions: 1. Is it worth the human attention? 2. Is it worth > > > > (scarce!) CI minutes? > > > > > > 3. Is it worth arguing about? > > > > You are aware of the problems we're currently struggeling with, aren't you? > > Darn, we're having *SEVERE* problems with the CI, the QEMU project ran out > > of CI minutes for the second time this year, and you ask whether it's worth > > arguing about??? You're not serious with this question, are you? > > > > Thomas > > Yah just couldn't resist. How many minutes do we use per month btw? 100,000 wall clock minutes lasted about 2+1/2 weeks this month, and similar in Jan too. Last year we were managing to get through the whole month on 100,000, but we got backlogged with merges due to the xmas / new year shutdown, and catching up is exhausting our allowance too quickly, as well as natural growth in amount of stuff we're testing per job. 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 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386
On Tue, Feb 28, 2023 at 10:14:52AM +0100, Thomas Huth wrote: > On 28/02/2023 10.03, Michael S. Tsirkin wrote: > > On Tue, Feb 28, 2023 at 08:59:52AM +0000, Daniel P. Berrangé wrote: > > > On Tue, Feb 28, 2023 at 03:19:20AM -0500, Michael S. Tsirkin wrote: > > > > On Tue, Feb 28, 2023 at 08:49:09AM +0100, Thomas Huth wrote: > > > > > On 27/02/2023 21.12, Michael S. Tsirkin wrote: > > > > > > On Mon, Feb 27, 2023 at 11:50:07AM +, Daniel P. Berrangé wrote: > > > > > > > I feel like we should have separate deprecation entries for the > > > > > > > i686 host support, and for qemu-system-i386 emulator binary, as > > > > > > > although they're related they are independant features with > > > > > > > differing impact. eg removing qemu-system-i386 affects all > > > > > > > host architectures, not merely 32-bit x86 host, so I think we > > > > > > > can explain the impact more clearly if we separate them. > > > > > > > > > > > > Removing qemu-system-i386 seems ok to me - I think > > > > > > qemu-system-x86_64 is > > > > > > a superset. > > > > > > > > > > > > Removing support for building on 32 bit systems seems like a pity - > > > > > > it's > > > > > > one of a small number of ways to run 64 bit binaries on 32 bit > > > > > > systems, > > > > > > and the maintainance overhead is quite small. > > > > > > > > > > Note: We're talking about 32-bit *x86* hosts here. Do you really > > > > > think that > > > > > someone is still using QEMU usermode emulation > > > > > to run 64-bit binaries on a 32-bit x86 host?? ... If so, I'd be very > > > > > surprised! > > > > > > > > I don't know - why x86 specifically? One can build a 32 bit binary on > > > > any host. > > > > I think 32 bit x86 environments are just more common in the cloud. > > > > > > Can you point to anything that backs up that assertion. Clouds I've > > > seen always give you a 64-bit environment, and many OS no longer > > > even ship 32-bit installable media. > > > > Sorry about being unclear. I meant that it seems easier to run CI in the > > cloud in a 32 bit x64 environment than get a 32 bit ARM environment. > > It's still doable ... but for how much longer? We're currently depending on > Fedora, but they also slowly drop more and more support for this > environment, see e.g.: FWIW, we should cull our fedora-i386-cross.docker dockerfile and replace it with a debian i686 dockerfile generated by lcitool. There's no compelling reason why i686 should be different from all our other cross builds which are based on Debian. The Debian lcitool generated container would have access to a wider range of deps than our hand written Fedora one. > https://www.theregister.com/2022/03/10/fedora_inches_closer_to_dropping/ 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 0/2] Deprecate support for 32-bit x86 and arm hosts
On Tue, Feb 28, 2023 at 08:39:49AM +0100, Thomas Huth wrote: > On 27/02/2023 19.38, Daniel P. Berrangé wrote: > > On Mon, Feb 27, 2023 at 12:10:48PM +0100, Thomas Huth wrote: > > > We're struggling quite badly with our CI minutes on the shared > > > gitlab runners, so we urgently need to think of ways to cut down > > > our supported build and target environments. qemu-system-i386 and > > > qemu-system-arm are not really required anymore, since nobody uses > > > KVM on the corresponding systems for production anymore, and the > > > -x86_64 and -arch64 variants are a proper superset of those binaries. > > > So it's time to deprecate them and the corresponding 32-bit host > > > environments now. > > > > > > This is a follow-up patch series from the previous discussion here: > > > > > > > > > https://lore.kernel.org/qemu-devel/20230130114428.1297295-1-th...@redhat.com/ > > > > > > where people still mentioned that there is still interest in certain > > > support for 32-bit host hardware. But as far as I could see, there is > > > no real need for 32-bit host support for system emulation on x86 and > > > arm anymore, so it should be fine if we drop these host environments > > > now (these are also the two architectures that contribute the most to > > > the long test times in our CI, so we would benefit a lot by dropping > > > those). > > > > Your description here is a little ambiguous about what's being > > proposed. When you say dropping 32-bit host support do you mean > > just for the system emulator binaries, or for QEMU entirely ? > > Just for system emulation. Some people said that user emulation still might > be useful for some 32-bit environments. > > > And when the deprecation period is passed, are you proposing > > to actively prevent 32-bit builds, or merely stopping CI testing > > and leave 32-bit builds still working if people want them ? > > CI is the main pain point, so that's the most important thing. So whether we > throw a warning or a hard error while configuring the build, I don't care > too much. If we're merely wanting to drop CI support, we can do that any time and deprecation is not required/expected. We should only be using deprecation where we're explicitly intending that the code will cease to work. 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 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386
On Tue, Feb 28, 2023 at 03:19:20AM -0500, Michael S. Tsirkin wrote: > On Tue, Feb 28, 2023 at 08:49:09AM +0100, Thomas Huth wrote: > > On 27/02/2023 21.12, Michael S. Tsirkin wrote: > > > On Mon, Feb 27, 2023 at 11:50:07AM +, Daniel P. Berrangé wrote: > > > > I feel like we should have separate deprecation entries for the > > > > i686 host support, and for qemu-system-i386 emulator binary, as > > > > although they're related they are independant features with > > > > differing impact. eg removing qemu-system-i386 affects all > > > > host architectures, not merely 32-bit x86 host, so I think we > > > > can explain the impact more clearly if we separate them. > > > > > > Removing qemu-system-i386 seems ok to me - I think qemu-system-x86_64 is > > > a superset. > > > > > > Removing support for building on 32 bit systems seems like a pity - it's > > > one of a small number of ways to run 64 bit binaries on 32 bit systems, > > > and the maintainance overhead is quite small. > > > > Note: We're talking about 32-bit *x86* hosts here. Do you really think that > > someone is still using QEMU usermode emulation > > to run 64-bit binaries on a 32-bit x86 host?? ... If so, I'd be very > > surprised! > > I don't know - why x86 specifically? One can build a 32 bit binary on any > host. > I think 32 bit x86 environments are just more common in the cloud. Can you point to anything that backs up that assertion. Clouds I've seen always give you a 64-bit environment, and many OS no longer even ship 32-bit installable media. I would be surprised if 32-bit is above very very low single digits usage compared to x86_64. > > > In fact, keeping this support around forces correct use of > > > posix APIs such as e.g. PRIx64 which makes the code base > > > more future-proof. > > > > If you're concerned about PRIx64 and friends: We still continue to do > > compile testing with 32-bit MIPS cross-compilers and Windows 32-bit > > cross-compilers for now. The only thing we'd lose is the 32-bit "make check" > > run in the CI. > > > > Thomas > > Yes - fundamentally 32 bit does not seem that different from e.g. > windows builds - we presumably support these but AFAIK CI does not > test these. We do compile test windows in CI via mingw, and we also do build and unit tests via msys. Even Windows has dropped 32-bit support though, and so the only reason we keep 32-bit Windows around is because of Windows 10. Once a Windows 12 comes along, we'll not need to support 32-bit Windows either. 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 0/2] Deprecate support for 32-bit x86 and arm hosts
On Mon, Feb 27, 2023 at 12:10:48PM +0100, Thomas Huth wrote: > We're struggling quite badly with our CI minutes on the shared > gitlab runners, so we urgently need to think of ways to cut down > our supported build and target environments. qemu-system-i386 and > qemu-system-arm are not really required anymore, since nobody uses > KVM on the corresponding systems for production anymore, and the > -x86_64 and -arch64 variants are a proper superset of those binaries. > So it's time to deprecate them and the corresponding 32-bit host > environments now. > > This is a follow-up patch series from the previous discussion here: > > https://lore.kernel.org/qemu-devel/20230130114428.1297295-1-th...@redhat.com/ > > where people still mentioned that there is still interest in certain > support for 32-bit host hardware. But as far as I could see, there is > no real need for 32-bit host support for system emulation on x86 and > arm anymore, so it should be fine if we drop these host environments > now (these are also the two architectures that contribute the most to > the long test times in our CI, so we would benefit a lot by dropping > those). Your description here is a little ambiguous about what's being proposed. When you say dropping 32-bit host support do you mean just for the system emulator binaries, or for QEMU entirely ? And when the deprecation period is passed, are you proposing to actively prevent 32-bit builds, or merely stopping CI testing and leave 32-bit builds still working if people want them ? 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 1/2] docs/about: Deprecate 32-bit x86 hosts and qemu-system-i386
On Mon, Feb 27, 2023 at 12:10:49PM +0100, Thomas Huth wrote: > Hardly anybody still uses 32-bit x86 hosts today, so we should > start deprecating them to finally have less test efforts. > With regards to 32-bit KVM support in the x86 Linux kernel, > the developers confirmed that they do not need a recent > qemu-system-i386 binary here: > > https://lore.kernel.org/kvm/y%2ffkts5ajfy0h...@google.com/ > > Signed-off-by: Thomas Huth > --- > docs/about/deprecated.rst | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 15084f7bea..98517f5187 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -196,6 +196,19 @@ CI coverage support may bitrot away before the > deprecation process > completes. The little endian variants of MIPS (both 32 and 64 bit) are > still a supported host architecture. > > +32-bit x86 hosts and ``qemu-system-i386`` (since 8.0) > +' > + > +Testing 32-bit x86 host OS support takes a lot of precious time during the > +QEMU contiguous integration tests, and considering that most OS vendors > +stopped shipping 32-bit variants of their x86 OS distributions and most > +x86 hardware from the past >10 years is capable of 64-bit, keeping the > +32-bit support alive is an inadequate burden for the QEMU project. Thus > +QEMU will soon drop the support for 32-bit x86 host systems and the > +``qemu-system-i386`` binary. Use ``qemu-system-x86_64`` (which is a proper > +superset of ``qemu-system-i386``) on a 64-bit host machine instead. I feel like we should have separate deprecation entries for the i686 host support, and for qemu-system-i386 emulator binary, as although they're related they are independant features with differing impact. eg removing qemu-system-i386 affects all host architectures, not merely 32-bit x86 host, so I think we can explain the impact more clearly if we separate them. How about something like 32-bit x86 hosts Support for 32-bit x86 host deployments is increasingly uncommon in mainstream Linux distributions given the widespread availability of 64-bit x86 hardware. The QEMU project no longer considers 32-bit x86 support to be an effective use of its limited resources, and thus intends to discontinue it. Current users of QEMU on 32-bit x86 hosts should either continue using existing releases of QEMU, with the caveat that they will no longer get security fixes, or migrate to a 64-bit platform which remains capable of running 32-bit guests if needed. ``qemu-system-i386`` binary removal ''' The ``qemu-system-x86_64`` binary can be used to run 32-bit guests by selecting a 32-bit CPU model, including KVM support on x86_64 hosts. Once support for the 32-bit x86 host platform is discontinued, the ``qemu-system-i386`` binary will be redundant. Current users are recommended to reconfigure their systems to use the ``qemu-system-x86_64`` binary. Same point for the next patch about 32-bit arm vs qemu-system-arm binary. 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 5/6] tests: add G_GNUC_PRINTF for various functions
On Thu, Dec 29, 2022 at 10:34:55AM +0100, Thomas Huth wrote:
> On 19/12/2022 14.02, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > tests/qtest/ahci-test.c | 3 +++
> > tests/qtest/arm-cpu-features.c| 1 +
> > tests/qtest/erst-test.c | 2 +-
> > tests/qtest/ide-test.c| 3 ++-
> > tests/qtest/ivshmem-test.c| 4 ++--
> > tests/qtest/libqmp.c | 2 +-
> > tests/qtest/libqos/libqos-pc.h| 6 --
> > tests/qtest/libqos/libqos-spapr.h | 6 --
> > tests/qtest/libqos/libqos.h | 6 --
> > tests/qtest/libqos/virtio-9p.c| 1 +
> > tests/qtest/migration-helpers.h | 1 +
> > tests/qtest/rtas-test.c | 2 +-
> > tests/qtest/usb-hcd-uhci-test.c | 4 ++--
> > tests/unit/test-qmp-cmds.c| 13 +
> > 14 files changed, 36 insertions(+), 18 deletions(-)
> ...
> > diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
> > index 2373cd64cb..6d52b4e5d8 100644
> > --- a/tests/unit/test-qmp-cmds.c
> > +++ b/tests/unit/test-qmp-cmds.c
> > @@ -138,6 +138,7 @@ void qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
> > }
> > +G_GNUC_PRINTF(2, 3)
> > static QObject *do_qmp_dispatch(bool allow_oob, const char *template, ...)
> > {
> > va_list ap;
> > @@ -160,6 +161,7 @@ static QObject *do_qmp_dispatch(bool allow_oob, const
> > char *template, ...)
> > return ret;
> > }
> > +G_GNUC_PRINTF(3, 4)
> > static void do_qmp_dispatch_error(bool allow_oob, ErrorClass cls,
> > const char *template, ...)
> > {
> > @@ -269,7 +271,7 @@ static void test_dispatch_cmd_io(void)
> > static void test_dispatch_cmd_deprecated(void)
> > {
> > -const char *cmd = "{ 'execute': 'test-command-features1' }";
> > +#define cmd "{ 'execute': 'test-command-features1' }"
> > QDict *ret;
>
> That looks weird, why is this required?
This means that the compiler can see we're passing a constant string
into the API call a few lines lower. Without it, the compiler can't
see that 'cmd' doesn't contain any printf format specifiers, and thus
it would force us to use func('%s', cmd)... except that's not actually
possible since our crazy json printf formatter doesn't allow arbitrary
'%s', the '%s' can only occur at well defined points in the JSON doc.
Using a constant string via #define was the easiest way to give the
compiler visibility of the safety.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH 5/6] tests: add G_GNUC_PRINTF for various functions
Signed-off-by: Daniel P. Berrangé
---
tests/qtest/ahci-test.c | 3 +++
tests/qtest/arm-cpu-features.c| 1 +
tests/qtest/erst-test.c | 2 +-
tests/qtest/ide-test.c| 3 ++-
tests/qtest/ivshmem-test.c| 4 ++--
tests/qtest/libqmp.c | 2 +-
tests/qtest/libqos/libqos-pc.h| 6 --
tests/qtest/libqos/libqos-spapr.h | 6 --
tests/qtest/libqos/libqos.h | 6 --
tests/qtest/libqos/virtio-9p.c| 1 +
tests/qtest/migration-helpers.h | 1 +
tests/qtest/rtas-test.c | 2 +-
tests/qtest/usb-hcd-uhci-test.c | 4 ++--
tests/unit/test-qmp-cmds.c| 13 +
14 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 66652fed04..1967cd5898 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -154,6 +154,7 @@ static void ahci_migrate(AHCIQState *from, AHCIQState *to,
const char *uri)
/**
* Start a Q35 machine and bookmark a handle to the AHCI device.
*/
+G_GNUC_PRINTF(1, 0)
static AHCIQState *ahci_vboot(const char *cli, va_list ap)
{
AHCIQState *s;
@@ -171,6 +172,7 @@ static AHCIQState *ahci_vboot(const char *cli, va_list ap)
/**
* Start a Q35 machine and bookmark a handle to the AHCI device.
*/
+G_GNUC_PRINTF(1, 2)
static AHCIQState *ahci_boot(const char *cli, ...)
{
AHCIQState *s;
@@ -209,6 +211,7 @@ static void ahci_shutdown(AHCIQState *ahci)
* Boot and fully enable the HBA device.
* @see ahci_boot, ahci_pci_enable and ahci_hba_enable.
*/
+G_GNUC_PRINTF(1, 2)
static AHCIQState *ahci_boot_and_enable(const char *cli, ...)
{
AHCIQState *ahci;
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 5a14527386..8691802950 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -32,6 +32,7 @@ static QDict *do_query_no_props(QTestState *qts, const char
*cpu_type)
QUERY_TAIL, cpu_type);
}
+G_GNUC_PRINTF(3, 4)
static QDict *do_query(QTestState *qts, const char *cpu_type,
const char *fmt, ...)
{
diff --git a/tests/qtest/erst-test.c b/tests/qtest/erst-test.c
index 974e8bcfe5..c45bee7f05 100644
--- a/tests/qtest/erst-test.c
+++ b/tests/qtest/erst-test.c
@@ -98,7 +98,7 @@ static void setup_vm_cmd(ERSTState *s, const char *cmd)
const char *arch = qtest_get_arch();
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-s->qs = qtest_pc_boot(cmd);
+s->qs = qtest_pc_boot("%s", cmd);
} else {
g_printerr("erst-test tests are only available on x86\n");
exit(EXIT_FAILURE);
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index dbe1563b23..dcb050bf9b 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -125,6 +125,7 @@ static QGuestAllocator guest_malloc;
static char *tmp_path[2];
static char *debug_path;
+G_GNUC_PRINTF(1, 2)
static QTestState *ide_test_start(const char *cmdline_fmt, ...)
{
QTestState *qts;
@@ -788,7 +789,7 @@ static void test_flush_nodev(void)
QPCIDevice *dev;
QPCIBar bmdma_bar, ide_bar;
-qts = ide_test_start("");
+qts = ide_test_start("%s", "");
dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
index cd550c8935..9bf8e78df6 100644
--- a/tests/qtest/ivshmem-test.c
+++ b/tests/qtest/ivshmem-test.c
@@ -109,9 +109,9 @@ static void setup_vm_cmd(IVState *s, const char *cmd, bool
msix)
const char *arch = qtest_get_arch();
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-s->qs = qtest_pc_boot(cmd);
+s->qs = qtest_pc_boot("%s", cmd);
} else if (strcmp(arch, "ppc64") == 0) {
-s->qs = qtest_spapr_boot(cmd);
+s->qs = qtest_spapr_boot("%s", cmd);
} else {
g_printerr("ivshmem-test tests are only available on x86 or ppc64\n");
exit(EXIT_FAILURE);
diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
index 2b08382e5d..a89cab03c3 100644
--- a/tests/qtest/libqmp.c
+++ b/tests/qtest/libqmp.c
@@ -134,7 +134,7 @@ static void socket_send_fds(int socket_fd, int *fds, size_t
fds_num,
* in the case that they choose to discard all replies up until
* a particular EVENT is received.
*/
-static void
+static G_GNUC_PRINTF(4, 0) void
_qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
const char *fmt, va_list ap)
{
diff --git a/tests/qtest/libqos/libqos-pc.h b/tests/qtest/libqos/libqos-pc.h
index 1a9923ead4..a2e4209a49 100644
--- a/tests/qtest/libqos/libqos-pc.h
+++ b/tests/qtest/libqos/libqos-pc.h
@@ -3,8 +3,10 @@
#include "libqos.h"
-QOSState *qtest_pc_vboot(const char *cmdline_fmt, va_list ap);
-QOSState *qtest_pc_boot(co
[PATCH 3/6] tools/virtiofsd: add G_GNUC_PRINTF for logging functions
Signed-off-by: Daniel P. Berrangé
---
tools/virtiofsd/fuse_log.c | 1 +
tools/virtiofsd/fuse_log.h | 6 --
tools/virtiofsd/passthrough_ll.c | 1 +
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/virtiofsd/fuse_log.c b/tools/virtiofsd/fuse_log.c
index 745d88cd2a..2de3f48ee7 100644
--- a/tools/virtiofsd/fuse_log.c
+++ b/tools/virtiofsd/fuse_log.c
@@ -12,6 +12,7 @@
#include "fuse_log.h"
+G_GNUC_PRINTF(2, 0)
static void default_log_func(__attribute__((unused)) enum fuse_log_level level,
const char *fmt, va_list ap)
{
diff --git a/tools/virtiofsd/fuse_log.h b/tools/virtiofsd/fuse_log.h
index 8d7091bd4d..e5c2967ab9 100644
--- a/tools/virtiofsd/fuse_log.h
+++ b/tools/virtiofsd/fuse_log.h
@@ -45,7 +45,8 @@ enum fuse_log_level {
* @param ap format string arguments
*/
typedef void (*fuse_log_func_t)(enum fuse_log_level level, const char *fmt,
-va_list ap);
+va_list ap)
+G_GNUC_PRINTF(2, 0);
/**
* Install a custom log handler function.
@@ -68,6 +69,7 @@ void fuse_set_log_func(fuse_log_func_t func);
* @param level severity level (FUSE_LOG_ERR, FUSE_LOG_DEBUG, etc)
* @param fmt sprintf-style format string including newline
*/
-void fuse_log(enum fuse_log_level level, const char *fmt, ...);
+void fuse_log(enum fuse_log_level level, const char *fmt, ...)
+G_GNUC_PRINTF(2, 3);
#endif /* FUSE_LOG_H_ */
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 20f0f41f99..40ea2ed27f 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -4182,6 +4182,7 @@ static void setup_nofile_rlimit(unsigned long
rlimit_nofile)
}
}
+G_GNUC_PRINTF(2, 0)
static void log_func(enum fuse_log_level level, const char *fmt, va_list ap)
{
g_autofree char *localfmt = NULL;
--
2.38.1
[PATCH 6/6] enforce use of G_GNUC_PRINTF attributes
We've been very gradually adding G_GNUC_PRINTF annotations to functions over years. This has been useful in detecting certain malformed printf strings, or cases where we pass user data as the printf format which is a potential security flaw. Given the inherant memory corruption danger in use of format strings vs mis-matched variadic arguments, it is worth applying G_GNUC_PRINTF to all functions using printf, even if we know they are safe. The compilers can reasonably reliably identify such places with the -Wsuggest-attribute=format / -Wmissing-format-attribute flags. Signed-off-by: Daniel P. Berrangé --- configure | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configure b/configure index 26c7bc5154..b9abe19e16 100755 --- a/configure +++ b/configure @@ -1208,6 +1208,8 @@ add_to warn_flags -Wnested-externs add_to warn_flags -Wendif-labels add_to warn_flags -Wexpansion-to-defined add_to warn_flags -Wimplicit-fallthrough=2 +add_to warn_flags -Wsuggest-attribute=format +add_to warn_flags -Wmissing-format-attribute nowarn_flags= add_to nowarn_flags -Wno-initializer-overrides -- 2.38.1
[PATCH 1/6] disas: add G_GNUC_PRINTF to gstring_printf
Signed-off-by: Daniel P. Berrangé
---
disas.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/disas.c b/disas.c
index 94d3b45042..31df8f5b89 100644
--- a/disas.c
+++ b/disas.c
@@ -239,6 +239,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong
code,
}
}
+G_GNUC_PRINTF(2, 3)
static int gstring_printf(FILE *stream, const char *fmt, ...)
{
/* We abuse the FILE parameter to pass a GString. */
--
2.38.1
[PATCH 4/6] util/error: add G_GNUC_PRINTF for various functions
Signed-off-by: Daniel P. Berrangé
---
util/error-report.c | 1 +
util/error.c| 1 +
2 files changed, 2 insertions(+)
diff --git a/util/error-report.c b/util/error-report.c
index 5edb2e6040..6e44a55732 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -193,6 +193,7 @@ real_time_iso8601(void)
* a single phrase, with no newline or trailing punctuation.
* Prepend the current location and append a newline.
*/
+G_GNUC_PRINTF(2, 0)
static void vreport(report_type type, const char *fmt, va_list ap)
{
gchar *timestr;
diff --git a/util/error.c b/util/error.c
index b6c89d1412..1e7af665b8 100644
--- a/util/error.c
+++ b/util/error.c
@@ -45,6 +45,7 @@ static void error_handle_fatal(Error **errp, Error *err)
}
}
+G_GNUC_PRINTF(6, 0)
static void error_setv(Error **errp,
const char *src, int line, const char *func,
ErrorClass err_class, const char *fmt, va_list ap,
--
2.38.1
[PATCH 0/6] enforce use of G_GNUC_PRINTF annotations
We've been very gradually adding G_GNUC_PRINTF annotations to functions over years. This has been useful in detecting certain malformed printf strings, or cases where we pass user data as the printf format which is a potential security flaw. Given the inherant memory corruption danger in use of format strings vs mis-matched variadic arguments, it is worth applying G_GNUC_PRINTF to all functions using printf, even if we know they are safe. The compilers can reasonably reliably identify such places with the -Wsuggest-attribute=format / -Wmissing-format-attribute flags. This series adds G_GNUC_PRINTF / G_GNUC_SCANF to allow the code locations that the compilers highlight. Then it adds the above warning flags to the build flags, to catch any future additions of functions that take printf/scanf format strings. Daniel P. Berrangé (6): disas: add G_GNUC_PRINTF to gstring_printf hw/xen: use G_GNUC_PRINTF/SCANF for various functions tools/virtiofsd: add G_GNUC_PRINTF for logging functions util/error: add G_GNUC_PRINTF for various functions tests: add G_GNUC_PRINTF for various functions enforce use of G_GNUC_PRINTF attributes configure | 2 ++ disas.c | 1 + hw/xen/xen-bus.c | 1 + hw/xen/xen_pvdev.c| 1 + include/hw/xen/xen-bus-helper.h | 6 -- include/hw/xen/xen-bus.h | 3 ++- tests/qtest/ahci-test.c | 3 +++ tests/qtest/arm-cpu-features.c| 1 + tests/qtest/erst-test.c | 2 +- tests/qtest/ide-test.c| 3 ++- tests/qtest/ivshmem-test.c| 4 ++-- tests/qtest/libqmp.c | 2 +- tests/qtest/libqos/libqos-pc.h| 6 -- tests/qtest/libqos/libqos-spapr.h | 6 -- tests/qtest/libqos/libqos.h | 6 -- tests/qtest/libqos/virtio-9p.c| 1 + tests/qtest/migration-helpers.h | 1 + tests/qtest/rtas-test.c | 2 +- tests/qtest/usb-hcd-uhci-test.c | 4 ++-- tests/unit/test-qmp-cmds.c| 13 + tools/virtiofsd/fuse_log.c| 1 + tools/virtiofsd/fuse_log.h| 6 -- tools/virtiofsd/passthrough_ll.c | 1 + util/error-report.c | 1 + util/error.c | 1 + 25 files changed, 55 insertions(+), 23 deletions(-) -- 2.38.1
[PATCH 2/6] hw/xen: use G_GNUC_PRINTF/SCANF for various functions
Signed-off-by: Daniel P. Berrangé
---
hw/xen/xen-bus.c| 1 +
hw/xen/xen_pvdev.c | 1 +
include/hw/xen/xen-bus-helper.h | 6 --
include/hw/xen/xen-bus.h| 3 ++-
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 645a29a5a0..df3f6b9ae0 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -561,6 +561,7 @@ void xen_device_backend_printf(XenDevice *xendev, const
char *key,
}
}
+G_GNUC_SCANF(3, 4)
static int xen_device_backend_scanf(XenDevice *xendev, const char *key,
const char *fmt, ...)
{
diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
index 037152f063..1a5177b354 100644
--- a/hw/xen/xen_pvdev.c
+++ b/hw/xen/xen_pvdev.c
@@ -196,6 +196,7 @@ const char *xenbus_strstate(enum xenbus_state state)
* 2 == noisy debug messages (logfile only).
* 3 == will flood your log (logfile only).
*/
+G_GNUC_PRINTF(3, 0)
static void xen_pv_output_msg(struct XenLegacyDevice *xendev,
FILE *f, const char *fmt, va_list args)
{
diff --git a/include/hw/xen/xen-bus-helper.h b/include/hw/xen/xen-bus-helper.h
index 629a904d1a..8782f30550 100644
--- a/include/hw/xen/xen-bus-helper.h
+++ b/include/hw/xen/xen-bus-helper.h
@@ -31,10 +31,12 @@ void xs_node_printf(struct xs_handle *xsh,
xs_transaction_t tid,
/* Read from node/key unless node is empty, in which case read from key */
int xs_node_vscanf(struct xs_handle *xsh, xs_transaction_t tid,
const char *node, const char *key, Error **errp,
- const char *fmt, va_list ap);
+ const char *fmt, va_list ap)
+G_GNUC_SCANF(6, 0);
int xs_node_scanf(struct xs_handle *xsh, xs_transaction_t tid,
const char *node, const char *key, Error **errp,
- const char *fmt, ...);
+ const char *fmt, ...)
+G_GNUC_SCANF(6, 7);
/* Watch node/key unless node is empty, in which case watch key */
void xs_node_watch(struct xs_handle *xsh, const char *node, const char *key,
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 713e763348..4d966a2dbb 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -94,7 +94,8 @@ void xen_device_frontend_printf(XenDevice *xendev, const char
*key,
G_GNUC_PRINTF(3, 4);
int xen_device_frontend_scanf(XenDevice *xendev, const char *key,
- const char *fmt, ...);
+ const char *fmt, ...)
+G_GNUC_SCANF(3, 4);
void xen_device_set_max_grant_refs(XenDevice *xendev, unsigned int nr_refs,
Error **errp);
--
2.38.1
Re: [PATCH v12 15/17] net: stream: move to QIO to enable additional parameters
On Thu, Oct 20, 2022 at 03:09:10PM +0200, Philippe Mathieu-Daudé wrote:
> On 20/10/22 11:16, Laurent Vivier wrote:
> > Use QIOChannel, QIOChannelSocket and QIONetListener.
> > This allows net/stream to use all the available parameters provided by
> > SocketAddress.
> >
> > Signed-off-by: Laurent Vivier
> > Acked-by: Michael S. Tsirkin
> > ---
> > meson | 2 +-
> > net/stream.c| 493 +---
> > qemu-options.hx | 4 +-
> > 3 files changed, 180 insertions(+), 319 deletions(-)
>
> > static int net_stream_server_init(NetClientState *peer,
> > @@ -283,105 +287,61 @@ static int net_stream_server_init(NetClientState
> > *peer,
> > {
> > NetClientState *nc;
> > NetStreamState *s;
> > -int fd, ret;
> > -
> > -switch (addr->type) {
> > -case SOCKET_ADDRESS_TYPE_INET: {
> > -struct sockaddr_in saddr_in;
> > -
> > -if (convert_host_port(&saddr_in, addr->u.inet.host,
> > addr->u.inet.port,
> > - errp) < 0) {
> > -return -1;
> > -}
> > +QIOChannelSocket *listen_sioc = qio_channel_socket_new();
> > -fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> > -if (fd < 0) {
> > -error_setg_errno(errp, errno, "can't create stream socket");
> > -return -1;
> > -}
> > -qemu_socket_set_nonblock(fd);
> > +nc = qemu_new_net_client(&net_stream_info, peer, model, name);
> > +s = DO_UPCAST(NetStreamState, nc, nc);
> > -socket_set_fast_reuse(fd);
> > +s->listen_ioc = QIO_CHANNEL(listen_sioc);
> > +qio_channel_socket_listen_async(listen_sioc, addr, 0,
> > +net_stream_server_listening, s,
> > +NULL, NULL);
> > -ret = bind(fd, (struct sockaddr *)&saddr_in, sizeof(saddr_in));
> > -if (ret < 0) {
> > -error_setg_errno(errp, errno, "can't bind ip=%s to socket",
> > - inet_ntoa(saddr_in.sin_addr));
> > -closesocket(fd);
> > -return -1;
> > -}
> > -break;
> > -}
> > -case SOCKET_ADDRESS_TYPE_UNIX: {
> > -struct sockaddr_un saddr_un;
> > -
> > -ret = unlink(addr->u.q_unix.path);
> > -if (ret < 0 && errno != ENOENT) {
> > -error_setg_errno(errp, errno, "failed to unlink socket %s",
> > - addr->u.q_unix.path);
> > -return -1;
> > -}
> > +return 0;
> > +}
> > -saddr_un.sun_family = PF_UNIX;
> > -ret = snprintf(saddr_un.sun_path, sizeof(saddr_un.sun_path), "%s",
> > - addr->u.q_unix.path);
> > -if (ret < 0 || ret >= sizeof(saddr_un.sun_path)) {
> > -error_setg(errp, "UNIX socket path '%s' is too long",
> > - addr->u.q_unix.path);
> > -error_append_hint(errp, "Path must be less than %zu bytes\n",
> > - sizeof(saddr_un.sun_path));
> > -return -1;
> > -}
> > +static void net_stream_client_connected(QIOTask *task, gpointer opaque)
> > +{
> > +NetStreamState *s = opaque;
> > +QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(s->ioc);
> > +SocketAddress *addr;
> > +gchar *uri;
> > +int ret;
> > -fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
> > -if (fd < 0) {
> > -error_setg_errno(errp, errno, "can't create stream socket");
> > -return -1;
> > -}
> > -qemu_socket_set_nonblock(fd);
> > -
> > -ret = bind(fd, (struct sockaddr *)&saddr_un, sizeof(saddr_un));
> > -if (ret < 0) {
> > -error_setg_errno(errp, errno, "can't create socket with path:
> > %s",
> > - saddr_un.sun_path);
> > -closesocket(fd);
> > -return -1;
> > -}
> > -break;
> > -}
> > -case SOCKET_ADDRESS_TYPE_FD:
> > -fd = monitor_fd_param(monitor_cur(), addr->u.fd.str, errp);
> > -if (fd == -1) {
> > -return -1;
> > -}
> > -ret = qemu_socket_try_set_nonblock(fd);
> > -if (ret < 0) {
> > -error_setg_errno(errp, -ret, "%s: Can't use file descriptor
> > %d",
> > - name, fd);
> > -return -1;
> > -}
> > -break;
> > -default:
> > -error_setg(errp, "only support inet or fd type");
> > -return -1;
> > +if (sioc->fd < 0) {
> > +qemu_set_info_str(&s->nc, "connection error");
> > +goto error;
> > }
> > -ret = listen(fd, 0);
> > -if (ret < 0) {
> > -error_setg_errno(errp, errno, "can't listen on socket");
> > -closesocket(fd);
> > -return -1;
> > +addr = qio_channel_socket_get_remote_address(sioc, NULL);
> > +g_assert(addr != NULL);
>
> Please use:
>
>addr = qio_channel_socket_get_remote_address(
Re: [libvirt PATCH] libxl: Fix build with recent Xen that introduces new disk backend type
On Tue, Aug 02, 2022 at 01:39:31PM +0200, Marek Marczykowski-Górecki wrote: > On Mon, Aug 01, 2022 at 10:36:07AM +0100, Daniel P. Berrangé wrote: > > Generally we want to see errors triggered from new enums arriving, > > as it can be a sign that libvirt code needs a semantic change in > > order to continue operating correctly. It isn't always correct > > to assume that the 'default' case gives the correct behaviour. > > Isn't that the exact purpose of 'default' label? If use of 'default' > means "any of the other 5 specific values, but lets save some characters > to not name them explicitly", then IMHO better to name them > explicitly... > > I can see a value of -Werror=switch-enum when adding new (internal) enum > value, to find all the cases where code needs to be adjusted, but even > then a grep is probably sufficient enough. On the other hand, if there > are cases where indeed all the values of (internal API) enum needs to be > handled explicitly, maybe simply omit 'default' label and use > -Werror=switch? > > Anyway, if tracking all the enums values of all the used 3rd-party APIs > is desirable (like, noticing when libxl adds new disk type), then it > probably should be a separate CI job, not the default devel build. > Otherwise breakages like this will happen from time to time, and will > be annoyed for people on involved in specific code part at all. > > As a short term fix, maybe Xen's CI can build libvirt with > -Wno-error=switch-enum? I think makes sense for a 3rd party CI todo that, since these warnings are primarily targetted at libvirt upstream maintainers, so that we catch problems before code is committed. 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: [libvirt PATCH] libxl: Fix build with recent Xen that introduces new disk backend type
On Mon, Aug 01, 2022 at 09:51:11AM +0100, Julien Grall wrote:
> Hi Michal,
>
> On 01/08/2022 09:23, Michal Prívozník wrote:
> > On 7/29/22 17:50, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko
> > >
> > > Xen toolstack has gained basic Virtio support recently which becides
> > > adding various virtio related stuff introduces new disk backend type
> > > LIBXL_DISK_BACKEND_STANDALONE [1].
> > >
> > > Unfortunately, this caused a regression in libvirt build with Xen support
> > > enabled, reported by the osstest today [2]:
> > >
> > > CC libxl/libvirt_driver_libxl_impl_la-xen_xl.lo
> > > ../../src/libxl/xen_xl.c: In function 'xenParseXLDisk':
> > > ../../src/libxl/xen_xl.c:779:17: error: enumeration value
> > > 'LIBXL_DISK_BACKEND_STANDALONE'
> > > not handled in switch [-Werror=switch-enum]
> > > switch (libxldisk->backend) {
> > > ^~
> > > cc1: all warnings being treated as errors
> > >
> > > The interesting fact is that switch already has a default branch (which
> > > ought
> > > to cover such new addition), but the error is triggered as -Wswitch-enum
> > > gives a warning about an omitted enumeration code even if there is a
> > > default
> > > label.
> >
> > This is expected and in fact working correctly. We want compiler to warn
> > us about enum members that are not handled in a switch() statement.
>
> For us this is treated as an error. Is it intended?
Yes & no, but mostly yes.
You can choose to build with -Werror or not. If building from .git
then it defaults to enabled, but can be disabled if desired.
Generally we want to see errors triggered from new enums arriving,
as it can be a sign that libvirt code needs a semantic change in
order to continue operating correctly. It isn't always correct
to assume that the 'default' case gives the correct behaviour.
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: [PULL 00/16] Kraxel 20220613 patches
On Tue, Jun 14, 2022 at 11:40:38AM +0200, Gerd Hoffmann wrote:
> On Mon, Jun 13, 2022 at 08:52:21AM -0700, Richard Henderson wrote:
> > On 6/13/22 04:36, Gerd Hoffmann wrote:
> > > The following changes since commit
> > > dcb40541ebca7ec98a14d461593b3cd7282b4fac:
> > >
> > >Merge tag 'mips-20220611' of https://github.com/philmd/qemu into
> > > staging (2022-06-11 21:13:27 -0700)
> > >
> > > are available in the Git repository at:
> > >
> > >git://git.kraxel.org/qemu tags/kraxel-20220613-pull-request
> > >
> > > for you to fetch changes up to 23b87f7a3a13e93e248eef8a4b7257548855a620:
> > >
> > >ui: move 'pc-bios/keymaps' to 'ui/keymaps' (2022-06-13 10:59:25 +0200)
> > >
> > >
> > > usb: add CanoKey device, fixes for ehci + redir
> > > ui: fixes for gtk and cocoa, move keymaps (v2), rework refresh rate
> > > virtio-gpu: scanout flush fix
> >
> > This doesn't even configure:
> >
> > ../src/ui/keymaps/meson.build:55:4: ERROR: File ar does not exist.
>
> Hmm, build worked here and CI passed too.
>
> I think this is one of those cases where the build directory must be
> deleted because one subdirectory is replaced by a compatibility
> symlink.
Except 'configure' deals with that, as it explicitly rm -rf's the
symlink target:
symlink() {
rm -rf "$2"
mkdir -p "$(dirname "$2")"
ln -s "$1" "$2"
}
so i'm pretty confused as to what's going wrong here still
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: [PULL 00/17] Kraxel 20220610 patches
On Sat, Jun 11, 2022 at 06:34:28PM +0200, Volker Rümelin wrote: > Am 10.06.22 um 22:16 schrieb Richard Henderson: > > On 6/10/22 02:20, Gerd Hoffmann wrote: > > > The following changes since commit > > > 9cc1bf1ebca550f8d90f967ccd2b6d2e00e81387: > > > > > > Merge tag 'pull-xen-20220609' of > > > https://xenbits.xen.org/git-http/people/aperard/qemu-dm into staging > > > (2022-06-09 08:25:17 -0700) > > > > > > are available in the Git repository at: > > > > > > git://git.kraxel.org/qemu tags/kraxel-20220610-pull-request > > > > > > for you to fetch changes up to 02319a4d67d3f19039127b8dc9ca9478b6d6ccd8: > > > > > > virtio-gpu: Respect UI refresh rate for EDID (2022-06-10 11:11:44 > > > +0200) > > > > > > > > > usb: add CanoKey device, fixes for ehci + redir > > > ui: fixes for gtk and cocoa, move keymaps, rework refresh rate > > > virtio-gpu: scanout flush fix > > > > This introduces regressions: > > > > https://gitlab.com/qemu-project/qemu/-/jobs/2576157660 > > https://gitlab.com/qemu-project/qemu/-/jobs/2576151565 > > https://gitlab.com/qemu-project/qemu/-/jobs/2576154539 > > https://gitlab.com/qemu-project/qemu/-/jobs/2575867208 > > > > > > (27/43) > > tests/avocado/vnc.py:Vnc.test_change_password_requires_a_password: > > ERROR: ConnectError: Failed to establish session: EOFError\n Exit code: > > 1\n Command: ./qemu-system-x86_64 -display none -vga none -chardev > > socket,id=mon,path=/var/tmp/avo_qemu_sock_4nrz0r37/qemu-2912538-7f732e94e0f0-monitor.sock > > -mon chardev=mon,mode=control -node... (0.09 s) > > (28/43) tests/avocado/vnc.py:Vnc.test_change_password: ERROR: > > ConnectError: Failed to establish session: EOFError\n Exit code: > > 1\n Command: ./qemu-system-x86_64 -display none -vga none -chardev > > socket,id=mon,path=/var/tmp/avo_qemu_sock_yhpzy5c3/qemu-2912543-7f732e94b438-monitor.sock > > -mon chardev=mon,mode=control -node... (0.09 s) > > (29/43) > > tests/avocado/vnc.py:Vnc.test_change_password_requires_a_password: > > ERROR: ConnectError: Failed to establish session: EOFError\n Exit code: > > 1\n Command: ./qemu-system-x86_64 -display none -vga none -chardev > > socket,id=mon,path=/var/tmp/avo_qemu_sock_tk3pfmt2/qemu-2912548-7f732e93d7b8-monitor.sock > > -mon chardev=mon,mode=control -node... (0.09 s) > > > > > > r~ > > > > This is caused by [PATCH 14/17] ui: move 'pc-bios/keymaps' to 'ui/keymaps'. > After this patch QEMU no longer finds it's keymaps if started directly from > the build directory. I just sent Gerd an update version which adds a symlink from the source tree to the build dir to solve this problem, along with updated commit message to reflect this need 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 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
On Wed, Mar 16, 2022 at 01:52:48PM +0400, marcandre.lur...@redhat.com wrote: > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > index 3baa5e3790f7..f2bd050e3b9a 100644 > --- a/include/qemu/compiler.h > +++ b/include/qemu/compiler.h > @@ -79,19 +79,12 @@ > #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \ > sizeof(QEMU_BUILD_BUG_ON_STRUCT(x))) > > -#if defined(__clang__) > -/* clang doesn't support gnu_printf, so use printf. */ > -# define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m))) > -#else > -/* Use gnu_printf (qemu uses standard format strings). */ > -# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m))) > -# if defined(_WIN32) > +#if !defined(__clang__) && defined(_WIN32) > /* > * Map __printf__ to __gnu_printf__ because we want standard format strings > even > * when MinGW or GLib include files use __printf__. > */ > -# define __printf__ __gnu_printf__ > -# endif > +# define __printf__ __gnu_printf__ > #endif I'm not convinced we shold have this remaining define, even before your patch. For code we've implemented, we should have used __gnu_printf__ already if we know it uses GNU format on Windows. For code in GLib, its header file uses __gnu_printf__ for anything that relies on its portable printf replacement, which is basically everything in GLib. For anything else we should honour whatever they declare, and not assume their impl is the GNU one. I guess it is easy enough to validate by deleting this and seeing if we get any warnings from the mingw CI jobs about printf/gnu_printf mismatch. 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 3/3] Use g_new() & friends where that makes obvious sense
On Mon, Mar 14, 2022 at 05:52:32PM +0100, Markus Armbruster wrote: > Peter Maydell writes: > > > On Mon, 14 Mar 2022 at 16:01, Markus Armbruster wrote: > >> > >> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, > >> for two reasons. One, it catches multiplication overflowing size_t. > >> Two, it returns T * rather than void *, which lets the compiler catch > >> more type errors. > >> > >> This commit only touches allocations with size arguments of the form > >> sizeof(T). > >> > >> Patch created mechanically with: > >> > >> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ > >> --macro-file scripts/cocci-macro-file.h FILES... > >> > >> Signed-off-by: Markus Armbruster > >> --- > > > >> 104 files changed, 197 insertions(+), 202 deletions(-) > > > > I'm not going to say you must split this patch up. I'm just going to > > say that I personally am not looking at it, because it's too big > > for me to deal with. > > As with all big but trivial Coccinelle patches, reviewing the Coccinelle > script and a reasonably representative sample of its output is almost > certainly a better use of reviewer time than attempting to get all the > patches reviewed. They are mind-numbingly dull! > > For what it's worth, we've used this script several times before. Last > in commit bdd81addf4. This Coccinelle is simple enough to understand, that I'd suggest that once we merge the Coccinelle script itself, then for ongoing usage, its output can be considered effectively pre-reviewed. The reviewer can just re-run the Coccinelle script themselves to prove it has the same output as the submitter claims, to validate no manual changes are hidden in the middle of the automated patch. 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: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API
On Tue, Sep 14, 2021 at 01:30:27PM +, P J P wrote: > Hello Philippe, all > > >On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé > > wrote: > >On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote: > >> This series is experimental! The goal is to better limit the > >> boundary of what code is considerated security critical, and > >> what is less critical (but still important!). > >> > >> This approach was quickly discussed few months ago with Markus > >> then Daniel. Instead of classifying the code on a file path > >> basis (see [1]), we insert (runtime) hints into the code > >> (which survive code movement). Offending unsafe code can > >> taint the global security policy. By default this policy > >> is 'none': the current behavior. It can be changed on the > >> command line to 'warn' to display warnings, and to 'strict' > >> to prohibit QEMU running with a tainted policy. > > > > * Thanks so much for restarting this thread. I've been at it intermittently > last few > months, thinking about how could we annotate the source/module objects. > > -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html > > * Last time we discussed about having both compile and run time options for > users > to be able to select the qualified objects/backends/devices as desired. Right, we have multiple different use cases here - People building QEMU who want to cut down what they ship to minimize the stuff they support, which is outside the security guarantee. This can be OS distros, but also any other consumer of QEMU eg. RHEL wants to cut out almost all non-virtualization stuff. There is a quirk here though, that RHEL still includes TCG which is considered outside the security guarantee. So a simple build time "--secure on|off" doesn't do the job on its own. We need something to let people understand the consequences of the build options they are enabling. NB, when I talk of build options, I include both configure/meson args, and also the CONFIG_* options set in configs/**/*.mak - Application developers want to check that they're not using stuff outside the security guarantee, even if the distro has enable it. These need to be able to query whether the VM they've launched has undesirable configuration choices. Some people fall into both groups, some people fall into neither group. > * To confirm: How/Where is the security policy defined? Is it > device/module specific OR QEMU project wide? Currently our only definition is in the docs https://qemu-project.gitlab.io/qemu/system/security.html#security-requirements Philippe's patch is proposing tagging against internal QEMU objects of various types. I further proposed that we expose this in QMP so it is introspectable. I think there's scope for doing stuff at build time with configure args and *mak CONFIG_* options, but haven't thought what that might look like. > > IOW, the reporting via QAPI introspetion is much more important > > for libvirt and mgmt apps, than any custom cli arg / printfs > > at the QEMU level. > > > > * True, while it makes sense to have a solution that is conversant with > the management/libvirt layers, It'll be great if we could address qemu/cli > other use cases too. > > >it feels like we need > > 'secure': 'bool' > > * Though we started the (above [*]) discussion with '--security' option in > mind, > I wonder if 'secure' annotation is much specific. And if we could widen its > scope. > --- x --- > > > Source annotations: I've been thinking over following approaches > === > > 1) Segregate the QEMU sources under > > ../staging/ <= devel/experimental, not for production usage > ../src/ <= good for production usage, hence security relevant > ../deprecated/ <= Bad for production usage, not security relevant > > - This is similar to Linux staging drivers' tree. > - Staging drivers are not considered for production usage and hence CVE > allocation. > - At build time by default we only build sources under ../src/ tree. > - But we can still have options to build /staging/ and /deprecated/ trees. > > - It's readily understandable to end users. I don't think we should be working in terms of source files at all. Some files contain multiple distinct bits of functionality that are not easily separated, and will have distinct security levels. Also IMHO it is unpleasant to be moving files around in git
Re: [RFC PATCH 06/10] qdev: Use qemu_security_policy_taint() API
On Thu, Sep 09, 2021 at 01:20:20AM +0200, Philippe Mathieu-Daudé wrote:
> Add DeviceClass::taints_security_policy field to allow an
> unsafe device to eventually taint the global security policy
> in DeviceRealize().
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/hw/qdev-core.h | 6 ++
> hw/core/qdev.c | 11 +++
> 2 files changed, 17 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bafc311bfa1..ff9ce6671be 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -122,6 +122,12 @@ struct DeviceClass {
> */
> bool user_creatable;
> bool hotpluggable;
> +/*
> + * %false if the device is within the QEMU security policy boundary,
> + * %true if there is no guarantee this device can be used safely.
> + * See: https://www.qemu.org/contribute/security-process/
> + */
> +bool taints_security_policy;
>
> /* callbacks */
> /*
Although your use case is for devices, it probably makes more sense
to push this up into the Object base class.
I think it will need to be a tri-state value too, not a simple
bool. It isn't feasible to mark all devices with this property,
so initially we'll have no information about whether most
devices are secure or insecure. This patch gives the implication
that all devices are secure, except for the few that have been
marked otherwise, which is not a good default IMHO.
We want to be able to make it clear when introspecting, that we
have no information on security available for most devices ie
- unset => no information on security (the current default)
- true => considered secure against malicious guest
- false => considered insecure against malicious guest
Then we can also extend 'ObjectTypeInfo' to have a
'*secure': 'bool'
to make 'qom-list-types' be able to introspect this upfront.
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cefc5eaa0a9..a5a00f3564c 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -31,6 +31,7 @@
> #include "qapi/qmp/qerror.h"
> #include "qapi/visitor.h"
> #include "qemu/error-report.h"
> +#include "qemu-common.h"
> #include "qemu/option.h"
> #include "hw/hotplug.h"
> #include "hw/irq.h"
> @@ -257,6 +258,13 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp)
> MachineClass *mc;
> Object *m_obj = qdev_get_machine();
>
> +if (qemu_security_policy_is_strict()
> +&& DEVICE_GET_CLASS(dev)->taints_security_policy) {
> +error_setg(errp, "Device '%s' can not be hotplugged when"
> + " 'strict' security policy is in place",
> + object_get_typename(OBJECT(dev)));
Do you need a 'return' here to stop execution after reportig
the error ?
> +}
> +
> if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> machine = MACHINE(m_obj);
> mc = MACHINE_GET_CLASS(machine);
> @@ -385,6 +393,9 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error
> **errp)
> } else {
> assert(!DEVICE_GET_CLASS(dev)->bus_type);
> }
> +qemu_security_policy_taint(DEVICE_GET_CLASS(dev)->taints_security_policy,
> + "device type %s",
> + object_get_typename(OBJECT(dev)));
>
> return object_property_set_bool(OBJECT(dev), "realized", true, errp);
> }
> --
> 2.31.1
>
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: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API
On Thu, Sep 09, 2021 at 11:40:07AM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> > Add the BlockDriver::bdrv_taints_security_policy() handler.
> > Drivers implementing it might taint the global QEMU security
> > policy.
> >
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > include/block/block_int.h | 6 +-
> > block.c | 6 ++
> > 2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index f1a54db0f8c..0ec0a5c06e9 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -169,7 +169,11 @@ struct BlockDriver {
> > int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
> >Error **errp);
> > void (*bdrv_close)(BlockDriverState *bs);
> > -
> > +/*
> > + * Return %true if the driver is withing QEMU security policy boundary,
> > + * %false otherwise. See:
> > https://www.qemu.org/contribute/security-process/
> > + */
> > +bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
Also as with previous comments, I think we should not refer
to tainting or the security policy here, but instead simply
document whether we consider the bdrv to be secure or not.
Tainting is merely one action that is taken in accordance with
the security policy, as a result of the information presented.
> > int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
> > Error **errp);
> > diff --git a/block.c b/block.c
> > index b2b66263f9a..696ba486001 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -49,6 +49,7 @@
> > #include "qemu/timer.h"
> > #include "qemu/cutils.h"
> > #include "qemu/id.h"
> > +#include "qemu-common.h"
> > #include "block/coroutines.h"
> >
> > #ifdef CONFIG_BSD
> > @@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs,
> > BlockDriver *drv,
> > }
> > }
> >
> > +if (drv->bdrv_taints_security_policy) {
> > +qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
> > + "Block protocol '%s'",
> > drv->format_name);
> > +}
> > +
> > return 0;
> > open_failed:
> > bs->drv = NULL;
>
> Again we need a way to report this via QAPI, but we don't have a natural
> place is hang this off for introspection before starting a guest.
>
> The best we can do is report the information after a block backend has
> been instantiated. eg Modify "BlockInfo" struct to gain
>
>'*secure': 'bool'
>
> Note I made this an optional field, since unless we mark every single
> block driver impl straight away, we'll need to cope with the absence
> of information.
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: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API
On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> Add the BlockDriver::bdrv_taints_security_policy() handler.
> Drivers implementing it might taint the global QEMU security
> policy.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/block/block_int.h | 6 +-
> block.c | 6 ++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f1a54db0f8c..0ec0a5c06e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -169,7 +169,11 @@ struct BlockDriver {
> int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>Error **errp);
> void (*bdrv_close)(BlockDriverState *bs);
> -
> +/*
> + * Return %true if the driver is withing QEMU security policy boundary,
> + * %false otherwise. See:
> https://www.qemu.org/contribute/security-process/
> + */
> +bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
>
> int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
> Error **errp);
> diff --git a/block.c b/block.c
> index b2b66263f9a..696ba486001 100644
> --- a/block.c
> +++ b/block.c
> @@ -49,6 +49,7 @@
> #include "qemu/timer.h"
> #include "qemu/cutils.h"
> #include "qemu/id.h"
> +#include "qemu-common.h"
> #include "block/coroutines.h"
>
> #ifdef CONFIG_BSD
> @@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs,
> BlockDriver *drv,
> }
> }
>
> +if (drv->bdrv_taints_security_policy) {
> +qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
> + "Block protocol '%s'", drv->format_name);
> +}
> +
> return 0;
> open_failed:
> bs->drv = NULL;
Again we need a way to report this via QAPI, but we don't have a natural
place is hang this off for introspection before starting a guest.
The best we can do is report the information after a block backend has
been instantiated. eg Modify "BlockInfo" struct to gain
'*secure': 'bool'
Note I made this an optional field, since unless we mark every single
block driver impl straight away, we'll need to cope with the absence
of information.
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: [RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe
On Thu, Sep 09, 2021 at 01:20:16AM +0200, Philippe Mathieu-Daudé wrote:
> Add the AccelClass::secure_policy_supported field to classify
> safe (within security boundary) vs unsafe accelerators.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/qemu/accel.h | 5 +
> accel/kvm/kvm-all.c | 1 +
> accel/xen/xen-all.c | 1 +
> softmmu/vl.c | 3 +++
> 4 files changed, 10 insertions(+)
>
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 4f4c283f6fc..895e30be0de 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -44,6 +44,11 @@ typedef struct AccelClass {
> hwaddr start_addr, hwaddr size);
> #endif
> bool *allowed;
> +/*
> + * Whether the accelerator is withing QEMU security policy boundary.
> + * See: https://www.qemu.org/contribute/security-process/
> + */
> +bool secure_policy_supported;
The security handling policy is a high level concept that is
open to variation over time and also by downstream distro
vendors.
At a code level we should be dealing in a more fundamental
concept. At an accelerator level we should really jsut
declare whether or not the accelerator impl is considered
to be secure against malicious guest code.
eg
/* Whether this accelerator is secure against execution
* of malciious guest machine code */
bool secure;
> /*
> * Array of global properties that would be applied when specific
> * accelerator is chosen. It works like MachineClass.compat_props
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 0125c17edb8..eb6b9e44df2 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3623,6 +3623,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void
> *data)
> ac->init_machine = kvm_init;
> ac->has_memory = kvm_accel_has_memory;
> ac->allowed = &kvm_allowed;
> +ac->secure_policy_supported = true;
>
> object_class_property_add(oc, "kernel-irqchip", "on|off|split",
> NULL, kvm_set_kernel_irqchip,
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 69aa7d018b2..57867af5faf 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -198,6 +198,7 @@ static void xen_accel_class_init(ObjectClass *oc, void
> *data)
> ac->setup_post = xen_setup_post;
> ac->allowed = &xen_allowed;
> ac->compat_props = g_ptr_array_new();
> +ac->secure_policy_supported = true;
>
> compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 92c05ac97ee..e4f94e159c3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2388,6 +2388,9 @@ static int do_configure_accelerator(void *opaque,
> QemuOpts *opts, Error **errp)
> return 0;
> }
>
> +qemu_security_policy_taint(!ac->secure_policy_supported,
> + "%s accelerator", acc);
We need this information to be introspectable, becuase stuff printed
to stderr is essentially opaque to libvirt and mgmt apps above.
We don't have a convenient "query-accel" command but I think this
could possibly fit into 'query-target'. ie the TargetInfo struct
gain a field:
##
# @TargetInfo:
#
# Information describing the QEMU target.
#
# @arch: the target architecture
# @secure: Whether the currently active accelerator for this target
# is secure against execution of malicous guest code
#
# Since: 1.2
##
{ 'struct': 'TargetInfo',
'data': { 'arch': 'SysEmuTarget',
'secure': 'bool'} }
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: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API
On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote: > Hi, > > This series is experimental! The goal is to better limit the > boundary of what code is considerated security critical, and > what is less critical (but still important!). > > This approach was quickly discussed few months ago with Markus > then Daniel. Instead of classifying the code on a file path > basis (see [1]), we insert (runtime) hints into the code > (which survive code movement). Offending unsafe code can > taint the global security policy. By default this policy > is 'none': the current behavior. It can be changed on the > command line to 'warn' to display warnings, and to 'strict' > to prohibit QEMU running with a tainted policy. Ok, so I infer that you based this idea on the "--compat-policy" arg used to control behaviour wrt to deprecations. With the deprecation support, the QAPI introspection data can report deprecations for machines / CPUs (and in theory devices later). Libvirt records this deprecation info and can report it to the user before even starting a guest, so they can avoid using a deprecated device in the first place. We also use this info to mark a guest as tainted + deprecation at the libvirt level and let mgmt apps query this status. The --compat-policy support has been integrated into libvirt but it is not something we expect real world deployments to use - rather it is targeted as a testing framework. Essentially I see the security reporting as needing to operate in a pretty similar manner. IOW, the reporting via QAPI introspetion is much more important for libvirt and mgmt apps, than any custom cli arg / printfs at the QEMU level. In terms of QAPI design we currently have 'deprecated': 'bool' field against MachineInfo and CpuDefinitionInfo types. it feels like we need 'secure': 'bool' field against the relevant types here too, though I think maybe we might need to make it an optional field to let us distinguish lack of information, since it will take a long time to annotate all areas. eg '*secure': 'bool' - not set => no info available on security characteristics - true => device is considered secure wrt malicious guest - false => device is not considered secure wrt malicious guest > As examples I started implementing unsafe code taint from > 3 different pieces of code: > - accelerators (KVM and Xen in allow-list) > - block drivers (vvfat and parcial null-co in deny-list) > - qdev (hobbyist devices regularly hit by fuzzer) > > I don't want the security researchers to not fuzz QEMU unsafe > areas, but I'd like to make it clearer what the community > priority is (currently 47 opened issues on [3]). 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 5/8] gitlab-ci: Add KVM s390x cross-build jobs
On Mon, Dec 07, 2020 at 11:26:58AM +0100, Philippe Mathieu-Daudé wrote: > On 12/7/20 11:25 AM, Daniel P. Berrangé wrote: > > On Mon, Dec 07, 2020 at 06:46:01AM +0100, Thomas Huth wrote: > >> On 06/12/2020 19.55, Philippe Mathieu-Daudé wrote: > >>> Cross-build s390x target with only KVM accelerator enabled. > >>> > >>> Signed-off-by: Philippe Mathieu-Daudé > >>> --- > >>> .gitlab-ci.d/crossbuilds-kvm-s390x.yml | 6 ++ > >>> .gitlab-ci.yml | 1 + > >>> MAINTAINERS| 1 + > >>> 3 files changed, 8 insertions(+) > >>> create mode 100644 .gitlab-ci.d/crossbuilds-kvm-s390x.yml > >>> > >>> diff --git a/.gitlab-ci.d/crossbuilds-kvm-s390x.yml > >>> b/.gitlab-ci.d/crossbuilds-kvm-s390x.yml > >>> new file mode 100644 > >>> index 000..1731af62056 > >>> --- /dev/null > >>> +++ b/.gitlab-ci.d/crossbuilds-kvm-s390x.yml > >>> @@ -0,0 +1,6 @@ > >>> +cross-s390x-kvm: > >>> + extends: .cross_accel_build_job > >>> + variables: > >>> +IMAGE: debian-s390x-cross > >>> +TARGETS: s390x-softmmu > >>> +ACCEL_CONFIGURE_OPTS: --disable-tcg > >>> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > >>> index 573afceb3c7..a69619d7319 100644 > >>> --- a/.gitlab-ci.yml > >>> +++ b/.gitlab-ci.yml > >>> @@ -14,6 +14,7 @@ include: > >>>- local: '/.gitlab-ci.d/crossbuilds.yml' > >>>- local: '/.gitlab-ci.d/crossbuilds-kvm-x86.yml' > >>>- local: '/.gitlab-ci.d/crossbuilds-kvm-arm.yml' > >>> + - local: '/.gitlab-ci.d/crossbuilds-kvm-s390x.yml' > >> > >> KVM code is already covered by the "cross-s390x-system" job, but an > >> additional compilation test with --disable-tcg makes sense here. I'd then > >> rather name it "cross-s390x-no-tcg" or so instead of "cross-s390x-kvm". > >> > >> And while you're at it, I'd maybe rather name the new file just > >> crossbuilds-s390x.yml and also move the other s390x related jobs into it? > > > > I don't think we really should split it up so much - just put these > > jobs in the existing crosbuilds.yml file. > > Don't we want to leverage MAINTAINERS file? As mentioned in the cover letter, I think this is mis-using the MAINTAINERS file to try to represent something different. The MAINTAINERS file says who is responsible for the contents of the .yml file, which is the CI maintainers, because we want a consistent gitlab configuration as a whole, not everyone doing their own thing. MAINTAINERS doesn't say who is responsible for making sure the actual jobs that run are passing, which is potentially a completely different person. If we want to track that, it is not the MAINTAINERS file. 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 5/8] gitlab-ci: Add KVM s390x cross-build jobs
On Mon, Dec 07, 2020 at 06:46:01AM +0100, Thomas Huth wrote: > On 06/12/2020 19.55, Philippe Mathieu-Daudé wrote: > > Cross-build s390x target with only KVM accelerator enabled. > > > > Signed-off-by: Philippe Mathieu-Daudé > > --- > > .gitlab-ci.d/crossbuilds-kvm-s390x.yml | 6 ++ > > .gitlab-ci.yml | 1 + > > MAINTAINERS| 1 + > > 3 files changed, 8 insertions(+) > > create mode 100644 .gitlab-ci.d/crossbuilds-kvm-s390x.yml > > > > diff --git a/.gitlab-ci.d/crossbuilds-kvm-s390x.yml > > b/.gitlab-ci.d/crossbuilds-kvm-s390x.yml > > new file mode 100644 > > index 000..1731af62056 > > --- /dev/null > > +++ b/.gitlab-ci.d/crossbuilds-kvm-s390x.yml > > @@ -0,0 +1,6 @@ > > +cross-s390x-kvm: > > + extends: .cross_accel_build_job > > + variables: > > +IMAGE: debian-s390x-cross > > +TARGETS: s390x-softmmu > > +ACCEL_CONFIGURE_OPTS: --disable-tcg > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > > index 573afceb3c7..a69619d7319 100644 > > --- a/.gitlab-ci.yml > > +++ b/.gitlab-ci.yml > > @@ -14,6 +14,7 @@ include: > >- local: '/.gitlab-ci.d/crossbuilds.yml' > >- local: '/.gitlab-ci.d/crossbuilds-kvm-x86.yml' > >- local: '/.gitlab-ci.d/crossbuilds-kvm-arm.yml' > > + - local: '/.gitlab-ci.d/crossbuilds-kvm-s390x.yml' > > KVM code is already covered by the "cross-s390x-system" job, but an > additional compilation test with --disable-tcg makes sense here. I'd then > rather name it "cross-s390x-no-tcg" or so instead of "cross-s390x-kvm". > > And while you're at it, I'd maybe rather name the new file just > crossbuilds-s390x.yml and also move the other s390x related jobs into it? I don't think we really should split it up so much - just put these jobs in the existing crosbuilds.yml file. 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 0/8] gitlab-ci: Add accelerator-specific Linux jobs
On Sun, Dec 06, 2020 at 07:55:00PM +0100, Philippe Mathieu-Daudé wrote: > Hi, > > I was custom to use Travis-CI for testing KVM builds on s390x/ppc > with the Travis-CI jobs. > > During October Travis-CI became unusable for me (extremely slow, > see [1]). Then my free Travis account got updated to the new > "10K credit minutes allotment" [2] which I burned without reading > the notification email in time (I'd burn them eventually anyway). > > Today Travis-CI is pointless to me. While I could pay to run my > QEMU jobs, I don't think it is fair for an Open Source project to > ask its forks to pay for a service. > > As we want forks to run some CI before contributing patches, and > we have cross-build Docker images available for Linux hosts, I > added some cross KVM/Xen build jobs to Gitlab-CI. > > Cross-building doesn't have the same coverage as native building, > as we can not run the tests. But this is still useful to get link > failures. > > Each job is added in its own YAML file, so it is easier to notify > subsystem maintainers in case of troubles. > > Resulting pipeline: > https://gitlab.com/philmd/qemu/-/pipelines/225948077 > > Regards, > > Phil. > > [1] https://travis-ci.community/t/build-delays-for-open-source-project/10272 > [2] https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing > > Philippe Mathieu-Daudé (8): > gitlab-ci: Replace YAML anchors by extends (cross_system_build_job) > gitlab-ci: Introduce 'cross_accel_build_job' template > gitlab-ci: Add KVM X86 cross-build jobs > gitlab-ci: Add KVM ARM cross-build jobs > gitlab-ci: Add KVM s390x cross-build jobs > gitlab-ci: Add KVM PPC cross-build jobs > gitlab-ci: Add KVM MIPS cross-build jobs > gitlab-ci: Add Xen cross-build jobs > > .gitlab-ci.d/crossbuilds-kvm-arm.yml | 5 +++ > .gitlab-ci.d/crossbuilds-kvm-mips.yml | 5 +++ > .gitlab-ci.d/crossbuilds-kvm-ppc.yml | 5 +++ > .gitlab-ci.d/crossbuilds-kvm-s390x.yml | 6 +++ > .gitlab-ci.d/crossbuilds-kvm-x86.yml | 6 +++ > .gitlab-ci.d/crossbuilds-xen.yml | 14 +++ Adding so many different files here is crazy IMHO, and then should all be under the same GitLab CI maintainers, not the respective arch maintainers. The MAINTAINERS file is saying who is responsible for the contents of the .yml file, not who is responsible for making sure KVM works on that arch. > .gitlab-ci.d/crossbuilds.yml | 52 -- > .gitlab-ci.yml | 6 +++ > MAINTAINERS| 6 +++ > 9 files changed, 85 insertions(+), 20 deletions(-) > create mode 100644 .gitlab-ci.d/crossbuilds-kvm-arm.yml > create mode 100644 .gitlab-ci.d/crossbuilds-kvm-mips.yml > create mode 100644 .gitlab-ci.d/crossbuilds-kvm-ppc.yml > create mode 100644 .gitlab-ci.d/crossbuilds-kvm-s390x.yml > create mode 100644 .gitlab-ci.d/crossbuilds-kvm-x86.yml > create mode 100644 .gitlab-ci.d/crossbuilds-xen.yml 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] qemu/atomic.h: prefix qemu_ to solve collisions
On Tue, Sep 22, 2020 at 08:56:06AM +0200, Paolo Bonzini wrote: > On 22/09/20 08:45, David Hildenbrand wrote: > >> It's certainly a good idea but it's quite verbose. > >> > >> What about using atomic__* as the prefix? It is not very common in QEMU > >> but there are some cases (and I cannot think of anything better). > > > > aqomic_*, lol :) > > Actually qatomic_ would be a good one, wouldn't it? Yes, I think just adding a 'q' on the front of methods is more than sufficient (see also all the qcrypto_*, qio_* APIs I wrote). The only think a plain 'q' prefix is likely to clash with is the Qt library and that isn't something we're likely to link with (famous last words...). 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 v2 26/58] xen-legacy-backend: Add missing typedef XenLegacyDevice
On Wed, Aug 19, 2020 at 08:12:04PM -0400, Eduardo Habkost wrote: > The typedef was used in the XENBACKEND_DEVICE macro, but it was > never defined. Define the typedef close to the type checking > macro. > > Signed-off-by: Eduardo Habkost > --- > Changes series v1 -> v2: new patch in series v2 > > --- > Cc: Stefano Stabellini > Cc: Anthony Perard > Cc: Paul Durrant > Cc: xen-devel@lists.xenproject.org > Cc: qemu-de...@nongnu.org > --- > include/hw/xen/xen-legacy-backend.h | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Daniel P. Berrangé 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: [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error
On Thu, Jul 16, 2020 at 02:37:04PM +0200, Philippe Mathieu-Daudé wrote:
> Let blk_attach_dev() take an Error* object to return helpful
> information. Adapt the callers.
>
> $ qemu-system-arm -M n800
> qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device 'sd-card'
> blk 'sd0' is already attached by device 'omap2-mmc'
> Drive 'sd0' is already in use because it has been automatically connected
> to another device
> (do you need 'if=none' in the drive options?)
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()")
> ---
> include/sysemu/block-backend.h | 2 +-
> block/block-backend.c| 11 ++-
> hw/block/fdc.c | 4 +---
> hw/block/swim.c | 4 +---
> hw/block/xen-block.c | 5 +++--
> hw/core/qdev-properties-system.c | 16 +---
> hw/ide/qdev.c| 4 +---
> hw/scsi/scsi-disk.c | 4 +---
> 8 files changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 8203d7f6f9..118fbad0b4 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
> void blk_iostatus_disable(BlockBackend *blk);
> void blk_iostatus_reset(BlockBackend *blk);
> void blk_iostatus_set_err(BlockBackend *blk, int error);
> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev);
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp);
> void blk_detach_dev(BlockBackend *blk, DeviceState *dev);
> DeviceState *blk_get_attached_dev(BlockBackend *blk);
> char *blk_get_attached_dev_id(BlockBackend *blk);
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 63ff940ef9..b7be0a4619 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm,
> uint64_t *shared_perm)
>
> /*
> * Attach device model @dev to @blk.
> + *
> + * @blk: Block backend
> + * @dev: Device to attach the block backend to
> + * @errp: pointer to NULL initialized error object
> + *
> * Return 0 on success, -EBUSY when a device model is attached already.
> */
> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp)
> {
> trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev)));
> if (blk->dev) {
> +error_setg(errp, "cannot attach blk '%s' to device '%s'",
> + blk_name(blk), object_get_typename(OBJECT(dev)));
> +error_append_hint(errp, "blk '%s' is already attached by device
> '%s'\n",
> + blk_name(blk),
> object_get_typename(OBJECT(blk->dev)));
I would have a preference for expanding the main error message and not
using a hint. Any hint is completely thrown away when using QMP :-(
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] trivial: Remove trailing whitespaces
On Mon, Jul 06, 2020 at 09:31:21AM -0700, no-re...@patchew.org wrote: > Patchew URL: > https://patchew.org/QEMU/20200706162300.1084753-1-dinec...@redhat.com/ > > > > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Subject: [PATCH] trivial: Remove trailing whitespaces > Type: series > Message-id: 20200706162300.1084753-1-dinec...@redhat.com > > === TEST SCRIPT BEGIN === > #!/bin/bash > git rev-parse base > /dev/null || exit 0 > git config --local diff.renamelimit 0 > git config --local diff.renames True > git config --local diff.algorithm histogram > ./scripts/checkpatch.pl --mailback base.. > === TEST SCRIPT END === > > From https://github.com/patchew-project/qemu > * [new tag] patchew/20200706162300.1084753-1-dinec...@redhat.com -> > patchew/20200706162300.1084753-1-dinec...@redhat.com > Switched to a new branch 'test' > 9af3e90 trivial: Remove trailing whitespaces > > === OUTPUT BEGIN === snip > ERROR: trailing whitespace > #2395: FILE: target/sh4/op_helper.c:149: > +^I$ One case of trailing whitespace missed. 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] trivial: Remove trailing whitespaces
On Mon, Jul 06, 2020 at 06:23:00PM +0200, Christophe de Dinechin wrote: > There are a number of unnecessary trailing whitespaces that have > accumulated over time in the source code. They cause stray changes > in patches if you use tools that automatically remove them. > > Tested by doing a `git diff -w` after the change. > > This could probably be turned into a pre-commit hook. scripts/checkpatch.pl ought to be made to check it. > > Signed-off-by: Christophe de Dinechin > --- > block/iscsi.c | 2 +- > disas/cris.c | 2 +- > disas/microblaze.c| 80 +++--- > disas/nios2.c | 256 +- > hmp-commands.hx | 2 +- > hw/alpha/typhoon.c| 6 +- > hw/arm/gumstix.c | 6 +- > hw/arm/omap1.c| 2 +- > hw/arm/stellaris.c| 2 +- > hw/char/etraxfs_ser.c | 2 +- > hw/core/ptimer.c | 2 +- > hw/cris/axis_dev88.c | 2 +- > hw/cris/boot.c| 2 +- > hw/display/qxl.c | 2 +- > hw/dma/etraxfs_dma.c | 18 +- > hw/dma/i82374.c | 2 +- > hw/i2c/bitbang_i2c.c | 2 +- > hw/input/tsc2005.c| 2 +- > hw/input/tsc210x.c| 2 +- > hw/intc/etraxfs_pic.c | 8 +- > hw/intc/sh_intc.c | 10 +- > hw/intc/xilinx_intc.c | 2 +- > hw/misc/imx25_ccm.c | 6 +- > hw/misc/imx31_ccm.c | 2 +- > hw/net/vmxnet3.h | 2 +- > hw/net/xilinx_ethlite.c | 2 +- > hw/pci/pcie.c | 2 +- > hw/sd/omap_mmc.c | 2 +- > hw/sh4/shix.c | 2 +- > hw/sparc64/sun4u.c| 2 +- > hw/timer/etraxfs_timer.c | 2 +- > hw/timer/xilinx_timer.c | 4 +- > hw/usb/hcd-musb.c | 10 +- > hw/usb/hcd-ohci.c | 6 +- > hw/usb/hcd-uhci.c | 2 +- > hw/virtio/virtio-pci.c| 2 +- > include/hw/cris/etraxfs_dma.h | 4 +- > include/hw/net/lance.h| 2 +- > include/hw/ppc/spapr.h| 2 +- > include/hw/xen/interface/io/ring.h| 34 +-- > include/qemu/log.h| 2 +- > include/qom/object.h | 4 +- > linux-user/cris/cpu_loop.c| 16 +- > linux-user/microblaze/cpu_loop.c | 16 +- > linux-user/mmap.c | 8 +- > linux-user/sparc/signal.c | 4 +- > linux-user/syscall.c | 24 +- > linux-user/syscall_defs.h | 2 +- > linux-user/uaccess.c | 2 +- > os-posix.c| 2 +- > qapi/qapi-util.c | 2 +- > qemu-img.c| 2 +- > qemu-options.hx | 26 +- > qom/object.c | 2 +- > target/cris/translate.c | 28 +- > target/cris/translate_v10.inc.c | 6 +- > target/i386/hvf/hvf.c | 4 +- > target/i386/hvf/x86.c | 4 +- > target/i386/hvf/x86_decode.c | 20 +- > target/i386/hvf/x86_decode.h | 4 +- > target/i386/hvf/x86_descr.c | 2 +- > target/i386/hvf/x86_emu.c | 2 +- > target/i386/hvf/x86_mmu.c | 6 +- > target/i386/hvf/x86_task.c| 2 +- > target/i386/hvf/x86hvf.c | 42 +-- > target/i386/translate.c | 8 +- > target/microblaze/mmu.c | 2 +- > target/microblaze/translate.c | 2 +- > target/sh4/op_helper.c| 4 +- > target/xtensa/core-de212/core-isa.h | 6 +- > .../xtensa/core-sample_controller/core-isa.h | 6 +- > target/xtensa/core-test_kc705_be/core-isa.h | 2 +- > tcg/sparc/tcg-target.inc.c| 2 +- > tcg/tcg.c | 32 +-- > tests/tcg/multiarch/test-mmap.c | 72 ++--- > ui/curses.c | 4 +- > ui/curses_keys.h | 4 +- > util/cutils.c
Re: [PATCH] autogen.sh: Restore --no-git (avoid git submodule update)
On Wed, Jun 03, 2020 at 12:31:09PM +0200, Pavel Hrdina wrote: > On Tue, Jun 02, 2020 at 04:47:45PM +0100, Ian Jackson wrote: > > Prior to 2621d48f005a "gnulib: delete all gnulib integration", > > one could pass ./autogen.sh --no-git to prevent the libvirt build > > system from running git submodule update. > > > > This feature is needed by systems like the Xen Project CI which want > > to explicitly control the revisions of every tree. These will > > typically arrange to initialise the submodules check out the right > > version of everything, and then expect the build system not to mess > > with it any more. > > To be honest I don't understand why would anyone want to keep track of > all submodules of all projects for any CI and update it manually every > time the upstream project changes these submodules. Sounds like a lot > of unnecessary work but maybe I'm missing something. Two possible scenarios that I think are valid - The CI job script does not have network access - The CI job script sees the source dir as read-only In both cases the CI harness would have to initialize the submodule before runing the CI job. I don't know if this is what Xen CI is hitting, but I think the request is reasonable none the less. Both Jenkins and GitLab CI have option to make the harness init submodules prior to the job running. > > Despite to the old documentation comments referring only to gnulib, > > the --no-git feature is required not only because of gnulib but also > > because of the other submodule, src/keycodemapdb. > > > > (And in any case, even if it were no longer required because all the > > submodules were removed, it ought ideally to have been retained as a > > no-op for compaibility reasons.) > > Well, we will break a lot more by switching to Meson build system where > everyone building libvirt will have to change their scripts as it will > not be compatible at all. Yes, but I think that's tangential, as the two above reasons will still apply, and Meson will cope with having submodules pre-initialized. 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: [Xen-devel] [PATCH v3 2/2] xen: Import other xen/io/*.h
On Fri, Jun 21, 2019 at 11:54:41AM +0100, Anthony PERARD wrote:
> A Xen public header have been imported into QEMU (by
> f65eadb639 "xen: import ring.h from xen"), but there are other header
> that depends on ring.h which come from the system when building QEMU.
>
> This patch resolves the issue of having headers from the system
> importing a different copie of ring.h.
>
> This patch is prompt by the build issue described in the previous
> patch: 'Revert xen/io/ring.h of "Clean up a few header guard symbols"'
>
> ring.h and the new imported headers are moved to
> "include/hw/xen/interface" as those describe interfaces with a guest.
>
> The imported headers are cleaned up a bit while importing them: some
> part of the file that QEMU doesn't use are removed (description
> of how to make hypercall in grant_table.h have been removed).
>
> Other cleanup:
> - xen-mapcache.c and xen-legacy-backend.c don't need grant_table.h.
> - xenfb.c doesn't need event_channel.h.
Personally I would have just kept the headers "as is" and not
changed anything. As long as the unused pieces don't actively
cause problems for the QEMU build, removing them just makes
life more complex if you periodically refresh the headers with
new copies from future Xen releases.
Not a show stopper though - your choice as maintainer, so
Reviewed-by: Daniel P. Berrangé
>
> Signed-off-by: Anthony PERARD
> ---
>
> Notes:
> v3:
> - keep original header guard
> - squashed of "xen: Fix build with public headers" and "xen: Import
> other xen/io/*.h" as this patch isn't the one that fix the build issue
> anymore.
>
> hw/9pfs/xen-9pfs.h |4 +-
> hw/block/xen_blkif.h |5 +-
> hw/char/xen_console.c|2 +-
> hw/display/xenfb.c |7 +-
> hw/net/xen_nic.c |2 +-
> hw/usb/xen-usb.c |3 +-
> hw/xen/xen-legacy-backend.c |2 -
> include/hw/xen/interface/grant_table.h | 36 +
> include/hw/xen/interface/io/blkif.h | 712 +++
> include/hw/xen/interface/io/console.h| 46 +
> include/hw/xen/interface/io/fbif.h | 156
> include/hw/xen/interface/io/kbdif.h | 566
> include/hw/xen/interface/io/netif.h | 1010 ++
> include/hw/xen/interface/io/protocols.h | 42 +
> include/hw/xen/{ => interface}/io/ring.h |0
> include/hw/xen/interface/io/usbif.h | 254 ++
> include/hw/xen/interface/io/xenbus.h | 70 ++
> include/hw/xen/xen_common.h |2 +-
> 18 files changed, 2903 insertions(+), 16 deletions(-)
> create mode 100644 include/hw/xen/interface/grant_table.h
> create mode 100644 include/hw/xen/interface/io/blkif.h
> create mode 100644 include/hw/xen/interface/io/console.h
> create mode 100644 include/hw/xen/interface/io/fbif.h
> create mode 100644 include/hw/xen/interface/io/kbdif.h
> create mode 100644 include/hw/xen/interface/io/netif.h
> create mode 100644 include/hw/xen/interface/io/protocols.h
> rename include/hw/xen/{ => interface}/io/ring.h (100%)
> create mode 100644 include/hw/xen/interface/io/usbif.h
> create mode 100644 include/hw/xen/interface/io/xenbus.h
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 :|
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/2] Revert xen/io/ring.h of "Clean up a few header guard symbols"
On Fri, Jun 21, 2019 at 11:54:40AM +0100, Anthony PERARD wrote: > This reverts changes to include/hw/xen/io/ring.h from commit > 37677d7db39a3c250ad661d00fb7c3b59d047b1f. > > Following 37677d7db3 "Clean up a few header guard symbols", QEMU start > to fail to build: > > In file included from ~/xen/tools/../tools/include/xen/io/blkif.h:31:0, > from ~/xen/tools/qemu-xen-dir/hw/block/xen_blkif.h:5, > from ~/xen/tools/qemu-xen-dir/hw/block/xen-block.c:22: > ~/xen/tools/../tools/include/xen/io/ring.h:68:0: error: "__CONST_RING_SIZE" > redefined [-Werror] > #define __CONST_RING_SIZE(_s, _sz) \ > > In file included from ~/xen/tools/qemu-xen-dir/hw/block/xen_blkif.h:4:0, > from ~/xen/tools/qemu-xen-dir/hw/block/xen-block.c:22: > ~/xen/tools/qemu-xen-dir/include/hw/xen/io/ring.h:66:0: note: this is the > location of the previous definition > #define __CONST_RING_SIZE(_s, _sz) \ > > The issue is that some public xen headers have been imported (by > f65eadb639 "xen: import ring.h from xen") but not all. With the change > in the guards symbole, the ring.h header start to be imported twice. > > Signed-off-by: Anthony PERARD > --- > CC: Markus Armbruster > --- > > Notes: > v3: > - new patch, replace "xen: Fix build with public headers" from previous > patch series version > - Revert problematic change instead. > > include/hw/xen/io/ring.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v2 1/4] xen: Fix build with public headers
On Tue, Jun 18, 2019 at 12:23:38PM +0100, Anthony PERARD wrote: > Following 37677d7db3 "Clean up a few header guard symbols", QEMU start > to fail to build: > > In file included from ~/xen/tools/../tools/include/xen/io/blkif.h:31:0, > from ~/xen/tools/qemu-xen-dir/hw/block/xen_blkif.h:5, > from ~/xen/tools/qemu-xen-dir/hw/block/xen-block.c:22: > ~/xen/tools/../tools/include/xen/io/ring.h:68:0: error: "__CONST_RING_SIZE" > redefined [-Werror] > #define __CONST_RING_SIZE(_s, _sz) \ > > In file included from ~/xen/tools/qemu-xen-dir/hw/block/xen_blkif.h:4:0, > from ~/xen/tools/qemu-xen-dir/hw/block/xen-block.c:22: > ~/xen/tools/qemu-xen-dir/include/hw/xen/io/ring.h:66:0: note: this is the > location of the previous definition > #define __CONST_RING_SIZE(_s, _sz) \ > > The issue is that some public xen headers have been imported (by > f65eadb639 "xen: import ring.h from xen") but not all. With the change > in the guards symbole, the ring.h header start to be imported twice. Ah, so the include/hw/xen/io/ring.h file in tree is a copy of /usr/include/xen/io/ring.h from xen-devel. Previously both these used "#ifndef __XEN_PUBLIC_IO_RING_H__". After the header guard cleanup in 37677d7db3, our local copy used a different header guard from the installed copy & thus we're not protected from dual inclusion. IMHO the right solutions here are either - Don't copy public Xen headers into our tree - Keep our Xen header copies identical to the originals Importing public headers and then changing them locally is the worst thing to do. With that in mind I think we should revert the part of commit 37677d7db3 that touched the imported Xen headers. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [libvirt] domXML modeling question
On Mon, Mar 04, 2019 at 04:42:32PM -0700, Jim Fehlig wrote: > Adding xen-devel to cc in case anyone there wants to comment on my latest > proposal... > > On 2/20/19 5:20 PM, Jim Fehlig wrote: > > There have been a few requests [1][2] to support Xen's max_grant_frames > > setting in libvirt domXML, but I'm not quite sure how to model it. The > > documentation [3] on this setting states: > > > > Specify the maximum number of grant frames the domain is allowed to have. > > This > > value controls how many pages the domain is able to grant access to for > > other > > domains, needed e.g. for the operation of paravirtualized devices. The > > default > > is settable via xl.conf(5). > > I've sent a patch to introduce an analogous default in the libvirt libxl > driver > > https://www.redhat.com/archives/libvir-list/2019-March/msg00123.html > > > > > It smells of a setting, e.g. the amount of memory a domain can > > share, but doesn't map to any of the existing settings. A new subelement > > doesn't feel right. Does anyone suggest a better way of > > modeling max_grant_frames? > > After discussing the max_grant_frames setting a bit more with Juergen I had > the idea to model it as IO buffer space (or DMA space) of a xenbus > "controller". All PV devices in the guest connect to the xenbus controller > and make use of the available I/O buffer space. Guests with more PV devices > requiring more buffer can increase the space on the xenbus controller > device. > > One small wrinkle in this idea is that we currently don't model xenbus in > libvirt. I'd need to add support for a new xenbus controller type and start > implicitly creating it when creating guests with PV devices, similar to > auto-creation of controllers in the qemu driver. Also, there is no existing > controller setting for specifying buffer space. Perhaps a 'ram' attribute > could be added, similar to specifying memory for devices? E.g. > > > > Any opinion on this approach? Or other ideas for modeling this setting > in libvirt? Regardless of max grant frames support I think modeling xenbus as a is a reasonable thing to want to do. I don't have a preference on whether you call it "ram" or explicitly "maxGrantFrames" as an attribute. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument
On Wed, Feb 20, 2019 at 11:53:42AM +0100, Marc-André Lureau wrote: > Hi > > On Wed, Feb 20, 2019 at 2:02 AM Philippe Mathieu-Daudé > wrote: > > > > Hi, > > > > This series convert the chardev::qemu_chr_write() to take unsigned > > length argument. To do so I went through all caller and checked if > > there are no negative value possible. > > > Changing signedness is problematic and can easily introduce bugs that > are easy to miss during review. > > I agree with Cornelia about idiomatic use of int. Changing "int" for > "size_t" isn't systematically a clear win. > > Even Google C++ style recommends to avoid unsigned types "(except for > representing bitfields or modular arithmetic). Do not use an unsigned > type merely to assert that a variable is non-negative." > https://google.github.io/styleguide/cppguide.html#Integer_Types - see > rationale > > Since Paolo you suggested the change, could you give some convincing > arguments that it's worth taking the plunge? The chardev write/read methods will end up calling libc read/write methods, whose parameters are "size_t count". Thus if there is QEMU code that could currently (mistakenly) pass a negative value for length to qemu_chr_write, unless something stops it, this is going to be cast to a size_t when we finally call read/ write on the FD, leading to a large positive value & array out of bounds read/write. IOW we already have inconsistent use of signed vs unsigned in our code which has potential to cause bugs. Converting chardev to use size_t we get rid fo the mismatch with the underlying libc APIs we call, which ultimately eliminates an area of risk longer term. There is a chance it could uncover some pre-existing dormant bugs, but provided we do due diligence to check callers I think its a win to be consistent with libc APIs in size_t usage for read/write. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v3 25/25] chardev: Let qemu_chr_write[_all] use size_t
On Wed, Feb 20, 2019 at 02:02:32AM +0100, Philippe Mathieu-Daudé wrote: > diff --git a/include/chardev/char.h b/include/chardev/char.h > index 0341dd1ba2..2e3b5a15ca 100644 > --- a/include/chardev/char.h > +++ b/include/chardev/char.h > @@ -221,7 +221,7 @@ void qemu_chr_set_feature(Chardev *chr, >ChardevFeature feature); > QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename, > bool permit_mux_mon); > -int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all); > +int qemu_chr_write(Chardev *s, const uint8_t *buf, size_t len, bool > write_all); Seeing this cleanup reminds me that I think we ought to change the chardev read & write functions to take "void *buf" instead. as is done for regular libc read/write functions. This would avoid casts in the callers between char */uint8_t * Something to think about for a future cleanup jobsame applies for the QIOChannel APIs which take a 'char *buf', annoyingly different from the chardev APIs :-( Both ought to have void *buf 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] avoid TABs in files that only contain a few
On Thu, Dec 13, 2018 at 11:37:37PM +0100, Paolo Bonzini wrote: > Most files that have TABs only contain a handful of them. Change > them to spaces so that we don't confuse people. > > disas, standard-headers, linux-headers and libdecnumber are imported > from other projects and probably should be exempted from the check. > Outside those, after this patch the following files still contain both > 8-space and TAB sequences at the beginning of the line. Many of them > have a majority of TABs, or were initially committed with all tabs. > crypto/aes.c Since you already cleaned some tabs in the previous patch, and the rest of crypto/ except desrfb.c is tab-clean, I'd like this to be fully cleaned too. > ui/vnc-enc-hextile-template.h > ui/vnc-enc-zywrle.h The VNC code was historically heavily tab-damaged and we've progressively cleaned it up when making changes. > The following have only TABs: > crypto/desrfb.c I'd rather like this to be cleaned to finish the job for crypto/. > Signed-off-by: Paolo Bonzini > --- > ui/vnc-enc-zywrle-template.c | 4 +- > ui/vnc.c | 4 +- That you've finished tab-cleaning of these files, reinforces to me that we should clean those other vnc files listed above. None the less Reviewed-by: Daniel P. Berrangé since those ones mentioned above can still be done as a separate commit to this. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [RFC 1/3] pvh: Add x86/HVM direct boot ABI header file
On Tue, Dec 11, 2018 at 02:57:29PM +, Liam Merwick wrote: > > > On 11/12/2018 14:01, Stefan Hajnoczi wrote: > > On Wed, Dec 05, 2018 at 10:37:24PM +, Liam Merwick wrote: > > > From: Liam Merwick > > > > > > The x86/HVM direct boot ABI permits Qemu to be able to boot directly > > > into the uncompressed Linux kernel binary without the need to run > > > firmware. > > > > > > https://xenbits.xen.org/docs/unstable/misc/pvh.html > > > > > > This commit adds the header file that defines the start_info struct > > > that needs to be populated in order to use this ABI. > > > > > > Signed-off-by: Maran Wilson > > > Signed-off-by: Liam Merwick > > > Reviewed-by: Konrad Rzeszutek Wilk > > > --- > > > include/hw/xen/start_info.h | 146 > > > > > > 1 file changed, 146 insertions(+) > > > create mode 100644 include/hw/xen/start_info.h > > > > Does it make sense to bring in Linux > > include/xen/interface/hvm/start_info.h via QEMU's > > include/standard-headers/? > > > > QEMU has a script in scripts/update-linux-header.sh for syncing Linux > > headers into include/standard-headers/. This makes it easy to keep > > Linux header files up-to-date. We basically treat files in > > include/standard-headers/ as auto-generated. > > > > If you define start_info.h yourself without using > > include/standard-headers/, then it won't be synced with Linux. > > > > That does seem better. I will make that change. > > One a related note, I'm trying to fix the mingw compilation errors [1] in > this series also. I can fix the format issues with PRIx64, etc but I can't > seem to find an include file to provide a declaration of mmap() et. al. - > has this been resolved before? A pointer to something similar to investigate > would be very welcome. There is no mmap() on mingw, so you'll have to make sure that code is conditionally compiled with #ifndef WIN32 where appropriate. > [1] > http://patchew.org/logs/1544049446-6359-1-git-send-email-liam.merw...@oracle.com/testing.docker-mingw@fedora/?type=message > 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 09/21] configure: use pkg-config for obtaining xen version
On Tue, Dec 11, 2018 at 10:34:52AM +, Peter Maydell wrote: > On Tue, 25 Apr 2017 at 19:35, Stefano Stabellini > wrote: > > > > From: Juergen Gross > > > > Instead of trying to guess the Xen version to use by compiling various > > test programs first just ask the system via pkg-config. Only if it > > can't return the version fall back to the test program scheme. > > > > If configure is being called with dedicated flags for the Xen libraries > > use those instead of the pkg-config output. This will avoid breaking > > an in-tree Xen build of an old Xen version while a new Xen version is > > installed on the build machine: pkg-config would pick up the installed > > Xen config files as the Xen tree wouldn't contain any of them. > > > > Signed-off-by: Juergen Gross > > Signed-off-by: Stefano Stabellini > > Tested-by: Paul Durrant > > Reviewed-by: Stefano Stabellini > > --- > > configure | 159 > > ++ > > 1 file changed, 88 insertions(+), 71 deletions(-) > > > > diff --git a/configure b/configure > > index 271bea8..3133ef8 100755 > > --- a/configure > > +++ b/configure > > @@ -1975,30 +1975,46 @@ fi > > # xen probe > > > > if test "$xen" != "no" ; then > > - xen_libs="-lxenstore -lxenctrl -lxenguest" > > - xen_stable_libs="-lxencall -lxenforeignmemory -lxengnttab -lxenevtchn" > > + # Check whether Xen library path is specified via --extra-ldflags to > > avoid > > + # overriding this setting with pkg-config output. If not, try pkg-config > > + # to obtain all needed flags. > > + > > + if ! echo $EXTRA_LDFLAGS | grep tools/libxc > /dev/null && \ > > + $pkg_config --exists xencontrol ; then > > +xen_ctrl_version="$(printf '%d%02d%02d' \ > > + $($pkg_config --modversion xencontrol | sed 's/\./ /g') )" > > +xen=yes > > +xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab" > > +xen_pc="$xen_pc xenevtchn xendevicemodel" > > +QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)" > > +libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu" > > +LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS" > > + else > > Hi -- this is an old patch, but MJT has just noticed that > it means that (assuming configure takes the "we have a pkg-config > for Xen" path) the Xen libraries get added to both libs_softmmu > and LDFLAGS, which means that everything, including the linux-user > binaries, gets linked against them. The old fallback path > only adds them to libs_softmmu. > > Juergen: is there a reason why you added the libs to both > libs_softmmu and LDFLAGS here? Can we just delete the line > that alters LDFLAGS? In the 'else' block that takes the non-pkg-config path, the libs are only added to "libs_softmmu". So I think removing LDFLAGS is right. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v2 03/18] xen: introduce 'xen-block', 'xen-disk' and 'xen-cdrom'
On Fri, Dec 07, 2018 at 03:26:01PM +, Anthony PERARD wrote:
> On Fri, Dec 07, 2018 at 02:39:40PM +, Paul Durrant wrote:
> > > -Original Message-
> > > From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> > > Sent: 07 December 2018 14:35
> > > To: Paul Durrant
> > > Cc: qemu-de...@nongnu.org; qemu-bl...@nongnu.org; xen-
> > > de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> > > ; Stefano Stabellini
> > > Subject: Re: [PATCH v2 03/18] xen: introduce 'xen-block', 'xen-disk' and
> > > 'xen-cdrom'
> > >
> > > On Thu, Dec 06, 2018 at 03:08:29PM +, Paul Durrant wrote:
> > > > +static char *disk_to_vbd_name(unsigned int disk)
> > > > +{
> > > > +char *name, *prefix = (disk >= 26) ?
> > > > +disk_to_vbd_name((disk / 26) - 1) : g_strdup("");
> > > > +
> > > > +name = g_strdup_printf("%s%c", prefix, 'a' + disk);
> > >
> > > I don't think that works, if disk is 27, we do ('a' + 27) here. It's
> > > probably missing a `disk % 26`.
> >
> > Damn, yes I was not allowing the >2 letters.
> >
> > >
> > > > +g_free(prefix);
> > > > +
> > > > +return name;
> > > > +}
> > >
> > > [...]
> > >
> > > > +static unsigned int vbd_name_to_disk(const char *name, const char
> > > **endp)
> > > > +{
> > > > +unsigned int disk = 0;
> > > > +
> > > > +while (*name != '\0') {
> > > > +if (!g_ascii_isalpha(*name) || !g_ascii_islower(*name)) {
> > > > +break;
> > > > +}
> > > > +
> > > > +disk *= 26;
> > > > +disk += *name++ - 'a';
> > > > +}
> > > > +*endp = name;
> > > > +
> > > > +return disk;
> > > > +}
> > > > +
> > > > +static void xen_block_set_vdev(Object *obj, Visitor *v, const char
> > > *name,
> > > > + void *opaque, Error **errp)
> > > > +{
> > >
> > > Setting vdev doesn't work. I've tried to add a disk `xvdaa', and it
> > > result in `xvda', or `d0p0' (in the trace). (Same result with `xvdaaa',
> > > and 'xvdba' gives 'xvdaa'/d26p0)
> > >
> >
> > Ok, that's weird. I'll have to figure that out.
>
> It's probably because 'a' is somtime 0 and sometime is 1.
>
> 'a' should be 0
> 'aa' should be 26,
> 'aaa' Seems to be 702.
>
> 'xvda': 0 -> 0 * 1
> 'xvdz': 25->25 * 1
> 'xvdaa': 26 ->1 * 26 + 0 * 1
> 'xvdaaa': 702 -> 1 * 26^2 + 1 * 26 + 0 * 1
>
> So, it's weird. Have fun fixing the algorithm for that.
Libvirt has code for going in both directions, that's LGPLv2+
licensed if you want it:
https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/virutil.c;h=279e6aedc0f5921c850130499fc95c7d4a1e34c9;hb=HEAD#l546
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 :|
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [libvirt] Likely build race, "/usr/bin/ld: cannot find -lvirt"
On Tue, Jun 12, 2018 at 04:16:53PM +0200, Jiri Denemark wrote: > On Tue, Jun 12, 2018 at 07:57:40 -0500, Eric Blake wrote: > > On 06/12/2018 06:11 AM, Jiri Denemark wrote: > > > > > I hit the same race twice on aarch64 and ppc64 and I can confirm the > > > installation phase fails if libvirt.la is installed later than libraries > > > which link to it. However, the dependencies seem to be set correctly in > > > the Makefiles. But it looks like they are only honored when linking the > > > library during the build phase. During make install libvirt.la and > > > libraries which link to it are installed independently. That is, > > > install-modLTLIBRARIES does not depend on anything except for the > > > mod_LTIBRARIES themselves. Thus when libtool decides to relink the > > > libraries libvirt.la may still be missing at this point. Manually > > > changing > > > > > > install-modLTLIBRARIES: $(mod_LTLIBRARIES) > > > > > > to > > > > > > install-modLTLIBRARIES: $(mod_LTLIBRARIES) install-libLTLIBRARIES > > > > > > fixed the problem for me (tested with an artificial delay added to > > > install-libLTLIBRARIES target), but I have no idea how to persuade > > > automake to generate something like that for us. > > > > > > Eric, is my investigation correct and do you have any ideas on how to > > > fix the race? > > > > Can you add that line directly into Makefile.am, or does doing that > > cause automake to complain and/or omit its normal rules because it > > thinks you are overriding its defaults? > > Yeah. It doesn't complain, but it omits its normal > install-modLTLIBRARIES rule which mean nothing will be installed. > However, the error is still reported so there are other libraries which > are not in mod_LTLIBRARIES affected too. What I find strange is that automake has chosen to wire up install-modLTLIBRARIES to the install-data-am target, instead of the install-exec-am target. mod_LTLIBRARIES = moddir = $(libdir)/libvirt/connection-driver ... mod_LTLIBRARIES += libvirt_driver_lxc.la I would have expected the _LTLIBRARIES suffix to cause it to be wired into the install-exec-am target > > I also tried adding install-modLTLIBRARIES-local target, but it didn't > work either since automake doesn't use this target (well I didn't really > hope it would work, but I tried it anyway). > > It's not really surprising bisecting found the following commit which > introduced the race, but I'm not really sure how to fix it. Isn't this a > bug in automake? :-) The attractive big hammer solution is to stop using libtool entirely and create shared libraries directly with gcc -shared, thus getting rid of the stupid shell wrapper scripts & relinking that libtool does > > commit 21639744f6371db0bfa1bd0d21fe5c51c6d6878a > Author: Daniel P. Berrangé > Date: Thu Jan 25 09:35:56 2018 + > > build: explicitly link all modules with libvirt.so > > The dlopened modules we currently build all use various symbols from > libvirt.so, but don't actually link to it. They rely on the libvirtd > daemon re-exporting the libvirt.so symbols. This means that at the > time the modules are linked, they contain a huge number of undefined > symbols. It also means that these undefined symbols are not versioned, > so despite us providing a LIBVIRT_PRIVATE_ version that > intentionally changes on every release, the loadable modules could > actually be loaded into any libvirtd regardless of version. > > This change explicitly links all modules against libvirt.so so > that they don't rely on the re-export behave and can be fully resolved > at build time. This will give us a stronger guarantee modules will > actually be loadable at runtime and that we're using modules from the > matched build. > > Signed-off-by: Daniel P. Berrange 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 0/3] Document intent for supported build platforms and bump min glib to 2.42
On Tue, May 08, 2018 at 03:50:49PM +0100, George Dunlap wrote: > On Fri, May 4, 2018 at 10:00 PM, Daniel P. Berrangé > wrote: > > CC'ing xen-devel in case Xen maintainers have a need for something that > > will that conflict with this proposal wrt supported build platforms. > > Thanks for the heads-up. CC'ing some more people who usually have > opinions on this sort of thing. > > > > > On Fri, May 04, 2018 at 05:00:23PM +0100, Daniel P. Berrangé wrote: > >> This short series is a followup the discussions around min glib version > >> when Olaf found we had accidentally increased the min glib by using a > >> newer function: > >> > >> https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg02699.html > >> > >> Some key points from that thread > >> > >> - Although we have a docker job that tries to test the min glib > >> version is adhered to, that's only run post-build, not by Peter's > >> merge tests, nor by patchew. > >> > >> - The docker min glib test failed to detect the problem anyway > >> because RHEL had backported the symbol in question. > >> > >> - The docker min glib test only builds with certain configure > >> options so isn't foolproof. > >> > >> - The modern distros we implicitly care about have way newer glib > >> than 2.22 > >> > >> - Peter's OS-X build host previously had 2.22, but after switching > >> from fink to homebrew now has 2.56 > >> > >> - I suggested following libvirt's lead in writing a policy for how > >> we pick supported OS targets to inform maintainers when min versions > >> can be increased. > >> > >> This series writes such a document largely based on one I wrote for > >> libvirt with a few changes, largely around OS-X and *BSD. Note it > >> is not meant to be an exhaustive list of distros we'll build on, rather > >> a representative selection, so that we can identify the range of 3rd > >> party library versions we need to care about. So if your favourite > >> distro is missing, dont be alarmed, as it probably ships similar > >> vintage software to one of those listed - if not feel free to suggest > >> additions. > >> > >> Based on that doc and https://repology.org/metapackage/glib/versions, > >> I identified that we could feasibly set min glib to 2.42. Note that > >> this would be dropping RHEL-6 as a build host (RHEL-6.0 came out in > >> 2010 so that's reasonable to drop IMHO). It would still cover 2 major > >> Debian versions and 2 most recent Ubuntu LTS (16.04, 18.04, but *not* > >> 14.04). This min glib lets us remove almost all our compat code. > >> > >> Most interestingly, thanks tothe new min version being greater than > >> 2.32, we can now use GLIB_VERSION_MAX_ALLOWED to validate the correct > >> API usage according to our min version: > >> > >> > >> https://developer.gnome.org/glib/stable/glib-Version-Information.html#GLIB-VERSION-MAX-ALLOWED:CAPS > >> > >> This means that *all* our CI jobs & developer builds will be enforcing > >> the min version, so means very many more conditionally built features > >> will get their build validated against min glib version. This would > >> do a much better job of catching mistakes than our min-glib docker > >> job, making that obsolete. > >> > >> Daniel P. Berrangé (3): > >> qemu-doc: provide details of supported build platforms > >> glib: bump min required glib library version to 2.42 > >> glib: enforce the minimum required version and warn about old APIs > > Two responses from me. > > With my Xen maintainer hat on: I wouldn't feel justified, personally, > in asking another project to continue supporting older versions. If > we didn't want to bump our own glib version, we would have to disable > "upstream" QEMU (as opposed to Xen's old qemu fork) for older versions > of glib. Or could you say that people need to use a stable version of QEMU ? eg users wanting Xen on RHEL-6, can use "upstream" QEMU, but they'll need to stick with the 2.12 stable branch version - not use git master or future releases. I guess it depends whether you can expect 2.12 QEMU to carry on working correctly with ongoing Xen changes. > > That said, when we've had similar discussions for our own project, > we've generally aimed at supporting all major currently-supported > d
Re: [Xen-devel] [PATCH 0/3] Document intent for supported build platforms and bump min glib to 2.42
CC'ing xen-devel in case Xen maintainers have a need for something that will that conflict with this proposal wrt supported build platforms. On Fri, May 04, 2018 at 05:00:23PM +0100, Daniel P. Berrangé wrote: > This short series is a followup the discussions around min glib version > when Olaf found we had accidentally increased the min glib by using a > newer function: > > https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg02699.html > > Some key points from that thread > > - Although we have a docker job that tries to test the min glib > version is adhered to, that's only run post-build, not by Peter's > merge tests, nor by patchew. > > - The docker min glib test failed to detect the problem anyway > because RHEL had backported the symbol in question. > > - The docker min glib test only builds with certain configure > options so isn't foolproof. > > - The modern distros we implicitly care about have way newer glib > than 2.22 > > - Peter's OS-X build host previously had 2.22, but after switching > from fink to homebrew now has 2.56 > > - I suggested following libvirt's lead in writing a policy for how > we pick supported OS targets to inform maintainers when min versions > can be increased. > > This series writes such a document largely based on one I wrote for > libvirt with a few changes, largely around OS-X and *BSD. Note it > is not meant to be an exhaustive list of distros we'll build on, rather > a representative selection, so that we can identify the range of 3rd > party library versions we need to care about. So if your favourite > distro is missing, dont be alarmed, as it probably ships similar > vintage software to one of those listed - if not feel free to suggest > additions. > > Based on that doc and https://repology.org/metapackage/glib/versions, > I identified that we could feasibly set min glib to 2.42. Note that > this would be dropping RHEL-6 as a build host (RHEL-6.0 came out in > 2010 so that's reasonable to drop IMHO). It would still cover 2 major > Debian versions and 2 most recent Ubuntu LTS (16.04, 18.04, but *not* > 14.04). This min glib lets us remove almost all our compat code. > > Most interestingly, thanks tothe new min version being greater than > 2.32, we can now use GLIB_VERSION_MAX_ALLOWED to validate the correct > API usage according to our min version: > > > https://developer.gnome.org/glib/stable/glib-Version-Information.html#GLIB-VERSION-MAX-ALLOWED:CAPS > > This means that *all* our CI jobs & developer builds will be enforcing > the min version, so means very many more conditionally built features > will get their build validated against min glib version. This would > do a much better job of catching mistakes than our min-glib docker > job, making that obsolete. > > Daniel P. Berrangé (3): > qemu-doc: provide details of supported build platforms > glib: bump min required glib library version to 2.42 > glib: enforce the minimum required version and warn about old APIs > > configure | 6 +- > include/glib-compat.h | 362 > ++-- > qemu-doc.texi | 68 + > tests/test-qmp-event.c | 2 +- > tests/tpm-emu.h | 4 +- > tests/vhost-user-test.c | 4 +- > trace/simple.c | 6 +- > 7 files changed, 123 insertions(+), 329 deletions(-) > > -- > 2.14.3 > 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report
On Tue, Apr 24, 2018 at 10:43:09AM -0500, Eric Blake wrote:
> On 04/24/2018 10:40 AM, Eric Blake wrote:
> > On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:
> >
> >>> - static void vreport(report_type type, const char *fmt, va_list ap)
> >>> + static void vreport(report_type type, int errnoval, const char *fmt,
> >>> va_list ap)
> >>> ...
> >>> + if (errnoval >= 0) {
> >>> + error_printf(": %s", strerror(errnoval);
> >>> + }
> >>>
> >>> and then add both
> >>> error_report_errno
> >>> error_vreport_errno
> >>> with the obvious semantics.
> >>
> >> That would be nice, because then we can make these two functions actually
> >> use strerror_r() instead of strerror(), for thread safety on all platforms.
> >
> > Except that strerror_r() is a bear to use portably, given that glibc's
> > default declaration differs from the POSIX requirement (you can force
> > glibc to give you the POSIX version, but doing so causes you to lose
> > access to many other useful extensions). It's rather telling that 'git
> > grep strerror_r' currently comes up empty.
>
> That said, glib's g_strerror() may be suitable for this purpose,
> although we are currently using it only in tests/ivshmem-test.c.
Yes, that uses strerror_r internally, or strerror_s on Windows:
https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gstrfuncs.c#L1236
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 :|
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report
On Tue, Apr 24, 2018 at 03:53:48PM +0100, Ian Jackson wrote:
> Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 15/16] os-posix:
> cleanup: Replace perror with error_report"):
> > On 04/19/2018 01:45 PM, Ian Jackson wrote:
> > > -perror("mlockall");
> > > +error_report("mlockall: %s", strerror(errno));
> > > }
> > >
> > > return ret;
> >
> > Thinking loudly, maybe we can refactor as error_report_errno(const char
> > *desc)...
>
> git-grep 'error_report.*errno' shows a lot of call sites that do
> something more exciting than const char *desc would support.
>
> I think the right approach would be
>
> - static void vreport(report_type type, const char *fmt, va_list ap)
> + static void vreport(report_type type, int errnoval, const char *fmt,
> va_list ap)
> ...
> + if (errnoval >= 0) {
> + error_printf(": %s", strerror(errnoval);
> + }
>
> and then add both
> error_report_errno
> error_vreport_errno
> with the obvious semantics.
That would be nice, because then we can make these two functions actually
use strerror_r() instead of strerror(), for thread safety on all platforms.
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 :|
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash
On Mon, Apr 23, 2018 at 05:21:42PM +0100, Anthony PERARD wrote:
> On Thu, Apr 19, 2018 at 05:45:19PM +0100, Ian Jackson wrote:
> > This makes it much easier to find a particular thing in config.log.
> >
> > The information may be lacking in other shells, resulting in harmless
> > empty output. (This is why we don't use the proper ${FUNCNAME[*]}
> > array syntax - other shells will choke on that.)
> >
> > The extra output is only printed if configure is run with bash. On
> > systems where /bin/sh is not bash, it is necessary to say bash
> > ./configure to get the extra debug info in the log.
> >
> > Signed-off-by: Ian Jackson
> > ---
> > configure | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/configure b/configure
> > index d5435ff..a4c5292 100755
> > --- a/configure
> > +++ b/configure
> > @@ -60,6 +60,10 @@ do_compiler() {
> > # is compiler binary to execute.
> > local compiler="$1"
> > shift
> > +echo >>config.log "
> > +funcs: ${FUNCNAME}
> > +lines: ${BASH_LINENO}
> > +files: ${BASH_SOURCE}"
> > echo $compiler "$@" >> config.log
> > $compiler "$@" >> config.log 2>&1 || return $?
> > # Test passed. If this is an --enable-werror build, rerun
>
> How is this usefull? All I have in my config.log is a lot of:
> funcs: do_compiler
> lines: 91
> files: ./configure
>
> And one:
> funcs: do_compiler
> lines: 95
> files: ./configure
>
> It still don't tell me which test had runned.
In autoconf, you would generally have a line output to stdout for every
test being run, as it is done so you can see immediately which test it
stopped on. QEMU's configure by comparison is completely silent, except
for the fnal summary at the end which is fine if everything works perfectly,
but not great when it doesn't.
Personally I'd suggest we add informative messages throughout the
configure script for each check being run. If people really hate the
idea of a verbose output from configure, we could leave it silent by
default and add a '--verbose' option to turn it on.
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 :|
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] make: move generated headers to qemu-build/
On Thu, Mar 22, 2018 at 09:27:55PM +0200, Michael S. Tsirkin wrote: > Make sure all generated files go into qemu-build subdirectory. > We can then include them like this: > #include "qemu-build/trace.h" > > This serves two purposes: > - make it easy to detect which files are in the source > directory (a bit more work for writers, easier for readers) > - reduce chances of conflicts with possible stale files in source > directory (which could be left over from e.g. old patches, etc) If people care about this, then they can just be doing a build with srcdir != builddir config. If people are using srcdir == builddir then they likely *want* all the generated files in their srcdir. IMHO it would be valid for us to consider if we could just mandate srcdir != builddir, but if people object to such a proposal, then I don't think we should arbitrarily move all generated source files in this way, as that's effectively the same thing forced onto devs. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] qemu: replace "" with <> in headers
On Wed, Mar 21, 2018 at 05:39:48PM +0200, Michael S. Tsirkin wrote: > On Wed, Mar 21, 2018 at 03:19:22PM +0000, Daniel P. Berrangé wrote: > > On Wed, Mar 21, 2018 at 04:46:32PM +0200, Michael S. Tsirkin wrote: > > > Our current scheme is to use > > > #include "" > > > for internal headers, and > > > #include <> > > > for external ones. > > > > > > Unfortunately this is not based on compiler support: from C point of > > > view, the "" form merely looks up headers in the current directory > > > and then falls back on <> directories. > > > > > > Thus, for example, a system header trace.h - should it be present - will > > > conflict with our local trace.h > > > > If our local "trace.h" is in the current directory, then using "" > > is right and you can still use to get the system version. > > > > If our local trace.h is in include/ top level, then it is going to > > block use of the system trace.h regardless of whether we use <> or "" > > > > Fortunately our include/ tree uses sub-dirs, so we would typically > > use #include "$subdir/trace.h" and #include would still > > find the system header. > > We just have to be careful we don't add stuff at the top level of > > our include/ dir with names that are liable to clash. This might > > suggest renaming include/elf.h to include/qemu/elf.h, or just > > moving elf.h to the qemu/ subdirectory. Likewise include/glib-compat.h > > might be better moved to qemu/ subdirectory. > > > > This is exactly what this patch proposes, with a uniform scheme: > start everything with qemu/. > > > > > > As another example of problems, a header by the same name in the source > > > directory will always be picked up first - before any headers in > > > the include directory. > > > > There's only a couple of headers in the top level of our include/ > > directory - everything else is pulled in with a named path > > eg #include "block/block_int.h", so that would not conflict with > > reference to a bare #include "block_int.h" from the current directory. > > We can not know that there are no system headers that start with block/ on > any current or future systems. Ah true, good point. I guess that's where the benefit of -iquote comes into play. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] qemu: replace "" with <> in headers
On Wed, Mar 21, 2018 at 04:46:32PM +0200, Michael S. Tsirkin wrote: > Our current scheme is to use > #include "" > for internal headers, and > #include <> > for external ones. > > Unfortunately this is not based on compiler support: from C point of > view, the "" form merely looks up headers in the current directory > and then falls back on <> directories. > > Thus, for example, a system header trace.h - should it be present - will > conflict with our local trace.h If our local "trace.h" is in the current directory, then using "" is right and you can still use to get the system version. If our local trace.h is in include/ top level, then it is going to block use of the system trace.h regardless of whether we use <> or "" Fortunately our include/ tree uses sub-dirs, so we would typically use #include "$subdir/trace.h" and #include would still find the system header. We just have to be careful we don't add stuff at the top level of our include/ dir with names that are liable to clash. This might suggest renaming include/elf.h to include/qemu/elf.h, or just moving elf.h to the qemu/ subdirectory. Likewise include/glib-compat.h might be better moved to qemu/ subdirectory. > As another example of problems, a header by the same name in the source > directory will always be picked up first - before any headers in > the include directory. There's only a couple of headers in the top level of our include/ directory - everything else is pulled in with a named path eg #include "block/block_int.h", so that would not conflict with reference to a bare #include "block_int.h" from the current directory. > Let's change the scheme: make sure all headers that are not > in the source directory are included through a path > starting with qemu/ , thus: > > #include <> > > headers in the same directory as source are included with > > #include "" > > as per standard. As stated before, I consider this a step backwards - it is a good clear standard to use "" for project local includes and <> for 3rd party / system includes IMHO. The change doesn't do anything beneficial for the two scenarios described above AFAICT. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-ppc] [PATCH] qemu: include generated files with <> and not ""
On Wed, Mar 21, 2018 at 03:08:36PM +0200, Michael S. Tsirkin wrote: > On Wed, Mar 21, 2018 at 08:16:00AM +0100, Thomas Huth wrote: > > On 20.03.2018 13:05, Michael S. Tsirkin wrote: > > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote: > > >> Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit : > > >>> QEMU coding style at the moment asks for all non-system > > >>> include files to be used with #include "foo.h". > > >>> However this rule actually does not make sense and > > >>> creates issues for when the included file is generated. > > >> > > >> If you change that, we can have issue when a system include has the same > > >> name as our local include. With "", system header are taken first. > > > > > > Are you sure? I just tested and that is not the case with > > > either gcc or clang. > > > > > >>> In C, include "file" means look in current directory, > > >>> then on include search path. Current directory here > > >>> means the source file directory. > > >>> By comparison include means look on include search path. > > >> > > >> Not exactly, there is the notion of "system header" too. > > >> > > >> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html > > >> > > >> #include > > >> This variant is used for system header files. It searches for a file > > >> named file in a standard list of system directories. You can prepend > > >> directories to this list with the -I option (see Invocation). > > > > > > This is exactly what we do. > > > > > >> #include "file" > > >> This variant is used for header files of your own program. It searches > > >> for a file named file first in the directory containing the current > > >> file, then in the quote directories and then the same directories used > > >> for . You can prepend directories to the list of quote directories > > >> with the -iquote option. > > > > > > Since we do not use -iquote, "" just adds the current directory. > > > > So why don't we simply switch to use -iquote instead of -I for adding > > search paths for our own headers? We then would get a clean separation > > of QEMU headers from system headers. > > > > Thomas > > It still leaves us with a host of problems e.g. the problem of stale > headers in the source directory. We have a patch on list which effectively solves the problem of stale generated files in source directory, so that's largely a non-issue at this point IMHO. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 05:33:42PM +0100, Stefan Weil wrote: > > Very large projects often split in sub projects, maybe one of them > describing the API. Then that API headers are similar to system headers > and can be included using <>, although they still belong to the same > larger project. Do we have a stable QEMU API described in a (small) > number of include files which typically do not change? If yes, then > those include files could be included using <> because we don't need > them in dependency lists or in static code analysis reports. QEMU doesn't have anything we'd call a stable API at the source level, anything is subject to change at any time, and often does. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 07:10:42PM +0200, Michael S. Tsirkin wrote: > On Tue, Mar 20, 2018 at 05:33:42PM +0100, Stefan Weil wrote: > > Using <> for system include files and "" for local include files is a > > convention, and as far as I know most projects adhere to that > > convention. So does QEMU currently. Such conventions are not only > > important for humans, but also for tools. There are more tools than the > > C preprocessor which handle <> and "" differently. For example the GNU > > compiler uses -MD or -MMD to automatically generate dependency rules for > > make. While -MD generates dependencies to all include files, -MMD does > > so only for user include files, but not for system include files. "user" > > and "system" means the different forms how include statements are > > written. QEMU still seems to use -MMD: > > > > rules.mak:QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d > > To my knowledge, and according to my limited testing, > system headers in this context means > the default ones not supplied with -I. GCC's definition of system header is here: https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 11:12:00AM -0500, Eric Blake wrote: > On 03/19/2018 08:54 PM, Michael S. Tsirkin wrote: > > QEMU coding style at the moment asks for all non-system > > include files to be used with #include "foo.h". > > [I'm replying without having read the rest of the thread, so bear with me if > I repeat some of the other comments that have already been made] > > And Markus even just did a cleanup along those lines. > > > However this rule actually does not make sense and > > creates issues for when the included file is generated. > > > > In C, include "file" means look in current directory, > > then on include search path. Current directory here > > means the source file directory. > > By comparison include means look on include search path. > > It's also nice when "file" means file belonging to our project, and > means 3rd-party file. So we have to choose which semantics are easier; > perhaps better Makefile rules that prevent us from seeing stale files is a > better solution than figuring out which files are generated. That's what I've attempted here: https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05421.html 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 03:50:02PM +0200, Michael S. Tsirkin wrote: > On Tue, Mar 20, 2018 at 01:41:17PM +0000, Daniel P. Berrangé wrote: > > On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote: > > > Hi, > > > > > > > > So for these, we should use "". None of these are generated files > > > > > though. > > > > > > > > That leads to crazy inconsistent message for developers where 50% of > > > > QEMU > > > > header files must use <> and the other 50% of header files must use "". > > > > > > The rules are pretty simple though: > > > > > >(1) Headers which are generated use <>. > > >(2) Headers which are in include/ use <>. > > >(3) Headers sitting in the same directory as the source files use "". > > > > We have 1200 header files in QEMU - a developer might just reasonably > > remember > > which header file is a QEMU header, vs which is a 3rd party system header. > > Expecting devs to remember which of those 3 buckets each QEMU header falls > > under is unreasonable IMHO. > > That's the C language for you though. For better or worse, these are > the rules that K&R came up with. No one seriously tries to keep compliance with original K&R C these days, things have moved on massively since then. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote: > Hi, > > > > So for these, we should use "". None of these are generated files though. > > > > That leads to crazy inconsistent message for developers where 50% of QEMU > > header files must use <> and the other 50% of header files must use "". > > The rules are pretty simple though: > >(1) Headers which are generated use <>. >(2) Headers which are in include/ use <>. >(3) Headers sitting in the same directory as the source files use "". We have 1200 header files in QEMU - a developer might just reasonably remember which header file is a QEMU header, vs which is a 3rd party system header. Expecting devs to remember which of those 3 buckets each QEMU header falls under is unreasonable IMHO. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 02:28:42PM +0200, Michael S. Tsirkin wrote: > On Tue, Mar 20, 2018 at 12:18:41PM +0000, Daniel P. Berrangé wrote: > > On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote: > > > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote: > > > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote: > > > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit : > > > > > > QEMU coding style at the moment asks for all non-system > > > > > > include files to be used with #include "foo.h". > > > > > > However this rule actually does not make sense and > > > > > > creates issues for when the included file is generated. > > > > > > > > > > If you change that, we can have issue when a system include has the > > > > > same > > > > > name as our local include. With "", system header are taken > > > > > first. > > > > > > > > > > In C, include "file" means look in current directory, > > > > > > then on include search path. Current directory here > > > > > > means the source file directory. > > > > > > By comparison include means look on include search path. > > > > > > > > > > Not exactly, there is the notion of "system header" too. > > > > > > > > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html > > > > > > > > > > #include > > > > > This variant is used for system header files. It searches for a file > > > > > named file in a standard list of system directories. You can prepend > > > > > directories to this list with the -I option (see Invocation). > > > > > > > > > > #include "file" > > > > > This variant is used for header files of your own program. It searches > > > > > for a file named file first in the directory containing the current > > > > > file, then in the quote directories and then the same directories used > > > > > for . You can prepend directories to the list of quote > > > > > directories > > > > > with the -iquote option. > > > > > > > > > > > As generated files are not in the search directory (unless the build > > > > > > directory happens to match the source directory), it does not make > > > > > > sense > > > > > > to include them with "" - doing so is merely more work for > > > > > > preprocessor > > > > > > and a source or errors if a stale file happens to exist in the > > > > > > source > > > > > > directory. > > > > > > > > > > I agree there is a problem with stale files. But linux, for instance, > > > > > asks for a "make mrproper" to avoid this. > > > > > > > > We can follow what autoconf does, and add a check to configure to see if > > > > there are generated files left in the source dir, when configuring with > > > > builddir != srcdir, and exit with error, telling user to clean their > > > > src dir first. > > > > > > > > > > This changes include directives for all generated files, across the > > > > > > tree. The idea is to avoid sending a huge amount of email. But when > > > > > > merging, the changes will be split with one commit per file, e.g. > > > > > > for > > > > > > ease of bisect in case of build failures, and to ease merging. > > > > > > > > > > > > Note that should some generated files be missed by this tree-wide > > > > > > refactoring, it isn't a big deal - this merely maintains the status > > > > > > quo, > > > > > > and this can be addressed by a separate patch on top. > > > > > > > > > > > > Signed-off-by: Michael S. Tsirkin > > > > > > > > > > I think your idea conflicts with what Markus has started to do: > > > > > > > > Yes, I don't think we should revert what Markus started. Both ways of > > > > referencing QEMU headers have downsides, but I think "..." has fewer > > > > downsides that "<">. > > > > > > Could you please explain what the advantage of "" is? > > > It seems to be gone since we moved headers away from > > > source. > > > > We moved *some* headers into the include/ directory tree. > > > > I still count 650+ headers which are alongside the .c files. > > So for these, we should use "". None of these are generated files though. That leads to crazy inconsistent message for developers where 50% of QEMU header files must use <> and the other 50% of header files must use "". Having a consistent message for developers is one of the key reasons why Markus submitted the patches to standardize on the use of "" for QEMU header files, leaving <> for system headers & external dependancies. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote: > On Tue, Mar 20, 2018 at 09:44:06AM +0000, Daniel P. Berrangé wrote: > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote: > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit : > > > > QEMU coding style at the moment asks for all non-system > > > > include files to be used with #include "foo.h". > > > > However this rule actually does not make sense and > > > > creates issues for when the included file is generated. > > > > > > If you change that, we can have issue when a system include has the same > > > name as our local include. With "", system header are taken first. > > > > > > In C, include "file" means look in current directory, > > > > then on include search path. Current directory here > > > > means the source file directory. > > > > By comparison include means look on include search path. > > > > > > Not exactly, there is the notion of "system header" too. > > > > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html > > > > > > #include > > > This variant is used for system header files. It searches for a file > > > named file in a standard list of system directories. You can prepend > > > directories to this list with the -I option (see Invocation). > > > > > > #include "file" > > > This variant is used for header files of your own program. It searches > > > for a file named file first in the directory containing the current > > > file, then in the quote directories and then the same directories used > > > for . You can prepend directories to the list of quote directories > > > with the -iquote option. > > > > > > > As generated files are not in the search directory (unless the build > > > > directory happens to match the source directory), it does not make sense > > > > to include them with "" - doing so is merely more work for preprocessor > > > > and a source or errors if a stale file happens to exist in the source > > > > directory. > > > > > > I agree there is a problem with stale files. But linux, for instance, > > > asks for a "make mrproper" to avoid this. > > > > We can follow what autoconf does, and add a check to configure to see if > > there are generated files left in the source dir, when configuring with > > builddir != srcdir, and exit with error, telling user to clean their > > src dir first. > > > > > > This changes include directives for all generated files, across the > > > > tree. The idea is to avoid sending a huge amount of email. But when > > > > merging, the changes will be split with one commit per file, e.g. for > > > > ease of bisect in case of build failures, and to ease merging. > > > > > > > > Note that should some generated files be missed by this tree-wide > > > > refactoring, it isn't a big deal - this merely maintains the status quo, > > > > and this can be addressed by a separate patch on top. > > > > > > > > Signed-off-by: Michael S. Tsirkin > > > > > > I think your idea conflicts with what Markus has started to do: > > > > Yes, I don't think we should revert what Markus started. Both ways of > > referencing QEMU headers have downsides, but I think "..." has fewer > > downsides that "<">. > > Could you please explain what the advantage of "" is? > It seems to be gone since we moved headers away from > source. We moved *some* headers into the include/ directory tree. I still count 650+ headers which are alongside the .c files. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 10:01:24AM +, Peter Maydell wrote: > On 20 March 2018 at 09:44, Daniel P. Berrangé wrote: > > We can follow what autoconf does, and add a check to configure to see if > > there are generated files left in the source dir, when configuring with > > builddir != srcdir, and exit with error, telling user to clean their > > src dir first. > > We already do this in our makefile...it just doesn't check every > single generated file. Ah yes, indeed: $ make Makefile:59: *** This is an out of tree build but your source tree (/home/berrange/src/virt/qemu) seems to have been used for an in-tree build. You can fix this by running "make distclean && rm -rf *-linux-user *-softmmu" in your source tree. Stop. It is checking for existance of config-host.mak. We have a convenient list of generated files in $(GENERATED_FILES), so I wonder if there's a practical way to check all of those too. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote: > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit : > > QEMU coding style at the moment asks for all non-system > > include files to be used with #include "foo.h". > > However this rule actually does not make sense and > > creates issues for when the included file is generated. > > If you change that, we can have issue when a system include has the same > name as our local include. With "", system header are taken first. > > In C, include "file" means look in current directory, > > then on include search path. Current directory here > > means the source file directory. > > By comparison include means look on include search path. > > Not exactly, there is the notion of "system header" too. > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html > > #include > This variant is used for system header files. It searches for a file > named file in a standard list of system directories. You can prepend > directories to this list with the -I option (see Invocation). > > #include "file" > This variant is used for header files of your own program. It searches > for a file named file first in the directory containing the current > file, then in the quote directories and then the same directories used > for . You can prepend directories to the list of quote directories > with the -iquote option. > > > As generated files are not in the search directory (unless the build > > directory happens to match the source directory), it does not make sense > > to include them with "" - doing so is merely more work for preprocessor > > and a source or errors if a stale file happens to exist in the source > > directory. > > I agree there is a problem with stale files. But linux, for instance, > asks for a "make mrproper" to avoid this. We can follow what autoconf does, and add a check to configure to see if there are generated files left in the source dir, when configuring with builddir != srcdir, and exit with error, telling user to clean their src dir first. > > This changes include directives for all generated files, across the > > tree. The idea is to avoid sending a huge amount of email. But when > > merging, the changes will be split with one commit per file, e.g. for > > ease of bisect in case of build failures, and to ease merging. > > > > Note that should some generated files be missed by this tree-wide > > refactoring, it isn't a big deal - this merely maintains the status quo, > > and this can be addressed by a separate patch on top. > > > > Signed-off-by: Michael S. Tsirkin > > I think your idea conflicts with what Markus has started to do: Yes, I don't think we should revert what Markus started. Both ways of referencing QEMU headers have downsides, but I think "..." has fewer downsides that "<">. The problem Michael is tackling should be pretty rare, because moist developers aren't frequently switching between srcdir==builddir and srcdir!=biulddir setups - they have their preference for which to use and stick with it. As long as we get ./configure to warn about the dirty srcdir it should be good enough 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [RFC PATCH 00/30] Xen Q35 Bringup patches + support for PCIe Extended Capabilities for passed through devices
On Tue, Mar 13, 2018 at 09:37:55PM +1000, Alexey G wrote: > On Tue, 13 Mar 2018 09:21:54 + > Daniel P. Berrangé wrote: > > >The subject line says to expect 30 patches, but you've only sent 18 to > >the list here. I eventually figured out that the first 12 patches were > >in Xen code and so not sent to qemu-devel. > > > >For future if you have changes that affect multiple completely separate > >projects, send them as separate series. ie just send PATCH 00/18 to > >QEMU devel so it doesn't look like a bunch of patches have gone > >missing. > > OK, we'll do for next versions. > > >> A new domain config option was implemented: device_model_machine. > >> It's a string which has following possible values: > >> - "i440" -- i440 emulation (default) > >> - "q35" -- emulate a Q35 machine. By default, the storage interface > >> is AHCI. > > > >Presumably this is mapping to the QEMU -machine arg, so it feels > >desirable to keep the same naming scheme. ie allow any of the > >versioned machine names that QEMU uses. eg any of "pc-q35-2.x" > >versioned types, or 'q35' as an alias for latest, and use > >"pc-i440fx-2.x" versioned types of 'pc' as an alias for latest, rather > >than 'i440' which is needlessly divering from the QEMU machine type. > > Yes, it is translated into the '-machine' argument. > > A direct mapping between the Xen device_model_machine option and QEMU > '-machine' argument won't be accepted by Xen maintainers I guess. > > The main problem with this approach is a requirement to have a match > between Xen/libxl and QEMU versions. If, for example, > device_model_machine tells something like "pc-q35-2.11" and later we > downgrade QEMU to some older version we'll likely have a problem > without changing anything in the domain config. So I guess the "use the > latest available" approach for machine selection (pc, q35, etc) is the > only possible option. Perhaps having the way to specify the exact QEMU > machine name and version in a separate domain config parameter (for > advanced use) might be feasible. At least with plain QEMU or KVM, using the versioned machine type names is important as that is what guarantees you a stable guest machine ABI, independant of QEMU version. If your deployment has a mixture of QEMU versions on different hosts, then you very much want to pick a versioned machine type to ensure compatibility for live migration. With libvirt we accept the short "pc" or "q35" names on input, but expand them to the fully versioned name when saving the config file, so no matter which QEMU version is used each time the guest is launched, the ABI is always the same. > > Also, parameter names do not speak for themselves I'm afraid. This way > we'll have, for example, device_model_machine="pc" vs > device_model_machine="q35"... a bit unclear I think. This may be > obvious for a QEMU user, but many Xen users didn't get used to QEMU > machines and there might be some wondering why "q35" is not "pc" and > why "pc" is an i440 system precisely. > > Another obstacle here is xen_platform_device option which indirectly > selects QEMU machine type for i440 at the moment (pc/xenfv), but this > may be addressed by controlling the Xen platform device independently > via a separate machine property or '-device xen-platform' like > Eduardo Habkost suggested. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [RFC PATCH 16/30] q35/xen: Add Xen platform device support for Q35
On Tue, Mar 13, 2018 at 04:34:01AM +1000, Alexey Gerasimenko wrote:
> Current Xen/QEMU method to control Xen Platform device on i440 is a bit
> odd -- enabling/disabling Xen platform device actually modifies the QEMU
> emulated machine type, namely xenfv <--> pc.
>
> In order to avoid multiplying machine types, use a new way to control Xen
> Platform device for QEMU -- "xen-platform-dev" machine property (bool).
> To maintain backward compatibility with existing Xen/QEMU setups, this
> is only applicable to q35 machine currently. i440 emulation still uses the
> old method (i.e. xenfv/pc machine selection) to control Xen Platform
> device, this may be changed later to xen-platform-dev property as well.
The change you made to q35 is pretty tiny, so I imagine the equiv
change to pc machine is equally small. IOW, I think you should just
convert them both straight away rather than providing an inconsistent
configuration approach for q35 vs pc.
> This way we can use a single machine type (q35) and change just
> xen-platform-dev value to on/off to control Xen platform device.
>
> Signed-off-by: Alexey Gerasimenko
> ---
> hw/core/machine.c | 21 +
> hw/i386/pc_q35.c| 14 ++
> include/hw/boards.h | 1 +
> qemu-options.hx | 1 +
> 4 files changed, 37 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 5e2bbcdace..205e7da3ce 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -290,6 +290,20 @@ static void machine_set_igd_gfx_passthru(Object *obj,
> bool value, Error **errp)
> ms->igd_gfx_passthru = value;
> }
>
> +static bool machine_get_xen_platform_dev(Object *obj, Error **errp)
> +{
> +MachineState *ms = MACHINE(obj);
> +
> +return ms->xen_platform_dev;
> +}
> +
> +static void machine_set_xen_platform_dev(Object *obj, bool value, Error
> **errp)
> +{
> +MachineState *ms = MACHINE(obj);
> +
> +ms->xen_platform_dev = value;
> +}
> +
> static char *machine_get_firmware(Object *obj, Error **errp)
> {
> MachineState *ms = MACHINE(obj);
> @@ -595,6 +609,13 @@ static void machine_class_init(ObjectClass *oc, void
> *data)
> object_class_property_set_description(oc, "igd-passthru",
> "Set on/off to enable/disable igd passthrou", &error_abort);
>
> +object_class_property_add_bool(oc, "xen-platform-dev",
> +machine_get_xen_platform_dev,
> +machine_set_xen_platform_dev, &error_abort);
> +object_class_property_set_description(oc, "xen-platform-dev",
> +"Set on/off to enable/disable Xen Platform device",
> +&error_abort);
> +
> object_class_property_add_str(oc, "firmware",
> machine_get_firmware, machine_set_firmware,
> &error_abort);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0db670f6d7..62caf924cf 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -56,6 +56,18 @@
> /* ICH9 AHCI has 6 ports */
> #define MAX_SATA_PORTS 6
>
> +static void q35_xen_hvm_init(MachineState *machine)
> +{
> +PCMachineState *pcms = PC_MACHINE(machine);
> +
> +if (xen_enabled()) {
> +/* check if Xen Platform device is enabled */
> +if (machine->xen_platform_dev) {
> +pci_create_simple(pcms->bus, -1, "xen-platform");
> +}
> +}
> +}
> +
> /* PC hardware initialisation */
> static void pc_q35_init(MachineState *machine)
> {
> @@ -207,6 +219,8 @@ static void pc_q35_init(MachineState *machine)
> if (xen_enabled()) {
> pci_bus_irqs(host_bus, xen_cmn_set_irq, xen_cmn_pci_slot_get_pirq,
> ich9_lpc, ICH9_XEN_NUM_IRQ_SOURCES);
> +
> +q35_xen_hvm_init(machine);
> } else {
> pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc_map_irq, ich9_lpc,
> ICH9_LPC_NB_PIRQS);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index efb0a9edfd..f35fc1cc03 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -238,6 +238,7 @@ struct MachineState {
> bool usb;
> bool usb_disabled;
> bool igd_gfx_passthru;
> +bool xen_platform_dev;
> char *firmware;
> bool iommu;
> bool suppress_vmdesc;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 6585058c6c..cee0b92028 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -38,6 +38,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> "dump-guest-core=on|off include guest memory in a core
> dump (default=on)\n"
> "mem-merge=on|off controls memory merge support
> (default: on)\n"
> "igd-passthru=on|off controls IGD GFX passthrough
> support (default=off)\n"
> +"xen-platform-dev=on|off controls Xen Platform device
> (default=off)\n"
> "aes-key-wrap=on|off controls support for AES key
> wrapping (default=on)\n"
> "dea-key-wrap=on|off controls support for DEA key
> wrapping (default=on)\n"
Re: [Xen-devel] [Qemu-devel] [RFC PATCH 00/30] Xen Q35 Bringup patches + support for PCIe Extended Capabilities for passed through devices
The subject line says to expect 30 patches, but you've only sent 18 to the list here. I eventually figured out that the first 12 patches were in Xen code and so not sent to qemu-devel. For future if you have changes that affect multiple completely separate projects, send them as separate series. ie just send PATCH 00/18 to QEMU devel so it doesn't look like a bunch of patches have gone missing. On Tue, Mar 13, 2018 at 04:33:45AM +1000, Alexey Gerasimenko wrote: > How to use the Q35 feature: > > A new domain config option was implemented: device_model_machine. It's > a string which has following possible values: > - "i440" -- i440 emulation (default) > - "q35" -- emulate a Q35 machine. By default, the storage interface is > AHCI. Presumably this is mapping to the QEMU -machine arg, so it feels desirable to keep the same naming scheme. ie allow any of the versioned machine names that QEMU uses. eg any of "pc-q35-2.x" versioned types, or 'q35' as an alias for latest, and use "pc-i440fx-2.x" versioned types of 'pc' as an alias for latest, rather than 'i440' which is needlessly divering from the QEMU machine type. 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 :| ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
