Re: [PATCH v3 0/9] Generalize memory encryption models

[Cornelia Huck]

On Mon, 22 Jun 2020 16:27:28 +0200
Christian Borntraeger  wrote:

> On 19.06.20 04:05, David Gibson wrote:
> > A number of hardware platforms are implementing mechanisms whereby the
> > hypervisor does not have unfettered access to guest memory, in order
> > to mitigate the security impact of a compromised hypervisor.
> > 
> > AMD's SEV implements this with in-cpu memory encryption, and Intel has
> > its own memory encryption mechanism.  POWER has an upcoming mechanism
> > to accomplish this in a different way, using a new memory protection
> > level plus a small trusted ultravisor.  s390 also has a protected
> > execution environment.
> > 
> > The current code (committed or draft) for these features has each
> > platform's version configured entirely differently.  That doesn't seem
> > ideal for users, or particularly for management layers.
> > 
> > AMD SEV introduces a notionally generic machine option
> > "machine-encryption", but it doesn't actually cover any cases other
> > than SEV.
> > 
> > This series is a proposal to at least partially unify configuration
> > for these mechanisms, by renaming and generalizing AMD's
> > "memory-encryption" property.  It is replaced by a
> > "host-trust-limitation" property pointing to a platform specific
> > object which configures and manages the specific details.
> > 
> > For now this series covers just AMD SEV and POWER PEF.  I'm hoping it
> > can be extended to cover the Intel and s390 mechanisms as well,
> > though.  
> 
> Let me try to summarize what I understand what you try to achieve:
> one command line parameter for all platforms that 
> 
> common across all platforms:
> - disable KSM
> - by default enables iommu_platform
> 
> 
> per platform:
> - setup the necessary encryption scheme when appropriate
> - block migration
> -
> 
> 
> The tricky part is certainly the per platform thing. For example on
> s390 we just have a cpumodel flag that provides interfaces to the guest
> to switch into protected mode via the ultravisor. This works perfectly
> fine with the host model, so no need to configure anything.  The platform
> code then disables KSM _on_switchover_ and not in general. Because the 
> guest CAN switch into protected, but it does not have to.
> 
> So this feels really hard to do right. Would a virtual BoF on KVM forum
> be too late? We had a BoF on protected guests last year and that was
> valuable.

Maybe we can do some kind of call to discuss this earlier? (Maybe in
the KVM call slot on Tuesdays?) I think it would be really helpful if
everybody would have at least a general understanding about how
encryption/protection works on the different architectures.

Re: [PULL 00/18] Block patches

[Max Reitz]

On 23.06.20 14:55, Peter Maydell wrote:
> On Mon, 22 Jun 2020 at 16:11, Max Reitz  wrote:
>>
>> The following changes since commit bae31bfa48b9caecee25da3d5333901a126a06b4:
>>
>>   Merge remote-tracking branch 
>> 'remotes/kraxel/tags/audio-20200619-pull-request' into staging (2020-06-19 
>> 22:56:59 +0100)
>>
>> are available in the Git repository at:
>>
>>   https://github.com/XanClic/qemu.git tags/pull-block-2020-06-22
>>
>> for you to fetch changes up to 74c55e4142a7bb835c38d3770c74210cbb1e4fab:
>>
>>   iotests: don't test qcow2.py inside 291 (2020-06-22 16:05:23 +0200)
>>
>> 
>> Block patches:
>> - Support modifying a LUKS-encrypted image's keyslots
>> - iotest fixes
>>
>> 
> 
> Hi; I see various iotest failures, different things on
> PPC64 Linux, OpenBSD and FreeBSD, and on an AArch32 build
> that happens to not have optional crypto libs installed.

OK.  Sorry.  None of this looks easily fixable, so I suppose I’ll have
to drop everything but the last two patches for now.

> On PPC64 Linux, lots of iotests fail like this:
> 
>   TESTiotest-qcow2: 001 [fail]
> QEMU  --
> "/home/pm215/qemu/build/all/tests/qemu-iotests/../../ppc64-softmmu/qemu-system-ppc64"
> -nodefaults -display none -accel qtest
> QEMU_IMG  -- 
> "/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-img"
> QEMU_IO   --
> "/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-io"  --cache
> writeback --aio threads -f qcow2
> QEMU_NBD  -- 
> "/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-nbd"
> IMGFMT-- qcow2 (compat=1.1)
> IMGPROTO  -- file
> PLATFORM  -- Linux/ppc64 gcc1-power7 3.10.0-862.14.4.el7.ppc64
> TEST_DIR  -- /home/pm215/qemu/build/all/tests/qemu-iotests/scratch
> SOCK_DIR  -- /tmp/tmp.vvBdnkatyZ
> SOCKET_SCM_HELPER --
> /home/pm215/qemu/build/all/tests/qemu-iotests/socket_scm_helper
> 
> --- /home/pm215/qemu/tests/qemu-iotests/001.out 2015-04-08
> 18:43:24.908449234 +
> +++ /home/pm215/qemu/build/all/tests/qemu-iotests/001.out.bad
> 2020-06-23 10:42:29.262626202 +
> @@ -1,5 +1,6 @@
>  QA output created by 001
> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> +./common.filter: line 128: readarray: -d: invalid option
> +readarray: usage: readarray [-n count] [-O origin] [-s count] [-t]
> [-u fd] [-C callback] [-c quantum] [array]
> 
>  == reading whole image ==
>  read 134217728/134217728 bytes at offset 0
>   TESTiotest-qcow2: 002 [fail]
> QEMU  --
> "/home/pm215/qemu/build/all/tests/qemu-iotests/../../ppc64-softmmu/qemu-system-ppc64"
> -nodefaults -display none -accel qtest
> QEMU_IMG  -- 
> "/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-img"
> QEMU_IO   --
> "/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-io"  --cache
> writeback --aio threads -f qcow2
> QEMU_NBD  -- 
> "/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-nbd"
> IMGFMT-- qcow2 (compat=1.1)
> IMGPROTO  -- file
> PLATFORM  -- Linux/ppc64 gcc1-power7 3.10.0-862.14.4.el7.ppc64
> TEST_DIR  -- /home/pm215/qemu/build/all/tests/qemu-iotests/scratch
> SOCK_DIR  -- /tmp/tmp.vvBdnkatyZ
> SOCKET_SCM_HELPER --
> /home/pm215/qemu/build/all/tests/qemu-iotests/socket_scm_helper
> 
> Looks like you're trying to use a readarray option that doesn't
> exist (maybe only exists in newer shells?)

Yes, I am.  Well, that’s a real shame. :/

I wasn’t aware that readarray’s -d option was only introduced at some
later point.  Looks like that happened in 4.4, which was only released
2016.  So that’s probably too new indeed.

> iotests failures on aarch32 which happens to not have some
> optional crypto lib dependency installed I guess; these
> iotests ought to be made to skip if the functionality they're
> testing isn't compiled into this QEMU:

Right.  Maxim, can you do that?

>   TESTiotest-qcow2: 293 [fail]
> QEMU  --
> "/home/peter.maydell/qemu/build/all-a32/tests/qemu-iotests/../../aarch64-softmmu/qemu-system-aarch64"
> -nodefaults -display none -machine virt -accel qtest
> QEMU_IMG  --
> "/home/peter.maydell/qemu/build/all-a32/tests/qemu-iotests/../../qemu-img"
> QEMU_IO   --
> "/home/peter.maydell/qemu/build/all-a32/tests/qemu-iotests/../../qemu-io"
>  --cache writeback --aio threads -f qcow2
> QEMU_NBD  --
> "/home/peter.maydell/qemu/build/all-a32/tests/qemu-iotests/../../qemu-nbd"
> IMGFMT-- qcow2 (compat=1.1)
> IMGPROTO  -- file
> PLATFORM  -- Linux/aarch64 mustang-maydell 4.15.0-101-generic
> TEST_DIR  --
> /home/peter.maydell/qemu/build/all-a32/tests/qemu-iotests/scratch
> SOCK_DIR  -- /tmp/tmp.tjBWiNDvED
> SOCKET_SCM_HELPER --
> /home/peter.maydell/qemu/build/all-a32/tests/qemu-iotests/socket_scm_helper
> 
> --- /home/peter.maydell/qemu/tests/qemu-iotests/293.out 2020-06-23
> 10:38:50.091867725 +
> +++ /home/peter.maydell/qemu/build/all-a

Re: [PATCH] libqos: usb-hcd-ehci: use 32-bit write for config register

[Thomas Huth]

On 23/06/2020 18.18, Paolo Bonzini wrote:

The memory region ops have min_access_size == 4 so obey it.

Signed-off-by: Paolo Bonzini 
---
  tests/qtest/usb-hcd-ehci-test.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/usb-hcd-ehci-test.c b/tests/qtest/usb-hcd-ehci-test.c
index 5251d539e9..c51e8bb223 100644
--- a/tests/qtest/usb-hcd-ehci-test.c
+++ b/tests/qtest/usb-hcd-ehci-test.c
@@ -96,7 +96,7 @@ static void pci_ehci_port_1(void)
  static void pci_ehci_config(void)
  {
  /* hands over all ports from companion uhci to ehci */
-qpci_io_writew(ehci1.dev, ehci1.bar, 0x60, 1);
+qpci_io_writel(ehci1.dev, ehci1.bar, 0x60, 1);
  }
  
  static void pci_uhci_port_2(void)


Passes "make check-qtest-x86_64" on a s390x host, too:

Tested-by: Thomas Huth 

Re: [PATCH] libqos: pci-pc: use 32-bit write for EJ register

[Thomas Huth]

On 23/06/2020 18.18, Paolo Bonzini wrote:

The memory region ops have min_access_size == 4 so obey it.

Signed-off-by: Paolo Bonzini 
---
  tests/qtest/libqos/pci-pc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/pci-pc.c b/tests/qtest/libqos/pci-pc.c
index 0bc591d1da..3bb2eb3ba8 100644
--- a/tests/qtest/libqos/pci-pc.c
+++ b/tests/qtest/libqos/pci-pc.c
@@ -186,7 +186,7 @@ void qpci_unplug_acpi_device_test(QTestState *qts, const 
char *id, uint8_t slot)
  g_assert(!qdict_haskey(response, "error"));
  qobject_unref(response);
  
-qtest_outb(qts, ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);

+qtest_outl(qts, ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
  
  qtest_qmp_eventwait(qts, "DEVICE_DELETED");

  }


I was a little bit afraid that this could cause endianess issues on big 
endian hosts, but I gave it a try on a s390x machine and it seems to 
work fine.


Tested-by: Thomas Huth 

Re: [PATCH] fuzz: fix broken qtest check at rcu_disable_atfork

[Thomas Huth]

On 18/06/2020 18.05, Alexander Bulekov wrote:

The qtest_enabled check introduced in d6919e4 always returns false, as
it is called prior to configure_accelerators(). Instead of trying to
skip rcu_disable_atfork in qemu_main, simply call rcu_enable_atfork in
the fuzzer, after qemu_main returns.

Reported-by: Thomas Huth 
Signed-off-by: Alexander Bulekov 
---
  softmmu/vl.c| 12 +---
  tests/qtest/fuzz/fuzz.c |  3 +++
  2 files changed, 4 insertions(+), 11 deletions(-)


Thanks, queued to qtest-next now:

 https://gitlab.com/huth/qemu/-/commits/qtest-next/

 Thomas

Re: [PATCH] fuzz: do not use POSIX shm for coverage bitmap

[Thomas Huth]

On 22/06/2020 18.50, Alexander Bulekov wrote:

We used shm_open with mmap to share libfuzzer's coverage bitmap with
child (runner) processes. The same functionality can be achieved with
MAP_SHARED | MAP_ANONYMOUS, since we do not care about naming or
permissioning the shared memory object.

Signed-off-by: Alexander Bulekov 


Thanks, queued to qtest-next now:

 https://gitlab.com/huth/qemu/-/commits/qtest-next/

 Thomas

[PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addresses

[Janosch Frank]

The component entries written by zipl contain short PSWs, not
addresses. Let's mask them and only pass the address part to
jump_to_IPL_code(uint64_t address) because it expects an address as
visible by the name of the argument.

Signed-off-by: Janosch Frank 
---
 pc-bios/s390-ccw/bootmap.c | 5 +++--
 pc-bios/s390-ccw/bootmap.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 97205674e5..8547a140df 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -10,6 +10,7 @@
 
 #include "libc.h"
 #include "s390-ccw.h"
+#include "s390-arch.h"
 #include "bootmap.h"
 #include "virtio.h"
 #include "bswap.h"
@@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry)
 char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */
 
 blockno = entry->data.blockno;
-address = entry->load_address;
+address = entry->psw & PSW_MASK_SHORT_ADDR;
 
 debug_print_int("loading segment at block", blockno);
 debug_print_int("addr", address);
@@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
 IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
 
 /* should not return */
-jump_to_IPL_code(entry->load_address);
+jump_to_IPL_code(entry->psw & PSW_MASK_SHORT_ADDR);
 }
 
 static void ipl_scsi(void)
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 12a0166aae..e07f87e690 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -68,7 +68,7 @@ typedef struct ComponentEntry {
 ScsiBlockPtr data;
 uint8_t pad[7];
 uint8_t component_type;
-uint64_t load_address;
+uint64_t psw;
 } __attribute((packed)) ComponentEntry;
 
 typedef struct ComponentHeader {
-- 
2.25.1

[PATCH v5 03/12] pc-bios: s390x: Move sleep and yield to helper.h

[Janosch Frank]

They are definitely helper functions.

Signed-off-by: Janosch Frank 
Reviewed-by: Christian Borntraeger 
Reviewed-by: Thomas Huth 
Reviewed-by: David Hildenbrand 
---
 pc-bios/s390-ccw/helper.h  | 17 +
 pc-bios/s390-ccw/s390-ccw.h| 18 --
 pc-bios/s390-ccw/virtio-net.c  |  1 +
 pc-bios/s390-ccw/virtio-scsi.c |  1 +
 4 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index 78d5bc7442..32a453b634 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -14,6 +14,7 @@
 #define S390_CCW_HELPER_H
 
 #include "s390-ccw.h"
+#include "s390-time.h"
 
 /* Avoids compiler warnings when casting a pointer to a u32 */
 static inline uint32_t ptr2u32(void *ptr)
@@ -28,4 +29,20 @@ static inline void *u32toptr(uint32_t n)
 return (void *)(uint64_t)n;
 }
 
+static inline void yield(void)
+{
+asm volatile ("diag 0,0,0x44"
+  : :
+  : "memory", "cc");
+}
+
+static inline void sleep(unsigned int seconds)
+{
+ulong target = get_time_seconds() + seconds;
+
+while (get_time_seconds() < target) {
+yield();
+}
+}
+
 #endif
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index fae1de363f..c5820e43ae 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -142,26 +142,8 @@ static inline void debug_print_addr(const char *desc, void 
*p)
 #define KVM_S390_VIRTIO_SET_STATUS  2
 #define KVM_S390_VIRTIO_CCW_NOTIFY  3
 
-static inline void yield(void)
-{
-asm volatile ("diag 0,0,0x44"
-  : :
-  : "memory", "cc");
-}
-
 #define MAX_SECTOR_SIZE 4096
 
