Re: How to run crypto benchmarks tests?

2021-01-20 Thread Philippe Mathieu-Daudé
On 1/20/21 2:58 PM, Philippe Mathieu-Daudé wrote:
> On 1/20/21 2:06 PM, Daniel P. Berrangé wrote:
>> On Wed, Jan 20, 2021 at 01:50:48PM +0100, Philippe Mathieu-Daudé wrote:
>>> Hi,
>>>
>>> Using the following "build tools and doc" config:
>>>
>>> ../configure --disable-system --disable-user

So when using --disable-system, tools have to be explicitly selected
with --enable-tools.

>> Either way, all of this is surrounded by 'if have_block' in tests/meson.build
>> which should apply if you have tools enabled or system emulators enabled.
> 
> That helped:
> 
>   block layer: NO
> 
> I'll see why the tools are not automatically selected.
> 
> Thanks,
> 
> Phil.
> 




Re: How to run crypto benchmarks tests?

2021-01-20 Thread Philippe Mathieu-Daudé
On 1/20/21 2:06 PM, Daniel P. Berrangé wrote:
> On Wed, Jan 20, 2021 at 01:50:48PM +0100, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> Using the following "build tools and doc" config:
>>
>> ../configure --disable-system --disable-user
>>  ...
>>  TLS priority: "NORMAL"
>>GNUTLS support: YES
>> libgcrypt: NO
>>nettle: YES
>>   XTS: YES
>>  libtasn1: YES
>>   PAM: YES
>>  ...
>>
>> $ make check-help
>> ...
>>  make check-speed  Run qobject speed tests
>> ...
>> Test targets:
>>   check  - Run all tests (check-help for details)
>>   bench  - Run all benchmarks
>>   docker - Help about targets running tests
>> inside containers
>>
>> $ make check-speed
>> make: *** No rule to make target 'bench-speed', needed by 'check-speed'.
>>  Stop.
>> $ make bench-speed
>> make: *** No rule to make target 'bench-speed'.  Stop.
>> $ make check-bench
>> make: *** No rule to make target 'check-bench'.  Stop.
>> $ make bench
>> make: Nothing to be done for 'bench'.
>>
>> I want to run these tests:
>>
>> $ ls -1 tests/test-crypto-*c
>> tests/test-crypto-afsplit.c
>> tests/test-crypto-block.c
>> tests/test-crypto-cipher.c
>> tests/test-crypto-hash.c
>> tests/test-crypto-hmac.c
>> tests/test-crypto-ivgen.c
>> tests/test-crypto-pbkdf.c
>> tests/test-crypto-secret.c
>> tests/test-crypto-tlscredsx509.c
>> tests/test-crypto-tlssession.c
>> tests/test-crypto-xts.c
> 
> These aren't benchmarks - they're regular unit tests - eg make check-unit 
> 
> The benchmarks are tests/benchmark-crypto*.c

Oops indeed. I want to run both to be sure.

> Either way, all of this is surrounded by 'if have_block' in tests/meson.build
> which should apply if you have tools enabled or system emulators enabled.

That helped:

  block layer: NO

I'll see why the tools are not automatically selected.

Thanks,

Phil.




How to run crypto benchmarks tests?

2021-01-20 Thread Philippe Mathieu-Daudé
Hi,

Using the following "build tools and doc" config:

../configure --disable-system --disable-user
 ...
 TLS priority: "NORMAL"
   GNUTLS support: YES
libgcrypt: NO
   nettle: YES
  XTS: YES
 libtasn1: YES
  PAM: YES
 ...

$ make check-help
...
 make check-speed  Run qobject speed tests
...
Test targets:
  check  - Run all tests (check-help for details)
  bench  - Run all benchmarks
  docker - Help about targets running tests
inside containers

$ make check-speed
make: *** No rule to make target 'bench-speed', needed by 'check-speed'.
 Stop.
$ make bench-speed
make: *** No rule to make target 'bench-speed'.  Stop.
$ make check-bench
make: *** No rule to make target 'check-bench'.  Stop.
$ make bench
make: Nothing to be done for 'bench'.

I want to run these tests:

$ ls -1 tests/test-crypto-*c
tests/test-crypto-afsplit.c
tests/test-crypto-block.c
tests/test-crypto-cipher.c
tests/test-crypto-hash.c
tests/test-crypto-hmac.c
tests/test-crypto-ivgen.c
tests/test-crypto-pbkdf.c
tests/test-crypto-secret.c
tests/test-crypto-tlscredsx509.c
tests/test-crypto-tlssession.c
tests/test-crypto-xts.c

What am I doing wrong? IIRC "make check-speed" used to work,
maybe something went wrong in commit 9ed7247a596
("meson: convert the speed tests")?

Thanks,

Phil.




Re: [PULL 3/5] linux-user: add missing IPv6 get/setsockopt option

2021-01-20 Thread Philippe Mathieu-Daudé
On 1/19/21 6:54 PM, Laurent Vivier wrote:
> From: Shu-Chun Weng 
> 
> IPV6_ADDR_PREFERENCES (RFC5014: Source address selection) was not supported.
> 
> Signed-off-by: Shu-Chun Weng 
> Reviewed-by: Laurent Vivier 
> Message-Id: <20201218193213.3566856-4-...@google.com>
> Signed-off-by: Laurent Vivier 
> ---
>  linux-user/syscall.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 969db2008104..70c61d15ebf8 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -51,6 +51,7 @@
>  #include 
>  #include 
>  //#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2272,6 +2273,7 @@ static abi_long do_setsockopt(int sockfd, int level, 
> int optname,
>  case IPV6_RECVDSTOPTS:
>  case IPV6_2292DSTOPTS:
>  case IPV6_TCLASS:
> +case IPV6_ADDR_PREFERENCES:
>  #ifdef IPV6_RECVPATHMTU
>  case IPV6_RECVPATHMTU:
>  #endif
> @@ -2926,6 +2928,7 @@ get_timeout:
>  case IPV6_RECVDSTOPTS:
>  case IPV6_2292DSTOPTS:
>  case IPV6_TCLASS:
> +case IPV6_ADDR_PREFERENCES:
>  #ifdef IPV6_RECVPATHMTU
>  case IPV6_RECVPATHMTU:
>  #endif
> 

Building on Centos7:

../linux-user/syscall.c: In function 'do_setsockopt':
../linux-user/syscall.c:2276:14: error: 'IPV6_ADDR_PREFERENCES'
undeclared (first use in this function)
 case IPV6_ADDR_PREFERENCES:
  ^
../linux-user/syscall.c:2276:14: note: each undeclared identifier is
reported only once for each function it appears in
../linux-user/syscall.c: In function 'do_getsockopt':
../linux-user/syscall.c:2931:14: error: 'IPV6_ADDR_PREFERENCES'
undeclared (first use in this function)
 case IPV6_ADDR_PREFERENCES:
  ^



Re: [PATCH] fuzz: refine the ide/ahci fuzzer configs

2021-01-20 Thread Philippe Mathieu-Daudé
On 1/20/21 7:07 AM, Alexander Bulekov wrote:
> Disks work differently depending on the x86 machine type (SATA vs PATA).
> Additionally, we should fuzz the atapi code paths, which might contain
> vulnerabilities such as CVE-2020-29443. This patch adds hard-disk and
> cdrom generic-fuzzer configs for both the pc (PATA) and q35 (SATA)
> machine types.

Yet another point for using qgraph generated configs ;)




Re: [PATCH 0/5] tcg: Dynamically allocate temporaries

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/20/21 12:06 AM, BALATON Zoltan wrote:
> On Tue, 19 Jan 2021, Richard Henderson wrote:
>> My recent change for caching tcg constants has, in a number of cases,
>> overflowed the statically allocated array of temporaries.  Change to
>> dynamic allocation.
> 
> This seems to work for me so
> 
> Tested-by: BALATON Zoltan 
> 
> but have you done any performance tests to check that this actually
> improves emulation speed? To mee it seems slower. Booting AmigaOS on
> sam460ex with c0dd6654f207 (just before your TCG series) takes:
> 
> real    0m33.829s
> user    0m34.432s
> sys    0m0.296s
> 
> but on HEAD with this series:
> 
> real    0m44.381s
> user    0m46.058s
> sys    0m0.532s
> 
> This is noticable decrease in speed also without measuring it. With just
> increasing the TCG_MAX_TEMPS to 2048 on 7c79721606be without this series
> I get:
> 
> real    0m42.681s
> user    0m44.208s
> sys    0m0.435s
> 
> So the performance regression is somewhere in the original series not in
> this fix up series.

Cc'ing Lukas for the performance part, as he is investigating
how to catch such regressions.

>> I'll note that nothing in check-acceptance triggers this overflow.
>> Anyone care to add some more test cases there?
> 
> The proposed test for the upcoming pegasos2 machine may also catch this
> (when that will be merged, its dependencies are still under review)

What are your running on pegasos2?

> or
> the sam460ex test that currently only checks the firmware could be
> enhanced to try to boot AROS if somebody wants to do that. The drawback
> is that it needs an external iso whereas the current test doesn't need
> any additional images but it did not catch problems with IRQ and neither
> this problem with TCG temps.

So this other option is not very useful, right?



Re: [RFC PATCH 2/2] hw/usb/dev-uas: Report command additional adb length as unsupported

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/18/21 6:03 PM, Philippe Mathieu-Daudé wrote:
> We are not ready to handle additional CDB data.
> 
> If a guest send a packet with such additional data,
> report the command parameter as not supported.
> 
> We can then explicit there is nothing in this additional
> buffer, by fixing its size to zero.
> 
> This fixes an error when building with Clang 11:
> 
>   usb/dev-uas.c:157:31: error: field 'status' with variable sized type 
> 'uas_iu' not at the end of a struct or class is a GNU extension 
> [-Werror,-Wgnu-variable-sized-type-not-at-end]
>   uas_iustatus;
> ^
> 
> Reported-by: Daniele Buono 

TBH this should be (chronological order):
Reported-by: Ed Maste 
Reported-by: Daniele Buono 
Reported-by: Han Han 

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Cc: Ed Maste 
> Cc: Han Han 
> Cc: Marc-André Lureau 
> Cc: Paolo Bonzini 
> Cc: Gustavo A. R. Silva 
> ---
>  hw/usb/dev-uas.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index cec071d96c4..b6434ad4b9c 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -16,6 +16,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
>  #include "qemu/module.h"
> +#include "qemu/log.h"
>  
>  #include "hw/usb.h"
>  #include "migration/vmstate.h"
> @@ -70,7 +71,7 @@ typedef struct {
>  uint8_treserved_2;
>  uint64_t   lun;
>  uint8_tcdb[16];
> -uint8_tadd_cdb[];
> +uint8_tadd_cdb[0];  /* not supported by QEMU */
>  } QEMU_PACKED  uas_iu_command;
>  
>  typedef struct {
> @@ -700,6 +701,11 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
>  uint32_t len;
>  uint16_t tag = be16_to_cpu(iu->hdr.tag);
>  
> +if (iu->command.add_cdb_length > 0) {
> +qemu_log_mask(LOG_UNIMP, "additional adb length not yet 
> supported\n");
> +goto unsupported_len;
> +}
> +
>  if (uas_using_streams(uas) && tag > UAS_MAX_STREAMS) {
>  goto invalid_tag;
>  }
> @@ -735,6 +741,10 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
>  }
>  return;
>  
> +unsupported_len:
> +usb_uas_queue_fake_sense(uas, tag, sense_code_INVALID_PARAM_VALUE);
> +return;
> +
>  invalid_tag:
>  usb_uas_queue_fake_sense(uas, tag, sense_code_INVALID_TAG);
>  return;
> 




Re: [PATCH] usb: Fix clang build

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/20/21 12:20 AM, Philippe Mathieu-Daudé wrote:
> Hi Eric,
> 
> On 1/20/21 12:07 AM, Eric Blake wrote:
>> ../hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 
>> 'uas_iu' not at the end of a struct or class is a GNU extension 
>> [-Werror,-Wgnu-variable-sized-type-not-at-end]
>> uas_iustatus;
>>   ^
>>
>> Fix this by specifying a size for the add_cdb member; and at present,
>> the code does not actually use that field other than for the size
>> chosen for the packed uas_iu_command struct, and the choice of one
>> byte does not change the size of the uas_iu union.
> 
> I sent a maybe safer approach (from the bus PoV):
> https://www.mail-archive.com/qemu-block@nongnu.org/msg79192.html
> 
> Do you mind reviewing it?
> 
>>
>> Signed-off-by: Eric Blake 
>> ---
>>
>> I'm not sure why none of our CI tools pick up this particular clang
>> build failure; I hit it on Fedora 33 when configuring to build the
>> entire tree with clang.

BTW first report is from 28 Sep 2020 (Ed):
https://www.mail-archive.com/qemu-devel@nongnu.org/msg745525.html

Then on 23 Oct 2020 (Daniele):
https://www.mail-archive.com/qemu-devel@nongnu.org/msg753674.html

Then on 10 Nov 2020 (Han):
https://www.mail-archive.com/qemu-devel@nongnu.org/msg759108.html

> Same issue after upgrading to f33. I sent a patch to bump our CI:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg774117.html
> 
> To track Fedora releases I was thinking about a gitlab job checking
> if we are using the latest, else failing; smth as:
> 
>  $ curl https://getfedora.org/ | grep -q 'Fedora 33 released'
> 




Re: [PATCH] usb: Fix clang build

2021-01-19 Thread Philippe Mathieu-Daudé
Hi Eric,

On 1/20/21 12:07 AM, Eric Blake wrote:
> ../hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 
> 'uas_iu' not at the end of a struct or class is a GNU extension 
> [-Werror,-Wgnu-variable-sized-type-not-at-end]
> uas_iustatus;
>   ^
> 
> Fix this by specifying a size for the add_cdb member; and at present,
> the code does not actually use that field other than for the size
> chosen for the packed uas_iu_command struct, and the choice of one
> byte does not change the size of the uas_iu union.

I sent a maybe safer approach (from the bus PoV):
https://www.mail-archive.com/qemu-block@nongnu.org/msg79192.html

Do you mind reviewing it?

> 
> Signed-off-by: Eric Blake 
> ---
> 
> I'm not sure why none of our CI tools pick up this particular clang
> build failure; I hit it on Fedora 33 when configuring to build the
> entire tree with clang.

Same issue after upgrading to f33. I sent a patch to bump our CI:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg774117.html

To track Fedora releases I was thinking about a gitlab job checking
if we are using the latest, else failing; smth as:

 $ curl https://getfedora.org/ | grep -q 'Fedora 33 released'




Re: [PATCH v2 08/22] tcg/mips: Split out target constraints to tcg-target-con-str.h

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/15/21 10:04 PM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson 
> ---
>  tcg/mips/tcg-target-con-str.h | 24 
>  tcg/mips/tcg-target.h |  1 +
>  tcg/mips/tcg-target.c.inc | 72 ---
>  3 files changed, 41 insertions(+), 56 deletions(-)
>  create mode 100644 tcg/mips/tcg-target-con-str.h
> 
> diff --git a/tcg/mips/tcg-target-con-str.h b/tcg/mips/tcg-target-con-str.h
> new file mode 100644
> index 00..e4b2965c72
> --- /dev/null
> +++ b/tcg/mips/tcg-target-con-str.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Define MIPS target-specific operand constraints.
> + * Copyright (c) 2021 Linaro
> + */
> +
> +/*
> + * Define constraint letters for register sets:
> + * REGS(letter, register_mask)
> + */
> +REGS('r', ALL_GENERAL_REGS)
> +REGS('L', ALL_QLOAD_REGS)
> +REGS('S', ALL_QSTORE_REGS)
> +
> +/*
> + * Define constraint letters for constants:
> + * CONST(letter, TCG_CT_CONST_* bit set)
> + */
> +CONST('I', TCG_CT_CONST_U16)
> +CONST('J', TCG_CT_CONST_S16)
> +CONST('K', TCG_CT_CONST_P2M1)
> +CONST('N', TCG_CT_CONST_N16)
> +CONST('W', TCG_CT_CONST_WSZ)
> +CONST('Z', TCG_CT_CONST_ZERO)

With the cheating comment Peter requested:
Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH v2 07/22] tcg/tci: Split out target constraints to tcg-target-con-str.h

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/15/21 10:04 PM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson 
> ---
>  tcg/tci/tcg-target-con-str.h | 11 +++
>  tcg/tci/tcg-target.h |  2 ++
>  tcg/tci/tcg-target.c.inc | 14 --
>  3 files changed, 13 insertions(+), 14 deletions(-)
>  create mode 100644 tcg/tci/tcg-target-con-str.h
> 
> diff --git a/tcg/tci/tcg-target-con-str.h b/tcg/tci/tcg-target-con-str.h
> new file mode 100644
> index 00..87c0f19e9c
> --- /dev/null
> +++ b/tcg/tci/tcg-target-con-str.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Define TCI target-specific operand constraints.
> + * Copyright (c) 2021 Linaro
> + */
> +
> +/*
> + * Define constraint letters for register sets:
> + * REGS(letter, register_mask)
> + */
> +REGS('r', MAKE_64BIT_MASK(0, TCG_TARGET_NB_REGS))
> diff --git a/tcg/tci/tcg-target.h b/tcg/tci/tcg-target.h
> index bb784e018e..ab832aecc3 100644
> --- a/tcg/tci/tcg-target.h
> +++ b/tcg/tci/tcg-target.h
> @@ -207,4 +207,6 @@ static inline void tb_target_set_jmp_target(uintptr_t 
> tc_ptr, uintptr_t jmp_rx,
>  /* no need to flush icache explicitly */
>  }
>  
> +#define TCG_TARGET_CON_STR_H
> +
>  #endif /* TCG_TARGET_H */
> diff --git a/tcg/tci/tcg-target.c.inc b/tcg/tci/tcg-target.c.inc
> index 9c45f5f88f..c913d85c37 100644
> --- a/tcg/tci/tcg-target.c.inc
> +++ b/tcg/tci/tcg-target.c.inc
> @@ -384,20 +384,6 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int 
> type,
>  return true;
>  }
>  
> -/* Parse target specific constraints. */
> -static const char *target_parse_constraint(TCGArgConstraint *ct,
> -   const char *ct_str, TCGType type)
> -{
> -    switch (*ct_str++) {
> -case 'r':
> -ct->regs = BIT(TCG_TARGET_NB_REGS) - 1;
> -break;

Easy one :)
Reviewed-by: Philippe Mathieu-Daudé 

> -default:
> -return NULL;
> -}
> -return ct_str;
> -}



Re: [PATCH 1/2] qemu/compiler: Split out qemu_build_not_reached_always

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 7:55 PM, Richard Henderson wrote:
> Provide a symbol that can always be used to signal an error,
> regardless of optimization.  Usage of this should be protected
> by e.g. __builtin_constant_p, which guards for optimization.
> 
> Signed-off-by: Richard Henderson 
> ---
>  include/qemu/compiler.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH] usb: add pcap support.

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 8:44 PM, Gerd Hoffmann wrote:
> Log all traffic of a specific usb device to a pcap file for later
> inspection.  File format is compatible with linux usb monitor.
> 
> Usage:
>   qemu -device usb-${somedevice},pcap=file.pcap
>   wireshark file.pcap

Great!

> Signed-off-by: Gerd Hoffmann 
> ---
>  include/hw/usb.h   |   8 ++
>  hw/usb/bus.c   |  16 +++
>  hw/usb/core.c  |  17 
>  hw/usb/pcap.c  | 242 +
>  hw/usb/meson.build |   1 +
>  5 files changed, 284 insertions(+)
>  create mode 100644 hw/usb/pcap.c
...

> diff --git a/hw/usb/pcap.c b/hw/usb/pcap.c
> new file mode 100644
> index ..d3162d65e5fe
> --- /dev/null
> +++ b/hw/usb/pcap.c
> @@ -0,0 +1,242 @@

Missing license.

> +#include "qemu/osdep.h"
> +#include "hw/usb.h"
> +
> +#define PCAP_MAGIC   0xa1b2c3d4
> +#define PCAP_MAJOR   2
> +#define PCAP_MINOR   4
> +
> +/* https://wiki.wireshark.org/Development/LibpcapFileFormat */
> +
> +struct pcap_hdr {
> +uint32_t magic_number;   /* magic number */
> +uint16_t version_major;  /* major version number */
> +uint16_t version_minor;  /* minor version number */
> +int32_t  thiszone;   /* GMT to local correction */
> +uint32_t sigfigs;/* accuracy of timestamps */
> +uint32_t snaplen;/* max length of captured packets, in octets */
> +uint32_t network;/* data link type */
> +};

QEMU_PACKED?

> +
> +struct pcaprec_hdr {
> +uint32_t ts_sec; /* timestamp seconds */
> +uint32_t ts_usec;/* timestamp microseconds */
> +uint32_t incl_len;   /* number of octets of packet saved in file */
> +uint32_t orig_len;   /* actual length of packet */
> +};

QEMU_PACKED?

> +
> +/* https://www.tcpdump.org/linktypes.html */
> +/* linux: Documentation/usb/usbmon.rst */
> +/* linux: drivers/usb/mon/mon_bin.c */
> +
> +#define LINKTYPE_USB_LINUX   189  /* first 48 bytes only */
> +#define LINKTYPE_USB_LINUX_MMAPPED   220  /* full 64 byte header */
> +
> +struct usbmon_packet {
> +uint64_t id; /*  0: URB ID - from submission to callback */
> +unsigned char type;  /*  8: Same as text; extensible. */
> +unsigned char xfer_type; /* ISO (0), Intr, Control, Bulk (3) */
> +unsigned char epnum; /* Endpoint number and transfer direction */
> +unsigned char devnum;/* Device address */
> +uint16_t busnum; /* 12: Bus number */
> +char flag_setup; /* 14: Same as text */
> +char flag_data;  /* 15: Same as text; Binary zero is OK. */
> +int64_t ts_sec;  /* 16: gettimeofday */
> +int32_t ts_usec; /* 24: gettimeofday */
> +int32_t status;  /* 28: */
> +unsigned int length; /* 32: Length of data (submitted or actual) */
> +unsigned int len_cap;/* 36: Delivered length */
> +union {  /* 40: */
> +unsigned char setup[8]; /* Only for Control S-type */
> +struct iso_rec {/* Only for ISO */
> +int32_t error_count;
> +int32_t numdesc;
> +} iso;
> +} s;
> +int32_t interval;/* 48: Only for Interrupt and ISO */
> +int32_t start_frame; /* 52: For ISO */
> +uint32_t xfer_flags; /* 56: copy of URB's transfer_flags */
> +uint32_t ndesc;  /* 60: Actual number of ISO descriptors */
> +};   /* 64 total length */

QEMU_PACKED?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH v1 3/6] tests/docker: make _copy_with_mkdir accept missing files

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 6:52 PM, Alex Bennée wrote:
> Depending on the linker/ldd setup we might get a file with no path.
> Typically this is the psuedo library linux-vdso.so which doesn't
> actually exist on the disk. Rather than try and catch these distro
> specific edge cases just shout about it and try and continue.
> 
> Signed-off-by: Alex Bennée 
> ---
>  tests/docker/docker.py | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 



