[PATCH] hw/display/macfb: Classify the "nubus-macfb" as display device

[Thomas Huth]

The "nubus-macfb" currently shows up as uncategorized device in
the output of "-device help". Put it into the display category
to fix this ugliness.

Signed-off-by: Thomas Huth 
---
 hw/display/macfb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index ff8bdb846b..d8183b9bbd 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -450,6 +450,7 @@ static void macfb_nubus_class_init(ObjectClass *klass, void 
*data)
 dc->desc = "Nubus Macintosh framebuffer";
 dc->reset = macfb_nubus_reset;
 dc->vmsd = &vmstate_macfb;
+set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 device_class_set_props(dc, macfb_nubus_properties);
 }
 
-- 
2.27.0

Re: [PULL 00/44] Python patches

[Philippe Mathieu-Daudé]

On 5/30/21 9:22 PM, John Snow wrote:
> On 5/30/21 3:09 PM, Peter Maydell wrote:
>> Fails to build on my machine that runs the BSD VMs, apparently
>> before it gets to the point of launching the VM:
>>
>> make: Entering directory '/home/peter.maydell/qemu-freebsd/build'
>> /usr/bin/python3 -B /home/peter.maydell/qemu-freebsd/meson/meson.py
>> introspect --targets --tests --benchmarks | /usr/bin/python3 -B
>> scripts/mtest2make.py > Makefile.mtest
>> { \
>>    echo 'ninja-targets = \'; \
>>    /usr/bin/ninja -t targets all | sed 's/:.*//; $!s/$/ \\/'; \
>>    echo 'build-files = \'; \
>>    /usr/bin/ninja -t query build.ninja | sed -n '1,/^  input:/d; /^
>> outputs:/q; s/$/ \\/p'; \
>> } > Makefile.ninja.tmp && mv Makefile.ninja.tmp Makefile.ninja
>> (GIT="git" "/home/peter.maydell/qemu-freebsd/scripts/git-submodule.sh"
>> update ui/keycodemapdb meson tests/fp/berkeley-testfloat-3
>> tests/fp/berkeley-softfloat-3 dtc capstone slirp)
>> (GIT="git" "/home/peter.maydell/qemu-freebsd/scripts/git-submodule.sh"
>> update ui/keycodemapdb meson tests/fp/berkeley-testfloat-3
>> tests/fp/berkeley-softfloat-3 dtc capstone slirp)
>> /usr/bin/python3 -B /home/peter.maydell/qemu-freebsd/tests/vm/freebsd
>> --debug --genisoimage /usr/bin/genisoimage    --image
>> "/home/peter.maydell/.cache/qemu-vm/images/freebsd.img" --force
>> --build-image /home/peter.maydell/.cache/qemu-vm/images/freebsd.img
>> Traceback (most recent call last):
>>    File "/home/peter.maydell/qemu-freebsd/tests/vm/freebsd", line 21,
>> in 
>>  import basevm
>>    File "/home/peter.maydell/qemu-freebsd/tests/vm/basevm.py", line 22,
>> in 
>>  from qemu.machine import QEMUMachine
>> ImportError: bad magic number in 'qemu': b'\x03\xf3\r\n'
>> /home/peter.maydell/qemu-freebsd/tests/vm/Makefile.include:79: recipe
>> for target '/home/peter.maydell/.cache/qemu-vm/images/freebsd.img'
>> failed
>> make: *** [/home/peter.maydell/.cache/qemu-vm/images/freebsd.img] Error 1
>> make: Leaving directory '/home/peter.maydell/qemu-freebsd/build'

> In case that doesn't fix it, can you tell me what version of Python your
> BSD setup uses? (And what version of FreeBSD you use?) -- I'll try to
> set up a VM and see if I can reproduce the problem.

IIUC correctly this is on the host (likely Linux); no BSD VM could be
launched yet.

Re: [PATCH] hw/display/macfb: Classify the "nubus-macfb" as display device

[Laurent Vivier]

Le 31/05/2021 à 09:32, Thomas Huth a écrit :
> The "nubus-macfb" currently shows up as uncategorized device in
> the output of "-device help". Put it into the display category
> to fix this ugliness.
> 
> Signed-off-by: Thomas Huth 
> ---
>  hw/display/macfb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index ff8bdb846b..d8183b9bbd 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -450,6 +450,7 @@ static void macfb_nubus_class_init(ObjectClass *klass, 
> void *data)
>  dc->desc = "Nubus Macintosh framebuffer";
>  dc->reset = macfb_nubus_reset;
>  dc->vmsd = &vmstate_macfb;
> +set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>  device_class_set_props(dc, macfb_nubus_properties);
>  }
>  
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH v2 1/2] hw/char: sifive_uart

[Philippe Mathieu-Daudé]

On 5/30/21 12:49 PM, Lukas Jünger wrote:
> Make function names consistent
> 
> Signed-off-by: Lukas Jünger 
> ---
>  hw/char/sifive_uart.c | 44 +--
>  1 file changed, 22 insertions(+), 22 deletions(-)

> @@ -183,9 +183,9 @@ SiFiveUARTState *sifive_uart_create(MemoryRegion 
> *address_space, hwaddr base,
>  SiFiveUARTState *s = g_malloc0(sizeof(SiFiveUARTState));
>  s->irq = irq;
>  qemu_chr_fe_init(&s->chr, chr, &error_abort);
> -qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx, uart_event,
> -uart_be_change, s, NULL, true);
> -memory_region_init_io(&s->mmio, NULL, &uart_ops, s,
> +qemu_chr_fe_set_handlers(&s->chr, sifive_uart_can_rx, sifive_uart_rx,
> +sifive_uart_event, sifive_uart_be_change, s, NULL, true);

Incorrect indentation, otherwise:
Reviewed-by: Philippe Mathieu-Daudé 

> +memory_region_init_io(&s->mmio, NULL, &sifive_uart_ops, s,
>TYPE_SIFIVE_UART, SIFIVE_UART_MAX);
>  memory_region_add_subregion(address_space, base, &s->mmio);
>  return s;
> 

Re: [PATCH] replay: improve determinism of virtio-net

[Pavel Dovgalyuk]

On 31.05.2021 09:39, Jason Wang wrote:


在 2021/5/31 下午2:35, Pavel Dovgalyuk 写道:

On 31.05.2021 07:55, Jason Wang wrote:


在 2021/5/17 下午9:04, Pavel Dovgalyuk 写道:

virtio-net device uses bottom halves for callbacks.
These callbacks should be deterministic, because they affect VM state.
This patch replaces BH invocations with corresponding replay functions,
making them deterministic in record/replay mode.
This patch also disables guest announce timers for record/replay,
because they break correct loadvm in deterministic mode.



Virtio-net can be configured to work in tx timer mode. Do we need to 
care about that?


What does it mean? This patch fixes interaction with TX timer. Is it 
related to that mode?



I meant is the timer used by virtio_net_handle_tx_timer() safe consider 
you disable announce timer.


I'm not sure that tx_timer is ok. It uses virtual time, but is not saved 
in vmstate.




Thanks








Signed-off-by: Pavel Dovgalyuk 
---
  hw/net/virtio-net.c |   13 +
  1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6b7e8dd04e..e876363236 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -44,6 +44,7 @@
  #include "hw/pci/pci.h"
  #include "net_rx_pkt.h"
  #include "hw/virtio/vhost.h"
+#include "sysemu/replay.h"
  #define VIRTIO_NET_VM_VERSION    11
@@ -394,7 +395,7 @@ static void virtio_net_set_status(struct 
VirtIODevice *vdev, uint8_t status)

  timer_mod(q->tx_timer,
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
  } else {
-    qemu_bh_schedule(q->tx_bh);
+    replay_bh_schedule_event(q->tx_bh);
  }
  } else {
  if (q->tx_timer) {
@@ -2546,7 +2547,7 @@ static void 
virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)

  return;
  }
  virtio_queue_set_notification(vq, 0);
-    qemu_bh_schedule(q->tx_bh);
+    replay_bh_schedule_event(q->tx_bh);



Not familiar with replay but any chance to change qemu_bh_schedule() 
instead?


It would be better, but sometimes qemu_bh_schedule is used for the 
callbacks that are not related to the guest state change.




Thanks



  }
  static void virtio_net_tx_timer(void *opaque)
@@ -2602,7 +2603,7 @@ static void virtio_net_tx_bh(void *opaque)
  /* If we flush a full burst of packets, assume there are
   * more coming and immediately reschedule */
  if (ret >= n->tx_burst) {
-    qemu_bh_schedule(q->tx_bh);
+    replay_bh_schedule_event(q->tx_bh);
  q->tx_waiting = 1;
  return;
  }
@@ -2616,7 +2617,7 @@ static void virtio_net_tx_bh(void *opaque)
  return;
  } else if (ret > 0) {
  virtio_queue_set_notification(q->tx_vq, 0);
-    qemu_bh_schedule(q->tx_bh);
+    replay_bh_schedule_event(q->tx_bh);
  q->tx_waiting = 1;
  }
  }
@@ -3206,6 +3207,10 @@ static void 
virtio_net_device_realize(DeviceState *dev, Error **errp)

  n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
  }