-#include "s390-time.h"
-
-static inline void sleep(unsigned int seconds)
-{
-ulong target = get_time_seconds() + seconds;
-
-while (get_time_seconds() < target) {
-yield();
-}
-}
-
 static inline void IPL_assert(bool term, const char *message)
 {
 if (!term) {
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index a13f3b6fb9..2fcb0a58c5 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -20,6 +20,7 @@
 #include "s390-ccw.h"
 #include "virtio.h"
 #include "s390-time.h"
+#include "helper.h"
 
 #ifndef DEBUG_VIRTIO_NET
 #define DEBUG_VIRTIO_NET 0
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 7bf0be4ffa..eddfb8a7ad 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -15,6 +15,7 @@
 #include "scsi.h"
 #include "virtio-scsi.h"
 #include "s390-time.h"
+#include "helper.h"
 
 static ScsiDevice default_scsi_device;
 static VirtioScsiCmdReq req;
-- 
2.25.1

[PATCH v5 02/12] pc-bios: s390x: Consolidate timing functions into time.h

[Janosch Frank]

Let's consolidate timing related functions into one header.

Signed-off-by: Janosch Frank 
Acked-by: Thomas Huth 
---
 pc-bios/s390-ccw/menu.c|  1 +
 pc-bios/s390-ccw/netmain.c | 15 +++
 pc-bios/s390-ccw/s390-ccw.h|  8 
 pc-bios/s390-ccw/s390-time.h   | 23 +++
 pc-bios/s390-ccw/virtio-net.c  |  1 +
 pc-bios/s390-ccw/virtio-scsi.c |  1 +
 pc-bios/s390-ccw/virtio.c  | 18 +++---
 7 files changed, 36 insertions(+), 31 deletions(-)
 create mode 100644 pc-bios/s390-ccw/s390-time.h

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index ce3815b201..de8260a5d6 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -12,6 +12,7 @@
 #include "libc.h"
 #include "s390-ccw.h"
 #include "sclp.h"
+#include "s390-time.h"
 
 #define KEYCODE_NO_INP '\0'
 #define KEYCODE_ESCAPE '\033'
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index 309ffa30d9..f1ee63577a 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -35,6 +35,7 @@
 #include "s390-ccw.h"
 #include "cio.h"
 #include "virtio.h"
+#include "s390-time.h"
 
 #define DEFAULT_BOOT_RETRIES 10
 #define DEFAULT_TFTP_RETRIES 20
@@ -57,24 +58,14 @@ static SubChannelId net_schid = { .one = 1 };
 static uint8_t mac[6];
 static uint64_t dest_timer;
 
-static uint64_t get_timer_ms(void)
-{
-uint64_t clk;
-
-asm volatile(" stck %0 " : : "Q"(clk) : "memory");
-
-/* Bit 51 is incremented each microsecond */
-return (clk >> (63 - 51)) / 1000;
-}
-
 void set_timer(int val)
 {
-dest_timer = get_timer_ms() + val;
+dest_timer = get_time_ms() + val;
 }
 
 int get_timer(void)
 {
-return dest_timer - get_timer_ms();
+return dest_timer - get_time_ms();
 }
 
 int get_sec_ticks(void)
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 21f27e7990..fae1de363f 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -74,8 +74,6 @@ unsigned long virtio_load_direct(ulong rec_list1, ulong 
rec_list2,
 bool virtio_is_supported(SubChannelId schid);
 void virtio_blk_setup_device(SubChannelId schid);
 int virtio_read(ulong sector, void *load_addr);
-u64 get_clock(void);
-ulong get_second(void);
 
 /* bootmap.c */
 void zipl_load(void);
@@ -153,11 +151,13 @@ static inline void yield(void)
 
 #define MAX_SECTOR_SIZE 4096
 
+#include "s390-time.h"
+
 static inline void sleep(unsigned int seconds)
 {
-ulong target = get_second() + seconds;
+ulong target = get_time_seconds() + seconds;
 
-while (get_second() < target) {
+while (get_time_seconds() < target) {
 yield();
 }
 }
diff --git a/pc-bios/s390-ccw/s390-time.h b/pc-bios/s390-ccw/s390-time.h
new file mode 100644
index 00..ed6d982371
--- /dev/null
+++ b/pc-bios/s390-ccw/s390-time.h
@@ -0,0 +1,23 @@
+#ifndef TIME_H
+#define TIME_H
+
+static inline u64 get_clock(void)
+{
+u64 r;
+
+asm volatile("stck %0" : "=Q" (r) : : "cc");
+return r;
+}
+
+static inline u64 get_time_ms(void)
+{
+/* Bit 51 is incremented each microsecond */
+return (get_clock() >> 12) / 1000;
+}
+
+static inline u64 get_time_seconds(void)
+{
+return get_time_ms() / 1000;
+}
+
+#endif
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index ff7f4dad25..a13f3b6fb9 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -19,6 +19,7 @@
 #include 
 #include "s390-ccw.h"
 #include "virtio.h"
+#include "s390-time.h"
 
 #ifndef DEBUG_VIRTIO_NET
 #define DEBUG_VIRTIO_NET 0
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 4fe4b9d261..7bf0be4ffa 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -14,6 +14,7 @@
 #include "virtio.h"
 #include "scsi.h"
 #include "virtio-scsi.h"
+#include "s390-time.h"
 
 static ScsiDevice default_scsi_device;
 static VirtioScsiCmdReq req;
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index fb40ca9828..ab49840db8 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -15,6 +15,7 @@
 #include "virtio-scsi.h"
 #include "bswap.h"
 #include "helper.h"
+#include "s390-time.h"
 
 #define VRING_WAIT_REPLY_TIMEOUT 30
 
@@ -157,19 +158,6 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
 }
 }
 
-u64 get_clock(void)
-{
-u64 r;
-
-asm volatile("stck %0" : "=Q" (r) : : "cc");
-return r;
-}
-
-ulong get_second(void)
-{
-return (get_clock() >> 12) / 100;
-}
-
 int vr_poll(VRing *vr)
 {
 if (vr->used->idx == vr->used_idx) {
@@ -194,7 +182,7 @@ int vr_poll(VRing *vr)
  */
 int vring_wait_reply(void)
 {
-ulong target_second = get_second() + vdev.wait_reply_timeout;
+ulong target_second = get_time_seconds() + vdev.wait_reply_timeout;
 
 /* Wait for any queue to be updated by the host */
 do {
@@ -207,7 +195,7 @@ int vring_wait_reply(void)
 if (r) {
 return 0;
 }
-} whil

[PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Janosch Frank]

The bios is in dire need for a cleanup as there are still a lot of
magic constants being used throughout as well as duplicated code.

In the first part of this series we consolidate constants and
functions, as well as doing some minor cleanups and fixes.

The patches are available here:
https://github.com/frankjaa/qemu/pull/new/cleanup_bios

v5:
* Fixed whitespace damage
* Removed reset PSW mask changes in dasd-ipl.c
* Added jump2ipl.c cleanup patches

v4:
* Renamed time.h to s390-time.h
* Fixed function names in sleep()
* Changed order of sense_id_ccw initialization
* Added missing include before sleep()

v3:
* Dropped 0x00 to 0x0/0 patch
* Moved some timing functions into helper.h instead of time.h
* Fixed IPL psw manipulation in dasd-ipl.c
* Minor cosmetic fixes found by review

v2:
* Included cio fixup to get rid of compile errors...
* Minor cosmetic fixes found by review


Janosch Frank (12):
  pc-bios: s390x: cio.c cleanup and compile fix
  pc-bios: s390x: Consolidate timing functions into time.h
  pc-bios: s390x: Move sleep and yield to helper.h
  pc-bios: s390x: Get rid of magic offsets into the lowcore
  pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
  pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
  pc-bios: s390x: Use PSW masks where possible and introduce
PSW_MASK_SHORT_ADDR
  pc-bios: s390x: Move panic() into header and add infinite loop
  pc-bios: s390x: Use ebcdic2ascii table
  pc-bios: s390x: Make u32 ptr check explicit
  pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
  pc-bios: s390x: Cleanup jump to ipl code

 pc-bios/s390-ccw/bootmap.c |  9 
 pc-bios/s390-ccw/bootmap.h |  2 +-
 pc-bios/s390-ccw/cio.c | 40 +++---
 pc-bios/s390-ccw/cio.h | 17 ++-
 pc-bios/s390-ccw/dasd-ipl.c|  3 ---
 pc-bios/s390-ccw/helper.h  | 19 +++-
 pc-bios/s390-ccw/jump2ipl.c| 35 -
 pc-bios/s390-ccw/main.c| 15 +++--
 pc-bios/s390-ccw/menu.c|  1 +
 pc-bios/s390-ccw/netmain.c | 23 +++
 pc-bios/s390-ccw/s390-arch.h   |  4 +++-
 pc-bios/s390-ccw/s390-ccw.h| 27 ++-
 pc-bios/s390-ccw/s390-time.h   | 23 +++
 pc-bios/s390-ccw/start.S   |  5 +++--
 pc-bios/s390-ccw/virtio-net.c  |  2 ++
 pc-bios/s390-ccw/virtio-scsi.c |  2 ++
 pc-bios/s390-ccw/virtio.c  | 18 +++
 17 files changed, 120 insertions(+), 125 deletions(-)
 create mode 100644 pc-bios/s390-ccw/s390-time.h

-- 
2.25.1

[PATCH v5 07/12] pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR

[Janosch Frank]

Let's move some of the PSW mask defines into s390-arch.h and use them
in jump2ipl.c. Also let's introduce a new constant for the address
mask of 8 byte (short) PSWs.

Signed-off-by: Janosch Frank 
Reviewed-by: David Hildenbrand 
---
 pc-bios/s390-ccw/jump2ipl.c  | 10 --
 pc-bios/s390-ccw/s390-arch.h |  2 ++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 4eba2510b0..767012bf0c 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -8,12 +8,10 @@
 
 #include "libc.h"
 #include "s390-ccw.h"
+#include "s390-arch.h"
 
 #define KERN_IMAGE_START 0x01UL
-#define PSW_MASK_64 0x0001ULL
-#define PSW_MASK_32 0x8000ULL
-#define PSW_MASK_SHORTPSW 0x0008ULL
-#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
+#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
 
 typedef struct ResetInfo {
 uint64_t ipl_psw;
@@ -54,7 +52,7 @@ void jump_to_IPL_code(uint64_t address)
 
 current->ipl_psw = (uint64_t) &jump_to_IPL_2;
 current->ipl_psw |= RESET_PSW_MASK;
-current->ipl_continue = address & 0x7fff;
+current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
 
 debug_print_int("set IPL addr to", current->ipl_continue);
 
@@ -86,7 +84,7 @@ void jump_to_low_kernel(void)
 
 /* Trying to get PSW at zero address */
 if (*((uint64_t *)0) & RESET_PSW_MASK) {
-jump_to_IPL_code((*((uint64_t *)0)) & 0x7fff);
+jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR);
 }
 
 /* No other option left, so use the Linux kernel start address */
diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 73852029d4..6da44d4436 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -26,9 +26,11 @@ _Static_assert(sizeof(struct PSWLegacy) == 8, "PSWLegacy 
size incorrect");
 
 /* s390 psw bit masks */
 #define PSW_MASK_IOINT  0x0200ULL
+#define PSW_MASK_SHORTPSW   0x0008ULL
 #define PSW_MASK_WAIT   0x0002ULL
 #define PSW_MASK_EAMODE 0x0001ULL
 #define PSW_MASK_BAMODE 0x8000ULL
+#define PSW_MASK_SHORT_ADDR 0x7fffULL
 #define PSW_MASK_64 (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
 
 /* Low core mapping */
-- 
2.25.1

[PATCH v5 01/12] pc-bios: s390x: cio.c cleanup and compile fix

[Janosch Frank]

Let's initialize the structs at the beginning to ease reading and also
zeroing all other fields. This also makes the compiler stop
complaining about sense_id_ccw.flags being ored into when it's not
initialized.

Signed-off-by: Janosch Frank 
Reviewed-by: Pierre Morel 
Reviewed-by: Thomas Huth 
Reviewed-by: David Hildenbrand 
Reviewed-by: Christian Borntraeger 
---
 pc-bios/s390-ccw/cio.c | 40 ++--
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
index 339ec5fbe7..83ca27ab41 100644
--- a/pc-bios/s390-ccw/cio.c
+++ b/pc-bios/s390-ccw/cio.c
@@ -49,13 +49,13 @@ void enable_subchannel(SubChannelId schid)
 
 uint16_t cu_type(SubChannelId schid)
 {
-Ccw1 sense_id_ccw;
 SenseId sense_data;
-
-sense_id_ccw.cmd_code = CCW_CMD_SENSE_ID;
-sense_id_ccw.cda = ptr2u32(&sense_data);
-sense_id_ccw.count = sizeof(sense_data);
-sense_id_ccw.flags |= CCW_FLAG_SLI;
+Ccw1 sense_id_ccw = {
+.cmd_code = CCW_CMD_SENSE_ID,
+.flags = CCW_FLAG_SLI,
+.count = sizeof(sense_data),
+.cda = ptr2u32(&sense_data),
+};
 
 if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
 panic("Failed to run SenseID CCw\n");
@@ -67,13 +67,13 @@ uint16_t cu_type(SubChannelId schid)
 int basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
  uint16_t data_size)
 {
-Ccw1 senseCcw;
+Ccw1 senseCcw = {
+.cmd_code = CCW_CMD_BASIC_SENSE,
+.count = data_size,
+.cda = ptr2u32(sense_data),
+};
 Irb irb;
 
-senseCcw.cmd_code = CCW_CMD_BASIC_SENSE;
-senseCcw.cda = ptr2u32(sense_data);
-senseCcw.count = data_size;
-
 return __do_cio(schid, ptr2u32(&senseCcw), CCW_FMT1, &irb);
 }
 
@@ -314,7 +314,17 @@ static void print_irb_err(Irb *irb)
  */
 static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
 {
-CmdOrb orb = {};
+/*
+ * QEMU's CIO implementation requires prefetch and 64-bit idaws. We
+ * allow all paths.
+ */
+CmdOrb orb = {
+.fmt = fmt,
+.pfch = 1,
+.c64 = 1,
+.lpm = 0xFF,
+.cpa = ccw_addr,
+};
 int rc;
 
 IPL_assert(fmt == 0 || fmt == 1, "Invalid ccw format");
@@ -324,12 +334,6 @@ static int __do_cio(SubChannelId schid, uint32_t ccw_addr, 
int fmt, Irb *irb)
 IPL_assert(ccw_addr <= 0xFF - 8, "Invalid ccw address");
 }
 
-orb.fmt = fmt;
-orb.pfch = 1;  /* QEMU's cio implementation requires prefetch */
-orb.c64 = 1;   /* QEMU's cio implementation requires 64-bit idaws */
-orb.lpm = 0xFF; /* All paths allowed */
-orb.cpa = ccw_addr;
-
 rc = ssch(schid, &orb);
 if (rc == 1 || rc == 2) {
 /* Subchannel status pending or busy. Eat status and ask for retry. */
-- 
2.25.1

Re: [PATCH v3 9/9] host trust limitation: Alter virtio default properties for protected guests

[Michael S. Tsirkin]

On Fri, Jun 19, 2020 at 01:16:38PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 19, 2020 at 07:47:20AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 19, 2020 at 12:06:02PM +1000, David Gibson wrote:
> > > > > The default behaviour for virtio devices is not to use the platforms 
> > > > > normal
> > > > > DMA paths, but instead to use the fact that it's running in a 
> > > > > hypervisor
> > > > > to directly access guest memory.  That doesn't work if the guest's 
> > > > > memory
> > > > > is protected from hypervisor access, such as with AMD's SEV or 
> > > > > POWER's PEF.
> > > > > 
> > > > > So, if a host trust limitation mechanism is enabled, then apply the
> > > > > iommu_platform=on option so it will go through normal DMA mechanisms.
> > > > > Those will presumably have some way of marking memory as shared with 
> > > > > the
> > > > > hypervisor or hardware so that DMA will work.
> > > > > 
> > > > > Signed-off-by: David Gibson 
> > > > > ---
> > > > >  hw/core/machine.c | 11 +++
> > > > >  1 file changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index a71792bc16..8dfc1bb3f8 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -28,6 +28,8 @@
> > > > >  #include "hw/mem/nvdimm.h"
> > > > >  #include "migration/vmstate.h"
> > > > >  #include "exec/host-trust-limitation.h"
> > > > > +#include "hw/virtio/virtio.h"
> > > > > +#include "hw/virtio/virtio-pci.h"
> > > > >  
> > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > >  { "virtio-balloon-device", "page-poison", "false" },
> > > > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState 
> > > > > *machine)
> > > > >   * areas.
> > > > >   */
> > > > >  machine_set_mem_merge(OBJECT(machine), false, &error_abort);
> > > > > +
> > > > > +/*
> > > > > + * Virtio devices can't count on directly accessing guest
> > > > > + * memory, so they need iommu_platform=on to use normal DMA
> > > > > + * mechanisms.  That requires disabling legacy virtio support
> > > > > + * for virtio pci devices
> > > > > + */
> > > > > +object_register_sugar_prop(TYPE_VIRTIO_PCI, 
> > > > > "disable-legacy", "on");
> > > > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE, 
> > > > > "iommu_platform", "on");
> > > > >  }
> > > > 
> > > > Silently changing the user's request configuration like this is a bad 
> > > > idea.
> > > > The "disable-legacy" option in particular is undesirable as that 
> > > > switches
> > > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > > the guest.
> > > > 
> > > > If some options are incompatible with encryption, then we should raise a
> > > > fatal error at startup, so applications/admins are aware that their 
> > > > requested
> > > > config is broken.
> > >
> > > Agreed - my suggestion is an on/off/auto property, auto value
> > > changes automatically, on/off is validated.
> > 
> > In fact should we extend all bit properties to allow an auto value?
> 
> If "auto" was made the default that creates a similar headache, as to
> preserve existing configuration semantics we expose to apps, libvirt
> would need to find all the properties changed to use "auto" and manually
> set them back to on/off explicitly.
> 
> Regards,
> Daniel

It's QEMU's job to try and have more or less consistent semantics across
versions. QEMU does not guarantee not to change any option defaults
though.

My point is to add ability to differentiate between property values
set by user and ones set by machine type for compatibility.


-- 
MST

[PATCH v5 06/12] pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64

[Janosch Frank]

This constant enables 64 bit addressing, not the ESAME architecture,
so it shouldn't be named ZMODE.

Signed-off-by: Janosch Frank 
---
 pc-bios/s390-ccw/s390-arch.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 5f36361c02..73852029d4 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -29,7 +29,7 @@ _Static_assert(sizeof(struct PSWLegacy) == 8, "PSWLegacy size 
incorrect");
 #define PSW_MASK_WAIT   0x0002ULL
 #define PSW_MASK_EAMODE 0x0001ULL
 #define PSW_MASK_BAMODE 0x8000ULL
-#define PSW_MASK_ZMODE  (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
+#define PSW_MASK_64 (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
 
 /* Low core mapping */
 typedef struct LowCore {
-- 
2.25.1

[PATCH v5 04/12] pc-bios: s390x: Get rid of magic offsets into the lowcore

[Janosch Frank]

If we have a lowcore struct that has members for offsets that we want
to touch, why not use it?

Signed-off-by: Janosch Frank 
Reviewed-by: David Hildenbrand 
---
 pc-bios/s390-ccw/cio.h  | 17 +++--
 pc-bios/s390-ccw/main.c |  8 +++-
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index aaa432dedd..1e5d4e92e1 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -122,12 +122,17 @@ typedef struct schib {
 } __attribute__ ((packed, aligned(4))) Schib;
 
 typedef struct subchannel_id {
-__u32 cssid:8;
-__u32:4;
-__u32 m:1;
-__u32 ssid:2;
-__u32 one:1;
-__u32 sch_no:16;
+union {
+struct {
+__u16 cssid:8;
+__u16 reserved:4;
+__u16 m:1;
+__u16 ssid:2;
+__u16 one:1;
+};
+__u16 sch_id;
+};
+__u16 sch_no;
 } __attribute__ ((packed, aligned(4))) SubChannelId;
 
 struct chsc_header {
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 4e65b411e1..8b912454c9 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -36,11 +36,9 @@ LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
  */
 void write_subsystem_identification(void)
 {
-SubChannelId *schid = (SubChannelId *) 184;
-uint32_t *zeroes = (uint32_t *) 188;
-
-*schid = blk_schid;
-*zeroes = 0;
+lowcore->subchannel_id = blk_schid.sch_id;
+lowcore->subchannel_nr = blk_schid.sch_no;
+lowcore->io_int_parm = 0;
 }
 
 void write_iplb_location(void)
-- 
2.25.1

[PATCH v5 05/12] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes

[Janosch Frank]

jump_to_low_kernel() and the functions that it calls will already or
64 bit addressing into the reset psw mask when executing
jump_to_IPL_2() after the diag308 subcode 1.

The kernel proper is then branched to rather than doing a full PSW
change.

Signed-off-by: Janosch Frank 
---
 pc-bios/s390-ccw/dasd-ipl.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0fc879bb8e..e8f2846740 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -206,7 +206,6 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, 
uint32_t addr)
  */
 void dasd_ipl(SubChannelId schid, uint16_t cutype)
 {
-PSWLegacy *pswl = (PSWLegacy *) 0x00;
 uint32_t ipl2_addr;
 
 /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
@@ -229,7 +228,5 @@ void dasd_ipl(SubChannelId schid, uint16_t cutype)
 run_ipl2(schid, cutype, ipl2_addr);
 
 /* Transfer control to the guest operating system */
-pswl->mask |= PSW_MASK_EAMODE;   /* Force z-mode */
-pswl->addr |= PSW_MASK_BAMODE;   /* ...  */
 jump_to_low_kernel();
 }
-- 
2.25.1

[RFC v5 12/12] pc-bios: s390x: Cleanup jump to ipl code

[Janosch Frank]

jump_to_IPL_code takes a 64 bit address, masks it with the short psw
address mask and later branches to it using a full 64 bit register.

* As the masking is not necessary, let's remove it
* Without the mask we can save the ipl address to a static 64 bit
  function ptr as we later branch to it
* Let's also clean up the variable names and remove the now unneeded
  ResetInfo

Signed-off-by: Janosch Frank 
---
 pc-bios/s390-ccw/jump2ipl.c | 27 +++
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 767012bf0c..aef37cea76 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -13,20 +13,15 @@
 #define KERN_IMAGE_START 0x01UL
 #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
 
-typedef struct ResetInfo {
-uint64_t ipl_psw;
-uint32_t ipl_continue;
-} ResetInfo;
-
-static ResetInfo save;
+static void (*ipl_continue)(void);
+static uint64_t psw_save;
 
 static void jump_to_IPL_2(void)
 {
-ResetInfo *current = 0;
+uint64_t *psw_current = 0;
 
-void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
-*current = save;
-ipl(); /* should not return */
+*psw_current = psw_save;
+ipl_continue(); /* should not return */
 }
 
 void jump_to_IPL_code(uint64_t address)
@@ -46,15 +41,15 @@ void jump_to_IPL_code(uint64_t address)
  * content of non-BIOS memory after we loaded the guest, so we
  * save the original content and restore it in jump_to_IPL_2.
  */
-ResetInfo *current = 0;
+uint64_t *psw_current = 0;
 
-save = *current;
+psw_save = *psw_current;
 
-current->ipl_psw = (uint64_t) &jump_to_IPL_2;
-current->ipl_psw |= RESET_PSW_MASK;
-current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
+*psw_current = (uint64_t) &jump_to_IPL_2;
+*psw_current |= RESET_PSW_MASK;
+ipl_continue = (void *)address;
 
-debug_print_int("set IPL addr to", current->ipl_continue);
+debug_print_int("set IPL addr to", (uint64_t)ipl_continue);
 
 /* Ensure the guest output starts fresh */
 sclp_print("\n");
-- 
2.25.1

[PATCH v5 08/12] pc-bios: s390x: Move panic() into header and add infinite loop

[Janosch Frank]

panic() was defined for the ccw and net bios, i.e. twice, so it's
cleaner to rather put it into the header.

Also let's add an infinite loop into the assembly of disabled_wait() so
the caller doesn't need to take care of it.

Signed-off-by: Janosch Frank 
Reviewed-by: Pierre Morel 
Reviewed-by: David Hildenbrand 
Reviewed-by: Thomas Huth 
---
 pc-bios/s390-ccw/main.c | 7 ---
 pc-bios/s390-ccw/netmain.c  | 8 
 pc-bios/s390-ccw/s390-ccw.h | 9 +++--
 pc-bios/s390-ccw/start.S| 5 +++--
 4 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 8b912454c9..146a50760b 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -46,13 +46,6 @@ void write_iplb_location(void)
 lowcore->ptr_iplb = ptr2u32(&iplb);
 }
 
-void panic(const char *string)
-{
-sclp_print(string);
-disabled_wait();
-while (1) { }
-}
-
 unsigned int get_loadparm_index(void)
 {
 return atoui(loadparm_str);
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index f1ee63577a..056e93a818 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -439,14 +439,6 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
 return rc;
 }
 
-void panic(const char *string)
-{
-sclp_print(string);
-for (;;) {
-disabled_wait();
-}
-}
-
 void write_subsystem_identification(void)
 {
 SubChannelId *schid = (SubChannelId *) 184;
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index c5820e43ae..36b884cced 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -50,12 +50,11 @@ typedef unsigned long long __u64;
 #include "iplb.h"
 
 /* start.s */
-void disabled_wait(void);
+void disabled_wait(void) __attribute__ ((__noreturn__));
 void consume_sclp_int(void);
 void consume_io_int(void);
 
 /* main.c */
-void panic(const char *string);
 void write_subsystem_identification(void);
 void write_iplb_location(void);
 extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
@@ -91,6 +90,12 @@ bool menu_is_enabled_enum(void);
 
 #define MAX_BOOT_ENTRIES  31
 
+static inline void panic(const char *string)
+{
+sclp_print(string);
+disabled_wait();
+}
+
 static inline void fill_hex(char *out, unsigned char val)
 {
 const char hex[] = "0123456789abcdef";
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index aa8fceb19d..ce519300a1 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -47,8 +47,9 @@ memsetxc:
  */
.globl disabled_wait
 disabled_wait:
-larl %r1,disabled_wait_psw
-lpswe   0(%r1)
+   larl%r1,disabled_wait_psw
+   lpswe   0(%r1)
+1: j   1b
 
 
 /*
-- 
2.25.1

[PATCH v5 10/12] pc-bios: s390x: Make u32 ptr check explicit

[Janosch Frank]

Let's make it a bit more clear that we check the full 64 bits to fit
into the 32 we return.

Signed-off-by: Janosch Frank 
Suggested-by: David Hildenbrand 
Reviewed-by: David Hildenbrand 
Reviewed-by: Thomas Huth 
---
 pc-bios/s390-ccw/helper.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index 32a453b634..dfcfea0ff0 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -19,7 +19,7 @@
 /* Avoids compiler warnings when casting a pointer to a u32 */
 static inline uint32_t ptr2u32(void *ptr)
 {
-IPL_assert((uint64_t)ptr <= 0x, "ptr2u32: ptr too large");
+IPL_assert((uint64_t)ptr <= 0xull, "ptr2u32: ptr too large");
 return (uint32_t)(uint64_t)ptr;
 }
 
-- 
2.25.1

[PATCH v5 09/12] pc-bios: s390x: Use ebcdic2ascii table

[Janosch Frank]

Why should we do conversion of a ebcdic value if we have a handy table
where we could look up the ascii value instead?

Signed-off-by: Janosch Frank 
Reviewed-by: David Hildenbrand 
Reviewed-by: Thomas Huth 
---
 pc-bios/s390-ccw/bootmap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index d13b7cbd15..97205674e5 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -328,9 +328,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
 msg[0] = '2';
 break;
 default:
-msg[0] = vlbl->LDL_version;
-msg[0] &= 0x0f; /* convert EBCDIC   */
-msg[0] |= 0x30; /* to ASCII (digit) */
+msg[0] = ebc2asc[vlbl->LDL_version];
 msg[1] = '?';
 break;
 }
-- 
2.25.1

Re: [PATCH] libqos: usb-hcd-ehci: use 32-bit write for config register

[Paolo Bonzini]

On 24/06/20 09:47, Thomas Huth wrote:
>>
>>   {
>>   /* hands over all ports from companion uhci to ehci */
>> -    qpci_io_writew(ehci1.dev, ehci1.bar, 0x60, 1);
>> +    qpci_io_writel(ehci1.dev, ehci1.bar, 0x60, 1);
>>   }
>>     static void pci_uhci_port_2(void)
> 
> Passes "make check-qtest-x86_64" on a s390x host, too:
> 
> Tested-by: Thomas Huth 

Ah, I see what you mean now, so I'll clarify.  Endianness is handled by
the memory core and libqos so for little-endian devices (such as PCI
devices) it's okay.  For big-endian using the wrong size would not work,
but it should fail the same on LE and BE hosts.  endianness-test checks
that.

Paolo

Re: [PATCH] libqos: pci-pc: use 32-bit write for EJ register

[Paolo Bonzini]

On 24/06/20 09:46, Thomas Huth wrote:
> On 23/06/2020 18.18, Paolo Bonzini wrote:
>> The memory region ops have min_access_size == 4 so obey it.
>>
>> Signed-off-by: Paolo Bonzini 
>> ---
>>   tests/qtest/libqos/pci-pc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/libqos/pci-pc.c b/tests/qtest/libqos/pci-pc.c
>> index 0bc591d1da..3bb2eb3ba8 100644
>> --- a/tests/qtest/libqos/pci-pc.c
>> +++ b/tests/qtest/libqos/pci-pc.c
>> @@ -186,7 +186,7 @@ void qpci_unplug_acpi_device_test(QTestState *qts,
>> const char *id, uint8_t slot)
>>   g_assert(!qdict_haskey(response, "error"));
>>   qobject_unref(response);
>>   -    qtest_outb(qts, ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
>> +    qtest_outl(qts, ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
>>     qtest_qmp_eventwait(qts, "DEVICE_DELETED");
>>   }
> 
> I was a little bit afraid that this could cause endianess issues on big
> endian hosts, but I gave it a try on a s390x machine and it seems to
> work fine.
> 
> Tested-by: Thomas Huth 

Also because this is "pci-pc.c". :))  But seriously: if anything this
would fix big endian bugs, not break them.

Paolo

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200624075226.92728-1-fran...@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1
Type: series
Message-id: 20200624075226.92728-1-fran...@linux.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag] patchew/20200624075226.92728-1-fran...@linux.ibm.com -> 
patchew/20200624075226.92728-1-fran...@linux.ibm.com
Switched to a new branch 'test'
f7789d0 pc-bios: s390x: Cleanup jump to ipl code
8680ca2 pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
830483b pc-bios: s390x: Make u32 ptr check explicit
f313bb2 pc-bios: s390x: Use ebcdic2ascii table
00728a2 pc-bios: s390x: Move panic() into header and add infinite loop
d025a9c pc-bios: s390x: Use PSW masks where possible and introduce 
PSW_MASK_SHORT_ADDR
e33e9ff pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
c98ec6e pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
f22b573 pc-bios: s390x: Get rid of magic offsets into the lowcore
aafb016 pc-bios: s390x: Move sleep and yield to helper.h
855c1c9 pc-bios: s390x: Consolidate timing functions into time.h
b906f41 pc-bios: s390x: cio.c cleanup and compile fix

=== OUTPUT BEGIN ===
1/12 Checking commit b906f41df28f (pc-bios: s390x: cio.c cleanup and compile 
fix)
2/12 Checking commit 855c1c9be4fe (pc-bios: s390x: Consolidate timing functions 
into time.h)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#94: 
new file mode 100644

total: 0 errors, 1 warnings, 142 lines checked

Patch 2/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/12 Checking commit aafb016fdfe0 (pc-bios: s390x: Move sleep and yield to 
helper.h)
4/12 Checking commit f22b573bc73e (pc-bios: s390x: Get rid of magic offsets 
into the lowcore)
ERROR: spaces required around that ':' (ctx:VxV)
#30: FILE: pc-bios/s390-ccw/cio.h:127:
+__u16 cssid:8;
^

ERROR: spaces required around that ':' (ctx:VxV)
#31: FILE: pc-bios/s390-ccw/cio.h:128:
+__u16 reserved:4;
   ^

ERROR: spaces required around that ':' (ctx:VxV)
#32: FILE: pc-bios/s390-ccw/cio.h:129:
+__u16 m:1;
^

ERROR: spaces required around that ':' (ctx:VxV)
#33: FILE: pc-bios/s390-ccw/cio.h:130:
+__u16 ssid:2;
   ^

ERROR: spaces required around that ':' (ctx:VxV)
#34: FILE: pc-bios/s390-ccw/cio.h:131:
+__u16 one:1;
  ^

total: 5 errors, 0 warnings, 37 lines checked

Patch 4/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/12 Checking commit c98ec6eb101b (pc-bios: s390x: Remove unneeded dasd-ipl.c 
reset psw mask changes)
6/12 Checking commit e33e9fff145a (pc-bios: s390x: Rename PSW_MASK_ZMODE to 
PSW_MASK_64)
7/12 Checking commit d025a9ce4c67 (pc-bios: s390x: Use PSW masks where possible 
and introduce PSW_MASK_SHORT_ADDR)
8/12 Checking commit 00728a2fdf9e (pc-bios: s390x: Move panic() into header and 
add infinite loop)
9/12 Checking commit f313bb25948c (pc-bios: s390x: Use ebcdic2ascii table)
10/12 Checking commit 830483bf940d (pc-bios: s390x: Make u32 ptr check explicit)
11/12 Checking commit 8680ca210f71 (pc-bios: s390x: Fix bootmap.c passing PSWs 
as addresses)
12/12 Checking commit f7789d0e1906 (pc-bios: s390x: Cleanup jump to ipl code)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200624075226.92728-1-fran...@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v4 1/5] qdev: Introduce DEFINE_PROP_RESERVED_REGION

[Markus Armbruster]

Auger Eric  writes:

> Hi Markus,
>
> On 6/23/20 5:13 PM, Markus Armbruster wrote:
>> Eric Auger  writes:
>> 
>>> Introduce a new property defining a reserved region:
>>> ::.
>>>
>>> This will be used to encode reserved IOVA regions.
>>>
>>> For instance, in virtio-iommu use case, reserved IOVA regions
>>> will be passed by the machine code to the virtio-iommu-pci
>>> device (an array of those). The type of the reserved region
>>> will match the virtio_iommu_probe_resv_mem subtype value:
>>> - VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0)
>>> - VIRTIO_IOMMU_RESV_MEM_T_MSI (1)
>>>
>>> on PC/Q35 machine, this will be used to inform the
>>> virtio-iommu-pci device it should bypass the MSI region.
>>> The reserved region will be: 0xfee0:0xfeef:1.
>>>
>>> On ARM, we can declare the ITS MSI doorbell as an MSI
>>> region to prevent MSIs from being mapped on guest side.
>>>
>>> Signed-off-by: Eric Auger 
>>> Reviewed-by: Markus Armbruster 
>>>
>>> ---
>>>
>>> v3 -> v4:
>>> - use ':' instead of commas as separators.
>>> - rearrange error messages
>>> - check snprintf returned value
>>> - dared to keep Markus' R-b despite those changes
>>> ---
>>>  include/exec/memory.h|  6 +++
>>>  include/hw/qdev-properties.h |  3 ++
>>>  include/qemu/typedefs.h  |  1 +
>>>  hw/core/qdev-properties.c| 89 
>>>  4 files changed, 99 insertions(+)
>>>
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index 7207025bd4..d7a53b96cc 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -51,6 +51,12 @@ extern bool global_dirty_log;
>>>  
>>>  typedef struct MemoryRegionOps MemoryRegionOps;
>>>  
>>> +struct ReservedRegion {
>>> +hwaddr low;
>>> +hwaddr high;
>>> +unsigned int type;
>> 
>> Suggest to s/unsigned int/unsigned/.
>> 
>>> +};
>>> +
>>>  typedef struct IOMMUTLBEntry IOMMUTLBEntry;
>>>  
>>>  /* See address_space_translate: bit 0 is read, bit 1 is write.  */
>>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>>> index 5252bb6b1a..95d0e7201d 100644
>>> --- a/include/hw/qdev-properties.h
>>> +++ b/include/hw/qdev-properties.h
>>> @@ -19,6 +19,7 @@ extern const PropertyInfo qdev_prop_string;
>>>  extern const PropertyInfo qdev_prop_chr;
>>>  extern const PropertyInfo qdev_prop_tpm;
>>>  extern const PropertyInfo qdev_prop_macaddr;
>>> +extern const PropertyInfo qdev_prop_reserved_region;
>>>  extern const PropertyInfo qdev_prop_on_off_auto;
>>>  extern const PropertyInfo qdev_prop_multifd_compression;
>>>  extern const PropertyInfo qdev_prop_losttickpolicy;
>>> @@ -184,6 +185,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>>>  DEFINE_PROP(_n, _s, _f, qdev_prop_drive_iothread, BlockBackend *)
>>>  #define DEFINE_PROP_MACADDR(_n, _s, _f) \
>>>  DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
>>> +#define DEFINE_PROP_RESERVED_REGION(_n, _s, _f) \
>>> +DEFINE_PROP(_n, _s, _f, qdev_prop_reserved_region, ReservedRegion)
>>>  #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \
>>>  DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_on_off_auto, OnOffAuto)
>>>  #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \
>>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>>> index ce4a78b687..15f5047bf1 100644
>>> --- a/include/qemu/typedefs.h
>>> +++ b/include/qemu/typedefs.h
>>> @@ -58,6 +58,7 @@ typedef struct ISABus ISABus;
>>>  typedef struct ISADevice ISADevice;
>>>  typedef struct IsaDma IsaDma;
>>>  typedef struct MACAddr MACAddr;
>>> +typedef struct ReservedRegion ReservedRegion;
>>>  typedef struct MachineClass MachineClass;
>>>  typedef struct MachineState MachineState;
>>>  typedef struct MemoryListener MemoryListener;
>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index ead35d7ffd..193d0d95f9 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -15,6 +15,7 @@
>>>  #include "chardev/char.h"
>>>  #include "qemu/uuid.h"
>>>  #include "qemu/units.h"
>>> +#include "qemu/cutils.h"
>>>  
>>>  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
>>>Error **errp)
>>> @@ -578,6 +579,94 @@ const PropertyInfo qdev_prop_macaddr = {
>>>  .set   = set_mac,
>>>  };
>>>  
>>> +/* --- Reserved Region --- */
>>> +
>>> +/*
>>> + * accepted syntax version:
>> 
>> "version" feels redundant.  Suggest to capitalize "Accepted".
>> 
>>> + *   ::
>>> + *   where low/high addresses are uint64_t in hexadecimal
>>> + *   and type is an unsigned integer in decimal
>>> + */
>>> +static void get_reserved_region(Object *obj, Visitor *v, const char *name,
>>> +void *opaque, Error **errp)
>>> +{
>>> +DeviceState *dev = DEVICE(obj);
>>> +Property *prop = opaque;
>>> +ReservedRegion *rr = qdev_get_prop_ptr(dev, prop);
>>> +char buffer[64];
>>> +char *p = buffer;
>>> +int rc;
>>> +
>>> +rc = snprintf(buffer, sizeof

[PULL v2 0/2] Block patches

[Max Reitz]

The following changes since commit d88d5a3806d78dcfca648c62dae9d88d3e803bd2:

  Merge remote-tracking branch 'remotes/philmd-gitlab/tags/renesas-hw-20200622' 
into staging (2020-06-23 13:55:52 +0100)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2020-06-24

for you to fetch changes up to 24b861c0386a17ea31eb824310c21118fb7be883:

  iotests: don't test qcow2.py inside 291 (2020-06-24 10:00:04 +0200)


Block patches:
- Two iotest fixes


This is v2, where I dropped Maxim’s LUKS keyslot amendment series and my
iotest patches, because both caused iotest failures on some test
machines.

Philippe Mathieu-Daudé (1):
  iotests: Fix 051 output after qdev_init_nofail() removal

Vladimir Sementsov-Ogievskiy (1):
  iotests: don't test qcow2.py inside 291

 tests/qemu-iotests/051.pc.out |  4 ++--
 tests/qemu-iotests/291|  4 
 tests/qemu-iotests/291.out| 33 -
 3 files changed, 2 insertions(+), 39 deletions(-)

-- 
2.26.2

[PULL v2 2/2] iotests: don't test qcow2.py inside 291

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

820c6bee534ec3b added testing of qcow2.py into 291, and it breaks 291
with external data file. Actually, 291 is bad place for qcow2.py
testing, better add a separate test.

For now, drop qcow2.py testing from 291 to fix the regression.

Fixes: 820c6bee534ec3b
Reported-by: Max Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20200618154052.8629-1-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/291 |  4 
 tests/qemu-iotests/291.out | 33 -
 2 files changed, 37 deletions(-)

diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
index 28e4fb9b4d..1e0bb76959 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/291
@@ -64,8 +64,6 @@ $QEMU_IO -c 'w 1M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
 $QEMU_IMG bitmap --disable -f $IMGFMT "$TEST_IMG" b1
 $QEMU_IMG bitmap --enable -f $IMGFMT "$TEST_IMG" b2
 $QEMU_IO -c 'w 2M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
-echo "Check resulting qcow2 header extensions:"
-$PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 
 echo
 echo "=== Bitmap preservation not possible to non-qcow2 ==="
@@ -92,8 +90,6 @@ $QEMU_IMG bitmap --merge tmp -f $IMGFMT "$TEST_IMG" b0
 $QEMU_IMG bitmap --remove --image-opts \
 driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp
 _img_info --format-specific
-echo "Check resulting qcow2 header extensions:"
-$PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 
 echo
 echo "=== Check bitmap contents ==="
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index 08bf6b..9f661515b4 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -14,25 +14,6 @@ wrote 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1048576/1048576 bytes at offset 2097152
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Check resulting qcow2 header extensions:
-Header extension:
-magic 0xe2792aca (Backing format)
-length5
-data  'qcow2'
-
-Header extension:
-magic 0x6803f857 (Feature table)
-length336
-data  
-
-Header extension:
-magic 0x23852875 (Bitmaps)
-length24
-nb_bitmaps2
-reserved320
-bitmap_directory_size 0x40
-bitmap_directory_offset   0x51
-
 
 === Bitmap preservation not possible to non-qcow2 ===
 
@@ -84,20 +65,6 @@ Format specific information:
 granularity: 65536
 refcount bits: 16
 corrupt: false
-Check resulting qcow2 header extensions:
-Header extension:
-magic 0x6803f857 (Feature table)
-length336
-data  
-
-Header extension:
-magic 0x23852875 (Bitmaps)
-length24
-nb_bitmaps3
-reserved320
-bitmap_directory_size 0x60
-bitmap_directory_offset   0x52
-
 
 === Check bitmap contents ===
 
-- 
2.26.2

[PULL v2 1/2] iotests: Fix 051 output after qdev_init_nofail() removal

[Max Reitz]

From: Philippe Mathieu-Daudé 

Commit 96927c744 replaced qdev_init_nofail() call by
isa_realize_and_unref() which has a different error
message. Update the test output accordingly.

Gitlab CI error after merging b77b5b3dc7:
https://gitlab.com/qemu-project/qemu/-/jobs/597414772#L4375

Reported-by: Thomas Huth 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200616154949.6586-1-phi...@redhat.com>
Reviewed-by: Thomas Huth 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/051.pc.out | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index 0ea80d35f0..da8ad87187 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -142,7 +142,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive if=ide
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: Initialization of device ide-hd failed: Device needs media, 
but drive is empty
+(qemu) QEMU_PROG: Device needs media, but drive is empty
 
 Testing: -drive if=virtio
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -214,7 +214,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: Initialization of device ide-hd failed: Block node is 
read-only
+(qemu) QEMU_PROG: Block node is read-only
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on
 QEMU X.Y.Z monitor - type 'help' for more information
-- 
2.26.2

Re: [PATCH] qom: Allow object_property_add_child() to fail

[Markus Armbruster]

Eric Auger  writes:

> object_property_add() does not allow object_property_try_add()
> to gracefully fail as &error_abort is passed as an error handle.
>
> However such failure can easily be triggered from the QMP shell when,
> for instance, one attempts to create an object with an id that already
> exists:
>
> For instance, call twice:
> object-add qom-type=memory-backend-ram id=mem1 props.size=1073741824
> and QEMU aborts.
>
> This behavior is undesired as a user/management application mistake
> in reusing a property ID shouldn't result in loss of the VM and live
> data within.
>
> This patch introduces two new functions, object_property_add_err() and
> object_property_add_child_err() whose prototype features an error handle.
> object_property_add_child_err() now gets called from user_creatable_add_type.
> This solution was chosen instead of changing the prototype of existing
> functions because the number of existing callers is huge.
>
> The error now is returned gracefully to the QMP client.
>
> (QEMU) object-add qom-type=memory-backend-ram id=mem2  props.size=4294967296
> {"return": {}}
> (QEMU) object-add qom-type=memory-backend-ram id=mem2  props.size=4294967296
> {"error": {"class": "GenericError", "desc": "attempt to add duplicate property
> 'mem2' to object (type 'container')"}}
>
> Signed-off-by: Eric Auger 

Recent regression, my fault.  Please point that out, and add

  Fixes: d2623129a7dec1d3041ad1221dda1ca49c667532

Re: [PATCH v3 1/1] tricore: added triboard with tc27x_soc

[Bastian Koppelmann]

On Mon, Jun 22, 2020 at 03:19:34PM +0200, David Brenken wrote:
> From: Andreas Konopik 
> +const MemmapEntry tc27x_soc_memmap[] = {
> +[TC27XD_DSPR2] = { 0x5000,   0x1E000 },
> +[TC27XD_DCACHE2]   = { 0x5001E000,0x2000 },
> +[TC27XD_DTAG2] = { 0x500C, 0xC00 },

The size changed from 0xa00 to 0xc00 from v2. The manual states that it has no
size. I guess you inferred the size from the address range. How does real hw
behave if you access DTAG2?

> +[TC27XD_PSPR2] = { 0x5010,0x8000 },
> +[TC27XD_PCACHE2]   = { 0x50108000,0x4000 },
> +[TC27XD_PTAG2] = { 0x501C,0x1800 },
> +[TC27XD_DSPR1] = { 0x6000,   0x1E000 },
> +[TC27XD_DCACHE1]   = { 0x6001E000,0x2000 },
> +[TC27XD_DTAG1] = { 0x600C, 0xC00 },
> +[TC27XD_PSPR1] = { 0x6010,0x8000 },
> +[TC27XD_PCACHE1]   = { 0x60108000,0x4000 },
> +[TC27XD_PTAG1] = { 0x601C,0x1800 },
> +[TC27XD_DSPR0] = { 0x7000,   0x1C000 },
> +[TC27XD_PSPR0] = { 0x7010,0x6000 },
> +[TC27XD_PCACHE0]   = { 0x70106000,0x2000 },
> +[TC27XD_PTAG0] = { 0x701C, 0xC00 },
> +[TC27XD_PFLASH0_C] = { 0x8000,  0x20 },
> +[TC27XD_PFLASH1_C] = { 0x8020,  0x20 },
> +[TC27XD_OLDA_C]= { 0x8FE7,0x8000 },
> +[TC27XD_BROM_C]= { 0x8FFF8000,0x8000 },
> +[TC27XD_LMURAM_C]  = { 0x9000,0x8000 },
> +[TC27XD_EMEM_C]= { 0x9F00,  0x10 },
> +[TC27XD_PFLASH0_U] = { 0xA000,   0x0 },
> +[TC27XD_PFLASH1_U] = { 0xA020,   0x0 },
> +[TC27XD_DFLASH0]   = { 0xAF00,  0x104000 },
> +[TC27XD_DFLASH1]   = { 0xAF11,   0x1 },
> +[TC27XD_OLDA_U]= { 0xAFE7,   0x0 },
> +[TC27XD_BROM_U]= { 0xAFFF8000,   0x0 },
> +[TC27XD_LMURAM_U]  = { 0xB000,   0x0 },
> +[TC27XD_EMEM_U]= { 0xBF00,   0x0 },
> +[TC27XD_PSPRX] = { 0xC000,   0x0 },
> +[TC27XD_DSPRX] = { 0xD000,   0x0 },
> +};

Why not use KiB/MiB sizes as before? I created a patch for that. Can you check
that I didn't skrew up the sizes?

--- a/hw/tricore/tc27x_soc.c
+++ b/hw/tricore/tc27x_soc.c
@@ -26,44 +26,45 @@
 #include "hw/misc/unimp.h"
 #include "exec/address-spaces.h"
 #include "qemu/log.h"
+#include "qemu/units.h"
 #include "cpu.h"

 #include "hw/tricore/tc27x_soc.h"
 #include "hw/tricore/triboard.h"

 const MemmapEntry tc27x_soc_memmap[] = {
-[TC27XD_DSPR2] = { 0x5000,   0x1E000 },
-[TC27XD_DCACHE2]   = { 0x5001E000,0x2000 },
-[TC27XD_DTAG2] = { 0x500C, 0xC00 },
-[TC27XD_PSPR2] = { 0x5010,0x8000 },
-[TC27XD_PCACHE2]   = { 0x50108000,0x4000 },
-[TC27XD_PTAG2] = { 0x501C,0x1800 },
-[TC27XD_DSPR1] = { 0x6000,   0x1E000 },
-[TC27XD_DCACHE1]   = { 0x6001E000,0x2000 },
-[TC27XD_DTAG1] = { 0x600C, 0xC00 },
-[TC27XD_PSPR1] = { 0x6010,0x8000 },
-[TC27XD_PCACHE1]   = { 0x60108000,0x4000 },
-[TC27XD_PTAG1] = { 0x601C,0x1800 },
-[TC27XD_DSPR0] = { 0x7000,   0x1C000 },
-[TC27XD_PSPR0] = { 0x7010,0x6000 },
-[TC27XD_PCACHE0]   = { 0x70106000,0x2000 },
-[TC27XD_PTAG0] = { 0x701C, 0xC00 },
-[TC27XD_PFLASH0_C] = { 0x8000,  0x20 },
-[TC27XD_PFLASH1_C] = { 0x8020,  0x20 },
-[TC27XD_OLDA_C]= { 0x8FE7,0x8000 },
-[TC27XD_BROM_C]= { 0x8FFF8000,0x8000 },
-[TC27XD_LMURAM_C]  = { 0x9000,0x8000 },
-[TC27XD_EMEM_C]= { 0x9F00,  0x10 },
-[TC27XD_PFLASH0_U] = { 0xA000,   0x0 },
-[TC27XD_PFLASH1_U] = { 0xA020,   0x0 },
-[TC27XD_DFLASH0]   = { 0xAF00,  0x104000 },
-[TC27XD_DFLASH1]   = { 0xAF11,   0x1 },
-[TC27XD_OLDA_U]= { 0xAFE7,   0x0 },
-[TC27XD_BROM_U]= { 0xAFFF8000,   0x0 },
-[TC27XD_LMURAM_U]  = { 0xB000,   0x0 },
-[TC27XD_EMEM_U]= { 0xBF00,   0x0 },
-[TC27XD_PSPRX] = { 0xC000,   0x0 },
-[TC27XD_DSPRX] = { 0xD000,   0x0 },
+[TC27XD_DSPR2] = { 0x5000,120 * KiB },
+[TC27XD_DCACHE2]   = { 0x5001E000,  8 * KiB },
+[TC27XD_DTAG2] = { 0x500C,0xC00 },
+[TC27XD_PSPR2] = { 0x5010, 32 * KiB },
+[TC27XD_PCACHE2]   = { 0x50108000, 16 * KiB },
+[TC27XD_PTAG2] = { 0x501C,   0x1800 },
+[TC27XD_DSPR1] = { 0x6000,120 * KiB },
+[TC27XD_DCACHE1]   = { 0x6001E000,  8 * KiB },
+[TC27XD_DTAG1] = { 0x600C,0xC00 },
+[TC27XD_PSPR1] = { 0x6010, 32 * KiB },
+[TC27XD_PCACHE1]   = { 0x60108000, 16 * KiB },
+[TC27XD_PTAG1] = { 0x601C,   0x1800 },
+[TC27XD_DSPR

Re: -enablefips

[Markus Armbruster]

Gerd Hoffmann  writes:

> On Tue, Jun 23, 2020 at 11:51:09PM -0400, John Snow wrote:
>> I never knew what this option did, but the answer is ... strange!
>> 
>> It's only defined for linux, in os-posix.c. When called, it calls
>> fips_set_state(true), located in osdep.c.
>> 
>> This will read /proc/sys/crypto/fips_enabled and set the static global
>> 'fips_enabled' to true if this setting is on.
>
> IIRC the idea is to have a global switch to enable fips compilance for
> the whole distro.  RH specific.  rhel-7 kernel has it.  rhel-8 kernel
> too, so it probably isn't obsolete.  Not present in mainline kernels.
>
> I'm wondering what the point of the -enablefips switch is.  Shouldn't
> qemu check /proc/sys/crypto/fips_enabled unconditionally instead?

The switch feels rather silly to me.  If you take the trouble to put
your host in FIPS mode, requiring -enable-fips to make QEMU to actually
honor it makes no sense.  If you don't, QEMU's -enable-fips has no
effect.

I may well misremember things (it's been years), but I vaguely recall
-enable-fips being a lame compromise between "this ought to be upstream"
and "FIPS is stupid, and I want nothing of it".

>> (Tangent: what does *this* setting actually control? Should QEMU
>> meaningfully change its behavior when it's set?)
>
> fips is a security policy ...
>
>> This static global is exposed via the getter fips_get_state(). This
>> function is called only by vnc.c, and appears to disable the use of the
>> password option for -vnc.
>
> ... yes, "no passwords" is one of the rules.  There are probably more.
>
>> (If we really do want to keep it, it should probably go under -global
>> somewhere instead to help reduce flag clutter, but we'd need to have a
>> chat about what fips compliance means for literally every other spot in
>> QEMU that is capable of using or receiving a cleartext password.)
>
> Yep.  IIRC for spice this is handled in libspice-server.  We need to
> look at blockdev encryption I guess.  Any other places where qemu uses
> passwords directly?  I think we don't have to worry about indirect usage
> (sasl).

I'd expect the SASL libraries to honor FIPS mode by themselves.  But
best ask someone who actually knows how FIPS mode is supposed to work.

[PATCH] trivial: Remove extra character in configure help

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index ba88fd1824..c7a6a5adfe 100755
--- a/configure
+++ b/configure
@@ -1787,7 +1787,7 @@ Advanced options (experts only):
   --block-drv-ro-whitelist=L
set block driver read-only whitelist
(affects only QEMU, not qemu-img)
-  --enable-trace-backends=B Set trace backend
+  --enable-trace-backends= Set trace backend
Available backends: $trace_backend_list
   --with-trace-file=NAME   Full PATH,NAME of file to store traces
Default:trace-
-- 
2.26.2

Re: [PATCH] i386: Mask SVM features if nested SVM is disabled

[Paolo Bonzini]

On 24/06/20 01:01, Eduardo Habkost wrote:
> QEMU incorrectly validates FEAT_SVM feature flags against
> GET_SUPPORTED_CPUID even if SVM features are being masked out by
> cpu_x86_cpuid().  This can make QEMU print warnings on most AMD
> CPU models, even when SVM nesting is disabled (which is the
> default).
> 
> This bug was never detected before because of a Linux KVM bug:
> until Linux v5.6, KVM was not filtering out SVM features in
> GET_SUPPORTED_CPUID when nested was disabled.  This KVM bug was
> fixed in Linux v5.7-rc1, on Linux commit a50718cc3f43 ("KVM:
> nSVM: Expose SVM features to L1 iff nested is enabled").
> 
> Fix the problem by adding a CPUID_EXT3_SVM dependency to all
> FEAT_SVM feature flags in the feature_dependencies table.
> 
> Reported-by: Yanan Fu 
> Signed-off-by: Eduardo Habkost 
> ---
>  target/i386/cpu.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b1b311baa2..a9edcaf531 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1404,6 +1404,10 @@ static FeatureDep feature_dependencies[] = {
>  .from = { FEAT_VMX_SECONDARY_CTLS,  VMX_SECONDARY_EXEC_ENABLE_VMFUNC 
> },
>  .to = { FEAT_VMX_VMFUNC,~0ull },
>  },
> +{
> +.from = { FEAT_8000_0001_ECX,   CPUID_EXT3_SVM },
> +.to = { FEAT_SVM,   ~0ull },
> +},
>  };
>  
>  typedef struct X86RegisterInfo32 {
> 

Queued with this fixup:

diff --git a/tests/qtest/test-x86-cpuid-compat.c 
b/tests/qtest/test-x86-cpuid-compat.c
index 772287bdb4..8709e7d9ce 100644
--- a/tests/qtest/test-x86-cpuid-compat.c
+++ b/tests/qtest/test-x86-cpuid-compat.c
@@ -256,7 +256,7 @@ int main(int argc, char **argv)
"-cpu 486,+invtsc", "xlevel", 0x8007);
 /* CPUID[8000_000A].EDX: */
 add_cpuid_test("x86/cpuid/auto-xlevel/486/npt",
-   "-cpu 486,+npt", "xlevel", 0x800A);
+   "-cpu 486,+svm,+npt", "xlevel", 0x800A);
 /* CPUID[C000_0001].EDX: */
 add_cpuid_test("x86/cpuid/auto-xlevel2/phenom/xstore",
"-cpu phenom,+xstore", "xlevel2", 0xC001);
@@ -348,7 +348,7 @@ int main(int argc, char **argv)
"-machine pc-i440fx-2.4 -cpu SandyBridge,",
"xlevel", 0x8008);
 add_cpuid_test("x86/cpuid/xlevel-compat/pc-i440fx-2.4/npt-on",
-   "-machine pc-i440fx-2.4 -cpu SandyBridge,+npt",
+   "-machine pc-i440fx-2.4 -cpu SandyBridge,+svm,+npt",
"xlevel", 0x8008);
 
 /* Test feature parsing */

Paolo

[PATCH v2 10/25] test-util-filemonitor: Plug unlikely memory leak

[Markus Armbruster]

test_file_monitor_events() leaks an Error object when
qemu_file_monitor_add_watch() fails, which seems unlikely.  Plug it.

Cc: Daniel P. Berrangé 
Signed-off-by: Markus Armbruster 
---
 tests/test-util-filemonitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
index 45009c69f4..8f0eff3d03 100644
--- a/tests/test-util-filemonitor.c
+++ b/tests/test-util-filemonitor.c
@@ -495,6 +495,7 @@ test_file_monitor_events(void)
 if (*op->watchid < 0) {
 g_printerr("Unable to add watch %s",
error_get_pretty(local_err));
+error_free(local_err);
 goto cleanup;
 }
 if (debug) {
-- 
2.26.2

[PATCH v2 07/25] spapr: Plug minor memory leak in spapr_machine_init()

[Markus Armbruster]

spapr_machine_init() leaks an Error object when
kvmppc_check_papr_resize_hpt() fails and spapr->resize_hpt is
SPAPR_RESIZE_HPT_DISABLED, i.e. when the host doesn't support hash
page table resizing, and the user didn't ask for it.  As harmless as
memory leaks can possibly be.  Plug it.

Fixes: 30f4b05bd090564181554d0890605eb2c143e4ea
Cc: David Gibson 
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster 
---
 hw/ppc/spapr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bd9345cdac..9bd2183ead 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2731,6 +2731,7 @@ static void spapr_machine_init(MachineState *machine)
 error_report_err(resize_hpt_err);
 exit(1);
 }
+error_free(resize_hpt_err);
 
 spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
 
-- 
2.26.2

[PATCH v2 00/25] Error handling fixes & cleanups

[Markus Armbruster]

This series addresses a number of error handling issues I found while
working on error handling improvements.  It's based on my "[PULL
00/16] Qdev patches for 2020-06-23" merged into current master.

Based-on: <20200623142326.2349416-1-arm...@redhat.com>

v2:
* PATCH 04: Two more instances, in tests/check-qom-proplist
* PATCH 12: Dropped; PATCH 04 takes care of it
* PATCH 17: One more instance due to rebase; R-by kept
* PATCH 22-25: New

Markus Armbruster (25):
  net/virtio: Fix failover_replug_primary() return value regression
  pci: Delete useless error_propagate()
  Clean up some calls to ignore Error objects the right way
  tests: Use &error_abort where appropriate
  tests: Use error_free_or_abort() where appropriate
  usb/dev-mtp: Fix Error double free after inotify failure
  spapr: Plug minor memory leak in spapr_machine_init()
  qga: Plug unlikely memory leak in guest-set-memory-blocks
  sd/milkymist-memcard: Plug minor memory leak in realize
  test-util-filemonitor: Plug unlikely memory leak
  vnc: Plug minor memory leak in vnc_display_open()
  aspeed: Clean up roundabout error propagation
  qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp
  qdev: Drop qbus_set_hotplug_handler() parameter @errp
  hw: Fix error API violation around object_property_set_link()
  hw/arm: Drop useless object_property_set_link() error handling
  riscv/sifive_u: Fix sifive_u_soc_realize() error API violations
  riscv_hart: Fix riscv_harts_realize() error API violations
  mips/cps: Fix mips_cps_realize() error API violations
  x86: Fix x86_cpu_new() error API violations
  amd_iommu: Fix amdvi_realize() error API violation
  arm/stm32f205 arm/stm32f405: Fix realize error API violation
  aspeed: Fix realize error API violation
  hw/arm/armsse: Fix armsse_realize() error API violation
  arm/{bcm2835,fsl-imx25,fsl-imx6}: Fix realize error API violations

 include/hw/qdev-core.h |   5 +-
 chardev/char-socket.c  |   6 +-
 hw/9pfs/9p.c   |   6 +-
 hw/acpi/pcihp.c|   3 +-
 hw/acpi/piix4.c|   2 +-
 hw/arm/armsse.c|  61 +---
 hw/arm/armv7m.c|   7 +-
 hw/arm/aspeed_ast2600.c|  35 -
 hw/arm/aspeed_soc.c|  29 +++-
 hw/arm/bcm2835_peripherals.c   |  12 ++--
 hw/arm/fsl-imx25.c |  12 ++--
 hw/arm/fsl-imx6.c  |  12 ++--
 hw/arm/nrf51_soc.c |   6 +-
 hw/arm/stm32f205_soc.c |   2 +-
 hw/arm/stm32f405_soc.c |   2 +-
 hw/arm/virt.c  |   4 +-
 hw/char/virtio-serial-bus.c|   4 +-
 hw/core/bus.c  |   8 +--
 hw/display/virtio-gpu-pci.c|   2 +-
 hw/display/virtio-vga.c|   2 +-
 hw/dma/sparc32_dma.c   |   6 +-
 hw/dma/xilinx_axidma.c |  12 +---
 hw/i386/amd_iommu.c|   6 +-
 hw/i386/x86.c  |  12 +---
 hw/mips/cps.c  |  35 +
 hw/misc/macio/macio.c  |   3 +-
 hw/net/virtio-net.c|   2 +-
 hw/net/xilinx_axienet.c|  12 +---
 hw/pci/pci.c   |   3 -
 hw/pci/pcie.c  |   2 +-
 hw/pci/shpc.c  |   2 +-
 hw/ppc/spapr.c |   4 +-
 hw/ppc/spapr_drc.c |   4 +-
 hw/ppc/spapr_pci.c |   4 +-
 hw/riscv/riscv_hart.c  |  14 ++--
 hw/riscv/sifive_u.c|  12 +++-
 hw/s390x/ap-bridge.c   |   2 +-
 hw/s390x/css-bridge.c  |   2 +-
 hw/s390x/s390-pci-bus.c|  14 +---
 hw/scsi/scsi-bus.c |   2 +-
 hw/scsi/virtio-scsi.c  |   4 +-
 hw/scsi/vmw_pvscsi.c   |   2 +-
 hw/sd/milkymist-memcard.c  |   5 +-
 hw/usb/bus.c   |   2 +-
 hw/usb/dev-mtp.c   |   2 -
 hw/usb/dev-smartcard-reader.c  |   2 +-
 hw/virtio/virtio-iommu-pci.c   |   2 +-
 hw/xen/xen-bus.c   |   2 +-
 hw/xen/xen-legacy-backend.c|   2 +-
 qga/commands-posix.c   |   1 +
 tests/check-block-qdict.c  |  24 ++-
 tests/check-qobject.c  |   5 +-
 tests/check-qom-proplist.c |  14 ++--
 tests/test-base64.c|   3 +-
 tests/test-bdrv-graph-mod.c|   4 +-
 tests/test-block-iothread.c|   3 +-
 tests/test-crypto-cipher.c |   8 +--
 tests/test-io-task.c   |   4 +-
 tests/test-logging.c   |  12 +---
 tests/test-qemu-opts.c |  22 ++
 tests/test-replication.c   | 109 +
 tests/test-string-input-visitor.c  |  33 +++--
 tests/test-string-output-visitor.c |  16 ++---
 tests/test-util-filemonitor.c  |   1 +
 ui/vnc.c   |   6 +-
 65 files changed, 229 insertions(+), 432 deletions(-)

-- 
2.26.2

[PATCH v2 20/25] x86: Fix x86_cpu_new() error API violations

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

x86_cpu_new() is wrong that way: it passes &local_err to
object_property_set_uint() without checking it, and then to
qdev_realize().  Harmless, because the former can't actually fail
here.

Fix by checking for failure right away.  While there, replace
qdev_realize(); object_unref() by qdev_realize_and_unref().

Cc: Paolo Bonzini 
Cc: Richard Henderson 
Cc: Eduardo Habkost 
Signed-off-by: Markus Armbruster 
---
 hw/i386/x86.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 34229b45c7..3a7029e6db 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -118,16 +118,10 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState 
*x86ms,
 
 void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
 {
-Object *cpu = NULL;
-Error *local_err = NULL;
+Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
 
-cpu = object_new(MACHINE(x86ms)->cpu_type);
-
-object_property_set_uint(cpu, apic_id, "apic-id", &local_err);
-qdev_realize(DEVICE(cpu), NULL, &local_err);
-
-object_unref(cpu);
-error_propagate(errp, local_err);
+object_property_set_uint(cpu, apic_id, "apic-id", &error_abort);
+qdev_realize_and_unref(DEVICE(cpu), NULL, errp);
 }
 
 void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
-- 
2.26.2

[PATCH v2 14/25] qdev: Drop qbus_set_hotplug_handler() parameter @errp

[Markus Armbruster]

qbus_set_hotplug_handler() is a simple wrapper around
object_property_set_link().

object_property_set_link() fails when the property doesn't exist, is
not settable, or its .check() method fails.  These are all programming
errors here, so passing &error_abort to qbus_set_hotplug_handler() is
appropriate.

Most of its callers do.  Exceptions:

* pcie_cap_slot_init(), shpc_init(), spapr_phb_realize() pass NULL,
  i.e. they ignore errors.

* spapr_machine_init() passes &error_fatal.

* s390_pcihost_realize(), virtio_serial_device_realize(),
  s390_pcihost_plug() pass the error to their callers.  The latter two
  keep going after the error, which looks wrong.

Drop the @errp parameter, and instead pass &error_abort to
object_property_set_link().

Cc: Paolo Bonzini 
Cc: "Daniel P. Berrangé" 
Cc: Eduardo Habkost 
Signed-off-by: Markus Armbruster 
---
 include/hw/qdev-core.h|  3 +--
 hw/acpi/pcihp.c   |  3 +--
 hw/acpi/piix4.c   |  2 +-
 hw/char/virtio-serial-bus.c   |  4 ++--
 hw/core/bus.c |  6 +++---
 hw/pci/pcie.c |  2 +-
 hw/pci/shpc.c |  2 +-
 hw/ppc/spapr.c|  3 +--
 hw/ppc/spapr_pci.c|  4 ++--
 hw/s390x/ap-bridge.c  |  2 +-
 hw/s390x/css-bridge.c |  2 +-
 hw/s390x/s390-pci-bus.c   | 14 +++---
 hw/scsi/virtio-scsi.c |  4 ++--
 hw/scsi/vmw_pvscsi.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  2 +-
 15 files changed, 22 insertions(+), 33 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 78acdeaed6..fe78073c70 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -535,8 +535,7 @@ extern bool qdev_hot_removed;
 
 char *qdev_get_dev_path(DeviceState *dev);
 
-void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp);
-
+void qbus_set_hotplug_handler(BusState *bus, Object *handler);
 void qbus_set_bus_hotplug_handler(BusState *bus);
 
 static inline bool qbus_is_hotpluggable(BusState *bus)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 33ea2b76ae..9e31ab2da4 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -246,8 +246,7 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, 
AcpiPciHpState *s,
 object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
 PCIBus *sec = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
 
-qbus_set_hotplug_handler(BUS(sec), OBJECT(hotplug_dev),
- &error_abort);
+qbus_set_hotplug_handler(BUS(sec), OBJECT(hotplug_dev));
 /* We don't have to overwrite any other hotplug handler yet */
 assert(QLIST_EMPTY(&sec->child));
 }
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 1262abc77a..52bcbd6414 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -505,7 +505,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
 
 piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
pci_get_bus(dev), s);
-qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort);
+qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s));
 
 piix4_pm_add_propeties(s);
 }
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 262089c0c9..f9a4428bd6 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -1056,7 +1056,7 @@ static void virtio_serial_device_realize(DeviceState 
*dev, Error **errp)
 /* Spawn a new virtio-serial bus on which the ports will ride as devices */
 qbus_create_inplace(&vser->bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
 dev, vdev->bus_name);