Re: [PATCH 7/9] meson: Do not configure audio if system-mode is not selected

2021-01-19 Thread Philippe Mathieu-Daudé
Forgot to Cc Gerd, adding him.

On 1/19/21 7:50 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  configure   | 6 ++
>  meson.build | 4 +++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 9f016b06b54..a15bdfb6ef3 100755
> --- a/configure
> +++ b/configure
> @@ -2324,6 +2324,12 @@ if test -z "$want_tools"; then
>  fi
>  fi
>  
> +##
> +# Disable features only meaningful for system-mode emulation
> +if test "$softmmu" = "no"; then
> +audio_drv_list=""
> +fi
> +
>  ##
>  # Some versions of Mac OS X incorrectly define SIZE_MAX
>  cat > $TMPC << EOF
> diff --git a/meson.build b/meson.build
> index 575e34d88ac..e6c6d1518ef 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2425,7 +2425,9 @@
>  # TODO: add back version
>  summary_info += {'virgl support': config_host.has_key('CONFIG_VIRGL')}
>  summary_info += {'curl support':  curl.found()}
> -summary_info += {'Audio drivers': config_host['CONFIG_AUDIO_DRIVERS']}
> +if have_system
> +  summary_info += {'Audio drivers': config_host['CONFIG_AUDIO_DRIVERS']}
> +endif
>  summary_info += {'Block whitelist (rw)': 
> config_host['CONFIG_BDRV_RW_WHITELIST']}
>  summary_info += {'Block whitelist (ro)': 
> config_host['CONFIG_BDRV_RO_WHITELIST']}
>  summary_info += {'VirtFS support':have_virtfs}
> 




[PATCH 8/9] meson: Display if user-mode emulation is selected in summary

2021-01-19 Thread Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé 
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index e6c6d1518ef..0435bfd1c51 100644
--- a/meson.build
+++ b/meson.build
@@ -2371,6 +2371,7 @@
 endif
 
 summary_info += {'system-mode emulation': have_system}
+summary_info += {'user-mode emulation': have_user}
 summary_info += {'module support':config_host.has_key('CONFIG_MODULES')}
 if config_host.has_key('CONFIG_MODULES')
   summary_info += {'alternative module path': 
config_host.has_key('CONFIG_MODULE_UPGRADES')}
-- 
2.26.2




[PATCH 7/9] meson: Do not configure audio if system-mode is not selected

2021-01-19 Thread Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé 
---
 configure   | 6 ++
 meson.build | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 9f016b06b54..a15bdfb6ef3 100755
--- a/configure
+++ b/configure
@@ -2324,6 +2324,12 @@ if test -z "$want_tools"; then
 fi
 fi
 
+##
+# Disable features only meaningful for system-mode emulation
+if test "$softmmu" = "no"; then
+audio_drv_list=""
+fi
+
 ##
 # Some versions of Mac OS X incorrectly define SIZE_MAX
 cat > $TMPC << EOF
diff --git a/meson.build b/meson.build
index 575e34d88ac..e6c6d1518ef 100644
--- a/meson.build
+++ b/meson.build
@@ -2425,7 +2425,9 @@
 # TODO: add back version
 summary_info += {'virgl support': config_host.has_key('CONFIG_VIRGL')}
 summary_info += {'curl support':  curl.found()}
-summary_info += {'Audio drivers': config_host['CONFIG_AUDIO_DRIVERS']}
+if have_system
+  summary_info += {'Audio drivers': config_host['CONFIG_AUDIO_DRIVERS']}
+endif
 summary_info += {'Block whitelist (rw)': 
config_host['CONFIG_BDRV_RW_WHITELIST']}
 summary_info += {'Block whitelist (ro)': 
config_host['CONFIG_BDRV_RO_WHITELIST']}
 summary_info += {'VirtFS support':have_virtfs}
-- 
2.26.2




[PATCH 6/9] meson: Restrict system-mode specific accelerators

2021-01-19 Thread Philippe Mathieu-Daudé
Avoid displaying accelerators which are restricted to
system-emulation.

Signed-off-by: Philippe Mathieu-Daudé 
---
 meson.build | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/meson.build b/meson.build
index 940898eb5b4..575e34d88ac 100644
--- a/meson.build
+++ b/meson.build
@@ -2377,13 +2377,15 @@
 endif
 summary_info += {'target list':   ' '.join(target_dirs)}
 
-summary_info += {'KVM support':   config_all.has_key('CONFIG_KVM')}
-summary_info += {'HAX support':   config_all.has_key('CONFIG_HAX')}
-summary_info += {'HVF support':   config_all.has_key('CONFIG_HVF')}
-summary_info += {'WHPX support':  config_all.has_key('CONFIG_WHPX')}
-summary_info += {'Xen support':   
config_host.has_key('CONFIG_XEN_BACKEND')}
-if config_host.has_key('CONFIG_XEN_BACKEND')
-  summary_info += {'xen ctrl version':  
config_host['CONFIG_XEN_CTRL_INTERFACE_VERSION']}
+if have_system
+  summary_info += {'KVM support':   config_all.has_key('CONFIG_KVM')}
+  summary_info += {'HAX support':   config_all.has_key('CONFIG_HAX')}
+  summary_info += {'HVF support':   config_all.has_key('CONFIG_HVF')}
+  summary_info += {'WHPX support':  config_all.has_key('CONFIG_WHPX')}
+  summary_info += {'Xen support':   
config_host.has_key('CONFIG_XEN_BACKEND')}
+  if config_host.has_key('CONFIG_XEN_BACKEND')
+summary_info += {'xen ctrl version':  
config_host['CONFIG_XEN_CTRL_INTERFACE_VERSION']}
+  endif
 endif
 summary_info += {'TCG support':   config_all.has_key('CONFIG_TCG')}
 if config_all.has_key('CONFIG_TCG')
-- 
2.26.2




[PATCH 2/9] meson: Summarize compilation information altogether

2021-01-19 Thread Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé 
---
 meson.build | 53 -
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/meson.build b/meson.build
index ddac09ca159..27a2b67c42d 100644
--- a/meson.build
+++ b/meson.build
@@ -2304,6 +2304,8 @@
 summary_info += {'Documentation': build_docs}
 summary_info += {'Install blobs': get_option('install_blobs')}
 
+summary_info += {'host CPU':  cpu}
+summary_info += {'host endianness':   build_machine.endian()}
 summary_info += {'C compiler':meson.get_compiler('c').cmd_array()[0]}
 summary_info += {'Host C compiler':   meson.get_compiler('c', native: 
true).cmd_array()[0]}
 if link_language == 'cpp'
@@ -2329,6 +2331,32 @@
 endif
 summary_info += {'QEMU_CFLAGS':   config_host['QEMU_CFLAGS']}
 summary_info += {'QEMU_LDFLAGS':  config_host['QEMU_LDFLAGS']}
+summary_info += {'profiler':  config_host.has_key('CONFIG_PROFILER')}
+summary_info += {'link-time optimization (LTO)': get_option('b_lto')}
+summary_info += {'PIE':   get_option('b_pie')}
+summary_info += {'static build':  config_host.has_key('CONFIG_STATIC')}
+summary_info += {'malloc trim support': has_malloc_trim}
+summary_info += {'membarrier':config_host.has_key('CONFIG_MEMBARRIER')}
+summary_info += {'preadv support':config_host.has_key('CONFIG_PREADV')}
+summary_info += {'fdatasync': config_host.has_key('CONFIG_FDATASYNC')}
+summary_info += {'madvise':   config_host.has_key('CONFIG_MADVISE')}
+summary_info += {'posix_madvise': 
config_host.has_key('CONFIG_POSIX_MADVISE')}
+summary_info += {'posix_memalign':
config_host.has_key('CONFIG_POSIX_MEMALIGN')}
+summary_info += {'debug stack usage': 
config_host.has_key('CONFIG_DEBUG_STACK_USAGE')}
+summary_info += {'mutex debugging':   
config_host.has_key('CONFIG_DEBUG_MUTEX')}
+summary_info += {'memory allocator':  get_option('malloc')}
+summary_info += {'avx2 optimization': config_host.has_key('CONFIG_AVX2_OPT')}
+summary_info += {'avx512f optimization': 
config_host.has_key('CONFIG_AVX512F_OPT')}
+summary_info += {'gprof enabled': config_host.has_key('CONFIG_GPROF')}
+summary_info += {'gcov':  get_option('b_coverage')}
+summary_info += {'thread sanitizer':  config_host.has_key('CONFIG_TSAN')}
+summary_info += {'CFI support':   get_option('cfi')}
+if get_option('cfi')
+  summary_info += {'CFI debug support': get_option('cfi_debug')}
+endif
+summary_info += {'strip binaries':get_option('strip')}
+summary_info += {'mingw32 support':   targetos == 'windows'}
+
 summary_info += {'make':  config_host['MAKE']}
 summary_info += {'python':'@0@ (version: 
@1@)'.format(python.full_path(), python.language_version())}
 summary_info += {'sphinx-build':  sphinx_build.found()}
@@ -2342,15 +2370,8 @@
 if config_host.has_key('CONFIG_MODULES')
   summary_info += {'alternative module path': 
config_host.has_key('CONFIG_MODULE_UPGRADES')}
 endif
-summary_info += {'host CPU':  cpu}
-summary_info += {'host endianness':   build_machine.endian()}
 summary_info += {'target list':   ' '.join(target_dirs)}
-summary_info += {'gprof enabled': config_host.has_key('CONFIG_GPROF')}
 summary_info += {'sparse enabled':sparse.found()}
-summary_info += {'strip binaries':get_option('strip')}
-summary_info += {'profiler':  config_host.has_key('CONFIG_PROFILER')}
-summary_info += {'link-time optimization (LTO)': get_option('b_lto')}
-summary_info += {'static build':  config_host.has_key('CONFIG_STATIC')}
 if targetos == 'darwin'
   summary_info += {'Cocoa support':   cocoa.found()}
 endif
@@ -2382,7 +2403,6 @@
 # TODO: add back version
 summary_info += {'virgl support': config_host.has_key('CONFIG_VIRGL')}
 summary_info += {'curl support':  curl.found()}
-summary_info += {'mingw32 support':   targetos == 'windows'}
 summary_info += {'Audio drivers': config_host['CONFIG_AUDIO_DRIVERS']}
 summary_info += {'Block whitelist (rw)': 
config_host['CONFIG_BDRV_RW_WHITELIST']}
 summary_info += {'Block whitelist (ro)': 
config_host['CONFIG_BDRV_RO_WHITELIST']}
@@ -2400,7 +2420,6 @@
   summary_info += {'xen ctrl version':  
config_host['CONFIG_XEN_CTRL_INTERFACE_VERSION']}
 endif
 summary_info += {'brlapi support':brlapi.found()}
-summary_info += {'PIE':   get_option('b_pie')}
 summary_info += {'vde support':   config_host.has_key('CONFIG_VDE')}
 summary_info += {'netmap support':config_host.has_key('CONFIG_NETMAP')}
 summary_info += {'Linux AIO support': config_host.has_key('CONFIG_LINUX_AIO')}
@@ -2415,16 +2434,9 @@
   summary_info += {'TCG debug enabled': 
config_host.has_key('CONFIG_DEBUG_TCG')}
   summary_info += {'TCG interpreter':   
config_host.has_key('CONFIG_TCG_INTERPRETER')}
 endif
-summary_info += {'malloc trim support': has_malloc_trim}
 summary_info += {'RDMA support':  config_host.has_key('CONFIG_RDMA')}
 summary_info

[PATCH 3/9] meson: Summarize host binaries altogether

2021-01-19 Thread Philippe Mathieu-Daudé
Display host binaries altogether in Meson summary information.

Signed-off-by: Philippe Mathieu-Daudé 
---
 meson.build | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/meson.build b/meson.build
index 27a2b67c42d..73c8d04bbf0 100644
--- a/meson.build
+++ b/meson.build
@@ -2360,12 +2360,16 @@
 summary_info += {'make':  config_host['MAKE']}
 summary_info += {'python':'@0@ (version: 
@1@)'.format(python.full_path(), python.language_version())}
 summary_info += {'sphinx-build':  sphinx_build.found()}
+if config_host.has_key('HAVE_GDB_BIN')
+  summary_info += {'gdb': config_host['HAVE_GDB_BIN']}
+endif
 summary_info += {'genisoimage':   config_host['GENISOIMAGE']}
 # TODO: add back version
 summary_info += {'slirp support': slirp_opt == 'disabled' ? false : 
slirp_opt}
 if slirp_opt != 'disabled'
   summary_info += {'smbd':config_host['CONFIG_SMBD_COMMAND']}
 endif
+
 summary_info += {'module support':config_host.has_key('CONFIG_MODULES')}
 if config_host.has_key('CONFIG_MODULES')
   summary_info += {'alternative module path': 
config_host.has_key('CONFIG_MODULE_UPGRADES')}
@@ -2505,9 +2509,6 @@
 summary_info += {'default devices':   get_option('default_devices')}
 summary_info += {'plugin support':config_host.has_key('CONFIG_PLUGIN')}
 summary_info += {'fuzzing support':   config_host.has_key('CONFIG_FUZZ')}
-if config_host.has_key('HAVE_GDB_BIN')
-  summary_info += {'gdb': config_host['HAVE_GDB_BIN']}
-endif
 summary_info += {'rng-none':  config_host.has_key('CONFIG_RNG_NONE')}
 summary_info += {'Linux keyring': 
config_host.has_key('CONFIG_SECRET_KEYRING')}
 summary_info += {'FUSE exports':  fuse.found()}
-- 
2.26.2




[PATCH 5/9] meson: Display if system emulation is selected in summary

2021-01-19 Thread Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé 
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index d89eeb8c5ce..940898eb5b4 100644
--- a/meson.build
+++ b/meson.build
@@ -2370,6 +2370,7 @@
   summary_info += {'smbd':config_host['CONFIG_SMBD_COMMAND']}
 endif
 
+summary_info += {'system-mode emulation': have_system}
 summary_info += {'module support':config_host.has_key('CONFIG_MODULES')}
 if config_host.has_key('CONFIG_MODULES')
   summary_info += {'alternative module path': 
config_host.has_key('CONFIG_MODULE_UPGRADES')}
-- 
2.26.2




[PATCH 4/9] meson: Summarize accelerators altogether

2021-01-19 Thread Philippe Mathieu-Daudé
Display accelerators altogether in Meson summary information.

Signed-off-by: Philippe Mathieu-Daudé 
---
 meson.build | 28 +++-
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/meson.build b/meson.build
index 73c8d04bbf0..d89eeb8c5ce 100644
--- a/meson.build
+++ b/meson.build
@@ -2375,6 +2375,21 @@
   summary_info += {'alternative module path': 
config_host.has_key('CONFIG_MODULE_UPGRADES')}
 endif
 summary_info += {'target list':   ' '.join(target_dirs)}
+
+summary_info += {'KVM support':   config_all.has_key('CONFIG_KVM')}
+summary_info += {'HAX support':   config_all.has_key('CONFIG_HAX')}
+summary_info += {'HVF support':   config_all.has_key('CONFIG_HVF')}
+summary_info += {'WHPX support':  config_all.has_key('CONFIG_WHPX')}
+summary_info += {'Xen support':   
config_host.has_key('CONFIG_XEN_BACKEND')}
+if config_host.has_key('CONFIG_XEN_BACKEND')
+  summary_info += {'xen ctrl version':  
config_host['CONFIG_XEN_CTRL_INTERFACE_VERSION']}
+endif
+summary_info += {'TCG support':   config_all.has_key('CONFIG_TCG')}
+if config_all.has_key('CONFIG_TCG')
+  summary_info += {'TCG debug enabled': 
config_host.has_key('CONFIG_DEBUG_TCG')}
+  summary_info += {'TCG interpreter':   
config_host.has_key('CONFIG_TCG_INTERPRETER')}
+endif
+
 summary_info += {'sparse enabled':sparse.found()}
 if targetos == 'darwin'
   summary_info += {'Cocoa support':   cocoa.found()}
@@ -2419,25 +2434,12 @@
   summary_info += {'VNC JPEG support':  jpeg.found()}
   summary_info += {'VNC PNG support':   png.found()}
 endif
-summary_info += {'xen support':   
config_host.has_key('CONFIG_XEN_BACKEND')}
-if config_host.has_key('CONFIG_XEN_BACKEND')
-  summary_info += {'xen ctrl version':  
config_host['CONFIG_XEN_CTRL_INTERFACE_VERSION']}
-endif
 summary_info += {'brlapi support':brlapi.found()}
 summary_info += {'vde support':   config_host.has_key('CONFIG_VDE')}
 summary_info += {'netmap support':config_host.has_key('CONFIG_NETMAP')}
 summary_info += {'Linux AIO support': config_host.has_key('CONFIG_LINUX_AIO')}
 summary_info += {'Linux io_uring support': 
config_host.has_key('CONFIG_LINUX_IO_URING')}
 summary_info += {'ATTR/XATTR support': libattr.found()}
-summary_info += {'KVM support':   config_all.has_key('CONFIG_KVM')}
-summary_info += {'HAX support':   config_all.has_key('CONFIG_HAX')}
-summary_info += {'HVF support':   config_all.has_key('CONFIG_HVF')}
-summary_info += {'WHPX support':  config_all.has_key('CONFIG_WHPX')}
-summary_info += {'TCG support':   config_all.has_key('CONFIG_TCG')}
-if config_all.has_key('CONFIG_TCG')
-  summary_info += {'TCG debug enabled': 
config_host.has_key('CONFIG_DEBUG_TCG')}
-  summary_info += {'TCG interpreter':   
config_host.has_key('CONFIG_TCG_INTERPRETER')}
-endif
 summary_info += {'RDMA support':  config_host.has_key('CONFIG_RDMA')}
 summary_info += {'PVRDMA support':config_host.has_key('CONFIG_PVRDMA')}
 summary_info += {'fdt support':   fdt_opt == 'disabled' ? false : fdt_opt}
-- 
2.26.2




[PATCH 0/9] meson: Clarify summary

2021-01-19 Thread Philippe Mathieu-Daudé
Reorder stuffs in summary to quicker understand bug reports.

Remove information from deselected features when not necessary.

Philippe Mathieu-Daudé (9):
  meson: Summarize generic information first
  meson: Summarize compilation information altogether
  meson: Summarize host binaries altogether
  meson: Summarize accelerators altogether
  meson: Display if system emulation is selected in summary
  meson: Restrict system-mode specific accelerators
  meson: Do not configure audio if system-mode is not selected
  meson: Display if user-mode emulation is selected in summary
  meson: Summarize block layer information altogether

 configure   |   6 +++
 meson.build | 132 +---
 2 files changed, 81 insertions(+), 57 deletions(-)

-- 
2.26.2





[PATCH 9/9] meson: Summarize block layer information altogether

2021-01-19 Thread Philippe Mathieu-Daudé
Display if block layer is built. Do not display the block
specific information if not enabled.

Signed-off-by: Philippe Mathieu-Daudé 
---
Cc: qemu-bl...@nongnu.org
---
 meson.build | 31 ++-
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/meson.build b/meson.build
index 0435bfd1c51..dd4129ae8fd 100644
--- a/meson.build
+++ b/meson.build
@@ -2372,6 +2372,7 @@
 
 summary_info += {'system-mode emulation': have_system}
 summary_info += {'user-mode emulation': have_user}
+summary_info += {'block layer':   have_block}
 summary_info += {'module support':config_host.has_key('CONFIG_MODULES')}
 if config_host.has_key('CONFIG_MODULES')
   summary_info += {'alternative module path': 
config_host.has_key('CONFIG_MODULE_UPGRADES')}
@@ -2429,8 +2430,10 @@
 if have_system
   summary_info += {'Audio drivers': config_host['CONFIG_AUDIO_DRIVERS']}
 endif
-summary_info += {'Block whitelist (rw)': 
config_host['CONFIG_BDRV_RW_WHITELIST']}
-summary_info += {'Block whitelist (ro)': 
config_host['CONFIG_BDRV_RO_WHITELIST']}
+if have_block
+  summary_info += {'Block whitelist (rw)': 
config_host['CONFIG_BDRV_RW_WHITELIST']}
+  summary_info += {'Block whitelist (ro)': 
config_host['CONFIG_BDRV_RO_WHITELIST']}
+endif
 summary_info += {'VirtFS support':have_virtfs}
 summary_info += {'build virtiofs daemon': have_virtiofsd}
 summary_info += {'Multipath support': mpathpersist.found()}
@@ -2492,7 +2495,6 @@
 summary_info += {'TPM support':   config_host.has_key('CONFIG_TPM')}
 summary_info += {'libssh support':config_host.has_key('CONFIG_LIBSSH')}
 summary_info += {'QOM debugging': 
config_host.has_key('CONFIG_QOM_CAST_DEBUG')}
-summary_info += {'Live block migration': 
config_host.has_key('CONFIG_LIVE_BLOCK_MIGRATION')}
 summary_info += {'lzo support':   lzo.found()}
 summary_info += {'snappy support':snappy.found()}
 summary_info += {'bzip2 support': libbzip2.found()}
@@ -2500,16 +2502,19 @@
 summary_info += {'zstd support':  zstd.found()}
 summary_info += {'NUMA host support': config_host.has_key('CONFIG_NUMA')}
 summary_info += {'libxml2':   config_host.has_key('CONFIG_LIBXML2')}
-summary_info += {'replication support': 
config_host.has_key('CONFIG_REPLICATION')}
-summary_info += {'bochs support': config_host.has_key('CONFIG_BOCHS')}
-summary_info += {'cloop support': config_host.has_key('CONFIG_CLOOP')}
-summary_info += {'dmg support':   config_host.has_key('CONFIG_DMG')}
-summary_info += {'qcow v1 support':   config_host.has_key('CONFIG_QCOW1')}
-summary_info += {'vdi support':   config_host.has_key('CONFIG_VDI')}
-summary_info += {'vvfat support': config_host.has_key('CONFIG_VVFAT')}
-summary_info += {'qed support':   config_host.has_key('CONFIG_QED')}
-summary_info += {'parallels support': config_host.has_key('CONFIG_PARALLELS')}
-summary_info += {'sheepdog support':  config_host.has_key('CONFIG_SHEEPDOG')}
+if have_block
+  summary_info += {'Live block migration': 
config_host.has_key('CONFIG_LIVE_BLOCK_MIGRATION')}
+  summary_info += {'replication support': 
config_host.has_key('CONFIG_REPLICATION')}
+  summary_info += {'bochs support': config_host.has_key('CONFIG_BOCHS')}
+  summary_info += {'cloop support': config_host.has_key('CONFIG_CLOOP')}
+  summary_info += {'dmg support':   config_host.has_key('CONFIG_DMG')}
+  summary_info += {'qcow v1 support':   config_host.has_key('CONFIG_QCOW1')}
+  summary_info += {'vdi support':   config_host.has_key('CONFIG_VDI')}
+  summary_info += {'vvfat support': config_host.has_key('CONFIG_VVFAT')}
+  summary_info += {'qed support':   config_host.has_key('CONFIG_QED')}
+  summary_info += {'parallels support': 
config_host.has_key('CONFIG_PARALLELS')}
+  summary_info += {'sheepdog support':  config_host.has_key('CONFIG_SHEEPDOG')}
+endif
 summary_info += {'capstone':  capstone_opt == 'disabled' ? false : 
capstone_opt}
 summary_info += {'libpmem support':   config_host.has_key('CONFIG_LIBPMEM')}
 summary_info += {'libdaxctl support': config_host.has_key('CONFIG_LIBDAXCTL')}