+    if (replay_mode != REPLAY_MODE_NONE) {
+    n->host_features &= ~(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE);
+    }
+
  if (n->net_conf.duplex_str) {
  if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
  n->net_conf.duplex = DUPLEX_HALF;

Re: [PATCH] target/mips: Fix DBALIGN DSP-R2 opcode 'byte position' field size

[Philippe Mathieu-Daudé]

On 5/30/21 5:33 PM, Richard Henderson wrote:
> On 5/29/21 6:05 AM, Philippe Mathieu-Daudé wrote:
>> Per the "MIPS® DSP Module for MIPS64 Architecture" manual (rev 3.02),
>> Figure 5.12 "SPECIAL3 Encoding of APPEND/DAPPEND Instruction Sub-class"
>> the byte position field ('bp') is 2 bits, not 3.
> 
> Rev 2.34 has 3 bits, not 2.
> 
> The mips32 version of balign, that uses 2 bits...  Are you sure you
> looked at the right instruction?  Because 3 bits makes most sense for
> this instruction with a 64-bit register size.

Yes indeed it makes sense, and Rev 3.02 is incomplete...

Thanks,

Phil.

Re: [PATCH] hw/display/macfb: Classify the "nubus-macfb" as display device

[Philippe Mathieu-Daudé]

On 5/31/21 9:32 AM, Thomas Huth wrote:
> The "nubus-macfb" currently shows up as uncategorized device in
> the output of "-device help". Put it into the display category
> to fix this ugliness.
> 
> Signed-off-by: Thomas Huth 
> ---
>  hw/display/macfb.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [RFC v3 15/29] vhost: Add enable_custom_iommu to VhostOps

[Jason Wang]

在 2021/5/20 上午12:28, Eugenio Pérez 写道:

This operation enable the backend-specific IOTLB entries.

If a backend support this, it start managing its own entries, and vhost
can disable it through this operation and recover control.

Every enable/disable operation must also clear all IOTLB device entries.

At the moment, the only backend that does so is vhost-vdpa. To fully
support these, vdpa needs also to expose a way for vhost subsystem to
map and unmap entries. This will be done in future commits.

Signed-off-by: Eugenio Pérez 



I think there's probably no need to introduce this helper.

Instead, we can introduce ops like shadow_vq_start()/stop(). Then the 
details like this could be hided there.


(And hide the backend deatils (avoid calling vhost_vdpa_dma_map()) 
directly from the vhost.c)


Thanks



---
  include/hw/virtio/vhost-backend.h | 4 
  1 file changed, 4 insertions(+)

diff --git a/include/hw/virtio/vhost-backend.h 
b/include/hw/virtio/vhost-backend.h
index bcb112c166..f8eed2ace5 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -128,6 +128,9 @@ typedef bool (*vhost_force_iommu_op)(struct vhost_dev *dev);
  
  typedef int (*vhost_vring_pause_op)(struct vhost_dev *dev);
  
+typedef int (*vhost_enable_custom_iommu_op)(struct vhost_dev *dev,

+bool enable);
+
  typedef int (*vhost_get_iova_range)(struct vhost_dev *dev,
  hwaddr *first, hwaddr *last);
  
@@ -177,6 +180,7 @@ typedef struct VhostOps {

  vhost_get_device_id_op vhost_get_device_id;
  vhost_vring_pause_op vhost_vring_pause;
  vhost_force_iommu_op vhost_force_iommu;
+vhost_enable_custom_iommu_op vhost_enable_custom_iommu;
  vhost_get_iova_range vhost_get_iova_range;
  } VhostOps;
  

Re: [PATCH] qobject: Fix maybe uninitialized in qdict_array_split

[Janosch Frank]

On 5/18/21 3:06 PM, Janosch Frank wrote:
> Lets make the compiler happy.
> 
> Signed-off-by: Janosch Frank 

Ping
My build is still breaking on Ubuntu because of this.

> ---
>  qobject/block-qdict.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> index 1487cc5dd8..b26524429c 100644
> --- a/qobject/block-qdict.c
> +++ b/qobject/block-qdict.c
> @@ -224,7 +224,7 @@ void qdict_array_split(QDict *src, QList **dst)
>  for (i = 0; i < UINT_MAX; i++) {
>  QObject *subqobj;
>  bool is_subqdict;
> -QDict *subqdict;
> +QDict *subqdict = NULL;
>  char indexstr[32], prefix[32];
>  size_t snprintf_ret;
>  
> 

[PATCH] target/nios2: Mark raise_exception() as noreturn

[Philippe Mathieu-Daudé]

Raised exceptions don't return, so mark the helper with
TCG_CALL_NO_RETURN.

Fixes: 032c76bc6f9 ("nios2: Add architecture emulation support")
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/nios2/helper.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/nios2/helper.h b/target/nios2/helper.h
index b0cb9146a5f..9bf14c128ed 100644
--- a/target/nios2/helper.h
+++ b/target/nios2/helper.h
@@ -18,7 +18,7 @@
  * 
  */
 
-DEF_HELPER_2(raise_exception, void, env, i32)
+DEF_HELPER_FLAGS_2(raise_exception, TCG_CALL_NO_RETURN, noreturn, env, i32)
 
 #if !defined(CONFIG_USER_ONLY)
 DEF_HELPER_2(mmu_read_debug, void, env, i32)
-- 
2.26.3

Re: [RFC v3 21/29] vhost: Add VhostIOVATree

[Jason Wang]

在 2021/5/20 上午12:28, Eugenio Pérez 写道:

This tree is able to look for a translated address from a IOVA address.

At first glance is similar to util/iova-tree. However, SVQ working on
devices with limited IOVA space need more capabilities, like allocating
IOVA chunks or perform reverse translations (qemu addresses to iova).

Starting a sepparated implementation. Knowing than insertions/deletions
will not be as frequent as searches,



This might not be true if vIOMMU is enabled.



it uses an ordered array at
implementation.



I wonder how much overhead could g_array be if it needs to grow.



  A different name could be used, but ordered
searchable array is a little bit long though.



Note that we had a very good example for this. That is the kernel iova 
allocator which is implemented via rbtree.


Instead of figuring out g_array vs g_tree stuffs, I would simple go with 
g_tree first (based on util/iova-tree) and borrow the well design kernel 
iova allocator API to have a generic IOVA one instead of coupling it 
with vhost. It could be used by other userspace driver in the future:


init_iova_domain()/put_iova_domain();

alloc_iova()/free_iova();

find_iova();

Another reference is the iova allocator that is implemented in VFIO.

Thanks




Signed-off-by: Eugenio Pérez 
---
  hw/virtio/vhost-iova-tree.h |  50 ++
  hw/virtio/vhost-iova-tree.c | 188 
  hw/virtio/meson.build   |   2 +-
  3 files changed, 239 insertions(+), 1 deletion(-)
  create mode 100644 hw/virtio/vhost-iova-tree.h
  create mode 100644 hw/virtio/vhost-iova-tree.c

diff --git a/hw/virtio/vhost-iova-tree.h b/hw/virtio/vhost-iova-tree.h
new file mode 100644
index 00..2a44af8b3a
--- /dev/null
+++ b/hw/virtio/vhost-iova-tree.h
@@ -0,0 +1,50 @@
+/*
+ * vhost software live migration ring
+ *
+ * SPDX-FileCopyrightText: Red Hat, Inc. 2021
+ * SPDX-FileContributor: Author: Eugenio Pérez 
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_VIRTIO_VHOST_IOVA_TREE_H
+#define HW_VIRTIO_VHOST_IOVA_TREE_H
+
+#include 
+
+#include "exec/memory.h"
+
+typedef struct VhostDMAMap {
+void *translated_addr;
+hwaddr iova;
+hwaddr size;/* Inclusive */
+IOMMUAccessFlags perm;
+} VhostDMAMap;
+
+typedef enum VhostDMAMapNewRC {
+VHOST_DMA_MAP_OVERLAP = -2,
+VHOST_DMA_MAP_INVALID = -1,
+VHOST_DMA_MAP_OK = 0,
+} VhostDMAMapNewRC;
+
+/**
+ * VhostIOVATree
+ *
+ * Store and search IOVA -> Translated mappings.
+ *
+ * Note that it cannot remove nodes.
+ */
+typedef struct VhostIOVATree {
+/* Ordered array of reverse translations, IOVA address to qemu memory. */
+GArray *iova_taddr_map;
+} VhostIOVATree;
+
+void vhost_iova_tree_new(VhostIOVATree *iova_rm);
+void vhost_iova_tree_destroy(VhostIOVATree *iova_rm);
+
+const VhostDMAMap *vhost_iova_tree_find_taddr(const VhostIOVATree *iova_rm,
+  const VhostDMAMap *map);
+VhostDMAMapNewRC vhost_iova_tree_insert(VhostIOVATree *iova_rm,
+VhostDMAMap *map);
+
+#endif
diff --git a/hw/virtio/vhost-iova-tree.c b/hw/virtio/vhost-iova-tree.c
new file mode 100644
index 00..dfd7e448b5
--- /dev/null
+++ b/hw/virtio/vhost-iova-tree.c
@@ -0,0 +1,188 @@
+/*
+ * vhost software live migration ring
+ *
+ * SPDX-FileCopyrightText: Red Hat, Inc. 2021
+ * SPDX-FileContributor: Author: Eugenio Pérez 
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "vhost-iova-tree.h"
+
+#define G_ARRAY_NOT_ZERO_TERMINATED false
+#define G_ARRAY_NOT_CLEAR_ON_ALLOC false
+
+/**
+ * Inserts an element after an existing one in garray.
+ *
+ * @array  The array
+ * @prev_elem  The previous element of array of NULL if prepending
+ * @mapThe DMA map
+ *
+ * It provides the aditional advantage of being type safe over
+ * g_array_insert_val, which accepts a reference pointer instead of a value
+ * with no complains.
+ */
+static void vhost_iova_tree_insert_after(GArray *array,
+ const VhostDMAMap *prev_elem,
+ const VhostDMAMap *map)
+{
+size_t pos;
+
+if (!prev_elem) {
+pos = 0;
+} else {
+pos = prev_elem - &g_array_index(array, typeof(*prev_elem), 0) + 1;
+}
+
+g_array_insert_val(array, pos, *map);
+}
+
+static gint vhost_iova_tree_cmp_iova(gconstpointer a, gconstpointer b)
+{
+const VhostDMAMap *m1 = a, *m2 = b;
+
+if (m1->iova > m2->iova + m2->size) {
+return 1;
+}
+
+if (m1->iova + m1->size < m2->iova) {
+return -1;
+}
+
+/* Overlapped */
+return 0;
+}
+
+/**
+ * Find the previous node to a given iova
+ *
+ * @array  The ascending ordered-by-translated-addr array of VhostDMAMap
+ * @mapThe map to insert
+ * @prev   Returned location of the previous map
+ *
+ * Return VHOST_DMA_MAP_OK if everything went well, or VHOST_DMA_MAP_OVERLAP i

Re: [PATCH] qobject: Fix maybe uninitialized in qdict_array_split

[Philippe Mathieu-Daudé]

On 5/31/21 11:21 AM, Janosch Frank wrote:
> On 5/18/21 3:06 PM, Janosch Frank wrote:
>> Lets make the compiler happy.
>>
>> Signed-off-by: Janosch Frank 
> 
> Ping
> My build is still breaking on Ubuntu because of this.
> 
>> ---
>>  qobject/block-qdict.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
>> index 1487cc5dd8..b26524429c 100644
>> --- a/qobject/block-qdict.c
>> +++ b/qobject/block-qdict.c
>> @@ -224,7 +224,7 @@ void qdict_array_split(QDict *src, QList **dst)
>>  for (i = 0; i < UINT_MAX; i++) {
>>  QObject *subqobj;
>>  bool is_subqdict;
>> -QDict *subqdict;
>> +QDict *subqdict = NULL;
>>  char indexstr[32], prefix[32];
>>  size_t snprintf_ret;

Slightly clearer:

-- >8 --
diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index 1487cc5dd8b..8d0f00bc3ce 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -224,7 +224,6 @@ void qdict_array_split(QDict *src, QList **dst)
 for (i = 0; i < UINT_MAX; i++) {
 QObject *subqobj;
 bool is_subqdict;
-QDict *subqdict;
 char indexstr[32], prefix[32];
 size_t snprintf_ret;

@@ -249,14 +248,16 @@ void qdict_array_split(QDict *src, QList **dst)
 }

 if (is_subqdict) {
+QDict *subqdict = NULL;
+
 qdict_extract_subqdict(src, &subqdict, prefix);
 assert(qdict_size(subqdict) > 0);
+qlist_append_obj(*dst, QOBJECT(subqdict));
 } else {
 qobject_ref(subqobj);
 qdict_del(src, indexstr);
+qlist_append_obj(*dst, subqobj);
 }
-
-qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
 }
 }
---

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé 

Cc'ing qemu-trivial@

[PATCH] tests/vm/freebsd: Increase code coverage

[Philippe Mathieu-Daudé]

Install more dependencies to increase code coverage.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/vm/freebsd | 5 +
 1 file changed, 5 insertions(+)

diff --git a/tests/vm/freebsd b/tests/vm/freebsd
index 6e20e843228..85049b43136 100755
--- a/tests/vm/freebsd
+++ b/tests/vm/freebsd
@@ -46,7 +46,9 @@ class FreeBSDVM(basevm.BaseVM):
 "gettext",
 
 # libs: crypto
+"cyrus-sasl",
 "gnutls",
+"nettle",
 
 # libs: images
 "jpeg-turbo",
@@ -56,6 +58,7 @@ class FreeBSDVM(basevm.BaseVM):
 "sdl2",
 "gtk3",
 "libxkbcommon",
+"pixman",
 
 # libs: opengl
 "libepoxy",
@@ -63,6 +66,8 @@ class FreeBSDVM(basevm.BaseVM):
 
 # libs: migration
 "zstd",
+
+"usbredir",
 ]
 
 # TODO: Enable gnutls again once FreeBSD's libtasn1 got fixed
-- 
2.26.3

Re: [PATCH] qobject: Fix maybe uninitialized in qdict_array_split

[Janosch Frank]

On 5/31/21 11:44 AM, Philippe Mathieu-Daudé wrote:
> On 5/31/21 11:21 AM, Janosch Frank wrote:
>> On 5/18/21 3:06 PM, Janosch Frank wrote:
>>> Lets make the compiler happy.
>>>
>>> Signed-off-by: Janosch Frank 
>>
>> Ping
>> My build is still breaking on Ubuntu because of this.
>>
>>> ---
>>>  qobject/block-qdict.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
>>> index 1487cc5dd8..b26524429c 100644
>>> --- a/qobject/block-qdict.c
>>> +++ b/qobject/block-qdict.c
>>> @@ -224,7 +224,7 @@ void qdict_array_split(QDict *src, QList **dst)
>>>  for (i = 0; i < UINT_MAX; i++) {
>>>  QObject *subqobj;
>>>  bool is_subqdict;
>>> -QDict *subqdict;
>>> +QDict *subqdict = NULL;
>>>  char indexstr[32], prefix[32];
>>>  size_t snprintf_ret;
> 
> Slightly clearer:>
> -- >8 --
> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> index 1487cc5dd8b..8d0f00bc3ce 100644
> --- a/qobject/block-qdict.c
> +++ b/qobject/block-qdict.c
> @@ -224,7 +224,6 @@ void qdict_array_split(QDict *src, QList **dst)
>  for (i = 0; i < UINT_MAX; i++) {
>  QObject *subqobj;
>  bool is_subqdict;
> -QDict *subqdict;
>  char indexstr[32], prefix[32];
>  size_t snprintf_ret;
> 
> @@ -249,14 +248,16 @@ void qdict_array_split(QDict *src, QList **dst)
>  }
> 
>  if (is_subqdict) {
> +QDict *subqdict = NULL;
> +
>  qdict_extract_subqdict(src, &subqdict, prefix);
>  assert(qdict_size(subqdict) > 0);
> +qlist_append_obj(*dst, QOBJECT(subqdict));
>  } else {
>  qobject_ref(subqobj);
>  qdict_del(src, indexstr);
> +qlist_append_obj(*dst, subqobj);
>  }
> -
> -qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
>  }
>  }
> ---

If you want post that snippet yourself, go ahead.
After all I only fixed a symptom without a closer look into the code.

> 
> Anyhow,
> Reviewed-by: Philippe Mathieu-Daudé 
> 
> Cc'ing qemu-trivial@
> 

If not I'll take it, test on s390 and send a v2 with qemu-trivial and
you in CC.


Thanks for having a look!

[RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)

[Philippe Mathieu-Daudé]

To avoid leaking memory on the error path, reorder the
code as:
- check the parameters first
- check resource already existing
- finally allocate memory

Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
Signed-off-by: Philippe Mathieu-Daudé 
---
RFC because the s->iov check is dubious.
---
 hw/display/virtio-gpu.c | 28 +++-
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 4d549377cbc..8d047007bbb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
 return;
 }
 
-res = virtio_gpu_find_resource(g, cblob.resource_id);
-if (res) {
+if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
+cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
+  __func__);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+return;
+}
+
+if (virtio_gpu_find_resource(g, cblob.resource_id)) {
 qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
   __func__, cblob.resource_id);
 cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
@@ -352,25 +359,12 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
 res->resource_id = cblob.resource_id;
 res->blob_size = cblob.size;
 
-if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
-cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
-  __func__);
-cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-g_free(res);
-return;
-}
-
-if (res->iov) {
-cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-return;
-}
-
 ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
 cmd, &res->addrs, &res->iov,
 &res->iov_cnt);
-if (ret != 0) {
+if (ret != 0 || res->iov) {
 cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+g_free(res);
 return;
 }
 
-- 
2.26.3

Re: [PATCH qemu] Add basic power management to raspi.

[Philippe Mathieu-Daudé]

Hi Nolan,

Thanks for your patch!

There is something odd with your email address, which apparently
became source...@sigbus.net instead of no...@sigbus.net.

On 5/18/21 10:24 PM, ~nolanl wrote:
> From: Nolan Leake 
> 
> This is just enough to make reboot and poweroff work.

Please precise "for Linux kernels", since this doesn't work
with firmwares (ideally we should implement the FIRMWARE_NOTIFY_REBOOT
property - which Linux sends - to handle the machine reboot/halt
via the videocore firmware model).

> Notably, the
> watchdog timer functionality is not yet implemented.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/64
> Signed-off-by: Nolan Leake 
> ---
>  hw/arm/bcm2835_peripherals.c |  13 ++-
>  hw/misc/bcm2835_powermgt.c   | 157 +++
>  hw/misc/meson.build  |   1 +
>  include/hw/arm/bcm2835_peripherals.h |   3 +-
>  include/hw/misc/bcm2835_powermgt.h   |  29 +
>  5 files changed, 201 insertions(+), 2 deletions(-)
>  create mode 100644 hw/misc/bcm2835_powermgt.c
>  create mode 100644 include/hw/misc/bcm2835_powermgt.h
> 
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index dcff13433e..48538c9360 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -126,6 +126,10 @@ static void bcm2835_peripherals_init(Object *obj)
>  
>  object_property_add_const_link(OBJECT(&s->dwc2), "dma-mr",
> OBJECT(&s->gpu_bus_mr));
> +
> +/* Power Management */
> +object_initialize_child(obj, "powermgt", &s->powermgt,
> +TYPE_BCM2835_POWERMGT);
>  }
>  
>  static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
> @@ -364,9 +368,16 @@ static void bcm2835_peripherals_realize(DeviceState 
> *dev, Error **errp)
>  qdev_get_gpio_in_named(DEVICE(&s->ic), BCM2835_IC_GPU_IRQ,
> INTERRUPT_USB));
>  
> +/* Power Management */
> +if (!sysbus_realize(SYS_BUS_DEVICE(&s->powermgt), errp)) {
> +return;
> +}
> +
> +memory_region_add_subregion(&s->peri_mr, PM_OFFSET,
> +sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->powermgt), 0));
> +
>  create_unimp(s, &s->txp, "bcm2835-txp", TXP_OFFSET, 0x1000);
>  create_unimp(s, &s->armtmr, "bcm2835-sp804", ARMCTRL_TIMER0_1_OFFSET, 
> 0x40);
> -create_unimp(s, &s->powermgt, "bcm2835-powermgt", PM_OFFSET, 0x114);
>  create_unimp(s, &s->i2s, "bcm2835-i2s", I2S_OFFSET, 0x100);
>  create_unimp(s, &s->smi, "bcm2835-smi", SMI_OFFSET, 0x100);
>  create_unimp(s, &s->spi[0], "bcm2835-spi0", SPI0_OFFSET, 0x20);
> diff --git a/hw/misc/bcm2835_powermgt.c b/hw/misc/bcm2835_powermgt.c
> new file mode 100644
> index 00..81107ecc8f
> --- /dev/null
> +++ b/hw/misc/bcm2835_powermgt.c
> @@ -0,0 +1,157 @@
> +/*
> + * BCM2835 Power Management emulation
> + *
> + * Copyright (C) 2017 Marcin Chojnacki 
> + * Copyright (C) 2021 Nolan Leake 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "hw/misc/bcm2835_powermgt.h"
> +#include "migration/vmstate.h"
> +#include "sysemu/runstate.h"
> +
> +#define PASSWORD 0x5a00
> +#define PASSWORD_MASK 0xff00
> +
> +#define R_RSTC 0x1c
> +#define V_RSTC_RESET 0x20
> +#define R_RSTS 0x20
> +#define V_RSTS_POWEROFF 0x555
> +#define R_WDOG 0x24
> +
> +static uint64_t bcm2835_powermgt_read(void *opaque, hwaddr offset,
> +  unsigned size)
> +{
> +BCM2835PowerMgtState *s = (BCM2835PowerMgtState *)opaque;
> +uint32_t res = 0;
> +
> +assert(size == 4);

Instead of this assert, add in bcm2835_powermgt_ops:

  .impl.min_access_size = 4,
  .impl.max_access_size = 4,

> +
> +switch (offset) {
> +case R_RSTC:
> +res = s->rstc;
> +break;
> +case R_RSTS:
> +res = s->rsts;
> +break;
> +case R_WDOG:
> +res = s->wdog;
> +break;
> +
> +default:
> +qemu_log_mask(LOG_UNIMP,
> +  "bcm2835_powermgt_read: Unknown offset %x\n",
> +  (int)offset);
> +res = 0;
> +break;
> +}
> +
> +return res;
> +}
> +
> +static void bcm2835_powermgt_write(void *opaque, hwaddr offset,
> +   uint64_t value, unsigned size)
> +{
> +BCM2835PowerMgtState *s = (BCM2835PowerMgtState *)opaque;
> +
> +assert(size == 4);

(remove this assert too).

> +if ((value & PASSWORD_MASK) != PASSWORD) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "bcm2835_powermgt_write: Bad password %x at offset 
> %x\n",

Please prefix hexadecimal formats with 0x,

> +  (uint32_t)value, (int)offset);

and do not cast.

> +return;
> +}
> +
> +value = value & ~PAS

[PATCH] tests: Boot and halt a Linux guest on the Raspberry Pi 2 machine

[Philippe Mathieu-Daudé]

Add a test booting and quickly shutdown a raspi2 machine,
to test the power management model:

   (1/1) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_raspi2_initrd:
  console: [0.00] Booting Linux on physical CPU 0xf00
  console: [0.00] Linux version 4.14.98-v7+ (dom@dom-XPS-13-9370) (gcc 
version 4.9.3 (crosstool-NG crosstool-ng-1.22.0-88-g8460611)) #1200 SMP Tue Feb 
12 20:27:48 GMT 2019
  console: [0.00] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), 
cr=10c5387d
  console: [0.00] CPU: div instructions available: patching division 
code
  console: [0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT 
aliasing instruction cache
  console: [0.00] OF: fdt: Machine model: Raspberry Pi 2 Model B
  ...
  console: Boot successful.
  console: cat /proc/cpuinfo
  console: / # cat /proc/cpuinfo
  ...
  console: processor  : 3
  console: model name : ARMv7 Processor rev 5 (v7l)
  console: BogoMIPS   : 125.00
  console: Features   : half thumb fastmult vfp edsp neon vfpv3 tls vfpv4 
idiva idivt vfpd32 lpae evtstrm
  console: CPU implementer: 0x41
  console: CPU architecture: 7
  console: CPU variant: 0x0
  console: CPU part   : 0xc07
  console: CPU revision   : 5
  console: Hardware   : BCM2835
  console: Revision   : 
  console: Serial : 
  console: cat /proc/iomem
  console: / # cat /proc/iomem
  console: -3bff : System RAM
  console: 8000-00af : Kernel code
  console: 00c0-00d468ef : Kernel data
  console: 3f006000-3f006fff : dwc_otg
  console: 3f007000-3f007eff : /soc/dma@7e007000
  console: 3f00b880-3f00b8bf : /soc/mailbox@7e00b880
  console: 3f10-3f100027 : /soc/watchdog@7e10
  console: 3f101000-3f102fff : /soc/cprman@7e101000
  console: 3f20-3f2000b3 : /soc/gpio@7e20
  PASS (24.59 s)
  RESULTS: PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
CANCEL 0
  JOB TIME   : 25.02 s

Signed-off-by: Philippe Mathieu-Daudé 
---
Based-on: <162137039825.30884.244582579824080919...@git.sr.ht>
---
 tests/acceptance/boot_linux_console.py | 43 ++
 1 file changed, 43 insertions(+)

diff --git a/tests/acceptance/boot_linux_console.py 
b/tests/acceptance/boot_linux_console.py
index 276a53f1464..19d328203c7 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -16,6 +16,7 @@
 from avocado import skip
 from avocado import skipUnless
 from avocado_qemu import Test
+from avocado_qemu import exec_command
 from avocado_qemu import exec_command_and_wait_for_pattern
 from avocado_qemu import interrupt_interactive_console_until_pattern
 from avocado_qemu import wait_for_console_pattern
@@ -467,6 +468,48 @@ def test_arm_raspi2_uart0(self):
 """
 self.do_test_arm_raspi2(0)
 
+def test_arm_raspi2_initrd(self):
+"""
+:avocado: tags=arch:arm
+:avocado: tags=machine:raspi2
+"""
+deb_url = ('http://archive.raspberrypi.org/debian/'
+   'pool/main/r/raspberrypi-firmware/'
+   'raspberrypi-kernel_1.20190215-1_armhf.deb')
+deb_hash = 'cd284220b32128c5084037553db3c482426f3972'
+deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash)
+kernel_path = self.extract_from_deb(deb_path, '/boot/kernel7.img')
+dtb_path = self.extract_from_deb(deb_path, '/boot/bcm2709-rpi-2-b.dtb')
+
+initrd_url = ('https://github.com/groeck/linux-build-test/raw/'
+  '2eb0a73b5d5a28df3170c546ddaaa9757e1e0848/rootfs/'
+  'arm/rootfs-armv7a.cpio.gz')
+initrd_hash = '604b2e45cdf35045846b8bbfbf2129b1891bdc9c'
+initrd_path_gz = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
+initrd_path = os.path.join(self.workdir, 'rootfs.cpio')
+archive.gzip_uncompress(initrd_path_gz, initrd_path)
+
+self.vm.set_console()
+kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+   'earlycon=pl011,0x3f201000 console=ttyAMA0 '
+   'panic=-1 noreboot ' +
+   'dwc_otg.fiq_fsm_enable=0')
+self.vm.add_args('-kernel', kernel_path,
+ '-dtb', dtb_path,
+ '-initrd', initrd_path,
+ '-append', kernel_command_line,
+ '-no-reboot')
+self.vm.launch()
+self.wait_for_console_pattern('Boot successful.')
+
+exec_command_and_wait_for_pattern(self, 'cat /proc/cpuinfo',
+'BCM2835')
+exec_command_and_wait_for_pattern(self, 'cat /proc/iomem',
+'/soc/cprman@7e101000')
+exec_command(self, 'halt')
+# Wait for VM to shut down gracefully
+self.vm.wait()
+
 def test_arm_exynos4210_initrd(self):
 

Re: [PATCH v4 0/8] IOMMU: Add support for IOMMU Bypass Feature

[Xingang Wang]

Hi everyone,

Do you have any suggestions for this series of patches.

The modifications affects many modules, including new command line,
PCI modules and ACPI firmware tables.
There's modification in IORT DMAR and IVRS, both for arm and x86
platform.

I am not sure whether the modification is appropriate in all modules,
do you have any suggestions?

Thanks

Xingang

.

On 2021/5/25 11:49, Wang Xingang wrote:

From: Xingang Wang 

These patches add support for configure bypass_iommu on/off for
pci root bus, including primary bus and pxb root bus. At present,
all root bus will go through iommu when iommu is configured,
which is not flexible, because in many situations the need for using
iommu and bypass iommu aften exists at the same time.

So this add option to enable/disable bypass_iommu for primary bus
and pxb root bus. The bypass_iommu property is set to false default,
meaning that devcies will go through iommu if no explicit configuration
is added. When bypass_iommu is enabled for the root bus, devices
attached to it will bypass iommu, otherwise devices will go through
iommu.

This feature can be used in this manner:
arm: -machine virt,iommu=smmuv3,bypass_iommu=true
x86: -machine q35,bypass_iommu=true
pxb: -device pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,bypass_iommu=true

History:

v3 -> v4:
- simplify the logic in building the IORT idmap

v2 -> v3:
- rebase on top of v6.0.0-rc4
- Took into account Eric's comments, replace with a bypass_iommu
   proerty
- When building the IORT idmap, cover the whole RID space

v1 -> v2:
- rebase on top of v6.0.0-rc0
- Fix some issues
- Took into account Eric's comments, and remove the PCI_BUS_IOMMU flag,
   replace it with a property in PCIHostState.
- Add support for x86 iommu option

Xingang Wang (8):
   hw/pci/pci_host: Allow bypass iommu for pci host
   hw/pxb: Add a bypass iommu property
   hw/arm/virt: Add a machine option to bypass iommu for primary bus
   hw/i386: Add a pc machine option to bypass iommu for primary bus
   hw/pci: Add pci_bus_range to get bus number range
   hw/arm/virt-acpi-build: Add explicit IORT idmap for smmuv3 node
   hw/i386/acpi-build: Add explicit scope in DMAR table
   hw/i386/acpi-build: Add bypass_iommu check when building IVRS table

  hw/arm/virt-acpi-build.c| 135 
  hw/arm/virt.c   |  26 ++
  hw/i386/acpi-build.c|  70 ++-
  hw/i386/pc.c|  18 
  hw/pci-bridge/pci_expander_bridge.c |   3 +
  hw/pci-host/q35.c   |   1 +
  hw/pci/pci.c|  33 ++-
  hw/pci/pci_host.c   |   2 +
  include/hw/arm/virt.h   |   1 +
  include/hw/i386/pc.h|   1 +
  include/hw/pci/pci.h|   2 +
  include/hw/pci/pci_host.h   |   1 +
  12 files changed, 270 insertions(+), 23 deletions(-)

Re: [PATCH v7 1/1] qapi: introduce 'query-cpu-model-cpuid' action

[Valeriy Vdovin]

On Wed, May 26, 2021 at 05:44:24PM -0400, Eduardo Habkost wrote:
> On Tue, May 04, 2021 at 03:26:39PM +0300, Valeriy Vdovin wrote:
> > Introducing new qapi method 'query-cpu-model-cpuid'. This method can be 
> > used to
> > get virtualized cpu model info generated by QEMU during VM initialization in
> > the form of cpuid representation.
> > 
> > Diving into more details about virtual cpu generation: QEMU first parses 
> > '-cpu'
> > command line option. From there it takes the name of the model as the basis 
> > for
> > feature set of the new virtual cpu. After that it uses trailing '-cpu' 
> > options,
> > that state if additional cpu features should be present on the virtual cpu 
> > or
> > excluded from it (tokens '+'/'-' or '=on'/'=off').
> > After that QEMU checks if the host's cpu can actually support the derived
> > feature set and applies host limitations to it.
> > After this initialization procedure, virtual cpu has it's model and
> > vendor names, and a working feature set and is ready for identification
> > instructions such as CPUID.
> > 
> > Currently full output for this method is only supported for x86 cpus.
> > 
> > To learn exactly how virtual cpu is presented to the guest machine via CPUID
> > instruction, new qapi method can be used. By calling 'query-cpu-model-cpuid'
> > method, one can get a full listing of all CPUID leafs with subleafs which 
> > are
> > supported by the initialized virtual cpu.
> > 
> > Other than debug, the method is useful in cases when we would like to
> > utilize QEMU's virtual cpu initialization routines and put the retrieved
> > values into kernel CPUID overriding mechanics for more precise control
> > over how various processes perceive its underlying hardware with
> > container processes as a good example.
> > 
> > Output format:
> > The output is a plain list of leaf/subleaf agrument combinations, that
> > return 4 words in registers EAX, EBX, ECX, EDX.
> > 
> > Use example:
> > qmp_request: {
> >   "execute": "query-cpu-model-cpuid"
> > }
> > 
> > qmp_response: [
> >   {
> > "eax": 1073741825,
> > "edx": 77,
> > "leaf": 1073741824,
> > "ecx": 1447775574,
> > "ebx": 1263359563,
> > "subleaf": 0
> >   },
> >   {
> > "eax": 16777339,
> > "edx": 0,
> > "leaf": 1073741825,
> > "ecx": 0,
> > "ebx": 0,
> > "subleaf": 0
> >   },
> >   {
> > "eax": 13,
> > "edx": 1231384169,
> > "leaf": 0,
> > "ecx": 1818588270,
> > "ebx": 1970169159,
> > "subleaf": 0
> >   },
> >   {
> > "eax": 198354,
> > "edx": 126614527,
> >   
> > 
> > Signed-off-by: Valeriy Vdovin 
> 
> This breaks --disable-kvm builds (see below[1]), but I like the
> simplicity of this solution.
> 
> I think it will be an acceptable and welcome mechanism if we name
> and document it as KVM-specific.
> 
> A debugging command like this that returns the raw CPUID data
> directly from the KVM tables would be very useful for automated
> testing of our KVM CPUID initialization code.  We have some test
> cases for CPU configuration code, but they trust what the CPU
> objects tell us and won't catch mistakes in target/i386/kvm.c
> CPUID code.
> 
> [1] Build error when using --disable-kvm:
> 
>   [449/821] Linking target qemu-system-x86_64
>   FAILED: qemu-system-x86_64
>   c++  -o qemu-system-x86_64 qemu-system-x86_64.p/softmmu_main.c.o 
> libcommon.fa.p/hw_char_virtio-console.c.o [...]
>   /usr/bin/ld: 
> libqemu-x86_64-softmmu.fa.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o:
>  in function `qmp_marshal_query_cpu_model_cpuid':
>   
> /home/ehabkost/rh/proj/virt/qemu/build/qapi/qapi-commands-machine-target.c:278:
>  undefined reference to `qmp_query_cpu_model_cpuid'
>   collect2: error: ld returned 1 exit status
> 
> 
I'll add if defined(CONFIG_KVM) to this qmp method description.
> > 
> > ---
> > v2: - Removed leaf/subleaf iterators.
> > - Modified cpu_x86_cpuid to return false in cases when count is
> >   greater than supported subleaves.
> > v3: - Fixed structure name coding style.
> > - Added more comments
> > - Ensured buildability for non-x86 targets.
> > v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
> > - Fixed comments.
> > - Removed target check in qmp_query_cpu_model_cpuid.
> > v5: - Added error handling code in qmp_query_cpu_model_cpuid
> > v6: - Fixed error handling code. Added method to query_error_class
> > v7: - Changed implementation in favor of cached cpuid_data for
> > KVM_SET_CPUID2
> >  qapi/machine-target.json   | 51 ++
> >  target/i386/kvm/kvm.c  | 45 ++---
> >  tests/qtest/qmp-cmd-test.c |  1 +
> >  3 files changed, 93 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index e7811654b7..ad816a50b6 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json
> > @@ -329,3 +329,54 @@
> >  ##
> >  { 'command': 'qu

[PATCH v8] qapi: introduce 'query-kvm-cpuid' action

[Valeriy Vdovin]

Introducing new qapi method 'query-kvm-cpuid'. This method can be used to
get virtualized cpu model info generated by QEMU during VM initialization in
the form of cpuid representation.

Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
command line option. From there it takes the name of the model as the basis for
feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
that state if additional cpu features should be present on the virtual cpu or
excluded from it (tokens '+'/'-' or '=on'/'=off').
After that QEMU checks if the host's cpu can actually support the derived
feature set and applies host limitations to it.
After this initialization procedure, virtual cpu has it's model and
vendor names, and a working feature set and is ready for identification
instructions such as CPUID.

Currently full output for this method is only supported for x86 cpus.

To learn exactly how virtual cpu is presented to the guest machine via CPUID
instruction, new qapi method can be used. By calling 'query-kvm-cpuid'
method, one can get a full listing of all CPUID leafs with subleafs which are
supported by the initialized virtual cpu.

Other than debug, the method is useful in cases when we would like to
utilize QEMU's virtual cpu initialization routines and put the retrieved
values into kernel CPUID overriding mechanics for more precise control
over how various processes perceive its underlying hardware with
container processes as a good example.

Output format:
The output is a plain list of leaf/subleaf agrument combinations, that
return 4 words in registers EAX, EBX, ECX, EDX.

Use example:
qmp_request: {
  "execute": "query-kvm-cpuid"
}

qmp_response: [
  {
"eax": 1073741825,
"edx": 77,
"in_eax": 1073741824,
"ecx": 1447775574,
"ebx": 1263359563,
  },
  {
"eax": 16777339,
"edx": 0,
"in_eax": 1073741825,
"ecx": 0,
"ebx": 0,
  },
  {
"eax": 13,
"edx": 1231384169,
"in_eax": 0,
"ecx": 1818588270,
"ebx": 1970169159,
  },
  {
"eax": 198354,
"edx": 126614527,
  

Signed-off-by: Valeriy Vdovin 

v2: - Removed leaf/subleaf iterators.
- Modified cpu_x86_cpuid to return false in cases when count is
  greater than supported subleaves.
v3: - Fixed structure name coding style.
- Added more comments
- Ensured buildability for non-x86 targets.
v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
- Fixed comments.
- Removed target check in qmp_query_cpu_model_cpuid.
v5: - Added error handling code in qmp_query_cpu_model_cpuid
v6: - Fixed error handling code. Added method to query_error_class
v7: - Changed implementation in favor of cached cpuid_data for
  KVM_SET_CPUID2
v8: - Renamed qmp method to query-kvm-cpuid and some fields in response.
- Modified documentation to qmp method
- Removed helper struct declaration
---
 qapi/machine-target.json   | 43 ++
 target/i386/kvm/kvm.c  | 37 
 tests/qtest/qmp-cmd-test.c |  1 +
 3 files changed, 81 insertions(+)

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index e7811654b7..a83180dd24 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -329,3 +329,46 @@
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
   'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || 
defined(TARGET_S390X) || defined(TARGET_MIPS)' }
+
+##
+# @CpuidEntry:
+#
+# A single entry of a CPUID response.
+#
+# One entry holds full set of information (leaf) returned to the guest in 
response
+# to it calling a CPUID instruction with eax, ecx used as the agruments to that
+# instruction. ecx is an optional argument as not all of the leaves support it.
+#
+# @in_eax: CPUID argument in eax
+# @in_ecx: CPUID argument in ecx
+# @eax: eax
+# @ebx: ebx
+# @ecx: ecx
+# @edx: edx
+#
+# Since: 6.1
+##
+{ 'struct': 'CpuidEntry',
+  'data': { 'in_eax' : 'uint32',
+'*in_ecx' : 'uint32',
+'eax' : 'uint32',
+'ebx' : 'uint32',
+'ecx' : 'uint32',
+'edx' : 'uint32'
+  },
+  'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
+
+##
+# @query-kvm-cpuid:
+#
+# Returns raw data from the KVM CPUID table for the first VCPU.
+# The KVM CPUID table defines the response to the CPUID
+# instruction when executed by the guest operating system.
+#
+# Returns: a list of CpuidEntry
+#
+# Since: 6.1
+##
+{ 'command': 'query-kvm-cpuid',
+  'returns': ['CpuidEntry'],
+  'if': 'defined(TARGET_I386) && defined(CONFIG_KVM)' }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 7fe9f52710..a59d6efa41 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -20,6 +20,7 @@
 
 #include 
 #include "standard-headers/asm-x86/kvm_para.h"
+#include "qapi/qapi-commands-machine-target.h"
 
 #include "cpu.h"
 #include "sysemu/sysemu.h"
@@ -1464,6 +1465,38 @@ 

Re: [PATCH v2 01/33] block: rename bdrv_replace_child to bdrv_replace_child_tran

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

We have bdrv_replace_child() wrapper on bdrv_replace_child_noperm().
But bdrv_replace_child() doesn't update permissions. It's rather
strange, as normally it's expected that foo() should call foo_noperm()
and update permissions.

Let's rename and add comment.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)



Reviewed-by: Max Reitz 

Re: [PATCH v2 02/33] block: comment graph-modifying function not updating permissions

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c | 7 +++
  1 file changed, 7 insertions(+)



Reviewed-by: Max Reitz 

Re: [PATCH qemu v20] spapr: Implement Open Firmware client interface

[BALATON Zoltan]

On Sun, 30 May 2021, BALATON Zoltan wrote:

On Thu, 20 May 2021, Alexey Kardashevskiy wrote:

diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
new file mode 100644
index ..a283b7d251a7
--- /dev/null
+++ b/hw/ppc/vof.c
@@ -0,0 +1,1021 @@
+/*
+ * QEMU PowerPC Virtual Open Firmware.
+ *
+ * This implements client interface from OpenFirmware IEEE1275 on the QEMU
+ * side to leave only a very basic firmware in the VM.
+ *
+ * Copyright (c) 2021 IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/timer.h"
+#include "qemu/range.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include 
+#include "exec/ram_addr.h"
+#include "exec/address-spaces.h"
+#include "hw/ppc/vof.h"
+#include "hw/ppc/fdt.h"
+#include "sysemu/runstate.h"
+#include "qom/qom-qobject.h"
+#include "trace.h"
+
+#include 
+
+/*
+ * OF 1275 "nextprop" description suggests is it 32 bytes max but
+ * LoPAPR defines "ibm,query-interrupt-source-number" which is 33 chars 
long.

+ */
+#define OF_PROPNAME_LEN_MAX 64
+
+#define VOF_MAX_PATH256
+#define VOF_MAX_SETPROPLEN  2048
+#define VOF_MAX_METHODLEN   256
+#define VOF_MAX_FORTHCODE   256
+#define VOF_VTY_BUF_SIZE256
+
+typedef struct {
+uint64_t start;
+uint64_t size;
+} OfClaimed;
+
+typedef struct {
+char *path; /* the path used to open the instance */
+uint32_t phandle;
+} OfInstance;
+
+#define VOF_MEM_READ(pa, buf, size) \
+address_space_read_full(&address_space_memory, \
+(pa), MEMTXATTRS_UNSPECIFIED, (buf), (size))
+#define VOF_MEM_WRITE(pa, buf, size) \
+address_space_write(&address_space_memory, \
+(pa), MEMTXATTRS_UNSPECIFIED, (buf), (size))
+
+static int readstr(hwaddr pa, char *buf, int size)
+{
+if (VOF_MEM_READ(pa, buf, size) != MEMTX_OK) {
+return -1;
+}
+if (strnlen(buf, size) == size) {
+buf[size - 1] = '\0';
+trace_vof_error_str_truncated(buf, size);
+return -1;
+}
+return 0;
+}
+
+static bool cmpservice(const char *s, unsigned nargs, unsigned nret,
+   const char *s1, unsigned nargscheck, unsigned 
nretcheck)