-qbus_set_hotplug_handler(BUS(&vser->bus), OBJECT(vser), errp);
+qbus_set_hotplug_handler(BUS(&vser->bus), OBJECT(vser));
 vser->bus.vser = vser;
 QTAILQ_INIT(&vser->ports);
 
@@ -1147,7 +1147,7 @@ static void virtio_serial_device_unrealize(DeviceState 
*dev)
 g_free(vser->post_load);
 }
 
-qbus_set_hotplug_handler(BUS(&vser->bus), NULL, &error_abort);
+qbus_set_hotplug_handler(BUS(&vser->bus), NULL);
 
 virtio_cleanup(vdev);
 }
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 8d4e810d7f..544dd8a6fa 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -23,15 +23,15 @@
 #include "qemu/module.h"
 #include "qapi/error.h"
 
-void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
+void qbus_set_hotplug_handler(BusState *bus, Object *handler)
 {
 object_property_set_link(OBJECT(bus), handler,
- QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
+ QDEV_HOTPLUG_HANDLER_PROPERTY, &error_abort);
 }
 
 void qbus_set_bus_hotplug_handler(BusState *bus)
 {
-qbus_set_hotplug_handler(bus, OBJECT(bus), &error_abort);
+qbus_set_hotplug_handler(bus, OBJECT(bus));
 }
 
 int qbus_walk_children(BusState *bus,
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 086d0dfceb..5b48bae0f6 100

[PATCH v2 12/25] aspeed: Clean up roundabout error propagation

[Markus Armbruster]

Replace

sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
error_propagate(&err, local_err);
if (err) {
error_propagate(errp, err);
return;
}

by

sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
if (err) {
error_propagate(errp, err);
return;
}

Cc: Cédric Le Goater 
Signed-off-by: Markus Armbruster 
Reviewed-by: Cédric Le Goater 
---
 hw/arm/aspeed_ast2600.c | 10 --
 hw/arm/aspeed_soc.c | 10 --
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 6da687299f..08b3592e36 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -228,7 +228,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 int i;
 AspeedSoCState *s = ASPEED_SOC(dev);
 AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
-Error *err = NULL, *local_err = NULL;
+Error *err = NULL;
 qemu_irq irq;
 
 /* IO space */
@@ -394,8 +394,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 return;
 }
 object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
-sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
-error_propagate(&err, local_err);
+sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
 if (err) {
 error_propagate(errp, err);
 return;
@@ -446,11 +445,10 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 for (i = 0; i < sc->macs_num; i++) {
 object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
  &err);
-sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &local_err);
-error_propagate(&err, local_err);
+sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &err);
 if (err) {
 error_propagate(errp, err);
-   return;
+return;
 }
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
 sc->memmap[ASPEED_ETH1 + i]);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 810cf9b6cc..ec21de50ce 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -218,7 +218,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 int i;
 AspeedSoCState *s = ASPEED_SOC(dev);
 AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
-Error *err = NULL, *local_err = NULL;
+Error *err = NULL;
 
 /* IO space */
 create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
@@ -340,8 +340,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 /* SPI */
 for (i = 0; i < sc->spis_num; i++) {
 object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
-sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
-error_propagate(&err, local_err);
+sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
 if (err) {
 error_propagate(errp, err);
 return;
@@ -392,11 +391,10 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 for (i = 0; i < sc->macs_num; i++) {
 object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
  &err);
-sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &local_err);
-error_propagate(&err, local_err);
+sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &err);
 if (err) {
 error_propagate(errp, err);
-   return;
+return;
 }
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
 sc->memmap[ASPEED_ETH1 + i]);
-- 
2.26.2

[PATCH v2 05/25] tests: Use error_free_or_abort() where appropriate

[Markus Armbruster]

Replace

g_assert(err != NULL);
error_free(err);
err = NULL;

and variations thereof by

error_free_or_abort(&err);

Signed-off-by: Markus Armbruster 
---
 tests/check-block-qdict.c   | 24 ++--
 tests/check-qom-proplist.c  |  7 ++-
 tests/test-base64.c |  3 +--
 tests/test-bdrv-graph-mod.c |  4 +---
 tests/test-block-iothread.c |  3 +--
 tests/test-crypto-cipher.c  |  8 ++--
 tests/test-io-task.c|  4 +---
 7 files changed, 14 insertions(+), 39 deletions(-)

diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 73d3e9f574..5a25825093 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -610,9 +610,7 @@ static void qdict_rename_keys_test(void)
 copy = qdict_clone_shallow(dict);
 qdict_rename_keys(copy, renames, &local_err);
 
-g_assert(local_err != NULL);
-error_free(local_err);
-local_err = NULL;
+error_free_or_abort(&local_err);
 
 g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo");
 g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar");
@@ -649,9 +647,7 @@ static void qdict_crumple_test_bad_inputs(void)
 qdict_put_str(src, "rule.0.policy", "allow");
 
 g_assert(qdict_crumple(src, &error) == NULL);
-g_assert(error != NULL);
-error_free(error);
-error = NULL;
+error_free_or_abort(&error);
 qobject_unref(src);
 
 src = qdict_new();
@@ -660,9 +656,7 @@ static void qdict_crumple_test_bad_inputs(void)
 qdict_put_str(src, "rule.a", "allow");
 
 g_assert(qdict_crumple(src, &error) == NULL);
-g_assert(error != NULL);
-error_free(error);
-error = NULL;
+error_free_or_abort(&error);
 qobject_unref(src);
 
 src = qdict_new();
@@ -673,9 +667,7 @@ static void qdict_crumple_test_bad_inputs(void)
 qdict_put_str(src, "rule.b", "allow");
 
 g_assert(qdict_crumple(src, &error) == NULL);
-g_assert(error != NULL);
-error_free(error);
-error = NULL;
+error_free_or_abort(&error);
 qobject_unref(src);
 
 src = qdict_new();
@@ -684,9 +676,7 @@ static void qdict_crumple_test_bad_inputs(void)
 qdict_put_str(src, "rule.3", "allow");
 
 g_assert(qdict_crumple(src, &error) == NULL);
-g_assert(error != NULL);
-error_free(error);
-error = NULL;
+error_free_or_abort(&error);
 qobject_unref(src);
 
 src = qdict_new();
@@ -695,9 +685,7 @@ static void qdict_crumple_test_bad_inputs(void)
 qdict_put_str(src, "rule.+1", "allow");
 
 g_assert(qdict_crumple(src, &error) == NULL);
-g_assert(error != NULL);
-error_free(error);
-error = NULL;
+error_free_or_abort(&error);
 qobject_unref(src);
 }
 
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 8c71734e1a..e1e0a96661 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -491,17 +491,14 @@ static void test_dummy_getenum(void)
"av",
"BadAnimal",
&err);
-g_assert(err != NULL);
-error_free(err);
-err = NULL;
+error_free_or_abort(&err);
 
 /* A non-enum property name */
 val = object_property_get_enum(OBJECT(dobj),
"iv",
"DummyAnimal",
&err);
-g_assert(err != NULL);
-error_free(err);
+error_free_or_abort(&err);
 
 object_unparent(OBJECT(dobj));
 }
diff --git a/tests/test-base64.c b/tests/test-base64.c
index ec122ceba5..a7f722c459 100644
--- a/tests/test-base64.c
+++ b/tests/test-base64.c
@@ -54,10 +54,9 @@ static void test_base64_bad(const char *input,
  &len,
  &err);
 
-g_assert(err != NULL);
+error_free_or_abort(&err);
 g_assert(actual == NULL);
 g_assert_cmpint(len, ==, 0);
-error_free(err);
 }
 
 
diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
index f93f3168b0..8cff13830e 100644
--- a/tests/test-bdrv-graph-mod.c
+++ b/tests/test-bdrv-graph-mod.c
@@ -115,9 +115,7 @@ static void test_update_perm_tree(void)
   BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);
 
 bdrv_append(filter, bs, &local_err);
-
-g_assert_nonnull(local_err);
-error_free(local_err);
+error_free_or_abort(&local_err);
 
 blk_unref(root);
 }
diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c
index a953794be2..3f866a35c6 100644
--- a/tests/test-block-iothread.c
+++ b/tests/test-block-iothread.c
@@ -650,8 +650,7 @@ static void test_propagate_mirror(void)
 blk_insert_bs(blk, src, &error_abort);
 
 bdrv_try_set_aio_context(target, ctx, &local_err);
-g_assert(local_err);
-error_free(local_err);
+error_free_or_abort(&local_err);
 
 g_assert(blk_get_aio_context(blk) == main_ctx);
 g_assert(bdrv_get_aio_context(src) == main_ctx);
diff --git a/te

[PATCH v2 08/25] qga: Plug unlikely memory leak in guest-set-memory-blocks

[Markus Armbruster]

transfer_memory_block() leaks an Error object when reading file
/sys/devices/system/memory/memory/state fails with errno other
than ENOENT, and @sys2memblk is false, i.e. when the state file exists
but cannot be read (seems quite unlikely), and this is
guest-set-memory-blocks, not guest-get-memory-blocks.

Plug the leak.

Fixes: bd240fca42d5f072fb758a71720d9de9990ac553
Cc: Michael Roth 
Cc: Hailiang Zhang 
Signed-off-by: Markus Armbruster 
Reviewed-by: zhanghailiang 
---
 qga/commands-posix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index ae1348dc8f..cdbeb59dcc 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2421,6 +2421,7 @@ static void transfer_memory_block(GuestMemoryBlock 
*mem_blk, bool sys2memblk,
 if (sys2memblk) {
 error_propagate(errp, local_err);
 } else {
+error_free(local_err);
 result->response =
 GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
 }
-- 
2.26.2

[PATCH v2 03/25] Clean up some calls to ignore Error objects the right way

[Markus Armbruster]

Receiving the error in a local variable only to free it is less clear
(and also less efficient) than passing NULL.  Clean up.

Cc: Daniel P. Berrange 
Cc: Jerome Forissier 
CC: Greg Kurz 
Signed-off-by: Markus Armbruster 
---
 chardev/char-socket.c | 6 ++
 hw/9pfs/9p.c  | 6 ++
 hw/arm/virt.c | 4 +---
 hw/ppc/spapr_drc.c| 4 +---
 ui/vnc.c  | 3 +--
 5 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index afebeec5c3..b0cae97960 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -815,22 +815,20 @@ static void tcp_chr_tls_init(Chardev *chr)
 {
 SocketChardev *s = SOCKET_CHARDEV(chr);
 QIOChannelTLS *tioc;
-Error *err = NULL;
 gchar *name;
 
 if (s->is_listen) {
 tioc = qio_channel_tls_new_server(
 s->ioc, s->tls_creds,
 s->tls_authz,
-&err);
+NULL);
 } else {
 tioc = qio_channel_tls_new_client(
 s->ioc, s->tls_creds,
 s->addr->u.inet.host,
-&err);
+NULL);
 }
 if (tioc == NULL) {
-error_free(err);
 tcp_chr_disconnect(chr);
 return;
 }
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 45a788f6e6..9755fba9a9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1399,7 +1399,6 @@ static void coroutine_fn v9fs_attach(void *opaque)
 size_t offset = 7;
 V9fsQID qid;
 ssize_t err;
-Error *local_err = NULL;
 
 v9fs_string_init(&uname);
 v9fs_string_init(&aname);
@@ -1437,9 +1436,8 @@ static void coroutine_fn v9fs_attach(void *opaque)
 error_setg(&s->migration_blocker,
"Migration is disabled when VirtFS export path '%s' is 
mounted in the guest using mount_tag '%s'",
s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
-err = migrate_add_blocker(s->migration_blocker, &local_err);
-if (local_err) {
-error_free(local_err);
+err = migrate_add_blocker(s->migration_blocker, NULL);
+if (err < 0) {
 error_free(s->migration_blocker);
 s->migration_blocker = NULL;
 clunk_fid(s, fid);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index caceb1e4a0..29b9d5b2e6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -217,11 +217,9 @@ static bool cpu_type_valid(const char *cpu)
 
 static void create_kaslr_seed(VirtMachineState *vms, const char *node)
 {
-Error *err = NULL;
 uint64_t seed;
 
-if (qemu_guest_getrandom(&seed, sizeof(seed), &err)) {
-error_free(err);
+if (qemu_guest_getrandom(&seed, sizeof(seed), NULL)) {
 return;
 }
 qemu_fdt_setprop_u64(vms->fdt, node, "kaslr-seed", seed);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 2689104295..951bcdf2c0 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -1163,16 +1163,14 @@ static void rtas_ibm_configure_connector(PowerPCCPU 
*cpu,
 drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
 if (!drc->fdt) {
-Error *local_err = NULL;
 void *fdt;
 int fdt_size;
 
 fdt = create_device_tree(&fdt_size);
 
 if (drck->dt_populate(drc, spapr, fdt, &drc->fdt_start_offset,
-  &local_err)) {
+  NULL)) {
 g_free(fdt);
-error_free(local_err);
 rc = SPAPR_DR_CC_RESPONSE_ERROR;
 goto out;
 }
diff --git a/ui/vnc.c b/ui/vnc.c
index 12a12714e1..0702a76cce 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -458,9 +458,8 @@ static VncServerInfo2List 
*qmp_query_server_entry(QIOChannelSocket *ioc,
 Error *err = NULL;
 SocketAddress *addr;
 
-addr = qio_channel_socket_get_local_address(ioc, &err);
+addr = qio_channel_socket_get_local_address(ioc, NULL);
 if (!addr) {
-error_free(err);
 return prev;
 }
 
-- 
2.26.2

[PATCH v2 17/25] riscv/sifive_u: Fix sifive_u_soc_realize() error API violations

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

sifive_u_soc_realize() is wrong that way: it passes &err to
sysbus_realize() four times before checking it.  Harmless, because the
first three can't actually fail (I think).

Fix by checking for failure right away.

Cc: Palmer Dabbelt 
Cc: Alistair Francis 
Cc: Sagar Karandikar 
Cc: Bastian Koppelmann 
Cc: Bin Meng 
Cc: qemu-ri...@nongnu.org
Signed-off-by: Markus Armbruster 
Reviewed-by: Alistair Francis 
---
 hw/riscv/sifive_u.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 7d051e7c92..a1d2edfe13 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -677,11 +677,15 @@ static void sifive_u_soc_realize(DeviceState *dev, Error 
**errp)
 memmap[SIFIVE_U_CLINT].size, ms->smp.cpus,
 SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE, false);
 
-sysbus_realize(SYS_BUS_DEVICE(&s->prci), &err);
+if (!sysbus_realize(SYS_BUS_DEVICE(&s->prci), errp)) {
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base);
 
 qdev_prop_set_uint32(DEVICE(&s->gpio), "ngpio", 16);
-sysbus_realize(SYS_BUS_DEVICE(&s->gpio), &err);
+if (!sysbus_realize(SYS_BUS_DEVICE(&s->gpio), errp)) {
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio), 0, memmap[SIFIVE_U_GPIO].base);
 
 /* Pass all GPIOs to the SOC layer so they are available to the board */
@@ -695,7 +699,9 @@ static void sifive_u_soc_realize(DeviceState *dev, Error 
**errp)
 }
 
 qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
-sysbus_realize(SYS_BUS_DEVICE(&s->otp), &err);
+if (!sysbus_realize(SYS_BUS_DEVICE(&s->otp), errp)) {
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base);
 
 if (nd->used) {
-- 
2.26.2

[PATCH v2 06/25] usb/dev-mtp: Fix Error double free after inotify failure

[Markus Armbruster]

error_report_err() frees its first argument.  Freeing it again is
wrong.  Don't.

Fixes: 47287c27d0c367a89f7b2851e23a7f8b2d499dd6
Cc: Gerd Hoffmann 
Cc: Daniel P. Berrangé 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Markus Armbruster 
---
 hw/usb/dev-mtp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 168428156b..15a2243101 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -634,7 +634,6 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject 
*o)
 error_reportf_err(err,
   "usb-mtp: failed to add watch for %s: ",
   o->path);
-error_free(err);
 } else {
 trace_usb_mtp_file_monitor_event(s->dev.addr, o->path,
  "Watch Added");
@@ -1279,7 +1278,6 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
 if (err) {
 error_reportf_err(err,
   "usb-mtp: file monitoring init failed: ");
-error_free(err);
 } else {
 QTAILQ_INIT(&s->events);
 }
-- 
2.26.2

[PATCH v2 04/25] tests: Use &error_abort where appropriate

[Markus Armbruster]

Receiving the error in a local variable only to assert there is none
is less clear than passing &error_abort.  Clean up.

Signed-off-by: Markus Armbruster 
Reviewed-by: Thomas Huth 
---
 tests/check-qobject.c  |   5 +-
 tests/check-qom-proplist.c |   7 +-
 tests/test-logging.c   |  12 +---
 tests/test-qemu-opts.c |  22 ++
 tests/test-replication.c   | 109 +
 tests/test-string-input-visitor.c  |  33 +++--
 tests/test-string-output-visitor.c |  16 ++---
 7 files changed, 59 insertions(+), 145 deletions(-)

diff --git a/tests/check-qobject.c b/tests/check-qobject.c
index 593c3a0618..6b6deaeb8b 100644
--- a/tests/check-qobject.c
+++ b/tests/check-qobject.c
@@ -9,6 +9,7 @@
 
 #include "qemu/osdep.h"
 #include "block/qdict.h"
+#include "qapi/error.h"
 #include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
@@ -213,7 +214,6 @@ static void qobject_is_equal_list_test(void)
 
 static void qobject_is_equal_dict_test(void)
 {
-Error *local_err = NULL;
 QDict *dict_0, *dict_1, *dict_cloned;
 QDict *dict_different_key, *dict_different_value, *dict_different_null_key;
 QDict *dict_longer, *dict_shorter, *dict_nested;
@@ -276,8 +276,7 @@ static void qobject_is_equal_dict_test(void)
   dict_different_null_key, dict_longer, dict_shorter,
   dict_nested);
 
-dict_crumpled = qobject_to(QDict, qdict_crumple(dict_1, &local_err));
-g_assert(!local_err);
+dict_crumpled = qobject_to(QDict, qdict_crumple(dict_1, &error_abort));
 check_equal(dict_crumpled, dict_nested);
 
 qdict_flatten(dict_nested);
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 13a824cfae..8c71734e1a 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -419,9 +419,7 @@ static void test_dummy_createcmdl(void)
 g_assert(dobj->bv == true);
 g_assert(dobj->av == DUMMY_PLATYPUS);
 
-user_creatable_del("dev0", &err);
-g_assert(err == NULL);
-error_free(err);
+user_creatable_del("dev0", &error_abort);
 
 object_unref(OBJECT(dobj));
 
@@ -485,8 +483,7 @@ static void test_dummy_getenum(void)
 val = object_property_get_enum(OBJECT(dobj),
"av",
"DummyAnimal",
-   &err);
-g_assert(err == NULL);
+   &error_abort);
 g_assert(val == DUMMY_PLATYPUS);
 
 /* A bad enum type name */
diff --git a/tests/test-logging.c b/tests/test-logging.c
index 8580b82420..8a1161de1d 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -113,7 +113,6 @@ static void test_logfile_write(gconstpointer data)
 QemuLogFile *logfile;
 QemuLogFile *logfile2;
 gchar const *dir = data;
-Error *err = NULL;
 g_autofree gchar *file_path = NULL;
 g_autofree gchar *file_path1 = NULL;
 FILE *orig_fd;
@@ -132,8 +131,7 @@ static void test_logfile_write(gconstpointer data)
  * Test that even if an open file handle is changed,
  * our handle remains valid due to RCU.
  */
-qemu_set_log_filename(file_path, &err);
-g_assert(!err);
+qemu_set_log_filename(file_path, &error_abort);
 rcu_read_lock();
 logfile = atomic_rcu_read(&qemu_logfile);
 orig_fd = logfile->fd;
@@ -142,8 +140,7 @@ static void test_logfile_write(gconstpointer data)
 fflush(logfile->fd);
 
 /* Change the logfile and ensure that the handle is still valid. */
-qemu_set_log_filename(file_path1, &err);
-g_assert(!err);
+qemu_set_log_filename(file_path1, &error_abort);
 logfile2 = atomic_rcu_read(&qemu_logfile);
 g_assert(logfile->fd == orig_fd);
 g_assert(logfile2->fd != logfile->fd);
@@ -156,7 +153,6 @@ static void test_logfile_lock(gconstpointer data)
 {
 FILE *logfile;
 gchar const *dir = data;
-Error *err = NULL;
 g_autofree gchar *file_path = NULL;
 
 file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL);
@@ -166,7 +162,7 @@ static void test_logfile_lock(gconstpointer data)
  * that even if an open file handle is closed,
  * our handle remains valid for use due to RCU.
  */
-qemu_set_log_filename(file_path, &err);
+qemu_set_log_filename(file_path, &error_abort);
 logfile = qemu_log_lock();
 g_assert(logfile);
 fprintf(logfile, "%s 1st write to file\n", __func__);
@@ -180,8 +176,6 @@ static void test_logfile_lock(gconstpointer data)
 fprintf(logfile, "%s 2nd write to file\n", __func__);
 fflush(logfile);
 qemu_log_unlock(logfile);
-
-g_assert(!err);
 }
 
 /* Remove a directory and all its entries (non-recursive). */
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 2a0f42a09b..297ffe79dd 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -187,7 +187,6 @@ static void test_qemu_opt_get(void)
 
 static void test_qemu_opt_get_bool

[PATCH v2 09/25] sd/milkymist-memcard: Plug minor memory leak in realize

[Markus Armbruster]

milkymist_memcard_realize() leaks an Error object when realization of
its "sd-card" device fails.  Quite harmless, since we only ever
realize this once, in milkymist_init() via milkymist_memcard_create().

Plug the leak.

Fixes: 3d0369ba499866cc6a839f71212d97876500762d
Cc: Philippe Mathieu-Daudé 
Cc: Michael Walle 
Signed-off-by: Markus Armbruster 
---
 hw/sd/milkymist-memcard.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 482e97191e..afdb8aa0c0 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -280,9 +280,8 @@ static void milkymist_memcard_realize(DeviceState *dev, 
Error **errp)
 blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
 carddev = qdev_new(TYPE_SD_CARD);
 qdev_prop_set_drive(carddev, "drive", blk);
-qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err);
-if (err) {
-error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
+if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err)) {
+error_propagate_prepend(errp, err, "failed to init SD card: %s");
 return;
 }
 s->enabled = blk && blk_is_inserted(blk);
-- 
2.26.2

[PATCH v2 01/25] net/virtio: Fix failover_replug_primary() return value regression

[Markus Armbruster]

Commit 150ab54aa6 "net/virtio: fix re-plugging of primary device"
fixed failover_replug_primary() to return false on failure.  Commit
5a0948d36c "net/virtio: Fix failover error handling crash bugs" broke
it again for hotplug_handler_plug() failure.  Unbreak it.

Commit 5a0948d36c4cbc1c5534afac6fee99de55245d12

Fixes: 5a0948d36c4cbc1c5534afac6fee99de55245d12
Cc: Jens Freimann 
Cc: Michael S. Tsirkin 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Markus Armbruster 
Reviewed-by: Jens Freimann 
---
 hw/net/virtio-net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index aff67a92df..9bb5578e5d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3129,7 +3129,7 @@ static bool failover_replug_primary(VirtIONet *n, Error 
**errp)
 if (err) {
 goto out;
 }
-hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
+hotplug_handler_plug(hotplug_ctrl, n->primary_dev, &err);
 }
 
 out:
-- 
2.26.2

[PATCH v2 22/25] arm/stm32f205 arm/stm32f405: Fix realize error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

stm32f205_soc_realize() and stm32f405_soc_realize() are wrong that
way: they pass &err to object_property_set_int() without checking it,
and then to qdev_realize().  Harmless, because the former can't
actually fail here.

Fix by passing &error_abort instead.

Cc: Alistair Francis 
Cc: Peter Maydell 
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster 
---
 hw/arm/stm32f205_soc.c | 2 +-
 hw/arm/stm32f405_soc.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
index 19487544f0..56aef686c9 100644
--- a/hw/arm/stm32f205_soc.c
+++ b/hw/arm/stm32f205_soc.c
@@ -154,7 +154,7 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, 
Error **errp)
 
 /* ADC 1 to 3 */
 object_property_set_int(OBJECT(s->adc_irqs), STM_NUM_ADCS,
-"num-lines", &err);
+"num-lines", &error_abort);
 qdev_realize(DEVICE(s->adc_irqs), NULL, &err);
 if (err != NULL) {
 error_propagate(errp, err);
diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
index c12d9f999d..cf9228d8e7 100644
--- a/hw/arm/stm32f405_soc.c
+++ b/hw/arm/stm32f405_soc.c
@@ -172,7 +172,7 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, 
Error **errp)
 return;
 }
 object_property_set_int(OBJECT(&s->adc_irqs), STM_NUM_ADCS,
-"num-lines", &err);
+"num-lines", &error_abort);
 qdev_realize(DEVICE(&s->adc_irqs), NULL, &err);
 if (err != NULL) {
 error_propagate(errp, err);
-- 
2.26.2

[PATCH v2 13/25] qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp

[Markus Armbruster]

All callers pass &error_abort.  Drop the parameter.

Cc: Paolo Bonzini 
Cc: "Daniel P. Berrangé" 
Cc: Eduardo Habkost 
Signed-off-by: Markus Armbruster 
---
 include/hw/qdev-core.h  | 2 +-
 hw/core/bus.c   | 4 ++--
 hw/scsi/scsi-bus.c  | 2 +-
 hw/usb/bus.c| 2 +-
 hw/xen/xen-bus.c| 2 +-
 hw/xen/xen-legacy-backend.c | 2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 7dc10be46f..78acdeaed6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -537,7 +537,7 @@ char *qdev_get_dev_path(DeviceState *dev);
 
 void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp);
 
-void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp);
+void qbus_set_bus_hotplug_handler(BusState *bus);
 
 static inline bool qbus_is_hotpluggable(BusState *bus)
 {
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 6cc28b334e..8d4e810d7f 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -29,9 +29,9 @@ void qbus_set_hotplug_handler(BusState *bus, Object *handler, 
Error **errp)
  QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
 }
 
-void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp)
+void qbus_set_bus_hotplug_handler(BusState *bus)
 {
-qbus_set_hotplug_handler(bus, OBJECT(bus), errp);
+qbus_set_hotplug_handler(bus, OBJECT(bus), &error_abort);
 }
 
 int qbus_walk_children(BusState *bus,
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 27843bb04b..b878a08080 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -107,7 +107,7 @@ void scsi_bus_new(SCSIBus *bus, size_t bus_size, 
DeviceState *host,
 qbus_create_inplace(bus, bus_size, TYPE_SCSI_BUS, host, bus_name);
 bus->busnr = next_scsi_bus++;
 bus->info = info;
-qbus_set_bus_hotplug_handler(BUS(bus), &error_abort);
+qbus_set_bus_hotplug_handler(BUS(bus));
 }
 
 static void scsi_dma_restart_bh(void *opaque)
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index a81aee2051..957559b18d 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -84,7 +84,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size,
  USBBusOps *ops, DeviceState *host)
 {
 qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL);
-qbus_set_bus_hotplug_handler(BUS(bus), &error_abort);
+qbus_set_bus_hotplug_handler(BUS(bus));
 bus->ops = ops;
 bus->busnr = next_usb_bus++;
 QTAILQ_INIT(&bus->free);
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 4b00320f1c..c4e2162ae9 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1391,5 +1391,5 @@ void xen_bus_init(void)
 BusState *bus = qbus_create(TYPE_XEN_BUS, dev, NULL);
 
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-qbus_set_bus_hotplug_handler(bus, &error_abort);
+qbus_set_bus_hotplug_handler(bus);
 }
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 2335ee2e65..7d4b13351e 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -705,7 +705,7 @@ int xen_be_init(void)
 xen_sysdev = qdev_new(TYPE_XENSYSDEV);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(xen_sysdev), &error_fatal);
 xen_sysbus = qbus_create(TYPE_XENSYSBUS, xen_sysdev, "xen-sysbus");
-qbus_set_bus_hotplug_handler(xen_sysbus, &error_abort);
+qbus_set_bus_hotplug_handler(xen_sysbus);
 
 return 0;
 
-- 
2.26.2

[PATCH v2 25/25] arm/{bcm2835, fsl-imx25, fsl-imx6}: Fix realize error API violations

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

bcm2835_peripherals_realize(), fsl_imx25_realize() and
fsl_imx6_realize() are wrong that way: they pass &err to
object_property_set_uint() and object_property_set_bool() without
checking it, and then to sysbus_realize().  Harmless, because the
former can't actually fail here.

Fix by passing &error_abort instead.

Cc: Peter Maydell 
Cc: Andrew Baumann 
Cc: "Philippe Mathieu-Daudé" 
Cc: Jean-Christophe Dubois 
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster 
---
 hw/arm/bcm2835_peripherals.c | 12 
 hw/arm/fsl-imx25.c   | 12 +---
 hw/arm/fsl-imx6.c| 12 +---
 3 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 1e975d7eec..7ffdf62067 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -283,16 +283,12 @@ static void bcm2835_peripherals_realize(DeviceState *dev, 
Error **errp)
  * For the exact details please refer to the Arasan documentation:
  *   SD3.0_Host_AHB_eMMC4.4_Usersguide_ver5.9_jan11_10.pdf
  */
-object_property_set_uint(OBJECT(&s->sdhci), 3, "sd-spec-version", &err);
+object_property_set_uint(OBJECT(&s->sdhci), 3, "sd-spec-version",
+ &error_abort);
 object_property_set_uint(OBJECT(&s->sdhci), BCM2835_SDHC_CAPAREG, 
"capareg",
- &err);
+ &error_abort);
 object_property_set_bool(OBJECT(&s->sdhci), true, "pending-insert-quirk",
- &err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
-
+ &error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->sdhci), &err);
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index f32f9bce0f..7ab5c98fbe 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -260,15 +260,13 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
**errp)
 };
 
 object_property_set_uint(OBJECT(&s->esdhc[i]), 2, "sd-spec-version",
- &err);
+ &error_abort);
 object_property_set_uint(OBJECT(&s->esdhc[i]), 
IMX25_ESDHC_CAPABILITIES,
- "capareg", &err);
+ "capareg",
+ &error_abort);
 object_property_set_uint(OBJECT(&s->esdhc[i]), SDHCI_VENDOR_IMX,
- "vendor", &err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
+ "vendor",
+ &error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->esdhc[i]), &err);
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index d4bc4fae93..4ae3c3efc2 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -336,15 +336,13 @@ static void fsl_imx6_realize(DeviceState *dev, Error 
**errp)
 
 /* UHS-I SDIO3.0 SDR104 1.8V ADMA */
 object_property_set_uint(OBJECT(&s->esdhc[i]), 3, "sd-spec-version",
- &err);
+ &error_abort);
 object_property_set_uint(OBJECT(&s->esdhc[i]), IMX6_ESDHC_CAPABILITIES,
- "capareg", &err);
+ "capareg",
+ &error_abort);
 object_property_set_uint(OBJECT(&s->esdhc[i]), SDHCI_VENDOR_IMX,
- "vendor", &err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
+ "vendor",
+ &error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->esdhc[i]), &err);
 if (err) {
 error_propagate(errp, err);
-- 
2.26.2

[PATCH v2 18/25] riscv_hart: Fix riscv_harts_realize() error API violations

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

riscv_harts_realize() is wrong that way: it passes @errp to
riscv_hart_realize() in a loop.  I can't tell offhand whether this can
fail.

Fix by checking for failure in each iteration.

Cc: Palmer Dabbelt 
Cc: Alistair Francis 
Cc: Sagar Karandikar 
Cc: Bastian Koppelmann 
Cc: Bin Meng 
Cc: qemu-ri...@nongnu.org
Signed-off-by: Markus Armbruster 
Reviewed-by: Alistair Francis 
---
 hw/riscv/riscv_hart.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index e26c382259..f59fe52f0f 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -40,19 +40,13 @@ static void riscv_harts_cpu_reset(void *opaque)
 cpu_reset(CPU(cpu));
 }
 
-static void riscv_hart_realize(RISCVHartArrayState *s, int idx,
+static bool riscv_hart_realize(RISCVHartArrayState *s, int idx,
char *cpu_type, Error **errp)
 {
-Error *err = NULL;
-
 object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type);
 s->harts[idx].env.mhartid = s->hartid_base + idx;
 qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]);