-- 
2.26.2




[PATCH 1/9] meson: Summarize generic information first

2021-01-19 Thread Philippe Mathieu-Daudé
Move generic summary information on top.

Signed-off-by: Philippe Mathieu-Daudé 
---
 meson.build | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 3d889857a09..ddac09ca159 100644
--- a/meson.build
+++ b/meson.build
@@ -2301,6 +2301,9 @@
 summary_info += {'Source path':   meson.current_source_dir()}
 summary_info += {'GIT binary':config_host['GIT']}
 summary_info += {'GIT submodules':config_host['GIT_SUBMODULES']}
+summary_info += {'Documentation': build_docs}
+summary_info += {'Install blobs': get_option('install_blobs')}
+
 summary_info += {'C compiler':meson.get_compiler('c').cmd_array()[0]}
 summary_info += {'Host C compiler':   meson.get_compiler('c', native: 
true).cmd_array()[0]}
 if link_language == 'cpp'
@@ -2397,14 +2400,12 @@
   summary_info += {'xen ctrl version':  
config_host['CONFIG_XEN_CTRL_INTERFACE_VERSION']}
 endif
 summary_info += {'brlapi support':brlapi.found()}
-summary_info += {'Documentation': build_docs}
 summary_info += {'PIE':   get_option('b_pie')}
 summary_info += {'vde support':   config_host.has_key('CONFIG_VDE')}
 summary_info += {'netmap support':config_host.has_key('CONFIG_NETMAP')}
 summary_info += {'Linux AIO support': config_host.has_key('CONFIG_LINUX_AIO')}
 summary_info += {'Linux io_uring support': 
config_host.has_key('CONFIG_LINUX_IO_URING')}
 summary_info += {'ATTR/XATTR support': libattr.found()}
-summary_info += {'Install blobs': get_option('install_blobs')}
 summary_info += {'KVM support':   config_all.has_key('CONFIG_KVM')}
 summary_info += {'HAX support':   config_all.has_key('CONFIG_HAX')}
 summary_info += {'HVF support':   config_all.has_key('CONFIG_HVF')}
-- 
2.26.2




Re: [PATCH] pci: add romsize property

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 6:10 PM, Paolo Bonzini wrote:
> On 19/01/21 17:51, Philippe Mathieu-Daudé wrote:
>>> +    if (pci_dev->romsize != -1 && !is_power_of_2(pci_dev->romsize)) {
>>> +    error_setg(errp, "ROM size %d is not a power of two",
>>> pci_dev->romsize);
>>> +    return;
>>> +    }
>> Some cloud providers already complained the pow2 check in the pflash
>> device (wasting host storage). Personally I find using pow2 easier
>> and safer.
> 
> This check only applies to the value that is specified on the command
> line or in a global property, not to the file (the purpose of the
> property is exactly to override the file size, no matter if the file
> size is a power of two or not).

Doh sorry I completely misunderstood the purpose of the patch.

> Even if there is no value for the property, non-power-of-two ROMs files
> are accepted and changed into the next power of two:
> 
> pdev->romsize = pow2ceil(size);
> 
>> The pow2 check looks like a separate change however, maybe add in a
>> separate patch? Or maybe not:)
> 
> Not a separate patch for the above reason: the check is on the
> newly-introduced property.

Obviously :)

> 
> Paolo
> 




Re: [PATCH] pci: add romsize property

2021-01-19 Thread Philippe Mathieu-Daudé
+pflash

On 12/18/20 7:27 PM, Paolo Bonzini wrote:
> This property can be useful for distros to set up known-good ROM sizes for
> migration purposes.  The VM will fail to start if the ROM is too large,
> and migration compatibility will not be broken if the ROM is too small.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/pci/pci.c | 19 +--
>  hw/xen/xen_pt_load_rom.c | 14 --
>  include/hw/pci/pci.h |  1 +
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index d4349ea577..fd25253c2a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -67,6 +67,7 @@ static void pcibus_reset(BusState *qbus);
>  static Property pci_props[] = {
>  DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>  DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> +DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1),
>  DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
>  DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
>  QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> @@ -2106,6 +2107,11 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
> **errp)
>  bool is_default_rom;
>  uint16_t class_id;
>  
> +if (pci_dev->romsize != -1 && !is_power_of_2(pci_dev->romsize)) {
> +error_setg(errp, "ROM size %d is not a power of two", 
> pci_dev->romsize);
> +return;
> +}

Some cloud providers already complained the pow2 check in the pflash
device (wasting host storage). Personally I find using pow2 easier
and safer.

The pow2 check looks like a separate change however, maybe add in a
separate patch? Or maybe not :)

Regards,

Phil.




Re: [RFC PATCH 0/2] hw/usb/dev-uas: Fix Clang 11 -Wgnu-variable-sized-type-not-at-end error

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 5:41 PM, Gerd Hoffmann wrote:
> On Mon, Jan 18, 2021 at 06:03:06PM +0100, Philippe Mathieu-Daudé wrote:
>> Another attempt to fix the following Clang 11 warning:
>>
>>   usb/dev-uas.c:157:31: error: field 'status' with variable sized type 
>> 'uas_i=
>> u' not at the end of a struct or class is a GNU extension 
>> [-Werror,-Wgnu-vari=
>> able-sized-type-not-at-end]
>>   uas_iustatus;
>> ^
>> If a guest send a packet with additional data, respond
>> with "Illegal Request - parameter not supported".
> 
> Why rfc?  looks good to me as-is ;)

Sorry I forgot to explain. RFC because I don't know enough SCSI
to be sure than the device returning sense_code_INVALID_PARAM_VALUE
on the bus is appropriate. This is the best fit I could find.

> 
> thanks,
>   Gerd
> 




Re: [RFC PATCH v2 01/20] migration/vmstate: Restrict vmstate_dummy to user-mode

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 2:50 PM, Peter Maydell wrote:
> On Sun, 17 Jan 2021 at 19:24, Philippe Mathieu-Daudé  wrote:
>>
>> 'vmstate_dummy' is special and only used for user-mode. Rename
>> it to something more specific.
>> It was introduced restricted to user-mode in commit c71c3e99b8
>> ("Add a vmstate_dummy struct for CONFIG_USER_ONLY") but this
>> restriction was later removed in commit 6afc14e92ac ("migration:
>> Fix warning caused by missing declaration of vmstate_dummy").
>> Avoid the missing declaration warning by adding a stub for the
>> symbol, and restore the #ifdef'ry.
> 
> So what is the actual use of vmstate_dummy ? I had a grep
> through and as far as I can see the points where vmstate_cpu_common
> is used are all in softmmu-only code.

No clue, maybe simply remnant from unfinished work?

> I tried this patch
> and QEMU seems to compile OK:
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 140fa32a5e3..a827417a4d8 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -1131,8 +1131,6 @@ bool target_words_bigendian(void);
> 
>  #ifdef CONFIG_SOFTMMU
>  extern const VMStateDescription vmstate_cpu_common;
> -#else
> -#define vmstate_cpu_common vmstate_dummy
>  #endif
> 
>  #define VMSTATE_CPU() { \

Great! Maybe even restricting VMSTATE_CPU() to softmmu-only:

-- >8 --
@@ -1131,9 +1131,6 @@ bool target_words_bigendian(void);

 #ifdef CONFIG_SOFTMMU
 extern const VMStateDescription vmstate_cpu_common;
-#else
-#define vmstate_cpu_common vmstate_dummy
-#endif

 #define VMSTATE_CPU() {
 \
 .name = "parent_obj",
 \
@@ -1142,6 +1139,7 @@ extern const VMStateDescription vmstate_cpu_common;
 .flags = VMS_STRUCT,
 \
 .offset = 0,
 \
 }
+#endif

 #endif /* NEED_CPU_H */
---

I'll wait if David/Juan have any comment, else respin based
on your patch.

Thanks,

Phil.



Re: [PULL 05/30] Makefile: wrap ctags in quiet-command calls

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 3:27 PM, Daniel P. Berrangé wrote:
> On Tue, Jan 19, 2021 at 03:24:56PM +0100, Philippe Mathieu-Daudé wrote:
>> +Daniel
>>
>> On 1/19/21 11:00 AM, Alex Bennée wrote:
>>> Philippe Mathieu-Daudé  writes:
>>>> On Fri, Jan 15, 2021 at 2:08 PM Alex Bennée  wrote:
>>>>>
>>>>> For prettier output.
>>>>>
>>>>> Signed-off-by: Alex Bennée 
>>>>> Reviewed-by: Willian Rampazzo 
>>>>> Reviewed-by: Philippe Mathieu-Daudé 
>>>>> Message-Id: <20210114165730.31607-6-alex.ben...@linaro.org>
>>>>>
>>>>> diff --git a/Makefile b/Makefile
>>>>> index 0c509a7704..bbab640b31 100644
>>>>> --- a/Makefile
>>>>> +++ b/Makefile
>>>>> @@ -250,8 +250,13 @@ find-src-path = find "$(SRC_PATH)/" -path 
>>>>> "$(SRC_PATH)/meson" -prune -o \( -name
>>>>>
>>>>>  .PHONY: ctags
>>>>>  ctags:
>>>>> -   rm -f "$(SRC_PATH)/"tags
>>>>> -   $(find-src-path) -exec ctags -f "$(SRC_PATH)/"tags --append {} +
>>>>> +   $(call quiet-command,   \
>>>>> +   rm -f "$(SRC_PATH)/"tags,   \
>>>>> +   "CTAGS", "Remove old tags")
>>>>> +   $(call quiet-command, \
>>>>> +   $(find-src-path) -exec ctags\
>>>>> +   -f "$(SRC_PATH)/"tags --append {} +,\
>>>>> +   "CTAGS", "Re-index $(SRC_PATH)")
>>>>>
>>>>>  .PHONY: gtags
>>>>>  gtags:
>>>>> --
>>>>> 2.20.1
>>>>>
>>>>
>>>> Build now fails if ctags is not installed:
>>>>
>>>> $ if test -n "$MAKE_CHECK_ARGS"; then make -j"$JOBS" $MAKE_CHECK_ARGS ; fi
>>>> CTAGS Remove old tags
>>>> CTAGS Re-index /builds/philmd/qemu
>>>> find: 'ctags': No such file or directory
>>>> find: 'ctags': No such file or directory
>>>> find: 'ctags': No such file or directory
>>>> make: *** [Makefile:254: ctags] Error 1
>>>> make: *** Waiting for unfinished jobs
>>>
>>> Wait what, how? Have you got ctags in your MAKE_CHECK_ARGS? How did it
>>> not fail before?
>>>
>>> I suppose we could add checks for all the tooling in meson but it seems
>>> a little overkill for a developer convenience.  
>>
>> I figured out I was still using the Docker images generated after
>> testing Daniel's 'start using libvirt-ci's "lcitool" for dockerfiles'
>> series:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg772839.html
>>
>> Daniel, lcitool's ansible/vars/projects/qemu.yml apparently has
>> a strong dependency on ctags.
> 
> Huh ?  It doesn't mention ctags at all.  If it is present, it is only as
> a side-effect of a dependancy from some other package we genuinely do need.

Sorry, I meant QEMU as a strong dependency on ctags.

I'll send a patch to lcitool.

Regards,

Phil.




Re: [RFC PATCH] tests/docker: Allow passing --network option when building images

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 3:20 PM, Daniel P. Berrangé wrote:
> On Tue, Jan 19, 2021 at 02:40:13PM +0100, Philippe Mathieu-Daudé wrote:
>> On 1/19/21 12:27 PM, Alex Bennée wrote:
>>> Philippe Mathieu-Daudé  writes:
>>>
>>>> When using the Docker engine, build fails because the container is
>>>> unable to resolve hostnames:
>>>>
>>>>   $ make docker-image-debian-s390x-cross NETWORK=host ENGINE=docker
>>>> BUILD   debian10
>>>>   #6 9.679 Err:1 http://deb.debian.org/debian buster InRelease
>>>>   #6 9.679   Temporary failure resolving 'deb.debian.org'
>>>>   #6 16.69 Err:2 http://security.debian.org/debian-security buster/updates 
>>>> InRelease
>>>>   #6 16.69   Temporary failure resolving 'security.debian.org'
>>>>   #6 22.69 Err:3 http://deb.debian.org/debian buster-updates InRelease
>>>>   #6 22.69   Temporary failure resolving 'deb.debian.org'
>>>>   #6 22.74 W: Failed to fetch 
>>>> http://deb.debian.org/debian/dists/buster/InRelease  Temporary failure 
>>>> resolving 'deb.debian.org'
>>>>   #6 22.74 W: Failed to fetch 
>>>> http://security.debian.org/debian-security/dists/buster/updates/InRelease  
>>>> Temporary failure resolving 'security.debian.org'
>>>>   #6 22.74 W: Failed to fetch 
>>>> http://deb.debian.org/debian/dists/buster-updates/InRelease  Temporary 
>>>> failure resolving 'deb.debian.org'
>>>>   #6 22.74 W: Some index files failed to download. They have been
>>>>   ignored, or old ones used instead.
>>>
>>> I'm confused by this one as it currently works for me. That said I
>>> thought the actual behaviour was meant to be networking is enabled by
>>> default and explicitly disabled by the run step (which shouldn't be
>>> pulling extra stuff down).
>>>
>>> This was last tweaked by Daniel in 8a2390a4f47
>>>
>>> Have the defaults for docker engine changed?
>>
>> No idea as I'm not following their development, but TBH it
>> becomes harder and harder to use without tricks (I had to
>> add systemd.unified_cgroup_hierarchy=0 to kernel cmdline
>> to keep using it).
>>
>> Maybe I should switch to podman which is the recommended
>> engine on Fedora.
>>
>> Cc'ing Marc-André who added podman support (Daniel is in Cc).
> 
> I'm using podman exclusively since Docker doesn't work well with
> modern distros that use Cgroups v2.

OK this probably explains it.

Ideally we could add a check for this ("modern distro" -> docker
engine not recommended) but I guess I'm the only one using this
feature on Fedora (as nobody complained) so not a problem. I'll
see how to use podman.

Thanks,

Phil.



Re: [PULL 05/30] Makefile: wrap ctags in quiet-command calls

2021-01-19 Thread Philippe Mathieu-Daudé
+Daniel

On 1/19/21 11:00 AM, Alex Bennée wrote:
> Philippe Mathieu-Daudé  writes:
>> On Fri, Jan 15, 2021 at 2:08 PM Alex Bennée  wrote:
>>>
>>> For prettier output.
>>>
>>> Signed-off-by: Alex Bennée 
>>> Reviewed-by: Willian Rampazzo 
>>> Reviewed-by: Philippe Mathieu-Daudé 
>>> Message-Id: <20210114165730.31607-6-alex.ben...@linaro.org>
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 0c509a7704..bbab640b31 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -250,8 +250,13 @@ find-src-path = find "$(SRC_PATH)/" -path 
>>> "$(SRC_PATH)/meson" -prune -o \( -name
>>>
>>>  .PHONY: ctags
>>>  ctags:
>>> -   rm -f "$(SRC_PATH)/"tags
>>> -   $(find-src-path) -exec ctags -f "$(SRC_PATH)/"tags --append {} +
>>> +   $(call quiet-command,   \
>>> +   rm -f "$(SRC_PATH)/"tags,   \
>>> +   "CTAGS", "Remove old tags")
>>> +   $(call quiet-command, \
>>> +   $(find-src-path) -exec ctags\
>>> +   -f "$(SRC_PATH)/"tags --append {} +,\
>>> +   "CTAGS", "Re-index $(SRC_PATH)")
>>>
>>>  .PHONY: gtags
>>>  gtags:
>>> --
>>> 2.20.1
>>>
>>
>> Build now fails if ctags is not installed:
>>
>> $ if test -n "$MAKE_CHECK_ARGS"; then make -j"$JOBS" $MAKE_CHECK_ARGS ; fi
>> CTAGS Remove old tags
>> CTAGS Re-index /builds/philmd/qemu
>> find: 'ctags': No such file or directory
>> find: 'ctags': No such file or directory
>> find: 'ctags': No such file or directory
>> make: *** [Makefile:254: ctags] Error 1
>> make: *** Waiting for unfinished jobs
> 
> Wait what, how? Have you got ctags in your MAKE_CHECK_ARGS? How did it
> not fail before?
> 
> I suppose we could add checks for all the tooling in meson but it seems
> a little overkill for a developer convenience.  

I figured out I was still using the Docker images generated after
testing Daniel's 'start using libvirt-ci's "lcitool" for dockerfiles'
series:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg772839.html

Daniel, lcitool's ansible/vars/projects/qemu.yml apparently has
a strong dependency on ctags.

Regards,

Phil.




Re: [PATCH] tests/check-block.sh: Refuse to run the iotests with BusyBox' sed

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 2:47 PM, Thomas Huth wrote:
> BusyBox' sed reports itself as "This is not GNU sed version 4.0"
> when being run with the --version parameter. However, the iotests
> really need GNU sed, they do not work with the BusyBox version.
> So let's make sure that we really have GNU sed and refuse to run
> the tests with BusyBox' sed.
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/check-block.sh | 7 +++
>  1 file changed, 7 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH] ide: set an upper limit to nb_sectors

2021-01-19 Thread Philippe Mathieu-Daudé
Hi Prasad,

On 1/19/21 2:42 PM, P J P wrote:
> From: Prasad J Pandit 
> 
> Set an upper limit to number of sectors on an IDE disk media.
> This is to ensure that logical block addresses (LBA) and
> nb_sector arguments remain within INT_MAX range.
> 
> Suggested-by: Paolo Bonzini 
> Signed-off-by: Prasad J Pandit 
> ---
>  hw/ide/core.c | 23 ---
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> Update: limit s->nb_sectors count
>   -> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04270.html
>   -> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04173.html
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index b49e4cfbc6..064998804a 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1161,15 +1161,21 @@ static void ide_cfata_metadata_write(IDEState *s)
>  s->nsector << 9), 0x200 - 2));
>  }
> 
> +static void ide_set_nb_sectors(IDEState *s)
> +{
> +uint64_t nb_sectors;
> +
> +blk_get_geometry(s->blk, _sectors);

   /* An explanation here would be useful */

or better, add a self-explaining definition for this magic value.

> +s->nb_sectors = MIN(nb_sectors, (uint64_t)INT_MAX << 2);
> +}




Re: [PATCH] hw/usb: Convert to qdev_realize()

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 1:01 PM, Markus Armbruster wrote:
> Device code shouldn't mess with QOM property "realized" since we have
> proper interfaces (merge commit 6675a653).  Commit 8ddab8dd3d
> "usb/hcd-xhci: Split pci wrapper for xhci base model" and commit
> f00ff136ee "usb: hcd-xhci-sysbus: Attach xhci to sysbus device"
> reintroduced two instances.  Clean them up.  Note that s->xhci is
> a (bus-less) TYPE_XHCI device.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/usb/hcd-xhci-pci.c| 4 +---
>  hw/usb/hcd-xhci-sysbus.c | 5 +
>  2 files changed, 2 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 




Re: [RFC PATCH] tests/docker: Allow passing --network option when building images

2021-01-19 Thread Philippe Mathieu-Daudé
On 1/19/21 12:27 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé  writes:
> 
>> When using the Docker engine, build fails because the container is
>> unable to resolve hostnames:
>>
>>   $ make docker-image-debian-s390x-cross NETWORK=host ENGINE=docker
>> BUILD   debian10
>>   #6 9.679 Err:1 http://deb.debian.org/debian buster InRelease
>>   #6 9.679   Temporary failure resolving 'deb.debian.org'
>>   #6 16.69 Err:2 http://security.debian.org/debian-security buster/updates 
>> InRelease
>>   #6 16.69   Temporary failure resolving 'security.debian.org'
>>   #6 22.69 Err:3 http://deb.debian.org/debian buster-updates InRelease
>>   #6 22.69   Temporary failure resolving 'deb.debian.org'
>>   #6 22.74 W: Failed to fetch 
>> http://deb.debian.org/debian/dists/buster/InRelease  Temporary failure 
>> resolving 'deb.debian.org'
>>   #6 22.74 W: Failed to fetch 
>> http://security.debian.org/debian-security/dists/buster/updates/InRelease  
>> Temporary failure resolving 'security.debian.org'
>>   #6 22.74 W: Failed to fetch 
>> http://deb.debian.org/debian/dists/buster-updates/InRelease  Temporary 
>> failure resolving 'deb.debian.org'
>>   #6 22.74 W: Some index files failed to download. They have been
>>   ignored, or old ones used instead.
> 
> I'm confused by this one as it currently works for me. That said I
> thought the actual behaviour was meant to be networking is enabled by
> default and explicitly disabled by the run step (which shouldn't be
> pulling extra stuff down).
> 
> This was last tweaked by Daniel in 8a2390a4f47
> 
> Have the defaults for docker engine changed?

No idea as I'm not following their development, but TBH it
becomes harder and harder to use without tricks (I had to
add systemd.unified_cgroup_hierarchy=0 to kernel cmdline
to keep using it).

Maybe I should switch to podman which is the recommended
engine on Fedora.

Cc'ing Marc-André who added podman support (Daniel is in Cc).