+{
+if (strcmp(s, s1)) {
+return false;
+}
+if ((nargscheck && (nargs != nargscheck)) ||
+(nretcheck && (nret != nretcheck))) {
+trace_vof_error_param(s, nargscheck, nretcheck, nargs, nret);
+return false;
+}
+
+return true;
+}
+
+static void prop_format(char *tval, int tlen, const void *prop, int len)
+{
+int i;
+const unsigned char *c;
+char *t;
+const char bin[] = "...";
+
+for (i = 0, c = prop; i < len; ++i, ++c) {
+if (*c == '\0' && i == len - 1) {
+strncpy(tval, prop, tlen - 1);
+return;
+}
+if (*c < 0x20 || *c >= 0x80) {
+break;
+}
+}
+
+for (i = 0, c = prop, t = tval; i < len; ++i, ++c) {
+if (t >= tval + tlen - sizeof(bin) - 1 - 2 - 1) {
+strcpy(t, bin);
+return;
+}
+if (i && i % 4 == 0 && i != len - 1) {
+strcat(t, " ");
+++t;
+}
+t += sprintf(t, "%02X", *c & 0xFF);
+}
+}
+
+static int get_path(const void *fdt, int offset, char *buf, int len)
+{
+int ret;
+
+ret = fdt_get_path(fdt, offset, buf, len - 1);
+if (ret < 0) {
+return ret;
+}
+
+buf[len - 1] = '\0';
+
+return strlen(buf) + 1;
+}
+
+static int phandle_to_path(const void *fdt, uint32_t ph, char *buf, int 
len)

+{
+int ret;
+
+ret = fdt_node_offset_by_phandle(fdt, ph);
+if (ret < 0) {
+return ret;
+}
+
+return get_path(fdt, ret, buf, len);
+}
+
+static uint32_t vof_finddevice(const void *fdt, uint32_t nodeaddr)
+{
+char fullnode[VOF_MAX_PATH];
+uint32_t ret = -1;
+int offset;
+
+if (readstr(nodeaddr, fullnode, sizeof(fullnode))) {
+return (uint32_t) ret;
+}
+
+offset = fdt_path_offset(fdt, fullnode);
+if (offset >= 0) {
+ret = fdt_get_phandle(fdt, offset);
+}
+trace_vof_finddevice(fullnode, ret);
+return (uint32_t) ret;
+}


The Linux init function that runs on pegasos2 here:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/kernel/prom_init.c?h=v4.14.234#n2658

calls finddevice once with isa@c and next with isa@C (small and capital C) 
both of which works with the board firmware but with vof the comparison is 
case sensitive and one of these fails so I can't make it work. I don't know 
if this is a problem in libfdt or the vof_finddevice above should do 
something else to get case insensitive comparison.


This fixes the issue with Linux but I'm not sure if there's any better 
solution or would it break anything else.


Regards,
BALATON Zoltan


diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c

index a283b7d251..b47bbd509d 100644
--- a/hw/ppc/vof.c
+++ b/hw/ppc/vof.c
@@ -144,12 +144,15 @@ static uint32_t vof_finddevice(cons

Re: [RFC PATCH] block/vpc: Support probing of fixed-size VHD images

[Max Reitz]

On 19.05.21 12:19, Thomas Huth wrote:

On 29/03/2021 09.25, Thomas Huth wrote:

Fixed-size VHD images don't have a header, only a footer. To be able
to still detect them right, support probing via the file name, too.

Without this change, images get detected as raw:

$ qemu-img create -f vpc -o subformat=fixed test.vhd 2G
Formatting 'test.vhd', fmt=vpc size=2147483648 subformat=fixed
$ qemu-img info test.vhd
image: test.vhd
file format: raw
virtual size: 2 GiB (2147992064 bytes)
disk size: 8 KiB

With this change:

$ qemu-img info test.vhd
image: test.vhd
file format: vpc
virtual size: 2 GiB (2147991552 bytes)
disk size: 8 KiB

Resolves: https://bugs.launchpad.net/qemu/+bug/1819182
Signed-off-by: Thomas Huth 
---
  I've marked the subject with RFC since I'm not quite sure whether this
  is really a good idea... please let me know what you think about it...

  block/vpc.c | 12 +++-
  1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 17a705b482..be561e4b39 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -191,8 +191,18 @@ static uint32_t vpc_checksum(void *p, size_t size)
    static int vpc_probe(const uint8_t *buf, int buf_size, const char 
*filename)

  {
-    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
+    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8)) {
  return 100;
+    }
+
+    /* It could be a fixed-size image without header -> check 
extension, too */

+    if (filename) {
+    int len = strlen(filename);
+    if (len > 4 && !strcasecmp(&filename[len - 4], ".vhd")) {
+    return 10;
+    }
+    }
+
  return 0;
  }


Ping!

Anybody any comments on this one?

 Thomas


Sorry I’m replying so late, but honestly, it’s because I’m just a bit 
afraid to respond.  So, perhaps like others, I hoped someone else with a 
stronger opinion would do it in my stead.


I understand this addresses a real problem, but OTOH probing by the file 
extension intuitively seems like a bad solution to me. What’s the 
problem with simply requiring the user to spcify the format in such a case?


I mean, I can’t think of a concrete problem with probing by the filename 
extension.  The worst that can happen is that a raw image is called 
.vhd, we try to open it, and the vhd driver then says the image doesn’t 
work.  That could be a real problem, but, well, it would be kind of 
deserved.  (Unless the user really in good faith just thought .vhd would 
be a nice extension for their raw virtual HDD image.  Which, then again, 
I couldn’t blame them for.)


Perhaps we could print a message if the extension matches that advises 
the user to explicitly specify the format for such images?


Max

Re: [PATCH v2 2/6] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

[Kevin Wolf]

Am 27.05.2021 um 22:14 hat Paolo Bonzini geschrieben:
> On 27/05/21 17:51, Kevin Wolf wrote:
> > Am 24.05.2021 um 18:36 hat Paolo Bonzini geschrieben:
> > > bs->sg is only true for character devices, but block devices can also
> > > be used with scsi-block and scsi-generic.  Unfortunately BLKSECTGET
> > > returns bytes in an int for /dev/sgN devices, and sectors in a short
> > > for block devices, so account for that in the code.
> > > 
> > > The maximum transfer also need not be a power of 2 (for example I have
> > > seen disks with 1280 KiB maximum transfer) so there's no need to pass
> > > the result through pow2floor.
> > > 
> > > Signed-off-by: Paolo Bonzini 
> > 
> > Looks like this is more or less a revert of Maxim's commit 867eccfe. If
> > this is what we want, should this old commit be mentioned in one way or
> > another in the commit message?
> 
> It is (but it is not intentional).
> 
> > Apparently the motivation for Maxim's patch was, if I'm reading the
> > description correctly, that it affected non-sg cases by imposing
> > unnecessary restrictions. I see that patch 1 changed the max_iov part so
> > that it won't affect non-sg cases any more, but max_transfer could still
> > be more restricted than necessary, no?
> 
> Indeed the kernel puts no limit at all, but especially with O_DIRECT we
> probably benefit from avoiding the moral equivalent of "bufferbloat".

Yeah, that sounds plausible, but on the other hand the bug report Maxim
addressed was about performance issues related to buffer sizes being too
small. So even if we want to have some limit, max_transfer of the host
device is probably not the right one for the general case.

> > For convenience, the bug report fixed with that patch is here:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1647104
> > 
> > Are we really trying to describe different things (limits for SG_IO and
> > for normal I/O) in one value with max_transfer, even though it could be
> > two different numbers for the same block device?
> 
> > > -static int sg_get_max_transfer_length(int fd)
> > > +static int sg_get_max_transfer_length(int fd, struct stat *st)
> > 
> > This is now a misnomer. Should we revert to the pre-867eccfe name
> > hdev_get_max_transfer_length()?
> 
> Yes.
> 
> > >   static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
> > >   {
> > >   BDRVRawState *s = bs->opaque;
> > > +struct stat st;
> > > +
> > > +if (fstat(s->fd, &st)) {
> > > +return;
> > 
> > Don't we want to set errp? Or do you intentionally ignore the error?
> 
> Yes, since we ignore errors from the ioctl I figured it's the same for fstat
> (just do not do the ioctls).
> 
> However, skipping raw_probe_alignment is wrong.
> 
> Thanks for the review!  Should I wait for you to go through the other
> patches?

I went through the whole series, but had no comments for the other
patches, so the rest should be good.

Kevin

Re: HSS Issue with GCC 10, Qemu Setup for microchip-icicle-kit

[Rahul Pathak]

I followed the same link. I will elaborate what is happening at my end -

*First* -
Used the same versions as per the doc. Built HSS 2020.12 and used
core-image-minimal-dev-icicle-kit-es-sd-20201009141623.rootfs.wic.
Upon executing the qemu launch command as per the doc, it's just waits for
the connection on another serial console -

info: QEMU waiting for connection on: disconnected:unix
:serial1.sock,server=on

Even if I open sudo minicom -D unix\#serial1.sock, which remains offline
always.

*Second* -
If I remove the "-chardev
socket,id=serial1,path=serial1.sock,server=on,wait=on -serial chardev:serial1"
from the
qemu launch command then HSS boots but stops after sbi_init on all the
cores and put the cores in Idle -

[5.443011] boot_download_chunks_onExit():
boot_service(u54_1)::u54_1:sbi_init 8020
[5.444960] boot_wait_onEntry(): boot_service(u54_1)::Checking for IPI ACKs:
- -
[5.447962] boot_wait_handler(): boot_service(u54_1)::Checking for IPI ACKs:
ACK/IDLE ACK
[5.449343] RunStateMachine(): boot_service(u54_1)::Wait ->
boot_service(u54_1)::Idle

*Third -*
If I take the latest release of HSS 2021.04 and build with gcc10, it does
not boot at all even if I remove the serial1 like in the second case.


Thanks
Rahul

On Mon, May 31, 2021 at 8:19 AM Bin Meng  wrote:

> Hi Rahul,
>
> On Mon, May 31, 2021 at 1:08 AM Rahul Pathak 
> wrote:
> >
> > Hi Bin,
> >
> > I was reading a github issue which you raised for the issue with HSS
> because of GCC 10.1. Its pretty old and not sure what has changed so I
> thought to ask you over email for help.
>
> The HSS issue of GCC 10.1 was already fixed in the HSS mainline.
>
> > I myself is trying to make a setup to boot the  microchip-icicle-kit
> Qemu machine with HSS and Yocto linux image. Currently it does not work.
>
> Could you please elaborate which part does not work? Is that Linux,
> HSS, or U-Boot?
>
> >
> > Do you know what is the right setup to make  microchip-icicle-kit
> machine with HSS.bin and Yocto linux work on Qemu?. Probably there will be
> a working combination of HSS, Linux releases plus the toolchain version
> which makes this setup work.
> >
>
> I have not tried Yocto Linux yet with Microchip Icicle Kit on QEMU. I
> only tested the Linux images released by Microchip.
> Could you please follow the instructions @
>
> https://qemu.readthedocs.io/en/latest/system/riscv/microchip-icicle-kit.html
> to see which step might be wrong on your side?
>
> Regards,
> Bin
>

Re: [PATCH v2 03/33] block: introduce bdrv_replace_child_bs()

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

Add function to transactionally replace bs inside BdrvChild.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block.h |  2 ++
  block.c   | 31 +++
  2 files changed, 33 insertions(+)


Reviewed-by: Max Reitz 

Re: [PATCH v2 4/5] progressmeter: protect with a mutex

[Stefan Hajnoczi]

On Tue, May 18, 2021 at 12:14:17PM +0200, Emanuele Giuseppe Esposito wrote:
> On 18/05/2021 12:00, Vladimir Sementsov-Ogievskiy wrote:
> > 18.05.2021 12:40, Emanuele Giuseppe Esposito wrote:
> > > Progressmeter is protected by the AioContext mutex, which
> > > is taken by the block jobs and their caller (like blockdev).
> > > 
> > > We would like to remove the dependency of block layer code on the
> > > AioContext mutex, since most drivers and the core I/O code are already
> > > not relying on it.
> > > 
> > > Create a new C file to implement the ProgressMeter API, but keep the
> > > struct as public, to avoid forcing allocation on the heap.
> > > 
> > > Also add a mutex to be able to provide an accurate snapshot of the
> > > progress values to the caller.
> > > 
> > > Reviewed-by: Stefan Hajnoczi 
> > 
> > patch changed a lot, so you can't keep Stefan's r-b. r-b should be kept
> > if patch is unchanged.
> 
> Sorry, my bad. Will remove it, if we keep these changes (see below).

I took a look again:

Reviewed-by: Stefan Hajnoczi 

Regarding hiding the struct, in a library with a public API the authors
need to be very careful about exposing struct fields in order to prevent
ABI breakage or applications making use of internal fields.

However, in QEMU we're less strict since we have full control over the
codebase (including internal API consumers). If other factors outweigh
the need for strict encapsulation, then you can put the struct
definition in the header file. Grepping for "private" in QEMU shows lots
of examples and there are probably many more without an explicit
"private" comment. Code reviewers can reject patches that touch private
struct fields or patches can be submitted to remove existing access to
private fields, so we don't have the limitations that library authors
have.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics

[Bruno Piazera Larsen]

On 29/05/2021 02:47, David Gibson wrote:

On Thu, May 27, 2021 at 08:01:56AM +0200, Greg Kurz wrote:

On Wed, 26 May 2021 17:21:01 -0300
"Bruno Larsen (billionai)"  wrote:


This function requires surce code modification to be useful, which means

s/surce/source


it probably is not used often, and the move to using decodetree means
the statistics won't even be collected anymore.

Also removed setting dump_statistics in ppc_cpu_realize, since it was

s/ppc_cpu_realize/ppc_cpu_class_init

A rebase on main has triggered a conflict with this patch, so I've
removed it from my tree again.


Did you answer to the correct patch? From what I can see, this patch is 
still in, but patch 5 is not. I fixed the rebase problem that that one 
had and will send it later, with the rest of the patch series.


--

Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 


Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer 

Re: GSoC 2021: Request for WIKI account to update project details

[Stefan Hajnoczi]

On Thu, May 27, 2021 at 12:35:48AM +0530, Niteesh G. S. wrote:
> I kindly request the admin to grant me access to the wiki through the below
> credentials

Hi Niteesh,
I have created an account for you and sent you the details privately.

Stefan


signature.asc
Description: PGP signature

Re: HSS Issue with GCC 10, Qemu Setup for microchip-icicle-kit

[Rahul Pathak]

On top of that, it seems I cannot connect with the target using gdb

(gdb) target remote :1234
Remote debugging using :1234
bfd requires flen 8, but target has flen 0

Though the ABI is lp64 and ISA is rv64imac when the hss was built.

On Mon, May 31, 2021 at 7:37 PM Rahul Pathak 
wrote:

> I followed the same link. I will elaborate what is happening at my end -
>
> *First* -
> Used the same versions as per the doc. Built HSS 2020.12 and used
> core-image-minimal-dev-icicle-kit-es-sd-20201009141623.rootfs.wic.
> Upon executing the qemu launch command as per the doc, it's just waits
> for the connection on another serial console -
>
> info: QEMU waiting for connection on: disconnected:unix
> :serial1.sock,server=on
>
> Even if I open sudo minicom -D unix\#serial1.sock, which remains offline
> always.
>
> *Second* -
> If I remove the "-chardev
> socket,id=serial1,path=serial1.sock,server=on,wait=on -serial chardev:serial1"
> from the
> qemu launch command then HSS boots but stops after sbi_init on all the
> cores and put the cores in Idle -
>
> [5.443011] boot_download_chunks_onExit():
> boot_service(u54_1)::u54_1:sbi_init 8020
> [5.444960] boot_wait_onEntry(): boot_service(u54_1)::Checking for IPI
> ACKs: - -
> [5.447962] boot_wait_handler(): boot_service(u54_1)::Checking for IPI
> ACKs: ACK/IDLE ACK
> [5.449343] RunStateMachine(): boot_service(u54_1)::Wait ->
> boot_service(u54_1)::Idle
>
> *Third -*
> If I take the latest release of HSS 2021.04 and build with gcc10, it does
> not boot at all even if I remove the serial1 like in the second case.
>
>
> Thanks
> Rahul
>
> On Mon, May 31, 2021 at 8:19 AM Bin Meng  wrote:
>
>> Hi Rahul,
>>
>> On Mon, May 31, 2021 at 1:08 AM Rahul Pathak 
>> wrote:
>> >
>> > Hi Bin,
>> >
>> > I was reading a github issue which you raised for the issue with HSS
>> because of GCC 10.1. Its pretty old and not sure what has changed so I
>> thought to ask you over email for help.
>>
>> The HSS issue of GCC 10.1 was already fixed in the HSS mainline.
>>
>> > I myself is trying to make a setup to boot the  microchip-icicle-kit
>> Qemu machine with HSS and Yocto linux image. Currently it does not work.
>>
>> Could you please elaborate which part does not work? Is that Linux,
>> HSS, or U-Boot?
>>
>> >
>> > Do you know what is the right setup to make  microchip-icicle-kit
>> machine with HSS.bin and Yocto linux work on Qemu?. Probably there will be
>> a working combination of HSS, Linux releases plus the toolchain version
>> which makes this setup work.
>> >
>>
>> I have not tried Yocto Linux yet with Microchip Icicle Kit on QEMU. I
>> only tested the Linux images released by Microchip.
>> Could you please follow the instructions @
>>
>> https://qemu.readthedocs.io/en/latest/system/riscv/microchip-icicle-kit.html
>> to see which step might be wrong on your side?
>>
>> Regards,
>> Bin
>>
>

Re: [PATCH v2 0/3] block: drop BlockDriverState::read_only

[Kevin Wolf]

Am 27.05.2021 um 17:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> The field duplicates information in .open_flags. We have to carefully
> sync these two fields everywhere. It's simple to introduce a bug by
> forgetting it.
> 
> Let's drop the field, and fix users to call bdrv_is_read_only() and
> bdrv_is_writable() instead.
> 
> v2:
> 01: write "update_header =\n..." in one line
> 03: only change BlockBackendRootState, don't touch side logic

Thanks, applied to the block branch.

Kevin

[PATCH v3 0/4] target/ppc: Remove DO_PPC_STATISTICS and PPC_DUMP_CPU

[Bruno Larsen (billionai)]

These 2 defines are being obsoleted as we move to decodetree, so they
were removed.

Also, upon further inspection, qemu already doesn't build with them
enabled, and probably hasn't for a while, and no one complained, so I
don't think they were actually being used.

Based-on: dgibson's ppc-for-6.1 tree

Changelog for v3:
 * Re-added patch that removed cpu_dump_statistics from hw/core/cpu
 * added HMP documentation patch to this series

Changelog for v2:
 * removed patches that were already applied
 * also removed PPC_DUMP_CPU functinality

Bruno Larsen (billionai) (4):
  hw/core/cpu: removed cpu_dump_statistics function
  HMP: added info cpustats to removed_features.rst
  target/ppc: removed GEN_OPCODE decision tree
  target/ppc: removed all mentions to PPC_DUMP_CPU

 docs/system/removed-features.rst |   5 +
 hw/core/cpu-common.c |   9 --
 include/hw/core/cpu.h|  12 --
 target/ppc/cpu_init.c| 205 ---
 target/ppc/internal.h|   2 -
 target/ppc/translate.c   | 184 ---
 6 files changed, 5 insertions(+), 412 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] hw/core/cpu: removed cpu_dump_statistics function

[Bruno Larsen (billionai)]

No more architectures set the pointer to dump_statistics, so there's no
point in keeping it, or the related cpu_dump_statistics function.

Suggested-by: Richard Henderson 
Signed-off-by: Bruno Larsen (billionai) 
Message-Id: <20210526202104.127910-6-bruno.lar...@eldorado.org.br>
Reviewed-by: Richard Henderson 
Reviewed-by: Luis Pires 
Signed-off-by: David Gibson 
---
 hw/core/cpu-common.c  |  9 -
 include/hw/core/cpu.h | 12 
 2 files changed, 21 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 9530e266ec..e2f5a64604 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -109,15 +109,6 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
 }
 }
 
-void cpu_dump_statistics(CPUState *cpu, int flags)
-{
-CPUClass *cc = CPU_GET_CLASS(cpu);
-
-if (cc->dump_statistics) {
-cc->dump_statistics(cpu, flags);
-}
-}
-
 void cpu_reset(CPUState *cpu)
 {
 device_cold_reset(DEVICE(cpu));
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 044f668a6e..6b3bd3a1d4 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -92,7 +92,6 @@ struct SysemuCPUOps;
  * @has_work: Callback for checking if there is work to do.
  * @memory_rw_debug: Callback for GDB memory access.
  * @dump_state: Callback for dumping state.
- * @dump_statistics: Callback for dumping statistics.
  * @get_arch_id: Callback for getting architecture-dependent CPU ID.
  * @set_pc: Callback for setting the Program Counter register. This
  *   should have the semantics used by the target architecture when
@@ -134,7 +133,6 @@ struct CPUClass {
 int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
uint8_t *buf, int len, bool is_write);
 void (*dump_state)(CPUState *cpu, FILE *, int flags);
-void (*dump_statistics)(CPUState *cpu, int flags);
 int64_t (*get_arch_id)(CPUState *cpu);
 void (*set_pc)(CPUState *cpu, vaddr value);
 int (*gdb_read_register)(CPUState *cpu, GByteArray *buf, int reg);
@@ -534,16 +532,6 @@ enum CPUDumpFlags {
  */
 void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 
-/**
- * cpu_dump_statistics:
- * @cpu: The CPU whose state is to be dumped.
- * @flags: Flags what to dump.
- *
- * Dump CPU statistics to the current monitor if we have one, else to
- * stdout.
- */
-void cpu_dump_statistics(CPUState *cpu, int flags);
-
 #ifndef CONFIG_USER_ONLY
 /**
  * cpu_get_phys_page_attrs_debug:
-- 
2.17.1

[PATCH v3 2/4] HMP: added info cpustats to removed_features.rst

[Bruno Larsen (billionai)]

Documented the removal of the HMP command info cpustats

Signed-off-by: Bruno Larsen (billionai) 
---
 docs/system/removed-features.rst | 5 +
 1 file changed, 5 insertions(+)

diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 5a462ac568..2feae41089 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -249,6 +249,11 @@ Use ``migrate-set-parameters`` and ``info 
migrate-parameters`` instead.
 
 Use ``migrate-set-parameters`` instead.
 
+``info cpustats`` (removed in 6.1)
+'
+
+This command didn't produce any output already. Removed with no replacement.
+
 Guest Emulator ISAs
 ---
 
-- 
2.17.1

[PATCH v3 3/4] target/ppc: removed GEN_OPCODE decision tree

[Bruno Larsen (billionai)]

since both, PPC_DO_STATISTICS and PPC_DUMP_CPU, are obsoleted as
target/ppc moves to decodetree, we can remove this ifdef based decision
tree, and only have what is now the standard option for the macro.

Signed-off-by: Bruno Larsen (billionai) 
---
 target/ppc/translate.c | 79 --
 1 file changed, 79 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 5c56e33c3c..4b66563998 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1341,7 +1341,6 @@ typedef struct opcode_t {
 /*/
 /* PowerPC instructions table*/
 
-#if defined(DO_PPC_STATISTICS)
 #define GEN_OPCODE(name, op1, op2, op3, invl, _typ, _typ2)\
 { \
 .opc1 = op1,  \
@@ -1353,7 +1352,6 @@ typedef struct opcode_t {
 .type = _typ, \
 .type2 = _typ2,   \
 .handler = &gen_##name,   \
-.oname = stringify(name), \
 },\
 .oname = stringify(name), \
 }
@@ -1369,7 +1367,6 @@ typedef struct opcode_t {
 .type = _typ, \
 .type2 = _typ2,   \
 .handler = &gen_##name,   \
-.oname = stringify(name), \
 },\
 .oname = stringify(name), \
 }
@@ -1384,7 +1381,6 @@ typedef struct opcode_t {
 .type = _typ, \
 .type2 = _typ2,   \
 .handler = &gen_##name,   \
-.oname = onam,\
 },\
 .oname = onam,\
 }
@@ -1399,7 +1395,6 @@ typedef struct opcode_t {
 .type = _typ, \
 .type2 = _typ2,   \
 .handler = &gen_##name,   \
-.oname = stringify(name), \
 },\
 .oname = stringify(name), \
 }
@@ -1414,83 +1409,9 @@ typedef struct opcode_t {
 .type = _typ, \
 .type2 = _typ2,   \
 .handler = &gen_##name,   \
-.oname = onam,\
 },\
 .oname = onam,\
 }
-#else
-#define GEN_OPCODE(name, op1, op2, op3, invl, _typ, _typ2)\
-{ \
-.opc1 = op1,  \
-.opc2 = op2,  \
-.opc3 = op3,  \
-.opc4 = 0xff, \
-.handler = {  \
-.inval1  = invl,  \
-.type = _typ, \
-.type2 = _typ2,   \
-.handler = &gen_##name,   \
-},\
-.oname = stringify(name), \
-}
-#define GEN_OPCODE_DUAL(name, op1, op2, op3, invl1, invl2, _typ, _typ2)   \
-{ \
-.opc1 = op1,  \
-.opc

[PATCH v3 4/4] target/ppc: removed all mentions to PPC_DUMP_CPU

[Bruno Larsen (billionai)]

This feature will no longer be useful as ppc moves to using decotree for
TCG. And building with it enabled is no longer possible, due to changes
in opc_handler_t. Since the last commit that mentions it happened in
2014, I think it is safe to remove it.

Signed-off-by: Bruno Larsen (billionai) 
---
 target/ppc/cpu_init.c  | 205 -
 target/ppc/internal.h  |   2 -
 target/ppc/translate.c | 105 -
 3 files changed, 312 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 74a397ad6c..d0411e7302 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -8541,45 +8541,6 @@ static void init_ppc_proc(PowerPCCPU *cpu)
 }
 }
 