-qdev_realize(DEVICE(&s->harts[idx]), NULL, &err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
+return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp);
 }
 
 static void riscv_harts_realize(DeviceState *dev, Error **errp)
@@ -63,7 +57,9 @@ static void riscv_harts_realize(DeviceState *dev, Error 
**errp)
 s->harts = g_new0(RISCVCPU, s->num_harts);
 
 for (n = 0; n < s->num_harts; n++) {
-riscv_hart_realize(s, n, s->cpu_type, errp);
+if (!riscv_hart_realize(s, n, s->cpu_type, errp)) {
+return;
+}
 }
 }
 
-- 
2.26.2

Re: [PATCH] trivial: Remove extra character in configure help

[Christophe de Dinechin]

Please ignore. The =B appears intentional, even if it offsets the whole help 
text.

Maybe replace with =L to indicate a list is expected?

> On 24 Jun 2020, at 10:33, Christophe de Dinechin  wrote:
> 
> Signed-off-by: Christophe de Dinechin 
> ---
> configure | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index ba88fd1824..c7a6a5adfe 100755
> --- a/configure
> +++ b/configure
> @@ -1787,7 +1787,7 @@ Advanced options (experts only):
>   --block-drv-ro-whitelist=L
>set block driver read-only whitelist
>(affects only QEMU, not qemu-img)
> -  --enable-trace-backends=B Set trace backend
> +  --enable-trace-backends= Set trace backend
>Available backends: $trace_backend_list
>   --with-trace-file=NAME   Full PATH,NAME of file to store traces
>Default:trace-
> -- 
> 2.26.2
> 
> 

[PATCH v2 11/25] vnc: Plug minor memory leak in vnc_display_open()

[Markus Armbruster]

vnc_display_print_local_addr() leaks the Error object when
qio_channel_socket_get_local_address() fails.  Seems unlikely.  Called
when we create a VNC display with vnc_display_open().  Plug the leak
by passing NULL to ignore the error.

Cc: Daniel P. Berrange 
Cc: Gerd Hoffmann 
Signed-off-by: Markus Armbruster 
---
 ui/vnc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 0702a76cce..527ad25124 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3274,13 +3274,12 @@ int vnc_display_pw_expire(const char *id, time_t 
expires)
 static void vnc_display_print_local_addr(VncDisplay *vd)
 {
 SocketAddress *addr;
-Error *err = NULL;
 
 if (!vd->listener || !vd->listener->nsioc) {
 return;
 }
 
-addr = qio_channel_socket_get_local_address(vd->listener->sioc[0], &err);
+addr = qio_channel_socket_get_local_address(vd->listener->sioc[0], NULL);
 if (!addr) {
 return;
 }
-- 
2.26.2

[PATCH v2 02/25] pci: Delete useless error_propagate()

[Markus Armbruster]

Cc: Jens Freimann 
Cc: Michael S. Tsirkin 
Cc: Marcel Apfelbaum 
Signed-off-by: Markus Armbruster 
Reviewed-by: Jens Freimann 
---
 hw/pci/pci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b22dedc88c..de0fae10ab 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2123,7 +2123,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
**errp)
 if (!pci_bus_is_express(pci_get_bus(pci_dev))) {
 error_setg(errp, "failover primary device must be on "
  "PCIExpress bus");
-error_propagate(errp, local_err);
 pci_qdev_unrealize(DEVICE(pci_dev));
 return;
 }
@@ -2131,7 +2130,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
**errp)
 if (class_id != PCI_CLASS_NETWORK_ETHERNET) {
 error_setg(errp, "failover primary device is not an "
  "Ethernet device");
-error_propagate(errp, local_err);
 pci_qdev_unrealize(DEVICE(pci_dev));
 return;
 }
@@ -2141,7 +2139,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
**errp)
 } else {
 error_setg(errp, "failover: primary device must be in its own "
   "PCI slot");
-error_propagate(errp, local_err);
 pci_qdev_unrealize(DEVICE(pci_dev));
 return;
 }
-- 
2.26.2

[PATCH v2 19/25] mips/cps: Fix mips_cps_realize() error API violations

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

mips_cps_realize() is wrong that way: it passes &err to multiple
object_property_set_FOO() without checking for failure, and then to
sysbus_realize().  Harmless, because the object_property_set_FOO()
can't actually fail here.

Fix by passing &error_abort instead.

Cc: Aleksandar Markovic 
Cc: Aurelien Jarno 
Cc: Aleksandar Rikalo 
Signed-off-by: Markus Armbruster 
---
 hw/mips/cps.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 5382bc86f7..0d7f3cf673 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -100,10 +100,12 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 /* Inter-Thread Communication Unit */
 if (itu_present) {
 object_initialize_child(OBJECT(dev), "itu", &s->itu, TYPE_MIPS_ITU);
-object_property_set_int(OBJECT(&s->itu), 16, "num-fifo", &err);
-object_property_set_int(OBJECT(&s->itu), 16, "num-semaphores", &err);
+object_property_set_int(OBJECT(&s->itu), 16, "num-fifo",
+&error_abort);
+object_property_set_int(OBJECT(&s->itu), 16, "num-semaphores",
+&error_abort);
 object_property_set_bool(OBJECT(&s->itu), saar_present, "saar-present",
- &err);
+ &error_abort);
 if (saar_present) {
 s->itu.saar = &env->CP0_SAAR;
 }
@@ -119,8 +121,10 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 
 /* Cluster Power Controller */
 object_initialize_child(OBJECT(dev), "cpc", &s->cpc, TYPE_MIPS_CPC);
-object_property_set_int(OBJECT(&s->cpc), s->num_vp, "num-vp", &err);
-object_property_set_int(OBJECT(&s->cpc), 1, "vp-start-running", &err);
+object_property_set_int(OBJECT(&s->cpc), s->num_vp, "num-vp",
+&error_abort);
+object_property_set_int(OBJECT(&s->cpc), 1, "vp-start-running",
+&error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->cpc), &err);
 if (err != NULL) {
 error_propagate(errp, err);
@@ -132,8 +136,10 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 
 /* Global Interrupt Controller */
 object_initialize_child(OBJECT(dev), "gic", &s->gic, TYPE_MIPS_GIC);
-object_property_set_int(OBJECT(&s->gic), s->num_vp, "num-vp", &err);
-object_property_set_int(OBJECT(&s->gic), 128, "num-irq", &err);
+object_property_set_int(OBJECT(&s->gic), s->num_vp, "num-vp",
+&error_abort);
+object_property_set_int(OBJECT(&s->gic), 128, "num-irq",
+&error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->gic), &err);
 if (err != NULL) {
 error_propagate(errp, err);
@@ -147,9 +153,12 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 gcr_base = env->CP0_CMGCRBase << 4;
 
 object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
-object_property_set_int(OBJECT(&s->gcr), s->num_vp, "num-vp", &err);
-object_property_set_int(OBJECT(&s->gcr), 0x800, "gcr-rev", &err);
-object_property_set_int(OBJECT(&s->gcr), gcr_base, "gcr-base", &err);
+object_property_set_int(OBJECT(&s->gcr), s->num_vp, "num-vp",
+&error_abort);
+object_property_set_int(OBJECT(&s->gcr), 0x800, "gcr-rev",
+&error_abort);
+object_property_set_int(OBJECT(&s->gcr), gcr_base, "gcr-base",
+&error_abort);
 object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic",
  &error_abort);
 object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc",
-- 
2.26.2

[PATCH v2 23/25] aspeed: Fix realize error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

aspeed_soc_ast2600_realize() and aspeed_soc_realize() are wrong that
way: they pass &err to object_property_set_int() and
object_property_set_bool() without checking it, and then to
sysbus_realize().  Harmless, because the former can't actually fail
here.

Fix by passing &error_abort instead.

Cc: "Cédric Le Goater" 
Cc: Peter Maydell 
Cc: Andrew Jeffery 
Cc: Joel Stanley 
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster 
---
 hw/arm/aspeed_ast2600.c | 5 +++--
 hw/arm/aspeed_soc.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 4efac02e2b..59a7a1370b 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -383,7 +383,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 for (i = 0; i < sc->spis_num; i++) {
 object_property_set_link(OBJECT(&s->spi[i]), OBJECT(s->dram_mr),
  "dram", &error_abort);
-object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
+object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs",
+&error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
 if (err) {
 error_propagate(errp, err);
@@ -434,7 +435,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 /* Net */
 for (i = 0; i < sc->macs_num; i++) {
 object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
- &err);
+ &error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &err);
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 03b91bade6..311458aa76 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -333,7 +333,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 
 /* SPI */
 for (i = 0; i < sc->spis_num; i++) {
-object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
+object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs",
+&error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
 if (err) {
 error_propagate(errp, err);
@@ -384,7 +385,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 /* Net */
 for (i = 0; i < sc->macs_num; i++) {
 object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
- &err);
+ &error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &err);
 if (err) {
 error_propagate(errp, err);
-- 
2.26.2

[PATCH v2 24/25] hw/arm/armsse: Fix armsse_realize() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

armsse_realize() is wrong that way: it passes &err to
object_property_set_int() multiple times without checking it, and then
to sysbus_realize().  Harmless, because the former can't actually fail
here.

Fix by passing &error_abort instead.

Cc: Peter Maydell 
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster 
---
 hw/arm/armsse.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index c73cc6badf..e2cf43ee0b 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -991,13 +991,13 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->sysinfo), 0, 0x4002);
 /* System control registers */
 object_property_set_int(OBJECT(&s->sysctl), info->sys_version,
-"SYS_VERSION", &err);
+"SYS_VERSION", &error_abort);
 object_property_set_int(OBJECT(&s->sysctl), info->cpuwait_rst,
-"CPUWAIT_RST", &err);
+"CPUWAIT_RST", &error_abort);
 object_property_set_int(OBJECT(&s->sysctl), s->init_svtor,
-"INITSVTOR0_RST", &err);
+"INITSVTOR0_RST", &error_abort);
 object_property_set_int(OBJECT(&s->sysctl), s->init_svtor,
-"INITSVTOR1_RST", &err);
+"INITSVTOR1_RST", &error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->sysctl), &err);
 if (err) {
 error_propagate(errp, err);
-- 
2.26.2

[PATCH v2 21/25] amd_iommu: Fix amdvi_realize() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

amdvi_realize() is wrong that way: it passes @errp to qdev_realize(),
object_property_get_int(), and msi_init() without checking it.  I
can't tell offhand whether qdev_realize() can fail here.  Fix by
checking it for failure.  object_property_get_int() can't.  Fix by
passing &error_abort instead.

Cc: Paolo Bonzini 
Cc: Richard Henderson 
Cc: Eduardo Habkost 
Signed-off-by: Markus Armbruster 
---
 hw/i386/amd_iommu.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index b26d30da57..087f601666 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1549,7 +1549,9 @@ static void amdvi_realize(DeviceState *dev, Error **errp)
 
 /* This device should take care of IOMMU PCI properties */
 x86_iommu->type = TYPE_AMD;
-qdev_realize(DEVICE(&s->pci), &bus->qbus, errp);
+if (!qdev_realize(DEVICE(&s->pci), &bus->qbus, errp)) {
+return;
+}
 ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
  AMDVI_CAPAB_SIZE, errp);
 if (ret < 0) {
@@ -1578,7 +1580,7 @@ static void amdvi_realize(DeviceState *dev, Error **errp)
 sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
 sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
 pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
-s->devid = object_property_get_int(OBJECT(&s->pci), "addr", errp);
+s->devid = object_property_get_int(OBJECT(&s->pci), "addr", &error_abort);
 msi_init(&s->pci.dev, 0, 1, true, false, errp);
 amdvi_init(s);
 }
-- 
2.26.2

[PATCH v2 15/25] hw: Fix error API violation around object_property_set_link()

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

virtio_gpu_pci_base_realize(), virtio_vga_base_realize(),
sparc32_ledma_device_realize(), sparc32_dma_realize(),
sparc32_dma_realize() xilinx_axidma_realize(), mips_cps_realize(),
macio_realize_ide(), xilinx_enet_realize(), and
virtio_iommu_pci_realize() are wrong that way: they reuse the argument
they pass to object_property_set_link() for another call.

Harmless, because object_property_set_link() can't actually fail for
them: it fails when the property doesn't exist, is not settable, or
its .check() method fails.  Fix by passing &error_abort instead.

Cc: Gerd Hoffmann 
Cc: Mark Cave-Ayland 
Cc: "Edgar E. Iglesias" 
Cc: Alistair Francis 
Cc: Peter Maydell 
Cc: qemu-...@nongnu.org
Cc: Aleksandar Markovic 
Cc: Aurelien Jarno 
Cc: Aleksandar Rikalo 
Cc: Eric Auger 
Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Auger 
Reviewed-by: Alistair Francis 
---
 hw/display/virtio-gpu-pci.c  |  2 +-
 hw/display/virtio-vga.c  |  2 +-
 hw/dma/sparc32_dma.c |  6 +++---
 hw/dma/xilinx_axidma.c   | 12 ++--
 hw/mips/cps.c|  6 --
 hw/misc/macio/macio.c|  3 ++-
 hw/net/xilinx_axienet.c  | 12 ++--
 hw/virtio/virtio-iommu-pci.c |  2 +-
 8 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index b532fe8b5f..41b88b878d 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -44,7 +44,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 for (i = 0; i < g->conf.max_outputs; i++) {
 object_property_set_link(OBJECT(g->scanout[i].con),
  OBJECT(vpci_dev),
- "device", errp);
+ "device", &error_abort);
 }
 }
 
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 68a062ece6..67f409e106 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -154,7 +154,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 for (i = 0; i < g->conf.max_outputs; i++) {
 object_property_set_link(OBJECT(g->scanout[i].con),
  OBJECT(vpci_dev),
- "device", errp);
+ "device", &error_abort);
 }
 }
 
diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index f02aca6f40..2d7dbbb92d 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, 
Error **errp)
 d = qdev_new(TYPE_LANCE);
 object_property_add_child(OBJECT(dev), "lance", OBJECT(d));
 qdev_set_nic_properties(d, nd);
-object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp);
+object_property_set_link(OBJECT(d), OBJECT(dev), "dma", &error_abort);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(d), &error_fatal);
 }
 
@@ -379,7 +379,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error 
**errp)
 }
 
 espdma = qdev_new(TYPE_SPARC32_ESPDMA_DEVICE);
-object_property_set_link(OBJECT(espdma), iommu, "iommu", errp);
+object_property_set_link(OBJECT(espdma), iommu, "iommu", &error_abort);
 object_property_add_child(OBJECT(s), "espdma", OBJECT(espdma));
 sysbus_realize_and_unref(SYS_BUS_DEVICE(espdma), &error_fatal);
 
@@ -394,7 +394,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error 
**errp)
 sysbus_mmio_get_region(sbd, 0));
 
 ledma = qdev_new(TYPE_SPARC32_LEDMA_DEVICE);
-object_property_set_link(OBJECT(ledma), iommu, "iommu", errp);
+object_property_set_link(OBJECT(ledma), iommu, "iommu", &error_abort);
 object_property_add_child(OBJECT(s), "ledma", OBJECT(ledma));
 sysbus_realize_and_unref(SYS_BUS_DEVICE(ledma), &error_fatal);
 
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 6a9df2c4db..a069637bf2 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -537,7 +537,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error 
**errp)
 XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev);
 XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM(
 
&s->rx_control_dev);
-Error *local_err = NULL;
 int i;
 
 object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
@@ -548,11 +547,8 @@ static void xilinx_axidma_realize(DeviceState *dev, Error 
**errp)
  (Object **)&cs->dma,
  object_property_allow_set_link,
  OBJ_PROP_LINK_STRONG);
-object_property_set_

[PATCH v2 16/25] hw/arm: Drop useless object_property_set_link() error handling

[Markus Armbruster]

object_property_set_link() fails when the property doesn't exist, is
not settable, or its .check() method fails.  These are all programming
errors here, so passing it &error_abort is appropriate.

Cc: Peter Maydell 
Cc: "Cédric Le Goater" 
Cc: Andrew Jeffery 
Cc: Joel Stanley 
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster 
Reviewed-by: Cédric Le Goater 
---
 hw/arm/armsse.c | 53 ++---
 hw/arm/armv7m.c |  7 ++
 hw/arm/aspeed_ast2600.c | 20 
 hw/arm/aspeed_soc.c | 14 ---
 hw/arm/nrf51_soc.c  |  6 +
 5 files changed, 24 insertions(+), 76 deletions(-)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index e8f8f60abc..c73cc6badf 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -564,16 +564,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 &s->container, -1);
 }
 object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
- "memory", &err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
-object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
+ "memory", &error_abort);
+object_property_set_link(cpuobj, OBJECT(s), "idau", &error_abort);
 sysbus_realize(SYS_BUS_DEVICE(cpuobj), &err);
 if (err) {
 error_propagate(errp, err);
@@ -700,11 +692,7 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 return;
 }
 object_property_set_link(OBJECT(&s->mpc[i]), OBJECT(&s->sram[i]),
- "downstream", &err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
+ "downstream", &error_abort);
 sysbus_realize(SYS_BUS_DEVICE(&s->mpc[i]), &err);
 if (err) {
 error_propagate(errp, err);
@@ -755,11 +743,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer0), 0,
armsse_get_common_irq_in(s, 3));
 mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->timer0), 0);
-object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[0]", 
&err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
+object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[0]",
+ &error_abort);
 
 qdev_prop_set_uint32(DEVICE(&s->timer1), "pclk-frq", s->mainclk_frq);
 sysbus_realize(SYS_BUS_DEVICE(&s->timer1), &err);
@@ -770,12 +755,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer1), 0,
armsse_get_common_irq_in(s, 4));
 mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->timer1), 0);
-object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[1]", 
&err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
-
+object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[1]",
+ &error_abort);
 
 qdev_prop_set_uint32(DEVICE(&s->dualtimer), "pclk-frq", s->mainclk_frq);
 sysbus_realize(SYS_BUS_DEVICE(&s->dualtimer), &err);
@@ -786,11 +767,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 sysbus_connect_irq(SYS_BUS_DEVICE(&s->dualtimer), 0,
armsse_get_common_irq_in(s, 5));
 mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->dualtimer), 0);
-object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[2]", 
&err);
-if (err) {
-error_propagate(errp, err);
-return;
-}
+object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[2]",
+ &error_abort);
 
 if (info->has_mhus) {
 /*
@@ -815,12 +793,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 port = g_strdup_printf("port[%d]", i + 3);
 mr = sysbus_mmio_get_region(mhu_sbd, 0);
 object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr),
- port, &err);
+ port, &error_abort);
 g_free(port);
-if (err) {
-error_propagate(errp, err);
-return;
-}
 
 /*
  * Each MHU has an irq line for each CPU:
@@ -967,11 +941,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 sysbus_connect_irq(SYS_BUS_DEVICE(&s->s32ktimer), 0,
armsse_get_common_irq_in(s, 2));
 mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->s32ktimer), 0);
-object_property_set_link(OBJECT(&s->apb_ppc1), OBJECT(mr), "port[0]", 
&err);
-if (err) {
-error_propagate(errp, err)

Re: [PATCH v4 1/5] qdev: Introduce DEFINE_PROP_RESERVED_REGION

[Auger Eric]

Hi Markus,

On 6/24/20 10:10 AM, Markus Armbruster wrote:
> Auger Eric  writes:
> 
>> Hi Markus,
>>
>> On 6/23/20 5:13 PM, Markus Armbruster wrote:
>>> Eric Auger  writes:
>>>
 Introduce a new property defining a reserved region:
 ::.

 This will be used to encode reserved IOVA regions.

 For instance, in virtio-iommu use case, reserved IOVA regions
 will be passed by the machine code to the virtio-iommu-pci
 device (an array of those). The type of the reserved region
 will match the virtio_iommu_probe_resv_mem subtype value:
 - VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0)
 - VIRTIO_IOMMU_RESV_MEM_T_MSI (1)

 on PC/Q35 machine, this will be used to inform the
 virtio-iommu-pci device it should bypass the MSI region.
 The reserved region will be: 0xfee0:0xfeef:1.

 On ARM, we can declare the ITS MSI doorbell as an MSI
 region to prevent MSIs from being mapped on guest side.

 Signed-off-by: Eric Auger 
 Reviewed-by: Markus Armbruster 

 ---

 v3 -> v4:
 - use ':' instead of commas as separators.
 - rearrange error messages
 - check snprintf returned value
 - dared to keep Markus' R-b despite those changes
 ---
  include/exec/memory.h|  6 +++
  include/hw/qdev-properties.h |  3 ++
  include/qemu/typedefs.h  |  1 +
  hw/core/qdev-properties.c| 89 
  4 files changed, 99 insertions(+)

 diff --git a/include/exec/memory.h b/include/exec/memory.h
 index 7207025bd4..d7a53b96cc 100644
 --- a/include/exec/memory.h
 +++ b/include/exec/memory.h
 @@ -51,6 +51,12 @@ extern bool global_dirty_log;
  
  typedef struct MemoryRegionOps MemoryRegionOps;
  
 +struct ReservedRegion {
 +hwaddr low;
 +hwaddr high;
 +unsigned int type;
>>>
>>> Suggest to s/unsigned int/unsigned/.
>>>
 +};
 +
  typedef struct IOMMUTLBEntry IOMMUTLBEntry;
  
  /* See address_space_translate: bit 0 is read, bit 1 is write.  */
 diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
 index 5252bb6b1a..95d0e7201d 100644
 --- a/include/hw/qdev-properties.h
 +++ b/include/hw/qdev-properties.h
 @@ -19,6 +19,7 @@ extern const PropertyInfo qdev_prop_string;
  extern const PropertyInfo qdev_prop_chr;
  extern const PropertyInfo qdev_prop_tpm;
  extern const PropertyInfo qdev_prop_macaddr;
 +extern const PropertyInfo qdev_prop_reserved_region;
  extern const PropertyInfo qdev_prop_on_off_auto;
  extern const PropertyInfo qdev_prop_multifd_compression;
  extern const PropertyInfo qdev_prop_losttickpolicy;
 @@ -184,6 +185,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
  DEFINE_PROP(_n, _s, _f, qdev_prop_drive_iothread, BlockBackend *)
  #define DEFINE_PROP_MACADDR(_n, _s, _f) \
  DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
 +#define DEFINE_PROP_RESERVED_REGION(_n, _s, _f) \
 +DEFINE_PROP(_n, _s, _f, qdev_prop_reserved_region, ReservedRegion)
  #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \
  DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_on_off_auto, OnOffAuto)
  #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \
 diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
 index ce4a78b687..15f5047bf1 100644
 --- a/include/qemu/typedefs.h
 +++ b/include/qemu/typedefs.h
 @@ -58,6 +58,7 @@ typedef struct ISABus ISABus;
  typedef struct ISADevice ISADevice;
  typedef struct IsaDma IsaDma;
  typedef struct MACAddr MACAddr;
 +typedef struct ReservedRegion ReservedRegion;
  typedef struct MachineClass MachineClass;
  typedef struct MachineState MachineState;
  typedef struct MemoryListener MemoryListener;
 diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
 index ead35d7ffd..193d0d95f9 100644
 --- a/hw/core/qdev-properties.c
 +++ b/hw/core/qdev-properties.c
 @@ -15,6 +15,7 @@
  #include "chardev/char.h"
  #include "qemu/uuid.h"
  #include "qemu/units.h"
 +#include "qemu/cutils.h"
  
  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
Error **errp)
 @@ -578,6 +579,94 @@ const PropertyInfo qdev_prop_macaddr = {
  .set   = set_mac,
  };
  
 +/* --- Reserved Region --- */
 +
 +/*
 + * accepted syntax version:
>>>
>>> "version" feels redundant.  Suggest to capitalize "Accepted".
>>>
 + *   ::
 + *   where low/high addresses are uint64_t in hexadecimal
 + *   and type is an unsigned integer in decimal
 + */
 +static void get_reserved_region(Object *obj, Visitor *v, const char *name,
 +void *opaque, Error **errp)
 +{
 +DeviceState *dev = DEVICE(obj);
 +Property *prop 

Re: [PATCH] qom: Allow object_property_add_child() to fail

[Auger Eric]

Hi Markus,

On 6/24/20 10:22 AM, Markus Armbruster wrote:
> Eric Auger  writes:
> 
>> object_property_add() does not allow object_property_try_add()
>> to gracefully fail as &error_abort is passed as an error handle.
>>
>> However such failure can easily be triggered from the QMP shell when,
>> for instance, one attempts to create an object with an id that already
>> exists:
>>
>> For instance, call twice:
>> object-add qom-type=memory-backend-ram id=mem1 props.size=1073741824
>> and QEMU aborts.
>>
>> This behavior is undesired as a user/management application mistake
>> in reusing a property ID shouldn't result in loss of the VM and live
>> data within.
>>
>> This patch introduces two new functions, object_property_add_err() and
>> object_property_add_child_err() whose prototype features an error handle.
>> object_property_add_child_err() now gets called from user_creatable_add_type.
>> This solution was chosen instead of changing the prototype of existing
>> functions because the number of existing callers is huge.
>>
>> The error now is returned gracefully to the QMP client.
>>
>> (QEMU) object-add qom-type=memory-backend-ram id=mem2  props.size=4294967296
>> {"return": {}}
>> (QEMU) object-add qom-type=memory-backend-ram id=mem2  props.size=4294967296
>> {"error": {"class": "GenericError", "desc": "attempt to add duplicate 
>> property
>> 'mem2' to object (type 'container')"}}
>>
>> Signed-off-by: Eric Auger 
> 
> Recent regression, my fault.  Please point that out, and add
> 
>   Fixes: d2623129a7dec1d3041ad1221dda1ca49c667532
Thanks you for the reference. I will add the tag.

Eric
> 

Re: [PATCH QEMU v25 11/17] vfio: Get migration capability flags for container

[Cornelia Huck]

On Sun, 21 Jun 2020 01:51:20 +0530
Kirti Wankhede  wrote:

> Added helper functions to get IOMMU info capability chain.
> Added function to get migration capability information from that
> capability chain for IOMMU container.
> 
> Similar change was proposed earlier:
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg03759.html
> 
> Signed-off-by: Kirti Wankhede 
> Cc: Shameer Kolothum 
> Cc: Eric Auger 
> ---
>  hw/vfio/common.c  | 91 
> +++
>  include/hw/vfio/vfio-common.h |  3 ++
>  2 files changed, 86 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 90e9a854d82c..e0d3d4585a65 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1229,6 +1229,75 @@ static int vfio_init_container(VFIOContainer 
> *container, int group_fd,
>  return 0;
>  }
>  
> +static int vfio_get_iommu_info(VFIOContainer *container,
> +   struct vfio_iommu_type1_info **info)
> +{
> +
> +size_t argsz = sizeof(struct vfio_iommu_type1_info);
> +
> +*info = g_new0(struct vfio_iommu_type1_info, 1);
> +again:
> +(*info)->argsz = argsz;
> +
> +if (ioctl(container->fd, VFIO_IOMMU_GET_INFO, *info)) {
> +g_free(*info);
> +*info = NULL;
> +return -errno;
> +}
> +
> +if (((*info)->argsz > argsz)) {
> +argsz = (*info)->argsz;
> +*info = g_realloc(*info, argsz);

Do we need to guard against getting a bogus argsz value causing a huge
allocation that might fail and crash the program?

> +goto again;
> +}
> +
> +return 0;
> +}

(...)

> @@ -1314,15 +1384,20 @@ static int vfio_connect_container(VFIOGroup *group, 
> AddressSpace *as,
>   * existing Type1 IOMMUs generally support any IOVA we're
>   * going to actually try in practice.
>   */
> -info.argsz = sizeof(info);
> -ret = ioctl(fd, VFIO_IOMMU_GET_INFO, &info);
> -/* Ignore errors */
> -if (ret || !(info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
> +ret = vfio_get_iommu_info(container, &info);

Previously, we ignored errors from the IOMMU_GET_INFO ioctl, now we
error out. Was that change intended?

> +if (ret) {
> +goto free_container_exit;
> +}
> +
> +if (!(info->flags & VFIO_IOMMU_INFO_PGSIZES)) {
>  /* Assume 4k IOVA page size */
> -info.iova_pgsizes = 4096;
> +info->iova_pgsizes = 4096;
>  }
> -vfio_host_win_add(container, 0, (hwaddr)-1, info.iova_pgsizes);
> -container->pgsizes = info.iova_pgsizes;
> +vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes);
> +container->pgsizes = info->iova_pgsizes;
> +
> +vfio_get_iommu_info_migration(container, info);
> +g_free(info);
>  break;
>  }
>  case VFIO_SPAPR_TCE_v2_IOMMU:
(...)

Re: [PATCH] qom: Allow object_property_add_child() to fail

[Daniel P . Berrangé]

On Wed, Jun 24, 2020 at 10:22:14AM +0200, Markus Armbruster wrote:
> Eric Auger  writes:
> 
> > object_property_add() does not allow object_property_try_add()
> > to gracefully fail as &error_abort is passed as an error handle.
> >
> > However such failure can easily be triggered from the QMP shell when,
> > for instance, one attempts to create an object with an id that already
> > exists:
> >
> > For instance, call twice:
> > object-add qom-type=memory-backend-ram id=mem1 props.size=1073741824
> > and QEMU aborts.
> >
> > This behavior is undesired as a user/management application mistake
> > in reusing a property ID shouldn't result in loss of the VM and live
> > data within.
> >
> > This patch introduces two new functions, object_property_add_err() and
> > object_property_add_child_err() whose prototype features an error handle.
> > object_property_add_child_err() now gets called from 
> > user_creatable_add_type.
> > This solution was chosen instead of changing the prototype of existing
> > functions because the number of existing callers is huge.
> >
> > The error now is returned gracefully to the QMP client.
> >
> > (QEMU) object-add qom-type=memory-backend-ram id=mem2  props.size=4294967296
> > {"return": {}}
> > (QEMU) object-add qom-type=memory-backend-ram id=mem2  props.size=4294967296
> > {"error": {"class": "GenericError", "desc": "attempt to add duplicate 
> > property
> > 'mem2' to object (type 'container')"}}
> >
> > Signed-off-by: Eric Auger 
> 
> Recent regression, my fault.  Please point that out, and add
> 
>   Fixes: d2623129a7dec1d3041ad1221dda1ca49c667532

I noticed tests/qtest/qmp-cmd-test.c exercises object-add. Probably a
good idea to extend that to test duplicate ID scenario, as that would
have caught the accidental regression.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: -enablefips

[Daniel P . Berrangé]

On Wed, Jun 24, 2020 at 08:49:54AM +0200, Gerd Hoffmann wrote:
> On Tue, Jun 23, 2020 at 11:51:09PM -0400, John Snow wrote:
> > I never knew what this option did, but the answer is ... strange!
> > 
> > It's only defined for linux, in os-posix.c. When called, it calls
> > fips_set_state(true), located in osdep.c.
> > 
> > This will read /proc/sys/crypto/fips_enabled and set the static global
> > 'fips_enabled' to true if this setting is on.
> 
> IIRC the idea is to have a global switch to enable fips compilance for
> the whole distro.  RH specific.  rhel-7 kernel has it.  rhel-8 kernel
> too, so it probably isn't obsolete.  Not present in mainline kernels.
> 
> I'm wondering what the point of the -enablefips switch is.  Shouldn't
> qemu check /proc/sys/crypto/fips_enabled unconditionally instead?

Yes, but IIRC, there was a bit of a philisophical debate about the
value of FIPS and whether QEMU was going to accept it at all upstream.
I think the -enablefips switch was a compromise to get something
upstream.

> > (Tangent: what does *this* setting actually control? Should QEMU
> > meaningfully change its behavior when it's set?)
> 
> fips is a security policy ...
> 
> > This static global is exposed via the getter fips_get_state(). This
> > function is called only by vnc.c, and appears to disable the use of the
> > password option for -vnc.
> 
> ... yes, "no passwords" is one of the rules.  There are probably more.

It isn't so much "no passwords", rather in the VNC case the rule
we fal on is "no single DES" encryption algorithm !

Back when we added FIPS in QEMU, VNC was using the built-in DES
impl, and hence we needed to block this explicitly ourselves.

These days a sensible QEMU build will link to a crypto library
and get DES from there.  This crypto library will in turn block
our use of single DES when in FIPS mode, so QEMU won't have to
worry about it.

> > (If we really do want to keep it, it should probably go under -global
> > somewhere instead to help reduce flag clutter, but we'd need to have a
> > chat about what fips compliance means for literally every other spot in
> > QEMU that is capable of using or receiving a cleartext password.)
> 
> Yep.  IIRC for spice this is handled in libspice-server.  We need to
> look at blockdev encryption I guess.  Any other places where qemu uses
> passwords directly?  I think we don't have to worry about indirect usage
> (sasl).

I don't think it is about passwords. Encryption algorithms were the
really key thing AFAIK, and the intent is that by using a common crypto
library you get the FIPS support for free.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH] trivial: Remove extra character in configure help

[Daniel P . Berrangé]

On Wed, Jun 24, 2020 at 10:33:37AM +0200, Christophe de Dinechin wrote:
> Signed-off-by: Christophe de Dinechin 
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index ba88fd1824..c7a6a5adfe 100755
> --- a/configure
> +++ b/configure
> @@ -1787,7 +1787,7 @@ Advanced options (experts only):
>--block-drv-ro-whitelist=L
> set block driver read-only whitelist
> (affects only QEMU, not qemu-img)
> -  --enable-trace-backends=B Set trace backend
> +  --enable-trace-backends= Set trace backend

This is just following the style of the option above. "B" is a
placeholder for the desired backend(s).

> Available backends: $trace_backend_list
>--with-trace-file=NAME   Full PATH,NAME of file to store traces
> Default:trace-


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2 03/25] Clean up some calls to ignore Error objects the right way

[Greg Kurz]

On Wed, 24 Jun 2020 10:37:15 +0200
Markus Armbruster  wrote:

> Receiving the error in a local variable only to free it is less clear
> (and also less efficient) than passing NULL.  Clean up.
> 
> Cc: Daniel P. Berrange 
> Cc: Jerome Forissier 
> CC: Greg Kurz 
> Signed-off-by: Markus Armbruster 
> ---

It doesn't seem to be different from v1, so:

Reviewed-by: Greg Kurz 

and

Acked-by: Greg Kurz  # for 9pfs

>  chardev/char-socket.c | 6 ++
>  hw/9pfs/9p.c  | 6 ++
>  hw/arm/virt.c | 4 +---
>  hw/ppc/spapr_drc.c| 4 +---
>  ui/vnc.c  | 3 +--
>  5 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index afebeec5c3..b0cae97960 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -815,22 +815,20 @@ static void tcp_chr_tls_init(Chardev *chr)
>  {
>  SocketChardev *s = SOCKET_CHARDEV(chr);
>  QIOChannelTLS *tioc;
> -Error *err = NULL;
>  gchar *name;
>  
>  if (s->is_listen) {
>  tioc = qio_channel_tls_new_server(
>  s->ioc, s->tls_creds,
>  s->tls_authz,
> -&err);
> +NULL);
>  } else {
>  tioc = qio_channel_tls_new_client(
>  s->ioc, s->tls_creds,
>  s->addr->u.inet.host,
> -&err);
> +NULL);
>  }
>  if (tioc == NULL) {
> -error_free(err);
>  tcp_chr_disconnect(chr);
>  return;
>  }
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 45a788f6e6..9755fba9a9 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1399,7 +1399,6 @@ static void coroutine_fn v9fs_attach(void *opaque)
>  size_t offset = 7;
>  V9fsQID qid;
>  ssize_t err;
> -Error *local_err = NULL;
>  
>  v9fs_string_init(&uname);
>  v9fs_string_init(&aname);
> @@ -1437,9 +1436,8 @@ static void coroutine_fn v9fs_attach(void *opaque)
>  error_setg(&s->migration_blocker,
> "Migration is disabled when VirtFS export path '%s' is 
> mounted in the guest using mount_tag '%s'",
> s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
> -err = migrate_add_blocker(s->migration_blocker, &local_err);
> -if (local_err) {
> -error_free(local_err);
> +err = migrate_add_blocker(s->migration_blocker, NULL);
> +if (err < 0) {
>  error_free(s->migration_blocker);
>  s->migration_blocker = NULL;
>  clunk_fid(s, fid);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index caceb1e4a0..29b9d5b2e6 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -217,11 +217,9 @@ static bool cpu_type_valid(const char *cpu)
>  
>  static void create_kaslr_seed(VirtMachineState *vms, const char *node)
>  {
> -Error *err = NULL;
>  uint64_t seed;
>  
> -if (qemu_guest_getrandom(&seed, sizeof(seed), &err)) {
> -error_free(err);
> +if (qemu_guest_getrandom(&seed, sizeof(seed), NULL)) {
>  return;
>  }
>  qemu_fdt_setprop_u64(vms->fdt, node, "kaslr-seed", seed);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 2689104295..951bcdf2c0 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -1163,16 +1163,14 @@ static void rtas_ibm_configure_connector(PowerPCCPU 
> *cpu,
>  drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
>  if (!drc->fdt) {
> -Error *local_err = NULL;
>  void *fdt;
>  int fdt_size;
>  
>  fdt = create_device_tree(&fdt_size);
>  
>  if (drck->dt_populate(drc, spapr, fdt, &drc->fdt_start_offset,
> -  &local_err)) {
> +  NULL)) {
>  g_free(fdt);
> -error_free(local_err);
>  rc = SPAPR_DR_CC_RESPONSE_ERROR;
>  goto out;
>  }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 12a12714e1..0702a76cce 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -458,9 +458,8 @@ static VncServerInfo2List 
> *qmp_query_server_entry(QIOChannelSocket *ioc,
>  Error *err = NULL;
>  SocketAddress *addr;
>  
> -addr = qio_channel_socket_get_local_address(ioc, &err);
> +addr = qio_channel_socket_get_local_address(ioc, NULL);
>  if (!addr) {
> -error_free(err);
>  return prev;
>  }
>  

Re: [PATCH v2 07/25] spapr: Plug minor memory leak in spapr_machine_init()

[Greg Kurz]

On Wed, 24 Jun 2020 10:37:19 +0200
Markus Armbruster  wrote:

> spapr_machine_init() leaks an Error object when
> kvmppc_check_papr_resize_hpt() fails and spapr->resize_hpt is
> SPAPR_RESIZE_HPT_DISABLED, i.e. when the host doesn't support hash
> page table resizing, and the user didn't ask for it.  As harmless as
> memory leaks can possibly be.  Plug it.
> 
> Fixes: 30f4b05bd090564181554d0890605eb2c143e4ea
> Cc: David Gibson 
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster 
> ---

Ditto.

Reviewed-by: Greg Kurz 

>  hw/ppc/spapr.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bd9345cdac..9bd2183ead 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2731,6 +2731,7 @@ static void spapr_machine_init(MachineState *machine)
>  error_report_err(resize_hpt_err);
>  exit(1);
>  }
> +error_free(resize_hpt_err);
>  
>  spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
>  

Re: -enablefips

[Daniel P . Berrangé]

On Tue, Jun 23, 2020 at 11:51:09PM -0400, John Snow wrote:
> I never knew what this option did, but the answer is ... strange!
> 
> It's only defined for linux, in os-posix.c. When called, it calls
> fips_set_state(true), located in osdep.c.
> 
> This will read /proc/sys/crypto/fips_enabled and set the static global
> 'fips_enabled' to true if this setting is on.
> 
> (Tangent: what does *this* setting actually control? Should QEMU
> meaningfully change its behavior when it's set?)
> 
> This static global is exposed via the getter fips_get_state(). This
> function is called only by vnc.c, and appears to disable the use of the
> password option for -vnc.
> 
> This seems very high-level and abstract for something that ultimately
> only disables VNC password authentication. Is this misleadingly abstract?
> 
> The docs state:
> "enable FIPS 140-2 compliance"
> 
> Like hell it does.

It prevents the use of non-FIPS crypto in QEMU, so it isn't that
inaccurate.

> Can we deprecate this? It was added in 2012 and never seemed to pursue
> the mission laid out in the help file. If we do still want it, the
> documentation should be changed dramatically to reflect what it actually
> does.
> 
> This is so at risk of bit-rot, and a misleading crypto flag is certainly
> worse than no crypto flag. I think it should just go.

I think it can go.

FIPS support in QEMU is only needed if using our own crypto code which
lacks FIPS compliance, which essentially means our Single DES impl.

Anyone who cares about FIPS should be building QEMU with crypto provided
by libgcrypt. This will take care of FIPS compliance automatically,
so there's nothing QEMU needs do itself.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PULL 00/18] Block patches

[Thomas Huth]

On 24/06/2020 09.27, Max Reitz wrote:

On 23.06.20 14:55, Peter Maydell wrote:

On Mon, 22 Jun 2020 at 16:11, Max Reitz  wrote:


The following changes since commit bae31bfa48b9caecee25da3d5333901a126a06b4:

   Merge remote-tracking branch 
'remotes/kraxel/tags/audio-20200619-pull-request' into staging (2020-06-19 
22:56:59 +0100)

are available in the Git repository at:

   https://github.com/XanClic/qemu.git tags/pull-block-2020-06-22

for you to fetch changes up to 74c55e4142a7bb835c38d3770c74210cbb1e4fab:

   iotests: don't test qcow2.py inside 291 (2020-06-22 16:05:23 +0200)


Block patches:
- Support modifying a LUKS-encrypted image's keyslots
- iotest fixes




Hi; I see various iotest failures, different things on
PPC64 Linux, OpenBSD and FreeBSD, and on an AArch32 build
that happens to not have optional crypto libs installed.


OK.  Sorry.  None of this looks easily fixable, so I suppose I’ll have
to drop everything but the last two patches for now.


At least the problem with "seq" in test 293 should be easy to fix, since 
we're requiring bash for the iotests. See e.g. commit 30edd9fa50e86fbf 
as an example.


 Thomas

Re: [PATCH] trivial: Remove extra character in configure help

[Philippe Mathieu-Daudé]

On 6/24/20 10:59 AM, Daniel P. Berrangé wrote:
> On Wed, Jun 24, 2020 at 10:33:37AM +0200, Christophe de Dinechin wrote:
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  configure | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index ba88fd1824..c7a6a5adfe 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1787,7 +1787,7 @@ Advanced options (experts only):
>>--block-drv-ro-whitelist=L
>> set block driver read-only whitelist
>> (affects only QEMU, not qemu-img)
>> -  --enable-trace-backends=B Set trace backend
>> +  --enable-trace-backends= Set trace backend
> 
> This is just following the style of the option above. "B" is a
> placeholder for the desired backend(s).

I agree this is confusing, since then the filename is NAME, not N.

Maybe clearer to replace B -> BACKEND, L -> LIST, ...?

> 
>> Available backends: $trace_backend_list
>>--with-trace-file=NAME   Full PATH,NAME of file to store traces
>> Default:trace-
> 
> 
> Regards,
> Daniel
> 

[Bug 1884507] Re: 'none' machine should use 'none' display option

[Philippe Mathieu-Daudé]

> I think you made a wrong assumption here. "-display" is
> about the GUI backend that should be used. "-M" is about
> the emulated hardware. The emulated hardware options
> should never influence the host backend options.

Aright. What confuses me is having serial0/parallel0 chardevs
initialized when using the none-machine. I realized when
looking at your suggestion in comment #1 that the chardevs
(among other hardware related things) are initialized in
qemu_init().

I started testing using:

  bool is_none_machine = !strcmp(machine_class->name,
 MACHINE_TYPE_NAME("none"));

and disabling blocks of code with:

  if (!is_none_machine) {
  ...
  }

then planned to update this ticket description but you beat
me. I'll open a different issue.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1884507

Title:
  'none' machine should use 'none' display option

Status in QEMU:
  Invalid

Bug description:
  As the 'none' machine doesn't have any peripheral (except CPU cores)
  it is pointless to start a display. 

  '-M none' should imply '-display none'.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1884507/+subscriptions

Re: [PATCH] ibex_uart: fix XOR-as-pow

[Philippe Mathieu-Daudé]

On 6/24/20 8:33 AM, Paolo Bonzini wrote:
> On 23/06/20 22:07, Eric Blake wrote:
>>>
>>>  uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
>>>   baud *= 1000;
>>> -    baud /= 2 ^ 20;
>>> +    baud >>= 20;
>>
>> Dividing by 1M instead of 22 seems much more logical, indeed :)
> 
> Based on the spec, the "* 1000" is the clock, in other words this is a
> fixed point value relative to the clock:
> 
> f_baud = NCO * f_clock / 2^20
> 
> The example in the spec (https://docs.opentitan.org/hw/ip/uart/doc/) has
> f_clock = 50 MHz, while here it's only 1 kHz.  And the register is only
> 16 bit, so the above would only allow a baud rate up to 62 (65535 * 1000
> / 2^20).
> 
> Should the clock be a property of the device instead?

Ideally the device should use qdev_get_clock_in(),
(see docs/devel/clocks.rst):

  static void ibex_uart_init(Object *obj)
  {
 ...
 s->f_clk = qdev_get_clock_in(DEVICE(obj), "f_clock");
 ...

Then in ibex_uart_write():

 uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
 baud *= clock_get_hz(dev->f_clk));
 baud >>= 20;

Devices not using the QDEV_CLOCK API use QDEV properties.

> 
> Thanks,
> 
> Paolo
> 
>> It's odd that we are scaling up by 1000, down by 1024*1024, then
>>
>>>     s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10; 
> 
> 

Re: [PATCH v1 00/10] vDPA support in qemu

[Cindy Lu]

On Tue, Jun 23, 2020 at 5:43 PM Jason Wang  wrote:
>
>
> On 2020/6/23 下午5:16, Cindy Lu wrote:
> > On Tue, Jun 23, 2020 at 3:07 PM Markus Armbruster  wrote:
> >> Cindy Lu  writes:
> >>
> >>> vDPA device is a device that uses a datapath which complies with the
> >>> virtio specifications with vendor specific control path. vDPA devices
> >>> can be both physically located on the hardware or emulated by software.
> >>> This RFC introduce the vDPA support in qemu
> >>> TODO
> >>> 1) vIOMMU support
> >>> 2) live migration support
> >> This gives me the foggiest of ideas on what vDPA is.  Could we use
> >> docs/interop/vhost-vdpa.rst?
> >>
> > Sure will add this
> >
> >
>
> Not sure it's the best place since vhost-vdpa is kernel specific.
>
> Maybe kernel docs (TBD) is a better place and we can refer it this file
> in the future.
>
> But it doesn't harm if you said something more here and refer the kernel
> commit here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4c8cf31885f69e86be0b5b9e6677a26797365e1d
>
> Thanks
>
>
Hi Markus,
I think I agree with Jason's opinion, kernel docs is a better place.
Maybe we can keep what it is now, and do this job in the future.

Thanks
Cindy

Re: [RFC PATCH 3/3] fuzz: Add callbacks for dma-access functions

[Philippe Mathieu-Daudé]

On 6/11/20 7:56 AM, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov 
> ---
>  exec.c| 17 -
>  include/exec/memory.h |  8 
>  include/exec/memory_ldst_cached.inc.h |  9 +
>  include/sysemu/dma.h  |  5 -
>  memory_ldst.inc.c | 12 
>  5 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index be4be2df3a..2ed724ab54 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3247,7 +3247,10 @@ MemTxResult address_space_read_full(AddressSpace *as, 
> hwaddr addr,
>  {
>  MemTxResult result = MEMTX_OK;
>  FlatView *fv;
> -
> +#ifdef CONFIG_FUZZ
> +if(as->root == get_system_memory())

Since it is local to exec.c, you can directly use system_memory.

But why restrict this to the system memory anyway?

> +dma_read_cb(addr, len);
> +#endif

[PATCH v1] chardev/char-socket: fix double free of err after socket is disconnected

[Derek Su]

The err is freed in check_report_connect_error() conditionally,
calling error_free() directly may lead to a double-free bug.

Signed-off-by: Derek Su 
---
 chardev/char-socket.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index afebeec5c3..a009bed5ee 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1086,7 +1086,11 @@ static void qemu_chr_socket_connected(QIOTask *task, 
void *opaque)
 if (qio_task_propagate_error(task, &err)) {
 tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
 check_report_connect_error(chr, err);
-error_free(err);
+
+if (!s->connect_err_reported) {
+error_free(err);
+}
+
 goto cleanup;
 }
 
-- 
2.17.1

[PULL 01/12] minikconf: explicitly set encoding to UTF-8

[Stefan Hajnoczi]

QEMU currently only has ASCII Kconfig files but Linux actually uses
UTF-8. Explicitly specify the encoding and that we're doing text file
I/O.

It's unclear whether or not QEMU will ever need Unicode in its Kconfig
files. If we start using the help text then it will become an issue
sooner or later. Make this change now for consistency with Linux
Kconfig.

Reported-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Richard Henderson 
Message-id: 20200521153616.307100-1-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 scripts/minikconf.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/minikconf.py b/scripts/minikconf.py
index 90b99517c1..bcd91015d3 100755
--- a/scripts/minikconf.py
+++ b/scripts/minikconf.py
@@ -402,7 +402,7 @@ class KconfigParser:
 if incl_abs_fname in self.data.previously_included:
 return
 try:
-fp = open(incl_abs_fname, 'r')
+fp = open(incl_abs_fname, 'rt', encoding='utf-8')
 except IOError as e:
 raise KconfigParserError(self,
 '%s: %s' % (e.strerror, include))
@@ -696,7 +696,7 @@ if __name__ == '__main__':
 parser.do_assignment(name, value == 'y')
 external_vars.add(name[7:])
 else:
-fp = open(arg, 'r')
+fp = open(arg, 'rt', encoding='utf-8')
 parser.parse_file(fp)
 fp.close()
 
@@ -705,7 +705,7 @@ if __name__ == '__main__':
 if key not in external_vars and config[key]:
 print ('CONFIG_%s=y' % key)
 
-deps = open(argv[2], 'w')
+deps = open(argv[2], 'wt', encoding='utf-8')
 for fname in data.previously_included:
 print ('%s: %s' % (argv[1], fname), file=deps)
 deps.close()
-- 
2.26.2

[PULL 00/12] Block patches

[Stefan Hajnoczi]

The following changes since commit 171199f56f5f9bdf1e5d670d09ef1351d8f01bae:

  Merge remote-tracking branch 
'remotes/alistair/tags/pull-riscv-to-apply-20200619-3' into staging (2020-06-22 
14:45:25 +0100)

are available in the Git repository at:

  https://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 7838c67f22a81fcf669785cd6c0876438422071a:

  block/nvme: support nested aio_poll() (2020-06-23 15:46:08 +0100)


Pull request



Daniele Buono (4):
  coroutine: support SafeStack in ucontext backend
  coroutine: add check for SafeStack in sigaltstack
  configure: add flags to support SafeStack
  check-block: enable iotests with SafeStack

Stefan Hajnoczi (8):
  minikconf: explicitly set encoding to UTF-8
  block/nvme: poll queues without q->lock
  block/nvme: drop tautologous assertion
  block/nvme: don't access CQE after moving cq.head
  block/nvme: switch to a NVMeRequest freelist
  block/nvme: clarify that free_req_queue is protected by q->lock
  block/nvme: keep BDRVNVMeState pointer in NVMeQueuePair
  block/nvme: support nested aio_poll()

 configure|  73 
 include/qemu/coroutine_int.h |   5 +
 block/nvme.c | 220 +--
 util/coroutine-sigaltstack.c |   4 +
 util/coroutine-ucontext.c|  28 +
 block/trace-events   |   2 +-
 scripts/minikconf.py |   6 +-
 tests/check-block.sh |  12 +-
 8 files changed, 284 insertions(+), 66 deletions(-)

-- 
2.26.2

[PULL 03/12] coroutine: add check for SafeStack in sigaltstack

[Stefan Hajnoczi]

From: Daniele Buono 

Current implementation of LLVM's SafeStack is not compatible with
code that uses an alternate stack created with sigaltstack().
Since coroutine-sigaltstack relies on sigaltstack(), it is not
compatible with SafeStack. The resulting binary is incorrect, with
different coroutines sharing the same unsafe stack and producing
undefined behavior at runtime.

In the future LLVM may provide a SafeStack implementation compatible with
sigaltstack(). In the meantime, if SafeStack is desired, the coroutine
implementation from coroutine-ucontext should be used.
As a safety check, add a control in coroutine-sigaltstack to throw a
preprocessor #error if SafeStack is enabled and we are trying to
use coroutine-sigaltstack to implement coroutines.

Signed-off-by: Daniele Buono 
Message-id: 20200529205122.714-3-dbu...@linux.vnet.ibm.com
Signed-off-by: Stefan Hajnoczi 
---
 util/coroutine-sigaltstack.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index f6fc49a0e5..aade82afb8 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -30,6 +30,10 @@
 #include "qemu-common.h"
 #include "qemu/coroutine_int.h"
 
+#ifdef CONFIG_SAFESTACK
+#error "SafeStack is not compatible with code run in alternate signal stacks"
+#endif
+
 typedef struct {
 Coroutine base;
 void *stack;
-- 
2.26.2

[PULL 06/12] block/nvme: poll queues without q->lock

[Stefan Hajnoczi]

A lot of CPU time is spent simply locking/unlocking q->lock during
polling. Check for completion outside the lock to make q->lock disappear
from the profile.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Sergio Lopez 
Message-id: 20200617132201.1832152-2-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/nvme.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c
index eb2f54dd9d..e4375ec245 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -512,6 +512,18 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
 
 for (i = 0; i < s->nr_queues; i++) {
 NVMeQueuePair *q = s->queues[i];
+const size_t cqe_offset = q->cq.head * NVME_CQ_ENTRY_BYTES;
+NvmeCqe *cqe = (NvmeCqe *)&q->cq.queue[cqe_offset];
+
+/*
+ * Do an early check for completions. q->lock isn't needed because
+ * nvme_process_completion() only runs in the event loop thread and
+ * cannot race with itself.
+ */
+if ((le16_to_cpu(cqe->status) & 0x1) == q->cq_phase) {
+continue;
+}
+
 qemu_mutex_lock(&q->lock);
 while (nvme_process_completion(s, q)) {
 /* Keep polling */
-- 
2.26.2

[PULL 07/12] block/nvme: drop tautologous assertion

[Stefan Hajnoczi]

nvme_process_completion() explicitly checks cid so the assertion that
follows is always true:

  if (cid == 0 || cid > NVME_QUEUE_SIZE) {
  ...
  continue;
  }
  assert(cid <= NVME_QUEUE_SIZE);

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Sergio Lopez 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20200617132201.1832152-3-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/nvme.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index e4375ec245..d567ece3f4 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -336,7 +336,6 @@ static bool nvme_process_completion(BDRVNVMeState *s, 
NVMeQueuePair *q)
 cid);
 continue;
 }
-assert(cid <= NVME_QUEUE_SIZE);
 trace_nvme_complete_command(s, q->index, cid);
 preq = &q->reqs[cid - 1];
 req = *preq;
-- 
2.26.2

[PULL 02/12] coroutine: support SafeStack in ucontext backend

[Stefan Hajnoczi]

From: Daniele Buono 

LLVM's SafeStack instrumentation does not yet support programs that make
use of the APIs in ucontext.h
With the current implementation of coroutine-ucontext, the resulting
binary is incorrect, with different coroutines sharing the same unsafe
stack and producing undefined behavior at runtime.
This fix allocates an additional unsafe stack area for each coroutine,
and sets the new unsafe stack pointer before calling swapcontext() in
qemu_coroutine_new.
This is the only place where the pointer needs to be manually updated,
since sigsetjmp/siglongjmp are already instrumented by LLVM to properly
support SafeStack.
The additional stack is then freed in qemu_coroutine_delete.

Signed-off-by: Daniele Buono 
Message-id: 20200529205122.714-2-dbu...@linux.vnet.ibm.com
Signed-off-by: Stefan Hajnoczi 
---
 include/qemu/coroutine_int.h |  5 +
 util/coroutine-ucontext.c| 28 
 2 files changed, 33 insertions(+)

diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index bd6b0468e1..1da148552f 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,6 +28,11 @@
 #include "qemu/queue.h"
 #include "qemu/coroutine.h"
 
+#ifdef CONFIG_SAFESTACK
+/* Pointer to the unsafe stack, defined by the compiler */
+extern __thread void *__safestack_unsafe_stack_ptr;
+#endif
+
 #define COROUTINE_STACK_SIZE (1 << 20)
 
 typedef enum {
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 613f4c118e..f0b66320e1 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -45,6 +45,11 @@ typedef struct {
 Coroutine base;
 void *stack;
 size_t stack_size;
+#ifdef CONFIG_SAFESTACK
+/* Need an unsafe stack for each coroutine */
+void *unsafe_stack;
+size_t unsafe_stack_size;
+#endif
 sigjmp_buf env;
 
 void *tsan_co_fiber;
@@ -179,6 +184,10 @@ Coroutine *qemu_coroutine_new(void)
 co = g_malloc0(sizeof(*co));
 co->stack_size = COROUTINE_STACK_SIZE;
 co->stack = qemu_alloc_stack(&co->stack_size);
+#ifdef CONFIG_SAFESTACK
+co->unsafe_stack_size = COROUTINE_STACK_SIZE;
+co->unsafe_stack = qemu_alloc_stack(&co->unsafe_stack_size);
+#endif
 co->base.entry_arg = &old_env; /* stash away our jmp_buf */
 
 uc.uc_link = &old_uc;
@@ -203,6 +212,22 @@ Coroutine *qemu_coroutine_new(void)
 COROUTINE_YIELD,
 &fake_stack_save,
 co->stack, co->stack_size, co->tsan_co_fiber);
+
+#ifdef CONFIG_SAFESTACK
+/*
+ * Before we swap the context, set the new unsafe stack
+ * The unsafe stack grows just like the normal stack, so start from
+ * the last usable location of the memory area.
+ * NOTE: we don't have to re-set the usp afterwards because we are
+ * coming back to this context through a siglongjmp.
+ * The compiler already wrapped the corresponding sigsetjmp call with
+ * code that saves the usp on the (safe) stack before the call, and
+ * restores it right after (which is where we return with siglongjmp).
+ */
+void *usp = co->unsafe_stack + co->unsafe_stack_size;
+__safestack_unsafe_stack_ptr = usp;
+#endif
+
 swapcontext(&old_uc, &uc);
 }
 
@@ -235,6 +260,9 @@ void qemu_coroutine_delete(Coroutine *co_)
 #endif
 
 qemu_free_stack(co->stack, co->stack_size);
+#ifdef CONFIG_SAFESTACK
+qemu_free_stack(co->unsafe_stack, co->unsafe_stack_size);
+#endif
 g_free(co);
 }
 
-- 
2.26.2

[PULL 04/12] configure: add flags to support SafeStack

[Stefan Hajnoczi]

From: Daniele Buono 

This patch adds a flag to enable/disable the SafeStack instrumentation
provided by LLVM.

On enable, make sure that the compiler supports the flags, and that we
are using the proper coroutine implementation (coroutine-ucontext).
On disable, explicitly disable the option if it was enabled by default.

While SafeStack is supported only on Linux, NetBSD, FreeBSD and macOS,
we are not checking for the O.S. since this is already done by LLVM.

Signed-off-by: Daniele Buono 
Message-id: 20200529205122.714-4-dbu...@linux.vnet.ibm.com
Signed-off-by: Stefan Hajnoczi 
---
 configure | 73 +++
 1 file changed, 73 insertions(+)

diff --git a/configure b/configure
index ba88fd1824..ae8737d5a2 100755
--- a/configure
+++ b/configure
@@ -307,6 +307,7 @@ audio_win_int=""
 libs_qga=""
 debug_info="yes"
 stack_protector=""
+safe_stack=""
 use_containers="yes"
 gdb_bin=$(command -v "gdb-multiarch" || command -v "gdb")
 
@@ -1287,6 +1288,10 @@ for opt do
   ;;
   --disable-stack-protector) stack_protector="no"
   ;;
+  --enable-safe-stack) safe_stack="yes"
+  ;;
+  --disable-safe-stack) safe_stack="no"
+  ;;
   --disable-curses) curses="no"
   ;;
   --enable-curses) curses="yes"
@@ -1829,6 +1834,8 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   debug-tcg   TCG debugging (default is disabled)
   debug-info  debugging information
   sparse  sparse checker
+  safe-stack  SafeStack Stack Smash Protection. Depends on
+  clang/llvm >= 3.7 and requires coroutine backend ucontext.
 
   gnutls  GNUTLS cryptography support
   nettle  nettle cryptography support
@@ -5573,6 +5580,67 @@ if test "$debug_stack_usage" = "yes"; then
   fi
 fi
 
+##
+# SafeStack
+
+
+if test "$safe_stack" = "yes"; then
+cat > $TMPC << EOF
+int main(int argc, char *argv[])
+{
+#if ! __has_feature(safe_stack)
+#error SafeStack Disabled
+#endif
+return 0;
+}
+EOF
+  flag="-fsanitize=safe-stack"
+  # Check that safe-stack is supported and enabled.
+  if compile_prog "-Werror $flag" "$flag"; then
+# Flag needed both at compilation and at linking
+QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+QEMU_LDFLAGS="$QEMU_LDFLAGS $flag"
+  else
+error_exit "SafeStack not supported by your compiler"
+  fi
+  if test "$coroutine" != "ucontext"; then
+error_exit "SafeStack is only supported by the coroutine backend ucontext"
+  fi
+else
+cat > $TMPC << EOF
+int main(int argc, char *argv[])
+{
+#if defined(__has_feature)
+#if __has_feature(safe_stack)
+#error SafeStack Enabled
+#endif
+#endif
+return 0;
+}
+EOF
+if test "$safe_stack" = "no"; then
+  # Make sure that safe-stack is disabled
+  if ! compile_prog "-Werror" ""; then
+# SafeStack was already enabled, try to explicitly remove the feature
+flag="-fno-sanitize=safe-stack"
+if ! compile_prog "-Werror $flag" "$flag"; then
+  error_exit "Configure cannot disable SafeStack"
+fi
+QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+QEMU_LDFLAGS="$QEMU_LDFLAGS $flag"
+  fi
+else # "$safe_stack" = ""
+  # Set safe_stack to yes or no based on pre-existing flags
+  if compile_prog "-Werror" ""; then
+safe_stack="no"
+  else
+safe_stack="yes"
+if test "$coroutine" != "ucontext"; then
+  error_exit "SafeStack is only supported by the coroutine backend 
ucontext"
+fi
+  fi
+fi
+fi
 
 ##
 # check if we have open_by_handle_at
@@ -6765,6 +6833,7 @@ echo "sparse enabled$sparse"
 echo "strip binaries$strip_opt"
 echo "profiler  $profiler"
 echo "static build  $static"
+echo "safe stack$safe_stack"
 if test "$darwin" = "yes" ; then
 echo "Cocoa support $cocoa"
 fi
@@ -8370,6 +8439,10 @@ if test "$ccache_cpp2" = "yes"; then
   echo "export CCACHE_CPP2=y" >> $config_host_mak
 fi
 
+if test "$safe_stack" = "yes"; then
+  echo "CONFIG_SAFESTACK=y" >> $config_host_mak
+fi
+
 # If we're using a separate build tree, set it up now.
 # DIRS are directories which we simply mkdir in the build tree;
 # LINKS are things to symlink back into the source tree
-- 
2.26.2

[PULL 10/12] block/nvme: clarify that free_req_queue is protected by q->lock

[Stefan Hajnoczi]

Existing users access free_req_queue under q->lock. Document this.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Sergio Lopez 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20200617132201.1832152-6-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 8e60882af3..426c77e5ab 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -57,7 +57,6 @@ typedef struct {
 } NVMeRequest;
 
 typedef struct {
-CoQueue free_req_queue;
 QemuMutex   lock;
 
 /* Fields protected by BQL */
@@ -65,6 +64,7 @@ typedef struct {
 uint8_t *prp_list_pages;
 
 /* Fields protected by @lock */
+CoQueue free_req_queue;
 NVMeQueue   sq, cq;
 int cq_phase;
 int free_req_head;
-- 
2.26.2

[PULL 05/12] check-block: enable iotests with SafeStack

[Stefan Hajnoczi]

From: Daniele Buono 

SafeStack is a stack protection technique implemented in llvm. It is
enabled with a -fsanitize flag.
iotests are currently disabled when any -fsanitize option is used,
because such options tend to produce additional warnings and false
positives.

While common -fsanitize options are used to verify the code and not
added in production, SafeStack's main use is in production environments
to protect against stack smashing.

Since SafeStack does not print any warning or false positive, enable
iotests when SafeStack is the only -fsanitize option used.
This is likely going to be a production binary and we want to make sure
it works correctly.

Signed-off-by: Daniele Buono 
Message-id: 20200529205122.714-5-dbu...@linux.vnet.ibm.com
Signed-off-by: Stefan Hajnoczi 
---
 tests/check-block.sh | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index ad320c21ba..8e29c868e5 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; 
then
 exit 0
 fi
 
-if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
+# Disable tests with any sanitizer except for SafeStack
+CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+SANITIZE_FLAGS=""
+#Remove all occurrencies of -fsanitize=safe-stack
+for i in ${CFLAGS}; do
+if [ "${i}" != "-fsanitize=safe-stack" ]; then
+SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+fi
+done
+if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
+# Have a sanitize flag that is not allowed, stop
 echo "Sanitizers are enabled ==> Not running the qemu-iotests."
 exit 0
 fi
-- 
2.26.2

[PULL 08/12] block/nvme: don't access CQE after moving cq.head

[Stefan Hajnoczi]

Do not access a CQE after incrementing q->cq.head and releasing q->lock.
It is unlikely that this causes problems in practice but it's a latent
bug.

The reason why it should be safe at the moment is that completion
processing is not re-entrant and the CQ doorbell isn't written until the
end of nvme_process_completion().

Make this change now because QEMU expects completion processing to be
re-entrant and later patches will do that.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Sergio Lopez 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20200617132201.1832152-4-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/nvme.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index d567ece3f4..344893811a 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -321,11 +321,14 @@ static bool nvme_process_completion(BDRVNVMeState *s, 
NVMeQueuePair *q)
 q->busy = true;
 assert(q->inflight >= 0);
 while (q->inflight) {
+int ret;
 int16_t cid;
+
 c = (NvmeCqe *)&q->cq.queue[q->cq.head * NVME_CQ_ENTRY_BYTES];
 if ((le16_to_cpu(c->status) & 0x1) == q->cq_phase) {
 break;
 }
+ret = nvme_translate_error(c);
 q->cq.head = (q->cq.head + 1) % NVME_QUEUE_SIZE;
 if (!q->cq.head) {
 q->cq_phase = !q->cq_phase;
@@ -344,7 +347,7 @@ static bool nvme_process_completion(BDRVNVMeState *s, 
NVMeQueuePair *q)
 preq->busy = false;
 preq->cb = preq->opaque = NULL;
 qemu_mutex_unlock(&q->lock);
-req.cb(req.opaque, nvme_translate_error(c));
+req.cb(req.opaque, ret);
 qemu_mutex_lock(&q->lock);
 q->inflight--;
 progress = true;
-- 
2.26.2

[PULL 12/12] block/nvme: support nested aio_poll()

[Stefan Hajnoczi]

QEMU block drivers are supposed to support aio_poll() from I/O
completion callback functions. This means completion processing must be
re-entrant.

The standard approach is to schedule a BH during completion processing
and cancel it at the end of processing. If aio_poll() is invoked by a
callback function then the BH will run. The BH continues the suspended
completion processing.

All of this means that request A's cb() can synchronously wait for
request B to complete. Previously the nvme block driver would hang
because it didn't process completions from nested aio_poll().

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Sergio Lopez 
Message-id: 20200617132201.1832152-8-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/nvme.c   | 67 --
 block/trace-events |  2 +-
 2 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 8dc68d3daa..374e268915 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -74,9 +74,11 @@ typedef struct {
 int cq_phase;
 int free_req_head;
 NVMeRequest reqs[NVME_NUM_REQS];
-boolbusy;
 int need_kick;
 int inflight;
+
+/* Thread-safe, no lock necessary */
+QEMUBH  *completion_bh;
 } NVMeQueuePair;
 
 /* Memory mapped registers */
@@ -140,6 +142,8 @@ struct BDRVNVMeState {
 #define NVME_BLOCK_OPT_DEVICE "device"
 #define NVME_BLOCK_OPT_NAMESPACE "namespace"
 
+static void nvme_process_completion_bh(void *opaque);
+
 static QemuOptsList runtime_opts = {
 .name = "nvme",
 .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
@@ -181,6 +185,9 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue 
*q,
 
 static void nvme_free_queue_pair(NVMeQueuePair *q)
 {
+if (q->completion_bh) {
+qemu_bh_delete(q->completion_bh);
+}
 qemu_vfree(q->prp_list_pages);
 qemu_vfree(q->sq.queue);
 qemu_vfree(q->cq.queue);
@@ -214,6 +221,8 @@ static NVMeQueuePair 
*nvme_create_queue_pair(BlockDriverState *bs,
 q->index = idx;
 qemu_co_queue_init(&q->free_req_queue);
 q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);
+q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),
+  nvme_process_completion_bh, q);
 r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
   s->page_size * NVME_NUM_REQS,
   false, &prp_list_iova);
@@ -352,11 +361,21 @@ static bool nvme_process_completion(NVMeQueuePair *q)
 NvmeCqe *c;
 
 trace_nvme_process_completion(s, q->index, q->inflight);
-if (q->busy || s->plugged) {
-trace_nvme_process_completion_queue_busy(s, q->index);
+if (s->plugged) {
+trace_nvme_process_completion_queue_plugged(s, q->index);
 return false;
 }
-q->busy = true;
+
+/*
+ * Support re-entrancy when a request cb() function invokes aio_poll().
+ * Pending completions must be visible to aio_poll() so that a cb()
+ * function can wait for the completion of another request.
+ *
+ * The aio_poll() loop will execute our BH and we'll resume completion
+ * processing there.
+ */
+qemu_bh_schedule(q->completion_bh);
+
 assert(q->inflight >= 0);
 while (q->inflight) {
 int ret;
@@ -384,10 +403,10 @@ static bool nvme_process_completion(NVMeQueuePair *q)
 assert(req.cb);
 nvme_put_free_req_locked(q, preq);
 preq->cb = preq->opaque = NULL;
-qemu_mutex_unlock(&q->lock);
-req.cb(req.opaque, ret);
-qemu_mutex_lock(&q->lock);
 q->inflight--;
+qemu_mutex_unlock(&q->lock);
+req.cb(req.opaque, ret);
+qemu_mutex_lock(&q->lock);
 progress = true;
 }
 if (progress) {
@@ -396,10 +415,28 @@ static bool nvme_process_completion(NVMeQueuePair *q)
 *q->cq.doorbell = cpu_to_le32(q->cq.head);
 nvme_wake_free_req_locked(q);
 }
-q->busy = false;
+
+qemu_bh_cancel(q->completion_bh);
+
 return progress;
 }
 
+static void nvme_process_completion_bh(void *opaque)
+{
+NVMeQueuePair *q = opaque;
+
+/*
+ * We're being invoked because a nvme_process_completion() cb() function
+ * called aio_poll(). The callback may be waiting for further completions
+ * so notify the device that it has space to fill in more completions now.
+ */
+smp_mb_release();
+*q->cq.doorbell = cpu_to_le32(q->cq.head);
+nvme_wake_free_req_locked(q);
+
+nvme_process_completion(q);
+}
+
 static void nvme_trace_command(const NvmeCmd *cmd)
 {
 int i;
@@ -1309,6 +1346,13 @@ static void nvme_detach_aio_context(BlockDriverState *bs)
 {
 BDRVNVMeState *s = bs->opaque;
 
+for (int i = 0; i < s->nr_queues; i++) {
+NVMeQueuePair *q = s->queues[i];
+
+qemu_bh_delete(q->completion_bh);
+q->completion_bh = NULL;
+}
+
 aio_set_event_notifier(bdrv_get_aio_context(bs), &s

[PULL 11/12] block/nvme: keep BDRVNVMeState pointer in NVMeQueuePair

[Stefan Hajnoczi]

Passing around both BDRVNVMeState and NVMeQueuePair is unwieldy. Reduce
the number of function arguments by keeping the BDRVNVMeState pointer in
NVMeQueuePair. This will come in handly when a BH is introduced in a
later patch and only one argument can be passed to it.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Sergio Lopez 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20200617132201.1832152-7-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/nvme.c | 70 
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 426c77e5ab..8dc68d3daa 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -39,6 +39,8 @@
  */
 #define NVME_NUM_REQS (NVME_QUEUE_SIZE - 1)
 
+typedef struct BDRVNVMeState BDRVNVMeState;
+
 typedef struct {
 int32_t  head, tail;
 uint8_t  *queue;
@@ -59,8 +61,11 @@ typedef struct {
 typedef struct {
 QemuMutex   lock;
 
+/* Read from I/O code path, initialized under BQL */
+BDRVNVMeState   *s;
+int index;
+
 /* Fields protected by BQL */
-int index;
 uint8_t *prp_list_pages;
 
 /* Fields protected by @lock */
@@ -96,7 +101,7 @@ typedef volatile struct {
 
 QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
 
-typedef struct {
+struct BDRVNVMeState {
 AioContext *aio_context;
 QEMUVFIOState *vfio;
 NVMeRegs *regs;
@@ -130,7 +135,7 @@ typedef struct {
 
 /* PCI address (required for nvme_refresh_filename()) */
 char *device;
-} BDRVNVMeState;
+};
 
 #define NVME_BLOCK_OPT_DEVICE "device"
 #define NVME_BLOCK_OPT_NAMESPACE "namespace"
@@ -174,7 +179,7 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue 
*q,
 }
 }
 
-static void nvme_free_queue_pair(BlockDriverState *bs, NVMeQueuePair *q)
+static void nvme_free_queue_pair(NVMeQueuePair *q)
 {
 qemu_vfree(q->prp_list_pages);
 qemu_vfree(q->sq.queue);
@@ -205,6 +210,7 @@ static NVMeQueuePair 
*nvme_create_queue_pair(BlockDriverState *bs,
 uint64_t prp_list_iova;
 
 qemu_mutex_init(&q->lock);
+q->s = s;
 q->index = idx;
 qemu_co_queue_init(&q->free_req_queue);
 q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);
@@ -240,13 +246,15 @@ static NVMeQueuePair 
*nvme_create_queue_pair(BlockDriverState *bs,
 
 return q;
 fail:
-nvme_free_queue_pair(bs, q);
+nvme_free_queue_pair(q);
 return NULL;
 }
 
 /* With q->lock */
-static void nvme_kick(BDRVNVMeState *s, NVMeQueuePair *q)
+static void nvme_kick(NVMeQueuePair *q)
 {
+BDRVNVMeState *s = q->s;
+
 if (s->plugged || !q->need_kick) {
 return;
 }
@@ -295,21 +303,20 @@ static void nvme_put_free_req_locked(NVMeQueuePair *q, 
NVMeRequest *req)
 }
 
 /* With q->lock */
-static void nvme_wake_free_req_locked(BDRVNVMeState *s, NVMeQueuePair *q)
+static void nvme_wake_free_req_locked(NVMeQueuePair *q)
 {
 if (!qemu_co_queue_empty(&q->free_req_queue)) {
-replay_bh_schedule_oneshot_event(s->aio_context,
+replay_bh_schedule_oneshot_event(q->s->aio_context,
 nvme_free_req_queue_cb, q);
 }
 }
 
 /* Insert a request in the freelist and wake waiters */
-static void nvme_put_free_req_and_wake(BDRVNVMeState *s,  NVMeQueuePair *q,
-   NVMeRequest *req)
+static void nvme_put_free_req_and_wake(NVMeQueuePair *q, NVMeRequest *req)
 {
 qemu_mutex_lock(&q->lock);
 nvme_put_free_req_locked(q, req);
-nvme_wake_free_req_locked(s, q);
+nvme_wake_free_req_locked(q);
 qemu_mutex_unlock(&q->lock);
 }
 
@@ -336,8 +343,9 @@ static inline int nvme_translate_error(const NvmeCqe *c)
 }
 
 /* With q->lock */
-static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)
+static bool nvme_process_completion(NVMeQueuePair *q)
 {
+BDRVNVMeState *s = q->s;
 bool progress = false;
 NVMeRequest *preq;
 NVMeRequest req;
@@ -386,7 +394,7 @@ static bool nvme_process_completion(BDRVNVMeState *s, 
NVMeQueuePair *q)
 /* Notify the device so it can post more completions. */
 smp_mb_release();
 *q->cq.doorbell = cpu_to_le32(q->cq.head);
-nvme_wake_free_req_locked(s, q);
+nvme_wake_free_req_locked(q);
 }
 q->busy = false;
 return progress;
@@ -403,8 +411,7 @@ static void nvme_trace_command(const NvmeCmd *cmd)
 }
 }
 
-static void nvme_submit_command(BDRVNVMeState *s, NVMeQueuePair *q,
-NVMeRequest *req,
+static void nvme_submit_command(NVMeQueuePair *q, NVMeRequest *req,
 NvmeCmd *cmd, BlockCompletionFunc cb,
 void *opaque)
 {
@@ -413,15 +420,15 @@ static void nvme_submit_command(BDRVNVMeState *s, 
NVMeQueuePair *q,
 req->opaque = opaque;
 cmd->cid = cpu_to_le32(req->cid);
 
-trace_nvme_submit_command(s, q->index, req->cid);
+trace_nvme_submit_command(q->s, q->index, req->cid);
   

[PULL 09/12] block/nvme: switch to a NVMeRequest freelist

[Stefan Hajnoczi]

There are three issues with the current NVMeRequest->busy field:
1. The busy field is accidentally accessed outside q->lock when request
   submission fails.
2. Waiters on free_req_queue are not woken when a request is returned
   early due to submission failure.
2. Finding a free request involves scanning all requests. This makes
   request submission O(n^2).

Switch to an O(1) freelist that is always accessed under the lock.

Also differentiate between NVME_QUEUE_SIZE, the actual SQ/CQ size, and
NVME_NUM_REQS, the number of usable requests. This makes the code
simpler than using NVME_QUEUE_SIZE everywhere and having to keep in mind
that one slot is reserved.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Sergio Lopez 
Message-id: 20200617132201.1832152-5-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/nvme.c | 81 ++--
 1 file changed, 54 insertions(+), 27 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 344893811a..8e60882af3 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -33,6 +33,12 @@
 #define NVME_QUEUE_SIZE 128
 #define NVME_BAR_SIZE 8192
 
+/*
+ * We have to leave one slot empty as that is the full queue case where
+ * head == tail + 1.
+ */
+#define NVME_NUM_REQS (NVME_QUEUE_SIZE - 1)
+
 typedef struct {
 int32_t  head, tail;
 uint8_t  *queue;
@@ -47,7 +53,7 @@ typedef struct {
 int cid;
 void *prp_list_page;
 uint64_t prp_list_iova;
-bool busy;
+int free_req_next; /* q->reqs[] index of next free req */
 } NVMeRequest;
 
 typedef struct {
@@ -61,7 +67,8 @@ typedef struct {
 /* Fields protected by @lock */
 NVMeQueue   sq, cq;
 int cq_phase;
-NVMeRequest reqs[NVME_QUEUE_SIZE];
+int free_req_head;
+NVMeRequest reqs[NVME_NUM_REQS];
 boolbusy;
 int need_kick;
 int inflight;
@@ -200,19 +207,23 @@ static NVMeQueuePair 
*nvme_create_queue_pair(BlockDriverState *bs,
 qemu_mutex_init(&q->lock);
 q->index = idx;
 qemu_co_queue_init(&q->free_req_queue);
-q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE);
+q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);
 r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
-  s->page_size * NVME_QUEUE_SIZE,
+  s->page_size * NVME_NUM_REQS,
   false, &prp_list_iova);
 if (r) {
 goto fail;
 }
-for (i = 0; i < NVME_QUEUE_SIZE; i++) {
+q->free_req_head = -1;
+for (i = 0; i < NVME_NUM_REQS; i++) {
 NVMeRequest *req = &q->reqs[i];
 req->cid = i + 1;
+req->free_req_next = q->free_req_head;
+q->free_req_head = i;
 req->prp_list_page = q->prp_list_pages + i * s->page_size;
 req->prp_list_iova = prp_list_iova + i * s->page_size;
 }
+
 nvme_init_queue(bs, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
 if (local_err) {
 error_propagate(errp, local_err);
@@ -254,13 +265,11 @@ static void nvme_kick(BDRVNVMeState *s, NVMeQueuePair *q)
  */
 static NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
 {
-int i;
-NVMeRequest *req = NULL;
+NVMeRequest *req;
 
 qemu_mutex_lock(&q->lock);
-while (q->inflight + q->need_kick > NVME_QUEUE_SIZE - 2) {
-/* We have to leave one slot empty as that is the full queue case (head
- * == tail + 1). */
+
+while (q->free_req_head == -1) {
 if (qemu_in_coroutine()) {
 trace_nvme_free_req_queue_wait(q);
 qemu_co_queue_wait(&q->free_req_queue, &q->lock);
@@ -269,20 +278,41 @@ static NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
 return NULL;
 }
 }
-for (i = 0; i < NVME_QUEUE_SIZE; i++) {
-if (!q->reqs[i].busy) {
-q->reqs[i].busy = true;
-req = &q->reqs[i];
-break;
-}
-}
-/* We have checked inflight and need_kick while holding q->lock, so one
- * free req must be available. */
-assert(req);
+
+req = &q->reqs[q->free_req_head];
+q->free_req_head = req->free_req_next;
+req->free_req_next = -1;
+
 qemu_mutex_unlock(&q->lock);
 return req;
 }
 
+/* With q->lock */
+static void nvme_put_free_req_locked(NVMeQueuePair *q, NVMeRequest *req)
+{
+req->free_req_next = q->free_req_head;
+q->free_req_head = req - q->reqs;
+}
+
+/* With q->lock */
+static void nvme_wake_free_req_locked(BDRVNVMeState *s, NVMeQueuePair *q)
+{
+if (!qemu_co_queue_empty(&q->free_req_queue)) {
+replay_bh_schedule_oneshot_event(s->aio_context,
+nvme_free_req_queue_cb, q);
+}
+}
+
+/* Insert a request in the freelist and wake waiters */
+static void nvme_put_free_req_and_wake(BDRVNVMeState *s,  NVMeQueuePair *q,
+   NVMeRequest *req)
+{
+qemu_mutex_lock(&q->lock);
+nvme_put_free_req_locked(q, req);
+nvme_wake_free_req_loc

Re: [PATCH] MAINTAINERS: Cover pip requirements.txt

[Philippe Mathieu-Daudé]

Cleber are you OK with this?

On 6/5/20 6:37 PM, Philippe Mathieu-Daudé wrote:
> Add an entry in 'Python scripts' to cover the requirements.txt
> file added in commit 213137217a6 (this file contains a list of
> Python packages used by our test suite).
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3e7d9cb0a5..fc4148fba3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2219,6 +2219,7 @@ S: Odd fixes
>  F: python/qemu/*py
>  F: scripts/*.py
>  F: tests/*.py
> +F: tests/requirements.txt
>  
>  Benchmark util
>  M: Vladimir Sementsov-Ogievskiy 
> 

Re: [PATCH v1] chardev/char-socket: fix double free of err after socket is disconnected

[Philippe Mathieu-Daudé]

On 6/24/20 12:00 PM, Derek Su wrote:
> The err is freed in check_report_connect_error() conditionally,
> calling error_free() directly may lead to a double-free bug.

This seems the same issue Lichun is working on, right?
https://www.mail-archive.com/qemu-devel@nongnu.org/msg714709.html

> 
> Signed-off-by: Derek Su 
> ---
>  chardev/char-socket.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index afebeec5c3..a009bed5ee 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1086,7 +1086,11 @@ static void qemu_chr_socket_connected(QIOTask *task, 
> void *opaque)
>  if (qio_task_propagate_error(task, &err)) {
>  tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
>  check_report_connect_error(chr, err);
> -error_free(err);
> +
> +if (!s->connect_err_reported) {
> +error_free(err);
> +}
> +
>  goto cleanup;
>  }
>  
> 

Re: [PATCH v1] chardev/char-socket: fix double free of err after socket is disconnected

[Derek Su]

Oops! Sorry, I dont’t notice this patch before.

Thanks.

Derek

Philippe Mathieu-Daudé 於 2020年6月24日 週三，下午6:12寫道：

> On 6/24/20 12:00 PM, Derek Su wrote:
> > The err is freed in check_report_connect_error() conditionally,
> > calling error_free() directly may lead to a double-free bug.
>
> This seems the same issue Lichun is working on, right?
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714709.html
>
> >
> > Signed-off-by: Derek Su 
> > ---
> >  chardev/char-socket.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index afebeec5c3..a009bed5ee 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1086,7 +1086,11 @@ static void qemu_chr_socket_connected(QIOTask
> *task, void *opaque)
> >  if (qio_task_propagate_error(task, &err)) {
> >  tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
> >  check_report_connect_error(chr, err);
> > -error_free(err);
> > +
> > +if (!s->connect_err_reported) {
> > +error_free(err);
> > +}
> > +
> >  goto cleanup;
> >  }
> >
> >
>
> --

Best regards,

Derek Su
QNAP Systems, Inc.
Email: dere...@qnap.com
Tel: (+886)-2-2393-5152 ext. 15017
Address: 13F., No.56, Sec. 1, Xinsheng S. Rd., Zhongzheng Dist., Taipei
City, Taiwan

Re: [PATCH] Revert "tests/migration: Reduce autoconverge initial bandwidth"

[Philippe Mathieu-Daudé]

On 6/24/20 7:04 AM, Thomas Huth wrote:
> On 23/06/2020 19.35, Philippe Mathieu-Daudé wrote:
>> On 6/23/20 7:07 PM, Thomas Huth wrote:
>>> On 23/06/2020 17.39, Philippe Mathieu-Daudé wrote:
 On 6/23/20 4:56 PM, Michael S. Tsirkin wrote:
> This reverts commit 6d1da867e65f ("tests/migration: Reduce autoconverge 
> initial bandwidth")
> since that change makes unit tests much slower for all developers, while 
> it's not
> a robust way to fix migration tests. Migration tests need to find
> a more robust way to discover a reasonable bandwidth without slowing
> things down for everyone.

 Please also mention we can do this since 1de8e4c4dcf which allow
 marked the s390x job as "unstable" and allow it to fail.

 But if nobody is going to look at it, instead lets disable
 it until someone figure out the issue:

 -- >8 --
 diff --git a/.travis.yml b/.travis.yml
 index 74158f741b..364e67b14b 100644
 --- a/.travis.yml
 +++ b/.travis.yml
 @@ -507,6 +507,7 @@ jobs:

  - name: "[s390x] Clang (disable-tcg)"
arch: s390x
 +  if: false # Temporarily disabled due to issue testing migration
 (see commit 6d1da867e65).
dist: bionic
compiler: clang
addons:
>>>
>>> Sorry, but that looks wrong. First, the disable-tcg test does not run
>>> the qtests at all. So this is certainly the wrong location here.
>>
>> Indeed, this is the previous job:
>>
>> -- >8 --
>> diff --git a/.travis.yml b/.travis.yml
>> index 74158f741b..b399e20078 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -464,6 +464,7 @@ jobs:
>>  - CONFIG="--disable-containers
>> --target-list=ppc64-softmmu,ppc64le-linux-user"
>>
>>  - name: "[s390x] GCC check-tcg"
>> +  if: false # Temporarily disabled due to issue testing migration
>> (see commit 6d1da867e65).
>>arch: s390x
>>dist: bionic
>>addons:
>> ---
>>
>>> Second,
>>> if just one of the qtests is failing, please only disable that single
>>> failing qtest and not the whole test pipeline.
>>
>> Last time we talked about this Dave was against that option:
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg690085.html
>>
> 
> Was he? Citing his reply to the mail from your URL:
> 
>  "Before we take the hammer to it, could you try reducing it's initial
> bandwidth"
> 
> So all I can see is that he first wanted to try something different than
> disabling the test. And now,  instead of using a small hammer to disable
> just this test, you now even use a very *big* hammer to disable *all*
> tests. That's just a very bad idea. Please don't.

You are right. I was being concerned about having CI working because
the more red it stay, the less likely the community will worry about
it, and I didn't want we loose interest in testing (or discredit its
importance). I now understand without having CI gating, it is
pointless to try to keep it green (at the cost of having all local
testing running slower, it is worst if maintainers stop their local
testing).

WRT this test I have no idea what it is doing, furthermore why it
fails on s390x containers, so I sent a simple patch to fix the CI,
but failed to foreseen its negative effect on the rest of the
developers.

Thanks Michael for fixing my mess with your patch:

Reviewed-by: Philippe Mathieu-Daudé 

Regards,

Phil.

RE: sysbus failed assert for xen_sysdev

[Paul Durrant]

> -Original Message-
> From: Jason Andryuk 
> Sent: 24 June 2020 04:24
> To: Paul Durrant 
> Cc: Markus Armbruster ; Mark Cave-Ayland 
> ; Anthony
> PERARD ; xen-devel 
> ; QEMU  de...@nongnu.org>
> Subject: Re: sysbus failed assert for xen_sysdev
> 
> On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant  wrote:
> >
> > > -Original Message-
> > > From: Markus Armbruster 
> > > Sent: 23 June 2020 09:41
> > > To: Jason Andryuk 
> > > Cc: Mark Cave-Ayland ; Anthony PERARD 
> > > ;
> xen-
> > > devel ; Paul Durrant ; QEMU 
> > > 
> > > Subject: Re: sysbus failed assert for xen_sysdev
> > >
> > > Jason Andryuk  writes:
> > > > Then it gets farther... until
> > > > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > > > Assertion `dev->realized' failed.
> > > >
> > > > dev->id is NULL. The failing device is:
> > > > (gdb) p *dev.parent_obj.class.type
> > > > $12 = {name = 0x56207770 "cfi.pflash01",
> > > >
> >
> > Having commented out the call to xen_be_init() entirely (and xen_bus_init() 
> > for good measure) I also
> get this assertion failure, so
> > I don't think is related.
> 
> Yes, this is something different.  pc_pflash_create() calls
> qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
> pc_system_flash_map()...  and pc_system_flash_map() isn't called for
> Xen.
> 
> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
> there since accelerators have not been initialized yes, I guess?

Looks like it can be worked round by the following:

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1497d0e4ae..977d40afb8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
 if (!xen_enabled()) {
 pc_memory_init(pcms, system_memory,
rom_memory, &ram_memory);
-} else if (machine->kernel_filename != NULL) {
-/* For xen HVM direct kernel boot, load linux here */
-xen_load_linux(pcms);
+} else {
+pc_system_flash_cleanup_unused(pcms);
+if (machine->kernel_filename != NULL) {
+/* For xen HVM direct kernel boot, load linux here */
+xen_load_linux(pcms);
+}
 }

 gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index ec2a3b3e7e..0ff47a4b59 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
 }
 }

-static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
+void pc_system_flash_cleanup_unused(PCMachineState *pcms)
 {
 char *prop_name;
 int i;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e6135c34d6..497f2b7ab7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);

 /* pc_sysfw.c */
 void pc_system_flash_create(PCMachineState *pcms);
+void pc_system_flash_cleanup_unused(PCMachineState *pcms);
 void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);

 /* acpi-build.c */


> 
> Regards,
> Jason

[PATCH V6 1/4] hw/mips: Implement the kvm_type() hook in MachineClass

[Huacai Chen]

MIPS has two types of KVM: TE & VZ, and TE is the default type. Now we
can't create a VZ guest in QEMU because it lacks the kvm_type() hook in
MachineClass. This patch add the the kvm_type() hook to support both of
the two types.

Reviewed-by: Aleksandar Markovic 
Signed-off-by: Huacai Chen 
Co-developed-by: Jiaxun Yang 
---
 target/mips/kvm.c  | 20 
 target/mips/kvm_mips.h | 11 +++
 2 files changed, 31 insertions(+)

diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index 96cfa10..373f582 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -21,10 +21,12 @@
 #include "qemu/main-loop.h"
 #include "qemu/timer.h"
 #include "sysemu/kvm.h"
+#include "sysemu/kvm_int.h"
 #include "sysemu/runstate.h"
 #include "sysemu/cpus.h"
 #include "kvm_mips.h"
 #include "exec/memattrs.h"
+#include "hw/boards.h"
 
 #define DEBUG_KVM 0
 
@@ -1270,3 +1272,21 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
 abort();
 }
+
+int mips_kvm_type(MachineState *machine, const char *vm_type)
+{
+int r;
+KVMState *s = KVM_STATE(machine->accelerator);
+
+r = kvm_check_extension(s, KVM_CAP_MIPS_VZ);
+if (r > 0) {
+return KVM_VM_MIPS_VZ;
+}
+
+r = kvm_check_extension(s, KVM_CAP_MIPS_TE);
+if (r > 0) {
+return KVM_VM_MIPS_TE;
+}
+
+return -1;
+}
diff --git a/target/mips/kvm_mips.h b/target/mips/kvm_mips.h
index 1e40147..171d53d 100644
--- a/target/mips/kvm_mips.h
+++ b/target/mips/kvm_mips.h
@@ -12,6 +12,8 @@
 #ifndef KVM_MIPS_H
 #define KVM_MIPS_H
 
+#include "cpu.h"
+
 /**
  * kvm_mips_reset_vcpu:
  * @cpu: MIPSCPU
@@ -23,4 +25,13 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu);
 int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq, int level);
 int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int irq, int level);
 
+#ifdef CONFIG_KVM
+int mips_kvm_type(MachineState *machine, const char *vm_type);
+#else
+static inline int mips_kvm_type(MachineState *machine, const char *vm_type)
+{
+return 0;
+}
+#endif
+
 #endif /* KVM_MIPS_H */
-- 
2.7.0

[PATCH V6 0/4] mips: Add Loongson-3 machine support (with KVM)

[Huacai Chen]

Loongson-3 CPU family include Loongson-3A R1/R2/R3/R4 and Loongson-3B
R1/R2. Loongson-3A R1 is the oldest and its ISA is the smallest, while
Loongson-3A R4 is the newest and its ISA is almost the superset of all
others. To reduce complexity, in QEMU we just define two CPU types:

1, "Loongson-3A1000" CPU which is corresponding to Loongson-3A R1. It is
   suitable for TCG because Loongson-3A R1 has fewest ASE.
2, "Loongson-3A4000" CPU which is corresponding to Loongson-3A R4. It is
   suitable for KVM because Loongson-3A R4 has the VZ ASE.

Loongson-3 lacks English documents. I've tried to translated them with
translate.google.com, and the machine translated documents (together
with their original Chinese versions) are available here.

Loongson-3A R1 (Loongson-3A1000)
User Manual Part 1:
http://ftp.godson.ac.cn/lemote/3A1000_p1.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A1000_processor_user_manual_P1.pdf 
(Chinese Version)
User Manual Part 2:
http://ftp.godson.ac.cn/lemote/3A1000_p2.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A1000_processor_user_manual_P2.pdf 
(Chinese Version)

Loongson-3A R2 (Loongson-3A2000)
User Manual Part 1:
http://ftp.godson.ac.cn/lemote/3A2000_p1.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A2000_user1.pdf (Chinese Version)
User Manual Part 2:
http://ftp.godson.ac.cn/lemote/3A2000_p2.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A2000_user2.pdf (Chinese Version)

Loongson-3A R3 (Loongson-3A3000)
User Manual Part 1:
http://ftp.godson.ac.cn/lemote/3A3000_p1.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual1.pdf (Chinese 
Version)
User Manual Part 2:
http://ftp.godson.ac.cn/lemote/3A3000_p2.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual2.pdf (Chinese 
Version)

Loongson-3A R4 (Loongson-3A4000)
User Manual Part 1:
http://ftp.godson.ac.cn/lemote/3A4000_p1.pdf
http://ftp.godson.ac.cn/lemote/3A4000user.pdf (Chinese Version)
User Manual Part 2:
I'm sorry that it is unavailable now.

We are preparing to add QEMU's Loongson-3 support. MIPS VZ extension is
fully supported in Loongson-3A R4+, so we at first add QEMU/KVM support
in this series. And the next series will add QEMU/TCG support (it will
emulate Loongson-3A R1).

We already have a full functional Linux kernel (based on Linux-5.4.x LTS
but not upstream yet) here:

https://github.com/chenhuacai/linux

How to use QEMU/Loongson-3?
1, Download kernel source from the above URL;
2, Build a kernel with arch/mips/configs/loongson3_{def,hpc}config;
3, Boot a Loongson-3A4000 host with this kernel;
4, Build QEMU-5.0.0 with this patchset;
5, modprobe kvm;
6, Use QEMU with TCG (available in future):
   qemu-system-mips64el -M loongson3-virt,accel=tcg -cpu Loongson-3A1000 
-kernel  -append ...
   Use QEMU with KVM (available at present):
   qemu-system-mips64el -M loongson3-virt,accel=kvm -cpu Loongson-3A4000 
-kernel  -append ...

   The "-cpu" parameter is optional here and QEMU will use the correct type for 
TCG/KVM automatically.

V1 -> V2:
1, Add a cover letter;
2, Improve CPU definitions;
3, Remove LS7A-related things (Use GPEX instead);
4, Add a description of how to run QEMU/Loongson-3.

V2 -> V3:
1, Fix all possible checkpatch.pl errors and warnings.

V3 -> V4:
1, Sync code with upstream;
2, Remove merged patches;
3, Fix build failure without CONFIG_KVM;
4, Add Reviewed-by: Aleksandar Markovic .

V4 -> V5:
1, Improve coding style;
2, Remove merged patches;
3, Rename machine name from "loongson3" to "loongson3-virt";
4, Rework the "loongson3-virt" machine to drop any ISA things;
5, Rework "hw/mips: Implement the kvm_type() hook in MachineClass";
6, Add Jiaxun Yang as a reviewer of Loongson-3.

V5 -> V6:
1, Fix license preamble;
2, Improve commit messages;
3, Add hw/intc/loongson_liointc.c to MAINTAINERS;
4, Fix all possible checkpatch.pl errors and warnings.

Huacai Chen(4):
 hw/mips: Implement the kvm_type() hook in MachineClass
 hw/intc: Add Loongson liointc support
 hw/mips: Add Loongson-3 machine support (with KVM)
 MAINTAINERS: Add Loongson-3 maintainer and reviewer

Signed-off-by: Huacai Chen 
---
 MAINTAINERS  |   7 +
 default-configs/mips64el-softmmu.mak |   1 +
 hw/intc/Kconfig  |   3 +
 hw/intc/Makefile.objs|   1 +
 hw/intc/loongson_liointc.c   | 241 +
 hw/mips/Kconfig  |  11 +
 hw/mips/Makefile.objs|   1 +
 hw/mips/loongson3_virt.c | 978 +++
 target/mips/kvm.c|  20 +
 target/mips/kvm_mips.h   |  11 +
 10 files changed, 1274 insertions(+)
 create mode 100644 hw/intc/loongson_liointc.c
 create mode 100644 hw/mips/loongson3_virt.c
--
2.7.0

[PATCH V6 3/4] hw/mips: Add Loongson-3 machine support (with KVM)

[Huacai Chen]

Add Loongson-3 based machine support, it use liointc as the interrupt
controler and use GPEX as the pci controller. Currently it can only work
with KVM, but we will add TCG support in future.

As the machine model is not based on any exiting physical hardware, the
name of the machine is "loongson3-virt". It may be superseded in future
by a real machine model. If this happens, then a regular deprecation
procedure shall occur for "loongson3-virt" machine.

We now already have a full functional Linux kernel (based on Linux-5.4.x
LTS, the kvm host side has been upstream in Linux-5.8, but the kvm guest
side has not been upstream yet) here:

https://github.com/chenhuacai/linux

How to use QEMU/Loongson-3?
1, Download kernel source from the above URL;
2, Build a kernel with arch/mips/configs/loongson3_{def,hpc}config;
3, Boot the a Loongson-3A4000 host with this kernel;
4, Build QEMU-master with this patchset;
5, modprobe kvm;
6, Use QEMU with TCG (available in future):
   qemu-system-mips64el -M loongson3-virt,accel=tcg -cpu Loongson-3A1000 
-kernel  -append ...
   Use QEMU with KVM (available at present):
   qemu-system-mips64el -M loongson3-virt,accel=kvm -cpu Loongson-3A4000 
-kernel  -append ...

   The "-cpu" parameter is optional here and QEMU will use the correct type for 
TCG/KVM automatically.

Signed-off-by: Huacai Chen 
Co-developed-by: Jiaxun Yang 
---
 default-configs/mips64el-softmmu.mak |   1 +
 hw/mips/Kconfig  |  11 +
 hw/mips/Makefile.objs|   1 +
 hw/mips/loongson3_virt.c | 978 +++
 4 files changed, 991 insertions(+)
 create mode 100644 hw/mips/loongson3_virt.c

diff --git a/default-configs/mips64el-softmmu.mak 
b/default-configs/mips64el-softmmu.mak
index 9f8a3ef..26c660a 100644
--- a/default-configs/mips64el-softmmu.mak
+++ b/default-configs/mips64el-softmmu.mak
@@ -3,6 +3,7 @@
 include mips-softmmu-common.mak
 CONFIG_IDE_VIA=y
 CONFIG_FULOONG=y
+CONFIG_LOONGSON3V=y
 CONFIG_ATI_VGA=y
 CONFIG_RTL8139_PCI=y
 CONFIG_JAZZ=y
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index 67d39c5..cc5609b 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -45,6 +45,17 @@ config FULOONG
 bool
 select PCI_BONITO
 
+config LOONGSON3V
+bool
+select PCKBD
+select SERIAL
+select GOLDFISH_RTC
+select LOONGSON_LIOINTC
+select PCI_EXPRESS_GENERIC_BRIDGE
+select VIRTIO_VGA
+select QXL if SPICE
+select MSI_NONBROKEN
+
 config MIPS_CPS
 bool
 select PTIMER
diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs
index 739e2b7..0993852 100644
--- a/hw/mips/Makefile.objs
+++ b/hw/mips/Makefile.objs
@@ -4,5 +4,6 @@ obj-$(CONFIG_MALTA) += gt64xxx_pci.o malta.o
 obj-$(CONFIG_MIPSSIM) += mipssim.o
 obj-$(CONFIG_JAZZ) += jazz.o
 obj-$(CONFIG_FULOONG) += fuloong2e.o
+obj-$(CONFIG_LOONGSON3V) += loongson3_virt.o
 obj-$(CONFIG_MIPS_CPS) += cps.o
 obj-$(CONFIG_MIPS_BOSTON) += boston.o
diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
new file mode 100644
index 000..e0e1939
--- /dev/null
+++ b/hw/mips/loongson3_virt.c
@@ -0,0 +1,978 @@
+/*
+ * Generic Loongson-3 Platform support
+ *
+ * Copyright (c) 2016-2020 Huacai Chen (che...@lemote.com)
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see .
+ */
+
+/*
+ * Generic virtualized PC Platform based on Loongson-3 CPU (MIPS64R2 with
+ * extensions, 800~2000MHz)
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "cpu.h"
+#include "elf.h"
+#include "kvm_mips.h"
+#include "hw/boards.h"
+#include "hw/char/serial.h"
+#include "hw/mips/mips.h"
+#include "hw/mips/cpudevs.h"
+#include "hw/misc/empty_slot.h"
+#include "hw/intc/i8259.h"
+#include "hw/loader.h"
+#include "hw/isa/superio.h"
+#include "hw/pci/msi.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci_host.h"
+#include "hw/pci-host/gpex.h"
+#include "hw/rtc/mc146818rtc.h"
+#include "hw/usb.h"
+#include "net/net.h"
+#include "exec/address-spaces.h"
+#include "sysemu/kvm.h"
+#include "sysemu/qtest.h"
+#include "sysemu/reset.h"
+#include "sysemu/runstate.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+
+#define PM_CNTL_MODE  0x10
+
+/* Overall MMIO & Memory layout */
+enum {
+VIRT_LOWMEM,
+VIRT_PM,
+VIRT_FW_CFG,
+VIRT_RTC,
+VIRT_PCIE_PIO,
+VIRT_PCIE_ECAM,
+VIRT_BIOS_ROM,
+ 

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Cornelia Huck]

On Wed, 24 Jun 2020 03:52:14 -0400
Janosch Frank  wrote:

> The bios is in dire need for a cleanup as there are still a lot of
> magic constants being used throughout as well as duplicated code.
> 
> In the first part of this series we consolidate constants and
> functions, as well as doing some minor cleanups and fixes.
> 
> The patches are available here:
> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
> 
> v5:
>   * Fixed whitespace damage
>   * Removed reset PSW mask changes in dasd-ipl.c
>   * Added jump2ipl.c cleanup patches
> 
> v4:
>   * Renamed time.h to s390-time.h
>   * Fixed function names in sleep()
>   * Changed order of sense_id_ccw initialization
>   * Added missing include before sleep()
> 
> v3:
>   * Dropped 0x00 to 0x0/0 patch
>   * Moved some timing functions into helper.h instead of time.h
>   * Fixed IPL psw manipulation in dasd-ipl.c
>   * Minor cosmetic fixes found by review
> 
> v2:
>   * Included cio fixup to get rid of compile errors...
>   * Minor cosmetic fixes found by review
> 
> 
> Janosch Frank (12):
>   pc-bios: s390x: cio.c cleanup and compile fix
>   pc-bios: s390x: Consolidate timing functions into time.h
>   pc-bios: s390x: Move sleep and yield to helper.h
>   pc-bios: s390x: Get rid of magic offsets into the lowcore
>   pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>   pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>   pc-bios: s390x: Use PSW masks where possible and introduce
> PSW_MASK_SHORT_ADDR
>   pc-bios: s390x: Move panic() into header and add infinite loop
>   pc-bios: s390x: Use ebcdic2ascii table
>   pc-bios: s390x: Make u32 ptr check explicit
>   pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>   pc-bios: s390x: Cleanup jump to ipl code
> 
>  pc-bios/s390-ccw/bootmap.c |  9 
>  pc-bios/s390-ccw/bootmap.h |  2 +-
>  pc-bios/s390-ccw/cio.c | 40 +++---
>  pc-bios/s390-ccw/cio.h | 17 ++-
>  pc-bios/s390-ccw/dasd-ipl.c|  3 ---
>  pc-bios/s390-ccw/helper.h  | 19 +++-
>  pc-bios/s390-ccw/jump2ipl.c| 35 -
>  pc-bios/s390-ccw/main.c| 15 +++--
>  pc-bios/s390-ccw/menu.c|  1 +
>  pc-bios/s390-ccw/netmain.c | 23 +++
>  pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>  pc-bios/s390-ccw/s390-ccw.h| 27 ++-
>  pc-bios/s390-ccw/s390-time.h   | 23 +++
>  pc-bios/s390-ccw/start.S   |  5 +++--
>  pc-bios/s390-ccw/virtio-net.c  |  2 ++
>  pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>  pc-bios/s390-ccw/virtio.c  | 18 +++
>  17 files changed, 120 insertions(+), 125 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/s390-time.h
> 

Hm... what's the general status of this? Most of the patches have at
least one R-b/A-b already, I see.

Do the s390-ccw boot maintainers want to pick this (once the rest has
been looked at) and then send me a pull req, or should I pick it when
it is good to go? Softfreeze is less than two weeks away :)

[PATCH V6 4/4] MAINTAINERS: Add Loongson-3 maintainer and reviewer

[Huacai Chen]

Add myself as a maintainer of Loongson-3 virtual platform, and also add
Jiaxun Yang as a reviewer.

Signed-off-by: Huacai Chen 
Co-developed-by: Jiaxun Yang 
---
 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 51a4570..0226a74 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1095,6 +1095,13 @@ F: hw/isa/vt82c686.c
 F: hw/pci-host/bonito.c
 F: include/hw/isa/vt82c686.h
 
+Loongson-3 Virtual Platform
+M: Huacai Chen 
+R: Jiaxun Yang 
+S: Maintained
+F: hw/mips/loongson3_virt.c
+F: hw/intc/loongson_liointc.c
+
 Boston
 M: Paul Burton 
 R: Aleksandar Rikalo 
-- 
2.7.0

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Thomas Huth]

On 24/06/2020 12.44, Cornelia Huck wrote:

On Wed, 24 Jun 2020 03:52:14 -0400
Janosch Frank  wrote:


The bios is in dire need for a cleanup as there are still a lot of
magic constants being used throughout as well as duplicated code.

In the first part of this series we consolidate constants and
functions, as well as doing some minor cleanups and fixes.

The patches are available here:
https://github.com/frankjaa/qemu/pull/new/cleanup_bios

v5:
* Fixed whitespace damage
* Removed reset PSW mask changes in dasd-ipl.c
* Added jump2ipl.c cleanup patches

v4:
* Renamed time.h to s390-time.h
* Fixed function names in sleep()
* Changed order of sense_id_ccw initialization
* Added missing include before sleep()

v3:
* Dropped 0x00 to 0x0/0 patch
* Moved some timing functions into helper.h instead of time.h
* Fixed IPL psw manipulation in dasd-ipl.c
* Minor cosmetic fixes found by review

v2:
* Included cio fixup to get rid of compile errors...
* Minor cosmetic fixes found by review


Janosch Frank (12):
   pc-bios: s390x: cio.c cleanup and compile fix
   pc-bios: s390x: Consolidate timing functions into time.h
   pc-bios: s390x: Move sleep and yield to helper.h
   pc-bios: s390x: Get rid of magic offsets into the lowcore
   pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
   pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
   pc-bios: s390x: Use PSW masks where possible and introduce
 PSW_MASK_SHORT_ADDR
   pc-bios: s390x: Move panic() into header and add infinite loop
   pc-bios: s390x: Use ebcdic2ascii table
   pc-bios: s390x: Make u32 ptr check explicit
   pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
   pc-bios: s390x: Cleanup jump to ipl code

  pc-bios/s390-ccw/bootmap.c |  9 
  pc-bios/s390-ccw/bootmap.h |  2 +-
  pc-bios/s390-ccw/cio.c | 40 +++---
  pc-bios/s390-ccw/cio.h | 17 ++-
  pc-bios/s390-ccw/dasd-ipl.c|  3 ---
  pc-bios/s390-ccw/helper.h  | 19 +++-
  pc-bios/s390-ccw/jump2ipl.c| 35 -
  pc-bios/s390-ccw/main.c| 15 +++--
  pc-bios/s390-ccw/menu.c|  1 +
  pc-bios/s390-ccw/netmain.c | 23 +++
  pc-bios/s390-ccw/s390-arch.h   |  4 +++-
  pc-bios/s390-ccw/s390-ccw.h| 27 ++-
  pc-bios/s390-ccw/s390-time.h   | 23 +++
  pc-bios/s390-ccw/start.S   |  5 +++--
  pc-bios/s390-ccw/virtio-net.c  |  2 ++
  pc-bios/s390-ccw/virtio-scsi.c |  2 ++
  pc-bios/s390-ccw/virtio.c  | 18 +++
  17 files changed, 120 insertions(+), 125 deletions(-)
  create mode 100644 pc-bios/s390-ccw/s390-time.h



Hm... what's the general status of this? Most of the patches have at
least one R-b/A-b already, I see.

Do the s390-ccw boot maintainers want to pick this (once the rest has
been looked at) and then send me a pull req, or should I pick it when
it is good to go? Softfreeze is less than two weeks away :)


I'd like to review the missing parts and run my tests with the patches 
applied ... I'm just a little bit swamped right now, so please give me 
some more time...


 Thomas

[PATCH V6 2/4] hw/intc: Add Loongson liointc support

[Huacai Chen]

Loongson-3 has an integrated liointc (Local I/O interrupt controller).
It is similar to goldfish interrupt controller, but more powerful (e.g.,
it can route external interrupt to multi-cores).

Documents about Loongson-3's liointc:
1, https://wiki.godson.ac.cn/ip_block:liointc;
2, The "I/O中断" section of Loongson-3's user mannual, part 1.

Signed-off-by: Huacai Chen 
Signed-off-by: Jiaxun Yang 
---
 hw/intc/Kconfig|   3 +
 hw/intc/Makefile.objs  |   1 +
 hw/intc/loongson_liointc.c | 241 +
 3 files changed, 245 insertions(+)
 create mode 100644 hw/intc/loongson_liointc.c

diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index f562342..2ae1e89 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -64,3 +64,6 @@ config OMPIC
 
 config RX_ICU
 bool
+
+config LOONGSON_LIOINTC
+bool
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index a420263..3ac2b40 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -51,3 +51,4 @@ obj-$(CONFIG_MIPS_CPS) += mips_gic.o
 obj-$(CONFIG_NIOS2) += nios2_iic.o
 obj-$(CONFIG_OMPIC) += ompic.o
 obj-$(CONFIG_IBEX) += ibex_plic.o
+obj-$(CONFIG_LOONGSON_LIOINTC) += loongson_liointc.o
diff --git a/hw/intc/loongson_liointc.c b/hw/intc/loongson_liointc.c
new file mode 100644
index 000..e39e39e
--- /dev/null
+++ b/hw/intc/loongson_liointc.c
@@ -0,0 +1,241 @@
+/*
+ * QEMU Loongson Local I/O interrupt controler.
+ *
+ * Copyright (c) 2020 Jiaxun Yang 
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "qemu/module.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+
+#define D(x)
+
+#define NUM_IRQS32
+
+#define NUM_CORES   4
+#define NUM_IPS 4
+#define NUM_PARENTS (NUM_CORES * NUM_IPS)
+#define PARENT_COREx_IPy(x, y)(NUM_IPS * x + y)
+
+#define R_MAPPER_START0x0
+#define R_MAPPER_END  0x20
+#define R_ISR   R_MAPPER_END
+#define R_IEN   0x24
+#define R_IEN_SET   0x28
+#define R_IEN_CLR   0x2c
+#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)
+#define R_END   0x64
+
+#define TYPE_LOONGSON_LIOINTC "loongson.liointc"
+#define LOONGSON_LIOINTC(obj) \
+OBJECT_CHECK(struct loongson_liointc, (obj), TYPE_LOONGSON_LIOINTC)
+
+struct loongson_liointc {
+SysBusDevice parent_obj;
+
+MemoryRegion mmio;
+qemu_irq parent_irq[NUM_PARENTS];
+
+uint8_t mapper[NUM_IRQS]; /* 0:3 for core, 4:7 for IP */
+uint32_t isr;
+uint32_t ien;
+uint32_t per_core_isr[NUM_CORES];
+
+/* state of the interrupt input pins */
+uint32_t pin_state;
+bool parent_state[NUM_PARENTS];
+};
+
+static void update_irq(struct loongson_liointc *p)
+{
+uint32_t irq, core, ip;
+uint32_t per_ip_isr[NUM_IPS] = {0};
+
+/* level triggered interrupt */
+p->isr = p->pin_state;
+
+/* Clear disabled IRQs */
+p->isr &= p->ien;
+
+/* Clear per_core_isr */
+for (core = 0; core < NUM_CORES; core++) {
+p->per_core_isr[core] = 0;
+}
+
+/* Update per_core_isr and per_ip_isr */
+for (irq = 0; irq < NUM_IRQS; irq++) {
+if (!(p->isr & (1 << irq))) {
+continue;
+}
+
+for (core = 0; core < NUM_CORES; core++) {
+if ((p->mapper[irq] & (1 << core))) {
+p->per_core_isr[core] |= (1 << irq);
+}
+}
+
+for (ip = 0; ip < NUM_IPS; ip++) {
+if ((p->mapper[irq] & (1 << (ip + 4 {
+per_ip_isr[ip] |= (1 << irq);
+}
+}
+}
+
+/* Emit IRQ to parent! */
+for (core = 0; core < NUM_CORES; core++) {
+for (ip = 0; ip < NUM_IPS; ip++) {
+int parent = PARENT_COREx_IPy(core, ip);
+if (p->parent_state[parent] !=
+(!!p->per_core_isr[core] && !!per_ip_isr[ip])) {
+p->parent_state[parent] = !p->parent_state[parent];
+qemu_set_irq(p->parent_irq[parent], p->parent_state[parent]);
+}
+}
+}
+}
+
+static uint64_t
+liointc_read(void *opaque, hwaddr addr, unsigned int size)
+{
+struct loongson_liointc *p = opaque;
+uint32_t r = 0;
+
+/* Mapper is 1 byte */
+if (size == 1 && addr < R_MAPPER_END) {
+r = p->mapper[addr];
+goto out;
+}
+
+/* Rest is 4 byte */
+if (size != 4 || (addr % 4)) {
+got