> 
>>   Traceback (most recent call last):
>> File "./tests/docker/docker.py", line 709, in 
>>   sys.exit(main())
>> File "./tests/docker/docker.py", line 705, in main
>>   return args.cmdobj.run(args, argv)
>> File "./tests/docker/docker.py", line 498, in run
>>   dkr.build_image(tag, docker_dir, dockerfile,
>> File "./tests/docker/docker.py", line 353, in build_image
>>   self._do_check(build_args,
>> File "./tests/docker/docker.py", line 244, in _do_check
>>   return subprocess.check_call(self._command + cmd, **kwargs)
>>     File "/usr/lib64/python3.8/subprocess.py", line 364, in check_call
>>   raise CalledProcessError(retcode, cmd)
>>   make: *** [tests/docker/Makefile.include:61: docker-image-debian10] Error 1
>>
>> Fix by passing the NETWORK variable with --network= argument.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  tests/docker/Makefile.include | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
>> index bdc53ddfcf9..b65fd684011 100644
>> --- a/tests/docker/Makefile.include
>> +++ b/tests/docker/Makefile.include
>> @@ -63,6 +63,7 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
>>  $(if $V,,--quiet) \
>>  $(if $(NOCACHE),--no-cache, \
>>  $(if $(DOCKER_REGISTRY),--registry $(DOCKER_REGISTRY))) 
>> \
>> +$(if $(NETWORK),$(if $(subst
>> $(NETWORK),,1),--network=$(NETWORK))) \
> 
> which if it has we'll need to tweak both build and run steps?

I suppose you need need networking for git (submodule) clone?

Personally I don't require networking when running because I
share my QEMU source directory as a volume, so it never bothered
me.

> 
>>  $(if $(NOUSER),,--add-current-user) \
>>  $(if $(EXTRA_FILES),--extra-files $(EXTRA_FILES))\
>>  $(if $(EXECUTABLE),--include-executable=$(EXECUTABLE)),\
> 
> 



Re: [PATCH] tests/acceptance: Test PMON with Loongson-3A1000 CPU

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/19/21 7:59 AM, Jiaxun Yang wrote:
> On Tue, Jan 19, 2021, at 1:57 PM, Philippe Mathieu-Daudé wrote:
>> On 1/18/21 5:54 PM, Alex Bennée wrote:
>>> Philippe Mathieu-Daudé  writes:
>>>> On 1/12/21 3:07 AM, Jiaxun Yang wrote:
>>>>> Test booting of PMON bootloader on loongson3-virt platform.
>>>>>
>>>>> $ (venv) AVOCADO_ALLOW_UNTRUSTED_CODE=1 \
>>>>> avocado --show=app,console \
>>>>>   run -t machine:loongson3-virt tests/acceptance
>>>>> Fetching asset from 
>>>>> tests/acceptance/machine_mips_loongson3v.py:MipsLoongson3v.test_pmon_serial_console
>>>>> JOB ID : 8e202b3727847c9104d0d3d6546ed225d35f6706
>>>>> JOB LOG: 
>>>>> /home/flygoat/avocado/job-results/job-2021-01-12T10.02-8e202b3/job.log
>>>> ...
>>>>> console: This software may be redistributed under the BSD copyright.
>>>>> console: Copyright 2000-2002, Opsycon AB, Sweden.
>>>>> console: Copyright 2005, ICT CAS.
>>>>> console: CPU GODSON3 BogoMIPS: 1327
>>>>> PASS (3.89 s)
>>>>> RESULTS: PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
>>>>> CANCEL 0
>>>>> JOB TIME   : 4.38 s
>>>>>
>>>>> Signed-off-by: Jiaxun Yang 
>>>>> ---
>>>>>  MAINTAINERS |  1 +
>>>>>  tests/acceptance/machine_mips_loongson3v.py | 39 +
>>>>>  2 files changed, 40 insertions(+)
>>>>>  create mode 100644 tests/acceptance/machine_mips_loongson3v.py
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 4be087b88e..f38882f997 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -1164,6 +1164,7 @@ F: hw/intc/loongson_liointc.c
>>>>>  F: hw/mips/loongson3_bootp.c
>>>>>  F: hw/mips/loongson3_bootp.h
>>>>>  F: hw/mips/loongson3_virt.c
>>>>> +F: tests/acceptance/machine_mips_loongson3v.py
>>>>>  
>>>>>  Boston
>>>>>  M: Paul Burton 
>>>>> diff --git a/tests/acceptance/machine_mips_loongson3v.py 
>>>>> b/tests/acceptance/machine_mips_loongson3v.py
>>>>> new file mode 100644
>>>>> index 00..17a85de69f
>>>>> --- /dev/null
>>>>> +++ b/tests/acceptance/machine_mips_loongson3v.py
>>>>> @@ -0,0 +1,39 @@
>>>>> +# Functional tests for the Generic Loongson-3 Platform.
>>>>> +#
>>>>> +# Copyright (c) 2020 Philippe Mathieu-Daudé 
>>>>
>>>> 2021 Jiaxun Yang ? :D
>>
>> Jiaxun, if you agree I can update that line and queue your patch.
> 
> Please do. Thanks!

So:

Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Philippe Mathieu-Daudé 

and applied (not removing AVOCADO_ALLOW_UNTRUSTED_CODE,
fixing copyright line) to mips-next.

Thanks,

Phil.



[PATCH] target/arm/m_helper: Silence GCC 10 maybe-uninitialized error

2021-01-18 Thread Philippe Mathieu-Daudé
When building with GCC 10.2 configured with --extra-cflags=-Os, we get:

  target/arm/m_helper.c: In function ‘arm_v7m_cpu_do_interrupt’:
  target/arm/m_helper.c:1811:16: error: ‘restore_s16_s31’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
   1811 | if (restore_s16_s31) {
|^
  target/arm/m_helper.c:1350:10: note: ‘restore_s16_s31’ was declared here
   1350 | bool restore_s16_s31;
|  ^~~
  cc1: all warnings being treated as errors

Initialize the 'restore_s16_s31' variable to silence the warning.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/arm/m_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 61760030292..731c435c00b 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -1347,7 +1347,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
 bool exc_secure = false;
 bool return_to_secure;
 bool ftype;
-bool restore_s16_s31;
+bool restore_s16_s31 = false;
 
 /*
  * If we're not in Handler mode then jumps to magic exception-exit
-- 
2.26.2




Re: [PATCH] tests/acceptance: Test PMON with Loongson-3A1000 CPU

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 5:54 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé  writes:
>> On 1/12/21 3:07 AM, Jiaxun Yang wrote:
>>> Test booting of PMON bootloader on loongson3-virt platform.
>>>
>>> $ (venv) AVOCADO_ALLOW_UNTRUSTED_CODE=1 \
>>> avocado --show=app,console \
>>>   run -t machine:loongson3-virt tests/acceptance
>>> Fetching asset from 
>>> tests/acceptance/machine_mips_loongson3v.py:MipsLoongson3v.test_pmon_serial_console
>>> JOB ID : 8e202b3727847c9104d0d3d6546ed225d35f6706
>>> JOB LOG: 
>>> /home/flygoat/avocado/job-results/job-2021-01-12T10.02-8e202b3/job.log
>> ...
>>> console: This software may be redistributed under the BSD copyright.
>>> console: Copyright 2000-2002, Opsycon AB, Sweden.
>>> console: Copyright 2005, ICT CAS.
>>> console: CPU GODSON3 BogoMIPS: 1327
>>> PASS (3.89 s)
>>> RESULTS: PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
>>> CANCEL 0
>>> JOB TIME   : 4.38 s
>>>
>>> Signed-off-by: Jiaxun Yang 
>>> ---
>>>  MAINTAINERS |  1 +
>>>  tests/acceptance/machine_mips_loongson3v.py | 39 +
>>>  2 files changed, 40 insertions(+)
>>>  create mode 100644 tests/acceptance/machine_mips_loongson3v.py
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 4be087b88e..f38882f997 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1164,6 +1164,7 @@ F: hw/intc/loongson_liointc.c
>>>  F: hw/mips/loongson3_bootp.c
>>>  F: hw/mips/loongson3_bootp.h
>>>  F: hw/mips/loongson3_virt.c
>>> +F: tests/acceptance/machine_mips_loongson3v.py
>>>  
>>>  Boston
>>>  M: Paul Burton 
>>> diff --git a/tests/acceptance/machine_mips_loongson3v.py 
>>> b/tests/acceptance/machine_mips_loongson3v.py
>>> new file mode 100644
>>> index 00..17a85de69f
>>> --- /dev/null
>>> +++ b/tests/acceptance/machine_mips_loongson3v.py
>>> @@ -0,0 +1,39 @@
>>> +# Functional tests for the Generic Loongson-3 Platform.
>>> +#
>>> +# Copyright (c) 2020 Philippe Mathieu-Daudé 
>>
>> 2021 Jiaxun Yang ? :D

Jiaxun, if you agree I can update that line and queue your patch.

>>
>>> +#
>>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> +# See the COPYING file in the top-level directory.
>>> +#
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +import os
>>> +import time
>>> +
>>> +from avocado import skipUnless
>>> +from avocado_qemu import Test
>>> +from avocado_qemu import wait_for_console_pattern
>>> +
>>> +class MipsLoongson3v(Test):
>>> +@skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted 
>>> code')
>>
>> The source code is published [1], you provided reproducible
>> workflow [2] and a tag [3] with a public build artifacts [4],
>> so my understanding is this is "trustable" binary.
>>
>> Alex, would it be OK to add this test without the UNTRUSTED tag
>> (amending the links in the commit description)?
> 
> It's a subjective call. Having open source code is a minimum step to
> being "trusted" but really the trust is in the community that hosts the
> code. The upstream distros (e.g. Debian/Fedora) are trusted because
> people install their software on their desktops and basically give the
> software publisher root on their machines. There has to be a level of
> trust that the distros won't abuse that to steal information from their
> users.
> 
> I personally have no idea about the loongson community because it's not
> one I interact with so I have no idea what sort of place it is. Is it a
> code dump for semi-proprietary non-upstreamed kernels or is it a place
> that has a good development culture with a sane security process that is
> responsive to problems and moderately conservative with what they merge?
> 
> If you would trust your keys to a machine running this communities
> software then by all means treated it as a trusted source.

Subjective call understood :)

Thanks for your clear explanation,

Phil.



[RFC PATCH] tests/docker: Allow passing --network option when building images

2021-01-18 Thread Philippe Mathieu-Daudé
When using the Docker engine, build fails because the container is
unable to resolve hostnames:

  $ make docker-image-debian-s390x-cross NETWORK=host ENGINE=docker
BUILD   debian10
  #6 9.679 Err:1 http://deb.debian.org/debian buster InRelease
  #6 9.679   Temporary failure resolving 'deb.debian.org'
  #6 16.69 Err:2 http://security.debian.org/debian-security buster/updates 
InRelease
  #6 16.69   Temporary failure resolving 'security.debian.org'
  #6 22.69 Err:3 http://deb.debian.org/debian buster-updates InRelease
  #6 22.69   Temporary failure resolving 'deb.debian.org'
  #6 22.74 W: Failed to fetch 
http://deb.debian.org/debian/dists/buster/InRelease  Temporary failure 
resolving 'deb.debian.org'
  #6 22.74 W: Failed to fetch 
http://security.debian.org/debian-security/dists/buster/updates/InRelease  
Temporary failure resolving 'security.debian.org'
  #6 22.74 W: Failed to fetch 
http://deb.debian.org/debian/dists/buster-updates/InRelease  Temporary failure 
resolving 'deb.debian.org'
  #6 22.74 W: Some index files failed to download. They have been ignored, or 
old ones used instead.
  Traceback (most recent call last):
File "./tests/docker/docker.py", line 709, in 
  sys.exit(main())
File "./tests/docker/docker.py", line 705, in main
  return args.cmdobj.run(args, argv)
File "./tests/docker/docker.py", line 498, in run
  dkr.build_image(tag, docker_dir, dockerfile,
File "./tests/docker/docker.py", line 353, in build_image
  self._do_check(build_args,
File "./tests/docker/docker.py", line 244, in _do_check
  return subprocess.check_call(self._command + cmd, **kwargs)
File "/usr/lib64/python3.8/subprocess.py", line 364, in check_call
  raise CalledProcessError(retcode, cmd)
  make: *** [tests/docker/Makefile.include:61: docker-image-debian10] Error 1

Fix by passing the NETWORK variable with --network= argument.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/docker/Makefile.include | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index bdc53ddfcf9..b65fd684011 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -63,6 +63,7 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
$(if $V,,--quiet) \
$(if $(NOCACHE),--no-cache, \
$(if $(DOCKER_REGISTRY),--registry $(DOCKER_REGISTRY))) 
\
+   $(if $(NETWORK),$(if $(subst 
$(NETWORK),,1),--network=$(NETWORK))) \
$(if $(NOUSER),,--add-current-user) \
$(if $(EXTRA_FILES),--extra-files $(EXTRA_FILES))\
$(if $(EXECUTABLE),--include-executable=$(EXECUTABLE)),\
-- 
2.26.2




[PATCH] tests/docker: Fix typo in help message

2021-01-18 Thread Philippe Mathieu-Daudé
To have the variable properly passed, we need to set it,
ie. NOUSER=1. Fix the message displayed by 'make docker'.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/docker/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 0779dab5b96..bdc53ddfcf9 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -209,7 +209,7 @@ endif
@echo ' before running the command.'
@echo 'NETWORK=1Enable virtual network interface with 
default backend.'
@echo 'NETWORK=$$BACKEND Enable virtual network interface with 
$$BACKEND.'
-   @echo 'NOUSER   Define to disable adding current user 
to containers passwd.'
+   @echo 'NOUSER=1 Define to disable adding current user 
to containers passwd.'
@echo 'NOCACHE=1Ignore cache when build images.'
@echo 'EXECUTABLE=Include executable in image.'
@echo 'EXTRA_FILES=" [... ]"'
-- 
2.26.2




[PATCH] tests/docker: Fix _get_so_libs() for docker-binfmt-image

2021-01-18 Thread Philippe Mathieu-Daudé
Fix a variable rename mistake from commit 5e33f7fead5:

  Traceback (most recent call last):
File "./tests/docker/docker.py", line 710, in 
  sys.exit(main())
File "./tests/docker/docker.py", line 706, in main
  return args.cmdobj.run(args, argv)
File "./tests/docker/docker.py", line 489, in run
  _copy_binary_with_libs(args.include_executable,
File "./tests/docker/docker.py", line 149, in _copy_binary_with_libs
  libs = _get_so_libs(src)
File "./tests/docker/docker.py", line 123, in _get_so_libs
  libs.append(s.group(1))
  NameError: name 's' is not defined

Fixes: 5e33f7fead5 ("tests/docker: better handle symlinked libs")
Signed-off-by: Philippe Mathieu-Daudé 
---
"Tested-by" but apparently not enough... Well actually it was on
Debian, now using Fedora.
---
 tests/docker/docker.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 884dfeb29c4..0b4f6167b3d 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -120,7 +120,7 @@ def _get_so_libs(executable):
 search = ldd_re.search(line)
 if search:
 try:
-libs.append(s.group(1))
+libs.append(search.group(1))
 except IndexError:
 pass
 except subprocess.CalledProcessError:
-- 
2.26.2




Re: [PATCH] hw/misc: sifive_u_otp: Use error_report() when block operation fails

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/19/21 4:23 AM, Bin Meng wrote:
> From: Bin Meng 
> 
> At present when blk_pread() / blk_pwrite() fails, a guest error
> is logged, but this is not really a guest error. Change to use
> error_report() instead.
> 
> Signed-off-by: Bin Meng 
> ---
> 
>  hw/misc/sifive_u_otp.c | 13 +
>  1 file changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 



Re: [PATCH] target/riscv: Declare csr_ops[] with a known size

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/19/21 3:52 AM, Bin Meng wrote:
> From: Bin Meng 
> 
> csr_ops[] is currently declared with an unknown size in cpu.h.
> Since the array size is known, let's do a complete declaration.
> 
> Signed-off-by: Bin Meng 
> ---
> 
>  target/riscv/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 



Re: [PULL 05/30] Makefile: wrap ctags in quiet-command calls

2021-01-18 Thread Philippe Mathieu-Daudé
Hi Alex,

On Fri, Jan 15, 2021 at 2:08 PM Alex Bennée  wrote:
>
> For prettier output.
>
> Signed-off-by: Alex Bennée 
> Reviewed-by: Willian Rampazzo 
> Reviewed-by: Philippe Mathieu-Daudé 
> Message-Id: <20210114165730.31607-6-alex.ben...@linaro.org>
>
> diff --git a/Makefile b/Makefile
> index 0c509a7704..bbab640b31 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -250,8 +250,13 @@ find-src-path = find "$(SRC_PATH)/" -path 
> "$(SRC_PATH)/meson" -prune -o \( -name
>
>  .PHONY: ctags
>  ctags:
> -   rm -f "$(SRC_PATH)/"tags
> -   $(find-src-path) -exec ctags -f "$(SRC_PATH)/"tags --append {} +
> +   $(call quiet-command,   \
> +   rm -f "$(SRC_PATH)/"tags,   \
> +   "CTAGS", "Remove old tags")
> +   $(call quiet-command, \
> +   $(find-src-path) -exec ctags\
> +   -f "$(SRC_PATH)/"tags --append {} +,\
> +   "CTAGS", "Re-index $(SRC_PATH)")
>
>  .PHONY: gtags
>  gtags:
> --
> 2.20.1
>

Build now fails if ctags is not installed:

$ if test -n "$MAKE_CHECK_ARGS"; then make -j"$JOBS" $MAKE_CHECK_ARGS ; fi
CTAGS Remove old tags
CTAGS Re-index /builds/philmd/qemu
find: 'ctags': No such file or directory
find: 'ctags': No such file or directory
find: 'ctags': No such file or directory
make: *** [Makefile:254: ctags] Error 1
make: *** Waiting for unfinished jobs




[PATCH] docker: Bump Fedora images to release 33

2021-01-18 Thread Philippe Mathieu-Daudé
Fedora 33 was released on October 27, 2020.

Update all the Fedora 32 images to this new release.

Suggested-by: Daniel Berrangé 
Signed-off-by: Philippe Mathieu-Daudé 
---
Based-on: <20210118181115.313742-1-phi...@redhat.com>
hw/usb/hcd-xhci: Fix extraneous format-truncation error on 32-bit hosts

Based-on: <20210118170308.282442-1-phi...@redhat.com>
hw/usb/dev-uas: Fix Clang 11 -Wgnu-variable-sized-type-not-at-end error
---
 tests/docker/dockerfiles/fedora-cris-cross.docker  | 2 +-
 tests/docker/dockerfiles/fedora-i386-cross.docker  | 2 +-
 tests/docker/dockerfiles/fedora-win32-cross.docker | 2 +-
 tests/docker/dockerfiles/fedora-win64-cross.docker | 2 +-
 tests/docker/dockerfiles/fedora.docker | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/docker/dockerfiles/fedora-cris-cross.docker 
b/tests/docker/dockerfiles/fedora-cris-cross.docker
index 09e7e449f9b..1dfff6e0b96 100644
--- a/tests/docker/dockerfiles/fedora-cris-cross.docker
+++ b/tests/docker/dockerfiles/fedora-cris-cross.docker
@@ -2,7 +2,7 @@
 # Cross compiler for cris system tests
 #
 
-FROM fedora:30
+FROM fedora:33
 ENV PACKAGES gcc-cris-linux-gnu
 RUN dnf install -y $PACKAGES
 RUN rpm -q $PACKAGES | sort > /packages.txt
diff --git a/tests/docker/dockerfiles/fedora-i386-cross.docker 
b/tests/docker/dockerfiles/fedora-i386-cross.docker
index a6e411291b9..966072c08e6 100644
--- a/tests/docker/dockerfiles/fedora-i386-cross.docker
+++ b/tests/docker/dockerfiles/fedora-i386-cross.docker
@@ -1,4 +1,4 @@
-FROM fedora:31
+FROM fedora:33
 ENV PACKAGES \
 bzip2 \
 diffutils \
diff --git a/tests/docker/dockerfiles/fedora-win32-cross.docker 
b/tests/docker/dockerfiles/fedora-win32-cross.docker
index 087df598a09..81b5659e9c5 100644
--- a/tests/docker/dockerfiles/fedora-win32-cross.docker
+++ b/tests/docker/dockerfiles/fedora-win32-cross.docker
@@ -1,4 +1,4 @@
-FROM fedora:32
+FROM fedora:33
 
 # Please keep this list sorted alphabetically
 ENV PACKAGES \
diff --git a/tests/docker/dockerfiles/fedora-win64-cross.docker 
b/tests/docker/dockerfiles/fedora-win64-cross.docker
index d5d2f5f00d6..bcb428e7242 100644
--- a/tests/docker/dockerfiles/fedora-win64-cross.docker
+++ b/tests/docker/dockerfiles/fedora-win64-cross.docker
@@ -1,4 +1,4 @@
-FROM fedora:32
+FROM fedora:33
 
 # Please keep this list sorted alphabetically
 ENV PACKAGES \
diff --git a/tests/docker/dockerfiles/fedora.docker 
b/tests/docker/dockerfiles/fedora.docker
index 0b5053f2d09..9ba8c147edd 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -1,4 +1,4 @@
-FROM fedora:32
+FROM fedora:33
 
 # Please keep this list sorted alphabetically
 ENV PACKAGES \
-- 
2.26.2




[PATCH] hw/usb/hcd-xhci: Fix extraneous format-truncation error on 32-bit hosts

2021-01-18 Thread Philippe Mathieu-Daudé
For some reason the assert() added in commit ccb799313a5
("hw/usb: avoid format truncation warning when formatting
port name") does not fix when building with GCC 10.

KISS and expand the buffer by 4 bytes to silent the following
error when using GCC 10.2.1 on Fedora 33:

  hw/usb/hcd-xhci.c: In function 'usb_xhci_realize':
  hw/usb/hcd-xhci.c:3309:54: error: '%d' directive output may be truncated 
writing between 1 and 8 bytes into a region of size 5 
[-Werror=format-truncation=]
   3309 | snprintf(port->name, sizeof(port->name), "usb2 port #%d", 
i+1);
|  ^~~
  hw/usb/hcd-xhci.c:3309:54: note: directive argument in the range [1, 89478486]
  In file included from /usr/include/stdio.h:866,
   from include/qemu/osdep.h:85,
   from hw/usb/hcd-xhci.c:22:
  /usr/include/bits/stdio2.h:70:10: note: '__builtin___snprintf_chk' output 
between 13 and 20 bytes into a destination of size 16
 70 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
|  ^~~~
 71 |__bos (__s), __fmt, __va_arg_pack ());
|~
  hw/usb/hcd-xhci.c:3323:54: error: '%d' directive output may be truncated 
writing between 1 and 8 bytes into a region of size 5 
[-Werror=format-truncation=]
   3323 | snprintf(port->name, sizeof(port->name), "usb3 port #%d", 
i+1);
|  ^~~
  hw/usb/hcd-xhci.c:3323:54: note: directive argument in the range [1, 89478486]
  In file included from /usr/include/stdio.h:866,
   from include/qemu/osdep.h:85,
   from hw/usb/hcd-xhci.c:22:
  /usr/include/bits/stdio2.h:70:10: note: '__builtin___snprintf_chk' output 
between 13 and 20 bytes into a destination of size 16
 70 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
|  ^~~~
 71 |__bos (__s), __fmt, __va_arg_pack ());
|~
  cc1: all warnings being treated as errors

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/usb/hcd-xhci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index 02ebd764509..7bba361f3bb 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -128,7 +128,7 @@ typedef struct XHCIPort {
 uint32_t portnr;
 USBPort  *uport;
 uint32_t speedmask;
-char name[16];
+char name[20];
 MemoryRegion mem;
 } XHCIPort;
 
-- 
2.26.2




[RFC PATCH 1/2] scsi/utils: Add INVALID_PARAM_VALUE sense code definition

2021-01-18 Thread Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/scsi/utils.h | 2 ++
 scsi/utils.c | 5 +
 2 files changed, 7 insertions(+)

diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index fbc55882799..096489c6cd1 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -57,6 +57,8 @@ extern const struct SCSISense sense_code_LBA_OUT_OF_RANGE;
 extern const struct SCSISense sense_code_INVALID_FIELD;
 /* Illegal request, Invalid field in parameter list */
 extern const struct SCSISense sense_code_INVALID_PARAM;
+/* Illegal request, Invalid value in parameter list */
+extern const struct SCSISense sense_code_INVALID_PARAM_VALUE;
 /* Illegal request, Parameter list length error */
 extern const struct SCSISense sense_code_INVALID_PARAM_LEN;
 /* Illegal request, LUN not supported */
diff --git a/scsi/utils.c b/scsi/utils.c
index b37c2830148..793c3a6b9c9 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -197,6 +197,11 @@ const struct SCSISense sense_code_INVALID_PARAM = {
 .key = ILLEGAL_REQUEST, .asc = 0x26, .ascq = 0x00
 };
 
+/* Illegal request, Invalid value in parameter list */
+const struct SCSISense sense_code_INVALID_PARAM_VALUE = {
+.key = ILLEGAL_REQUEST, .asc = 0x26, .ascq = 0x01
+};
+
 /* Illegal request, Parameter list length error */
 const struct SCSISense sense_code_INVALID_PARAM_LEN = {
 .key = ILLEGAL_REQUEST, .asc = 0x1a, .ascq = 0x00
-- 
2.26.2




[RFC PATCH 2/2] hw/usb/dev-uas: Report command additional adb length as unsupported

2021-01-18 Thread Philippe Mathieu-Daudé
We are not ready to handle additional CDB data.

If a guest send a packet with such additional data,
report the command parameter as not supported.

We can then explicit there is nothing in this additional
buffer, by fixing its size to zero.

This fixes an error when building with Clang 11:

  usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_iu' 
not at the end of a struct or class is a GNU extension 
[-Werror,-Wgnu-variable-sized-type-not-at-end]
  uas_iustatus;
^

Reported-by: Daniele Buono 
Signed-off-by: Philippe Mathieu-Daudé 
---
Cc: Ed Maste 
Cc: Han Han 
Cc: Marc-André Lureau 
Cc: Paolo Bonzini 
Cc: Gustavo A. R. Silva 
---
 hw/usb/dev-uas.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index cec071d96c4..b6434ad4b9c 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -16,6 +16,7 @@
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
+#include "qemu/log.h"
 
 #include "hw/usb.h"
 #include "migration/vmstate.h"
@@ -70,7 +71,7 @@ typedef struct {
 uint8_treserved_2;
 uint64_t   lun;
 uint8_tcdb[16];
-uint8_tadd_cdb[];
+uint8_tadd_cdb[0];  /* not supported by QEMU */
 } QEMU_PACKED  uas_iu_command;
 
 typedef struct {
@@ -700,6 +701,11 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
 uint32_t len;
 uint16_t tag = be16_to_cpu(iu->hdr.tag);
 
+if (iu->command.add_cdb_length > 0) {
+qemu_log_mask(LOG_UNIMP, "additional adb length not yet supported\n");
+goto unsupported_len;
+}
+
 if (uas_using_streams(uas) && tag > UAS_MAX_STREAMS) {
 goto invalid_tag;
 }
@@ -735,6 +741,10 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
 }
 return;
 
+unsupported_len:
+usb_uas_queue_fake_sense(uas, tag, sense_code_INVALID_PARAM_VALUE);
+return;
+
 invalid_tag:
 usb_uas_queue_fake_sense(uas, tag, sense_code_INVALID_TAG);
 return;
-- 
2.26.2




[RFC PATCH 0/2] hw/usb/dev-uas: Fix Clang 11 -Wgnu-variable-sized-type-not-at-end error

2021-01-18 Thread Philippe Mathieu-Daudé
Another attempt to fix the following Clang 11 warning:

  usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_i=
u' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-vari=
able-sized-type-not-at-end]
  uas_iustatus;
^
If a guest send a packet with additional data, respond
with "Illegal Request - parameter not supported".

Philippe Mathieu-Daud=C3=A9 (2):
  scsi/utils: Add INVALID_PARAM_VALUE sense code definition
  hw/usb/dev-uas: Report command additional adb length as unsupported

 include/scsi/utils.h |  2 ++
 hw/usb/dev-uas.c | 12 +++-
 scsi/utils.c |  5 +
 3 files changed, 18 insertions(+), 1 deletion(-)

--=20
2.26.2





Re: [PATCH 2/2] virtio-scsi-test: Test writing to scsi-cd device

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 1:34 PM, Kevin Wolf wrote:
> This tests that trying to write to a (read-only) scsi-cd device backed
> by a read-write image file doesn't crash and results in the correct
> error.
> 
> This is a regression test for https://bugs.launchpad.net/bugs/1906693.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qtest/virtio-scsi-test.c | 39 ++
>  1 file changed, 39 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH 1/2] block: Separate blk_is_writable() and blk_supports_write_perm()

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 1:34 PM, Kevin Wolf wrote:
> Currently, blk_is_read_only() tells whether a given BlockBackend can
> only be used in read-only mode because its root node is read-only. Some
> callers actually try to answer a slightly different question: Is the
> BlockBackend configured to be writable, by taking write permissions on
> the root node?
> 
> This can differ, for example, for CD-ROM devices which don't take write
> permissions, but may be backed by a writable image file. scsi-cd allows
> write requests to the drive if blk_is_read_only() returns false.
> However, the write request will immediately run into an assertion
> failure because the write permission is missing.
> 
> This patch introduces separate functions for both questions.
> blk_supports_write_perm() answers the question whether the block
> node/image file can support writable devices, whereas blk_is_writable()
> tells whether the BlockBackend is currently configured to be writable.
> 
> All calls of blk_is_read_only() are converted to one of the two new
> functions.
> 
> Fixes: https://bugs.launchpad.net/bugs/1906693
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Kevin Wolf 
> ---
>  include/sysemu/block-backend.h |  3 ++-
>  block/block-backend.c  | 19 ---
>  hw/block/dataplane/xen-block.c |  2 +-
>  hw/block/fdc.c |  9 +
>  hw/block/m25p80.c  |  6 +++---
>  hw/block/nand.c|  2 +-
>  hw/block/nvme-ns.c |  7 ---
>  hw/block/onenand.c |  2 +-
>  hw/block/pflash_cfi01.c|  2 +-
>  hw/block/pflash_cfi02.c|  2 +-
>  hw/block/swim.c|  6 +++---
>  hw/block/virtio-blk.c  |  6 +++---
>  hw/block/xen-block.c   |  2 +-
>  hw/ide/core.c  |  2 +-
>  hw/misc/sifive_u_otp.c |  2 +-
>  hw/ppc/pnv_pnor.c  |  2 +-
>  hw/scsi/scsi-disk.c| 10 +-
>  hw/scsi/scsi-generic.c |  4 ++--
>  hw/sd/sd.c |  6 +++---
>  hw/usb/dev-storage.c   |  4 ++--
>  20 files changed, 57 insertions(+), 41 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 




Re: [PULL 00/30] testing, gdbstub and semihosting

2021-01-18 Thread Philippe Mathieu-Daudé
Hi Alex,

On 1/18/21 1:18 PM, Alex Bennée wrote:
> 
> Peter Maydell  writes:
> 
>> On Fri, 15 Jan 2021 at 13:08, Alex Bennée  wrote:
>>>
>>> The following changes since commit 7c79721606be11b5bc556449e5bcbc331ef6867d:
>>>
>>>   Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-tcg-20210113' 
>>> into staging (2021-01-14 09:54:29 +)
>>>
>>> are available in the Git repository at:
>>>
>>>   https://github.com/stsquad/qemu.git tags/pull-testing-and-misc-150121-1
>>>
>>> for you to fetch changes up to be846761ca8b5a7e2e7b7108c8c156126b799824:
>>>
>>>   semihosting: Implement SYS_ISERROR (2021-01-15 11:12:34 +)
>>>
>>> 
>>> Testing, gdbstub and semihosting patches:
>>>
>>>   - clean-ups to docker images
>>>   - drop duplicate jobs from shippable
>>>   - prettier tag generation (+gtags)
>>>   - generate browsable source tree
>>>   - more Travis->GitLab migrations
>>>   - fix checkpatch to deal with commits
>>>   - gate gdbstub tests on 8.3.1, expand tests
>>>   - support Xfer:auxv:read gdb packet
>>>   - better gdbstub cleanup
>>>   - use GDB's SVE register layout
>>>   - make arm-compat-semihosting common
>>>   - add riscv semihosting support
>>>   - add HEAPINFO, ELAPSED, TICKFREQ, TMPNAM and ISERROR to semihosting
>>
>> Fails to build, netbsd:
>>
>> ../src/gdbstub.c: In function 'handle_query_xfer_auxv':
>> ../src/gdbstub.c:2258:26: error: 'struct image_info' has no member
>> named 'saved_auxv'
>>  saved_auxv = ts->info->saved_auxv;
>>   ^~
>> ../src/gdbstub.c:2259:24: error: 'struct image_info' has no member
>> named 'auxv_len'
>>  auxv_len = ts->info->auxv_len;
> 
> I've:
> 
> #if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX_USER)
> 
> around the code so it won't build for the *BSDs.

CONFIG_LINUX_USER implies CONFIG_USER_ONLY, right?

Maybe long-term this can become:

#if defined(CONFIG_LINUX_USER)
#elif defined(...BSD...)
#endif

(maybe worth to fix if the pullreq isn't processed,
else not a big deal).




Re: [PATCH v3] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib

2021-01-18 Thread Philippe Mathieu-Daudé
Ping^2

On Fri, Jan 8, 2021 at 5:02 PM Philippe Mathieu-Daudé  wrote:
>
> Ping?
>
> On 11/25/20 9:33 AM, Philippe Mathieu-Daudé wrote:
> > set_pci_host_devaddr() is hard to follow, thus bug-prone.
> >
> > For example, a bug was introduced in commit bccb20c49df, as
> > the same line might be used to parse a bus (up to 0xff) or
> > a slot (up to 0x1f).
> >
> > Instead of making things worst, rewrite using g_strsplit().
> >
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> > v3: Rebased
> > v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
> > ---
> >  hw/core/qdev-properties-system.c | 62 ++--
> >  1 file changed, 27 insertions(+), 35 deletions(-)
> >
> > diff --git a/hw/core/qdev-properties-system.c 
> > b/hw/core/qdev-properties-system.c
> > index 9d80a07d26f..79408e32289 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -857,11 +857,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor 
> > *v, const char *name,
> >  DeviceState *dev = DEVICE(obj);
> >  Property *prop = opaque;
> >  PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
> > -char *str, *p;
> > -char *e;
> > +g_autofree char *str = NULL;
> > +g_auto(GStrv) col_s0 = NULL;
> > +g_auto(GStrv) dot_s = NULL;
> > +char **col_s;
> >  unsigned long val;
> > -unsigned long dom = 0, bus = 0;
> > -unsigned int slot = 0, func = 0;
> >
> >  if (dev->realized) {
> >  qdev_prop_set_after_realize(dev, name, errp);
> > @@ -872,58 +872,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor 
> > *v, const char *name,
> >  return;
> >  }
> >
> > -p = str;
> > -val = strtoul(p, , 16);
> > -if (e == p || *e != ':') {
> > +col_s = col_s0 = g_strsplit(str, ":", 3);
> > +if (!col_s || !col_s[0] || !col_s[1]) {
> >  goto inval;
> >  }
> > -bus = val;
> >
> > -p = e + 1;
> > -val = strtoul(p, , 16);
> > -if (e == p) {
> > -goto inval;
> > -}
> > -if (*e == ':') {
> > -dom = bus;
> > -bus = val;
> > -p = e + 1;
> > -val = strtoul(p, , 16);
> > -if (e == p) {
> > +/* domain */
> > +if (col_s[2]) {
> > +if (qemu_strtoul(col_s[0], NULL, 16, ) < 0 || val > 0x) {
> >  goto inval;
> >  }
> > +addr->domain = val;
> > +col_s++;
> > +} else {
> > +addr->domain = 0;
> >  }
> > -slot = val;
> >
> > -if (*e != '.') {
> > +/* bus */
> > +if (qemu_strtoul(col_s[0], NULL, 16, ) < 0 || val > 0xff) {
> >  goto inval;
> >  }
> > -p = e + 1;
> > -val = strtoul(p, , 10);
> > -if (e == p) {
> > -goto inval;
> > -}
> > -func = val;
> > +addr->bus = val;
> >
> > -if (dom > 0x || bus > 0xff || slot > 0x1f || func > 7) {
> > +/* . */
> > +dot_s = g_strsplit(col_s[1], ".", 2);
> > +if (!dot_s || !dot_s[0] || !dot_s[1]) {
> >  goto inval;
> >  }
> >
> > -if (*e) {
> > +/* slot */
> > +if (qemu_strtoul(dot_s[0], NULL, 16, ) < 0 || val > 0x1f) {
> >  goto inval;
> >  }
> > +addr->slot = val;
> >
> > -addr->domain = dom;
> > -addr->bus = bus;
> > -addr->slot = slot;
> > -addr->function = func;
> > +/* func */
> > +if (qemu_strtoul(dot_s[1], NULL, 10, ) < 0 || val > 7) {
> > +goto inval;
> > +}
> > +addr->function = val;
> >
> > -g_free(str);
> >  return;
> >
> >  inval:
> >  error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > -g_free(str);
> >  }
> >
> >  const PropertyInfo qdev_prop_pci_host_devaddr = {
> >
>




[PATCH] tests/docker: Update the xtensa container to debian 10

2021-01-18 Thread Philippe Mathieu-Daudé
Align the Xtensa docker image with the other Debian-based ones.

Signed-off-by: Philippe Mathieu-Daudé 
---
 .gitlab-ci.d/containers.yml |  2 ++
 tests/docker/Makefile.include   |  1 +
 tests/docker/dockerfiles/debian-xtensa-cross.docker | 12 +++-
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
index 910754a699f..08bccd96901 100644
--- a/.gitlab-ci.d/containers.yml
+++ b/.gitlab-ci.d/containers.yml
@@ -204,6 +204,8 @@ tricore-debian-cross-container:
 
 xtensa-debian-cross-container:
   <<: *container_job_definition
+  stage: containers-layer2
+  needs: ['amd64-debian10-container']
   variables:
 NAME: debian-xtensa-cross
 
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index c254ac38d0a..464740533e9 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -134,6 +134,7 @@ docker-image-travis: NOUSER=1
 
 # Specialist build images, sometimes very limited tools
 docker-image-debian-tricore-cross: docker-image-debian10
+docker-image-debian-xtensa-cross: docker-image-debian10
 docker-image-debian-all-test-cross: docker-image-debian10
 docker-image-debian-arm64-test-cross: docker-image-debian11
 
diff --git a/tests/docker/dockerfiles/debian-xtensa-cross.docker 
b/tests/docker/dockerfiles/debian-xtensa-cross.docker
index ba4148299c5..f89f1d9e247 100644
--- a/tests/docker/dockerfiles/debian-xtensa-cross.docker
+++ b/tests/docker/dockerfiles/debian-xtensa-cross.docker
@@ -1,22 +1,16 @@
 #
 # Docker cross-compiler target
 #
-# This docker target builds on the debian stretch base image,
+# This docker target builds on the debian Buster base image,
 # using a prebuilt toolchains for Xtensa cores from:
 # https://github.com/foss-xtensa/toolchain/releases
 #
-FROM debian:stretch-slim
+FROM qemu/debian10
 
 RUN apt-get update && \
-DEBIAN_FRONTEND=noninteractive apt install -yy eatmydata && \
 DEBIAN_FRONTEND=noninteractive eatmydata \
 apt-get install -y --no-install-recommends \
-build-essential \
-ca-certificates \
-curl \
-gettext \
-git \
-python3-minimal
+curl
 
 ENV CPU_LIST dc232b dc233c de233_fpu dsp3400
 ENV TOOLCHAIN_RELEASE 2020.07
-- 
2.26.2




Re: [Qemu-devel] [PULL 7/7] hw/usb: avoid format truncation warning when formatting port name

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 12:35 PM, Daniel P. Berrangé wrote:
> On Mon, Jan 18, 2021 at 12:31:07PM +0100, Philippe Mathieu-Daudé wrote:
>> Hello,
>>
>> On 5/3/19 8:59 AM, Gerd Hoffmann wrote:
>>> From: Daniel P. Berrangé 
>>>
>>> hw/usb/hcd-xhci.c: In function ‘usb_xhci_realize’:
>>> hw/usb/hcd-xhci.c:3339:66: warning: ‘%d’ directive output may be truncated 
>>> writing between 1 and 10 bytes into a region of size 5 [-Wformat-trunca\
>>> tion=]
>>>  3339 | snprintf(port->name, sizeof(port->name), "usb2 port 
>>> #%d", i+1);
>>>   |  ^~
>>> hw/usb/hcd-xhci.c:3339:54: note: directive argument in the range [1, 
>>> 2147483647]
>>>  3339 | snprintf(port->name, sizeof(port->name), "usb2 port 
>>> #%d", i+1);
>>>   |  ^~~
>>>
>>> The xhci code formats the port name into a fixed length
>>> buffer which is only large enough to hold port numbers
>>> upto 5 digits in decimal representation. We're never
>>> going to have a port number that large, so aserting the
>>> port number is sensible is sufficient to tell GCC the
>>> formatted string won't be truncated.
>>>
>>> Signed-off-by: Daniel P. Berrangé 
>>> Message-Id: <20190412121626.19829-5-berra...@redhat.com>
>>>
>>> [ kraxel: also s/int/unsigned int/ to tell gcc they can't
>>>   go negative. ]
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>> ---
>>>  hw/usb/hcd-xhci.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>>> index d8472b4fea7f..2e9a839f2bf9 100644
>>> --- a/hw/usb/hcd-xhci.c
>>> +++ b/hw/usb/hcd-xhci.c
>>> @@ -3306,7 +3306,7 @@ static void usb_xhci_init(XHCIState *xhci)
>>>  {
>>>  DeviceState *dev = DEVICE(xhci);
>>>  XHCIPort *port;
>>> -int i, usbports, speedmask;
>>> +unsigned int i, usbports, speedmask;
>>>  
>>>  xhci->usbsts = USBSTS_HCH;
>>>  
>>> @@ -3336,6 +3336,7 @@ static void usb_xhci_init(XHCIState *xhci)
>>>  USB_SPEED_MASK_LOW  |
>>>  USB_SPEED_MASK_FULL |
>>>  USB_SPEED_MASK_HIGH;
>>> +assert(i < MAXPORTS);
>>>  snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
>>>  speedmask |= port->speedmask;
>>>  }
>>> @@ -3349,6 +3350,7 @@ static void usb_xhci_init(XHCIState *xhci)
>>>  }
>>>  port->uport = >uports[i];
>>>  port->speedmask = USB_SPEED_MASK_SUPER;
>>> +assert(i < MAXPORTS);
>>>  snprintf(port->name, sizeof(port->name), "usb3 port #%d", i+1);
>>>  speedmask |= port->speedmask;
>>>  }
>>>
>>
>> I am confused, I upgraded Fedora 32 -> 33 and am now getting this
>> error back, the assertion being apparently ignored:
> 
> I'm not seeing this on F33 myself, but our CI is still F32. We
> should upgrade that.
> 
> What are your configure args ?

Ah sorry, this is my --extra-cflags=-m32 config directory.




Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

2021-01-18 Thread Philippe Mathieu-Daudé
On 11/19/20 5:16 PM, Daniele Buono wrote:
> Hi Philippe,
> 
> On 11/6/2020 9:28 AM, Philippe Mathieu-Daudé wrote:
>> On 11/5/20 11:18 PM, Daniele Buono wrote:
>>> The UASStatus data structure has a variable sized field inside of
>>> type uas_iu,
>>> that however is not placed at the end of the data structure.
>>>
>>> This placement triggers a warning with clang 11, and while not a bug
>>> right now,
>>> (the status is never a uas_iu_command, which is the variable-sized
>>> case),
>>> it could become one in the future.
>>
>> The problem is uas_iu_command::add_cdb, indeed.
>>
>>>
>>> ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with
>>> variable sized type 'uas_iu' not at the end of a struct or class is a
>>> GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>>
>> If possible remove the "../qemu-base/" as it does not provide
>> any useful information.
>>
> Sure, will do at the next cycle
>>>  uas_iu    status;
>>>    ^
>>> 1 error generated.
>>>
>>> Fix this by moving uas_iu at the end of the struct
>>
>> Your patch silents the warning, but the problem is the same.
>> It would be safer/cleaner to make 'status' a pointer on the heap IMO.
> 
> I'm thinking of moving 'status' in a pointer with the following code
> changes:
> 
> UASStatus is allocated in `usb_uas_alloc_status`, which currently does
> not take a type or size for the union field. I'm thinking of adding
> requested size for the status, like this:
> 
> static UASStatus *usb_uas_alloc_status(UASDevice *uas, uint8_t id,
> uint16_t tag, size_t size);
> 
> and the common call would be
> usb_uas_alloc_status([...],sizeof(uas_iu));
> 
> Also we'd need a double free when the object is freed. Right now
> it's handled in the code when the object is not used anymore with a
> `g_free(st);`.
> I'd have to replace it with
> `g_free(st->status); g_free(st);`. Would you suggest doing it place
> or by adding a usb_uas_dealloc_status() function?
> 
> ---
> 
> However, I am confused by the use of that variable-lenght field.
> I'm looking at what seems the only place where a command is
> parsed, in `usb_uas_handle_data`.
> 
> uas_iu iu;
> [...]
>     switch (p->ep->nr) {
>     case UAS_PIPE_ID_COMMAND:
>     length = MIN(sizeof(iu), p->iov.size);
>     usb_packet_copy(p, , length);
>     [...]
>     break;
> [...]
> 
> It would seem that the copy is limited to at most sizeof(uas_iu),
> so even if we had anything in add_cdb[], that wouldn't be copied
> here?
> 
> Is this intended?

Gerd, do you know?




Re: [Qemu-devel] [PULL 7/7] hw/usb: avoid format truncation warning when formatting port name

2021-01-18 Thread Philippe Mathieu-Daudé
Hello,

On 5/3/19 8:59 AM, Gerd Hoffmann wrote:
> From: Daniel P. Berrangé 
> 
> hw/usb/hcd-xhci.c: In function ‘usb_xhci_realize’:
> hw/usb/hcd-xhci.c:3339:66: warning: ‘%d’ directive output may be truncated 
> writing between 1 and 10 bytes into a region of size 5 [-Wformat-trunca\
> tion=]
>  3339 | snprintf(port->name, sizeof(port->name), "usb2 port #%d", 
> i+1);
>   |  ^~
> hw/usb/hcd-xhci.c:3339:54: note: directive argument in the range [1, 
> 2147483647]
>  3339 | snprintf(port->name, sizeof(port->name), "usb2 port #%d", 
> i+1);
>   |  ^~~
> 
> The xhci code formats the port name into a fixed length
> buffer which is only large enough to hold port numbers
> upto 5 digits in decimal representation. We're never
> going to have a port number that large, so aserting the
> port number is sensible is sufficient to tell GCC the
> formatted string won't be truncated.
> 
> Signed-off-by: Daniel P. Berrangé 
> Message-Id: <20190412121626.19829-5-berra...@redhat.com>
> 
> [ kraxel: also s/int/unsigned int/ to tell gcc they can't
>   go negative. ]
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/usb/hcd-xhci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index d8472b4fea7f..2e9a839f2bf9 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3306,7 +3306,7 @@ static void usb_xhci_init(XHCIState *xhci)
>  {
>  DeviceState *dev = DEVICE(xhci);
>  XHCIPort *port;
> -int i, usbports, speedmask;
> +unsigned int i, usbports, speedmask;
>  
>  xhci->usbsts = USBSTS_HCH;
>  
> @@ -3336,6 +3336,7 @@ static void usb_xhci_init(XHCIState *xhci)
>  USB_SPEED_MASK_LOW  |
>  USB_SPEED_MASK_FULL |
>  USB_SPEED_MASK_HIGH;
> +assert(i < MAXPORTS);
>  snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
>  speedmask |= port->speedmask;
>  }
> @@ -3349,6 +3350,7 @@ static void usb_xhci_init(XHCIState *xhci)
>  }
>  port->uport = >uports[i];
>  port->speedmask = USB_SPEED_MASK_SUPER;
> +assert(i < MAXPORTS);
>  snprintf(port->name, sizeof(port->name), "usb3 port #%d", i+1);
>  speedmask |= port->speedmask;
>  }
> 

I am confused, I upgraded Fedora 32 -> 33 and am now getting this
error back, the assertion being apparently ignored:

C compiler for the host machine: cc (gcc 10.2.1 "cc (GCC) 10.2.1
20201125 (Red Hat 10.2.1-9)")

...
  QEMU_CFLAGS: -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
-m32 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv  -m32
-Wold-style-declaration -Wold-style-definition -Wtype-limits
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
-Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined
-Wimplicit-fallthrough=2 -Wno-missing-include-dirs
-Wno-shift-negative-value -Wno-psabi -fstack-protector-strong
 QEMU_LDFLAGS: -Wl,--warn-common -Wl,-z,relro
-Wl,-z,now -m32  -m32 -fstack-protector-strong
...

[889/5130] Compiling C object libcommon.fa.p/hw_usb_hcd-xhci.c.o
FAILED: libcommon.fa.p/hw_usb_hcd-xhci.c.o
cc -Ilibcommon.fa.p -I. -I.. -I../slirp -I../slirp/src
-I../capstone/include/capstone -I../dtc/libfdt -Iqapi -Itrace -Iui
-Iui/shader -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include
-I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0
-I/usr/include/pixman-1 -I/usr/include/p11-kit-1
-fdiagnostics-color=auto -pipe -Wall -Winvalid-pch -Werror -std=gnu99
-O2 -g -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -m32 -D_GNU_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
-Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes
-fno-strict-aliasing -fno-common -fwrapv -m32 -Wold-style-declaration
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
-Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi
-fstack-protector-strong -isystem linux-headers -isystem linux-headers
-iquote tcg/i386 -iquote . -iquote accel/tcg -iquote include -iquote
disas/libvixl -pthread -fPIC -MD -MQ libcommon.fa.p/hw_usb_hcd-xhci.c.o
-MF libcommon.fa.p/hw_usb_hcd-xhci.c.o.d -o
libcommon.fa.p/hw_usb_hcd-xhci.c.o -c ../hw/usb/hcd-xhci.c
../hw/usb/hcd-xhci.c: In function 'usb_xhci_realize':
../hw/usb/hcd-xhci.c:3309:54: error: '%d' directive output may be
truncated writing between 1 and 8 bytes into a region of size 5
[-Werror=format-truncation=]
 3309 | snprintf(port->name, sizeof(port->name), "usb2 port
#%d", 

Re: [PATCH 3/4] pvpanic : update pvpanic spec document

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/15/21 7:34 PM, Mihai Carabas wrote:
> Add pvpanic PCI device support details in docs/specs/pvpanic.txt.
> 
> Signed-off-by: Mihai Carabas 
> ---
>  docs/specs/pvpanic.txt | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
> index a90fbca..974aafd 100644
> --- a/docs/specs/pvpanic.txt
> +++ b/docs/specs/pvpanic.txt
> @@ -1,7 +1,7 @@
>  PVPANIC DEVICE
>  ==
>  
> -pvpanic device is a simulated ISA device, through which a guest panic
> +pvpanic device is a simulated device, through which a guest panic
>  event is sent to qemu, and a QMP event is generated. This allows
>  management apps (e.g. libvirt) to be notified and respond to the event.
>  
> @@ -9,6 +9,9 @@ The management app has the option of waiting for 
> GUEST_PANICKED events,
>  and/or polling for guest-panicked RunState, to learn when the pvpanic
>  device has fired a panic event.
>  
> +The pvpanic device can be implemented as an ISA device (using IOPORT) or as a
> +PCI device.
> +
>  ISA Interface
>  -
>  
> @@ -24,6 +27,14 @@ bit 1: a guest panic has happened and will be handled by 
> the guest;
> the host should record it or report it, but should not affect
> the execution of the guest.
>  
> +PCI Interface
> +-
> +
> +The PCI interface is similar to the ISA interface except that it uses an MMIO
> +address space provided by its BAR0, 1 byte long. Any machine with a PCI 
> device

"device" -> "bus"?

> +can enable a pvpanic device by adding '-device pvpanic-pci' to the command
> +line.
> +
>  ACPI Interface
>  --
>  
> 




Re: [PATCH v2] ide: atapi: check logical block address and read size (CVE-2020-29443)

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 7:32 AM, P J P wrote:
> From: Prasad J Pandit 
> 
> While processing ATAPI cmd_read/cmd_read_cd commands,
> Logical Block Address (LBA) maybe invalid OR closer to the last block,
> leading to an OOB access issues. Add range check to avoid it.
> 
> Fixes: CVE-2020-29443
> Reported-by: Wenxiang Qian 
> Fix-suggested-by: Paolo Bonzini 

"Suggested-by"

> Signed-off-by: Prasad J Pandit 
> ---
>  hw/ide/atapi.c | 30 --
>  1 file changed, 24 insertions(+), 6 deletions(-)




Re: [PATCH v2 6/9] tests: Rename PAGE_SIZE definitions

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 7:38 AM, Jiaxun Yang wrote:
> As per POSIX specification of limits.h [1], OS libc may define
> PAGE_SIZE in limits.h.
> 
> Self defined PAGE_SIZE is frequently used in tests, to prevent
> collosion of definition, we give PAGE_SIZE definitons reasonable
> prefixs.
> 
> [1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html
> 
> Signed-off-by: Jiaxun Yang 
> Reviewed-by: Thomas Huth 
> ---
>  tests/migration/stress.c| 10 ++---
>  tests/qtest/libqos/malloc-pc.c  |  4 +-
>  tests/qtest/libqos/malloc-spapr.c   |  4 +-
>  tests/qtest/m25p80-test.c   | 54 +++---
>  tests/tcg/multiarch/system/memory.c |  6 +--
>  tests/test-xbzrle.c | 70 ++---
>  6 files changed, 74 insertions(+), 74 deletions(-)
...

> diff --git a/tests/tcg/multiarch/system/memory.c 
> b/tests/tcg/multiarch/system/memory.c
> index d124502d73..eb0ec6f8eb 100644
> --- a/tests/tcg/multiarch/system/memory.c
> +++ b/tests/tcg/multiarch/system/memory.c
> @@ -20,12 +20,12 @@
>  # error "Target does not specify CHECK_UNALIGNED"
>  #endif
>  
> -#define PAGE_SIZE 4096 /* nominal 4k "pages" */
> -#define TEST_SIZE (PAGE_SIZE * 4)  /* 4 pages */
> +#define MEM_PAGE_SIZE 4096 /* nominal 4k "pages" */
> +#define TEST_SIZE (MEM_PAGE_SIZE * 4)  /* 4 pages */

Clearer renaming TEST_PAGE_SIZE and TEST_MEM_SIZE.

If possible using TEST_PAGE_SIZE / TEST_MEM_SIZE:
Reviewed-by: Philippe Mathieu-Daudé 

>  
>  #define ARRAY_SIZE(x) ((sizeof(x) / sizeof((x)[0])))
>  
> -__attribute__((aligned(PAGE_SIZE)))
> +__attribute__((aligned(MEM_PAGE_SIZE)))
>  static uint8_t test_data[TEST_SIZE];




Re: [PATCH v2 5/9] elf2dmp: Rename PAGE_SIZE to ELF2DMP_PAGE_SIZE

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 7:38 AM, Jiaxun Yang wrote:
> As per POSIX specification of limits.h [1], OS libc may define
> PAGE_SIZE in limits.h.
> 
> To prevent collosion of definition, we rename PAGE_SIZE here.
> 
> [1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html
> 
> Signed-off-by: Jiaxun Yang 
> Reviewed-by: Thomas Huth 
> ---
>  contrib/elf2dmp/addrspace.h |  6 +++---
>  contrib/elf2dmp/addrspace.c |  4 ++--
>  contrib/elf2dmp/main.c  | 18 +-
>  3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/contrib/elf2dmp/addrspace.h b/contrib/elf2dmp/addrspace.h
> index d87f6a18c6..00b44c1218 100644
> --- a/contrib/elf2dmp/addrspace.h
> +++ b/contrib/elf2dmp/addrspace.h
> @@ -10,9 +10,9 @@
>  
>  #include "qemu_elf.h"
>  
> -#define PAGE_BITS 12
> -#define PAGE_SIZE (1ULL << PAGE_BITS)
> -#define PFN_MASK (~(PAGE_SIZE - 1))
> +#define ELF2DMP_PAGE_BITS 12
> +#define ELF2DMP_PAGE_SIZE (1ULL << ELF2DMP_PAGE_BITS)
> +#define ELF2DMP_PFN_MASK (~(ELF2DMP_PAGE_SIZE - 1))

Here you renamed all definitions, better.

Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH v2 4/9] hw/block/nand: Rename PAGE_SIZE to NAND_PAGE_SIZE

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 7:38 AM, Jiaxun Yang wrote:
> As per POSIX specification of limits.h [1], OS libc may define
> PAGE_SIZE in limits.h.
> 
> To prevent collosion of definition, we rename PAGE_SIZE here.
> 
> [1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html
> 
> Signed-off-by: Jiaxun Yang 
> Reviewed-by: Thomas Huth 
> ---
>  hw/block/nand.c | 40 
>  1 file changed, 20 insertions(+), 20 deletions(-)
...

> -# define PAGE_SIZE   256
> +# define NAND_PAGE_SIZE 256
>  # define PAGE_SHIFT  8
>  # define PAGE_SECTORS1
>  # define ADDR_SHIFT  8
>  # include "nand.c"

Why not rename all SIZE/SHIFT/SECTORS at once, to avoid
having half NAND and half generic names?




Re: [PATCH v2 3/9] osdep.h: Remove include

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 7:38 AM, Jiaxun Yang wrote:
> From: Michael Forney 
> 
> Prior to 2a4b472c3c, sys/signal.h was only included on OpenBSD
> (apart from two .c files). The POSIX standard location for this
> header is just  and in fact, OpenBSD's signal.h includes
> sys/signal.h itself.
> 
> Unconditionally including  on musl causes warnings
> for just about every source file:
> 
>   /usr/include/sys/signal.h:1:2: warning: #warning redirecting incorrect 
> #include  to  [-Wcpp]
>   1 | #warning redirecting incorrect #include  to 
> |  ^~~
> 
> Since there don't seem to be any platforms which require including
>  in addition to , and some platforms like
> Haiku lack it completely, just remove it.
> 
> Tested building on OpenBSD after removing this include.
> 
> Signed-off-by: Michael Forney 
> Reviewed-by: Eric Blake 
> [jiaxun.y...@flygoat.com: Move to meson]
> Signed-off-by: Jiaxun Yang 
> ---
>  meson.build  | 1 -
>  include/qemu/osdep.h | 4 
>  2 files changed, 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH v2 2/9] libvhost-user: Include poll.h instead of sys/poll.h

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 7:38 AM, Jiaxun Yang wrote:
> Musl libc complains about it's wrong usage.
> 
> In file included from ../subprojects/libvhost-user/libvhost-user.h:20,
>  from ../subprojects/libvhost-user/libvhost-user-glib.h:19,
>  from ../subprojects/libvhost-user/libvhost-user-glib.c:15:
> /usr/include/sys/poll.h:1:2: error: #warning redirecting incorrect #include 
>  to  [-Werror=cpp]
> 1 | #warning redirecting incorrect #include  to 
>   |  ^~~
> 
> Signed-off-by: Jiaxun Yang 
> Reviewed-by: Thomas Huth 
> ---
>  subprojects/libvhost-user/libvhost-user.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 10:29 AM, Claudio Fontana wrote:
> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>> cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
>> and are not available when TCG accelerator is not built. Add stubs so
>> linking without TCG succeed.
> 
> The reason why stubs are needed here at all seems to be that that the code
> calling cpu_loop_exit is not refactored properly yet;

I agree ...

> if we look at the example of i386, after the refactoring moving tcg related 
> code into target/i386/tcg/,
> (and really even before that I think),
> the code calling cpu_loop_exit is not built for non-TCG at all, and so we 
> don't need stubs.
> 
> I am ok with this anyway, just wanted to convey that I think we should look 
> at stubs as a necessary evil until all code stops mixing tcg, kvm and other 
> accels...
> 
> Thanks,
> 
> Claudio
> 
>>
>> Problematic files:
>>
>> - hw/semihosting/console.c in qemu_semihosting_console_inc()
>> - hw/ppc/spapr_hcall.c in h_confer()
>> - hw/s390x/ipl.c in s390_ipl_reset_request()
>> - hw/misc/mips_itu.c

... but I have no clue how to refactore these, as they
are used in both KVM and TCG.

How would you do? I'm stuck with the semihosting code
dependency on ARM since 2 years...

Phil.



Re: [RFC PATCH 6/6] softmmu: Restrict watchpoint handlers to TCG accelerator

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 10:10 AM, Claudio Fontana wrote:
> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>> Watchpoint funtions use cpu_restore_state() which is only
>> available when TCG accelerator is built. Restrict them
>> to TCG.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
> 
> I am doing some of this in my series, and I did not notice that
> cpu_watchpoint_insert was also TCG only.
> 
> Probably we should merge this somehow.
> 
> I thought it was used by gdbstub.c as well, passing flags BP_GDB .

BP_MEM_ACCESS for watchpoint.

> I noticed that gdbstub does something else entirely for kvm_enabled(), ie, 
> kvm_insert_breakpoint,
> but what about the other accels, it seems that the code flows to the 
> cpu_breakpoint_insert and watchpoint_insert..?
> 
> should cpu_breakpoint_insert have the same fate then?
> 
> And is this really all TCG specific?
> 
> From gdbstub.c:1020:
> 
> static int gdb_breakpoint_insert(int type, target_ulong addr, target_ulong 
> len)
> {
> CPUState *cpu;
> int err = 0;
> 
> if (kvm_enabled()) {
> return kvm_insert_breakpoint(gdbserver_state.c_cpu, addr, len, type);

Doh I missed that. I remember Peter and Richard explained it once
but I forgot and couldn't find on the list, maybe it was on IRC.

See i.e. in target/arm/kvm64.c:

 312 int kvm_arch_insert_hw_breakpoint(target_ulong addr,
 313   target_ulong len, int type)
 314 {
 315 switch (type) {
 316 case GDB_BREAKPOINT_HW:
 317 return insert_hw_breakpoint(addr);
 318 break;
 319 case GDB_WATCHPOINT_READ:
 320 case GDB_WATCHPOINT_WRITE:
 321 case GDB_WATCHPOINT_ACCESS:
 322 return insert_hw_watchpoint(addr, len, type);
 323 default:
 324 return -ENOSYS;
 325 }
 326 }

> 
>> ---
>> RFC because we could keep that code by adding an empty
>> stub for cpu_restore_state(), but it is unclear as
>> the function is named generically.
>> ---
>>  include/hw/core/cpu.h | 4 ++--
>>  softmmu/physmem.c | 4 
>>  2 files changed, 6 insertions(+), 2 deletions(-)



Re: [PATCH-for-5.2 2/2] gitlab-ci: Avoid running the EDK2 job when not necessary

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 9:30 AM, Thomas Huth wrote:
> On 17/01/2021 19.48, Philippe Mathieu-Daudé wrote:
>> On 11/11/20 10:18 AM, Philippe Mathieu-Daudé wrote:
>>> On 11/10/20 4:35 PM, Daniel P. Berrangé wrote:
>>>> On Tue, Nov 10, 2020 at 01:16:06PM +0100, Philippe Mathieu-Daudé wrote:
>>>>> The EDK2 jobs use the 'changes' keyword, which "makes it
>>>>> possible to define if a job should be created based on files
>>>>> modified by a Git push event." (see [1]). This keyword comes
>>>>> with a warning:
>>>>>
>>>>>    Caution:
>>>>>
>>>>>  In pipelines with sources other than the three above
>>>>>  changes can’t determine if a given file is new or old
>>>>>  and always returns true."
>>>>>
>>>>> In commit 922febe2af we moved the YAML config file from the
>>>>> repository root directory to the .gitlab-ci.d/ directory.
>>>>>
>>>>> We didn't respect the previous warning and disabled the
>>>>> 'changes' filter rule, as the files are now in a (directory)
>>>>> three above the YAML config file.
>>>>
>>>> This description is a bit wierd. I don't see how the location
>>>> in the directory tree has any relevance here.
>>>>
>>>> IIUC the caution docs quoted above are referring to what triggered
>>>> the pipeline. They're saying that if the trigger was not a "branch",
>>>> "merge request", or "external pull request", then the "changes" rule
>>>> always evaluates true.
>>>>
>>>> The "branch" source us a bit wierd though, as I'm not seeing
>>>> how gitlab figures out which commits are "new" to the pipeline
>>>> and thus whether the files were modified or not.
>>>>
>>>> Strangely qemu-project/qemu CI for master seems to be behaving
>>>> correctly and skipping the jobs.
>>>
>>> What about this one?
>>>
>>> https://gitlab.com/berrange/qemu/-/jobs/827459510
>>
>> Ping?
> 
> Daniel's comment sounded like there are some changes required here, at
> least to improve the commit message? So I was expecting a v2 here.
> Please clarify Daniel's concerns, then I can either pick up this version
> here or the v2 (in case you send one).

Sorry, this "ping" was for Daniel. I'm not sure how to continue,
so shared an example in his namespace of "incorrect behavior".

Meanwhile I'm tempted to put those jobs in Manual mode (2 months now),
until we settle this with Daniel.

Thanks,

Phil.




Re: [PATCH 00/18] hw: Mark the device with no migratable fields

2021-01-18 Thread Philippe Mathieu-Daudé
Hi Laurent,

On 1/18/21 8:33 AM, Laurent Vivier wrote:
> Le 14/01/2021 à 16:49, Philippe Mathieu-Daudé a écrit :
>> On 7/9/20 9:19 PM, Peter Maydell wrote:
>>> On Fri, 3 Jul 2020 at 21:19, Philippe Mathieu-Daudé  wrote:
>>>>
>>>> This is a proof-of-concept after chatting with Peter Maydell
>>>> on IRC earlier.
>>>>
>>>> Introduce the vmstate_no_state_to_migrate structure, and
>>>> a reference to it: vmstate_qdev_no_state_to_migrate.
>>>> Use this reference in devices with no fields to migrate.
>>>>
>>>> This is useful to catch devices missing vmstate, such:
>>>> - ads7846
>>>> - mcf-uart
>>>> - mcf-fec
>>>> - versatile_i2c
>>>> - ...
>>>>
>>>> I am not sure about:
>>>> - gpex-pcihost
>>>
>>> I think it's correct that this has no internal state:
>>> the only interesting state is in the GPEXRootState, which
>>> is a TYPE_GPEX_ROOT_DEVICE which migrates itself.
>>>
>>> I made some comments on the "meaty" bits of the patchset,
>>> and reviewed one or two of the "mark this device as
>>> having no migration state" patches, but it doesn't seem
>>> worth reviewing all of them until the migration submaintainers
>>> have a chance to weigh in on whether they like the concept
>>> (I expect they're busy right now with freeze-related stuff :-))
>>
>> Now that we are far from freeze-date is a good time to ping
>> again on this concept :)
>>
>> Most of the devices are ARM except:
>> - cpu-cluster (Eduardo/Marcel)
>> - hcd-ohci (Gerd)
>> - mac-nubus-bridge (Laurent)
>> - generic QOM (Daniel, Paolo)
>>
>> Is someone against this proposal?
> 
> I'm not against the proposal, but I don't understand why we need this.

IIRC the IRC discussion followed this thread:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg554453.html

Quoting Peter:

> I think we should care about migration on all architectures
> and devices, in the sense that we want savevm/loadvm to work.
> This is a really useful debugging and user tool, and when
> I'm reviewing devices it's the minimum bar I think new
> devices should clear. You then get migration "for free" but
> I don't particularly expect it to be used compared to
> snapshot save/restore. (Of course some of our existing code
> doesn't support this, and we don't have a good way of testing
> so bugs creep in easily, but as a principle I think it's
> good.)

Currently there is no automatic way to catch missing vmstate,
we rely on code review (mostly from Peter...).

To be able to add a code check to catch the future device added,
we need to first clean the (old) devices missing VMState, justifying
why each doesn't have any field to migrate.

Also IMO it is simpler to have an unified API, rather than explaining
each experienced and new contributor why "old style qdev" are allowed
to do things than "new introduced qdev" can't do anymore.

Regards,

Phil.



Re: [PATCH v3 2/2] hw/mips/loongson3_virt: Add TCG SMP support

2021-01-18 Thread Philippe Mathieu-Daudé
On 1/18/21 2:17 AM, Jiaxun Yang wrote:
> loongson3_virt has KVM SMP support in kenrel.
> This patch adds TCG SMP support by enable IPI controller
> for machine.
> 
> Also add definition about IRQs to enhance readability.
> 
> Note that TCG SMP can only support up to 4 CPUs as we
> didn't implement multi-node support.
> 
> Signed-off-by: Jiaxun Yang 
> ---
>  hw/mips/loongson3_bootp.h |  1 +
>  hw/mips/loongson3_virt.c  | 41 ---
>  hw/mips/Kconfig   |  1 +
>  3 files changed, 36 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 



Re: [PATCH v3 1/2] hw/intc: Add Loongson Inter Processor Interrupt controller

2021-01-18 Thread Philippe Mathieu-Daudé
Hi Jiaxun,

On 1/18/21 2:17 AM, Jiaxun Yang wrote:
> Loongson IPI controller is a MMIO based simple level triggered
> interrupt controller. It will trigger IRQ to it's upstream
> processor when set register is written.
> 
> It also has 4 64bit mailboxes to pass boot information to
> secondary processor.
> 
> Signed-off-by: Jiaxun Yang 
> ---
>  include/hw/intc/loongson_ipi.h |  20 
>  hw/intc/loongson_ipi.c | 174 +
>  hw/intc/Kconfig|   3 +
>  hw/intc/meson.build|   1 +
>  hw/intc/trace-events   |   4 +
>  5 files changed, 202 insertions(+)
>  create mode 100644 include/hw/intc/loongson_ipi.h
>  create mode 100644 hw/intc/loongson_ipi.c
...

> +static void loongson_ipi_init(Object *obj)
> +{
> +struct loongson_ipi *p = LOONGSON_IPI(obj);
> +
> +sysbus_init_irq(SYS_BUS_DEVICE(obj), >parent_irq);
> +
> +memory_region_init_io(>mmio, obj, _mmio_ops, p,
> +  "loongson.ipi", R_END * 4);
> +sysbus_init_mmio(SYS_BUS_DEVICE(obj), >mmio);
> +qemu_register_reset(ipi_reset, p);

You forgot to address 2 comments from v2:

- Use DeviceReset instead of qemu_register_reset()
- Missing VMState

https://www.mail-archive.com/qemu-devel@nongnu.org/msg772949.html

> +}
> +
> +static const TypeInfo loongson_ipi_info = {
> +.name  = TYPE_LOONGSON_IPI,
> +.parent= TYPE_SYS_BUS_DEVICE,
> +.instance_size = sizeof(struct loongson_ipi),
> +.instance_init = loongson_ipi_init,
> +};



Re: [PATCH v2 2/2] hw/mips/loongson3_virt: Add TCG SMP support

2021-01-17 Thread Philippe Mathieu-Daudé
On 1/17/21 11:46 PM, Philippe Mathieu-Daudé wrote:
> Hi Jiaxun,
> 
> On 1/14/21 2:31 AM, Jiaxun Yang wrote:
>> loongson3_virt has KVM SMP support in kenrel.
>> This patch adds TCG SMP support by enable IPI controller
>> for machine.
>>
>> Note that TCG SMP can only support up to 4 CPUs as we
>> didn't implement multi-node support.
>>
>> Signed-off-by: Jiaxun Yang 
>> ---
>>  hw/mips/loongson3_bootp.h |  1 +
>>  hw/mips/loongson3_virt.c  | 20 +++-
>>  hw/mips/Kconfig   |  1 +
>>  3 files changed, 21 insertions(+), 1 deletion(-)
...
>> +if (!kvm_enabled()) {
>> +/* IPI is handled by kernel for KVM */
>> +DeviceState *ipi;
>> +ipi = qdev_new(TYPE_LOONGSON_IPI);
>> +sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), _fatal);
>> +sysbus_mmio_map(SYS_BUS_DEVICE(ipi), 0,
>> +virt_memmap[VIRT_IPIS].base + IPI_REG_SPACE * 
>> i);
>> +sysbus_connect_irq(SYS_BUS_DEVICE(ipi), 0, cpu->env.irq[6]);
> 
> While this works, it is very fragile. If multiple IRQs share the same
> CPU pin, the better way is to use an OR gate (modeled as TYPE_OR_IRQ
> device).

Doh I misread, I thought it was a signal from a core to the IPI, but
this is a signal from each IPI to its CPU core, so this is good.

> 
>>  }
>>  
>>  for (ip = 0; ip < 4 ; ip++) {
>>  int pin = i * 4 + ip;
>>  sysbus_connect_irq(SYS_BUS_DEVICE(liointc),
>> pin, cpu->env.irq[ip + 2]);
> 
> Oops, we already use it without OR gate :/

Ditto, I misread, code is correct (4 outputs from LIOINTC to
4 input of CPU core).

> 
> Regards,
> 
> Phil.
> 



Re: [PATCH v2 2/2] hw/mips/loongson3_virt: Add TCG SMP support

2021-01-17 Thread Philippe Mathieu-Daudé
Hi Jiaxun,

On 1/14/21 2:31 AM, Jiaxun Yang wrote:
> loongson3_virt has KVM SMP support in kenrel.
> This patch adds TCG SMP support by enable IPI controller
> for machine.
> 
> Note that TCG SMP can only support up to 4 CPUs as we
> didn't implement multi-node support.
> 
> Signed-off-by: Jiaxun Yang 
> ---
>  hw/mips/loongson3_bootp.h |  1 +
>  hw/mips/loongson3_virt.c  | 20 +++-
>  hw/mips/Kconfig   |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mips/loongson3_bootp.h b/hw/mips/loongson3_bootp.h
> index 09f8480abf..4756aa44f6 100644
> --- a/hw/mips/loongson3_bootp.h
> +++ b/hw/mips/loongson3_bootp.h
> @@ -210,6 +210,7 @@ enum {
>  VIRT_PCIE_ECAM,
>  VIRT_BIOS_ROM,
>  VIRT_UART,
> +VIRT_IPIS,
>  VIRT_LIOINTC,
>  VIRT_PCIE_MMIO,
>  VIRT_HIGHMEM
> diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> index d4a82fa536..0684a035b0 100644
> --- a/hw/mips/loongson3_virt.c
> +++ b/hw/mips/loongson3_virt.c
> @@ -35,6 +35,7 @@
>  #include "hw/boards.h"
>  #include "hw/char/serial.h"
>  #include "hw/intc/loongson_liointc.h"
> +#include "hw/intc/loongson_ipi.h"
>  #include "hw/mips/mips.h"
>  #include "hw/mips/cpudevs.h"
>  #include "hw/mips/fw_cfg.h"
> @@ -59,6 +60,7 @@
>  
>  #define PM_CNTL_MODE  0x10
>  
> +#define LOONGSON_TCG_MAX_VCPUS  4
>  #define LOONGSON_MAX_VCPUS  16

A GS464V node has 4 cores, right? What about:

#define LOONGSON_VCPUS_PER_NODE 4

#define LOONGSON_NODES_MAX_TCG  1
#define LOONGSON_NODES_MAX_KVM  4

>  
>  /*
> @@ -71,6 +73,7 @@
>  #define UART_IRQ0
>  #define RTC_IRQ 1
>  #define PCIE_IRQ_BASE   2

Can you add a definition for the number 6? Maybe:

   #define IPI_IRQ 6

> +#define IPI_REG_SPACE   0x100
>  
>  const struct MemmapEntry virt_memmap[] = {
>  [VIRT_LOWMEM] =  { 0x,0x1000 },
> @@ -81,6 +84,7 @@ const struct MemmapEntry virt_memmap[] = {
>  [VIRT_PCIE_ECAM] =   { 0x1a00, 0x200 },
>  [VIRT_BIOS_ROM] ={ 0x1fc0,  0x20 },
>  [VIRT_UART] ={ 0x1fe001e0,   0x8 },
> +[VIRT_IPIS] ={ 0x3ff01000, 0x400 },
>  [VIRT_LIOINTC] = { 0x3ff01400,  0x64 },
>  [VIRT_PCIE_MMIO] =   { 0x4000,0x4000 },
>  [VIRT_HIGHMEM] = { 0x8000,   0x0 }, /* Variable */
> @@ -495,6 +499,10 @@ static void mips_loongson3_virt_init(MachineState 
> *machine)
>  error_report("Loongson-3/TCG needs cpu type Loongson-3A1000");
>  exit(1);
>  }
> +if (machine->smp.cpus > LOONGSON_TCG_MAX_VCPUS) {
> +error_report("Loongson-3/TCG supports up to 4 CPUs");
> +exit(1);
> +}
>  } else {
>  if (!machine->cpu_type) {
>  machine->cpu_type = MIPS_CPU_TYPE_NAME("Loongson-3A4000");
> @@ -545,7 +553,17 @@ static void mips_loongson3_virt_init(MachineState 
> *machine)
>  qemu_register_reset(main_cpu_reset, cpu);
>  
>  if (i >= 4) {
> -continue; /* Only node-0 can be connected to LIOINTC */
> +continue; /* Only node-0 can be connected to LIOINTC and IPI */
> +}
> +
> +if (!kvm_enabled()) {
> +/* IPI is handled by kernel for KVM */
> +DeviceState *ipi;
> +ipi = qdev_new(TYPE_LOONGSON_IPI);
> +sysbus_realize_and_unref(SYS_BUS_DEVICE(ipi), _fatal);
> +sysbus_mmio_map(SYS_BUS_DEVICE(ipi), 0,
> +virt_memmap[VIRT_IPIS].base + IPI_REG_SPACE * i);
> +sysbus_connect_irq(SYS_BUS_DEVICE(ipi), 0, cpu->env.irq[6]);

While this works, it is very fragile. If multiple IRQs share the same
CPU pin, the better way is to use an OR gate (modeled as TYPE_OR_IRQ
device).

>  }
>  
>  for (ip = 0; ip < 4 ; ip++) {
>  int pin = i * 4 + ip;
>  sysbus_connect_irq(SYS_BUS_DEVICE(liointc),
> pin, cpu->env.irq[ip + 2]);

Oops, we already use it without OR gate :/

Regards,

Phil.



[RFC PATCH v2 19/20] stubs/vmstate: Add VMSTATE_END_OF_LIST to vmstate_user_mode_cpu_dummy

2021-01-17 Thread Philippe Mathieu-Daudé
Add a name and end marker to the vmstate_user_mode_cpu_dummy variable.

Reported-by: Dr. David Alan Gilbert 
Signed-off-by: Philippe Mathieu-Daudé 
---
 stubs/vmstate.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index f561f9f39bd..1d0e03e233b 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -2,7 +2,12 @@
 #include "migration/vmstate.h"
 
 #if defined(CONFIG_USER_ONLY)
-const VMStateDescription vmstate_user_mode_cpu_dummy = {};
+const VMStateDescription vmstate_user_mode_cpu_dummy = {
+.name = "cpu_common_user",
+.fields = (VMStateField[]) {
+VMSTATE_END_OF_LIST()
+},
+};
 #endif
 
 const VMStateDescription vmstate_no_state_to_migrate = {
-- 
2.26.2




[RFC PATCH v2 18/20] hw/core/qdev: Display warning for devices missing migration state

2021-01-17 Thread Philippe Mathieu-Daudé
When built with --enable-qdev-debug, QEMU displays warnings
listing devices missing migration state:

  $ qemu-system-arm -S -M spitz
  qemu-system-arm: warning: missing migration state for type: 
'pxa270-c0-arm-cpu'
  qemu-system-arm: warning: missing migration state for type: 'serial'
  qemu-system-arm: warning: missing migration state for type: 'pxa2xx-pcmcia'
  qemu-system-arm: warning: missing migration state for type: 'pxa2xx-pcmcia'
  qemu-system-arm: warning: missing migration state for type: 'pxa2xx-i2c-slave'
  qemu-system-arm: warning: missing migration state for type: 'pxa2xx-i2c-slave'
  qemu-system-arm: warning: missing migration state for type: 'ads7846'
  qemu-system-arm: warning: missing migration state for type: 'max'

Signed-off-by: Philippe Mathieu-Daudé 
---
Unresolved issue:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg721700.html
Peter:
> I think where we'd like to get to is installing a migration
> blocker if the machine has any devices which don't have a vmsd.
> But for that we'd need to be pretty sure we'd got all the devices
> on machines where we care about migration, and we're clearly a
> fair way from that (eg we need to do something about the
> devices like the CPU which don't have a vmsd but handle their
> migration some other way so they don't trigger the condition
> for warning/migration-blocker).
---
 configure  | 10 ++
 meson.build|  1 +
 hw/core/qdev.c |  5 +
 3 files changed, 16 insertions(+)

diff --git a/configure b/configure
index 155dda124c2..984befbb99d 100755
--- a/configure
+++ b/configure
@@ -383,6 +383,7 @@ blobs="true"
 pkgversion=""
 pie=""
 qom_cast_debug="yes"
+qdev_debug="no"
 trace_backends="log"
 trace_file="trace"
 spice="$default_feature"
@@ -1005,6 +1006,10 @@ for opt do
   ;;
   --enable-qom-cast-debug) qom_cast_debug="yes"
   ;;
+  --disable-qdev-debug) qdev_debug="no"
+  ;;
+  --enable-qdev-debug) qdev_debug="yes"
+  ;;
   --disable-virtfs) virtfs="disabled"
   ;;
   --enable-virtfs) virtfs="enabled"
@@ -1048,6 +1053,7 @@ for opt do
   debug="yes"
   strip_opt="no"
   fortify_source="no"
+  qdev_debug="yes"
   ;;
   --enable-sanitizers) sanitizers="yes"
   ;;
@@ -5912,6 +5918,10 @@ if test "$qom_cast_debug" = "yes" ; then
   echo "CONFIG_QOM_CAST_DEBUG=y" >> $config_host_mak
 fi
 
+if test "$qdev_debug" = "yes" ; then
+  echo "CONFIG_QDEV_DEBUG=y" >> $config_host_mak
+fi
+
 echo "CONFIG_COROUTINE_BACKEND=$coroutine" >> $config_host_mak
 if test "$coroutine_pool" = "yes" ; then
   echo "CONFIG_COROUTINE_POOL=1" >> $config_host_mak
diff --git a/meson.build b/meson.build
index 3d889857a09..545c8f9f88b 100644
--- a/meson.build
+++ b/meson.build
@@ -2472,6 +2472,7 @@
 summary_info += {'TPM support':   config_host.has_key('CONFIG_TPM')}
 summary_info += {'libssh support':config_host.has_key('CONFIG_LIBSSH')}
 summary_info += {'QOM debugging': 
config_host.has_key('CONFIG_QOM_CAST_DEBUG')}
+summary_info += {'QDEV debugging':config_host.has_key('CONFIG_QDEV_DEBUG')}
 summary_info += {'Live block migration': 
config_host.has_key('CONFIG_LIVE_BLOCK_MIGRATION')}
 summary_info += {'lzo support':   lzo.found()}
 summary_info += {'snappy support':snappy.found()}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f0d0afd438d..9a73a242fa4 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -792,6 +792,11 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
_err) < 0) {
 goto post_realize_fail;
 }
+} else {
+#ifdef CONFIG_QDEV_DEBUG
+warn_report("missing migration state for type: '%s'",
+object_get_typename(OBJECT(dev)));
+#endif
 }
 
 /*
-- 
2.26.2




[RFC PATCH v2 14/20] hw/misc/unimp: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/misc/unimp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
index 6cfc5727f0b..e5ede95c124 100644
--- a/hw/misc/unimp.c
+++ b/hw/misc/unimp.c
@@ -81,6 +81,7 @@ static void unimp_class_init(ObjectClass *klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->realize = unimp_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 device_class_set_props(dc, unimp_properties);
 }
 
-- 
2.26.2




[RFC PATCH v2 16/20] hw/sparc64/sun4u: Mark devices with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
These devices don't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Add a more descriptive comment to keep a clear separation
between static property vs runtime changeable.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sparc64/sun4u.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 0fa13a73302..fdf0aa875be 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -84,12 +84,15 @@ struct hwdef {
 struct EbusState {
 /*< private >*/
 PCIDevice parent_obj;
+/*< public >*/
 
 ISABus *isa_bus;
 qemu_irq isa_bus_irqs[ISA_NUM_IRQS];
-uint64_t console_serial_base;
 MemoryRegion bar0;
 MemoryRegion bar1;
+
+/* Properties */
+uint64_t console_serial_base;
 };
 
 #define TYPE_EBUS "ebus"
@@ -386,6 +389,7 @@ static void ebus_class_init(ObjectClass *klass, void *data)
 k->device_id = PCI_DEVICE_ID_SUN_EBUS;
 k->revision = 0x01;
 k->class_id = PCI_CLASS_BRIDGE_OTHER;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 device_class_set_props(dc, ebus_properties);
 }
 
-- 
2.26.2




[RFC PATCH v2 10/20] hw/usb/hcd-ohci: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Add a more descriptive comment to keep a clear separation
between static property vs runtime changeable.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/usb/hcd-ohci.h | 2 ++
 hw/usb/hcd-ohci.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/usb/hcd-ohci.h b/hw/usb/hcd-ohci.h
index 11ac57058d1..fd4842a352f 100644
--- a/hw/usb/hcd-ohci.h
+++ b/hw/usb/hcd-ohci.h
@@ -101,6 +101,8 @@ struct OHCISysBusState {
 /*< public >*/
 
 OHCIState ohci;
+
+/* Properties */
 char *masterbus;
 uint32_t num_ports;
 uint32_t firstport;
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index f8c64c8b95b..302aab30992 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -2007,6 +2007,7 @@ static void ohci_sysbus_class_init(ObjectClass *klass, 
void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->realize = ohci_realize_pxa;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 set_bit(DEVICE_CATEGORY_USB, dc->categories);
 dc->desc = "OHCI USB Controller";
 device_class_set_props(dc, ohci_sysbus_properties);
-- 
2.26.2




[RFC PATCH v2 06/20] hw/arm/msf2-soc: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Add a more descriptive comment to keep a clear separation
between static property vs runtime changeable.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/arm/msf2-soc.h | 11 ++-
 hw/arm/msf2-soc.c |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/hw/arm/msf2-soc.h b/include/hw/arm/msf2-soc.h
index d4061846855..41a328c77f9 100644
--- a/include/hw/arm/msf2-soc.h
+++ b/include/hw/arm/msf2-soc.h
@@ -52,6 +52,12 @@ struct MSF2State {
 
 ARMv7MState armv7m;
 
+MSF2SysregState sysreg;
+MSSTimerState timer;
+MSSSpiState spi[MSF2_NUM_SPIS];
+MSF2EmacState emac;
+
+/* Properties */
 char *cpu_type;
 char *part_name;
 uint64_t envm_size;
@@ -60,11 +66,6 @@ struct MSF2State {
 uint32_t m3clk;
 uint8_t apb0div;
 uint8_t apb1div;
-
-MSF2SysregState sysreg;
-MSSTimerState timer;
-MSSSpiState spi[MSF2_NUM_SPIS];
-MSF2EmacState emac;
 };
 
 #endif
diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index d2c29e82d13..2d163710f54 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -224,6 +224,7 @@ static void m2sxxx_soc_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->realize = m2sxxx_soc_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 device_class_set_props(dc, m2sxxx_soc_properties);
 }
 
-- 
2.26.2




[RFC PATCH v2 17/20] hw/pci-host/gpex: Mark device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
TYPE_GPEX_HOST does not have internal state to migrate.
Its only interesting state is in the GPEXRootState, which
is a TYPE_GPEX_ROOT_DEVICE which migrates itself.
Explicit there is nothing to migrate by using the special
vmstate_qdev_no_state_to_migrate.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/pci-host/gpex.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
index 2bdbe7b4561..2565dc27ae4 100644
--- a/hw/pci-host/gpex.c
+++ b/hw/pci-host/gpex.c
@@ -115,6 +115,7 @@ static void gpex_host_class_init(ObjectClass *klass, void 
*data)
 
 hc->root_bus_path = gpex_host_root_bus_path;
 dc->realize = gpex_host_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 dc->fw_name = "pci";
 }
-- 
2.26.2




[RFC PATCH v2 05/20] hw/arm/bcm283x: Mark devices with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
These devices don't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Add a more descriptive comment to keep a clear separation
between static property vs runtime changeable.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/arm/bcm2836.h | 5 +++--
 hw/arm/bcm2835_peripherals.c | 1 +
 hw/arm/bcm2836.c | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h
index 6f90cabfa3a..becb6cfd0a7 100644
--- a/include/hw/arm/bcm2836.h
+++ b/include/hw/arm/bcm2836.h
@@ -35,13 +35,14 @@ struct BCM283XState {
 DeviceState parent_obj;
 /*< public >*/
 
-uint32_t enabled_cpus;
-
 struct {
 ARMCPU core;
 } cpu[BCM283X_NCPUS];
 BCM2836ControlState control;
 BCM2835PeripheralState peripherals;
+
+/* Properties */
+uint32_t enabled_cpus;
 };
 
 #endif /* BCM2836_H */
diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index dcff13433e5..8cf85f028fd 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -386,6 +386,7 @@ static void bcm2835_peripherals_class_init(ObjectClass *oc, 
void *data)
 DeviceClass *dc = DEVICE_CLASS(oc);
 
 dc->realize = bcm2835_peripherals_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 }
 
 static const TypeInfo bcm2835_peripherals_type_info = {
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index de7ade2878e..d2de99147cc 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -176,6 +176,7 @@ static void bcm283x_class_init(ObjectClass *oc, void *data)
 
 /* Reason: Must be wired up in code (see raspi_init() function) */
 dc->user_creatable = false;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 }
 
 static void bcm2835_class_init(ObjectClass *oc, void *data)
-- 
2.26.2




[RFC PATCH v2 09/20] hw/cpu/cluster: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/cpu/cluster.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c
index e444b7c29d1..95653a643ad 100644
--- a/hw/cpu/cluster.c
+++ b/hw/cpu/cluster.c
@@ -80,6 +80,7 @@ static void cpu_cluster_class_init(ObjectClass *klass, void 
*data)
 
 device_class_set_props(dc, cpu_cluster_properties);
 dc->realize = cpu_cluster_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 
 /* This is not directly for users, CPU children must be attached by code */
 dc->user_creatable = false;
-- 
2.26.2




[RFC PATCH v2 20/20] migration/vmstate: Simplify vmstate for user-mode CPU

2021-01-17 Thread Philippe Mathieu-Daudé
User-mode wants an empty vmstate for the CPUs. We can
use the generic vmstate_no_state_to_migrate object which
is the same.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/core/cpu.h   | 2 +-
 include/migration/vmstate.h | 3 ---
 stubs/vmstate.c | 9 -
 3 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index c79a58db9b9..01e75cc7403 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -1132,7 +1132,7 @@ bool target_words_bigendian(void);
 #ifdef CONFIG_SOFTMMU
 extern const VMStateDescription vmstate_cpu_common;
 #else
-#define vmstate_cpu_common vmstate_user_mode_cpu_dummy
+#define vmstate_cpu_common vmstate_no_state_to_migrate
 #endif
 
 #define VMSTATE_CPU() { \
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 50559598eac..dfe20b5caa1 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -194,9 +194,6 @@ struct VMStateDescription {
 const VMStateDescription **subsections;
 };
 
-#if defined(CONFIG_USER_ONLY)
-extern const VMStateDescription vmstate_user_mode_cpu_dummy;
-#endif
 extern const VMStateDescription vmstate_no_state_to_migrate;
 
 extern const VMStateInfo vmstate_info_bool;
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index 1d0e03e233b..c360a929f60 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -1,15 +1,6 @@
 #include "qemu/osdep.h"
 #include "migration/vmstate.h"
 
-#if defined(CONFIG_USER_ONLY)
-const VMStateDescription vmstate_user_mode_cpu_dummy = {
-.name = "cpu_common_user",
-.fields = (VMStateField[]) {
-VMSTATE_END_OF_LIST()
-},
-};
-#endif
-
 const VMStateDescription vmstate_no_state_to_migrate = {
 .name = "empty-state",
 .fields = (VMStateField[]) {
-- 
2.26.2




[RFC PATCH v2 15/20] hw/nubus/mac-nubus-bridge: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/nubus/mac-nubus-bridge.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index 7c329300b82..ede36ccc5dd 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -27,6 +27,7 @@ static void mac_nubus_bridge_class_init(ObjectClass *klass, 
void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->desc = "Nubus bridge";
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 }
 
 static const TypeInfo mac_nubus_bridge_info = {
-- 
2.26.2




[RFC PATCH v2 08/20] hw/cpu/a9mpcore: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Add a more descriptive comment to keep a clear separation
between static property vs runtime changeable.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/cpu/a9mpcore.h | 3 ++-
 hw/cpu/a9mpcore.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
index e0396ab6af7..234ac13be2c 100644
--- a/include/hw/cpu/a9mpcore.h
+++ b/include/hw/cpu/a9mpcore.h
@@ -25,10 +25,11 @@ struct A9MPPrivState {
 SysBusDevice parent_obj;
 /*< public >*/
 
+/* Properties */
 uint32_t num_cpu;
-MemoryRegion container;
 uint32_t num_irq;
 
+MemoryRegion container;
 A9SCUState scu;
 GICState gic;
 A9GTimerState gtimer;
diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index d03f57e579b..2e1d2d46b5b 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -175,6 +175,7 @@ static void a9mp_priv_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->realize = a9mp_priv_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 device_class_set_props(dc, a9mp_priv_properties);
 }
 
-- 
2.26.2




[RFC PATCH v2 04/20] hw/arm/aspeed_soc: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/aspeed_soc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 7eefd54ac07..b503d32fef6 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -407,6 +407,7 @@ static void aspeed_soc_class_init(ObjectClass *oc, void 
*data)
 dc->realize = aspeed_soc_realize;
 /* Reason: Uses serial_hds and nd_table in realize() directly */
 dc->user_creatable = false;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 device_class_set_props(dc, aspeed_soc_properties);
 }
 
-- 
2.26.2




[RFC PATCH v2 13/20] hw/misc/iotkit-sysinfo: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/misc/iotkit-sysinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/iotkit-sysinfo.c b/hw/misc/iotkit-sysinfo.c
index b2dcfc4376c..8bb9a2ef8b2 100644
--- a/hw/misc/iotkit-sysinfo.c
+++ b/hw/misc/iotkit-sysinfo.c
@@ -120,6 +120,7 @@ static void iotkit_sysinfo_class_init(ObjectClass *klass, 
void *data)
  * This device has no guest-modifiable state and so it
  * does not need a reset function or VMState.
  */
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 
 device_class_set_props(dc, iotkit_sysinfo_props);
 }
-- 
2.26.2




[RFC PATCH v2 03/20] hw/arm/armv7m: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
The TYPE_BITBAND device doesn't have fields to migrate.
Be explicit by using vmstate_qdev_no_state_to_migrate.

Reviewed-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Reworded (Peter)
---
 hw/arm/armv7m.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 8224d4ade9f..41ac1b88ab4 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -347,6 +347,7 @@ static void bitband_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->realize = bitband_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 device_class_set_props(dc, bitband_properties);
 }
 
-- 
2.26.2




[RFC PATCH v2 01/20] migration/vmstate: Restrict vmstate_dummy to user-mode

2021-01-17 Thread Philippe Mathieu-Daudé
'vmstate_dummy' is special and only used for user-mode. Rename
it to something more specific.
It was introduced restricted to user-mode in commit c71c3e99b8
("Add a vmstate_dummy struct for CONFIG_USER_ONLY") but this
restriction was later removed in commit 6afc14e92ac ("migration:
Fix warning caused by missing declaration of vmstate_dummy").
Avoid the missing declaration warning by adding a stub for the
symbol, and restore the #ifdef'ry.

Suggested-by: Daniel Berrangé 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/core/cpu.h   | 2 +-
 include/migration/vmstate.h | 4 +++-
 stubs/vmstate.c | 4 +++-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 140fa32a5e3..c79a58db9b9 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -1132,7 +1132,7 @@ bool target_words_bigendian(void);
 #ifdef CONFIG_SOFTMMU
 extern const VMStateDescription vmstate_cpu_common;
 #else
-#define vmstate_cpu_common vmstate_dummy
+#define vmstate_cpu_common vmstate_user_mode_cpu_dummy
 #endif
 
 #define VMSTATE_CPU() { \
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 075ee800960..dda65c9987d 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -194,7 +194,9 @@ struct VMStateDescription {
 const VMStateDescription **subsections;
 };
 
-extern const VMStateDescription vmstate_dummy;
+#if defined(CONFIG_USER_ONLY)
+extern const VMStateDescription vmstate_user_mode_cpu_dummy;
+#endif
 
 extern const VMStateInfo vmstate_info_bool;
 
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index cc4fe41dfc2..8da777a1fb4 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -1,7 +1,9 @@
 #include "qemu/osdep.h"
 #include "migration/vmstate.h"
 
-const VMStateDescription vmstate_dummy = {};
+#if defined(CONFIG_USER_ONLY)
+const VMStateDescription vmstate_user_mode_cpu_dummy = {};
+#endif
 
 int vmstate_register_with_alias_id(VMStateIf *obj,
uint32_t instance_id,
-- 
2.26.2




[RFC PATCH v2 11/20] hw/intc/arm_gicv2m: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Add a more descriptive comment to keep a clear separation
between static property vs runtime changeable.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/intc/arm_gicv2m.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
index d564b857eba..664cc9fb032 100644
--- a/hw/intc/arm_gicv2m.c
+++ b/hw/intc/arm_gicv2m.c
@@ -55,6 +55,7 @@ struct ARMGICv2mState {
 MemoryRegion iomem;
 qemu_irq spi[GICV2M_NUM_SPI_MAX];
 
+/* Properties */
 uint32_t base_spi;
 uint32_t num_spi;
 };
@@ -182,6 +183,7 @@ static void gicv2m_class_init(ObjectClass *klass, void 
*data)
 
 device_class_set_props(dc, gicv2m_properties);
 dc->realize = gicv2m_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 }
 
 static const TypeInfo gicv2m_info = {
-- 
2.26.2




[RFC PATCH v2 12/20] hw/misc/armsse-cpuid: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/misc/armsse-cpuid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/armsse-cpuid.c b/hw/misc/armsse-cpuid.c
index d58138dc28c..61251d538b9 100644
--- a/hw/misc/armsse-cpuid.c
+++ b/hw/misc/armsse-cpuid.c
@@ -115,6 +115,7 @@ static void armsse_cpuid_class_init(ObjectClass *klass, 
void *data)
  * This device has no guest-modifiable state and so it
  * does not need a reset function or VMState.
  */
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 
 device_class_set_props(dc, armsse_cpuid_props);
 }
-- 
2.26.2




[RFC PATCH v2 07/20] hw/core/split-irq: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
This device doesn't have fields to migrate. Be explicit
by using vmstate_qdev_no_state_to_migrate.

Reviewed-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/core/split-irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/core/split-irq.c b/hw/core/split-irq.c
index 3b90af2e8f9..a7072f922cd 100644
--- a/hw/core/split-irq.c
+++ b/hw/core/split-irq.c
@@ -71,6 +71,7 @@ static void split_irq_class_init(ObjectClass *klass, void 
*data)
 /* No state to reset or migrate */
 device_class_set_props(dc, split_irq_properties);
 dc->realize = split_irq_realize;
+dc->vmsd = vmstate_qdev_no_state_to_migrate;
 
 /* Reason: Needs to be wired up to work */
 dc->user_creatable = false;
-- 
2.26.2




[RFC PATCH v2 02/20] hw/core/qdev: Add vmstate_qdev_no_state_to_migrate

2021-01-17 Thread Philippe Mathieu-Daudé
Add vmstate_qdev_no_state_to_migrate, which is simply a
pointer to vmstate_no_state_to_migrate. This way all
qdev devices (including "hw/qdev-core.h") don't have to
include "migration/vmstate.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
Unresolved issues:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg721695.html
Peter:
> Does this definitely not put any data into the migration stream?
> We don't want to change what's on the wire for machines that
> use devices that start using this. (If it does by default, it
> would be easy to make the migration code special case the
> magic symbol to act like "no vmsd specified").

https://www.mail-archive.com/qemu-devel@nongnu.org/msg727634.html
Dave:
> I'd need to test it to be sure, but I think if we added a .needed
> to vmstate_no_state_to_migrate with a function that always returned
> false, then I think the stream would stay unchanged.
---
 include/hw/qdev-core.h  | 2 ++
 include/migration/vmstate.h | 1 +
 hw/core/qdev.c  | 3 +++
 migration/vmstate.c | 7 +++
 stubs/vmstate.c | 7 +++
 5 files changed, 20 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bafc311bfa1..d2c7a46e6a2 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -140,6 +140,8 @@ struct DeviceClass {
 const char *bus_type;
 };
 
+extern const VMStateDescription *vmstate_qdev_no_state_to_migrate;
+
 typedef struct NamedGPIOList NamedGPIOList;
 
 struct NamedGPIOList {
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index dda65c9987d..50559598eac 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -197,6 +197,7 @@ struct VMStateDescription {
 #if defined(CONFIG_USER_ONLY)
 extern const VMStateDescription vmstate_user_mode_cpu_dummy;
 #endif
+extern const VMStateDescription vmstate_no_state_to_migrate;
 
 extern const VMStateInfo vmstate_info_bool;
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a9..f0d0afd438d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -44,6 +44,9 @@
 static bool qdev_hot_added = false;
 bool qdev_hot_removed = false;
 
+const VMStateDescription *vmstate_qdev_no_state_to_migrate =
+_no_state_to_migrate;
+
 const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
 {
 DeviceClass *dc = DEVICE_GET_CLASS(dev);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 05f87cdddc5..2c373774dfa 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -20,6 +20,13 @@
 #include "qemu/error-report.h"
 #include "trace.h"
 
+const VMStateDescription vmstate_no_state_to_migrate = {
+.name = "empty-state",
+.fields = (VMStateField[]) {
+VMSTATE_END_OF_LIST()
+}
+};
+
 static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc);
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index 8da777a1fb4..f561f9f39bd 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -5,6 +5,13 @@
 const VMStateDescription vmstate_user_mode_cpu_dummy = {};
 #endif
 
+const VMStateDescription vmstate_no_state_to_migrate = {
+.name = "empty-state",
+.fields = (VMStateField[]) {
+VMSTATE_END_OF_LIST()
+}
+};
+
 int vmstate_register_with_alias_id(VMStateIf *obj,
uint32_t instance_id,
const VMStateDescription *vmsd,
-- 
2.26.2




[RFC PATCH v2 00/20] hw: Mark the device with no migratable fields

2021-01-17 Thread Philippe Mathieu-Daudé
Since v1:
- Tried to address Dave and Daniel comments
- Added Peter R-b
- Handle GPEX device

This is a proof-of-concept after chatting with Peter Maydell
on IRC last year.

Introduce the vmstate_no_state_to_migrate structure, and
a reference to it: vmstate_qdev_no_state_to_migrate.
Use this reference in devices with no fields to migrate.

This is useful to catch devices missing vmstate, such:
- ads7846
- mcf-uart
- mcf-fec
- versatile_i2c
- ...

v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg719788.html

Philippe Mathieu-Daudé (20):
  migration/vmstate: Restrict vmstate_dummy to user-mode
  hw/core/qdev: Add vmstate_qdev_no_state_to_migrate
  hw/arm/armv7m: Mark the device with no migratable fields
  hw/arm/aspeed_soc: Mark the device with no migratable fields
  hw/arm/bcm283x: Mark devices with no migratable fields
  hw/arm/msf2-soc: Mark the device with no migratable fields
  hw/core/split-irq: Mark the device with no migratable fields
  hw/cpu/a9mpcore: Mark the device with no migratable fields
  hw/cpu/cluster: Mark the device with no migratable fields
  hw/usb/hcd-ohci: Mark the device with no migratable fields
  hw/intc/arm_gicv2m: Mark the device with no migratable fields
  hw/misc/armsse-cpuid: Mark the device with no migratable fields
  hw/misc/iotkit-sysinfo: Mark the device with no migratable fields
  hw/misc/unimp: Mark the device with no migratable fields
  hw/nubus/mac-nubus-bridge: Mark the device with no migratable fields
  hw/sparc64/sun4u: Mark devices with no migratable fields
  hw/pci-host/gpex: Mark device with no migratable fields
  hw/core/qdev: Display warning for devices missing migration state
  stubs/vmstate: Add VMSTATE_END_OF_LIST to vmstate_user_mode_cpu_dummy
  migration/vmstate: Simplify vmstate for user-mode CPU

 configure| 10 ++
 meson.build  |  1 +
 hw/usb/hcd-ohci.h|  2 ++
 include/hw/arm/bcm2836.h |  5 +++--
 include/hw/arm/msf2-soc.h| 11 ++-
 include/hw/core/cpu.h|  2 +-
 include/hw/cpu/a9mpcore.h|  3 ++-
 include/hw/qdev-core.h   |  2 ++
 include/migration/vmstate.h  |  2 +-
 hw/arm/armv7m.c  |  1 +
 hw/arm/aspeed_soc.c  |  1 +
 hw/arm/bcm2835_peripherals.c |  1 +
 hw/arm/bcm2836.c |  1 +
 hw/arm/msf2-soc.c|  1 +
 hw/core/qdev.c   |  8 
 hw/core/split-irq.c  |  1 +
 hw/cpu/a9mpcore.c|  1 +
 hw/cpu/cluster.c |  1 +
 hw/intc/arm_gicv2m.c |  2 ++
 hw/misc/armsse-cpuid.c   |  1 +
 hw/misc/iotkit-sysinfo.c |  1 +
 hw/misc/unimp.c  |  1 +
 hw/nubus/mac-nubus-bridge.c  |  1 +
 hw/pci-host/gpex.c   |  1 +
 hw/sparc64/sun4u.c   |  6 +-
 hw/usb/hcd-ohci.c|  1 +
 migration/vmstate.c  |  7 +++
 stubs/vmstate.c  |  7 ++-
 28 files changed, 70 insertions(+), 12 deletions(-)

-- 
2.26.2




Re: [PATCH 1/4] tests/qtest: Remove TPM tests

2021-01-17 Thread Philippe Mathieu-Daudé
On 1/17/21 7:47 PM, Paolo Bonzini wrote:
> On 15/01/21 16:09, Philippe Mathieu-Daudé wrote:
>> |The TPM tests are failing, and no further tests are run, making the
>> rest of the testsuite pointless:|
> 
> Just use -k when running tests, it's a good idea in general.

Yes, this used to be the default. I still see it in the
Meson conversion in commit a2ce7dbd917 ("meson: convert
ests/qtest to meson"), see tests/qtest/meson.build:

265 test('qtest-@0@/@1@'.format(target_base, test),
266  qtest_executables[test],
267  depends: [test_deps, qtest_emulator],
268  env: qtest_env,
269  args: ['--tap', '-k'],
270  protocol: 'tap',
271  suite: ['qtest', 'qtest-' + target_base])
272   endforeach
273 endforeach

Not sure what is going on.




Re: [PATCH-for-5.2 2/2] gitlab-ci: Avoid running the EDK2 job when not necessary

2021-01-17 Thread Philippe Mathieu-Daudé
On 11/11/20 10:18 AM, Philippe Mathieu-Daudé wrote:
> On 11/10/20 4:35 PM, Daniel P. Berrangé wrote:
>> On Tue, Nov 10, 2020 at 01:16:06PM +0100, Philippe Mathieu-Daudé wrote:
>>> The EDK2 jobs use the 'changes' keyword, which "makes it
>>> possible to define if a job should be created based on files
>>> modified by a Git push event." (see [1]). This keyword comes
>>> with a warning:
>>>
>>>   Caution:
>>>
>>> In pipelines with sources other than the three above
>>> changes can’t determine if a given file is new or old
>>> and always returns true."
>>>
>>> In commit 922febe2af we moved the YAML config file from the
>>> repository root directory to the .gitlab-ci.d/ directory.
>>>
>>> We didn't respect the previous warning and disabled the
>>> 'changes' filter rule, as the files are now in a (directory)
>>> three above the YAML config file.
>>
>> This description is a bit wierd. I don't see how the location
>> in the directory tree has any relevance here.
>>
>> IIUC the caution docs quoted above are referring to what triggered 
>> the pipeline. They're saying that if the trigger was not a "branch", 
>> "merge request", or "external pull request", then the "changes" rule 
>> always evaluates true.
>>
>> The "branch" source us a bit wierd though, as I'm not seeing
>> how gitlab figures out which commits are "new" to the pipeline
>> and thus whether the files were modified or not.
>>
>> Strangely qemu-project/qemu CI for master seems to be behaving
>> correctly and skipping the jobs.
> 
> What about this one?
> 
> https://gitlab.com/berrange/qemu/-/jobs/827459510

Ping?

> 
>>
>> Something is fishy here and clearly not working, so clearly
>> changes are needed, but the commit message is not explaining
>> it for me.
>>
>>> This jobs takes ~40min, and needlessly burns the 2000 minutes
>>> available to GitLab free users. Follow the recommendations in
>>> [3] and disable this job by default (except if we push a tag
>>> or the branch contains 'edk2'). Note we do not remove the job
>>> from the pipeline, it can still be triggered manually from the
>>> WebUI.




Re: [RFC PATCH 18/18] hw/core/qdev: Display warning for devices missing migration state

2021-01-17 Thread Philippe Mathieu-Daudé
On 7/9/20 9:14 PM, Peter Maydell wrote:
> On Fri, 3 Jul 2020 at 21:19, Philippe Mathieu-Daudé  wrote:
>>
>> When built with --enable-qdev-debug, QEMU displays warnings
>> listing devices missing migration state:
>>
>>   $ qemu-system-arm -S -M spitz
>>   qemu-system-arm: warning: missing migration state for type: 
>> 'pxa270-c0-arm-cpu'
>>   qemu-system-arm: warning: missing migration state for type: 'serial'
>>   qemu-system-arm: warning: missing migration state for type: 'pxa2xx-pcmcia'
>>   qemu-system-arm: warning: missing migration state for type: 'pxa2xx-pcmcia'
>>   qemu-system-arm: warning: missing migration state for type: 
>> 'pxa2xx-i2c-slave'
>>   qemu-system-arm: warning: missing migration state for type: 
>> 'pxa2xx-i2c-slave'
>>   qemu-system-arm: warning: missing migration state for type: 'ads7846'
>>   qemu-system-arm: warning: missing migration state for type: 'max'
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>> RFC because there might be something simpler than --enable-qdev-debug.
> 
> I think where we'd like to get to is installing a migration
> blocker if the machine has any devices which don't have a vmsd.
> But for that we'd need to be pretty sure we'd got all the devices
> on machines where we care about migration, and we're clearly a
> fair way from that (eg we need to do something about the
> devices like the CPU which don't have a vmsd but handle their
> migration some other way so they don't trigger the condition
> for warning/migration-blocker).

Dave made a comment about it, I'd rather let him have a look.

> I don't have a strong objection to this --enable-qdev-debug, I guess.
> Another option halfway between this and a full migration-blocker
> would be do a warn_report() for the relevant devices when savevm
> tries to migrate them.

OK. The problem is vmstate_save_state() is not qdev specific, it
migrates a blob, which we can not report much about. I'll repost
using 2 warnings.

Thanks for your review,

Phil.



[PATCH v2] softmmu/physmem: Silence GCC 10 maybe-uninitialized error

2021-01-17 Thread Philippe Mathieu-Daudé
When building with GCC 10.2 configured with --extra-cflags=-Os, we get:

  softmmu/physmem.c: In function ‘address_space_translate_for_iotlb’:
  softmmu/physmem.c:643:26: error: ‘notifier’ may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
643 | notifier->active = true;
|  ^
  softmmu/physmem.c:608:23: note: ‘notifier’ was declared here
608 | TCGIOMMUNotifier *notifier;
|   ^~~~

Initialize 'notifier' to silence the warning.

Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Remove pointless assert (Peter Maydell)

Yet another hole in our CI.

Supersedes: <20210117160754.4086411-1-f4...@amsat.org>
---
 softmmu/physmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 6301f4f0a5c..cdcd197656f 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -605,7 +605,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
  * when the IOMMU tells us the mappings we've cached have changed.
  */
 MemoryRegion *mr = MEMORY_REGION(iommu_mr);
-TCGIOMMUNotifier *notifier;
+TCGIOMMUNotifier *notifier = NULL;
 int i;
 
 for (i = 0; i < cpu->iommu_notifiers->len; i++) {
-- 
2.26.2




Re: [PATCH] softmmu/physmem: Hint notifier is not NULL in as_translate_for_iotlb()

2021-01-17 Thread Philippe Mathieu-Daudé
On 1/17/21 5:47 PM, Peter Maydell wrote:
> On Sun, 17 Jan 2021 at 16:07, Philippe Mathieu-Daudé  wrote:
>>
>> When using GCC 10.2 configured with --extra-cflags=-Os, we get:
>>
>>   softmmu/physmem.c: In function ‘address_space_translate_for_iotlb’:
>>   softmmu/physmem.c:643:26: error: ‘notifier’ may be used uninitialized in 
>> this function [-Werror=maybe-uninitialized]
>> 643 | notifier->active = true;
>> |  ^
>>   softmmu/physmem.c:608:23: note: ‘notifier’ was declared here
>> 608 | TCGIOMMUNotifier *notifier;
>> |   ^~~~
>>
>> Insert assertions as hint to the compiler that 'notifier' can
>> not be NULL there.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>> Yet another hole in our CI.
>> ---
>>  softmmu/physmem.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 6301f4f0a5c..65602ed548e 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -605,7 +605,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
>>   * when the IOMMU tells us the mappings we've cached have changed.
>>   */
>>  MemoryRegion *mr = MEMORY_REGION(iommu_mr);
>> -TCGIOMMUNotifier *notifier;
>> +TCGIOMMUNotifier *notifier = NULL;
>>  int i;
>>
>>  for (i = 0; i < cpu->iommu_notifiers->len; i++) {
>> @@ -638,6 +638,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
>>  memory_region_register_iommu_notifier(notifier->mr, >n,
>>_fatal);
>>  }
>> +assert(notifier != NULL);
>>
>>  if (!notifier->active) {
>>  notifier->active = true;
> 
> Is the assert() necessary to prevent the compiler complaining?
> Usually we don't bother if it's about to be dereferenced anyway.

Yes you are right, the assert() is not necessary. Simply initializing
the value silents the error.

Regards,

Phil.



[PATCH 4/6] accel/tcg: Declare missing cpu_loop_exit*() stubs

2021-01-17 Thread Philippe Mathieu-Daudé
cpu_loop_exit*() functions are declared in accel/tcg/cpu-exec-common.c,
and are not available when TCG accelerator is not built. Add stubs so
linking without TCG succeed.

Problematic files:

- hw/semihosting/console.c in qemu_semihosting_console_inc()
- hw/ppc/spapr_hcall.c in h_confer()
- hw/s390x/ipl.c in s390_ipl_reset_request()
- hw/misc/mips_itu.c

Signed-off-by: Philippe Mathieu-Daudé 
---
I suppose the s390x kvm-only build didn't catch this because
of compiler optimization:

in s390_ipl_reset_request():

640 if (tcg_enabled()) {
641 cpu_loop_exit(cs);
642 }

and "sysemu/tcg.h" is:

 13 #ifdef CONFIG_TCG
 14 extern bool tcg_allowed;
 15 #define tcg_enabled() (tcg_allowed)
 16 #else
 17 #define tcg_enabled() 0
 18 #endif
---
 accel/stubs/tcg-stub.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
index 8c18d3eabdd..2304606f8e0 100644
--- a/accel/stubs/tcg-stub.c
+++ b/accel/stubs/tcg-stub.c
@@ -28,3 +28,13 @@ void *probe_access(CPUArchState *env, target_ulong addr, int 
size,
  /* Handled by hardware accelerator. */
  g_assert_not_reached();
 }
+
+void QEMU_NORETURN cpu_loop_exit(CPUState *cpu)
+{
+g_assert_not_reached();
+}
+
+void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc)
+{
+g_assert_not_reached();
+}
-- 
2.26.2




  1   2   3   4   5   6   7   8   9   10   >