-#if defined(PPC_DUMP_CPU)
-static void dump_ppc_sprs(CPUPPCState *env)
-{
-ppc_spr_t *spr;
-#if !defined(CONFIG_USER_ONLY)
-uint32_t sr, sw;
-#endif
-uint32_t ur, uw;
-int i, j, n;
-
-printf("Special purpose registers:\n");
-for (i = 0; i < 32; i++) {
-for (j = 0; j < 32; j++) {
-n = (i << 5) | j;
-spr = &env->spr_cb[n];
-uw = spr->uea_write != NULL && spr->uea_write != SPR_NOACCESS;
-ur = spr->uea_read != NULL && spr->uea_read != SPR_NOACCESS;
-#if !defined(CONFIG_USER_ONLY)
-sw = spr->oea_write != NULL && spr->oea_write != SPR_NOACCESS;
-sr = spr->oea_read != NULL && spr->oea_read != SPR_NOACCESS;
-if (sw || sr || uw || ur) {
-printf("SPR: %4d (%03x) %-8s s%c%c u%c%c\n",
-   (i << 5) | j, (i << 5) | j, spr->name,
-   sw ? 'w' : '-', sr ? 'r' : '-',
-   uw ? 'w' : '-', ur ? 'r' : '-');
-}
-#else
-if (uw || ur) {
-printf("SPR: %4d (%03x) %-8s u%c%c\n",
-   (i << 5) | j, (i << 5) | j, spr->name,
-   uw ? 'w' : '-', ur ? 'r' : '-');
-}
-#endif
-}
-}
-fflush(stdout);
-fflush(stderr);
-}
-#endif
 
 static void ppc_cpu_realize(DeviceState *dev, Error **errp)
 {
@@ -8616,172 +8577,6 @@ static void ppc_cpu_realize(DeviceState *dev, Error 
**errp)
 
 pcc->parent_realize(dev, errp);
 
-#if defined(PPC_DUMP_CPU)
-{
-CPUPPCState *env = &cpu->env;
-const char *mmu_model, *excp_model, *bus_model;
-switch (env->mmu_model) {
-case POWERPC_MMU_32B:
-mmu_model = "PowerPC 32";
-break;
-case POWERPC_MMU_SOFT_6xx:
-mmu_model = "PowerPC 6xx/7xx with software driven TLBs";
-break;
-case POWERPC_MMU_SOFT_74xx:
-mmu_model = "PowerPC 74xx with software driven TLBs";
-break;
-case POWERPC_MMU_SOFT_4xx:
-mmu_model = "PowerPC 4xx with software driven TLBs";
-break;
-case POWERPC_MMU_SOFT_4xx_Z:
-mmu_model = "PowerPC 4xx with software driven TLBs "
-"and zones protections";
-break;
-case POWERPC_MMU_REAL:
-mmu_model = "PowerPC real mode only";
-break;
-case POWERPC_MMU_MPC8xx:
-mmu_model = "PowerPC MPC8xx";
-break;
-case POWERPC_MMU_BOOKE:
-mmu_model = "PowerPC BookE";
-break;
-case POWERPC_MMU_BOOKE206:
-mmu_model = "PowerPC BookE 2.06";
-break;
-case POWERPC_MMU_601:
-mmu_model = "PowerPC 601";
-break;
-#if defined(TARGET_PPC64)
-case POWERPC_MMU_64B:
-mmu_model = "PowerPC 64";
-break;
-#endif
-default:
-mmu_model = "Unknown or invalid";
-break;
-}
-switch (env->excp_model) {
-case POWERPC_EXCP_STD:
-excp_model = "PowerPC";
-break;
-case POWERPC_EXCP_40x:
-excp_model = "PowerPC 40x";
-break;
-case POWERPC_EXCP_601:
-excp_model = "PowerPC 601";
-break;
-case POWERPC_EXCP_602:
-excp_model = "PowerPC 602";
-break;
-case POWERPC_EXCP_603:
-excp_model = "PowerPC 603";
-break;
-case POWERPC_EXCP_603E:
-excp_model = "PowerPC 603e";
-break;
-case POWERPC_EXCP_604:
-excp_model = "PowerPC 604";
-break;
-case POWERPC_EXCP_7x0:
-excp_model = "PowerPC 740/750";
-break;
-case POWERPC_EXCP_7x5:
-excp_model = "PowerPC 745/755";
-break;
-case POWERPC_EXCP_74xx:
-excp_model = "PowerPC 74xx";
-break;
-case POWERPC_EXCP_BOOKE:
-excp_model = "PowerPC BookE";
-break;
-#if defined(TARGET_PPC64)
-case POWERPC_EXCP_970:
-excp_model = "PowerPC 970";
-break;
-#endif
-default:
-excp_model = "Unknown or invalid";
-  

Re: [PULL 2/3] hw/usb: hcd-xhci-pci: Raise MSI/MSI-X interrupts only when told to

[Alexander Bulekov]

On 210528 1622, Gerd Hoffmann wrote:
> From: Ruimei Yan 
> 
> At present MSI / MSI-X interrupts are triggered regardless of the
> irq level. We should have checked the level to determine whether
> the interrupt needs to be delivered.
> 
> The level check logic was present in early versions of the xhci
> model, but got dropped later by a rework of interrupt handling
> under commit 4c4abe7cc903 ("xhci: rework interrupt handling").
> 
> Fixes: 4c4abe7cc903 ("xhci: rework interrupt handling")
> Signed-off-by: Ruimei Yan 
> Signed-off-by: Bin Meng 
> Message-Id: <20210521024224.2277634-1-bmeng...@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Gerd Hoffmann 

Hi,
FYI, OSS-Fuzz detected that this fixed this stack-overflow:
https://bugs.launchpad.net/bugs/1905444
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=27796#c5
-Alex

Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed

[Niklas Cassel]

On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
> On May 28 11:05, Niklas Cassel wrote:
> > From: Niklas Cassel 
> > 
> > In the Zoned Namespace Command Set Specification, chapter
> > 2.5.1 Managing resources
> > 
> > "The controller may transition zones in the ZSIO:Implicitly Opened state
> > to the ZSC:Closed state for resource management purposes."
> > 
> > The word may in this sentence means that automatically transitioning
> > an implicitly opened zone to closed is completely optional.
> > 
> > Add a new parameter so that the user can control if this automatic
> > transitioning should be performed or not.
> > 
> > Being able to control this can help with verifying that e.g. a user-space
> > program behaves properly even without this optional ZNS feature.
> > 
> > The default value is set to true, in order to not change the existing
> > behavior.
> > 
> > Signed-off-by: Niklas Cassel 
> > ---
> > hw/nvme/ctrl.c | 9 -
> > hw/nvme/ns.c   | 2 ++
> > hw/nvme/nvme.h | 1 +
> > 3 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 40a7efcea9..d00f0297a5 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -141,6 +141,11 @@
> >  *
> >  * zoned.cross_read=
> >  * Setting this property to true enables Read Across Zone 
> > Boundaries.
> > + *
> > + * zoned.auto_transition= > true>
> > + * Indicates if zones in zone state implicitly opened can be
> > + * automatically transitioned to zone state closed for resource
> > + * management purposes.
> >  */
> > 
> > #include "qemu/osdep.h"
> > @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace 
> > *ns, NvmeZone *zone,
> > /* fallthrough */
> > 
> > case NVME_ZONE_STATE_CLOSED:
> > -nvme_zrm_auto_transition_zone(ns);
> > +if (ns->params.auto_transition_zones) {
> > +nvme_zrm_auto_transition_zone(ns);
> > +}
> > status = nvme_aor_check(ns, act, 1);
> > if (status) {
> > return status;
> > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> > index 3fec9c6273..31dee43d30 100644
> > --- a/hw/nvme/ns.c
> > +++ b/hw/nvme/ns.c
> > @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
> >params.max_open_zones, 0),
> > DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
> >params.zd_extension_size, 0),
> > +DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
> > + params.auto_transition_zones, true),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> > 
> > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> > index 81a35cda14..bd86054db2 100644
> > --- a/hw/nvme/nvme.h
> > +++ b/hw/nvme/nvme.h
> > @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
> > uint32_t max_active_zones;
> > uint32_t max_open_zones;
> > uint32_t zd_extension_size;
> > +bool auto_transition_zones;
> > } NvmeNamespaceParams;
> > 
> > typedef struct NvmeNamespace {
> > -- 
> > 2.31.1
> > 
> 
> Looks good Niklas!
> 
> Reviewed-by: Klaus Jensen 

In reality, it is the controller that does the auto transitioning.

In theory, one namespace could be attached to two different controllers,
and I guess, in that case, it depends on if the controller that we used
when doing the write supports auto transitioning or not, that determines
if a zone will be auto transitioned or not.

If we were to change this to be a parameter of the controller instead
of a parameter of the namespace, we would require to refactor a lot of
code in the regular write path. As we currently don't have any NvmeRequest
object in nvme_zrm_open_flags().

Thoughts?


Kind regards,
Niklas

Re: [PATCH v2 2/5] block-backend: improve blk_root_get_parent_desc()

[Kevin Wolf]

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We have different types of parents: block nodes, block backends and
> jobs. So, it makes sense to specify type together with name.
> 
> While being here also use g_autofree.
> 
> iotest 307 output is updated.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Alberto Garcia 
> ---
>  block/block-backend.c  | 9 -
>  tests/qemu-iotests/307.out | 2 +-
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 6fca9853e1..2b7e9b5192 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -142,19 +142,18 @@ static void blk_root_set_aio_ctx(BdrvChild *child, 
> AioContext *ctx,
>  static char *blk_root_get_parent_desc(BdrvChild *child)
>  {
>  BlockBackend *blk = child->opaque;
> -char *dev_id;
> +g_autofree char *dev_id = NULL;
>  
>  if (blk->name) {
> -return g_strdup(blk->name);
> +return g_strdup_printf("block device '%s'", blk->name);
>  }
>  
>  dev_id = blk_get_attached_dev_id(blk);
>  if (*dev_id) {
> -return dev_id;
> +return g_strdup_printf("block device '%s'", dev_id);
>  } else {
>  /* TODO Callback into the BB owner for something more detailed */
> -g_free(dev_id);
> -return g_strdup("a block device");
> +return g_strdup("unnamed block device");

We should probably keep the article: "an unnamed block device"

>  }
>  }

Kevin

Re: [PATCH v2 4/5] block: simplify bdrv_child_user_desc()

[Kevin Wolf]

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> All existing parent types (block nodes, block devices, jobs) has the
> realization. So, drop unreachable code.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Your fixes the other days showed that vvfat has a BdrvChildClass, too.
This instance doesn't define .get_parent_desc(), so the code that this
patch removes is potentially still reachable.

Kevin

Re: [PATCH v2 05/33] qdev-properties: PropertyInfo: add realized_set_allowed field

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

Add field, so property can declare support for setting the property
when device is realized. To be used in the following commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/hw/qdev-properties.h | 1 +
  hw/core/qdev-properties.c| 6 +++---
  2 files changed, 4 insertions(+), 3 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v2 06/33] qdev: allow setting drive property for realized device

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

We need an ability to insert filters above top block node, attached to
block device. It can't be achieved with blockdev-reopen command. So, we
want do it with help of qom-set.

Intended usage:

Assume there is a node A that is attached to some guest device.

1. blockdev-add to create a filter node B that has A as its child.

2. qom-set to change the node attached to the guest device’s
BlockBackend from A to B.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  hw/core/qdev-properties-system.c | 43 +++-
  1 file changed, 31 insertions(+), 12 deletions(-)


Tentative

Reviewed-by: Max Reitz 

Re: [PATCH v2 5/5] block: improve permission conflict error message

[Kevin Wolf]

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Now permissions are updated as follows:
>  1. do graph modifications ignoring permissions
>  2. do permission update
> 
>  (of course, we rollback [1] if [2] fails)
> 
> So, on stage [2] we can't say which users are "old" and which are
> "new" and exist only since [1]. And current error message is a bit
> outdated. Let's improve it, to make everything clean.
> 
> While being here, add also a comment and some good assertions.
> 
> iotests 283, 307, qsd-jobs outputs are updated.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block.c   | 29 ---
>  tests/qemu-iotests/283.out|  2 +-
>  tests/qemu-iotests/307.out|  2 +-
>  tests/qemu-iotests/tests/qsd-jobs.out |  2 +-
>  4 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 2f73523285..354438d918 100644
> --- a/block.c
> +++ b/block.c
> @@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c)
>  return c->klass->get_parent_desc(c);
>  }
>  
> +/*
> + * Check that @a allows everything that @b needs. @a and @b must reference 
> same
> + * child node.
> + */
>  static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
>  {
> -g_autofree char *user = NULL;
> -g_autofree char *perm_names = NULL;
> +g_autofree char *a_user = NULL;
> +g_autofree char *a_against = NULL;
> +g_autofree char *b_user = NULL;
> +g_autofree char *b_perm = NULL;
> +
> +assert(a->bs);
> +assert(a->bs == b->bs);
>  
>  if ((b->perm & a->shared_perm) == b->perm) {
>  return true;
>  }
>  
> -perm_names = bdrv_perm_names(b->perm & ~a->shared_perm);
> -user = bdrv_child_user_desc(a);
> -error_setg(errp, "Conflicts with use by %s as '%s', which does not "
> -   "allow '%s' on %s",
> -   user, a->name, perm_names, bdrv_get_node_name(b->bs));
> +a_user = bdrv_child_user_desc(a);
> +a_against = bdrv_perm_names(b->perm & ~a->shared_perm);
> +
> +b_user = bdrv_child_user_desc(b);
> +b_perm = bdrv_perm_names(b->perm);
> +error_setg(errp, "Permission conflict on node '%s': %s wants to use it 
> as "
> +   "'%s', which requires these permissions: %s. On the other 
> hand %s "
> +   "wants to use it as '%s', which doesn't share: %s",
> +   bdrv_get_node_name(b->bs),
> +   b_user, b->name, b_perm, a_user, a->name, a_against);

I think the combination of a_against and b_perm is confusing to report
because one is the intersection of permissions (i.e. only the
permissions that actually conflict) and the other the full list of
unshared permissions.

We could report both the full list of required permissions (which is
what your current error message claims to report) and of unshared
permissions. I'm not sure if there is actually any use for this
information.

The other option that would feel consistent is to report only the
conflicting permissions, and report them only once because they are the
same for both sides.

Kevin

>  
>  return false;
>  }
> diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
> index c9397bfc44..92f3cc1ed5 100644
> --- a/tests/qemu-iotests/283.out
> +++ b/tests/qemu-iotests/283.out
> @@ -5,7 +5,7 @@
>  {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": 
> "base", "node-name": "other", "take-child-perms": ["write"]}}
>  {"return": {}}
>  {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": 
> "full", "target": "target"}}
> -{"error": {"class": "GenericError", "desc": "Cannot append backup-top 
> filter: Conflicts with use by node 'source' as 'image', which does not allow 
> 'write' on base"}}
> +{"error": {"class": "GenericError", "desc": "Cannot append backup-top 
> filter: Permission conflict on node 'base': node 'other' wants to use it as 
> 'image', which requires these permissions: write. On the other hand node 
> 'source' wants to use it as 'image', which doesn't share: write"}}
>  
>  === backup-top should be gone after job-finalize ===
>  
> diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
> index 66bf2ddb74..e03932ba4f 100644
> --- a/tests/qemu-iotests/307.out
> +++ b/tests/qemu-iotests/307.out
> @@ -53,7 +53,7 @@ exports available: 1
>  
>  === Add a writable export ===
>  {"execute": "block-export-add", "arguments": {"description": "This is the 
> writable second export", "id": "export1", "name": "export1", "node-name": 
> "fmt", "type": "nbd", "writable": true, "writethrough": true}}
> -{"error": {"class": "GenericError", "desc": "Conflicts with use by block 
> device 'sda' as 'root', which does not allow 'write' on fmt"}}
> +{"error": {"class": "GenericError", "desc": "Permission conflict on node 
> 'fmt': unnamed block device wants to use it as 'root', which requires these 
> permissions: consistent read, write. On the 

Re: [PATCH v2 07/33] block: rename backup-top to copy-before-write

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

We are going to convert backup_top to full featured public filter,
which can be used in separate of backup job. Start from renaming from
"how it used" to "what it does".

While updating comments in 283 iotest, drop and rephrase also things
about ".active", as this field is now dropped, and filter doesn't have
"inactive" mode.

Note that this change may be considered as incompatible interface
change, as backup-top filter format name was visible through
query-block and query-named-block-nodes.

Still, consider the following reasoning:

1. backup-top was never documented, so if someone depends on format
name (for driver that can't be used other than it is automatically
inserted on backup job start), it's a kind of "undocumented feature
use". So I think we are free to change it.

2. There is a hope, that there is no such users: it's a lot more native
to give a good node-name to backup-top filter if need to operate
with it somehow, and don't touch format name.

3. Another "incompatible" change in further commit would be moving
copy-before-write filter from using backing child to file child. And
this is even more reasonable than renaming: for now all public
filters are file-child based.

So, it's a risky change, but risk seems small and good interface worth
it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/{backup-top.h => copy-before-write.h} |  28 +++---
  block/backup.c  |  22 ++---
  block/{backup-top.c => copy-before-write.c} | 100 ++--
  MAINTAINERS |   4 +-
  block/meson.build   |   2 +-
  tests/qemu-iotests/283  |  35 +++
  tests/qemu-iotests/283.out  |   4 +-
  7 files changed, 95 insertions(+), 100 deletions(-)
  rename block/{backup-top.h => copy-before-write.h} (56%)
  rename block/{backup-top.c => copy-before-write.c} (62%)


Reviewed-by: Max Reitz 

Re: [PATCH v2 2/5] block-backend: improve blk_root_get_parent_desc()

[Vladimir Sementsov-Ogievskiy]

31.05.2021 18:45, Kevin Wolf wrote:

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:

We have different types of parents: block nodes, block backends and
jobs. So, it makes sense to specify type together with name.

While being here also use g_autofree.

iotest 307 output is updated.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
  block/block-backend.c  | 9 -
  tests/qemu-iotests/307.out | 2 +-
  2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6fca9853e1..2b7e9b5192 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -142,19 +142,18 @@ static void blk_root_set_aio_ctx(BdrvChild *child, 
AioContext *ctx,
  static char *blk_root_get_parent_desc(BdrvChild *child)
  {
  BlockBackend *blk = child->opaque;
-char *dev_id;
+g_autofree char *dev_id = NULL;
  
  if (blk->name) {

-return g_strdup(blk->name);
+return g_strdup_printf("block device '%s'", blk->name);
  }
  
  dev_id = blk_get_attached_dev_id(blk);

  if (*dev_id) {
-return dev_id;
+return g_strdup_printf("block device '%s'", dev_id);
  } else {
  /* TODO Callback into the BB owner for something more detailed */
-g_free(dev_id);
-return g_strdup("a block device");
+return g_strdup("unnamed block device");


We should probably keep the article: "an unnamed block device"


OK, will do


--
Best regards,
Vladimir

Re: [PATCH v2 5/5] block: improve permission conflict error message

[Vladimir Sementsov-Ogievskiy]

31.05.2021 19:07, Kevin Wolf wrote:

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:

Now permissions are updated as follows:
  1. do graph modifications ignoring permissions
  2. do permission update

  (of course, we rollback [1] if [2] fails)

So, on stage [2] we can't say which users are "old" and which are
"new" and exist only since [1]. And current error message is a bit
outdated. Let's improve it, to make everything clean.

While being here, add also a comment and some good assertions.

iotests 283, 307, qsd-jobs outputs are updated.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c   | 29 ---
  tests/qemu-iotests/283.out|  2 +-
  tests/qemu-iotests/307.out|  2 +-
  tests/qemu-iotests/tests/qsd-jobs.out |  2 +-
  4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 2f73523285..354438d918 100644
--- a/block.c
+++ b/block.c
@@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c)
  return c->klass->get_parent_desc(c);
  }
  
+/*

+ * Check that @a allows everything that @b needs. @a and @b must reference same
+ * child node.
+ */
  static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
  {
-g_autofree char *user = NULL;
-g_autofree char *perm_names = NULL;
+g_autofree char *a_user = NULL;
+g_autofree char *a_against = NULL;
+g_autofree char *b_user = NULL;
+g_autofree char *b_perm = NULL;
+
+assert(a->bs);
+assert(a->bs == b->bs);
  
  if ((b->perm & a->shared_perm) == b->perm) {

  return true;
  }
  
-perm_names = bdrv_perm_names(b->perm & ~a->shared_perm);

-user = bdrv_child_user_desc(a);
-error_setg(errp, "Conflicts with use by %s as '%s', which does not "
-   "allow '%s' on %s",
-   user, a->name, perm_names, bdrv_get_node_name(b->bs));
+a_user = bdrv_child_user_desc(a);
+a_against = bdrv_perm_names(b->perm & ~a->shared_perm);
+
+b_user = bdrv_child_user_desc(b);
+b_perm = bdrv_perm_names(b->perm);
+error_setg(errp, "Permission conflict on node '%s': %s wants to use it as "
+   "'%s', which requires these permissions: %s. On the other hand %s 
"
+   "wants to use it as '%s', which doesn't share: %s",
+   bdrv_get_node_name(b->bs),
+   b_user, b->name, b_perm, a_user, a->name, a_against);


I think the combination of a_against and b_perm is confusing to report
because one is the intersection of permissions (i.e. only the
permissions that actually conflict) and the other the full list of
unshared permissions.

We could report both the full list of required permissions (which is
what your current error message claims to report) and of unshared
permissions. I'm not sure if there is actually any use for this
information.

The other option that would feel consistent is to report only the
conflicting permissions, and report them only once because they are the
same for both sides.



Agreed.

So, what about:

  error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s 
(%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), a_against, b_user, b->name, 
a_user, a->name);

?




  
  return false;

  }
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index c9397bfc44..92f3cc1ed5 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -5,7 +5,7 @@
  {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": 
"other", "take-child-perms": ["write"]}}
  {"return": {}}
  {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", 
"target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: 
Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}}
+{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: 
Permission conflict on node 'base': node 'other' wants to use it as 'image', which requires these permissions: write. On the 
other hand node 'source' wants to use it as 'image', which doesn't share: write"}}
  
  === backup-top should be gone after job-finalize ===
  
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out

index 66bf2ddb74..e03932ba4f 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -53,7 +53,7 @@ exports available: 1
  
  === Add a writable export ===

  {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", 
"node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}}
-{"error": {"class": "GenericError", "desc": "Conflicts with use by block device 
'sda' as 'root', which does not allow 'write' on fmt"}}
+{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt': 
unnamed block device wants 

Re: [PATCH v2 12/33] block/copy-before-write: drop extra bdrv_unref on failure path

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

bdrv_attach_child() do bdrv_unref() on failure, so we shouldn't do it
by hand here.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/copy-before-write.c | 1 -
  1 file changed, 1 deletion(-)


Reviewed-by: Max Reitz 

Re: [PATCH v2 4/5] block: simplify bdrv_child_user_desc()

[Vladimir Sementsov-Ogievskiy]

31.05.2021 18:50, Kevin Wolf wrote:

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:

All existing parent types (block nodes, block devices, jobs) has the
realization. So, drop unreachable code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 


Your fixes the other days showed that vvfat has a BdrvChildClass, too.
This instance doesn't define .get_parent_desc(), so the code that this
patch removes is potentially still reachable.



Right. Will fix it and add an assertion to bdrv_attach_child_common()


--
Best regards,
Vladimir

Re: [PATCH v2 13/33] block/copy-before-write: use file child instead of backing

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

We are going to publish copy-before-write filter, and there no public
backing-child-based filter in Qemu. No reason to create a precedent, so
let's refactor copy-before-write filter instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/copy-before-write.c | 39 ++-
  1 file changed, 22 insertions(+), 17 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v2 5/5] block: improve permission conflict error message

[Kevin Wolf]

Am 31.05.2021 um 18:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 31.05.2021 19:07, Kevin Wolf wrote:
> > Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > Now permissions are updated as follows:
> > >   1. do graph modifications ignoring permissions
> > >   2. do permission update
> > > 
> > >   (of course, we rollback [1] if [2] fails)
> > > 
> > > So, on stage [2] we can't say which users are "old" and which are
> > > "new" and exist only since [1]. And current error message is a bit
> > > outdated. Let's improve it, to make everything clean.
> > > 
> > > While being here, add also a comment and some good assertions.
> > > 
> > > iotests 283, 307, qsd-jobs outputs are updated.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > > ---
> > >   block.c   | 29 ---
> > >   tests/qemu-iotests/283.out|  2 +-
> > >   tests/qemu-iotests/307.out|  2 +-
> > >   tests/qemu-iotests/tests/qsd-jobs.out |  2 +-
> > >   4 files changed, 25 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 2f73523285..354438d918 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c)
> > >   return c->klass->get_parent_desc(c);
> > >   }
> > > +/*
> > > + * Check that @a allows everything that @b needs. @a and @b must 
> > > reference same
> > > + * child node.
> > > + */
> > >   static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
> > >   {
> > > -g_autofree char *user = NULL;
> > > -g_autofree char *perm_names = NULL;
> > > +g_autofree char *a_user = NULL;
> > > +g_autofree char *a_against = NULL;
> > > +g_autofree char *b_user = NULL;
> > > +g_autofree char *b_perm = NULL;
> > > +
> > > +assert(a->bs);
> > > +assert(a->bs == b->bs);
> > >   if ((b->perm & a->shared_perm) == b->perm) {
> > >   return true;
> > >   }
> > > -perm_names = bdrv_perm_names(b->perm & ~a->shared_perm);
> > > -user = bdrv_child_user_desc(a);
> > > -error_setg(errp, "Conflicts with use by %s as '%s', which does not "
> > > -   "allow '%s' on %s",
> > > -   user, a->name, perm_names, bdrv_get_node_name(b->bs));
> > > +a_user = bdrv_child_user_desc(a);
> > > +a_against = bdrv_perm_names(b->perm & ~a->shared_perm);
> > > +
> > > +b_user = bdrv_child_user_desc(b);
> > > +b_perm = bdrv_perm_names(b->perm);
> > > +error_setg(errp, "Permission conflict on node '%s': %s wants to use 
> > > it as "
> > > +   "'%s', which requires these permissions: %s. On the other 
> > > hand %s "
> > > +   "wants to use it as '%s', which doesn't share: %s",
> > > +   bdrv_get_node_name(b->bs),
> > > +   b_user, b->name, b_perm, a_user, a->name, a_against);
> > 
> > I think the combination of a_against and b_perm is confusing to report
> > because one is the intersection of permissions (i.e. only the
> > permissions that actually conflict) and the other the full list of
> > unshared permissions.
> > 
> > We could report both the full list of required permissions (which is
> > what your current error message claims to report) and of unshared
> > permissions. I'm not sure if there is actually any use for this
> > information.
> > 
> > The other option that would feel consistent is to report only the
> > conflicting permissions, and report them only once because they are the
> > same for both sides.
> > 
> 
> Agreed.
> 
> So, what about:
> 
>   error_setg(errp, "Permission conflict on node '%s": permissions %s are both 
> required by %s (%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), 
> a_against, b_user, b->name, a_user, a->name);

I'm not sure if I'm happy with the child names simply in parentheses,
but I don't have a good alternative. I was thinking something like
"(node used as %s)", but while writing down the example below, that
turned out confusing because a_user and b_user can refer to nodes, too.

"permissions '%s'" with single quotes might be preferable, too.

So a real error message from the current version of the patch is:

Permission conflict on node 'base': node 'other' wants to use it as
'image', which requires these permissions: write. On the other hand
node 'source' wants to use it as 'image', which doesn't share: write

It would then become:

Permission conflict on node 'base': permissions 'write' are both
required by node 'other' (image) and unshared by 'source' (image).

Looks like an improvement to me, but if anyone has a good idea what to
do about the unclear meaning of the parentheses, I would be happy to
hear suggestions.

Kevin

> > >   return false;
> > >   }
> > > diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
> > > index c9397bfc44..92f3cc1ed5 100644
> > > --- a/tests/qemu-iotests/283.out
> > > +++ b/tests/qemu-iotests/

Re: [PATCH v2 2/6] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

[Paolo Bonzini]

On 31/05/21 15:59, Kevin Wolf wrote:

Apparently the motivation for Maxim's patch was, if I'm reading the
description correctly, that it affected non-sg cases by imposing
unnecessary restrictions. I see that patch 1 changed the max_iov part so
that it won't affect non-sg cases any more, but max_transfer could still
be more restricted than necessary, no?


Indeed the kernel puts no limit at all, but especially with O_DIRECT we
probably benefit from avoiding the moral equivalent of "bufferbloat".


Yeah, that sounds plausible, but on the other hand the bug report Maxim
addressed was about performance issues related to buffer sizes being too
small. So even if we want to have some limit, max_transfer of the host
device is probably not the right one for the general case.


Yeah, for a simple dd with O_DIRECT there is no real max_transfer, and 
if you are willing to allocate a big enough buffer.  Quick test on my 
laptop, reading 12.5 GiB:


   163840   9.46777s
   327680   9.41480s
   520192   9.39520s (max_iov * 4K)
   614400   9.06289s
   655360   8.85762s
   1310720  8.75502s
   2621440  8.26522s
   5242880  7.88319s
   10485760 7.66751s
   20971520 7.42627s

In practice using blktrace shows that virtual address space is 
fragmented enough that the cap for I/O operations is not max_transfer 
but max_iov * 4096 (as was before the series)...  and yet the benefit 
effectively *begins* there because it's where the cost of the system 
calls is amortized over multiple kernel<->disk communications.


Things are probably more complicated if more than one I/O is in flight, 
and with async I/O instead of read/write, but still a huge part of 
performance is seemingly the cost of system calls (not just the context 
switch, also pinning the I/O buffer and all other ancillary costs).


So the solution is probably to add a max_hw_transfer limit in addition 
to max_transfer, and have max_hw_iov instead of max_iov to match.


Paolo

Re: [PATCH v2 5/5] block: improve permission conflict error message

[Vladimir Sementsov-Ogievskiy]

31.05.2021 19:35, Kevin Wolf wrote:

Am 31.05.2021 um 18:18 hat Vladimir Sementsov-Ogievskiy geschrieben:

31.05.2021 19:07, Kevin Wolf wrote:

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:

Now permissions are updated as follows:
   1. do graph modifications ignoring permissions
   2. do permission update

   (of course, we rollback [1] if [2] fails)

So, on stage [2] we can't say which users are "old" and which are
"new" and exist only since [1]. And current error message is a bit
outdated. Let's improve it, to make everything clean.

While being here, add also a comment and some good assertions.

iotests 283, 307, qsd-jobs outputs are updated.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
   block.c   | 29 ---
   tests/qemu-iotests/283.out|  2 +-
   tests/qemu-iotests/307.out|  2 +-
   tests/qemu-iotests/tests/qsd-jobs.out |  2 +-
   4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 2f73523285..354438d918 100644
--- a/block.c
+++ b/block.c
@@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c)
   return c->klass->get_parent_desc(c);
   }
+/*
+ * Check that @a allows everything that @b needs. @a and @b must reference same
+ * child node.
+ */
   static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
   {
-g_autofree char *user = NULL;
-g_autofree char *perm_names = NULL;
+g_autofree char *a_user = NULL;
+g_autofree char *a_against = NULL;
+g_autofree char *b_user = NULL;
+g_autofree char *b_perm = NULL;
+
+assert(a->bs);
+assert(a->bs == b->bs);
   if ((b->perm & a->shared_perm) == b->perm) {
   return true;
   }
-perm_names = bdrv_perm_names(b->perm & ~a->shared_perm);
-user = bdrv_child_user_desc(a);
-error_setg(errp, "Conflicts with use by %s as '%s', which does not "
-   "allow '%s' on %s",
-   user, a->name, perm_names, bdrv_get_node_name(b->bs));
+a_user = bdrv_child_user_desc(a);
+a_against = bdrv_perm_names(b->perm & ~a->shared_perm);
+
+b_user = bdrv_child_user_desc(b);
+b_perm = bdrv_perm_names(b->perm);
+error_setg(errp, "Permission conflict on node '%s': %s wants to use it as "
+   "'%s', which requires these permissions: %s. On the other hand %s 
"
+   "wants to use it as '%s', which doesn't share: %s",
+   bdrv_get_node_name(b->bs),
+   b_user, b->name, b_perm, a_user, a->name, a_against);


I think the combination of a_against and b_perm is confusing to report
because one is the intersection of permissions (i.e. only the
permissions that actually conflict) and the other the full list of
unshared permissions.

We could report both the full list of required permissions (which is
what your current error message claims to report) and of unshared
permissions. I'm not sure if there is actually any use for this
information.

The other option that would feel consistent is to report only the
conflicting permissions, and report them only once because they are the
same for both sides.



Agreed.

So, what about:

   error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s 
(%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), a_against, b_user, b->name, 
a_user, a->name);


I'm not sure if I'm happy with the child names simply in parentheses,
but I don't have a good alternative. I was thinking something like
"(node used as %s)", but while writing down the example below, that
turned out confusing because a_user and b_user can refer to nodes, too.

"permissions '%s'" with single quotes might be preferable, too.

So a real error message from the current version of the patch is:

 Permission conflict on node 'base': node 'other' wants to use it as
 'image', which requires these permissions: write. On the other hand
 node 'source' wants to use it as 'image', which doesn't share: write

It would then become:

 Permission conflict on node 'base': permissions 'write' are both
 required by node 'other' (image) and unshared by 'source' (image).

Looks like an improvement to me, but if anyone has a good idea what to
do about the unclear meaning of the parentheses, I would be happy to
hear suggestions.



The only idea I have is duplicating (hmm, "triplicating" is an existing word?) 
the node of conflict:

bs_n = bdrv_get_node_name(b->bs);

error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s 
(uses node '%s' as '%s' child) and unshared by %s (uses node '%s' as '%s' child).", bs_n, 
a_against, b_user, bs_n, b->name, a_user, bs_n, a->name);

--
Best regards,
Vladimir

Re: [PATCH v2 14/33] block/copy-before-write: bdrv_cbw_append(): replace child at last

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

Refactor the function to replace child at last. Thus we don't need to
revert it and code is simplified.

block-copy state initilization being done before replacing the child


still *initialization

Max


doesn't need any drained section.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
  block/copy-before-write.c | 33 +++--
  1 file changed, 11 insertions(+), 22 deletions(-)

Re: [PATCH] oslib-posix: Remove OpenBSD workaround for fcntl("/dev/null", F_SETFL, O_NONBLOCK) failure

[Paolo Bonzini]

On 01/04/21 19:34, Brad Smith wrote:

OpenBSD prior to 6.3 required a workaround to utilize fcntl(F_SETFL) on memory
devices.

Since modern verions of OpenBSD that are only officialy supported and buildable
on do not have this issue I am garbage collecting this workaround.


Signed-off-by: Brad Smith 

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 36820fec16..7b4bec1402 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -273,17 +273,6 @@ int qemu_try_set_nonblock(int fd)
  return -errno;
  }
  if (fcntl(fd, F_SETFL, f | O_NONBLOCK) == -1) {
-#ifdef __OpenBSD__
-/*
- * Previous to OpenBSD 6.3, fcntl(F_SETFL) is not permitted on
- * memory devices and sets errno to ENODEV.
- * It's OK if we fail to set O_NONBLOCK on devices like /dev/null,
- * because they will never block anyway.
- */
-if (errno == ENODEV) {
-return 0;
-}
-#endif
  return -errno;
  }
  return 0;



Queued, thanks.

Paolo

Re: [PATCH v2 15/33] block/copy-before-write: introduce cbw_init()

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

Move part of bdrv_cbw_append() to new function cbw_open(). It's an
intermediate step for adding noramal .bdrv_open() handler to the


Didn’t notice this in v1, but: *normal

Max


filter. With this commit no logic is changed, but we have a function
which will be turned into .bdrv_open() handler in future commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
  block/copy-before-write.c | 69 +++
  1 file changed, 41 insertions(+), 28 deletions(-)
  

[PATCH v1 0/6] support dirtyrate at the granualrity of vcpu

[huangy81]

From: Hyman Huang(黄勇) 

Since the Dirty Ring on QEMU part has been merged recently, how to use
this feature is under consideration.

In the scene of migration, it is valuable to provide a more accurante
interface to track dirty memory than existing one, so that the upper
layer application can make a wise decision, or whatever. More importantly,
dirtyrate info at the granualrity of vcpu could provide a possibility to
make migration convergent by imposing restriction on vcpu. With Dirty
Ring, we can calculate dirtyrate efficiently and cheaply.

The old interface implemented by sampling pages, it consumes cpu 
resource, and the larger guest memory size become, the more cpu resource
it consumes, namely, hard to scale. New interface has no such drawback.

Please review, thanks !

Best Regards !

Hyman Huang(黄勇) (6):
  KVM: add kvm_dirty_ring_enabled function
  KVM: introduce dirty_pages into CPUState
  migration/dirtyrate: add vcpu option for qmp calc-dirty-rate
  migration/dirtyrate: adjust struct DirtyRateStat
  migration/dirtyrate: check support of calculation for vcpu
  migration/dirtyrate: implement dirty-ring dirtyrate calculation

 accel/kvm/kvm-all.c|  11 +++
 include/hw/core/cpu.h  |   2 +
 include/sysemu/kvm.h   |   1 +
 migration/dirtyrate.c  | 179 +
 migration/dirtyrate.h  |  19 -
 migration/trace-events |   1 +
 qapi/migration.json|  28 ++-
 7 files changed, 222 insertions(+), 19 deletions(-)

-- 
2.24.3

Re: [PATCH v2 18/33] block/copy-before-write: cbw_init(): use options

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

One more step closer to .bdrv_open(): use options instead of plain
arguments. Move to bdrv_open_child() calls, native for drive open
handlers.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/copy-before-write.c | 40 +--
  1 file changed, 26 insertions(+), 14 deletions(-)



Reviewed-by: Max Reitz 

[PATCH v1 3/6] migration/dirtyrate: add vcpu option for qmp calc-dirty-rate

[huangy81]

From: Hyman Huang(黄勇) 

calculate dirtyrate for each vcpu if vcpu is true, add the
dirtyrate of each vcpu to the return value also.

Signed-off-by: Hyman Huang(黄勇) 
---
 migration/dirtyrate.c |  5 -
 migration/dirtyrate.h |  1 +
 qapi/migration.json   | 28 ++--
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index ccb98147e8..3c1a824a41 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -383,7 +383,10 @@ void *get_dirtyrate_thread(void *arg)
 return NULL;
 }
 
-void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
+void qmp_calc_dirty_rate(int64_t calc_time,
+ bool has_vcpu,
+ bool vcpu,
+ Error **errp)
 {
 static struct DirtyRateConfig config;
 QemuThread thread;
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 6ec429534d..f20dd52d77 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -38,6 +38,7 @@
 struct DirtyRateConfig {
 uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
 int64_t sample_period_seconds; /* time duration between two sampling */
+bool vcpu; /* calculate dirtyrate for each vcpu using dirty ring */
 };
 
 /*
diff --git a/qapi/migration.json b/qapi/migration.json
index 7a5bdf9a0d..896ebcb93b 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1708,6 +1708,21 @@
 { 'event': 'UNPLUG_PRIMARY',
   'data': { 'device-id': 'str' } }
 
+##
+# @DirtyRateVcpu:
+#
+# Dirty rate of vcpu.
+#
+# @id: vcpu index.
+#
+# @dirty-rate: dirty rate.
+#
+# Since: 6.1
+#
+##
+{ 'struct': 'DirtyRateVcpu',
+  'data': { 'id': 'int', 'dirty-rate': 'int64' } }
+
 ##
 # @DirtyRateStatus:
 #
@@ -1740,6 +1755,10 @@
 #
 # @calc-time: time in units of second for sample dirty pages
 #
+# @vcpu: calculate dirtyrate for each vcpu (Since 6.1)
+#
+# @vcpu-dirty-rate: dirtyrate for each vcpu (Since 6.1)
+#
 # Since: 5.2
 #
 ##
@@ -1747,7 +1766,9 @@
   'data': {'*dirty-rate': 'int64',
'status': 'DirtyRateStatus',
'start-time': 'int64',
-   'calc-time': 'int64'} }
+   'calc-time': 'int64',
+   '*vcpu': 'bool',
+   '*vcpu-dirty-rate': [ 'DirtyRateVcpu' ] } }
 
 ##
 # @calc-dirty-rate:
@@ -1756,13 +1777,16 @@
 #
 # @calc-time: time in units of second for sample dirty pages
 #
+# @vcpu: calculate vcpu dirty rate if true, the default value is
+#false (since 6.1)
+#
 # Since: 5.2
 #
 # Example:
 #   {"command": "calc-dirty-rate", "data": {"calc-time": 1} }
 #
 ##
-{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
+{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64', '*vcpu': 
'bool'} }
 
 ##
 # @query-dirty-rate:
-- 
2.24.3

[PATCH v1 1/6] KVM: add kvm_dirty_ring_enabled function

[huangy81]

From: Hyman Huang(黄勇) 

introduce kvm_dirty_ring_enabled to show if kvm-reaper is working.
dirtyrate thread could use it to check if calculation can base on
dirty ring feature.

Signed-off-by: Hyman Huang(黄勇) 
---
 accel/kvm/kvm-all.c  | 5 +
 include/sysemu/kvm.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c7ec538850..2e96b77b31 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2293,6 +2293,11 @@ bool kvm_vcpu_id_is_valid(int vcpu_id)
 return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s);
 }
 
+bool kvm_dirty_ring_enabled(void)
+{
+return kvm_state->kvm_dirty_ring_size ? true : false;
+}
+
 static int kvm_init(MachineState *ms)
 {
 MachineClass *mc = MACHINE_GET_CLASS(ms);
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a1ab1ee12d..7b22aeb6ae 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -547,4 +547,5 @@ bool kvm_cpu_check_are_resettable(void);
 
 bool kvm_arch_cpu_check_are_resettable(void);
 
+bool kvm_dirty_ring_enabled(void);
 #endif
-- 
2.24.3

[PATCH v1 5/6] migration/dirtyrate: check support of calculation for vcpu

[huangy81]

From: Hyman Huang(黄勇) 

vcpu method only works when kvm dirty ring is enabled, use
kvm_dirty_ring_enabled to probe if dirty ring is enabled.

Signed-off-by: Hyman Huang(黄勇) 
---
 migration/dirtyrate.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 7952eb6117..da6500c8ec 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -16,6 +16,7 @@
 #include "cpu.h"
 #include "exec/ramblock.h"
 #include "qemu/rcu_queue.h"
+#include "sysemu/kvm.h"
 #include "qapi/qapi-commands-migration.h"
 #include "ram.h"
 #include "trace.h"
@@ -415,6 +416,14 @@ void qmp_calc_dirty_rate(int64_t calc_time,
 return;
 }
 
+/*
+ * Vcpu method only works when kvm dirty ring is enabled.
+ */
+if (has_vcpu && vcpu && !kvm_dirty_ring_enabled()) {
+error_setg(errp, "kvm dirty ring is disabled, use sample method.");
+return;
+}
+
 /*
  * Init calculation state as unstarted.
  */
@@ -427,6 +436,7 @@ void qmp_calc_dirty_rate(int64_t calc_time,
 
 config.sample_period_seconds = calc_time;
 config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
+config.vcpu = has_vcpu ? vcpu : false;
 qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
(void *)&config, QEMU_THREAD_DETACHED);
 }
-- 
2.24.3

[PATCH v1 4/6] migration/dirtyrate: adjust struct DirtyRateStat

[huangy81]

From: Hyman Huang(黄勇) 

use union to store stat data of two mutual exclusive methods.

Signed-off-by: Hyman Huang(黄勇) 
---
 migration/dirtyrate.c | 32 
 migration/dirtyrate.h | 18 +++---
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 3c1a824a41..7952eb6117 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -78,31 +78,39 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
 return info;
 }
 
-static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
+static void init_dirtyrate_stat(int64_t start_time,
+int64_t calc_time,
+struct DirtyRateConfig config)
 {
-DirtyStat.total_dirty_samples = 0;
-DirtyStat.total_sample_count = 0;
-DirtyStat.total_block_mem_MB = 0;
 DirtyStat.dirty_rate = -1;
 DirtyStat.start_time = start_time;
 DirtyStat.calc_time = calc_time;
+
+if (config.vcpu) {
+DirtyStat.method.vcpu.nvcpu = -1;
+DirtyStat.method.vcpu.rates = NULL;
+} else {
+DirtyStat.method.vm.total_dirty_samples = 0;
+DirtyStat.method.vm.total_sample_count = 0;
+DirtyStat.method.vm.total_block_mem_MB = 0;
+}
 }
 
 static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
 {
-DirtyStat.total_dirty_samples += info->sample_dirty_count;
-DirtyStat.total_sample_count += info->sample_pages_count;
+DirtyStat.method.vm.total_dirty_samples += info->sample_dirty_count;
+DirtyStat.method.vm.total_sample_count += info->sample_pages_count;
 /* size of total pages in MB */
-DirtyStat.total_block_mem_MB += (info->ramblock_pages *
+DirtyStat.method.vm.total_block_mem_MB += (info->ramblock_pages *
  TARGET_PAGE_SIZE) >> 20;
 }
 
 static void update_dirtyrate(uint64_t msec)
 {
 uint64_t dirtyrate;
-uint64_t total_dirty_samples = DirtyStat.total_dirty_samples;
-uint64_t total_sample_count = DirtyStat.total_sample_count;
-uint64_t total_block_mem_MB = DirtyStat.total_block_mem_MB;
+uint64_t total_dirty_samples = DirtyStat.method.vm.total_dirty_samples;
+uint64_t total_sample_count = DirtyStat.method.vm.total_sample_count;
+uint64_t total_block_mem_MB = DirtyStat.method.vm.total_block_mem_MB;
 
 dirtyrate = total_dirty_samples * total_block_mem_MB *
 1000 / (total_sample_count * msec);
@@ -315,7 +323,7 @@ static bool compare_page_hash_info(struct RamblockDirtyInfo 
*info,
 update_dirtyrate_stat(block_dinfo);
 }
 
-if (DirtyStat.total_sample_count == 0) {
+if (DirtyStat.method.vm.total_sample_count == 0) {
 return false;
 }
 
@@ -371,7 +379,7 @@ void *get_dirtyrate_thread(void *arg)
 
 start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
 calc_time = config.sample_period_seconds;
-init_dirtyrate_stat(start_time, calc_time);
+init_dirtyrate_stat(start_time, calc_time, config);
 
 calculate_dirtyrate(config);
 
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index f20dd52d77..3ab8e81f42 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -54,16 +54,28 @@ struct RamblockDirtyInfo {
 uint32_t *hash_result; /* array of hash result for sampled pages */
 };
 
+typedef struct SampleVMStat {
+uint64_t total_dirty_samples; /* total dirty sampled page */
+uint64_t total_sample_count; /* total sampled pages */
+uint64_t total_block_mem_MB; /* size of total sampled pages in MB */
+} SampleVMStat;
+
+typedef struct VcpuStat {
+int nvcpu; /* number of vcpu */
+DirtyRateVcpu *rates; /* array of dirty rate for each vcpu */
+} VcpuStat;
+
 /*
  * Store calculation statistics for each measure.
  */
 struct DirtyRateStat {
-uint64_t total_dirty_samples; /* total dirty sampled page */
-uint64_t total_sample_count; /* total sampled pages */
-uint64_t total_block_mem_MB; /* size of total sampled pages in MB */
 int64_t dirty_rate; /* dirty rate in MB/s */
 int64_t start_time; /* calculation start time in units of second */
 int64_t calc_time; /* time duration of two sampling in units of second */
+union {
+SampleVMStat vm;
+VcpuStat vcpu;
+} method;
 };
 
 void *get_dirtyrate_thread(void *arg);
-- 
2.24.3

[PATCH v1 2/6] KVM: introduce dirty_pages into CPUState

[huangy81]

From: Hyman Huang(黄勇) 

dirty_pages is used to calculate dirtyrate via dirty ring, when enabled,
kvm-reaper will increase the dirty pages after gfns being dirtied.

Signed-off-by: Hyman Huang(黄勇) 
---
 accel/kvm/kvm-all.c   | 6 ++
 include/hw/core/cpu.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 2e96b77b31..52cba1b094 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -506,6 +506,9 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
 }
 }
 
+cpu->dirty_pages = 0;
+cpu->stat_dirty_pages = false;
+
 ret = kvm_arch_init_vcpu(cpu);
 if (ret < 0) {
 error_setg_errno(errp, -ret,
@@ -739,6 +742,9 @@ static uint32_t kvm_dirty_ring_reap_one(KVMState *s, 
CPUState *cpu)
  cur->offset);
 dirty_gfn_set_collected(cur);
 trace_kvm_dirty_ring_page(cpu->cpu_index, fetch, cur->offset);
+if (cpu->stat_dirty_pages) {
+cpu->dirty_pages++;
+}
 fetch++;
 count++;
 }
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 044f668a6e..973c193501 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -375,6 +375,8 @@ struct CPUState {
 struct kvm_run *kvm_run;
 struct kvm_dirty_gfn *kvm_dirty_gfns;
 uint32_t kvm_fetch_index;
+uint64_t dirty_pages;
+bool stat_dirty_pages;
 
 /* Used for events with 'vcpu' and *without* the 'disabled' properties */
 DECLARE_BITMAP(trace_dstate_delayed, CPU_TRACE_DSTATE_MAX_EVENTS);
-- 
2.24.3

Re: [PATCH v2 00/33] block: publish backup-top filter

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

v2:
01-02: new
03: don't bother with supporting empty child: we should never have such
 at this point
05: add comment
06: keep checking conflict with global
 add realized_set_allowed to qdev_prop_drive_iothread
07: improve cbw_cbw() name
 improve commit message
10: rebased on unchanged backup_calculate_cluster_size(). keep r-b  CHECK ME


I checked you! It’s ok. :)

(and btw, I’ll look at the rest tomorrow)


12: new
13: drop extra bdrv_unref()
18: add compress local variable
 add comment about x-deprecated-compress
19: new, replacement for "[PATCH 17/21] block/block-copy: switch to fully set bitmap 
by default"
22: improve qapi documentation
23-33: test: a lot of refactoring

[PATCH v1 6/6] migration/dirtyrate: implement dirty-ring dirtyrate calculation

[huangy81]

From: Hyman Huang(黄勇) 

use dirty ring feature to implement dirtyrate calculation.
to enable it, set vcpu option as true in qmp calc-dirty-rate.

Signed-off-by: Hyman Huang(黄勇) 
---
 migration/dirtyrate.c  | 146 ++---
 migration/trace-events |   1 +
 2 files changed, 139 insertions(+), 8 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index da6500c8ec..028c11d117 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -16,14 +16,22 @@
 #include "cpu.h"
 #include "exec/ramblock.h"
 #include "qemu/rcu_queue.h"
+#include "qemu/main-loop.h"
 #include "sysemu/kvm.h"
 #include "qapi/qapi-commands-migration.h"
 #include "ram.h"
 #include "trace.h"
 #include "dirtyrate.h"
 
+typedef enum {
+CALC_NONE = 0,
+CALC_DIRTY_RING,
+CALC_SAMPLE_PAGES,
+} CalcMethod;
+
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
 static struct DirtyRateStat DirtyStat;
+static CalcMethod last_method = CALC_NONE;
 
 static int64_t set_sample_page_period(int64_t msec, int64_t initial_time)
 {
@@ -64,6 +72,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
 {
 int64_t dirty_rate = DirtyStat.dirty_rate;
 struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
+DirtyRateVcpuList *head = NULL, **tail = &head;
 
 if (qatomic_read(&CalculatingState) == DIRTY_RATE_STATUS_MEASURED) {
 info->has_dirty_rate = true;
@@ -73,6 +82,22 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
 info->status = CalculatingState;
 info->start_time = DirtyStat.start_time;
 info->calc_time = DirtyStat.calc_time;
+info->has_vcpu = true;
+
+if (last_method == CALC_DIRTY_RING) {
+int i = 0;
+info->vcpu = true;
+info->has_vcpu_dirty_rate = true;
+for (i = 0; i < DirtyStat.method.vcpu.nvcpu; i++) {
+DirtyRateVcpu *rate = g_malloc0(sizeof(DirtyRateVcpu));
+rate->id = DirtyStat.method.vcpu.rates[i].id;
+rate->dirty_rate = DirtyStat.method.vcpu.rates[i].dirty_rate;
+QAPI_LIST_APPEND(tail, rate);
+}
+info->vcpu_dirty_rate = head;
+} else {
+info->vcpu = false;
+}
 
 trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
 
@@ -87,13 +112,29 @@ static void init_dirtyrate_stat(int64_t start_time,
 DirtyStat.start_time = start_time;
 DirtyStat.calc_time = calc_time;
 
-if (config.vcpu) {
-DirtyStat.method.vcpu.nvcpu = -1;
-DirtyStat.method.vcpu.rates = NULL;
-} else {
-DirtyStat.method.vm.total_dirty_samples = 0;
-DirtyStat.method.vm.total_sample_count = 0;
-DirtyStat.method.vm.total_block_mem_MB = 0;
+switch (last_method) {
+case CALC_NONE:
+case CALC_SAMPLE_PAGES:
+if (config.vcpu) {
+DirtyStat.method.vcpu.nvcpu = -1;
+DirtyStat.method.vcpu.rates = NULL;
+} else {
+DirtyStat.method.vm.total_dirty_samples = 0;
+DirtyStat.method.vm.total_sample_count = 0;
+DirtyStat.method.vm.total_block_mem_MB = 0;
+}
+break;
+case CALC_DIRTY_RING:
+if (!config.vcpu) {
+g_free(DirtyStat.method.vcpu.rates);
+DirtyStat.method.vcpu.rates = NULL;
+DirtyStat.method.vm.total_dirty_samples = 0;
+DirtyStat.method.vm.total_sample_count = 0;
+DirtyStat.method.vm.total_block_mem_MB = 0;
+}
+break;
+default:
+break;
 }
 }
 
@@ -331,7 +372,84 @@ static bool compare_page_hash_info(struct 
RamblockDirtyInfo *info,
 return true;
 }
 
-static void calculate_dirtyrate(struct DirtyRateConfig config)
+static void stat_dirtypages(CPUState *cpu, bool start)
+{
+cpu->stat_dirty_pages = start;
+}
+
+static void start_kvm_dirty_log(void)
+{
+qemu_mutex_lock_iothread();
+memory_global_dirty_log_start();
+qemu_mutex_unlock_iothread();
+}
+
+static void stop_kvm_dirty_log(void)
+{
+qemu_mutex_lock_iothread();
+memory_global_dirty_log_stop();
+qemu_mutex_unlock_iothread();
+}
+
+static int64_t do_calculate_dirtyrate_vcpu(CPUState *cpu)
+{
+uint64_t memory_size_MB;
+int64_t time_s;
+
+memory_size_MB = (cpu->dirty_pages * TARGET_PAGE_SIZE) >> 20;
+time_s = DirtyStat.calc_time;
+
+return memory_size_MB / time_s;
+}
+
+static void calculate_dirtyrate_vcpu(struct DirtyRateConfig config)
+{
+CPUState *cpu;
+int64_t msec = 0;
+int64_t start_time;
+uint64_t dirtyrate = 0;
+uint64_t dirtyrate_sum = 0;
+int nvcpu, i = 0;
+
+CPU_FOREACH(cpu) {
+stat_dirtypages(cpu, true);
+nvcpu++;
+}
+
+DirtyStat.method.vcpu.nvcpu = nvcpu;
+
+if (last_method != CALC_DIRTY_RING) {
+DirtyStat.method.vcpu.rates =
+g_malloc0(sizeof(DirtyRateVcpu) * nvcpu);
+}
+
+start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+DirtyStat.start_time = start_time / 1000;
+
+start

Re: [PATCH v2 19/33] block/copy-before-write: initialize block-copy bitmap

[Max Reitz]

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

We are going to publish copy-before-write filter to be used in separate
of backup. Future step would support bitmap for the filter. But let's
start from full set bitmap.

We have to modify backup, as bitmap is first initialized by
copy-before-write filter, and then backup modifies it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/backup.c| 16 +++-
  block/copy-before-write.c |  4 
  2 files changed, 11 insertions(+), 9 deletions(-)



Reviewed-by: Max Reitz 

Re: [PATCH v2 00/33] block: publish backup-top filter

[Vladimir Sementsov-Ogievskiy]

31.05.2021 20:11, Max Reitz wrote:

On 20.05.21 16:21, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

v2:
01-02: new
03: don't bother with supporting empty child: we should never have such
 at this point
05: add comment
06: keep checking conflict with global
 add realized_set_allowed to qdev_prop_drive_iothread
07: improve cbw_cbw() name
 improve commit message
10: rebased on unchanged backup_calculate_cluster_size(). keep r-b  CHECK ME


I checked you! It’s ok. :)

(and btw, I’ll look at the rest tomorrow)


Thanks)

Actually, "CHECK ME" was for myself, and "ME" is a patch, not me) I forget to 
drop it.. Interesting, did I forget to check




12: new
13: drop extra bdrv_unref()
18: add compress local variable
 add comment about x-deprecated-compress
19: new, replacement for "[PATCH 17/21] block/block-copy: switch to fully set bitmap 
by default"
22: improve qapi documentation
23-33: test: a lot of refactoring





--
Best regards,
Vladimir

RE: [PATCH v3 3/4] target/ppc: removed GEN_OPCODE decision tree

[Luis Fernando Fujita Pires]

From: Bruno Larsen (billionai) 
>  .opc1 = op1, 
>  \
> @@ -1353,7 +1352,6 @@ typedef struct opcode_t {
>  .type = _typ,
>  \
>  .type2 = _typ2,  
>  \
>  .handler = &gen_##name,  
>  \
> -.oname = stringify(name),
>  \
>  },   
>  \
>  .oname = stringify(name),
>  \
>  }

I guess you could remove oname from opcode_t too, now that it's not being used 
anywhere.

--
Luis Pires
Instituto de Pesquisas ELDORADO 
Aviso Legal - Disclaimer 

RE: [PATCH v3 4/4] target/ppc: removed all mentions to PPC_DUMP_CPU

[Luis Fernando Fujita Pires]

From: Bruno Larsen (billionai) 
> This feature will no longer be useful as ppc moves to using decotree for TCG.
> And building with it enabled is no longer possible, due to changes in
> opc_handler_t. Since the last commit that mentions it happened in 2014, I 
> think
> it is safe to remove it.
> 
> Signed-off-by: Bruno Larsen (billionai) 
> ---
>  target/ppc/cpu_init.c  | 205 -
>  target/ppc/internal.h  |   2 -
>  target/ppc/translate.c | 105 -
>  3 files changed, 312 deletions(-)

I believe you lost Richard's review for this one. In addition to approving it, 
he also noted the a typo in the commit message ("decotree" -> "decodetree").

Reviewed-by: Luis Pires 

--
Luis Pires
Instituto de Pesquisas ELDORADO 
Aviso Legal - Disclaimer 

Re: GSoC 2021: Request for WIKI account to update project details

[Niteesh G. S.]

Hello Stefan,

Thank you.


On Mon, May 31, 2021 at 8:00 PM Stefan Hajnoczi  wrote:

> On Thu, May 27, 2021 at 12:35:48AM +0530, Niteesh G. S. wrote:
> > I kindly request the admin to grant me access to the wiki through the
> below
> > credentials
>
> Hi Niteesh,
> I have created an account for you and sent you the details privately.
>
> Stefan
>

Re: [PATCH v3 2/4] HMP: added info cpustats to removed_features.rst

[Bruno Piazera Larsen]

On 31/05/2021 11:56, Bruno Larsen (billionai) wrote:

Documented the removal of the HMP command info cpustats

Signed-off-by: Bruno Larsen (billionai) 


Oops, I seem to have dropped a few tags:

Reviewed-by: Luis Pires
Reviewed-by: Lucas Mateus
Reviewed-by: Greg Kurz


---
  docs/system/removed-features.rst | 5 +
  1 file changed, 5 insertions(+)

diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 5a462ac568..2feae41089 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -249,6 +249,11 @@ Use ``migrate-set-parameters`` and ``info 
migrate-parameters`` instead.
  
  Use ``migrate-set-parameters`` instead.
  
+``info cpustats`` (removed in 6.1)

+'
+
+This command didn't produce any output already. Removed with no replacement.
+
  Guest Emulator ISAs
  ---
  

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 


Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer 

Re: [PATCH 02/11] target/ppc: moved ppc_store_sdr1 to cpu.c

[Bruno Piazera Larsen]

On 13/05/2021 00:50, David Gibson wrote:

On Wed, May 12, 2021 at 11:08:04AM -0300, Bruno Larsen (billionai) wrote:

Moved this function that is required in !TCG cases into a
common code file

The reasons it's needed by !TCG are kind of bogus, related to
weirdness in the way KVM PR works.  But it's fair not to care about
that right now, so, applied to ppc-for-6.1.
Now that the future is here, I was looking into why might the reasons be 
bogus. From what I can see, what should be happening is just storing 
what was retrieved by the kvm ioctl, right? Am I missing something?

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 


Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer 

[Bug 1905444] Re: [OSS-Fuzz] Issue 27796 in oss-fuzz: qemu:qemu-fuzz-i386-target-generic-fuzz-xhci: Stack-overflow in address_space_stl_internal

[Thomas Huth]

As mentioned by Alexander here:
https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg08637.html
this has likely been fixed by this commit here:
https://gitlab.com/qemu-project/qemu/-/commit/3c6151cd11ae7e4a7dae10f8c17ab1fe2f0a73bf
... thus I'm marking this as fixed now. If it occurs again, please open a new 
ticket in the Gitlab bug tracker. Thanks!

** Changed in: qemu
   Status: New => Fix Committed

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

Title:
  [OSS-Fuzz] Issue 27796 in oss-fuzz: qemu:qemu-fuzz-i386-target-
  generic-fuzz-xhci: Stack-overflow in address_space_stl_internal

Status in QEMU:
  Fix Committed

Bug description:
   affects qemu

  OSS-Fuzz Report: https://bugs.chromium.org/p/oss-
  fuzz/issues/detail?id=27796

  === Reproducer (build with --enable-sanitizers) ===
  cat << EOF | ./qemu-system-i386 -display none  -machine accel=qtest, \
  -m 512M -machine q35 -nodefaults \
  -drive file=null-co://,if=none,format=raw,id=disk0 \
  -device qemu-xhci,id=xhci -device usb-tablet,bus=xhci.0 \
  -qtest-log none -qtest stdio
  outl 0xcf8 0x8803
  outw 0xcfc 0x5e46
  outl 0xcf8 0x8810
  outl 0xcfc 0xff5a5e46
  write 0xff5a5020 0x6 0x0b70
  outl 0xcf8 0x8893
  outb 0xcfc 0x93
  writel 0xff5a7000 0xff5a5020
  write 0xff5a700c 0x4 0x0c0c2e58
  write 0xff5a4040 0x4 0x00d26001
  write 0xff5a4044 0x4 0x030
  EOF

  === Stack Trace ===
  ==50473==ERROR: AddressSanitizer: stack-overflow on address 0x7ffe3ec97e28 
(pc 0x55e292eac159 bp 0x7ffe3ec98670 sp 0x7ffe3ec97e30 T0)
  #0 0x55e292eac159 in __asan_memcpy (u-system-i386+0x2a0e159)
  #1 0x55e2944bc04e in flatview_do_translate softmmu/physmem.c:513:12
  #2 0x55e2944dbe90 in flatview_translate softmmu/physmem.c:563:15
  #3 0x55e2944dbe90 in address_space_translate include/exec/memory.h:2362:12
  #4 0x55e2944dbe90 in address_space_stl_internal memory_ldst.c.inc:316:10
  #5 0x55e29393d2a0 in xhci_intr_update hw/usb/hcd-xhci.c:554:13
  #6 0x55e29393efb9 in xhci_runtime_write hw/usb/hcd-xhci.c:3032:9
  #7 0x55e294230428 in memory_region_write_accessor softmmu/memory.c:484:5
  #8 0x55e29422fe63 in access_with_adjusted_size softmmu/memory.c:545:18
  #9 0x55e29422f6fc in memory_region_dispatch_write softmmu/memory.c
  #10 0x55e2944dc03c in address_space_stl_internal memory_ldst.c.inc:319:13
  #11 0x55e29393d2a0 in xhci_intr_update hw/usb/hcd-xhci.c:554:13
  #12 0x55e29393efb9 in xhci_runtime_write hw/usb/hcd-xhci.c:3032:9
  #13 0x55e294230428 in memory_region_write_accessor softmmu/memory.c:484:5
  #14 0x55e29422fe63 in access_with_adjusted_size softmmu/memory.c:545:18
  #15 0x55e29422f6fc in memory_region_dispatch_write softmmu/memory.c
  #16 0x55e2944dc03c in address_space_stl_internal memory_ldst.c.inc:319:13
  #17 0x55e29393d2a0 in xhci_intr_update hw/usb/hcd-xhci.c:554:13
  #18 0x55e29393efb9 in xhci_runtime_write hw/usb/hcd-xhci.c:3032:9

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

Re: [PATCH v6 08/26] tcg: Build ffi data structures for helpers

[Philippe Mathieu-Daudé]

Hi Richard,

On 5/3/21 1:57 AM, Richard Henderson wrote:
> Add libffi as a build requirement for TCI.
> Add libffi to the dockerfiles to satisfy that requirement.
> 
> Construct an ffi_cif structure for each unique typemask.
> Record the result in a separate hash table for later lookup;
> this allows helper_table to stay const.
> 
> Signed-off-by: Richard Henderson 
> ---
>  meson.build   |  9 ++-
>  tcg/tcg.c | 58 +++
>  tests/docker/dockerfiles/alpine.docker|  1 +
>  tests/docker/dockerfiles/centos7.docker   |  1 +
>  tests/docker/dockerfiles/centos8.docker   |  1 +
>  tests/docker/dockerfiles/debian10.docker  |  1 +
>  .../dockerfiles/fedora-i386-cross.docker  |  1 +
>  .../dockerfiles/fedora-win32-cross.docker |  1 +
>  .../dockerfiles/fedora-win64-cross.docker |  1 +
>  tests/docker/dockerfiles/fedora.docker|  1 +
>  tests/docker/dockerfiles/ubuntu.docker|  1 +
>  tests/docker/dockerfiles/ubuntu1804.docker|  1 +
>  tests/docker/dockerfiles/ubuntu2004.docker|  1 +
>  13 files changed, 77 insertions(+), 1 deletion(-)

> @@ -1135,6 +1152,47 @@ void tcg_context_init(TCGContext *s)
>  (gpointer)&all_helpers[i]);
>  }
>  
> +#ifdef CONFIG_TCG_INTERPRETER
> +/* g_direct_hash/equal for direct comparisons on uint32_t.  */

Why not use g_int_hash() then?

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé 

> +ffi_table = g_hash_table_new(NULL, NULL);
> +for (i = 0; i < ARRAY_SIZE(all_helpers); ++i) {
> +struct {
> +ffi_cif cif;
> +ffi_type *args[];
> +} *ca;
> +uint32_t typemask = all_helpers[i].typemask;
> +gpointer hash = (gpointer)(uintptr_t)typemask;
> +ffi_status status;
> +int nargs;
> +
> +if (g_hash_table_lookup(ffi_table, hash)) {
> +continue;
> +}
> +
> +/* Ignoring the return type, find the last non-zero field. */
> +nargs = 32 - clz32(typemask >> 3);
> +nargs = DIV_ROUND_UP(nargs, 3);
> +
> +ca = g_malloc0(sizeof(*ca) + nargs * sizeof(ffi_type *));
> +ca->cif.rtype = typecode_to_ffi[typemask & 7];
> +ca->cif.nargs = nargs;
> +
> +if (nargs != 0) {
> +ca->cif.arg_types = ca->args;
> +for (i = 0; i < nargs; ++i) {
> +int typecode = extract32(typemask, (i + 1) * 3, 3);
> +ca->args[i] = typecode_to_ffi[typecode];
> +}
> +}
> +
> +status = ffi_prep_cif(&ca->cif, FFI_DEFAULT_ABI, nargs,
> +  ca->cif.rtype, ca->cif.arg_types);
> +assert(status == FFI_OK);
> +
> +g_hash_table_insert(ffi_table, hash, (gpointer)&ca->cif);
> +}
> +#endif
> +
>  tcg_target_init(s);
>  process_op_defs(s);

[target/ppc] excp_helper.c and mmu_helper.c cleanup

[Lucas Mateus Martins Araujo e Castro]

Hi everyone,

I'm working on cleaning up some of the changes to enable the disable-tcg 
option on PPC, right now focusing on target/ppc/excp_helper.c and 
target/ppc/mmu_helper.c as these files have functions that are needed in 
a !TCG build but also contains code that doesn't compile in a !TCG 
build, and currently that is dealt with #ifdef.


For excp_helper.c I moved all exception handling functions to a new file 
(named target/ppc/excp_handler.c for now) and left only the helpers in 
it, and changed meson.build to always compile the new file and only 
compile the file with the helpers in a build with TCG.


For mmu_helper.c the idea is to move all the code inside #ifdef 
CONFIG_TCG to another file that shouldn't be compiled in a !TCG build. 
But these changes are based on Richard Henderson's patch, so it depends 
if they'll be applied as is or there will be another version.


Also I'm looking into the possibility of not compiling 
ppc_tlb_invalidate_all in mmu_helper.c, but that's only possible if this 
function is not used in a !TCG build, does anyone know if this function 
is used in some corner case when running with KVM?


Any opinion on these changes?

--
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO 


Departamento Computação Embarcada
Estagiario
Aviso Legal - Disclaimer 

Re: [PATCH] tests: Boot and halt a Linux guest on the Raspberry Pi 2 machine

[Wainer dos Santos Moschetta]

On 5/31/21 8:38 AM, Philippe Mathieu-Daudé wrote:

Add a test booting and quickly shutdown a raspi2 machine,
to test the power management model:

(1/1) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_raspi2_initrd:
   console: [0.00] Booting Linux on physical CPU 0xf00
   console: [0.00] Linux version 4.14.98-v7+ (dom@dom-XPS-13-9370) (gcc 
version 4.9.3 (crosstool-NG crosstool-ng-1.22.0-88-g8460611)) #1200 SMP Tue Feb 
12 20:27:48 GMT 2019
   console: [0.00] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), 
cr=10c5387d
   console: [0.00] CPU: div instructions available: patching division 
code
   console: [0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT 
aliasing instruction cache
   console: [0.00] OF: fdt: Machine model: Raspberry Pi 2 Model B
   ...
   console: Boot successful.
   console: cat /proc/cpuinfo
   console: / # cat /proc/cpuinfo
   ...
   console: processor  : 3
   console: model name : ARMv7 Processor rev 5 (v7l)
   console: BogoMIPS   : 125.00
   console: Features   : half thumb fastmult vfp edsp neon vfpv3 tls vfpv4 
idiva idivt vfpd32 lpae evtstrm
   console: CPU implementer: 0x41
   console: CPU architecture: 7
   console: CPU variant: 0x0
   console: CPU part   : 0xc07
   console: CPU revision   : 5
   console: Hardware   : BCM2835
   console: Revision   : 
   console: Serial : 
   console: cat /proc/iomem
   console: / # cat /proc/iomem
   console: -3bff : System RAM
   console: 8000-00af : Kernel code
   console: 00c0-00d468ef : Kernel data
   console: 3f006000-3f006fff : dwc_otg
   console: 3f007000-3f007eff : /soc/dma@7e007000
   console: 3f00b880-3f00b8bf : /soc/mailbox@7e00b880
   console: 3f10-3f100027 : /soc/watchdog@7e10
   console: 3f101000-3f102fff : /soc/cprman@7e101000
   console: 3f20-3f2000b3 : /soc/gpio@7e20
   PASS (24.59 s)
   RESULTS: PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
CANCEL 0
   JOB TIME   : 25.02 s

Signed-off-by: Philippe Mathieu-Daudé 
---
Based-on: <162137039825.30884.244582579824080919...@git.sr.ht>
---
  tests/acceptance/boot_linux_console.py | 43 ++
  1 file changed, 43 insertions(+)


Even though I did not patch my sources with the fix, I ran the test and 
it failed on the VM's shutdown (as expected, I suppose). The test code 
looks good to me, so:


Reviewed-by: Wainer dos Santos Moschetta 



diff --git a/tests/acceptance/boot_linux_console.py 
b/tests/acceptance/boot_linux_console.py
index 276a53f1464..19d328203c7 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -16,6 +16,7 @@
  from avocado import skip
  from avocado import skipUnless
  from avocado_qemu import Test
+from avocado_qemu import exec_command
  from avocado_qemu import exec_command_and_wait_for_pattern
  from avocado_qemu import interrupt_interactive_console_until_pattern
  from avocado_qemu import wait_for_console_pattern
@@ -467,6 +468,48 @@ def test_arm_raspi2_uart0(self):
  """
  self.do_test_arm_raspi2(0)
  
+def test_arm_raspi2_initrd(self):

+"""
+:avocado: tags=arch:arm
+:avocado: tags=machine:raspi2
+"""
+deb_url = ('http://archive.raspberrypi.org/debian/'
+   'pool/main/r/raspberrypi-firmware/'
+   'raspberrypi-kernel_1.20190215-1_armhf.deb')
+deb_hash = 'cd284220b32128c5084037553db3c482426f3972'
+deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash)
+kernel_path = self.extract_from_deb(deb_path, '/boot/kernel7.img')
+dtb_path = self.extract_from_deb(deb_path, '/boot/bcm2709-rpi-2-b.dtb')
+
+initrd_url = ('https://github.com/groeck/linux-build-test/raw/'
+  '2eb0a73b5d5a28df3170c546ddaaa9757e1e0848/rootfs/'
+  'arm/rootfs-armv7a.cpio.gz')
+initrd_hash = '604b2e45cdf35045846b8bbfbf2129b1891bdc9c'
+initrd_path_gz = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
+initrd_path = os.path.join(self.workdir, 'rootfs.cpio')
+archive.gzip_uncompress(initrd_path_gz, initrd_path)
+
+self.vm.set_console()
+kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+   'earlycon=pl011,0x3f201000 console=ttyAMA0 '
+   'panic=-1 noreboot ' +
+   'dwc_otg.fiq_fsm_enable=0')
+self.vm.add_args('-kernel', kernel_path,
+ '-dtb', dtb_path,
+ '-initrd', initrd_path,
+ '-append', kernel_command_line,
+ '-no-reboot')
+self.vm.launch()
+self.wait_for_console_pattern('Boot successful.')
+
+exec_command_and_wait_for_pattern(self, 'cat /proc/cpuinfo',
+  

Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed

[Klaus Jensen]

On May 31 15:42, Niklas Cassel wrote:

On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:

On May 28 11:05, Niklas Cassel wrote:
> From: Niklas Cassel 
>
> In the Zoned Namespace Command Set Specification, chapter
> 2.5.1 Managing resources
>
> "The controller may transition zones in the ZSIO:Implicitly Opened state
> to the ZSC:Closed state for resource management purposes."
>
> The word may in this sentence means that automatically transitioning
> an implicitly opened zone to closed is completely optional.
>
> Add a new parameter so that the user can control if this automatic
> transitioning should be performed or not.
>
> Being able to control this can help with verifying that e.g. a user-space
> program behaves properly even without this optional ZNS feature.
>
> The default value is set to true, in order to not change the existing
> behavior.
>
> Signed-off-by: Niklas Cassel 
> ---
> hw/nvme/ctrl.c | 9 -
> hw/nvme/ns.c   | 2 ++
> hw/nvme/nvme.h | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 40a7efcea9..d00f0297a5 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -141,6 +141,11 @@
>  *
>  * zoned.cross_read=
>  * Setting this property to true enables Read Across Zone Boundaries.
> + *
> + * zoned.auto_transition=
> + * Indicates if zones in zone state implicitly opened can be
> + * automatically transitioned to zone state closed for resource
> + * management purposes.
>  */
>
> #include "qemu/osdep.h"
> @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, 
NvmeZone *zone,
> /* fallthrough */
>
> case NVME_ZONE_STATE_CLOSED:
> -nvme_zrm_auto_transition_zone(ns);
> +if (ns->params.auto_transition_zones) {
> +nvme_zrm_auto_transition_zone(ns);
> +}
> status = nvme_aor_check(ns, act, 1);
> if (status) {
> return status;
> diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> index 3fec9c6273..31dee43d30 100644
> --- a/hw/nvme/ns.c
> +++ b/hw/nvme/ns.c
> @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
>params.max_open_zones, 0),
> DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
>params.zd_extension_size, 0),
> +DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
> + params.auto_transition_zones, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> index 81a35cda14..bd86054db2 100644
> --- a/hw/nvme/nvme.h
> +++ b/hw/nvme/nvme.h
> @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
> uint32_t max_active_zones;
> uint32_t max_open_zones;
> uint32_t zd_extension_size;
> +bool auto_transition_zones;
> } NvmeNamespaceParams;
>
> typedef struct NvmeNamespace {
> --
> 2.31.1
>

Looks good Niklas!

Reviewed-by: Klaus Jensen 


In reality, it is the controller that does the auto transitioning.

In theory, one namespace could be attached to two different controllers,
and I guess, in that case, it depends on if the controller that we used
when doing the write supports auto transitioning or not, that determines
if a zone will be auto transitioned or not.

If we were to change this to be a parameter of the controller instead
of a parameter of the namespace, we would require to refactor a lot of
code in the regular write path. As we currently don't have any NvmeRequest
object in nvme_zrm_open_flags().

Thoughts?



I think you are right. This should be controller-specific behavior. I 
took the liberty of moving the parameter; the refactor is minimal.



From: Niklas Cassel 

In the Zoned Namespace Command Set Specification, chapter
2.5.1 Managing resources

"The controller may transition zones in the ZSIO:Implicitly Opened state
to the ZSC:Closed state for resource management purposes."

The word may in this sentence means that automatically transitioning
an implicitly opened zone to closed is completely optional.

Add a new parameter so that the user can control if this automatic
transitioning should be performed or not.

Being able to control this can help with verifying that e.g. a user-space
program behaves properly even without this optional ZNS feature.

The default value is set to true, in order to not change the existing
behavior.

Signed-off-by: Niklas Cassel 
[k.jensen: moved parameter to controller]
Signed-off-by: Klaus Jensen 
---
 hw/nvme/nvme.h |  1 +
 hw/nvme/ctrl.c | 32 ++--
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 81a35cda142b..93a7e0e5380e 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -382,6 +382,7 @@ typedef struct NvmeParams {
 uint8_t  vsl;
 bool use_intel_id;
 uint8_t  zasl;
+bool auto_transition_zones;
 bool legacy_cmb;
 } NvmeParams;
 
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c

index 

Re: [PATCH v3 1/5] blkdebug: refactor removal of a suspended request

[Vladimir Sementsov-Ogievskiy]

17.05.2021 17:50, Emanuele Giuseppe Esposito wrote:

Extract to a separate function.  Do not rely on FOREACH_SAFE, which is
only "safe" if the *current* node is removed---not if another node is
removed.  Instead, just walk the entire list from the beginning when
asked to resume all suspended requests with a given tag.

Co-developed-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/blkdebug.c | 28 +---
  1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2c0b9b0ee8..8f19d991fa 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -793,7 +793,6 @@ static void suspend_request(BlockDriverState *bs, 
BlkdebugRule *rule)
  printf("blkdebug: Resuming request '%s'\n", r.tag);
  }
  
-QLIST_REMOVE(&r, next);

  g_free(r.tag);
  }
  
@@ -869,25 +868,35 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,

  return 0;
  }
  
-static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)

+static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all)
  {
-BDRVBlkdebugState *s = bs->opaque;
-BlkdebugSuspendedReq *r, *next;
+BlkdebugSuspendedReq *r;
  
-QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) {

+retry:
+QLIST_FOREACH(r, &s->suspended_reqs, next) {
  if (!strcmp(r->tag, tag)) {
+QLIST_REMOVE(r, next);
  qemu_coroutine_enter(r->co);
+if (all) {
+goto retry;
+}
  return 0;
  }
  }
  return -ENOENT;
  }
  
+static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)

+{
+BDRVBlkdebugState *s = bs->opaque;
+
+return resume_req_by_tag(s, tag, false);
+}
+
  static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
  const char *tag)
  {
  BDRVBlkdebugState *s = bs->opaque;
-BlkdebugSuspendedReq *r, *r_next;
  BlkdebugRule *rule, *next;
  int i, ret = -ENOENT;
  
@@ -900,11 +909,8 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,

  }
  }
  }
-QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, r_next) {
-if (!strcmp(r->tag, tag)) {
-qemu_coroutine_enter(r->co);
-ret = 0;
-}
+if (resume_req_by_tag(s, tag, true) == 0) {
+ret = 0;
  }
  return ret;
  }



Interesting, could we really have several suspended_reqs with same tag, keeping 
in mind suspend_requests() removes rule before creating suspended_req with same 
tag.. Probably user could create new rule with same tag, when existing requests 
is suspended? Not sure is such behavior expected or should be abandoned. Still, 
it's all not about that patch:

Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir

Re: [PATCH v3 2/5] blkdebug: move post-resume handling to resume_req_by_tag

[Vladimir Sementsov-Ogievskiy]

17.05.2021 17:50, Emanuele Giuseppe Esposito wrote:

We want to move qemu_coroutine_yield() after the loop on rules,
because QLIST_FOREACH_SAFE is wrong if the rule list is modified
while the coroutine has yielded.  Therefore move the suspended
request to the heap and clean it up from the remove side.
All that is left is for blkdebug_debug_event to handle the
yielding.

Co-developed-by: Paolo Bonzini
Signed-off-by: Emanuele Giuseppe Esposito



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v3 3/5] blkdebug: track all actions

[Vladimir Sementsov-Ogievskiy]

17.05.2021 17:50, Emanuele Giuseppe Esposito wrote:

Add a counter for each action that a rule can trigger.
This is mainly used to keep track of how many coroutine_yield()
we need to perform after processing all rules in the list.

Co-developed-by: Paolo Bonzini
Signed-off-by: Emanuele Giuseppe Esposito



Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir

Re: [PATCH v3] target/i386/sev: add support to query the attestation report

[Eduardo Habkost]

On Thu, Apr 29, 2021 at 12:07:28PM -0500, Brijesh Singh wrote:
> The SEV FW >= 0.23 added a new command that can be used to query the
> attestation report containing the SHA-256 digest of the guest memory
> and VMSA encrypted with the LAUNCH_UPDATE and sign it with the PEK.
> 
> Note, we already have a command (LAUNCH_MEASURE) that can be used to
> query the SHA-256 digest of the guest memory encrypted through the
> LAUNCH_UPDATE. The main difference between previous and this command
> is that the report is signed with the PEK and unlike the LAUNCH_MEASURE
> command the ATTESATION_REPORT command can be called while the guest
> is running.
> 
> Add a QMP interface "query-sev-attestation-report" that can be used
> to get the report encoded in base64.
> 
> Cc: James Bottomley 
> Cc: Tom Lendacky 
> Cc: Eric Blake 
> Cc: Paolo Bonzini 
> Cc: k...@vger.kernel.org
> Reviewed-by: James Bottomley 
> Tested-by: James Bottomley 
> Signed-off-by: Brijesh Singh 
> ---
[...]
> +gsize len;
[...]
> +/* verify the input mnonce length */
> +if (len != sizeof(input.mnonce)) {
> +error_setg(errp, "SEV: mnonce must be %ld bytes (got %ld)",
> +sizeof(input.mnonce), len);

This breaks the build on i386.  Failed CI job:
https://gitlab.com/ehabkost/qemu/-/jobs/1300032082

I'm applying the following fixup.

Signed-off-by: Eduardo Habkost 
---
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 12899a31736..0e135d56e53 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -517,7 +517,7 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
 
 /* verify the input mnonce length */
 if (len != sizeof(input.mnonce)) {
-error_setg(errp, "SEV: mnonce must be %ld bytes (got %ld)",
+error_setg(errp, "SEV: mnonce must be %ld bytes (got %" G_GSIZE_FORMAT 
")",
 sizeof(input.mnonce), len);
 g_free(buf);
 return NULL;

-- 
Eduardo

Re: [PATCH 0/2] [RESEND] SEV firmware error list touchups

[Eduardo Habkost]

On Fri, Apr 30, 2021 at 08:48:28AM -0500, Connor Kuehl wrote:
> Connor Kuehl (2):
>   sev: use explicit indices for mapping firmware error codes to strings
>   sev: add missing firmware error conditions
> 
>  target/i386/sev.c | 48 ---
>  1 file changed, 25 insertions(+), 23 deletions(-)

I'm queueing this on x86-next, apologies for the delay.

-- 
Eduardo

Re: [PATCH v2] hw/nvme/ctrl: fix csi field for cns 0x00 and 0x11

[Klaus Jensen]

On Apr 27 12:00, Gollu Appalanaidu wrote:

As per the TP 4056d Namespace types CNS 0x00 and CNS 0x11
CSI field shouldn't use but it is being used for these two
Identify command CNS values, fix that.

Remove 'nvme_csi_has_nvm_support()' helper as suggested by
Klaus we can safely assume NVM command set support for all
namespaces.

Suggested-by: Klaus Jensen 
Signed-off-by: Gollu Appalanaidu 


LGTM.

Reviewed-by: Klaus Jensen 


---
-v2: add sugggestions from Klaus
We can Remove 'nvme_csi_has_nvm_support()' helper, we can
assume NVM command set support for all namespaces.

hw/nvme/ctrl.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2e7498a73e..7fcd699235 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4178,16 +4178,6 @@ static uint16_t nvme_rpt_empty_id_struct(NvmeCtrl *n, 
NvmeRequest *req)
return nvme_c2h(n, id, sizeof(id), req);
}

-static inline bool nvme_csi_has_nvm_support(NvmeNamespace *ns)
-{
-switch (ns->csi) {
-case NVME_CSI_NVM:
-case NVME_CSI_ZONED:
-return true;
-}
-return false;
-}
-
static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeRequest *req)
{
trace_pci_nvme_identify_ctrl();
@@ -4244,7 +4234,7 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest 
*req, bool active)
}
}

-if (c->csi == NVME_CSI_NVM && nvme_csi_has_nvm_support(ns)) {
+if (active || ns->csi == NVME_CSI_NVM) {
return nvme_c2h(n, (uint8_t *)&ns->id_ns, sizeof(NvmeIdNs), req);
}

@@ -4315,7 +4305,7 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, 
NvmeRequest *req,
}
}

-if (c->csi == NVME_CSI_NVM && nvme_csi_has_nvm_support(ns)) {
+if (c->csi == NVME_CSI_NVM) {
return nvme_rpt_empty_id_struct(n, req);
} else if (c->csi == NVME_CSI_ZONED && ns->csi == NVME_CSI_ZONED) {
return nvme_c2h(n, (uint8_t *)ns->id_ns_zoned, sizeof(NvmeIdNsZoned),
--
2.17.1



signature.asc
Description: PGP signature

Re: [PATCH 1/3] hw/nvme/ctrl: add controller list cns 0x13

[Klaus Jensen]

On May 17 15:37, Gollu Appalanaidu wrote:

Add the controller identifiers list available in NVM Subsystem
that may or may not be attached to namespaces.

Signed-off-by: Gollu Appalanaidu 
---
hw/nvme/ctrl.c   | 25 +++--
hw/nvme/trace-events |  2 +-
include/block/nvme.h |  1 +
3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2e7498a73e..d08a3350e2 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4251,7 +4251,8 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest 
*req, bool active)
return NVME_INVALID_CMD_SET | NVME_DNR;
}

-static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req)
+static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
+bool attached)
{
NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
uint16_t min_id = le16_to_cpu(c->ctrlid);
@@ -4261,15 +4262,17 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl 
*n, NvmeRequest *req)
NvmeCtrl *ctrl;
int cntlid, nr_ids = 0;

-trace_pci_nvme_identify_ns_attached_list(min_id);
+trace_pci_nvme_identify_ctrl_list(c->cns, min_id);

-if (c->nsid == NVME_NSID_BROADCAST) {
-return NVME_INVALID_FIELD | NVME_DNR;
-}
+if (attached) {
+if (c->nsid == NVME_NSID_BROADCAST) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}

-ns = nvme_subsys_ns(n->subsys, c->nsid);
-if (!ns) {
-return NVME_INVALID_FIELD | NVME_DNR;
+ns = nvme_subsys_ns(n->subsys, c->nsid);
+if (!ns) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
}

for (cntlid = min_id; cntlid < ARRAY_SIZE(n->subsys->ctrls); cntlid++) {
@@ -4278,7 +4281,7 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl 
*n, NvmeRequest *req)
continue;
}

-if (!nvme_ns(ctrl, c->nsid)) {
+if (attached && !nvme_ns(ctrl, c->nsid)) {
continue;
}

@@ -4493,7 +4496,9 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest 
*req)
case NVME_ID_CNS_NS_PRESENT:
return nvme_identify_ns(n, req, false);
case NVME_ID_CNS_NS_ATTACHED_CTRL_LIST:
-return nvme_identify_ns_attached_list(n, req);
+return nvme_identify_ctrl_list(n, req, true);
+case NVME_ID_CNS_CTRL_LIST:
+return nvme_identify_ctrl_list(n, req, false);
case NVME_ID_CNS_CS_NS:
return nvme_identify_ns_csi(n, req, true);
case NVME_ID_CNS_CS_NS_PRESENT:
diff --git a/hw/nvme/trace-events b/hw/nvme/trace-events
index ea33d0ccc3..7ba3714671 100644
--- a/hw/nvme/trace-events
+++ b/hw/nvme/trace-events
@@ -55,7 +55,7 @@ pci_nvme_identify(uint16_t cid, uint8_t cns, uint16_t ctrlid, 
uint8_t csi) "cid
pci_nvme_identify_ctrl(void) "identify controller"
pci_nvme_identify_ctrl_csi(uint8_t csi) "identify controller, csi=0x%"PRIx8""
pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
-pci_nvme_identify_ns_attached_list(uint16_t cntid) "cntid=%"PRIu16""
+pci_nvme_identify_ctrl_list(uint8_t cns, uint16_t cntid) "cns 0x%"PRIx8" 
cntid=%"PRIu16""


Inconsistency here. Please use "field value", not "field=value" for 
cntid.



pci_nvme_identify_ns_csi(uint32_t ns, uint8_t csi) "nsid=%"PRIu32", 
csi=0x%"PRIx8""
pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
pci_nvme_identify_nslist_csi(uint16_t ns, uint8_t csi) "nsid=%"PRIu16", 
csi=0x%"PRIx8""
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 0ff9ce17a9..188ab460df 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -980,6 +980,7 @@ enum NvmeIdCns {
NVME_ID_CNS_NS_PRESENT_LIST   = 0x10,
NVME_ID_CNS_NS_PRESENT= 0x11,
NVME_ID_CNS_NS_ATTACHED_CTRL_LIST = 0x12,
+NVME_ID_CNS_CTRL_LIST = 0x13,
NVME_ID_CNS_CS_NS_PRESENT_LIST= 0x1a,
NVME_ID_CNS_CS_NS_PRESENT = 0x1b,
NVME_ID_CNS_IO_COMMAND_SET= 0x1c,
--
2.17.1



--
One of us - No more doubt, silence or taboo about mental illness.


signature.asc
Description: PGP signature

Re: [PATCH] tests/vm/freebsd: Increase code coverage

[Wainer dos Santos Moschetta]

Hi,

On 5/31/21 7:03 AM, Philippe Mathieu-Daudé wrote:

Install more dependencies to increase code coverage.

Signed-off-by: Philippe Mathieu-Daudé 
---
  tests/vm/freebsd | 5 +
  1 file changed, 5 insertions(+)


With or without this patch I got an error when `make vm-build-freebsd`. 
It fails to install packages.


For example, with this patch I got:

< Output omitted>

### Installing packages ...
Failed to prepare guest environment
Traceback (most recent call last):
  File "/home/wmoschet/src/qemu/tests/vm/basevm.py", line 634, in main
    return vm.build_image(args.image)
  File "/home/wmoschet/src/qemu/tests/vm/freebsd", line 206, in build_image
    self.ssh_root_check("pkg install -y %s\n" % " ".join(self.pkgs))
  File "/home/wmoschet/src/qemu/tests/vm/basevm.py", line 255, in 
ssh_root_check

    self._ssh_do(self._config["root_user"], cmd, True)
  File "/home/wmoschet/src/qemu/tests/vm/basevm.py", line 242, in _ssh_do
    raise Exception("SSH command failed: %s" % cmd)
Exception: SSH command failed: pkg install -y git pkgconf bzip2 python37 
ninja bash gmake gsed gettext cyrus-sasl gnutls nettle jpeg-turbo png 
sdl2 gtk3 libxkbcommon pixman libepoxy mesa-libs zstd usbredir


Is it a known issue?



diff --git a/tests/vm/freebsd b/tests/vm/freebsd
index 6e20e843228..85049b43136 100755
--- a/tests/vm/freebsd
+++ b/tests/vm/freebsd
@@ -46,7 +46,9 @@ class FreeBSDVM(basevm.BaseVM):
  "gettext",
  
  # libs: crypto

+"cyrus-sasl",
  "gnutls",
+"nettle",
  
  # libs: images

  "jpeg-turbo",
@@ -56,6 +58,7 @@ class FreeBSDVM(basevm.BaseVM):
  "sdl2",
  "gtk3",
  "libxkbcommon",
+"pixman",
  
  # libs: opengl

  "libepoxy",
@@ -63,6 +66,8 @@ class FreeBSDVM(basevm.BaseVM):
  
  # libs: migration

  "zstd",
+
+"usbredir",
  ]
  
  # TODO: Enable gnutls again once FreeBSD's libtasn1 got fixed

Re: [PATCH 2/3] hw/nvme/ctrl: fix endian conversion for nsid in ctrl list

[Klaus Jensen]

On May 17 15:37, Gollu Appalanaidu wrote:

In Identify Ctrl List of the CNS 0x12 and 0x13 no endian conversion
for the nsid field.

Signed-off-by: Gollu Appalanaidu 
---
hw/nvme/ctrl.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index d08a3350e2..813a72c655 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4255,6 +4255,7 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, 
NvmeRequest *req,
bool attached)
{
NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
+uint32_t nsid = le32_to_cpu(c->nsid);
uint16_t min_id = le16_to_cpu(c->ctrlid);
uint16_t list[NVME_CONTROLLER_LIST_SIZE] = {};
uint16_t *ids = &list[1];
@@ -4265,11 +4266,11 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, 
NvmeRequest *req,
trace_pci_nvme_identify_ctrl_list(c->cns, min_id);

if (attached) {
-if (c->nsid == NVME_NSID_BROADCAST) {
+if (nsid == NVME_NSID_BROADCAST) {
return NVME_INVALID_FIELD | NVME_DNR;
}

-ns = nvme_subsys_ns(n->subsys, c->nsid);
+ns = nvme_subsys_ns(n->subsys, nsid);
if (!ns) {
return NVME_INVALID_FIELD | NVME_DNR;
}
@@ -4281,7 +4282,7 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, 
NvmeRequest *req,
continue;
}

-if (attached && !nvme_ns(ctrl, c->nsid)) {
+if (attached && !nvme_ns(ctrl, nsid)) {
continue;
}

--
2.17.1



I know that the endianness conversion was missing before your patch, but 
please squash this up into patch 1.


signature.asc
Description: PGP signature

[PATCH v2] docs/devel: Explain in more detail the TB chaining mechanisms

[Luis Pires]

Signed-off-by: Luis Pires 
---
v2:
 - s/outer execution loop/main loop
 - Mention re-evaluation of cpu_exec_interrupt()
 - Changed wording on lookup_and_goto_ptr()
 - Added more details to step 2 of goto+tb + exit_tb
 - Added details about when goto_tb + exit_tb cannot be used

 docs/devel/tcg.rst | 105 +++--
 1 file changed, 93 insertions(+), 12 deletions(-)

diff --git a/docs/devel/tcg.rst b/docs/devel/tcg.rst
index 4ebde44b9d..9cc56df22a 100644
--- a/docs/devel/tcg.rst
+++ b/docs/devel/tcg.rst
@@ -11,13 +11,14 @@ performances.
 QEMU's dynamic translation backend is called TCG, for "Tiny Code
 Generator". For more information, please take a look at ``tcg/README``.
 
-Some notable features of QEMU's dynamic translator are:
+The following sections outline some notable features and implementation
+details of QEMU's dynamic translator.
 
 CPU state optimisations
 ---
 
-The target CPUs have many internal states which change the way it
-evaluates instructions. In order to achieve a good speed, the
+The target CPUs have many internal states which change the way they
+evaluate instructions. In order to achieve a good speed, the
 translation phase considers that some state information of the virtual
 CPU cannot change in it. The state is recorded in the Translation
 Block (TB). If the state changes (e.g. privilege level), a new TB will
@@ -31,17 +32,97 @@ Direct block chaining
 -
 
 After each translated basic block is executed, QEMU uses the simulated
-Program Counter (PC) and other cpu state information (such as the CS
+Program Counter (PC) and other CPU state information (such as the CS
 segment base value) to find the next basic block.
 
-In order to accelerate the most common cases where the new simulated PC
-is known, QEMU can patch a basic block so that it jumps directly to the
-next one.
-
-The most portable code uses an indirect jump. An indirect jump makes
-it easier to make the jump target modification atomic. On some host
-architectures (such as x86 or PowerPC), the ``JUMP`` opcode is
-directly patched so that the block chaining has no overhead.
+In its simplest, less optimized form, this is done by exiting from the
+current TB, going through the TB epilogue, and then back to the
+main loop. That’s where QEMU looks for the next TB to execute,
+translating it from the guest architecture if it isn’t already available
+in memory. Then QEMU proceeds to execute this next TB, starting at the
+prologue and then moving on to the translated instructions.
+
+Exiting from the TB this way will cause the ``cpu_exec_interrupt()``
+callback to be re-evaluated before executing additional instructions.
+It is mandatory to exit this way after any CPU state changes that may
+unmask interrupts.
+
+In order to accelerate the most common cases where the TB for the new
+simulated PC is already available, QEMU has mechanisms that allow
+multiple TBs to be chained directly, without having to go back to the
+main loop as described above. These mechanisms are:
+
+``lookup_and_goto_ptr``
+^^^
+
+Calling ``tcg_gen_lookup_and_goto_ptr()`` will emit a call to
+``helper_lookup_tb_ptr``. This helper will look for an existing TB that
+matches the current CPU state. If the destination TB is available its
+code address is returned, otherwise the address of the JIT epilogue is
+returned. The call to the helper is always followed by the tcg ``goto_ptr``
+opcode, which branches to the returned address. In this way, we either
+branch to the next TB or return to the main loop.
+
+``goto_tb + exit_tb``
+^
+
+The translation code usually implements branching by performing the
+following steps:
+
+1. Call ``tcg_gen_goto_tb()`` passing a jump slot index (either 0 or 1)
+   as a parameter.
+
+2. Emit TCG instructions to update the CPU state with any information
+   that has been assumed constant and is required by the main loop to
+   correctly locate and execute the next TB. For most guests, this is
+   just the PC of the branch destination, but others may store additional
+   data. The information updated in this step must be inferable from both
+   ``cpu_get_tb_cpu_state()`` and ``cpu_restore_state()``.
+
+3. Call ``tcg_gen_exit_tb()`` passing the address of the current TB and
+   the jump slot index again.
+
+Step 1, ``tcg_gen_goto_tb()``, will emit a ``goto_tb`` TCG
+instruction that later on gets translated to a jump to an address
+associated with the specified jump slot. Initially, this is the address
+of step 2's instructions, which update the CPU state information. Step 3,
+``tcg_gen_exit_tb()``, exits from the current TB returning a tagged
+pointer composed of the last executed TB’s address and the jump slot
+index.
+
+The first time this whole sequence is executed, step 1 simply jumps
+to step 2. Then the CPU state information gets updated and we exit from
+the current TB. As a result, the behavior is very similar to

Re: HSS Issue with GCC 10, Qemu Setup for microchip-icicle-kit

[Alistair Francis]

On Tue, Jun 1, 2021 at 12:06 AM Rahul Pathak  wrote:
>
> I followed the same link. I will elaborate what is happening at my end -
>
> First -
> Used the same versions as per the doc. Built HSS 2020.12 and used 
> core-image-minimal-dev-icicle-kit-es-sd-20201009141623.rootfs.wic.
> Upon executing the qemu launch command as per the doc, it's just waits for 
> the connection on another serial console -
>
> info: QEMU waiting for connection on: disconnected:unix:serial1.sock,server=on

That seems like you are passing an argument to QEMU to expose a
socket. You don't need to do that and can instead use stdio.

For the runqemu command in OE you can do that with `runqemu nographic`

Alistair

>
> Even if I open sudo minicom -D unix\#serial1.sock, which remains offline 
> always.
>
> Second -
> If I remove the "-chardev 
> socket,id=serial1,path=serial1.sock,server=on,wait=on -serial 
> chardev:serial1" from the
> qemu launch command then HSS boots but stops after sbi_init on all the cores 
> and put the cores in Idle -
>
> [5.443011] boot_download_chunks_onExit(): boot_service(u54_1)::u54_1:sbi_init 
> 8020
> [5.444960] boot_wait_onEntry(): boot_service(u54_1)::Checking for IPI ACKs: - 
> -
> [5.447962] boot_wait_handler(): boot_service(u54_1)::Checking for IPI ACKs: 
> ACK/IDLE ACK
> [5.449343] RunStateMachine(): boot_service(u54_1)::Wait -> 
> boot_service(u54_1)::Idle
>
> Third -
> If I take the latest release of HSS 2021.04 and build with gcc10, it does not 
> boot at all even if I remove the serial1 like in the second case.
>
>
> Thanks
> Rahul
>
> On Mon, May 31, 2021 at 8:19 AM Bin Meng  wrote:
>>
>> Hi Rahul,
>>
>> On Mon, May 31, 2021 at 1:08 AM Rahul Pathak  
>> wrote:
>> >
>> > Hi Bin,
>> >
>> > I was reading a github issue which you raised for the issue with HSS 
>> > because of GCC 10.1. Its pretty old and not sure what has changed so I 
>> > thought to ask you over email for help.
>>
>> The HSS issue of GCC 10.1 was already fixed in the HSS mainline.
>>
>> > I myself is trying to make a setup to boot the  microchip-icicle-kit Qemu 
>> > machine with HSS and Yocto linux image. Currently it does not work.
>>
>> Could you please elaborate which part does not work? Is that Linux,
>> HSS, or U-Boot?
>>
>> >
>> > Do you know what is the right setup to make  microchip-icicle-kit machine 
>> > with HSS.bin and Yocto linux work on Qemu?. Probably there will be a 
>> > working combination of HSS, Linux releases plus the toolchain version 
>> > which makes this setup work.
>> >
>>
>> I have not tried Yocto Linux yet with Microchip Icicle Kit on QEMU. I
>> only tested the Linux images released by Microchip.
>> Could you please follow the instructions @
>> https://qemu.readthedocs.io/en/latest/system/riscv/microchip-icicle-kit.html
>> to see which step might be wrong on your side?
>>
>> Regards,
>> Bin

Re: HSS Issue with GCC 10, Qemu Setup for microchip-icicle-kit

[Alistair Francis]

On Tue, Jun 1, 2021 at 12:43 AM Rahul Pathak  wrote:
>
> On top of that, it seems I cannot connect with the target using gdb
>
> (gdb) target remote :1234
> Remote debugging using :1234
> bfd requires flen 8, but target has flen 0
>
> Though the ABI is lp64 and ISA is rv64imac when the hss was built.

Strange, how are you building GDB?

Alistair

>
> On Mon, May 31, 2021 at 7:37 PM Rahul Pathak  wrote:
>>
>> I followed the same link. I will elaborate what is happening at my end -
>>
>> First -
>> Used the same versions as per the doc. Built HSS 2020.12 and used 
>> core-image-minimal-dev-icicle-kit-es-sd-20201009141623.rootfs.wic.
>> Upon executing the qemu launch command as per the doc, it's just waits for 
>> the connection on another serial console -
>>
>> info: QEMU waiting for connection on: 
>> disconnected:unix:serial1.sock,server=on
>>
>> Even if I open sudo minicom -D unix\#serial1.sock, which remains offline 
>> always.
>>
>> Second -
>> If I remove the "-chardev 
>> socket,id=serial1,path=serial1.sock,server=on,wait=on -serial 
>> chardev:serial1" from the
>> qemu launch command then HSS boots but stops after sbi_init on all the cores 
>> and put the cores in Idle -
>>
>> [5.443011] boot_download_chunks_onExit(): 
>> boot_service(u54_1)::u54_1:sbi_init 8020
>> [5.444960] boot_wait_onEntry(): boot_service(u54_1)::Checking for IPI ACKs: 
>> - -
>> [5.447962] boot_wait_handler(): boot_service(u54_1)::Checking for IPI ACKs: 
>> ACK/IDLE ACK
>> [5.449343] RunStateMachine(): boot_service(u54_1)::Wait -> 
>> boot_service(u54_1)::Idle
>>
>> Third -
>> If I take the latest release of HSS 2021.04 and build with gcc10, it does 
>> not boot at all even if I remove the serial1 like in the second case.
>>
>>
>> Thanks
>> Rahul
>>
>> On Mon, May 31, 2021 at 8:19 AM Bin Meng  wrote:
>>>
>>> Hi Rahul,
>>>
>>> On Mon, May 31, 2021 at 1:08 AM Rahul Pathak  
>>> wrote:
>>> >
>>> > Hi Bin,
>>> >
>>> > I was reading a github issue which you raised for the issue with HSS 
>>> > because of GCC 10.1. Its pretty old and not sure what has changed so I 
>>> > thought to ask you over email for help.
>>>
>>> The HSS issue of GCC 10.1 was already fixed in the HSS mainline.
>>>
>>> > I myself is trying to make a setup to boot the  microchip-icicle-kit Qemu 
>>> > machine with HSS and Yocto linux image. Currently it does not work.
>>>
>>> Could you please elaborate which part does not work? Is that Linux,
>>> HSS, or U-Boot?
>>>
>>> >
>>> > Do you know what is the right setup to make  microchip-icicle-kit machine 
>>> > with HSS.bin and Yocto linux work on Qemu?. Probably there will be a 
>>> > working combination of HSS, Linux releases plus the toolchain version 
>>> > which makes this setup work.
>>> >
>>>
>>> I have not tried Yocto Linux yet with Microchip Icicle Kit on QEMU. I
>>> only tested the Linux images released by Microchip.
>>> Could you please follow the instructions @
>>> https://qemu.readthedocs.io/en/latest/system/riscv/microchip-icicle-kit.html
>>> to see which step might be wrong on your side?
>>>
>>> Regards,
>>> Bin

[PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[Peter Xu]

These two commands are missing when adding the QMP sister commands.  Add them,
so developers can play with them easier.

Cc: Dr. David Alan Gilbert 
Cc: Juan Quintela 
Cc: Leonardo Bras Soares Passos 
Cc: Chuan Zheng 
Cc: huang...@chinatelecom.cn
Signed-off-by: Peter Xu 
---
PS: I really doubt whether this is working as expected... I ran one 200MB/s
workload inside, what I measured is 20MB/s with current algorithm...  Sampling
512 pages out of 1G mem is not wise enough I guess, especially that assumes
dirty workload is spread across the memories while it's normally not the case..
---
 hmp-commands-info.hx  | 13 +
 hmp-commands.hx   | 14 ++
 include/monitor/hmp.h |  2 ++
 migration/dirtyrate.c | 43 +++
 4 files changed, 72 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ab0c7aa5eea..f8a9141dd8a 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -880,3 +880,16 @@ SRST
   ``info replay``
 Display the record/replay information: mode and the current icount.
 ERST
+
+{
+.name   = "dirty_rate",
+.args_type  = "",
+.params = "",
+.help   = "show dirty rate information",
+.cmd= hmp_info_dirty_rate,
+},
+
+SRST
+  ``info dirty_rate``
+Display the vcpu dirty rate information.
+ERST
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2d21fe5ad42..4c27fb91f7d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1727,3 +1727,17 @@ ERST
 .flags  = "p",
 },
 
+SRST
+``calc_dirty_rate`` *second*
+  Start a round of dirty rate measurement with the period specified in 
*second*.
+  The result of the dirty rate measurement may be observed with ``info
+  dirty_rate`` command.
+ERST
+
+{
+.name   = "calc_dirty_rate",
+.args_type  = "second:l",
+.params = "second",
+.help   = "start a round of guest dirty rate measurement",
+.cmd= hmp_calc_dirty_rate,
+},
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 605d57287ae..3baa1058e2c 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -129,5 +129,7 @@ void hmp_info_replay(Monitor *mon, const QDict *qdict);
 void hmp_replay_break(Monitor *mon, const QDict *qdict);
 void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
 void hmp_replay_seek(Monitor *mon, const QDict *qdict);
+void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
+void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index ccb98147e89..382287d2912 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -20,6 +20,9 @@
 #include "ram.h"
 #include "trace.h"
 #include "dirtyrate.h"
+#include "monitor/hmp.h"
+#include "monitor/monitor.h"
+#include "qapi/qmp/qdict.h"
 
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
 static struct DirtyRateStat DirtyStat;
@@ -424,3 +427,43 @@ struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
 {
 return query_dirty_rate_info();
 }
+
+void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict)
+{
+DirtyRateInfo *info = query_dirty_rate_info();
+
+monitor_printf(mon, "Status: %s\n",
+   DirtyRateStatus_str(info->status));
+monitor_printf(mon, "Start Time: %"PRIi64" (ms)\n",
+   info->start_time);
+monitor_printf(mon, "Period: %"PRIi64" (sec)\n",
+   info->calc_time);
+monitor_printf(mon, "Dirty rate: ");
+if (info->has_dirty_rate) {
+monitor_printf(mon, "%"PRIi64" (MB/s)\n", info->dirty_rate);
+} else {
+monitor_printf(mon, "(not ready)\n");
+}
+g_free(info);
+}
+
+void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict)
+{
+int64_t sec = qdict_get_try_int(qdict, "second", 0);
+Error *err = NULL;
+
+if (!sec) {
+monitor_printf(mon, "Incorrect period length specified!\n");
+return;
+}
+
+qmp_calc_dirty_rate(sec, &err);
+if (err) {
+hmp_handle_error(mon, err);
+return;
+}
+
+monitor_printf(mon, "Starting dirty rate measurement with period %"PRIi64
+   " seconds\n", sec);
+monitor_printf(mon, "[Please use 'info dirty_rate' to check results]\n");
+}
-- 
2.31.1

Re: HSS Issue with GCC 10, Qemu Setup for microchip-icicle-kit

[Bin Meng]

Hi Rahul,

On Mon, May 31, 2021 at 10:43 PM Rahul Pathak  wrote:
>
> On top of that, it seems I cannot connect with the target using gdb
>
> (gdb) target remote :1234
> Remote debugging using :1234
> bfd requires flen 8, but target has flen 0
>
> Though the ABI is lp64 and ISA is rv64imac when the hss was built.
>

Did you feed gdb the image you wanted to debug before "target remote:"?

The PolarFire SoC has 1+4 HARTs and you should follow the instructions
@ https://wiki.qemu.org/Documentation/Platforms/RISCV#Attaching_GDB to
do the debug.

Regards,
Bin

Re: HSS Issue with GCC 10, Qemu Setup for microchip-icicle-kit

[Rahul Pathak]

Hi BIn,Alistair,

I was passing the hss.elf file and it was strange that gdb after connecting
was not letting the target to continue from gdb.
what worked was to not pass anything and then connect the to target then
load the symbol file as hss.elf.
I followed the steps from the "Attaching the GDB" doc and was able to debug.


For the qemu command line from the doc, I made the "wait=off" then qemu was
not waiting for another serial connection
and launched the hss.


The problem remains is that I still do not have the u-boot and linux
booting. The unix\#serial1.sock remains offline always.
These are the HSS logs -

[0.115001] HSS_E51_Banner(): PolarFire(R) SoC Hart Software Services (HSS)
- version 0.99.15

(c) Copyright 2017-2020 Microchip Corporation.





[0.116234] HSS_E51_Banner(): incorporating OpenSBI - version 0.6


(c) Copyright 2019-2020 Western Digital Corporation.





[0.117071] HSS_PrintBuildId(): Build ID:
811342a39f80176f9e2086bf963a83224b3d3a2e

[0.117817] HSS_PrintToolVersions(): Built with the following tools:


 - riscv64-unknown-linux-gnu-gcc (GCC) 10.2.0


 - GNU ld (GNU Binutils) 2.36.1





[0.118760] HSS_MemTestDDRFast(): DDR size is 1 GiB


[0.130270] HSS_MMCInit(): Attempting to select SDCARD ... Passed


Press a key to enter CLI, ESC to skip


Timeout in 5 seconds

.


[5.138747] HSS_TinyCLI_Parser(): CLI check timeout


[5.139371] IPI_QueuesInit(): Initializing IPI Queues (9000 bytes @
8000e40)...
[5.140435] HSS_PMP_Init(): Initializing PMPs


[5.141093] HSS_BootInit(): Initializing Boot Image..


[5.141787] getBootImageFromMMC_(): Preparing to copy from MMC to DDR ...


[5.142671] getBootImageFromMMC_(): Attempting to read image header (1552
bytes) ...

[5.144118] GPT_ValidateHeader(): Validated GPT Header ...


[5.153768] GPT_ValidatePartitionEntries(): Validated GPT Partition Entries
...

[5.155210] copyBootImageToDDR_(): Copying 436008 bytes to 0xA000


[5.407848] copyBootImageToDDR_(): Calculated CRC32 of image in DDR is
795fbbea

[5.412058] HSS_BootInit():  boot image passed CRC


[5.412407] HSS_BootInit(): Boot image set name: "PolarFire-SoC-HSS::U-Boot"


[5.412951] HSS_BootInit(): Boot Image registered...


[5.413376] HSS_Boot_RestartCore(): called for all harts


[5.414295] RunStateMachine(): boot_service(u54_1)::Init ->
boot_service(u54_1)::SetupPMP
[5.414812] RunStateMachine(): boot_service(u54_2)::Init ->
boot_service(u54_2)::SetupPMP

[5.415207] RunStateMachine(): boot_service(u54_3)::Init ->
boot_service(u54_3)::SetupPMP
[5.415631] RunStateMachine(): boot_service(u54_4)::Init ->
boot_service(u54_4)::SetupPMP

[5.416107] RunStateMachine(): usbdmsc_service::init ->
usbdmsc_service::idle

[5.417164] RunStateMachine(): boot_service(u54_1)::SetupPMP ->
boot_service(u54_1)::SetupPMPComplete

[5.417887] RunStateMachine(): boot_service(u54_2)::SetupPMP ->
boot_service(u54_2)::SetupPMPComplete

[5.418552] RunStateMachine(): boot_service(u54_3)::SetupPMP ->
boot_service(u54_3)::SetupPMPComplete

[5.419890] RunStateMachine(): boot_service(u54_4)::SetupPMP ->
boot_service(u54_4)::SetupPMPComplete
[23.955147] RunStateMachine(): boot_service(u54_1)::SetupPMPComplete ->
boot_service(u54_1)::ZeroInit
[23.955754] RunStateMachine(): boot_service(u54_2)::SetupPMPComplete ->
boot_service(u54_2)::ZeroInit
[23.956259] RunStateMachine(): boot_service(u54_3)::SetupPMPComplete ->
boot_service(u54_3)::ZeroInit
[23.956757] RunStateMachine(): boot_service(u54_4)::SetupPMPComplete ->
boot_service(u54_4)::ZeroInit
[23.957371] RunStateMachine(): boot_service(u54_1)::ZeroInit ->
boot_service(u54_1)::Download
[23.957876] RunStateMachine(): boot_service(u54_2)::ZeroInit ->
boot_service(u54_2)::Download
[23.958386] RunStateMachine(): boot_service(u54_3)::ZeroInit ->
boot_service(u54_3)::Download
[23.958856] RunStateMachine(): boot_service(u54_4)::ZeroInit ->
boot_service(u54_4)::Download
[23.960300] RunStateMachine(): boot_service(u54_2)::Download ->
boot_service(u54_2)::Idle
[23.960723] RunStateMachine(): boot_service(u54_3)::Download ->
boot_service(u54_3)::Idle
[23.961129] RunStateMachine(): boot_service(u54_4)::Download ->
boot_service(u54_4)::Idle
[23.983168] RunStateMachine(): boot_service(u54_1)::Download ->
boot_service(u54_1)::Wait
[23.983661] boot_download_chunks_onExit():
boot_service(u54_1)::u54_2:sbi_init 8020
[23.984374] boot_download_chunks_onExit():
boot_service(u54_1)::u54_3:sbi_init 8020
[23.985418] boot_download_chunks_onExit():
boot_service(u54_1)::u54_4:sbi_init 8020
[23.986783] boot_download_chunks_onExit():
boot_service(u54_1)::u54_1:sbi_init 8020
[23.989086] boot_wait_onEntry(): boot_service(u54_1)::Checking for IPI
ACKs: - -
[23.992106] boot_wait_handler(): boot_service(u54_1)::Checking for IPI
ACKs: ACK/IDLE ACK
[23.994062] RunStateMachine(): boot_service(u54_1)::Wait ->
boot_service(u54_1)::Idle


One thing I overlooked in the document is that we are preparing the *.wic
file after downloading
but passing the *.img in the qemu command. How to convert

Re: [target/ppc] excp_helper.c and mmu_helper.c cleanup

[David Gibson]

On Mon, May 31, 2021 at 04:21:11PM -0300, Lucas Mateus Martins Araujo e Castro 
wrote:
> Hi everyone,
> 
> I'm working on cleaning up some of the changes to enable the disable-tcg
> option on PPC, right now focusing on target/ppc/excp_helper.c and
> target/ppc/mmu_helper.c as these files have functions that are needed in a
> !TCG build but also contains code that doesn't compile in a !TCG build, and
> currently that is dealt with #ifdef.
> 
> For excp_helper.c I moved all exception handling functions to a new file
> (named target/ppc/excp_handler.c for now) and left only the helpers in it,
> and changed meson.build to always compile the new file and only compile the
> file with the helpers in a build with TCG.

That sounds reasonable.

> For mmu_helper.c the idea is to move all the code inside #ifdef CONFIG_TCG
> to another file that shouldn't be compiled in a !TCG build. But these
> changes are based on Richard Henderson's patch, so it depends if they'll be
> applied as is or there will be another version.

Ok.

> Also I'm looking into the possibility of not compiling
> ppc_tlb_invalidate_all in mmu_helper.c, but that's only possible if this
> function is not used in a !TCG build, does anyone know if this function is
> used in some corner case when running with KVM?

I'm pretty sure if ppc_tlb_invalidate_all() was ever called with KVM
that would be a bug, so that sounds sensible.

> Any opinion on these changes?

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v3 4/4] target/ppc: removed all mentions to PPC_DUMP_CPU

[da...@gibson.dropbear.id.au]

On Mon, May 31, 2021 at 05:46:07PM +, Luis Fernando Fujita Pires wrote:
> From: Bruno Larsen (billionai) 
> > This feature will no longer be useful as ppc moves to using decotree for 
> > TCG.
> > And building with it enabled is no longer possible, due to changes in
> > opc_handler_t. Since the last commit that mentions it happened in 2014, I 
> > think
> > it is safe to remove it.
> > 
> > Signed-off-by: Bruno Larsen (billionai) 
> > ---
> >  target/ppc/cpu_init.c  | 205 -
> >  target/ppc/internal.h  |   2 -
> >  target/ppc/translate.c | 105 -
> >  3 files changed, 312 deletions(-)
> 
> I believe you lost Richard's review for this one. In addition to approving 
> it, he also noted the a typo in the commit message ("decotree" -> 
> "decodetree").

I folded in the typo fix and Richard's R-b for this patch and the
previous one.

> Reviewed-by: Luis Pires 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v3 0/4] target/ppc: Remove DO_PPC_STATISTICS and PPC_DUMP_CPU

[David Gibson]

On Mon, May 31, 2021 at 11:56:25AM -0300, Bruno Larsen (billionai) wrote:
> These 2 defines are being obsoleted as we move to decodetree, so they
> were removed.
> 
> Also, upon further inspection, qemu already doesn't build with them
> enabled, and probably hasn't for a while, and no one complained, so I
> don't think they were actually being used.
> 
> Based-on: dgibson's ppc-for-6.1 tree

Applied to ppc-for-6.1, thanks.

> 
> Changelog for v3:
>  * Re-added patch that removed cpu_dump_statistics from hw/core/cpu
>  * added HMP documentation patch to this series
> 
> Changelog for v2:
>  * removed patches that were already applied
>  * also removed PPC_DUMP_CPU functinality
> 
> Bruno Larsen (billionai) (4):
>   hw/core/cpu: removed cpu_dump_statistics function
>   HMP: added info cpustats to removed_features.rst
>   target/ppc: removed GEN_OPCODE decision tree
>   target/ppc: removed all mentions to PPC_DUMP_CPU
> 
>  docs/system/removed-features.rst |   5 +
>  hw/core/cpu-common.c |   9 --
>  include/hw/core/cpu.h|  12 --
>  target/ppc/cpu_init.c| 205 ---
>  target/ppc/internal.h|   2 -
>  target/ppc/translate.c   | 184 ---
>  6 files changed, 5 insertions(+), 412 deletions(-)
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v4] target/ppc: overhauled and moved logic of storing fpscr

[David Gibson]

On Thu, May 27, 2021 at 01:35:22PM -0300, Bruno Larsen (billionai) wrote:
65;6401;1c> Followed the suggested overhaul to store_fpscr logic, and moved it 
to
> cpu.c where it can be accessed in !TCG builds.
> 
> The overhaul was suggested because storing a value to fpscr should
> never raise an exception, so we could remove all the mess that happened
> with POWERPC_EXCP_FP.
> 
> We also moved fpscr_set_rounding_mode into cpu.c as it could now be moved
> there, and it is needed when a value for the fpscr is being stored
> directly.
> 
> Suggested-by: Richard Henderson 
> Signed-off-by: Bruno Larsen (billionai) 
> Reviewed-by: Richard Henderson 

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/cpu.c|  43 
>  target/ppc/cpu.h|  12 +-
>  target/ppc/fpu_helper.c | 238 +++-
>  target/ppc/gdbstub.c|   6 +-
>  4 files changed, 65 insertions(+), 234 deletions(-)
> 
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index c8e87e30f1..19d67b5b07 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -25,6 +25,7 @@
>  #include "fpu/softfloat-helpers.h"
>  #include "mmu-hash64.h"
>  #include "helper_regs.h"
> +#include "sysemu/tcg.h"
>  
>  target_ulong cpu_read_xer(CPUPPCState *env)
>  {
> @@ -109,3 +110,45 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>  /* The gtse bit affects hflags */
>  hreg_compute_hflags(env);
>  }
> +
> +static inline void fpscr_set_rounding_mode(CPUPPCState *env)
> +{
> +int rnd_type;
> +
> +/* Set rounding mode */
> +switch (fpscr_rn) {
> +case 0:
> +/* Best approximation (round to nearest) */
> +rnd_type = float_round_nearest_even;
> +break;
> +case 1:
> +/* Smaller magnitude (round toward zero) */
> +rnd_type = float_round_to_zero;
> +break;
> +case 2:
> +/* Round toward +infinite */
> +rnd_type = float_round_up;
> +break;
> +default:
> +case 3:
> +/* Round toward -infinite */
> +rnd_type = float_round_down;
> +break;
> +}
> +set_float_rounding_mode(rnd_type, &env->fp_status);
> +}
> +
> +void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
> +{
> +val &= ~(FP_VX | FP_FEX);
> +if (val & FPSCR_IX) {
> +val |= FP_VX;
> +}
> +if ((val >> FPSCR_XX) & (val >> FPSCR_XE) & 0x1f) {
> +val |= FP_FEX;
> +}
> +env->fpscr = val;
> +if (tcg_enabled()) {
> +fpscr_set_rounding_mode(env);
> +}
> +}
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index b0934d9be4..b7ae4902e4 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -675,11 +675,11 @@ enum {
>  #define fpscr_ni (((env->fpscr) >> FPSCR_NI) & 0x1)
>  #define fpscr_rn (((env->fpscr) >> FPSCR_RN0)& 0x3)
>  /* Invalid operation exception summary */
> -#define fpscr_ix ((env->fpscr) & ((1 << FPSCR_VXSNAN) | (1 << FPSCR_VXISI)  
> | \
> -  (1 << FPSCR_VXIDI)  | (1 << FPSCR_VXZDZ)  
> | \
> -  (1 << FPSCR_VXIMZ)  | (1 << FPSCR_VXVC)   
> | \
> -  (1 << FPSCR_VXSOFT) | (1 << FPSCR_VXSQRT) 
> | \
> -  (1 << FPSCR_VXCVI)))
> +#define FPSCR_IX ((1 << FPSCR_VXSNAN) | (1 << FPSCR_VXISI)  | \
> +  (1 << FPSCR_VXIDI)  | (1 << FPSCR_VXZDZ)  | \
> +  (1 << FPSCR_VXIMZ)  | (1 << FPSCR_VXVC)   | \
> +  (1 << FPSCR_VXSOFT) | (1 << FPSCR_VXSQRT) | \
> +  (1 << FPSCR_VXCVI))
>  /* exception summary */
>  #define fpscr_ex  (((env->fpscr) >> FPSCR_XX) & 0x1F)
>  /* enabled exception summary */
> @@ -1332,7 +1332,7 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, 
> PPCVirtualHypervisor *vhyp);
>  #endif
>  #endif
>  
> -void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
> +void ppc_store_fpscr(CPUPPCState *env, target_ulong val);
>  void helper_hfscr_facility_check(CPUPPCState *env, uint32_t bit,
>   const char *caller, uint32_t cause);
>  
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index a4a283df2b..c4896cecc8 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -383,247 +383,35 @@ static inline void float_inexact_excp(CPUPPCState *env)
>  }
>  }
>  
> -static inline void fpscr_set_rounding_mode(CPUPPCState *env)
> -{
> -int rnd_type;
> -
> -/* Set rounding mode */
> -switch (fpscr_rn) {
> -case 0:
> -/* Best approximation (round to nearest) */
> -rnd_type = float_round_nearest_even;
> -break;
> -case 1:
> -/* Smaller magnitude (round toward zero) */
> -rnd_type = float_round_to_zero;
> -break;
> -case 2:
> -/* Round toward +infinite */
> -rnd_type = float_round_up;
> -break;
> -default:
> -case 3:
> -/* Round toward -infinite */
> -rnd_t

[PATCH] Add display suboptions to man pages

[email]

From: Ahmed Abouzied 

Updates man pages with the suboptions for the `-display`.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/128
Buglink: https://bugs.launchpad.net/qemu/+bug/1620660

Signed-off-by: Ahmed Abouzied 
---

This is just a trivial update for the man pages. It's the first time I
get to know QEMU source code. I didn't know which maintainer to CC here.
The `scripts/get_maintainer.pl` script didn't show any maintainers for
this file. I did CC the trivial mailing list because this is a very
small change.

Please let me know if the patch format is incorrect or something and
I'll fix it.

Looking forward to getting back feedback.

Thanks

---
 qemu-options.hx | 71 ++---
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 14258784b3..587cd7786d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -227,15 +227,15 @@ DEF("numa", HAS_ARG, QEMU_OPTION_numa,
 QEMU_ARCH_ALL)
 SRST
 ``-numa 
node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=initiator]``
-  \ 
+  \
 ``-numa 
node[,memdev=id][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=initiator]``
   \
 ``-numa dist,src=source,dst=destination,val=distance``
-  \ 
+  \
 ``-numa cpu,node-id=node[,socket-id=x][,core-id=y][,thread-id=z]``
-  \ 
+  \
 ``-numa 
hmat-lb,initiator=node,target=node,hierarchy=hierarchy,data-type=tpye[,latency=lat][,bandwidth=bw]``
-  \ 
+  \
 ``-numa 
hmat-cache,node-id=node,size=size,level=level[,associativity=str][,policy=str][,line=size]``
 Define a NUMA node and assign RAM and VCPUs to it. Set the NUMA
 distance from a source node to a destination node. Set the ACPI
@@ -430,7 +430,7 @@ DEF("global", HAS_ARG, QEMU_OPTION_global,
 QEMU_ARCH_ALL)
 SRST
 ``-global driver.prop=value``
-  \ 
+  \
 ``-global driver=driver,property=property,value=value``
 Set default value of driver's property prop to value, e.g.:
 
@@ -976,9 +976,9 @@ SRST
 ``-hda file``
   \
 ``-hdb file``
-  \ 
+  \
 ``-hdc file``
-  \ 
+  \
 ``-hdd file``
 Use file as hard disk 0, 1, 2 or 3 image (see the :ref:`disk images`
 chapter in the System Emulation Users Guide).
@@ -1468,7 +1468,7 @@ DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
 
 SRST
 ``-fsdev local,id=id,path=path,security_model=security_model 
[,writeout=writeout][,readonly=on][,fmode=fmode][,dmode=dmode] 
[,throttling.option=value[,throttling.option=value[,...]]]``
-  \ 
+  \
 ``-fsdev proxy,id=id,socket=socket[,writeout=writeout][,readonly=on]``
   \
 ``-fsdev proxy,id=id,sock_fd=sock_fd[,writeout=writeout][,readonly=on]``
@@ -1589,9 +1589,9 @@ DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
 
 SRST
 ``-virtfs local,path=path,mount_tag=mount_tag 
,security_model=security_model[,writeout=writeout][,readonly=on] 
[,fmode=fmode][,dmode=dmode][,multidevs=multidevs]``
-  \ 
+  \
 ``-virtfs proxy,socket=socket,mount_tag=mount_tag 
[,writeout=writeout][,readonly=on]``
-  \ 
+  \
 ``-virtfs proxy,sock_fd=sock_fd,mount_tag=mount_tag 
[,writeout=writeout][,readonly=on]``
   \
 ``-virtfs synth,mount_tag=mount_tag``
@@ -1819,11 +1819,22 @@ SRST
 old style -sdl/-curses/... options. Use ``-display help`` to list
 the available display types. Valid values for type are
 
-``sdl``
+``spice-app[,gl=on|off]``
+Start QEMU as a Spice server and launch the default Spice client
+application. The Spice server will redirect the serial consoles
+and QEMU monitors. (Since 4.0)
+
+
``sdl[,alt_grab=on|off][,ctrl_grab=on|off][,window_close=on|off][,gl=on|core|es|off]``
+
 Display video output via SDL (usually in a separate graphics
 window; see the SDL documentation for other possibilities).
 
-``curses``
+``gtk[,grab_on_hover=on|off][,gl=on|off]``
+Display video output in a GTK window. This interface provides
+drop-down menus and other UI elements to configure and control
+the VM during runtime.
+
+``curses [,charset=]``
 Display video output via curses. For graphics device models
 which support a text mode, QEMU can display this output using a
 curses/ncurses interface. Nothing is displayed when the graphics
@@ -1834,6 +1845,14 @@ SRST
 ``charset=CP850`` for IBM CP850 encoding. The default is
 ``CP437``.
 
+``vnc=[,]``
+Start a VNC server on display 
+
+``egl-headless[,rendernode]``
+Offload all OpenGL operations to a local DRI device. For any
+graphical display, this display needs to be paired with either
+VNC or SPICE displays.
+
 ``none``
 Do not display video output. The guest will still see an
 emulated graphics card, but its output will not be displayed to
@@ -1842,24 +1861,8 @@ SRST
 also changes the destination of the serial and parallel port
 data.
 
-``gtk``
-Display video output in a GTK window. This interface provides
-drop-down menus and othe

Re: [PATCH 07/11] target/rx: Use FloatRoundMode in helper_set_fpsw

[Philippe Mathieu-Daudé]

On 5/27/21 6:14 AM, Richard Henderson wrote:
> Use the proper type for the roundmode array.
> 
> Cc: Philippe Mathieu-Daudé 
> Cc: Yoshinori Sato 
> Signed-off-by: Richard Henderson 
> ---
>  target/rx/op_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 09/11] target/mips: Drop inline markers from msa_helper.c

[Philippe Mathieu-Daudé]

On 5/27/21 6:14 AM, Richard Henderson wrote:
> Some of these functions are quite large.
> Let the compiler decide whether to inline.
> 
> Cc: Philippe Mathieu-Daudé 
> Signed-off-by: Richard Henderson 
> ---
>  target/mips/tcg/msa_helper.c | 262 ---
>  1 file changed, 121 insertions(+), 141 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 11/11] target/mips: Drop denormal operand to update_msacsr

[Philippe Mathieu-Daudé]

On 5/27/21 6:14 AM, Richard Henderson wrote:
> The comment about not signaling all underflow cases is
> almost certainly incorrect.  It has been there since the
> initial commit of the file.
> 
> There is a bit of code below that sets underflow with
> float_flag_oflush_denormal, which is probably the fix
> for whatever the original case may have been.
> 
> Cc: Yongbok Kim 
> Cc: Philippe Mathieu-Daudé 
> Signed-off-by: Richard Henderson 
> ---
>  target/mips/tcg/msa_helper.c | 32 ++--
>  1 file changed, 10 insertions(+), 22 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé