Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Gerd Hoffmann]

On Tue, Nov 08, 2022 at 05:23:24PM +0100, Claudio Fontana wrote:
> The GTK Clipboard implementation may cause guest hangs.
> 
> Therefore implement a new configure switch --enable-gtk-clipboard,
> disabled by default, as a meson option.

Hmm, I was thinking about a runtime option, add 'clipboard' bool to
DisplayGTK in qapi/ui.json) and just skip the call to
gd_clipboard_init() unless the option is explicitly enabled ...

I don't feel like vetoing a compile time option though, so in case you
prefer to stick with this:

Acked-by: Gerd Hoffmann 

take care,
  Gerd

Re: [PATCH] qapi/block-core: Fix BlockdevOptionsNvmeIoUring @path description

[Stefano Garzarella]

On Tue, Nov 08, 2022 at 02:23:47PM +, Alberto Faria wrote:

The nvme-io_uring BlockDriver's path option must point at the character
device of an NVMe namespace, not at an image file.

Fixes: fd66dbd424f5 ("blkio: add libblkio block driver")
Suggested-by: Stefano Garzarella 
Signed-off-by: Alberto Faria 
---
qapi/block-core.json | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)


Thanks for fixing this:

Reviewed-by: Stefano Garzarella 



diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6d904004f8..95ac4fa634 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3704,7 +3704,7 @@
#
# Driver specific block device options for the nvme-io_uring backend.
#
-# @path: path to the image file
+# @path: path to the NVMe namespace's character device (e.g. /dev/ng0n1).
#
# Since: 7.2
##
--
2.38.1

Re: [PATCH V2 0/4] hw/block/nvme: Implement ZNS finish-zone ZDC AEN

[Klaus Jensen]

On Oct 21 16:10, clay.may...@kioxia.com wrote:
> From: Clay Mayers 
> 
> ZNS controllers have the option to limit the time a zone can remain in
> the active state.  It begins with a background process in the controller
> setting the finish-zone-recommended FZR attribute for a zone.  As part of
> setting this attribute, the zone's id is added to the namespace's
> zone-descriptor-changed (ZDC) log page. If enabled, items added to the
> ZDC log page generate a ZDC "asynchronous event notification" AEN. Optionally,
> the control can induce a "zone excursion" forcing the zone into the finished
> state that also generates a ZDC event.
> 
> Zone enabled applications need to properly handle ZDC events. In a real 
> device,
> the timeout is many hours making testing an application difficult.
> Implemented is the generation of FZR ZDC events to speed up O/S and 
> application
> testing.
> 
> Added to the zoned NVMe command set is an optional, per-namespace timer
> (zoned.finish_time) to set the FZR attr for long-lived active zones; A per
> namespace ZDC log page; AEN results to including CQE.DW1 (the NSID of the ZDC
> AEN) and generating a ZDC AEN if it's been enabled. Zone excursions are not
> modeled.
> 
> See section 5.5 of the NVMe Zoned Namespace Command Set Specification v1.1
> for more details.
> 
> Changes since v1
> - Fixed offset length checking in zdc log page
> - Moved zdc_event_queued to the patch 4
> - Unwatched zdc events in nvme_exit()
> 
> Clay Mayers (4):
>   hw/block/nvme: add ZONE_FINISH_RECOMMENDED functionality
>   hw/block/nvme: add zone descriptor changed log page
>   hw/block/nvme: supply dw1 for aen result
>   hw/block/nvme: add zone descriptor changed AEN
> 
>  docs/system/devices/nvme.rst |   5 +
>  hw/nvme/ctrl.c   | 174 +--
>  hw/nvme/ns.c |  15 +++
>  hw/nvme/nvme.h   |  37 +++-
>  hw/nvme/trace-events |   3 +-
>  include/block/nvme.h |  14 ++-
>  6 files changed, 233 insertions(+), 15 deletions(-)
> 
> -- 
> 2.27.0
> 

Nice work Clay!

Series looks good to me,

Reviewed-by: Klaus Jensen 


signature.asc
Description: PGP signature

[PATCH] util/qemu-config: Fix "query-command-line-options" to provide the right values

[Thomas Huth]

The "query-command-line-options" command uses a hand-crafted list
of options that should be returned for the "machine" parameter.
This is pretty much out of sync with reality, for example settings
like "kvm_shadow_mem" or "accel" are not parameters for the machine
anymore. Also, there is no distinction between the targets here, so
e.g. the s390x-specific values like "loadparm" in this list also
show up with the other targets like x86_64.

Let's fix this now by geting rid of the hand-crafted list and by
querying the properties of the machine class instead to assemble
the list.

Fixes: 0a7cf217d8 ("fix regression of qmp_query_command_line_options")
Signed-off-by: Thomas Huth 
---
 util/qemu-config.c | 141 -
 1 file changed, 50 insertions(+), 91 deletions(-)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 433488aa56..097c6c3939 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -8,6 +8,7 @@
 #include "qemu/error-report.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
+#include "qom/object.h"
 
 static QemuOptsList *vm_config_groups[48];
 static QemuOptsList *drive_config_groups[5];
@@ -149,97 +150,55 @@ static CommandLineParameterInfoList 
*get_drive_infolist(void)
 return head;
 }
 
-/* restore machine options that are now machine's properties */
-static QemuOptsList machine_opts = {
-.merge_lists = true,
-.head = QTAILQ_HEAD_INITIALIZER(machine_opts.head),
-.desc = {
-{
-.name = "type",
-.type = QEMU_OPT_STRING,
-.help = "emulated machine"
-},{
-.name = "accel",
-.type = QEMU_OPT_STRING,
-.help = "accelerator list",
-},{
-.name = "kernel_irqchip",
-.type = QEMU_OPT_BOOL,
-.help = "use KVM in-kernel irqchip",
-},{
-.name = "kvm_shadow_mem",
-.type = QEMU_OPT_SIZE,
-.help = "KVM shadow MMU size",
-},{
-.name = "kernel",
-.type = QEMU_OPT_STRING,
-.help = "Linux kernel image file",
-},{
-.name = "initrd",
-.type = QEMU_OPT_STRING,
-.help = "Linux initial ramdisk file",
-},{
-.name = "append",
-.type = QEMU_OPT_STRING,
-.help = "Linux kernel command line",
-},{
-.name = "dtb",
-.type = QEMU_OPT_STRING,
-.help = "Linux kernel device tree file",
-},{
-.name = "dumpdtb",
-.type = QEMU_OPT_STRING,
-.help = "Dump current dtb to a file and quit",
-},{
-.name = "phandle_start",
-.type = QEMU_OPT_NUMBER,
-.help = "The first phandle ID we may generate dynamically",
-},{
-.name = "dt_compatible",
-.type = QEMU_OPT_STRING,
-.help = "Overrides the \"compatible\" property of the dt root 
node",
-},{
-.name = "dump-guest-core",
-.type = QEMU_OPT_BOOL,
-.help = "Include guest memory in  a core dump",
-},{
-.name = "mem-merge",
-.type = QEMU_OPT_BOOL,
-.help = "enable/disable memory merge support",
-},{
-.name = "usb",
-.type = QEMU_OPT_BOOL,
-.help = "Set on/off to enable/disable usb",
-},{
-.name = "firmware",
-.type = QEMU_OPT_STRING,
-.help = "firmware image",
-},{
-.name = "iommu",
-.type = QEMU_OPT_BOOL,
-.help = "Set on/off to enable/disable Intel IOMMU (VT-d)",
-},{
-.name = "suppress-vmdesc",
-.type = QEMU_OPT_BOOL,
-.help = "Set on to disable self-describing migration",
-},{
-.name = "aes-key-wrap",
-.type = QEMU_OPT_BOOL,
-.help = "enable/disable AES key wrapping using the CPACF wrapping 
key",
-},{
-.name = "dea-key-wrap",
-.type = QEMU_OPT_BOOL,
-.help = "enable/disable DEA key wrapping using the CPACF wrapping 
key",
-},{
-.name = "loadparm",
-.type = QEMU_OPT_STRING,
-.help = "Up to 8 chars in set of [A-Za-z0-9. ](lower case chars"
-" converted to upper case) to pass to machine"
-" loader, boot manager, and guest kernel",
-},
-{ /* End of list */ }
+static CommandLineParameterInfoList *query_machine_properties(void)
+{
+CommandLineParameterInfoList *param_list = NULL;
+CommandLineParameterInfo *info;
+ObjectPropertyIterator iter;
+ObjectProperty *prop;
+ObjectClass *oc;
+
+oc = object_get_class(container_get(object_get_root(), "/machine"));
+assert(oc);
+
+/* Add entry for the "type" parameter */
+info = g_malloc0(sizeof(*info));
+info->name = g_strdup("type");
+  

Re: [PULL v4 29/83] virtio: introduce virtio_queue_enable()

[Laszlo Ersek]

On 11/09/22 07:59, Michael S. Tsirkin wrote:
> On Wed, Nov 09, 2022 at 01:52:01AM -0500, Michael S. Tsirkin wrote:
>> On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
>>> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi  wrote:

 On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin  wrote:
>
> From: Kangjie Xu 
>
> Introduce the interface queue_enable() in VirtioDeviceClass and the
> fucntion virtio_queue_enable() in virtio, it can be called when
> VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
> started. It only supports the devices of virtio 1 or later. The
> not-supported devices can only start the virtqueue when DRIVER_OK.
>
> Signed-off-by: Kangjie Xu 
> Signed-off-by: Xuan Zhuo 
> Acked-by: Jason Wang 
> Message-Id: <20221017092558.111082-4-xuanz...@linux.alibaba.com>
> Reviewed-by: Michael S. Tsirkin 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  include/hw/virtio/virtio.h |  2 ++
>  hw/virtio/virtio.c | 14 ++
>  2 files changed, 16 insertions(+)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 74d76c1dbc..b00b3fcf31 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -149,6 +149,7 @@ struct VirtioDeviceClass {
>  void (*reset)(VirtIODevice *vdev);
>  void (*set_status)(VirtIODevice *vdev, uint8_t val);
>  void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> +void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
>  /* For transitional devices, this is a bitmap of features
>   * that are only exposed on the legacy interface but not
>   * the modern one.
> @@ -288,6 +289,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice 
> *vdev, int n,
>  int virtio_set_status(VirtIODevice *vdev, uint8_t val);
>  void virtio_reset(void *opaque);
>  void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
>  void virtio_update_irq(VirtIODevice *vdev);
>  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index cf5f9ca387..9683b2e158 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev, 
> uint32_t queue_index)
>  __virtio_queue_reset(vdev, queue_index);
>  }
>
> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> +{
> +VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +
> +if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +error_report("queue_enable is only suppported in devices of 
> virtio "
> + "1.0 or later.");

 Why is this triggering here? Maybe virtio_queue_enable() is called too
 early. I have verified that the Linux guest driver sets VERSION_1. I
 didn't check what SeaBIOS does.
>>>
>>> Looks like a bug, we should check device features here at least and it
>>> should be guest errors instead of error_report() here.
>>>
>>> Thanks
>>
>> But it's weird, queue enable is written before guest features?
>> How come?
>
> It's a bios bug:
>
>
>
> vp_init_simple(&vdrive->vp, pci);
> if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
> dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
> goto fail;
> }
>
> if (vdrive->vp.use_modern) {
> struct vp_device *vp = &vdrive->vp;
> u64 features = vp_get_features(vp);
> u64 version1 = 1ull << VIRTIO_F_VERSION_1;
> u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
> u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
> if (!(features & version1)) {
> dprintf(1, "modern device without virtio_1 feature bit: %pP\n", 
> pci);
> goto fail;
> }
>
> features = features & (version1 | iommu_platform | blk_size);
> vp_set_features(vp, features);
> status |= VIRTIO_CONFIG_S_FEATURES_OK;
> vp_set_status(vp, status);
>
>
>
> Not good - does not match the spec. Here's what the spec says:
>
>
> The driver MUST follow this sequence to initialize a device:
> 1. Reset the device.
> 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
> 3. Set the DRIVER status bit: the guest OS knows how to drive the device.
> 4. Read device feature bits, and write the subset of feature bits understood 
> by the OS and driver to the
> device. During this step the driver MAY read (but MUST NOT write) the 
> device-specific configuration
> fields to check that it can support the device before accepting it.
> 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature 
> bits after this step.
> 6. Re-read device status to ensure the FEA

Re: [PATCH 01/13] qed: Don't yield in bdrv_qed_co_drain_begin()

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

We want to change .bdrv_co_drained_begin() back to be a non-coroutine
callback, so in preparation, avoid yielding in its implementation.

Because we increase bs->in_flight and bdrv_drained_begin() polls, the
behaviour is unchanged.

Signed-off-by: Kevin Wolf


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 01/13] qed: Don't yield in bdrv_qed_co_drain_begin()

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

  int ret;
  
  trace_qed_need_check_timer_cb(s);

@@ -310,9 +309,20 @@ static void coroutine_fn qed_need_check_timer_entry(void 
*opaque)
  (void) ret;
  }
  
+static void coroutine_fn qed_need_check_timer_entry(void *opaque)

+{
+BDRVQEDState *s = opaque;
+
+qed_need_check_timer(opaque);
+bdrv_dec_in_flight(s->bs);


hmm, one question: don't we need aio_wait_kick() call here?


+}
+
  static void qed_need_che


--
Best regards,
Vladimir

Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()

[Bin Meng]

Hi,

On Mon, Nov 7, 2022 at 7:08 PM Mauro Matteo Cascella
 wrote:
>
> On Mon, Nov 7, 2022 at 11:35 AM Mauro Matteo Cascella
>  wrote:
> >
> > Make sure to reset data_count if it's equal to (or exceeds) block_size.
> > This prevents an off-by-one read / write when accessing s->fifo_buffer
> > in sdhci_read_dataport / sdhci_write_dataport, both called right after
> > sdhci_buff_access_is_sequential.
> >
> > Fixes: CVE-2022-3872
> > Reported-by: RivenDell 
> > Reported-by: Siqi Chen 
> > Reported-by: ningqiang 
> > Signed-off-by: Mauro Matteo Cascella 
> > ---
> >  hw/sd/sdhci.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 306070c872..aa2fd79df2 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
> >  static inline bool
> >  sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
> >  {
> > +if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
> > +s->data_count = 0;
> > +}
> > +
> >  if ((s->data_count & 0x3) != byte_num) {
> >  trace_sdhci_error("Non-sequential access to Buffer Data Port 
> > register"
> >"is prohibited\n");
> > --
> > 2.38.1
> >
>
> Reproducer:
>
> cat << EOF | ./qemu-system-x86_64 -machine accel=qtest \
> -nodefaults -drive if=none,index=0,file=null-co://,format=raw,id=mydrive \
> -device sdhci-pci -device sd-card,drive=mydrive -nographic -qtest stdio
> outl 0xcf8 0x80001004
> outl 0xcfc 0x107
> outl 0xcf8 0x80001010
> outl 0xcfc 0xfebf1000
> writel 0xfebf102c 0x7
> writel 0xfebf1004 0x10200
> writel 0xfebf100c 0x20
> writel 0xfebf1028 0x1
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfeb

Re: [PATCH v3 2/4] python/qmp: increase read buffer size

[Daniel P . Berrangé]

On Tue, Nov 08, 2022 at 03:38:21PM -0500, John Snow wrote:
> On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov
>  wrote:
> >
> > After modification of "query-machines" command the buffer size should be
> > more than 452kB to contain output with compat-props.
> >
> > Signed-off-by: Maksim Davydov 
> > Reviewed-by: Vladimir Sementsov-Ogievskiy 
> > ---
> >  python/qemu/qmp/qmp_client.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/python/qemu/qmp/qmp_client.py b/python/qemu/qmp/qmp_client.py
> > index 5dcda04a75..659fe4d98c 100644
> > --- a/python/qemu/qmp/qmp_client.py
> > +++ b/python/qemu/qmp/qmp_client.py
> > @@ -197,8 +197,8 @@ async def run(self, address='/tmp/qemu.socket'):
> >  #: Logger object used for debugging messages.
> >  logger = logging.getLogger(__name__)
> >
> > -# Read buffer limit; large enough to accept query-qmp-schema
> > -_limit = (256 * 1024)
> > +# Read buffer limit; large enough to accept query-machines
> > +_limit = (512 * 1024)
> 
> wow :)

Meanwhile over in python/qemu/qmp/protocol.py the read buffer limit is
set to just 64 kb.

If the current output of a particular command is known to 450 kb, then
setting this limit to 512 kb is waaay to conservative, and we'll
inevitably have to change it again when someone finds the next command
that overflows.

Recall this thread

   https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg01060.html

In fact, let me be the someone who demonstrates a real case where 512kb
is not enough

Create a 200 deep chain of qcow2 files

   qemu-img create -f qcow2 demo0.img 1G
   j=0
   for i in `seq 1 200`
   do
 qemu-img create -f qcow2 -o backing_file=demo$j.qcow2 \
  -o backing_fmt=qcow2 demo$i.qcow2
 j=$i
   done

Launch qemu with that, and then run

  "query-named-block-nodes"

and (many minutes later) you'll get an 86 MB  QMP reply.

Now, a bare no-arg "query-named-block-nodes" is known to be a bad
command as it returns data which is a combinatorial expansion of
the number of block nodes.  Essentially it is broken by design,
and no one should use it, without setting flat=true.


If we repeat it with flat=true, then we can get a 2.7 MB QMP
reply.  This is large, but a valid reply. Basically 13 KB of
data per qcow2 file.

Libvirt caps the backing chain depth it is willing to accept at 200
qcow2 files, but allows multiple disks. So with 4 disks, each with
200 deep chain, you'll get 10.8 MB of json back. Opps

...Libvirt's QMP reply limit is 10 MB, so even libvirt will break
quite quickly. Libvirt can cope with around 787 qcow2 files in
at 13 kb per file. NB I'm assuming paths under the dir
/var/lib/libvirt/images/, shorter paths will allow for more.


The 64 KB limit will only cope with 4 qcow2 files before breaking,
while a 512 KB limit will only cope with 39 files before breaking.
Neither is anywhere near sufficient.

I'd suggest following libvirt here and setting the read limits to
10 MB.

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

Re: [PATCH v2] qom.json: default the prealloc-threads to smp-cpus

[Markus Armbruster]

The subject is misleading, I'm afraid.  It suggests you're changing the
default.  You don't, you just fix its documentation.

Zhenyu Zhang  writes:

> Since the amount of prealloc-threads to smp-cpus is
> defaulted in hostmem, so sync this information.

Covering history could be helpful.

Here's my try

qapi/qom: Memory backend property prealloc-threads doc fix

Commit ffac16fab3 "hostmem: introduce "prealloc-threads" property"
(v5.0.0) changed the default number of threads from number of CPUs
to 1.  This was deemed a regression, and fixed in commit f8d426a685
"hostmem: default the amount of prealloc-threads to smp-cpus".
Except the documentation remained unchanged.  Update it now.

>
> Signed-off-by: Zhenyu Zhang 

The following part ...

> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg919682.html
>
> Changelog
> =
> v2:
>   * This property is available since 5.0. (Philippe)
> ---

... needs to go below the --- line, so it doesn't go into git.

>  qapi/qom.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 87fcad2423..b2f6bceec7 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -576,7 +576,7 @@
>  #
>  # @prealloc: if true, preallocate memory (default: false)
>  #
> -# @prealloc-threads: number of CPU threads to use for prealloc (default: 1)
> +# @prealloc-threads: number of CPU threads to use for prealloc (default: 
> smp-cpus) (since 5.0)

Long line.

"smp-cpus" is not defined.  It's QOM property /machine/smp member @cpus,
commonly set with -M smp.cpus=N (or its sugared form -smp cpus=N).

Suggest

   # @prealloc-threads: number of CPU threads to use for prealloc
   # (default: number of CPUs) (since 5.0)

>  #
>  # @prealloc-context: thread context to use for creation of preallocation 
> threads
>  #(default: none) (since 7.2)

Re: Questions about QEMU exception

[Alex Bennée]

"Li, Kevin"  writes:

> Hi qemu community,
>
>  
>
> We are working on some open source project which uses qemu on mac, and we 
> have some signing process to sign
> qemu-system-x86_64.
>
> If qemu-system-x86_64 is not signed, we don’t see any problem, but after sign 
> it, we got the following error:
>
>  
>
> qemu-system-x86_64 -M none -netdev help]: stdout=\"Accelerators supported in 
> QEMU binary:\\ntcg\\nhax\\nhvf\\n\",
> stderr=\"qemu-system-x86_64: allocate 1073741824 bytes for jit buffer: 
> Invalid argument
>
>  
>
> Does anyone has clue about what change may result in this failure?

Not sure about the details but I suspect this is something to do with
the way we have to jump through hoops to allocate the code buffer on
MacOSX.

You could stick some printfs around:

  alloc_code_gen_buffer_splitwx_vmremap

and

  alloc_code_gen_buffer_anon

AIUI we have to allocate a non-executable but writable buffer for the
code and then remap an executable view of the same region to execute the
generated JIT code. Maybe being a signed binary adds more restrictions
on which OS apis can be called?

-- 
Alex Bennée

Re: [PATCH V2] hw/riscv: virt: Remove size restriction for pflash

[Sunil V L]

Thanks a lot for the feedback and history. I understand now there are good
reasons to mandate fixed size flash. Will drop this patch.

Thanks
Sunil

Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()

[Mauro Matteo Cascella]

On Wed, Nov 9, 2022 at 10:45 AM Siqi Chen  wrote:
>
> Hi,
>
> >This reproducer does not crash my QEMU. Am I missing anything?
> I submitted the reproducer. Because the overflow is only one byte, it may not 
> be detected by the host's heap allocator.  Do you compile your qemu with 
> sanitizer?  This is my build configuration: "./configure 
> --target-list=x86_64-softmmu --enable-sanitizers"

Right, you need to recompile QEMU with ASAN support. This is an
excerpt of the stack trace:

=
==39159==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x61522880 at pc 0x55b023db5fe1 bp 0x7fffeeef1650 sp
0x7fffeeef1648
WRITE of size 1 at 0x61522880 thread T0
#0 0x55b023db5fe0 in sdhci_write_dataport ../../hw/sd/sdhci.c:562
#1 0x55b023dc1cc7 in sdhci_write ../../hw/sd/sdhci.c:1216
#2 0x55b024521e01 in memory_region_write_accessor ../../softmmu/memory.c:492
#3 0x55b0245222ab in access_with_adjusted_size ../../softmmu/memory.c:554
#4 0x55b02452ff15 in memory_region_dispatch_write
../../softmmu/memory.c:1514
#5 0x55b024568c67 in flatview_write_continue ../../softmmu/physmem.c:2814
#6 0x55b02456908d in flatview_write ../../softmmu/physmem.c:2856
#7 0x55b024569a74 in address_space_write ../../softmmu/physmem.c:2952
#8 0x55b02457a63c in qtest_process_command ../../softmmu/qtest.c:538
#9 0x55b02457ef97 in qtest_process_inbuf ../../softmmu/qtest.c:796
#10 0x55b02457f089 in qtest_read ../../softmmu/qtest.c:808
#11 0x55b0249d4372 in qemu_chr_be_write_impl ../../chardev/char.c:201
#12 0x55b0249d4414 in qemu_chr_be_write ../../chardev/char.c:213
#13 0x55b0249db586 in fd_chr_read ../../chardev/char-fd.c:72
#14 0x55b02466ba5b in qio_channel_fd_source_dispatch
../../io/channel-watch.c:84
#15 0x7f88093af0ae in g_main_context_dispatch
(/lib64/libglib-2.0.so.0+0x550ae)
#16 0x55b024c5ff14 in glib_pollfds_poll ../../util/main-loop.c:297
#17 0x55b024c600fa in os_host_main_loop_wait ../../util/main-loop.c:320
#18 0x55b024c603f3 in main_loop_wait ../../util/main-loop.c:596
#19 0x55b023fcca21 in qemu_main_loop ../../softmmu/runstate.c:726
#20 0x55b023679735 in qemu_main ../../softmmu/main.c:36
#21 0x55b023679766 in main ../../softmmu/main.c:45
#22 0x7f8808728f5f in __libc_start_call_main (/lib64/libc.so.6+0x40f5f)
#23 0x7f880872900f in __libc_start_main_impl (/lib64/libc.so.6+0x4100f)
#24 0x55b023679644 in _start (./qemu-system-x86_64+0x20f2644)

> Thanks,
> Siqi Chen.
>
>
>
> Bin Meng  于2022年11月9日周三 17:30写道：
>>
>> Hi,
>>
>> On Mon, Nov 7, 2022 at 7:08 PM Mauro Matteo Cascella
>>  wrote:
>> >
>> > On Mon, Nov 7, 2022 at 11:35 AM Mauro Matteo Cascella
>> >  wrote:
>> > >
>> > > Make sure to reset data_count if it's equal to (or exceeds) block_size.
>> > > This prevents an off-by-one read / write when accessing s->fifo_buffer
>> > > in sdhci_read_dataport / sdhci_write_dataport, both called right after
>> > > sdhci_buff_access_is_sequential.
>> > >
>> > > Fixes: CVE-2022-3872
>> > > Reported-by: RivenDell 
>> > > Reported-by: Siqi Chen 
>> > > Reported-by: ningqiang 
>> > > Signed-off-by: Mauro Matteo Cascella 
>> > > ---
>> > >  hw/sd/sdhci.c | 4 
>> > >  1 file changed, 4 insertions(+)
>> > >
>> > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> > > index 306070c872..aa2fd79df2 100644
>> > > --- a/hw/sd/sdhci.c
>> > > +++ b/hw/sd/sdhci.c
>> > > @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
>> > >  static inline bool
>> > >  sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
>> > >  {
>> > > +if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
>> > > +s->data_count = 0;
>> > > +}
>> > > +
>> > >  if ((s->data_count & 0x3) != byte_num) {
>> > >  trace_sdhci_error("Non-sequential access to Buffer Data Port 
>> > > register"
>> > >"is prohibited\n");
>> > > --
>> > > 2.38.1
>> > >
>> >
>> > Reproducer:
>> >
>> > cat << EOF | ./qemu-system-x86_64 -machine accel=qtest \
>> > -nodefaults -drive if=none,index=0,file=null-co://,format=raw,id=mydrive \
>> > -device sdhci-pci -device sd-card,drive=mydrive -nographic -qtest stdio
>> > outl 0xcf8 0x80001004
>> > outl 0xcfc 0x107
>> > outl 0xcf8 0x80001010
>> > outl 0xcfc 0xfebf1000
>> > writel 0xfebf102c 0x7
>> > writel 0xfebf1004 0x10200
>> > writel 0xfebf100c 0x20
>> > writel 0xfebf1028 0x1
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0x

Re: Questions about QEMU exception

[Peter Maydell]

On Wed, 9 Nov 2022 at 01:53, Li, Kevin  wrote:
>
> Hi qemu community,
>
>
>
> We are working on some open source project which uses qemu on mac, and we 
> have some signing process to sign qemu-system-x86_64.
>
> If qemu-system-x86_64 is not signed, we don’t see any problem, but after sign 
> it, we got the following error:
>
>
>
> qemu-system-x86_64 -M none -netdev help]: stdout=\"Accelerators supported in 
> QEMU binary:\\ntcg\\nhax\\nhvf\\n\", stderr=\"qemu-system-x86_64: allocate 
> 1073741824 bytes for jit buffer: Invalid argument
>
>
>
> Does anyone has clue about what change may result in this failure?

You don't say which QEMU version you're using. Does it still happen
with the most recent release? Does it still happen if you build
from current head-of-git ?

PS: I think the QEMU build process should already be signing the executable,
so I'm not sure why you need to sign it again (see scripts/entitlement.sh).

thanks
-- PMM

Re: [PATCH 2/3] hvf: implement guest debugging on Apple Silicon hosts

[Mads Ynddal]

> On 8 Nov 2022, at 12.51, Mads Ynddal  wrote:
> 
> I also noticed you are adding 1 to the WRPs and BRPs. As I interpret the
> documentation, you should subtract 1 instead, given the value 0 is reserved:

I tested it, and you do have to add 1. I guess it's implied that you cannot have
less than 2 WRPs/BRPs then.

Re: [PATCH] Revert "hw/block/pflash_cfi0{1, 2}: Error out if device length isn't a power of two"

[Daniel Henrique Barboza]

On 11/8/22 15:13, Stefan Hajnoczi wrote:

On Tue, 8 Nov 2022 at 13:10, Philippe Mathieu-Daudé  wrote:


On 8/11/22 18:26, Daniel Henrique Barboza wrote:

This commit caused a regression [1] that prevents machines that uses
Open Virtual Machine Firmware (OVMF) to boot.

This is a long standing behavior with how pflash handles images. More
information about why this happens can be found in [2] and commit
06f1521795 ("pflash: Require backend size to match device, improve
errors").

This reverts commit 334c388f25707a234c4a0dea05b9df08d7746638.

[1] https://gitlab.com/qemu-project/qemu/-/issues/1294
[2] https://lore.kernel.org/qemu-devel/20190308062455.29755-1-arm...@redhat.com/

Cc: Bernhard Beschow 
Cc: Philippe Mathieu-Daudé 
Cc: Stefan Hajnoczi 
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1294
Signed-off-by: Daniel Henrique Barboza 
---
   hw/block/pflash_cfi01.c | 8 ++--
   hw/block/pflash_cfi02.c | 5 -
   2 files changed, 2 insertions(+), 11 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Thanks, our patches crossed :)
https://lore.kernel.org/qemu-devel/20221108175755.95141-1-phi...@linaro.org/


Well of course that we both decided to send the PR almost at the same time :D



I'm queuing yours which is first and will amend the description
(if you don't disagree).


I've already applied yours, Philippe, because the description is more
comprehensive.

Daniel, thank you for sending your version of the patch!


Thank you Phil for sending the revert and Stefan for quickly queueing
it. The sooner we get rid of the regression the better.


Daniel





Stefan

Re: [PATCH 02/13] test-bdrv-drain: Don't yield in .bdrv_co_drained_begin/end()

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

We want to change .bdrv_co_drained_begin/end() back to be non-coroutine
callbacks, so in preparation, avoid yielding in their implementation.

This does almost the same as the existing logic in bdrv_drain_invoke(),
by creating and entering coroutines internally. However, since the test
case is by far the heaviest user of coroutine code in drain callbacks,
it is preferable to have the complexity in the test case rather than the
drain core, which is already complicated enough without this.

The behaviour for bdrv_drain_begin() is unchanged because we increase
bs->in_flight and this is still polled. However, bdrv_drain_end()
doesn't wait for the spawned coroutine to complete any more. This is
fine, we don't rely on bdrv_drain_end() restarting all operations
immediately before the next aio_poll().

Signed-off-by: Kevin Wolf 
---
  tests/unit/test-bdrv-drain.c | 64 ++--
  1 file changed, 46 insertions(+), 18 deletions(-)

diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 09dc4a4891..24f34e24ad 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -38,12 +38,22 @@ typedef struct BDRVTestState {
  bool sleep_in_drain_begin;
  } BDRVTestState;
  
+static void coroutine_fn sleep_in_drain_begin(void *opaque)

+{
+BlockDriverState *bs = opaque;
+
+qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10);
+bdrv_dec_in_flight(bs);
+}
+
  static void coroutine_fn bdrv_test_co_drain_begin(BlockDriverState *bs)
  {
  BDRVTestState *s = bs->opaque;
  s->drain_count++;
  if (s->sleep_in_drain_begin) {
-qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10);
+Coroutine *co = qemu_coroutine_create(sleep_in_drain_begin, bs);
+bdrv_inc_in_flight(bs);
+aio_co_enter(bdrv_get_aio_context(bs), co);
  }
  }
  
@@ -1916,6 +1926,21 @@ static int coroutine_fn bdrv_replace_test_co_preadv(BlockDriverState *bs,

  return 0;
  }
  
+static void coroutine_fn bdrv_replace_test_drain_co(void *opaque)

+{
+BlockDriverState *bs = opaque;
+BDRVReplaceTestState *s = bs->opaque;
+
+/* Keep waking io_co up until it is done */
+while (s->io_co) {
+aio_co_wake(s->io_co);
+s->io_co = NULL;
+qemu_coroutine_yield();
+}
+s->drain_co = NULL;
+bdrv_dec_in_flight(bs);
+}


Same question, don't we need aio_wait_kick() after decrement in_flight.

Also, seems we have here extra waiting level: a special coroutine that waits in 
a loop.

Could we just do in .drain_begin:

if (s->io_co) {
   bdrv_inc_in_flight(bs);
}

and in .co_preadv instead of waking s->drain_co simply

if (s->drain_count == 1) {
  bdrv_dec_in_flight(bs);
  aio_wait_kick();
}


or even better, do inc in_flight when io_co becomes not NULL.


+
  /**
   * If .drain_count is 0, wake up .io_co if there is one; and set
   * .was_drained.
@@ -1926,20 +1951,27 @@ static void coroutine_fn 
bdrv_replace_test_co_drain_begin(BlockDriverState *bs)
  BDRVReplaceTestState *s = bs->opaque;
  
  if (!s->drain_count) {

-/* Keep waking io_co up until it is done */
-s->drain_co = qemu_coroutine_self();
-while (s->io_co) {
-aio_co_wake(s->io_co);
-s->io_co = NULL;
-qemu_coroutine_yield();
-}
-s->drain_co = NULL;
-
+s->drain_co = qemu_coroutine_create(bdrv_replace_test_drain_co, bs);
+bdrv_inc_in_flight(bs);
+aio_co_enter(bdrv_get_aio_context(bs), s->drain_co);
  s->was_drained = true;
  }
  s->drain_count++;
  }
  
+static void coroutine_fn bdrv_replace_test_read_entry(void *opaque)

+{
+BlockDriverState *bs = opaque;
+char data;
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, &data, 1);
+int ret;
+
+/* Queue a read request post-drain */
+ret = bdrv_replace_test_co_preadv(bs, 0, 1, &qiov, 0);
+g_assert(ret >= 0);
+bdrv_dec_in_flight(bs);
+}
+
  /**
   * Reduce .drain_count, set .was_undrained once it reaches 0.
   * If .drain_count reaches 0 and the node has a backing file, issue a
@@ -1951,17 +1983,13 @@ static void coroutine_fn 
bdrv_replace_test_co_drain_end(BlockDriverState *bs)
  
  g_assert(s->drain_count > 0);

  if (!--s->drain_count) {
-int ret;
-
  s->was_undrained = true;
  
  if (bs->backing) {

-char data;
-QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, &data, 1);
-
-/* Queue a read request post-drain */
-ret = bdrv_replace_test_co_preadv(bs, 0, 1, &qiov, 0);
-g_assert(ret >= 0);
+Coroutine *co = qemu_coroutine_create(bdrv_replace_test_read_entry,
+  bs);
+bdrv_inc_in_flight(bs);
+aio_co_enter(bdrv_get_aio_context(bs), co);
  }
  }
  }


--
Best regards,
Vladimir

[PATCH 0/2] hw/nvme: errp fixes

[Klaus Jensen]

From: Klaus Jensen 

Fix a couple of invalid errp usages.

Klaus Jensen (2):
  hw/nvme: fix incorrect use of errp/local_err
  hw/nvme: cleanup error reporting in nvme_init_pci()

 hw/nvme/ctrl.c | 60 --
 1 file changed, 29 insertions(+), 31 deletions(-)

-- 
2.38.1

[PATCH 2/2] hw/nvme: cleanup error reporting in nvme_init_pci()

[Klaus Jensen]

From: Klaus Jensen 

Replace the local Error variable with errp and ERRP_GUARD().

Signed-off-by: Klaus Jensen 
---
 hw/nvme/ctrl.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 4cc6ae753295..38eb5ec54f9d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7345,13 +7345,13 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, 
uint8_t offset)
 
 static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 {
+ERRP_GUARD();
+
 uint8_t *pci_conf = pci_dev->config;
 uint64_t bar_size;
 unsigned msix_table_offset, msix_pba_offset;
 int ret;
 
-Error *err = NULL;
-
 pci_conf[PCI_INTERRUPT_PIN] = 1;
 pci_config_set_prog_interface(pci_conf, 0x2);
 
@@ -7388,13 +7388,13 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice 
*pci_dev, Error **errp)
 }
 ret = msix_init(pci_dev, n->params.msix_qsize,
 &n->bar0, 0, msix_table_offset,
-&n->bar0, 0, msix_pba_offset, 0, &err);
+&n->bar0, 0, msix_pba_offset, 0, errp);
 if (ret < 0) {
 if (ret == -ENOTSUP) {
-warn_report_err(err);
+warn_report_err(*errp);
+*errp = NULL;
 } else {
-error_propagate(errp, err);
-return ret;
+return -1;
 }
 }
 
-- 
2.38.1

[PATCH 1/2] hw/nvme: fix incorrect use of errp/local_err

[Klaus Jensen]

From: Klaus Jensen 

Make nvme_check_constraints() return an int and fix incorrect use of
errp/local_err.

Signed-off-by: Klaus Jensen 
---
 hw/nvme/ctrl.c | 48 +++-
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index ac3885ce5079..4cc6ae753295 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7035,7 +7035,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
 },
 };
 
-static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
+static int nvme_check_params(NvmeCtrl *n, Error **errp)
 {
 NvmeParams *params = &n->params;
 
@@ -7049,38 +7049,38 @@ static void nvme_check_constraints(NvmeCtrl *n, Error 
**errp)
 if (n->namespace.blkconf.blk && n->subsys) {
 error_setg(errp, "subsystem support is unavailable with legacy "
"namespace ('drive' property)");
-return;
+return -1;
 }
 
 if (params->max_ioqpairs < 1 ||
 params->max_ioqpairs > NVME_MAX_IOQPAIRS) {
 error_setg(errp, "max_ioqpairs must be between 1 and %d",
NVME_MAX_IOQPAIRS);
-return;
+return -1;
 }
 
 if (params->msix_qsize < 1 ||
 params->msix_qsize > PCI_MSIX_FLAGS_QSIZE + 1) {
 error_setg(errp, "msix_qsize must be between 1 and %d",
PCI_MSIX_FLAGS_QSIZE + 1);
-return;
+return -1;
 }
 
 if (!params->serial) {
 error_setg(errp, "serial property not set");
-return;
+return -1;
 }
 
 if (n->pmr.dev) {
 if (host_memory_backend_is_mapped(n->pmr.dev)) {
 error_setg(errp, "can't use already busy memdev: %s",

object_get_canonical_path_component(OBJECT(n->pmr.dev)));
-return;
+return -1;
 }
 
 if (!is_power_of_2(n->pmr.dev->size)) {
 error_setg(errp, "pmr backend size needs to be power of 2 in 
size");
-return;
+return -1;
 }
 
 host_memory_backend_set_mapped(n->pmr.dev, true);
@@ -7089,64 +7089,64 @@ static void nvme_check_constraints(NvmeCtrl *n, Error 
**errp)
 if (n->params.zasl > n->params.mdts) {
 error_setg(errp, "zoned.zasl (Zone Append Size Limit) must be less "
"than or equal to mdts (Maximum Data Transfer Size)");
-return;
+return -1;
 }
 
 if (!n->params.vsl) {
 error_setg(errp, "vsl must be non-zero");
-return;
+return -1;
 }
 
 if (params->sriov_max_vfs) {
 if (!n->subsys) {
 error_setg(errp, "subsystem is required for the use of SR-IOV");
-return;
+return -1;
 }
 
 if (params->sriov_max_vfs > NVME_MAX_VFS) {
 error_setg(errp, "sriov_max_vfs must be between 0 and %d",
NVME_MAX_VFS);
-return;
+return -1;
 }
 
 if (params->cmb_size_mb) {
 error_setg(errp, "CMB is not supported with SR-IOV");
-return;
+return -1;
 }
 
 if (n->pmr.dev) {
 error_setg(errp, "PMR is not supported with SR-IOV");
-return;
+return -1;
 }
 
 if (!params->sriov_vq_flexible || !params->sriov_vi_flexible) {
 error_setg(errp, "both sriov_vq_flexible and sriov_vi_flexible"
" must be set for the use of SR-IOV");
-return;
+return -1;
 }
 
 if (params->sriov_vq_flexible < params->sriov_max_vfs * 2) {
 error_setg(errp, "sriov_vq_flexible must be greater than or equal"
" to %d (sriov_max_vfs * 2)", params->sriov_max_vfs * 
2);
-return;
+return -1;
 }
 
 if (params->max_ioqpairs < params->sriov_vq_flexible + 2) {
 error_setg(errp, "(max_ioqpairs - sriov_vq_flexible) must be"
" greater than or equal to 2");
-return;
+return -1;
 }
 
 if (params->sriov_vi_flexible < params->sriov_max_vfs) {
 error_setg(errp, "sriov_vi_flexible must be greater than or equal"
" to %d (sriov_max_vfs)", params->sriov_max_vfs);
-return;
+return -1;
 }
 
 if (params->msix_qsize < params->sriov_vi_flexible + 1) {
 error_setg(errp, "(msix_qsize - sriov_vi_flexible) must be"
" greater than or equal to 1");
-return;
+return -1;
 }
 
 if (params->sriov_max_vi_per_vf &&
@@ -7154,7 +7154,7 @@ static void nvme_check_constraints(NvmeCtrl *n, Error 
**errp)
 error_setg(errp, "sriov_max_vi_per_vf must meet:"
" (sriov_max_vi_per_vf - 1) %% %d == 0 and"
" sriov_max_vi_per_vf >= 1", NVME_VF_RES_GRANULARITY);
-retur

Re: [PATCH 2/3] hvf: implement guest debugging on Apple Silicon hosts

[Peter Maydell]

On Tue, 8 Nov 2022 at 11:51, Mads Ynddal  wrote:
> I also noticed you are adding 1 to the WRPs and BRPs. As I interpret the
> documentation, you should subtract 1 instead, given the value 0 is reserved:
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index dbc3605f6d..80a583cbd1 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -39,11 +39,11 @@ static void hvf_arm_init_debug(CPUState *cpu)
>  {
>  ARMCPU *arm_cpu = ARM_CPU(cpu);
>
> -max_hw_bps = 1 + extract64(arm_cpu->isar.id_aa64dfr0, 12, 4);
> +max_hw_bps = extract64(arm_cpu->isar.id_aa64dfr0, 12, 4) - 1;
>  hw_breakpoints =
>  g_array_sized_new(true, true, sizeof(HWBreakpoint), max_hw_bps);
>
> -max_hw_wps = 1 + extract64(arm_cpu->isar.id_aa64dfr0, 20, 4);
> +max_hw_wps = extract64(arm_cpu->isar.id_aa64dfr0, 20, 4) - 1;
>  hw_watchpoints =
>  g_array_sized_new(true, true, sizeof(HWWatchpoint), max_hw_wps);
>  return;
>
> But the documentation is a bit ambiguous on that. Maybe we can test it?

Adding 1 is correct -- the field definition is "number of breakpoints - 1",
so the number of bps is "field value + 1". You don't need to open-code this,
though -- there are functions arm_num_brps() and arm_num_wrps()
in target/arm/internals.h that extract the fields from the ID registers
and adjust them to give the actual number.

thanks
-- PMM

Re: [PATCH v3 2/4] python/qmp: increase read buffer size

[Daniel P . Berrangé]

On Wed, Nov 09, 2022 at 09:39:14AM +, Daniel P. Berrangé wrote:
> On Tue, Nov 08, 2022 at 03:38:21PM -0500, John Snow wrote:
> > On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov
> >  wrote:
> > >
> > > After modification of "query-machines" command the buffer size should be
> > > more than 452kB to contain output with compat-props.
> > >
> > > Signed-off-by: Maksim Davydov 
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy 
> > > ---
> > >  python/qemu/qmp/qmp_client.py | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/python/qemu/qmp/qmp_client.py b/python/qemu/qmp/qmp_client.py
> > > index 5dcda04a75..659fe4d98c 100644
> > > --- a/python/qemu/qmp/qmp_client.py
> > > +++ b/python/qemu/qmp/qmp_client.py
> > > @@ -197,8 +197,8 @@ async def run(self, address='/tmp/qemu.socket'):
> > >  #: Logger object used for debugging messages.
> > >  logger = logging.getLogger(__name__)
> > >
> > > -# Read buffer limit; large enough to accept query-qmp-schema
> > > -_limit = (256 * 1024)
> > > +# Read buffer limit; large enough to accept query-machines
> > > +_limit = (512 * 1024)
> > 
> > wow :)
> 
> Meanwhile over in python/qemu/qmp/protocol.py the read buffer limit is
> set to just 64 kb.
> 
> If the current output of a particular command is known to 450 kb, then
> setting this limit to 512 kb is waaay to conservative, and we'll
> inevitably have to change it again when someone finds the next command
> that overflows.
> 
> Recall this thread
> 
>https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg01060.html
> 
> In fact, let me be the someone who demonstrates a real case where 512kb
> is not enough

Another example...

Create a guest with 255 vCPUs (current RHEL downstream vCPU limit),
and run

  {"execute":"query-stats","arguments":{"target": "vcpu"}}

it'll get back a 0.38 MB  QMP reply.  RHEL raised the limit to 710
vCPUs, giving a little over 1 MB QMP reply. There is a strong desire
to go even higher. With 4096 vCPUs it'd get an ~6 MB QMP reply.

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

Re: [PULL v4 29/83] virtio: introduce virtio_queue_enable()

[Michael S. Tsirkin]

On Wed, Nov 09, 2022 at 09:56:40AM +0100, Laszlo Ersek wrote:
> On 11/09/22 07:59, Michael S. Tsirkin wrote:
> > On Wed, Nov 09, 2022 at 01:52:01AM -0500, Michael S. Tsirkin wrote:
> >> On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> >>> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi  wrote:
> 
>  On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin  wrote:
> >
> > From: Kangjie Xu 
> >
> > Introduce the interface queue_enable() in VirtioDeviceClass and the
> > fucntion virtio_queue_enable() in virtio, it can be called when
> > VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
> > started. It only supports the devices of virtio 1 or later. The
> > not-supported devices can only start the virtqueue when DRIVER_OK.
> >
> > Signed-off-by: Kangjie Xu 
> > Signed-off-by: Xuan Zhuo 
> > Acked-by: Jason Wang 
> > Message-Id: <20221017092558.111082-4-xuanz...@linux.alibaba.com>
> > Reviewed-by: Michael S. Tsirkin 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> >  include/hw/virtio/virtio.h |  2 ++
> >  hw/virtio/virtio.c | 14 ++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 74d76c1dbc..b00b3fcf31 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -149,6 +149,7 @@ struct VirtioDeviceClass {
> >  void (*reset)(VirtIODevice *vdev);
> >  void (*set_status)(VirtIODevice *vdev, uint8_t val);
> >  void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> > +void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
> >  /* For transitional devices, this is a bitmap of features
> >   * that are only exposed on the legacy interface but not
> >   * the modern one.
> > @@ -288,6 +289,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice 
> > *vdev, int n,
> >  int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> >  void virtio_reset(void *opaque);
> >  void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> > +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> >  void virtio_update_irq(VirtIODevice *vdev);
> >  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index cf5f9ca387..9683b2e158 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev, 
> > uint32_t queue_index)
> >  __virtio_queue_reset(vdev, queue_index);
> >  }
> >
> > +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> > +{
> > +VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > +
> > +if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > +error_report("queue_enable is only suppported in devices of 
> > virtio "
> > + "1.0 or later.");
> 
>  Why is this triggering here? Maybe virtio_queue_enable() is called too
>  early. I have verified that the Linux guest driver sets VERSION_1. I
>  didn't check what SeaBIOS does.
> >>>
> >>> Looks like a bug, we should check device features here at least and it
> >>> should be guest errors instead of error_report() here.
> >>>
> >>> Thanks
> >>
> >> But it's weird, queue enable is written before guest features?
> >> How come?
> >
> > It's a bios bug:
> >
> >
> >
> > vp_init_simple(&vdrive->vp, pci);
> > if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
> > dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
> > goto fail;
> > }
> >
> > if (vdrive->vp.use_modern) {
> > struct vp_device *vp = &vdrive->vp;
> > u64 features = vp_get_features(vp);
> > u64 version1 = 1ull << VIRTIO_F_VERSION_1;
> > u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
> > u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
> > if (!(features & version1)) {
> > dprintf(1, "modern device without virtio_1 feature bit: %pP\n", 
> > pci);
> > goto fail;
> > }
> >
> > features = features & (version1 | iommu_platform | blk_size);
> > vp_set_features(vp, features);
> > status |= VIRTIO_CONFIG_S_FEATURES_OK;
> > vp_set_status(vp, status);
> >
> >
> >
> > Not good - does not match the spec. Here's what the spec says:
> >
> >
> > The driver MUST follow this sequence to initialize a device:
> > 1. Reset the device.
> > 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
> > 3. Set the DRIVER status bit: the guest OS knows how to drive the device.
> > 4. Read device feature bits, and write the subset of feature bits 
> > understood by the OS and driver to the
> > device. During this step

[PATCH] virtio: remove the excess virtio features check

[Xuan Zhuo]

In virtio_queue_enable(), we checked virtio feature VIRTIO_F_VERSION_1.

This check is not necessary, and conflict with SeaBIOS. The problem
appeared in SeaBIOS. But we also remove this check.

Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg920538.html
Signed-off-by: Xuan Zhuo 
---
 hw/virtio/virtio.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9683b2e158..701e23ea6a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2499,11 +2499,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t 
queue_index)
 {
 VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
 
-if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
-error_report("queue_enable is only suppported in devices of virtio "
- "1.0 or later.");
-}
-
 if (k->queue_enable) {
 k->queue_enable(vdev, queue_index);
 }
-- 
2.32.0.3.g01195cf9f

Re: [PATCH 2/3] hvf: implement guest debugging on Apple Silicon hosts

[Francesco Cagnin]

> In my version, I added the sw breakpoints to `hvf_vcpu_state` so they would
> follow each CPU, but it's effectively a copy of the global set of breakpoints.

Ah, I missed this field, and it seems the proper one to use. I'll switch
to this.

> Moving it to `hvf_arch_update_guest_debug` would probably be best. Having it 
> in
> `hvf_arch_init_vcpu` would mean that it's always enabled. In some 
> corner-cases,
> that might have an adverse effect.

I agree, I'll move it to 'hvf_arch_update_guest_debug()'.

> I also noticed you are adding 1 to the WRPs and BRPs. As I interpret the
> documentation, you should subtract 1 instead, given the value 0 is reserved:
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index dbc3605f6d..80a583cbd1 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -39,11 +39,11 @@ static void hvf_arm_init_debug(CPUState *cpu)
> {
> ARMCPU *arm_cpu = ARM_CPU(cpu);
>
> - max_hw_bps = 1 + extract64(arm_cpu->isar.id_aa64dfr0, 12, 4);
> + max_hw_bps = extract64(arm_cpu->isar.id_aa64dfr0, 12, 4) - 1;
> hw_breakpoints =
> g_array_sized_new(true, true, sizeof(HWBreakpoint), max_hw_bps);
>
> - max_hw_wps = 1 + extract64(arm_cpu->isar.id_aa64dfr0, 20, 4);
> + max_hw_wps = extract64(arm_cpu->isar.id_aa64dfr0, 20, 4) - 1;
> hw_watchpoints =
> g_array_sized_new(true, true, sizeof(HWWatchpoint), max_hw_wps);
> return;
>
> But the documentation is a bit ambiguous on that. Maybe we can test it?

I tested this again and indeed adding 1 is correct. Thanks Peter for
also pointing out 'arm_num_brps()' and 'arm_num_wrps()', I'll switch to
those.

[PATCH] usbredir: Do not detach usb if backend chardev disconnect

[minglei.liu]

If the network between qemu and usbredirserver is temporarily disconnected,
the USB device in the VM will be unplugged. If the reconnect parameter is
configured for the backend chardev, the device will be reconnected later.
But from the inside of the VM, this USB device has experienced unplug and
re-plug, if the USB storage device has been mounted in the VM before,
the drive letter will change after the device is re-plugged.

So in this case, we no longer unplug the device, and operations to the USB
is returned immediately at this point.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1305
Signed-off-by: minglei.liu 
---
 hw/usb/redirect.c | 37 +++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 1bd30efc3e..73731bcab5 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -379,6 +379,11 @@ static void usbredir_cancel_packet(USBDevice *udev, 
USBPacket *p)
 USBRedirDevice *dev = USB_REDIRECT(udev);
 int i = USBEP2I(p->ep);
 
+if(!dev->parser) {
+ERROR("usbredir parser is not available, need reconnect.\n");
+return;
+}
+
 if (p->combined) {
 usb_combined_packet_cancel(udev, p);
 return;
@@ -519,6 +524,11 @@ static void usbredir_handle_reset(USBDevice *udev)
 {
 USBRedirDevice *dev = USB_REDIRECT(udev);
 
+if(!dev->parser) {
+ERROR("usbredir parser is not available, need reconnect.\n");
+return;
+}
+
 DPRINTF("reset device\n");
 usbredirparser_send_reset(dev->parser);
 usbredirparser_do_write(dev->parser);
@@ -959,6 +969,11 @@ static void usbredir_handle_data(USBDevice *udev, 
USBPacket *p)
 USBRedirDevice *dev = USB_REDIRECT(udev);
 uint8_t ep;
 
+if(!dev->parser) {
+ERROR("usbredir parser is not available, need reconnect.\n");
+return;
+}
+
 ep = p->ep->nr;
 if (p->pid == USB_TOKEN_IN) {
 ep |= USB_DIR_IN;
@@ -1027,6 +1042,11 @@ static void usbredir_ep_stopped(USBDevice *udev, 
USBEndpoint *uep)
 {
 USBRedirDevice *dev = USB_REDIRECT(udev);
 
+if(!dev->parser) {
+ERROR("usbredir parser is not available, need reconnect.\n");
+return;
+}
+
 usbredir_stop_ep(dev, USBEP2I(uep));
 usbredirparser_do_write(dev->parser);
 }
@@ -1098,6 +1118,11 @@ static void usbredir_handle_control(USBDevice *udev, 
USBPacket *p,
 USBRedirDevice *dev = USB_REDIRECT(udev);
 struct usb_redir_control_packet_header control_packet;
 
+if(!dev->parser) {
+ERROR("usbredir parser is not available, need reconnect.\n");
+return;
+}
+
 if (usbredir_already_in_flight(dev, p->id)) {
 p->status = USB_RET_ASYNC;
 return;
@@ -1155,6 +1180,11 @@ static int usbredir_alloc_streams(USBDevice *udev, 
USBEndpoint **eps,
 struct usb_redir_alloc_bulk_streams_header alloc_streams;
 int i;
 
+if(!dev->parser) {
+ERROR("usbredir parser is not available, need reconnect.\n");
+return -1;
+}
+
 if (!usbredirparser_peer_has_cap(dev->parser,
  usb_redir_cap_bulk_streams)) {
 ERROR("peer does not support streams\n");
@@ -1193,6 +1223,11 @@ static void usbredir_free_streams(USBDevice *udev, 
USBEndpoint **eps,
 struct usb_redir_free_bulk_streams_header free_streams;
 int i;
 
+if(!dev->parser) {
+ERROR("usbredir parser is not available, need reconnect.\n");
+return;
+}
+
 if (!usbredirparser_peer_has_cap(dev->parser,
  usb_redir_cap_bulk_streams)) {
 return;
@@ -1219,8 +1254,6 @@ static void usbredir_chardev_close_bh(void *opaque)
 USBRedirDevice *dev = opaque;
 
 qemu_bh_cancel(dev->device_reject_bh);
-usbredir_device_disconnect(dev);
-
 if (dev->parser) {
 DPRINTF("destroying usbredirparser\n");
 usbredirparser_destroy(dev->parser);
-- 
2.27.0

[PATCH] hw/pci-host/pnv_phb: Avoid quitting QEMU if hotplug of pnv-phb-root-port fails

[Thomas Huth]

Currently QEMU terminates if you try to hotplug pnv-phb-root-port in
an environment where it is not supported, e.g. if doing this:

 echo "device_add pnv-phb-root-port" | \
 ./qemu-system-ppc64 -monitor stdio -M powernv9

To avoid this problem, the pnv_phb_root_port_realize() function should
not use error_fatal when trying to set the properties which might not
be available.

Signed-off-by: Thomas Huth 
---
 hw/pci-host/pnv_phb.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index 7b11f1e8dd..0b26b43736 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -241,8 +241,16 @@ static void pnv_phb_root_port_realize(DeviceState *dev, 
Error **errp)
  * QOM id. 'chip_id' is going to be used as PCIE chassis for the
  * root port.
  */
-chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
-index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
+chip_id = object_property_get_int(OBJECT(bus), "chip-id", &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+index = object_property_get_int(OBJECT(bus), "phb-id", &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
 
 /* Set unique chassis/slot values for the root port */
 qdev_prop_set_uint8(dev, "chassis", chip_id);
-- 
2.31.1

Re: [PATCH 01/13] qed: Don't yield in bdrv_qed_co_drain_begin()

[Kevin Wolf]

Am 09.11.2022 um 10:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 11/8/22 15:37, Kevin Wolf wrote:
> >   int ret;
> >   trace_qed_need_check_timer_cb(s);
> > @@ -310,9 +309,20 @@ static void coroutine_fn 
> > qed_need_check_timer_entry(void *opaque)
> >   (void) ret;
> >   }
> > +static void coroutine_fn qed_need_check_timer_entry(void *opaque)
> > +{
> > +BDRVQEDState *s = opaque;
> > +
> > +qed_need_check_timer(opaque);
> > +bdrv_dec_in_flight(s->bs);
> 
> hmm, one question: don't we need aio_wait_kick() call here?

bdrv_dec_in_flight() already calls aio_wait_kick() internally, so any
places that use it don't need a separate aio_wait_kick().

Kevin

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 17:19 schrieb Vladimir Sementsov-Ogievskiy:
> [add Stefan]
> 
> 
> On 11/8/22 18:09, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 08/11/2022 um 15:48 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
 These functions end up calling bdrv_common_block_status_above(), a
 generated_co_wrapper function.
>>>
>>> generated_co_wrapper is not a coroutine_fn. Сonversely it's a function
>>> that do a class coroutine wrapping - start a coroutine and do POLL to
>>> wait for the coroutine to finish.
>>>
 In addition, they also happen to be always called in coroutine context,
 meaning all callers are coroutine_fn.
>>>
>>> That's also not a reason for marking them coroutine_fn. "coroutine_fn"
>>> means that the function can be called only from coroutine context.
>>>
 This means that the g_c_w function will enter the qemu_in_coroutine()
 case and eventually suspend (or in other words call
 qemu_coroutine_yield()).
 Therefore we need to mark such functions coroutine_fn too.
>>>
>>> I don't think so. Moreover, this breaks the concept, as your new
>>> coroutine_fn functions will call generated_co_wrapper functions which
>>> are not marked coroutine_fn and never was.
>>
>> Theoretically not, 
> 
> Agree, I was wrong in this point
> 
>> because marking it coroutine_fn we know that we are
>> going in the if(qemu_in_coroutine()) branch of the g_c_w, so we could
>> directly replace it with the actual function. Because it's a pain to do
>> it with hand, and at some point I/someone should use Alberto static
>> analyzer to get rid of that, I decided to leave g_c_w there.
>>
>> As I understand it, it seems that you and Paolo have a different
>> understanding on what coroutine_fn means and where it should be used.
> 
> Looks so...
> 
> But we have a documentation in coroutine.h, let's check:
> 
>  * Mark a function that executes in coroutine context
>  *
>  * Functions that execute in coroutine context cannot be called directly
> from
>  * normal functions.  In the future it would be nice to enable compiler or
>  * static checker support for catching such errors.  This annotation
> might make
>  * it possible and in the meantime it serves as documentation.
> 
> Not very clear, but still it say:
> 
>  coroutine_fn = "function that executes in coroutine context"
>  "functions that execute in coroutine context"  =  "cannot be called
> directly from normal functions"
> 
> So, IMHO that corresponds to my point of view: we shouldn't mark by
> coroutine_fn functions that can be called from both coroutine context
> and not.

Yes and the point is that these functions are not called by
non-coroutine context.

> 
> If we want to change the concept, we should start with rewriting this
> documentation.
> 
>> Honestly I don't understand your point, as you said
>>
>>> "coroutine_fn"
>>> means that the function can be called only from coroutine context.
>>
>> which is the case for these functions. So could you please clarify?
>>
>> What I do know is that it's extremely confusing to understand if a
>> function that is *not* marked as coroutine_fn is actually being called
>> also from coroutines or not.
>>
>> Which complicates A LOT whatever change or operation I want to perform
>> on the BlockDriver callbacks or any other function in the block layer,
>> because in the current approach for the AioContext lock removal (which
>> you are not aware of, I understand) we need to distinguish between
>> functions running in coroutine context and not, and throwing g_c_w
>> functions everywhere makes my work much harder that it already is.
> 
> OK, I understand the problem.
> 
> Hmm. Formally marking by "coroutine_fn" a function that theoretically
> can be called from any context doesn't break things. We just say that
> since that moment we don't allow to call this function from
> non-coroutine context.
> 
> OK, I tend to agree that you are on the right way, sorry for my skepticism)
> 
> PS: you recently introduced a lot of IO_CODE() / GLOBAL_STATE_CODE()
> marks, which (will be/already) turned into assertions.
> 
> This is a lot better than our "coroutine_fn" sign, which actually do no
> check (and can't do). Don't you plan to swap a "coroutine_fn" noop
> marker with more meaningful IN_COROUTINE(); (or something like this,
> which just do assert(qemu_in_coroutine())) at start of the function? It
> would be a lot safer.

CCing also Alberto and Paolo

So basically I think what we need is something that scans the whole
block layer code and puts the right coroutine_fn annotations (or
assertions, if you want) in the right places.

The rule should be (anyone please correct me if I am wrong):
- if a function is only called by coroutine_fn functions, then it's a
coroutine_fn. Theoretically all these functions should eventually end up
calling qemu_coroutine_yield or similar.

- if it's called only from non-coroutine, then leave it as it is.
Probably asserting is a good idea.

Re: [PATCH 02/13] test-bdrv-drain: Don't yield in .bdrv_co_drained_begin/end()

[Kevin Wolf]

Am 09.11.2022 um 11:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 11/8/22 15:37, Kevin Wolf wrote:
> > We want to change .bdrv_co_drained_begin/end() back to be non-coroutine
> > callbacks, so in preparation, avoid yielding in their implementation.
> > 
> > This does almost the same as the existing logic in bdrv_drain_invoke(),
> > by creating and entering coroutines internally. However, since the test
> > case is by far the heaviest user of coroutine code in drain callbacks,
> > it is preferable to have the complexity in the test case rather than the
> > drain core, which is already complicated enough without this.
> > 
> > The behaviour for bdrv_drain_begin() is unchanged because we increase
> > bs->in_flight and this is still polled. However, bdrv_drain_end()
> > doesn't wait for the spawned coroutine to complete any more. This is
> > fine, we don't rely on bdrv_drain_end() restarting all operations
> > immediately before the next aio_poll().
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >   tests/unit/test-bdrv-drain.c | 64 ++--
> >   1 file changed, 46 insertions(+), 18 deletions(-)
> > 
> > diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
> > index 09dc4a4891..24f34e24ad 100644
> > --- a/tests/unit/test-bdrv-drain.c
> > +++ b/tests/unit/test-bdrv-drain.c
> > @@ -38,12 +38,22 @@ typedef struct BDRVTestState {
> >   bool sleep_in_drain_begin;
> >   } BDRVTestState;
> > +static void coroutine_fn sleep_in_drain_begin(void *opaque)
> > +{
> > +BlockDriverState *bs = opaque;
> > +
> > +qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10);
> > +bdrv_dec_in_flight(bs);
> > +}
> > +
> >   static void coroutine_fn bdrv_test_co_drain_begin(BlockDriverState *bs)
> >   {
> >   BDRVTestState *s = bs->opaque;
> >   s->drain_count++;
> >   if (s->sleep_in_drain_begin) {
> > -qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10);
> > +Coroutine *co = qemu_coroutine_create(sleep_in_drain_begin, bs);
> > +bdrv_inc_in_flight(bs);
> > +aio_co_enter(bdrv_get_aio_context(bs), co);
> >   }
> >   }
> > @@ -1916,6 +1926,21 @@ static int coroutine_fn 
> > bdrv_replace_test_co_preadv(BlockDriverState *bs,
> >   return 0;
> >   }
> > +static void coroutine_fn bdrv_replace_test_drain_co(void *opaque)
> > +{
> > +BlockDriverState *bs = opaque;
> > +BDRVReplaceTestState *s = bs->opaque;
> > +
> > +/* Keep waking io_co up until it is done */
> > +while (s->io_co) {
> > +aio_co_wake(s->io_co);
> > +s->io_co = NULL;
> > +qemu_coroutine_yield();
> > +}
> > +s->drain_co = NULL;
> > +bdrv_dec_in_flight(bs);
> > +}
> 
> Same question, don't we need aio_wait_kick() after decrement in_flight.
> 
> Also, seems we have here extra waiting level: a special coroutine that waits 
> in a loop.
> 
> Could we just do in .drain_begin:
> 
> if (s->io_co) {
>bdrv_inc_in_flight(bs);
> }
> 
> and in .co_preadv instead of waking s->drain_co simply
> 
> if (s->drain_count == 1) {
>   bdrv_dec_in_flight(bs);
>   aio_wait_kick();
> }
> 
> or even better, do inc in_flight when io_co becomes not NULL.

I just did the minimal transformation of the existing code in the test
case.

These test cases often test specific interactions between coroutines, so
I could imagine that the additional yield is not just some inefficient
code, but coroutines that yield multiple times could actually be the
scenario that is supposed to be tested.

I didn't check it for this one, but making test cases more efficient
isn't automatically a good thing if they then end up not testing certain
code paths any more. So if you intend to make a change here, it would
need a careful analysis of all test cases that use the driver.

Kevin

Re: [PATCH 1/2] hw/nvme: fix incorrect use of errp/local_err

[Markus Armbruster]

Klaus Jensen  writes:

> From: Klaus Jensen 
>
> Make nvme_check_constraints() return an int and fix incorrect use of
> errp/local_err.
>
> Signed-off-by: Klaus Jensen 
> ---
>  hw/nvme/ctrl.c | 48 +++-
>  1 file changed, 23 insertions(+), 25 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index ac3885ce5079..4cc6ae753295 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -7035,7 +7035,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
>  },
>  };
>  
> -static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
> +static int nvme_check_params(NvmeCtrl *n, Error **errp)

I prefer bool true on success, false on failure.  I use int only when it
lets me return additional information, such as a non-negative value on
success, or a negative error code on failure.  nvme_init_pci() is an
example of the latter (although its caller doesn't care).

Local consistency with nvme_init_subsys() is desirable.  You could
convert it to bool, along with nvme_init_pci().  Or you keep all three
int.  Up to you.

>  {
>  NvmeParams *params = &n->params;
>  
> @@ -7049,38 +7049,38 @@ static void nvme_check_constraints(NvmeCtrl *n, Error 
> **errp)
>  if (n->namespace.blkconf.blk && n->subsys) {
>  error_setg(errp, "subsystem support is unavailable with legacy "
> "namespace ('drive' property)");
> -return;
> +return -1;
>  }
>  
>  if (params->max_ioqpairs < 1 ||
>  params->max_ioqpairs > NVME_MAX_IOQPAIRS) {
>  error_setg(errp, "max_ioqpairs must be between 1 and %d",
> NVME_MAX_IOQPAIRS);
> -return;
> +return -1;
>  }
>  
>  if (params->msix_qsize < 1 ||
>  params->msix_qsize > PCI_MSIX_FLAGS_QSIZE + 1) {
>  error_setg(errp, "msix_qsize must be between 1 and %d",
> PCI_MSIX_FLAGS_QSIZE + 1);
> -return;
> +return -1;
>  }
>  
>  if (!params->serial) {
>  error_setg(errp, "serial property not set");
> -return;
> +return -1;
>  }
>  
>  if (n->pmr.dev) {
>  if (host_memory_backend_is_mapped(n->pmr.dev)) {
>  error_setg(errp, "can't use already busy memdev: %s",
> 
> object_get_canonical_path_component(OBJECT(n->pmr.dev)));
> -return;
> +return -1;
>  }
>  
>  if (!is_power_of_2(n->pmr.dev->size)) {
>  error_setg(errp, "pmr backend size needs to be power of 2 in 
> size");
> -return;
> +return -1;
>  }
>  
>  host_memory_backend_set_mapped(n->pmr.dev, true);
> @@ -7089,64 +7089,64 @@ static void nvme_check_constraints(NvmeCtrl *n, Error 
> **errp)
>  if (n->params.zasl > n->params.mdts) {
>  error_setg(errp, "zoned.zasl (Zone Append Size Limit) must be less "
> "than or equal to mdts (Maximum Data Transfer Size)");
> -return;
> +return -1;
>  }
>  
>  if (!n->params.vsl) {
>  error_setg(errp, "vsl must be non-zero");
> -return;
> +return -1;
>  }
>  
>  if (params->sriov_max_vfs) {
>  if (!n->subsys) {
>  error_setg(errp, "subsystem is required for the use of SR-IOV");
> -return;
> +return -1;
>  }
>  
>  if (params->sriov_max_vfs > NVME_MAX_VFS) {
>  error_setg(errp, "sriov_max_vfs must be between 0 and %d",
> NVME_MAX_VFS);
> -return;
> +return -1;
>  }
>  
>  if (params->cmb_size_mb) {
>  error_setg(errp, "CMB is not supported with SR-IOV");
> -return;
> +return -1;
>  }
>  
>  if (n->pmr.dev) {
>  error_setg(errp, "PMR is not supported with SR-IOV");
> -return;
> +return -1;
>  }
>  
>  if (!params->sriov_vq_flexible || !params->sriov_vi_flexible) {
>  error_setg(errp, "both sriov_vq_flexible and sriov_vi_flexible"
> " must be set for the use of SR-IOV");
> -return;
> +return -1;
>  }
>  
>  if (params->sriov_vq_flexible < params->sriov_max_vfs * 2) {
>  error_setg(errp, "sriov_vq_flexible must be greater than or 
> equal"
> " to %d (sriov_max_vfs * 2)", params->sriov_max_vfs * 
> 2);
> -return;
> +return -1;
>  }
>  
>  if (params->max_ioqpairs < params->sriov_vq_flexible + 2) {
>  error_setg(errp, "(max_ioqpairs - sriov_vq_flexible) must be"
> " greater than or equal to 2");
> -return;
> +return -1;
>  }
>  
>  if (params->sriov_vi_flexible < params->sriov_max_vfs) {
>  error_setg(errp, "sriov_vi_flexible must be greater than or 
> equal"
>   

Re: [PATCH 2/2] hw/nvme: cleanup error reporting in nvme_init_pci()

[Markus Armbruster]

Klaus Jensen  writes:

> From: Klaus Jensen 
>
> Replace the local Error variable with errp and ERRP_GUARD().
>
> Signed-off-by: Klaus Jensen 
> ---
>  hw/nvme/ctrl.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 4cc6ae753295..38eb5ec54f9d 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -7345,13 +7345,13 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, 
> uint8_t offset)
>  
>  static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
>  {
> +ERRP_GUARD();
> +

Drop the blank line, please.

>  uint8_t *pci_conf = pci_dev->config;
>  uint64_t bar_size;
>  unsigned msix_table_offset, msix_pba_offset;
>  int ret;
>  
> -Error *err = NULL;
> -
>  pci_conf[PCI_INTERRUPT_PIN] = 1;
>  pci_config_set_prog_interface(pci_conf, 0x2);
>  
> @@ -7388,13 +7388,13 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice 
> *pci_dev, Error **errp)
>  }
>  ret = msix_init(pci_dev, n->params.msix_qsize,
>  &n->bar0, 0, msix_table_offset,
> -&n->bar0, 0, msix_pba_offset, 0, &err);
> +&n->bar0, 0, msix_pba_offset, 0, errp);
>  if (ret < 0) {
>  if (ret == -ENOTSUP) {
> -warn_report_err(err);
> +warn_report_err(*errp);
> +*errp = NULL;
>  } else {
> -error_propagate(errp, err);
> -return ret;
> +return -1;
>  }
>  }

Suggest to reduce nesting:

   if (ret == -ENOTSUP) {
   warn_report_err(*errp);
   *errp = NULL;
   } else if (ret < 0) {
   return -1;
   }

Entirely up to you.

Reviewed-by: Markus Armbruster 

Re: [PATCH 1/2] hw/nvme: fix incorrect use of errp/local_err

[Klaus Jensen]

On Nov  9 13:29, Markus Armbruster wrote:
> Klaus Jensen  writes:
> 
> > From: Klaus Jensen 
> >
> > Make nvme_check_constraints() return an int and fix incorrect use of
> > errp/local_err.
> >
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/nvme/ctrl.c | 48 +++-
> >  1 file changed, 23 insertions(+), 25 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index ac3885ce5079..4cc6ae753295 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -7035,7 +7035,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
> >  },
> >  };
> >  
> > -static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
> > +static int nvme_check_params(NvmeCtrl *n, Error **errp)
> 
> I prefer bool true on success, false on failure.  I use int only when it
> lets me return additional information, such as a non-negative value on
> success, or a negative error code on failure.  nvme_init_pci() is an
> example of the latter (although its caller doesn't care).
> 
> Local consistency with nvme_init_subsys() is desirable.  You could
> convert it to bool, along with nvme_init_pci().  Or you keep all three
> int.  Up to you.
> 
> >  {
> >  NvmeParams *params = &n->params;
> >  
> > @@ -7049,38 +7049,38 @@ static void nvme_check_constraints(NvmeCtrl *n, 
> > Error **errp)
> >  if (n->namespace.blkconf.blk && n->subsys) {
> >  error_setg(errp, "subsystem support is unavailable with legacy "
> > "namespace ('drive' property)");
> > -return;
> > +return -1;
> >  }
> >  
> >  if (params->max_ioqpairs < 1 ||
> >  params->max_ioqpairs > NVME_MAX_IOQPAIRS) {
> >  error_setg(errp, "max_ioqpairs must be between 1 and %d",
> > NVME_MAX_IOQPAIRS);
> > -return;
> > +return -1;
> >  }
> >  
> >  if (params->msix_qsize < 1 ||
> >  params->msix_qsize > PCI_MSIX_FLAGS_QSIZE + 1) {
> >  error_setg(errp, "msix_qsize must be between 1 and %d",
> > PCI_MSIX_FLAGS_QSIZE + 1);
> > -return;
> > +return -1;
> >  }
> >  
> >  if (!params->serial) {
> >  error_setg(errp, "serial property not set");
> > -return;
> > +return -1;
> >  }
> >  
> >  if (n->pmr.dev) {
> >  if (host_memory_backend_is_mapped(n->pmr.dev)) {
> >  error_setg(errp, "can't use already busy memdev: %s",
> > 
> > object_get_canonical_path_component(OBJECT(n->pmr.dev)));
> > -return;
> > +return -1;
> >  }
> >  
> >  if (!is_power_of_2(n->pmr.dev->size)) {
> >  error_setg(errp, "pmr backend size needs to be power of 2 in 
> > size");
> > -return;
> > +return -1;
> >  }
> >  
> >  host_memory_backend_set_mapped(n->pmr.dev, true);
> > @@ -7089,64 +7089,64 @@ static void nvme_check_constraints(NvmeCtrl *n, 
> > Error **errp)
> >  if (n->params.zasl > n->params.mdts) {
> >  error_setg(errp, "zoned.zasl (Zone Append Size Limit) must be less 
> > "
> > "than or equal to mdts (Maximum Data Transfer Size)");
> > -return;
> > +return -1;
> >  }
> >  
> >  if (!n->params.vsl) {
> >  error_setg(errp, "vsl must be non-zero");
> > -return;
> > +return -1;
> >  }
> >  
> >  if (params->sriov_max_vfs) {
> >  if (!n->subsys) {
> >  error_setg(errp, "subsystem is required for the use of 
> > SR-IOV");
> > -return;
> > +return -1;
> >  }
> >  
> >  if (params->sriov_max_vfs > NVME_MAX_VFS) {
> >  error_setg(errp, "sriov_max_vfs must be between 0 and %d",
> > NVME_MAX_VFS);
> > -return;
> > +return -1;
> >  }
> >  
> >  if (params->cmb_size_mb) {
> >  error_setg(errp, "CMB is not supported with SR-IOV");
> > -return;
> > +return -1;
> >  }
> >  
> >  if (n->pmr.dev) {
> >  error_setg(errp, "PMR is not supported with SR-IOV");
> > -return;
> > +return -1;
> >  }
> >  
> >  if (!params->sriov_vq_flexible || !params->sriov_vi_flexible) {
> >  error_setg(errp, "both sriov_vq_flexible and sriov_vi_flexible"
> > " must be set for the use of SR-IOV");
> > -return;
> > +return -1;
> >  }
> >  
> >  if (params->sriov_vq_flexible < params->sriov_max_vfs * 2) {
> >  error_setg(errp, "sriov_vq_flexible must be greater than or 
> > equal"
> > " to %d (sriov_max_vfs * 2)", params->sriov_max_vfs 
> > * 2);
> > -return;
> > +return -1;
> >  }
> >  
> >  if (params->max_ioqpairs < params->sriov_vq_flexible + 2) {
> >  error_setg(errp, "(

Re: [PATCH 2/2] hw/nvme: cleanup error reporting in nvme_init_pci()

[Klaus Jensen]

On Nov  9 13:33, Markus Armbruster wrote:
> Klaus Jensen  writes:
> 
> > From: Klaus Jensen 
> >
> > Replace the local Error variable with errp and ERRP_GUARD().
> >
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/nvme/ctrl.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 4cc6ae753295..38eb5ec54f9d 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -7345,13 +7345,13 @@ static int nvme_add_pm_capability(PCIDevice 
> > *pci_dev, uint8_t offset)
> >  
> >  static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
> >  {
> > +ERRP_GUARD();
> > +
> 
> Drop the blank line, please.
> 

Roger.

> >  uint8_t *pci_conf = pci_dev->config;
> >  uint64_t bar_size;
> >  unsigned msix_table_offset, msix_pba_offset;
> >  int ret;
> >  
> > -Error *err = NULL;
> > -
> >  pci_conf[PCI_INTERRUPT_PIN] = 1;
> >  pci_config_set_prog_interface(pci_conf, 0x2);
> >  
> > @@ -7388,13 +7388,13 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice 
> > *pci_dev, Error **errp)
> >  }
> >  ret = msix_init(pci_dev, n->params.msix_qsize,
> >  &n->bar0, 0, msix_table_offset,
> > -&n->bar0, 0, msix_pba_offset, 0, &err);
> > +&n->bar0, 0, msix_pba_offset, 0, errp);
> >  if (ret < 0) {
> >  if (ret == -ENOTSUP) {
> > -warn_report_err(err);
> > +warn_report_err(*errp);
> > +*errp = NULL;
> >  } else {
> > -error_propagate(errp, err);
> > -return ret;
> > +return -1;
> >  }
> >  }
> 
> Suggest to reduce nesting:
> 
>if (ret == -ENOTSUP) {
>warn_report_err(*errp);
>*errp = NULL;
>} else if (ret < 0) {
>return -1;
>}
> 
> Entirely up to you.
> 
> Reviewed-by: Markus Armbruster 
> 

Makes sense, thanks!


signature.asc
Description: PGP signature

Re: [PATCH 1/2] hw/nvme: fix incorrect use of errp/local_err

[Markus Armbruster]

Klaus Jensen  writes:

> From: Klaus Jensen 
>
> Make nvme_check_constraints() return an int and fix incorrect use of
> errp/local_err.
>
> Signed-off-by: Klaus Jensen 

One more question: what exactly do you mean by "incorrect use of
errp/local_err"?  Is it incorrect in the sense of "behaves badly", or
merely in the sense of "doesn't use the Error API the way it wants to be
used"?

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Claudio Fontana]

On 11/9/22 09:04, Gerd Hoffmann wrote:
> On Tue, Nov 08, 2022 at 05:23:24PM +0100, Claudio Fontana wrote:
>> The GTK Clipboard implementation may cause guest hangs.
>>
>> Therefore implement a new configure switch --enable-gtk-clipboard,
>> disabled by default, as a meson option.
> 
> Hmm, I was thinking about a runtime option, add 'clipboard' bool to
> DisplayGTK in qapi/ui.json) and just skip the call to
> gd_clipboard_init() unless the option is explicitly enabled ...
> 
> I don't feel like vetoing a compile time option though, so in case you
> prefer to stick with this:
> 
> Acked-by: Gerd Hoffmann 
> 
> take care,
>   Gerd
> 
> 

Thanks Gerd,

I think at least for our packaging purposes we'd rather have it as a configure 
time option,
so as to not put functionality in the hands of our users that can potentially 
lock the guest.

Is someone going to queue this, where?

Thanks,

Claudio

Re: [PATCH 1/2] hw/nvme: fix incorrect use of errp/local_err

[Klaus Jensen]

On Nov  9 13:36, Markus Armbruster wrote:
> Klaus Jensen  writes:
> 
> > From: Klaus Jensen 
> >
> > Make nvme_check_constraints() return an int and fix incorrect use of
> > errp/local_err.
> >
> > Signed-off-by: Klaus Jensen 
> 
> One more question: what exactly do you mean by "incorrect use of
> errp/local_err"?  Is it incorrect in the sense of "behaves badly", or
> merely in the sense of "doesn't use the Error API the way it wants to be
> used"?
> 

It's the last hunk of the patch:

@@ -7586,7 +7585,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
   &pci_dev->qdev, n->parent_obj.qdev.id);

 if (nvme_init_subsys(n, errp)) {
-error_propagate(errp, local_err);
 return;
 }

It propagates local_err (and it's NULL here).

And the bug is a consequence of the error-prone use of an unneeded local
error value.


signature.asc
Description: PGP signature

Re: [PATCH 1/3] arm: move KVM breakpoints helpers

[Francesco Cagnin]

> I was planning to move hypervisor-specific code to target/arm/$hypervisor/, 
> but here Francesco wants to re-use the same code
> between 2 hypervisors... Maybe move it to target/arm/hyp_gdbstub.c
> and let meson add it conditionally?

Something like this?

-arm_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c', 'kvm64.c'),
if_false: files('kvm-stub.c'))
+arm_ss.add(when: 'CONFIG_KVM', if_true: files('hyp_gdbstub.c',
'kvm.c', 'kvm64.c'), if_false: files('kvm-stub.c'))
+arm_ss.add(when: 'CONFIG_HVF', if_true: files('hyp_gdbstub.c'))

Re: [PATCH 1/2] hw/nvme: fix incorrect use of errp/local_err

[Markus Armbruster]

Klaus Jensen  writes:

> On Nov  9 13:36, Markus Armbruster wrote:
>> Klaus Jensen  writes:
>> 
>> > From: Klaus Jensen 
>> >
>> > Make nvme_check_constraints() return an int and fix incorrect use of
>> > errp/local_err.
>> >
>> > Signed-off-by: Klaus Jensen 
>> 
>> One more question: what exactly do you mean by "incorrect use of
>> errp/local_err"?  Is it incorrect in the sense of "behaves badly", or
>> merely in the sense of "doesn't use the Error API the way it wants to be
>> used"?
>> 
>
> It's the last hunk of the patch:
>
> @@ -7586,7 +7585,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> **errp)
>&pci_dev->qdev, n->parent_obj.qdev.id);
>
>  if (nvme_init_subsys(n, errp)) {
> -error_propagate(errp, local_err);
>  return;
>  }
>
> It propagates local_err (and it's NULL here).

Now I see, thanks!

Harmless, because error_propagate(errp, NULL) does nothing.  Worth
cleaning up all the same.

> And the bug is a consequence of the error-prone use of an unneeded local
> error value.

Yes.  Eliminating unnecessary error_propagate() tends to result in more
concise and clearer code.

[PATCH v3] tests/qtest: netdev: test stream and dgram backends

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
Acked-by: Michael S. Tsirkin 
---

Notes:
v3:
- Add "-M none" to avoid error:
  "No machine specified, and there is no default"

v2:
- Fix ipv6 free port allocation
- Check for IPv4, IPv6, AF_UNIX
- Use g_mkdtemp() rather than g_file_open_tmp()
- Use socketpair() in test_stream_fd()

v1: compared to v14 of "qapi: net: add unix socket type support to netdev 
backend":
- use IP addresses 127.0.0.1 and ::1 rather than localhost

 tests/qtest/meson.build |   2 +
 tests/qtest/netdev-socket.c | 435 
 2 files changed, 437 insertions(+)
 create mode 100644 tests/qtest/netdev-socket.c

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c07a5b1a5f43..43d075b76280 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -27,6 +27,7 @@ qtests_generic = [
   'test-hmp',
   'qos-test',
   'readconfig-test',
+  'netdev-socket',
 ]
 if config_host.has_key('CONFIG_MODULES')
   qtests_generic += [ 'modules-test' ]
@@ -304,6 +305,7 @@ qtests = {
   'tpm-tis-device-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'],
   'tpm-tis-device-test': [io, tpmemu_files, 'tpm-tis-util.c'],
   'vmgenid-test': files('boot-sector.c', 'acpi-utils.c'),
+  'netdev-socket': files('netdev-socket.c', '../unit/socket-helpers.c'),
 }
 
 gvnc = dependency('gvnc-1.0', required: false)
diff --git a/tests/qtest/netdev-socket.c b/tests/qtest/netdev-socket.c
new file mode 100644
index ..b6b59244a282
--- /dev/null
+++ b/tests/qtest/netdev-socket.c
@@ -0,0 +1,435 @@
+/*
+ * QTest testcase for netdev stream and dgram
+ *
+ * Copyright (c) 2022 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include 
+#include "../unit/socket-helpers.h"
+#include "libqtest.h"
+
+#define CONNECTION_TIMEOUT5
+
+#define EXPECT_STATE(q, e, t) \
+do {  \
+char *resp = qtest_hmp(q, "info network");\
+if (t) {  \
+strrchr(resp, t)[0] = 0;  \
+} \
+g_test_timer_start(); \
+while (g_test_timer_elapsed() < CONNECTION_TIMEOUT) { \
+if (strcmp(resp, e) == 0) {   \
+break;\
+} \
+g_free(resp); \
+resp = qtest_hmp(q, "info network");  \
+if (t) {  \
+strrchr(resp, t)[0] = 0;  \
+} \
+} \
+g_assert_cmpstr(resp, ==, e); \
+g_free(resp); \
+} while (0)
+
+static char *tmpdir;
+
+static int inet_get_free_port_socket_ipv4(int sock)
+{
+struct sockaddr_in addr;
+socklen_t len;
+
+memset(&addr, 0, sizeof(addr));
+addr.sin_family = AF_INET;
+addr.sin_addr.s_addr = INADDR_ANY;
+addr.sin_port = 0;
+if (bind(sock, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
+return -1;
+}
+
+len = sizeof(addr);
+if (getsockname(sock,  (struct sockaddr *)&addr, &len) < 0) {
+return -1;
+}
+
+return ntohs(addr.sin_port);
+}
+
+static int inet_get_free_port_socket_ipv6(int sock)
+{
+struct sockaddr_in6 addr;
+socklen_t len;
+
+memset(&addr, 0, sizeof(addr));
+addr.sin6_family = AF_INET6;
+addr.sin6_addr = in6addr_any;
+addr.sin6_port = 0;
+if (bind(sock, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
+return -1;
+}
+
+len = sizeof(addr);
+if (getsockname(sock,  (struct sockaddr *)&addr, &len) < 0) {
+return -1;
+}
+
+return ntohs(addr.sin6_port);
+}
+
+static int inet_get_free_port_multiple(int nb, int *port, bool ipv6)
+{
+int sock[nb];
+int i;
+
+for (i = 0; i < nb; i++) {
+sock[i] = socket(ipv6 ? AF_INET6 : AF_INET, SOCK_STREAM, 0);
+if (sock[i] < 0) {
+break;
+}
+port[i] = ipv6 ? inet_get_free_port_socket_ipv6(sock[i]) :
+ inet_get_free_port_socket_ipv4(sock[i]);
+if (port[i] == -1) {
+break;
+}
+}
+
+nb = i;
+for (i = 0; i < nb; i++) {
+closesocket(sock[i]);
+}
+
+return nb;
+}
+
+static int inet_get_free_port(bool ipv6)
+{
+int nb, port;
+
+nb = inet_get_free_port_multiple(1, &port, ipv6);
+g_assert_cmpint(nb, ==, 1);
+
+return port;
+}
+
+static void test_stream_inet_ipv4(void)
+{
+QTestState *qts0, *qts1;
+char *expect;
+int port;
+
+port = inet_get_free_port(false);
+qts0 = qtest

Re: [PATCH] hw/pci-host/pnv_phb: Avoid quitting QEMU if hotplug of pnv-phb-root-port fails

[Cédric Le Goater]

On 11/9/22 13:22, Thomas Huth wrote:

Currently QEMU terminates if you try to hotplug pnv-phb-root-port in
an environment where it is not supported, e.g. if doing this:

  echo "device_add pnv-phb-root-port" | \
  ./qemu-system-ppc64 -monitor stdio -M powernv9

To avoid this problem, the pnv_phb_root_port_realize() function should
not use error_fatal when trying to set the properties which might not
be available.


Fixes: c2f3f78af5 ("ppc/pnv: set root port chassis and slot using Bus 
properties")

Reviewed-by: Cédric Le Goater 


Signed-off-by: Thomas Huth 


Thanks,

C.



---
  hw/pci-host/pnv_phb.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index 7b11f1e8dd..0b26b43736 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -241,8 +241,16 @@ static void pnv_phb_root_port_realize(DeviceState *dev, 
Error **errp)
   * QOM id. 'chip_id' is going to be used as PCIE chassis for the
   * root port.
   */
-chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
-index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
+chip_id = object_property_get_int(OBJECT(bus), "chip-id", &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+index = object_property_get_int(OBJECT(bus), "phb-id", &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
  
  /* Set unique chassis/slot values for the root port */

  qdev_prop_set_uint8(dev, "chassis", chip_id);

Re: [PATCH v1 1/1] migration: Fix yank on postcopy multifd crashing guest after migration

[Dr. David Alan Gilbert]

* Leonardo Bras (leob...@redhat.com) wrote:
> When multifd and postcopy-ram capabilities are enabled, if a
> migrate-start-postcopy is attempted, the migration will finish sending the
> memory pages and then crash with the following error:

How does that happen? Isn't multifd+postcopy still disabled, I see in 
migrate_caps_check

if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {

if (cap_list[MIGRATION_CAPABILITY_MULTIFD]) {
error_setg(errp, "Postcopy is not yet compatible with multifd");
return false;
}
}


Dave

> qemu-system-x86_64: ../util/yank.c:107: yank_unregister_instance: Assertion
> `QLIST_EMPTY(&entry->yankfns)' failed.
> 
> This happens because even though all multifd channels could
> yank_register_function(), none of them could unregister it before
> unregistering the MIGRATION_YANK_INSTANCE, causing the assert to fail.
> 
> Fix that by calling multifd_load_cleanup() on postcopy_ram_listen_thread()
> before MIGRATION_YANK_INSTANCE is unregistered.
> 
> Fixes: b5eea99ec2 ("migration: Add yank feature")
> Reported-by: Li Xiaohui 
> Signed-off-by: Leonardo Bras 
> ---
>  migration/migration.h |  1 +
>  migration/migration.c | 18 +-
>  migration/savevm.c|  2 ++
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index cdad8aceaa..240f64efb0 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -473,6 +473,7 @@ void migration_make_urgent_request(void);
>  void migration_consume_urgent_request(void);
>  bool migration_rate_limit(void);
>  void migration_cancel(const Error *error);
> +bool migration_load_cleanup(void);
>  
>  void populate_vfio_info(MigrationInfo *info);
>  void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
> diff --git a/migration/migration.c b/migration/migration.c
> index 739bb683f3..4f363b2a95 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -486,6 +486,17 @@ void migrate_add_address(SocketAddress *address)
>QAPI_CLONE(SocketAddress, address));
>  }
>  
> +bool migration_load_cleanup(void)
> +{
> +Error *local_err = NULL;
> +
> +if (multifd_load_cleanup(&local_err)) {
> +error_report_err(local_err);
> +return true;
> +}
> +return false;
> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>  const char *p = NULL;
> @@ -540,8 +551,7 @@ static void process_incoming_migration_bh(void *opaque)
>   */
>  qemu_announce_self(&mis->announce_timer, migrate_announce_params());
>  
> -if (multifd_load_cleanup(&local_err) != 0) {
> -error_report_err(local_err);
> +if (migration_load_cleanup()) {
>  autostart = false;
>  }
>  /* If global state section was not received or we are in running
> @@ -646,9 +656,7 @@ fail:
>  migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
>MIGRATION_STATUS_FAILED);
>  qemu_fclose(mis->from_src_file);
> -if (multifd_load_cleanup(&local_err) != 0) {
> -error_report_err(local_err);
> -}
> +migration_load_cleanup();
>  exit(EXIT_FAILURE);
>  }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a0cdb714f7..250caff7f4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1889,6 +1889,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
>  exit(EXIT_FAILURE);
>  }
>  
> +migration_load_cleanup();
> +
>  migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> MIGRATION_STATUS_COMPLETED);
>  /*
> -- 
> 2.38.1
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[RFC PATCH 0/1] hw/arm: use -cpu max by default on sbsa-ref

[Leif Lindholm]

From: Leif Lindholm 

We have mainly (well, as will become clear, in fact "exclusively") been using
sbsa-ref with the "max" CPU. But sbsa-ref was created with a default CPU of
Cortex-A57, which we have not updated along the way.

However, the "max" cpu has seen a bug where Linux boot fails around UEFI
ExitBootServices. Marcin Juszkiewicz has found the cause for that, but that
requires a patch to TF-A. (Has that been submitted upstream?)

Turns out that due to a change in upstream TF-A last year, all supported cpus
other than "max" fail to even boot UEFI fully, due to the top-level (TF-A)
Makefile defaulting to enabling the maximum ARM architectural version
(currently 8.6), in combination with not verifying all features at runtime
using the ID registers.

Since the *point* of sbsa-ref is to serve as a continuously evolving platform
tracking (with some obvious lag) the evolution of the ARM architecture and the
SystemReady specifications, I don't really want to restrict the enabled
feature set in TF-A to the Cortex-A57 one.

My preferred course of action would be to change the default cpu to max -
maybe even dropping support for other cpus. I would then step the version
field that was added to the DT. *But* this would break existing boots with
old TF-A that can currently boot Linux.

I did contemplate weaving this into the platform versioning, but truth is
I'm not convinced that would help ... and it would delay getting the
reworked memory map out.

Cc: Peter Maydell 
Cc: Marcin Juszkiewicz 
Cc: Shashi Mallela 
Cc: Radoslaw Biernacki 

Leif Lindholm (1):
  hw/arm: use -cpu max by default for sbsa-ref

 hw/arm/sbsa-ref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.30.2

[RFC PATCH 1/1] hw/arm: use -cpu max by default for sbsa-ref

[Leif Lindholm]

From: Leif Lindholm 

Due to a change in upstream TF-A, boot fails (in edk2) on all cpus that
don't implement FEAT_DIT. The only currently emulated cpu that "supports"
this feature is "max". So switch to using that cpu by default for
sbsa-ref.

However, it is worth noting that the "max" cpu triggers another bug in
TF-A where Linux boot fails around UEFI ExitBootServices.

Signed-off-by: Leif Lindholm 
Cc: Peter Maydell 
Cc: Marcin Juszkiewicz 
Cc: Shashi Mallela 
Cc: Radoslaw Biernacki 
---
 hw/arm/sbsa-ref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 4bb444684f..8bb5c0fc70 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -852,7 +852,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
 
 mc->init = sbsa_ref_init;
 mc->desc = "QEMU 'SBSA Reference' ARM Virtual Machine";
-mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a57");
+mc->default_cpu_type = ARM_CPU_TYPE_NAME("max");
 mc->max_cpus = 512;
 mc->pci_allow_0_address = true;
 mc->minimum_page_bits = 12;
-- 
2.30.2

Re: [PATCH] hw/pci-host/pnv_phb: Avoid quitting QEMU if hotplug of pnv-phb-root-port fails

[Daniel Henrique Barboza]

On 11/9/22 09:22, Thomas Huth wrote:

Currently QEMU terminates if you try to hotplug pnv-phb-root-port in
an environment where it is not supported, e.g. if doing this:

  echo "device_add pnv-phb-root-port" | \
  ./qemu-system-ppc64 -monitor stdio -M powernv9

To avoid this problem, the pnv_phb_root_port_realize() function should
not use error_fatal when trying to set the properties which might not
be available.

Signed-off-by: Thomas Huth 
---


Ops, my bad. Thanks for fixing it up.

Reviewed-by: Daniel Henrique Barboza 


And queued in gitlab.com/danielhb/qemu/tree/ppc-next after adding the
"Fixes" tag Cedric mentioned. Thanks,


Daniel


  hw/pci-host/pnv_phb.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index 7b11f1e8dd..0b26b43736 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -241,8 +241,16 @@ static void pnv_phb_root_port_realize(DeviceState *dev, 
Error **errp)
   * QOM id. 'chip_id' is going to be used as PCIE chassis for the
   * root port.
   */
-chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
-index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
+chip_id = object_property_get_int(OBJECT(bus), "chip-id", &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+index = object_property_get_int(OBJECT(bus), "phb-id", &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
  
  /* Set unique chassis/slot values for the root port */

  qdev_prop_set_uint8(dev, "chassis", chip_id);

How about increasing max_cpus for q35 ?

[Dario Faggioli]

Hello,

Sorry for the potentially naive question, but I'm not clear what the
process would be if, say, I'd like to raise the number of maximum CPUs
a q35 VM can have.

So, right now we have:

void pc_q35_2_7_machine_options(MachineClass *m) {
  ...
  m->max_cpus = 255;
}

And:

void pc_q35_machine_options(MachineClass *m)
{
  ...
  m->max_cpus = 288;
}

Focusing on the latter, it comes from this commit:

https://gitlab.com/qemu-project/qemu/-/commit/00d0f9fd6602a27b204f672ef5bc8e69736c7ff1
  
  pc: q35: Bump max_cpus to 288

  Along with it for machine versions 2.7 and older keep
  it at 255.

So, it was 255 and is now 288. This seems to me to be there since QEMU
2.8.0.

Now, as far as I understand, KVM can handle 1024, at least since this
commit (and a couple of other related ones):

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=074c82c8f7cf8a46c3b81965f122599e3a133450
"kvm: x86: Increase MAX_VCPUS to 1024"

Which basically does:

-#define KVM_MAX_VCPUS 288
+#define KVM_MAX_VCPUS 1024

And it's included in kernels >= 5.15.

So, what's the correct way of bumping up the limit again? Just changing
that assignment in pc_q35_machine_options() ? Or do we want a new
version of the machine type or something like that?

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
---
<> (Raistlin Majere)


signature.asc
Description: This is a digitally signed message part

Re: [PATCH 02/13] test-bdrv-drain: Don't yield in .bdrv_co_drained_begin/end()

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

We want to change .bdrv_co_drained_begin/end() back to be non-coroutine
callbacks, so in preparation, avoid yielding in their implementation.

This does almost the same as the existing logic in bdrv_drain_invoke(),
by creating and entering coroutines internally. However, since the test
case is by far the heaviest user of coroutine code in drain callbacks,
it is preferable to have the complexity in the test case rather than the
drain core, which is already complicated enough without this.

The behaviour for bdrv_drain_begin() is unchanged because we increase
bs->in_flight and this is still polled. However, bdrv_drain_end()
doesn't wait for the spawned coroutine to complete any more. This is
fine, we don't rely on bdrv_drain_end() restarting all operations
immediately before the next aio_poll().

Signed-off-by: Kevin Wolf


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [RFC PATCH 0/1] hw/arm: use -cpu max by default on sbsa-ref

[Peter Maydell]

On Wed, 9 Nov 2022 at 13:35, Leif Lindholm  wrote:
>
> From: Leif Lindholm 
>
> We have mainly (well, as will become clear, in fact "exclusively") been using
> sbsa-ref with the "max" CPU. But sbsa-ref was created with a default CPU of
> Cortex-A57, which we have not updated along the way.

So, I don't have a strong opinion on what sbsa-ref's CPU type
should be: if 'max' works better for the use case we should
change to it.

> However, the "max" cpu has seen a bug where Linux boot fails around UEFI
> ExitBootServices. Marcin Juszkiewicz has found the cause for that, but that
> requires a patch to TF-A. (Has that been submitted upstream?)
>
> Turns out that due to a change in upstream TF-A last year, all supported cpus
> other than "max" fail to even boot UEFI fully, due to the top-level (TF-A)
> Makefile defaulting to enabling the maximum ARM architectural version
> (currently 8.6), in combination with not verifying all features at runtime
> using the ID registers.

This seems to me straightforwardly to be a problem that should be
fixed in TF-A. "Default to the newest possible architecture version
and don't check ID registers" is a recipe for "can't boot on
anything". Many *hardware* CPUs aren't that new yet!

Part of the purpose of sbsa-ref is to find problems with the
software stack -- so we should expect that sometimes the answer
is "fix the software stack", not "change the model's behaviour".

> Since the *point* of sbsa-ref is to serve as a continuously evolving platform
> tracking (with some obvious lag) the evolution of the ARM architecture and the
> SystemReady specifications, I don't really want to restrict the enabled
> feature set in TF-A to the Cortex-A57 one.
>
> My preferred course of action would be to change the default cpu to max -
> maybe even dropping support for other cpus. I would then step the version
> field that was added to the DT. *But* this would break existing boots with
> old TF-A that can currently boot Linux.

An intermediate option would be to move forward to for instance
the neoverse-n1 CPU type.

If you want to use 'max' for sbsa-ref then you probably also want to
look at making sure that TF-A is doing the same thing Linux does
where it checks ID registers for feature presence before enabling
feature use, because 'max' is a moving target and relies on the
guest code doing the right thing with feature checks. In particular
it does not honour the nominal architectural requirements about
v8.x/v9.x levels, but may implement features from further in the
"future" than is strictly permitted given the absence of some
other older features.

thanks
-- PMM

Re: [PATCH 03/13] block: Revert .bdrv_drained_begin/end to non-coroutine_fn

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

Polling during bdrv_drained_end() can be problematic (and in the future,
we may get cases for bdrv_drained_begin() where polling is forbidden,
and we don't care about already in-flight requests, but just want to
prevent new requests from arriving).

The .bdrv_drained_begin/end callbacks running in a coroutine is the only
reason why we have to do this polling, so make them non-coroutine
callbacks again. None of the callers actually yield any more.

This means that bdrv_drained_end() effectively doesn't poll any more,
even if AIO_WAIT_WHILE() loops are still there (their condition is false
from the beginning). This is generally not a problem, but in
test-bdrv-drain, some additional explicit aio_poll() calls need to be
added because the test case wants to verify the final state after BHs
have executed.


So, drained_end_counter is always zero since this commit (and is removed in the 
next one).



Signed-off-by: Kevin Wolf 
---
  include/block/block_int-common.h | 10 ---
  block.c  |  4 +--
  block/io.c   | 49 +---
  block/qed.c  |  4 +--
  block/throttle.c |  6 ++--
  tests/unit/test-bdrv-drain.c | 18 ++--
  6 files changed, 30 insertions(+), 61 deletions(-)



[..]


--- a/block/qed.c
+++ b/block/qed.c
@@ -365,7 +365,7 @@ static void bdrv_qed_attach_aio_context(BlockDriverState 
*bs,
  }
  }
  
-static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)

+static void bdrv_qed_co_drain_begin(BlockDriverState *bs)
  {
  BDRVQEDState *s = bs->opaque;
  
@@ -1661,7 +1661,7 @@ static BlockDriver bdrv_qed = {

  .bdrv_co_check= bdrv_qed_co_check,
  .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
  .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
-.bdrv_co_drain_begin  = bdrv_qed_co_drain_begin,
+.bdrv_drain_begin = bdrv_qed_co_drain_begin,


Rename to bdrv_qed_drain_begin without _co_, as for tests ?



  };
  
  static void bdrv_qed_init(void)

diff --git a/block/throttle.c b/block/throttle.c
index 131eba3ab4..6e3ae1b355 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -214,7 +214,7 @@ static void throttle_reopen_abort(BDRVReopenState 
*reopen_state)
  reopen_state->opaque = NULL;
  }
  
-static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)

+static void throttle_co_drain_begin(BlockDriverState *bs)


and here.

And you didn't drop coroutine_fn for throttle_co_drain_end

with that fixed:

Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()

[Siqi Chen]

Hi,

>This reproducer does not crash my QEMU. Am I missing anything?
I submitted the reproducer. Because the overflow is only one byte, it may
not be detected by the host's heap allocator.  Do you compile your qemu
with sanitizer?  This is my build configuration: "./configure
--target-list=x86_64-softmmu --enable-sanitizers"

Thanks,
Siqi Chen.



Bin Meng  于2022年11月9日周三 17:30写道：

> Hi,
>
> On Mon, Nov 7, 2022 at 7:08 PM Mauro Matteo Cascella
>  wrote:
> >
> > On Mon, Nov 7, 2022 at 11:35 AM Mauro Matteo Cascella
> >  wrote:
> > >
> > > Make sure to reset data_count if it's equal to (or exceeds) block_size.
> > > This prevents an off-by-one read / write when accessing s->fifo_buffer
> > > in sdhci_read_dataport / sdhci_write_dataport, both called right after
> > > sdhci_buff_access_is_sequential.
> > >
> > > Fixes: CVE-2022-3872
> > > Reported-by: RivenDell 
> > > Reported-by: Siqi Chen 
> > > Reported-by: ningqiang 
> > > Signed-off-by: Mauro Matteo Cascella 
> > > ---
> > >  hw/sd/sdhci.c | 4 
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > > index 306070c872..aa2fd79df2 100644
> > > --- a/hw/sd/sdhci.c
> > > +++ b/hw/sd/sdhci.c
> > > @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
> > >  static inline bool
> > >  sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
> > >  {
> > > +if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
> > > +s->data_count = 0;
> > > +}
> > > +
> > >  if ((s->data_count & 0x3) != byte_num) {
> > >  trace_sdhci_error("Non-sequential access to Buffer Data Port
> register"
> > >"is prohibited\n");
> > > --
> > > 2.38.1
> > >
> >
> > Reproducer:
> >
> > cat << EOF | ./qemu-system-x86_64 -machine accel=qtest \
> > -nodefaults -drive if=none,index=0,file=null-co://,format=raw,id=mydrive
> \
> > -device sdhci-pci -device sd-card,drive=mydrive -nographic -qtest stdio
> > outl 0xcf8 0x80001004
> > outl 0xcfc 0x107
> > outl 0xcf8 0x80001010
> > outl 0xcfc 0xfebf1000
> > writel 0xfebf102c 0x7
> > writel 0xfebf1004 0x10200
> > writel 0xfebf100c 0x20
> > writel 0xfebf1028 0x1
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > w

Re: [PATCH 04/13] block: Remove drained_end_counter

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

drained_end_counter is unused now, nobody changes its value any more. It
can be removed.

In cases where we had two almost identical functions that only differed
in whether the caller passes drained_end_counter, or whether they would
poll for a local drained_end_counter to reach 0, these become a single
function.

Signed-off-by: Kevin Wolf 


Reviewed-by: Vladimir Sementsov-Ogievskiy 

[..]

  
  /* Recursively call BlockDriver.bdrv_drain_begin/end callbacks */


Not about this patch, but what is recursive in bdrv_drain_invoke() ?


-static void bdrv_drain_invoke(BlockDriverState *bs, bool begin,
-  int *drained_end_counter)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
  {
  if (!bs->drv || (begin && !bs->drv->bdrv_drain_begin) ||
  (!begin && !bs->drv->bdrv_drain_end)) {


[..]

  
  /**

   * This function does not poll, nor must any of its recursively called
- * functions.  The *drained_end_counter pointee will be incremented
- * once 


Seems that is wrong already after previous commit.. Not critical


for every background operation scheduled, and decremented once
- * the operation settles.  Therefore, the pointer must remain valid
- * until the pointee reaches 0.  That implies that whoever sets up the
- * pointee has to poll until it is 0.
- *
- * We use atomic operations to access *drained_end_counter, because
- * (1) when called from bdrv_set_aio_context_ignore(), the subgraph of
- * @bs may contain nodes in different AioContexts,
- * (2) bdrv_drain_all_end() uses the same counter for all nodes,
- * regardless of which AioContext they are in.
+ * functions.
   */
  static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
-BdrvChild *parent, bool ignore_bds_parents,
-int *drained_end_counter)
+BdrvChild *parent, bool ignore_bds_parents)
  {
  BdrvChild *child;
  int old_quiesce_counter;
  
-assert(drained_end_counter != NULL);

-


[..]

--
Best regards,
Vladimir

[RFC PATCH] hw/intc: add implementation of GICD_IIDR to Arm GIC

[Alex Bennée]

a66a24585f (hw/intc/arm_gic: Implement read of GICC_IIDR) implemented
this for the CPU interface register. The fact we don't implement it
shows up when running Xen with -d guest_error which is definitely
wrong because the guest is perfectly entitled to read it.

Lightly re-factor this region of registers and also add a comment to
the function in case anyway was under the illusion we only return
bytes from a function called readb.

Signed-off-by: Alex Bennée 
---
 hw/intc/arm_gic.c | 43 +--
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 492b2421ab..515923416c 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -941,6 +941,10 @@ static void gic_complete_irq(GICState *s, int cpu, int 
irq, MemTxAttrs attrs)
 gic_update(s);
 }
 
+/*
+ * Although this is named a byte read we don't always return bytes and
+ * rely on the calling function oring bits together.
+ */
 static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
 {
 GICState *s = (GICState *)opaque;
@@ -954,23 +958,34 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr 
offset, MemTxAttrs attrs)
 cpu = gic_get_current_cpu(s);
 cm = 1 << cpu;
 if (offset < 0x100) {
-if (offset == 0) {  /* GICD_CTLR */
-if (s->security_extn && !attrs.secure) {
-/* The NS bank of this register is just an alias of the
- * EnableGrp1 bit in the S bank version.
- */
-return extract32(s->ctlr, 1, 1);
-} else {
-return s->ctlr;
+if (offset < 0xc ) {
+switch(offset) {
+case 0: /* GICD_CTLR[7:0] */
+{
+if (s->security_extn && !attrs.secure) {
+/* The NS bank of this register is just an alias of the
+ * EnableGrp1 bit in the S bank version.
+ */
+return extract32(s->ctlr, 1, 1);
+} else {
+return s->ctlr;
+}
 }
-}
-if (offset == 4)
-/* Interrupt Controller Type Register */
-return ((s->num_irq / 32) - 1)
+case 4: /* GIC_TYPER - Interrupt Controller Type Register */
+{
+return ((s->num_irq / 32) - 1)
 | ((s->num_cpu - 1) << 5)
 | (s->security_extn << 10);
-if (offset < 0x08)
-return 0;
+}
+case 8: /* GICD_IIDR - Implementer ID Register */
+{
+return 0x43b; /* Arm JEP106 identity */
+}
+default:
+/* return 0 for high bits of above */
+return 0;
+}
+}
 if (offset >= 0x80) {
 /* Interrupt Group Registers: these RAZ/WI if this is an NS
  * access to a GIC with the security extensions, or if the GIC
-- 
2.34.1

Re: [PATCH] virtio: remove the excess virtio features check

[Michael S. Tsirkin]

On Wed, Nov 09, 2022 at 07:10:21PM +0800, Xuan Zhuo wrote:
> In virtio_queue_enable(), we checked virtio feature VIRTIO_F_VERSION_1.
> 
> This check is not necessary, and conflict with SeaBIOS. The problem
> appeared in SeaBIOS. But we also remove this check.
> 
> Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg920538.html
> Signed-off-by: Xuan Zhuo 
> ---
>  hw/virtio/virtio.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 9683b2e158..701e23ea6a 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2499,11 +2499,6 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t 
> queue_index)
>  {
>  VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>  
> -if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> -error_report("queue_enable is only suppported in devices of virtio "
> - "1.0 or later.");
> -}
> -
>  if (k->queue_enable) {
>  k->queue_enable(vdev, queue_index);
>  }

Well this warning turned out to be actually useful.
Let's see whether we can fix seabios in time for release.
If yes I would just make it LOG_GUEST_ERR and limit to
latest machine type but not drop completely.

> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH V2] hw/riscv: virt: Remove size restriction for pflash

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Mon, Nov 07, 2022 at 06:32:01PM +0100, Andrew Jones wrote:

[...]

>> Padding is a good idea, but too much causes other problems. When building
>> lightweight VMs which may pull the firmware image from a network,
>> AArch64 VMs require 64MB of mostly zeros to be transferred first, which
>> can become a substantial amount of the overall boot time[*]. Being able to
>> create images smaller than the total flash device size, but still add some
>> pad for later growth, seems like the happy-medium to shoot for.
>
> QEMU configures the firmware using -blockdev,

Yes, even though the devices in question are not block devices.

>   so can use any file
> format that QEMU supports at the block layer.  IOW, you can store
> the firmware in a qcow2 file and thus you will never fetch any
> of the padding zeros to be transferred.  That said I'm not sure
> that libvirt supports anything other than a raw file today. 

Here's another idea.  The "raw" format supports exposing a slice of the
underlying block node (options @offset and @size).  It could support
padding.  Writing to the padding should then grow the underlying node.

Taking a step back to look at the bigger picture...  there are three
issues, I think:

(A) Storing padding on disk is wasteful.

Use a file system that supports sparse files, or an image format
that can represent the padding efficiently.

(B) Reading padding into memory is wasteful.

Matters mostly when a network is involved.  Use an image format that
can represent the padding efficiently.

(C) Dirtying memory for padding is wasteful.

I figure KSM could turn zero-padding into holes.

We could play with mmap() & friends.

Other ideas?

Any solution needs to work both for read-only and read/write padding.
Throwing away data written to the padding on cold restart is not what
I'd regard as "works".

Re: [PATCH V2] hw/riscv: virt: Remove size restriction for pflash

[Daniel P . Berrangé]

On Wed, Nov 09, 2022 at 04:26:53PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Mon, Nov 07, 2022 at 06:32:01PM +0100, Andrew Jones wrote:
> 
> [...]
> 
> >> Padding is a good idea, but too much causes other problems. When building
> >> lightweight VMs which may pull the firmware image from a network,
> >> AArch64 VMs require 64MB of mostly zeros to be transferred first, which
> >> can become a substantial amount of the overall boot time[*]. Being able to
> >> create images smaller than the total flash device size, but still add some
> >> pad for later growth, seems like the happy-medium to shoot for.
> >
> > QEMU configures the firmware using -blockdev,
> 
> Yes, even though the devices in question are not block devices.
> 
> >   so can use any file
> > format that QEMU supports at the block layer.  IOW, you can store
> > the firmware in a qcow2 file and thus you will never fetch any
> > of the padding zeros to be transferred.  That said I'm not sure
> > that libvirt supports anything other than a raw file today. 
> 
> Here's another idea.  The "raw" format supports exposing a slice of the
> underlying block node (options @offset and @size).  It could support
> padding.  Writing to the padding should then grow the underlying node.
> 
> Taking a step back to look at the bigger picture...  there are three
> issues, I think:
> 
> (A) Storing padding on disk is wasteful.
> 
> Use a file system that supports sparse files, or an image format
> that can represent the padding efficiently.
> 
> (B) Reading padding into memory is wasteful.
> 
> Matters mostly when a network is involved.  Use an image format that
> can represent the padding efficiently.
> 
> (C) Dirtying memory for padding is wasteful.
> 
> I figure KSM could turn zero-padding into holes.
> 
> We could play with mmap() & friends.
> 
> Other ideas?

Is (C) actually a separate issue ?  I thought it was simply the
result of (B) ?  ie if we skip reading the zero padding, we won't
be dirtying the memory with lots of zeros. we'll have mmap'd the
full 64 MB, but most won't be paged in since we wouldn't write
the zeros to it. Only if the guest writes to those areas do we
need to then flush it back out.

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

Re: [PATCH 05/13] block: Inline bdrv_drain_invoke()

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

bdrv_drain_invoke() has now two entirely separate cases that share no
code any more and are selected depending on a bool parameter. Each case
has only one caller. Just inline the function.

Signed-off-by: Kevin Wolf


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

RE: [PATCH v4 10/11] Hexagon (target/hexagon) Use direct block chaining for direct jump/branch

[Taylor Simpson]

> -Original Message-
> From: Richard Henderson 
> Sent: Tuesday, November 8, 2022 7:41 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@linaro.org; a...@rev.ng; a...@rev.ng; Brian Cain
> ; Matheus Bernardino (QUIC)
> 
> Subject: Re: [PATCH v4 10/11] Hexagon (target/hexagon) Use direct block
> chaining for direct jump/branch
> 
> >> -Original Message-
> >> From: Richard Henderson 
> >> Sent: Tuesday, November 8, 2022 1:24 AM
> >> To: Taylor Simpson ; qemu-devel@nongnu.org
> >> Cc: phi...@linaro.org; a...@rev.ng; a...@rev.ng; Brian Cain
> >> ; Matheus Bernardino (QUIC)
> >> 
> >> Subject: Re: [PATCH v4 10/11] Hexagon (target/hexagon) Use direct
> >> block chaining for direct jump/branch
> >>
> >> On 11/8/22 15:05, Taylor Simpson wrote:
> >>>static void hexagon_tr_tb_start(DisasContextBase *db, CPUState
> *cpu)
> >>>{
> >>> +DisasContext *ctx = container_of(db, DisasContext, base);
> >>> +ctx->branch_cond = TCG_COND_NEVER;
> >>>}
> >>
> >> Typically this would go in hexagon_tr_init_disas_context as well, but
> >> I don't suppose it really matters.
> >
> > AFAICT, these are always called back to back.  So, it's not clear to me what
> the distinction should be.
> 
> ops->tb_start is called after gen_tb_start, so you can emit code that
> ops->comes after the
> interrupt/icount check, but before the first guest instruction.  Rarely 
> needed,
> should probably be allowed to be NULL.

OK, I will move this to init_disas_context.

Thanks,
Taylor

[PATCH] Add missing pixman dependecy

[mrezanin]

From: Miroslav Rezanina 

Commit cfead31326 'acpi: pc: vga: use AcpiDevAmlIf interface to build VGA 
device descriptors' added
a new file - acpi-vga.c. This file (indirectly) includes pixman.h file so we 
need to ensure pixman
is available when file is compiled.

Signed-off-by: Miroslav Rezanina 
---
 hw/display/meson.build | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/display/meson.build b/hw/display/meson.build
index 7a725ed80e..c8e84e9707 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -43,7 +43,7 @@ if (config_all_devices.has_key('CONFIG_VGA_CIRRUS') or
 config_all_devices.has_key('CONFIG_VMWARE_VGA') or
 config_all_devices.has_key('CONFIG_ATI_VGA')
)
-  softmmu_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
+  softmmu_ss.add(when: 'CONFIG_ACPI', if_true: [files('acpi-vga.c'), pixman],
   if_false: files('acpi-vga-stub.c'))
 endif
 
@@ -51,7 +51,7 @@ if config_all_devices.has_key('CONFIG_QXL')
   qxl_ss = ss.source_set()
   qxl_ss.add(when: 'CONFIG_QXL', if_true: [files('qxl.c', 'qxl-logger.c', 
'qxl-render.c'),
pixman, spice])
-  qxl_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
+  qxl_ss.add(when: 'CONFIG_ACPI', if_true: [files('acpi-vga.c'), pixman],
   if_false: files('acpi-vga-stub.c'))
   hw_display_modules += {'qxl': qxl_ss}
 endif
@@ -99,14 +99,14 @@ if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
 if_true: [files('virtio-vga.c'), pixman])
   virtio_vga_ss.add(when: 'CONFIG_VHOST_USER_VGA',
 if_true: files('vhost-user-vga.c'))
-  virtio_vga_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
+  virtio_vga_ss.add(when: 'CONFIG_ACPI', if_true: [files('acpi-vga.c'), 
pixman],
  if_false: files('acpi-vga-stub.c'))
   hw_display_modules += {'virtio-vga': virtio_vga_ss}
 
   virtio_vga_gl_ss = ss.source_set()
   virtio_vga_gl_ss.add(when: ['CONFIG_VIRTIO_VGA', virgl, opengl],
if_true: [files('virtio-vga-gl.c'), pixman])
-  virtio_vga_gl_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
+  virtio_vga_gl_ss.add(when: 'CONFIG_ACPI', if_true: [files('acpi-vga.c'), 
pixman],
 if_false: files('acpi-vga-stub.c'))
   hw_display_modules += {'virtio-vga-gl': virtio_vga_gl_ss}
 endif
-- 
2.31.1

Re: [PATCH] avocado: use sha1 for fc31 imgs to avoid first time re-download

[Daniel Henrique Barboza]

On 10/27/22 06:01, Daniel P. Berrangé wrote:

On Thu, Oct 27, 2022 at 09:46:29AM +0200, Thomas Huth wrote:

On 24/10/2022 11.02, Daniel P. Berrangé wrote:

On Sat, Oct 22, 2022 at 02:03:50PM -0300, Daniel Henrique Barboza wrote:

'make check-avocado' will download any images that aren't present in the
cache via 'get-vm-images' in tests/Makefile.include. The target that
downloads fedora 31 images, get-vm-image-fedora-31, will use 'avocado
vmimage get  --distro=fedora --distro-version=31 --arch=(...)' to
download the image for each arch. Note that this command does not
support any argument to set the hash algorithm used and, based on the
avocado source code [1], DEFAULT_HASH_ALGORITHM is set to "sha1". The
sha1 hash is stored in a Fedora-Cloud-Base-31-1.9.{ARCH}.qcow2-CHECKSUM
in the cache.



For now, in QEMU, let's use sha1 for all Fedora 31 images. This will
immediately spares us at least one extra download for each Fedora 31
image that we're doing in all our CI runs.

[1] https://github.com/avocado-framework/avocado.git @ 942a5d6972906
[2] https://github.com/avocado-framework/avocado/issues/5496


Can we just ask Avocado maintainers to fix this problem on their
side to allow use of a modern hash alg as a priority item. We've
already had this problem in QEMU for over a year AFAICT, so doesn't
seem like we need to urgently do a workaround on QEMU side, so we
can get Avocado devs to commit to fixing it in the next month.


Do we have such a commitment? ... The avocado version in QEMU is completely
backlevel these days, it's still using version 88.1 from May 2021, i.e.
there hasn't been any update since more than a year. I recently tried to
bump it to a newer version on my own (since I'm still suffering from the
problem that find_free_port() does not work if you don't have a local IPv6
address), but it's not that straight forward since the recent versions of
avocado changed a lot of things (e.g. the new nrunner - do we want to run
tests in parallel? If so it breaks a lot of the timeout settings, I think),
so an update needs a lot of careful testing...


That it is so difficult to update Avocado after barely more than
1 year is not exactly a strong vote of confidence in our continued
use of Avocado long term :-(



By the way, Avocado just provided a fix for the problem this patch is trying
to amend:

https://github.com/avocado-framework/avocado/pull/5515#issuecomment-1308872846


Is there an easy way to plug upstream Avocado into QEMU? I would like to test
tests/avocado/boot_linux.py:BootLinuxPPC64.test_pseries_tcg to see if the 
problem
is fixed by Avocado upstream.


Thanks,


Daniel




With regards,
Daniel

Re: [RFC 5/8] vdpa: Add vdpa memory listener

[Eugenio Perez Martin]

On Tue, Aug 23, 2022 at 5:58 AM Jason Wang  wrote:
>
> On Fri, Aug 19, 2022 at 6:35 PM Eugenio Perez Martin
>  wrote:
> >
> > On Fri, Aug 19, 2022 at 11:01 AM Jason Wang  wrote:
> > >
> > > On Fri, Aug 19, 2022 at 4:30 PM Eugenio Perez Martin
> > >  wrote:
> > > >
> > > > On Fri, Aug 19, 2022 at 8:29 AM Jason Wang  wrote:
> > > > >
> > > > > On Thu, Aug 11, 2022 at 2:42 AM Eugenio Pérez  
> > > > > wrote:
> > > > > >
> > > > > > This enable net/vdpa to restart the full device when a migration is
> > > > > > started or stopped.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez 
> > > > >
> > > > > I have the following questions
> > > > >
> > > > > 1) any reason that we need to make this net specific? The dirty page
> > > > > tracking via shadow virtqueue is pretty general. And the net specific
> > > > > part was done via NetClientInfo anyhow.
> > > >
> > > > The listener is only used to know when migration is started / stopped,
> > > > no need for actual memory tracking. Maybe there is a better way to do
> > > > so?
> > >
> > > Not sure, SaveVMHandlers?
> > >
> >
> > I'm fine with investigating this, but the only entry in the doc says
> > it's the "legacy way". I assume the "modern way" is through
> > VMStateDescription, which is in virtio-net.
>
> Right.
>
> >
> > The "pre_save" member already assumes the vhost backend is stopped, so
> > I'm not sure if this way is valid.
>
> I wonder if we can
>
> 1) new VhostOps
> 2) call that ops in vhost_log_gloabal_start/stop?
>

Bringing this thread up as I'm rebasing on top of the latest asid version.

We can detect that moment when set_features is called to set _F_LOG.
The problem is that it makes us duplicate the startup and teardown
code of the backend.

SVQ was proposed originally that way [1].

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg06049.html

Thanks!

Re: [PATCH V2] hw/riscv: virt: Remove size restriction for pflash

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Wed, Nov 09, 2022 at 04:26:53PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé  writes:
>> 
>> > On Mon, Nov 07, 2022 at 06:32:01PM +0100, Andrew Jones wrote:
>> 
>> [...]
>> 
>> >> Padding is a good idea, but too much causes other problems. When building
>> >> lightweight VMs which may pull the firmware image from a network,
>> >> AArch64 VMs require 64MB of mostly zeros to be transferred first, which
>> >> can become a substantial amount of the overall boot time[*]. Being able to
>> >> create images smaller than the total flash device size, but still add some
>> >> pad for later growth, seems like the happy-medium to shoot for.
>> >
>> > QEMU configures the firmware using -blockdev,
>> 
>> Yes, even though the devices in question are not block devices.
>> 
>> >   so can use any file
>> > format that QEMU supports at the block layer.  IOW, you can store
>> > the firmware in a qcow2 file and thus you will never fetch any
>> > of the padding zeros to be transferred.  That said I'm not sure
>> > that libvirt supports anything other than a raw file today. 
>> 
>> Here's another idea.  The "raw" format supports exposing a slice of the
>> underlying block node (options @offset and @size).  It could support
>> padding.  Writing to the padding should then grow the underlying node.
>> 
>> Taking a step back to look at the bigger picture...  there are three
>> issues, I think:
>> 
>> (A) Storing padding on disk is wasteful.
>> 
>> Use a file system that supports sparse files, or an image format
>> that can represent the padding efficiently.
>> 
>> (B) Reading padding into memory is wasteful.
>> 
>> Matters mostly when a network is involved.  Use an image format that
>> can represent the padding efficiently.
>> 
>> (C) Dirtying memory for padding is wasteful.
>> 
>> I figure KSM could turn zero-padding into holes.
>> 
>> We could play with mmap() & friends.
>> 
>> Other ideas?
>
> Is (C) actually a separate issue ?  I thought it was simply the
> result of (B) ?  ie if we skip reading the zero padding, we won't
> be dirtying the memory with lots of zeros. we'll have mmap'd the
> full 64 MB, but most won't be paged in since we wouldn't write
> the zeros to it. Only if the guest writes to those areas do we
> need to then flush it back out.

I expressed myself poorly.  All three are related, but there's still a
distinction between each of them in my thinking.

Say we use an image format that compresses data.  Represents the padding
efficiently.  Storage on disk is efficient (A), and so is reading it
(B).  Trouble is decompressing it to memory dirties the memory unless we
take care not to write all-zero pages (C).

Clearer now?

Re: [PATCH V2] hw/riscv: virt: Remove size restriction for pflash

[Daniel P . Berrangé]

On Wed, Nov 09, 2022 at 04:45:18PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Wed, Nov 09, 2022 at 04:26:53PM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé  writes:
> >> 
> >> > On Mon, Nov 07, 2022 at 06:32:01PM +0100, Andrew Jones wrote:
> >> 
> >> [...]
> >> 
> >> >> Padding is a good idea, but too much causes other problems. When 
> >> >> building
> >> >> lightweight VMs which may pull the firmware image from a network,
> >> >> AArch64 VMs require 64MB of mostly zeros to be transferred first, which
> >> >> can become a substantial amount of the overall boot time[*]. Being able 
> >> >> to
> >> >> create images smaller than the total flash device size, but still add 
> >> >> some
> >> >> pad for later growth, seems like the happy-medium to shoot for.
> >> >
> >> > QEMU configures the firmware using -blockdev,
> >> 
> >> Yes, even though the devices in question are not block devices.
> >> 
> >> >   so can use any file
> >> > format that QEMU supports at the block layer.  IOW, you can store
> >> > the firmware in a qcow2 file and thus you will never fetch any
> >> > of the padding zeros to be transferred.  That said I'm not sure
> >> > that libvirt supports anything other than a raw file today. 
> >> 
> >> Here's another idea.  The "raw" format supports exposing a slice of the
> >> underlying block node (options @offset and @size).  It could support
> >> padding.  Writing to the padding should then grow the underlying node.
> >> 
> >> Taking a step back to look at the bigger picture...  there are three
> >> issues, I think:
> >> 
> >> (A) Storing padding on disk is wasteful.
> >> 
> >> Use a file system that supports sparse files, or an image format
> >> that can represent the padding efficiently.
> >> 
> >> (B) Reading padding into memory is wasteful.
> >> 
> >> Matters mostly when a network is involved.  Use an image format that
> >> can represent the padding efficiently.
> >> 
> >> (C) Dirtying memory for padding is wasteful.
> >> 
> >> I figure KSM could turn zero-padding into holes.
> >> 
> >> We could play with mmap() & friends.
> >> 
> >> Other ideas?
> >
> > Is (C) actually a separate issue ?  I thought it was simply the
> > result of (B) ?  ie if we skip reading the zero padding, we won't
> > be dirtying the memory with lots of zeros. we'll have mmap'd the
> > full 64 MB, but most won't be paged in since we wouldn't write
> > the zeros to it. Only if the guest writes to those areas do we
> > need to then flush it back out.
> 
> I expressed myself poorly.  All three are related, but there's still a
> distinction between each of them in my thinking.
> 
> Say we use an image format that compresses data.  Represents the padding
> efficiently.  Storage on disk is efficient (A), and so is reading it
> (B).  Trouble is decompressing it to memory dirties the memory unless we
> take care not to write all-zero pages (C).
> 
> Clearer now?

Ok yeah, so reading can be efficient, but if the reader doesn't pay
attention to where the holes are, it'll dirty memory anyway.

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

Re: [PATCH v9 0/8] KVM: mm: fd-based approach for supporting KVM

[Kirill A. Shutemov]

On Mon, Nov 07, 2022 at 04:41:41PM -0800, Isaku Yamahata wrote:
> On Thu, Nov 03, 2022 at 05:43:52PM +0530,
> Vishal Annapurve  wrote:
> 
> > On Tue, Oct 25, 2022 at 8:48 PM Chao Peng  
> > wrote:
> > >
> > > This patch series implements KVM guest private memory for confidential
> > > computing scenarios like Intel TDX[1]. If a TDX host accesses
> > > TDX-protected guest memory, machine check can happen which can further
> > > crash the running host system, this is terrible for multi-tenant
> > > configurations. The host accesses include those from KVM userspace like
> > > QEMU. This series addresses KVM userspace induced crash by introducing
> > > new mm and KVM interfaces so KVM userspace can still manage guest memory
> > > via a fd-based approach, but it can never access the guest memory
> > > content.
> > >
> > > The patch series touches both core mm and KVM code. I appreciate
> > > Andrew/Hugh and Paolo/Sean can review and pick these patches. Any other
> > > reviews are always welcome.
> > >   - 01: mm change, target for mm tree
> > >   - 02-08: KVM change, target for KVM tree
> > >
> > > Given KVM is the only current user for the mm part, I have chatted with
> > > Paolo and he is OK to merge the mm change through KVM tree, but
> > > reviewed-by/acked-by is still expected from the mm people.
> > >
> > > The patches have been verified in Intel TDX environment, but Vishal has
> > > done an excellent work on the selftests[4] which are dedicated for this
> > > series, making it possible to test this series without innovative
> > > hardware and fancy steps of building a VM environment. See Test section
> > > below for more info.
> > >
> > >
> > > Introduction
> > > 
> > > KVM userspace being able to crash the host is horrible. Under current
> > > KVM architecture, all guest memory is inherently accessible from KVM
> > > userspace and is exposed to the mentioned crash issue. The goal of this
> > > series is to provide a solution to align mm and KVM, on a userspace
> > > inaccessible approach of exposing guest memory.
> > >
> > > Normally, KVM populates secondary page table (e.g. EPT) by using a host
> > > virtual address (hva) from core mm page table (e.g. x86 userspace page
> > > table). This requires guest memory being mmaped into KVM userspace, but
> > > this is also the source where the mentioned crash issue can happen. In
> > > theory, apart from those 'shared' memory for device emulation etc, guest
> > > memory doesn't have to be mmaped into KVM userspace.
> > >
> > > This series introduces fd-based guest memory which will not be mmaped
> > > into KVM userspace. KVM populates secondary page table by using a
> > 
> > With no mappings in place for userspace VMM, IIUC, looks like the host
> > kernel will not be able to find the culprit userspace process in case
> > of Machine check error on guest private memory. As implemented in
> > hwpoison_user_mappings, host kernel tries to look at the processes
> > which have mapped the pfns with hardware error.
> > 
> > Is there a modification needed in mce handling logic of the host
> > kernel to immediately send a signal to the vcpu thread accessing
> > faulting pfn backing guest private memory?
> 
> mce_register_decode_chain() can be used.  MCE physical address(p->mce_addr)
> includes host key id in addition to real physical address.  By searching used
> hkid by KVM, we can determine if the page is assigned to guest TD or not. If
> yes, send SIGBUS.
> 
> kvm_machine_check() can be enhanced for KVM specific use.  This is before
> memory_failure() is called, though.
> 
> any other ideas?

That's too KVM-centric. It will not work for other possible user of
restricted memfd.

I tried to find a way to get it right: we need to get restricted memfd
code info about corrupted page so it can invalidate its users. On the next
request of the page the user will see an error. In case of KVM, the error
will likely escalate to SIGBUS.

The problem is that core-mm code that handles memory failure knows nothing
about restricted memfd. It only sees that the page belongs to a normal
memfd.

AFAICS, there's no way to get it intercepted from the shim level. shmem
code has to be patches. shmem_error_remove_page() has to call into
restricted memfd code.

Hugh, are you okay with this? Or maybe you have a better idea?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

[PATCH 2/4] tulip: Remove unused variable

[mrezanin]

From: Miroslav Rezanina 

Variable n used in tulip_idblock_crc function is only incremented but never 
read.
This causes 'Unused but set variable' warning on Clang 15.0.1 compiler.

Removing the variable to prevent the warning.

Signed-off-by: Miroslav Rezanina 
---
 hw/net/tulip.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index b9e42c322a..c2b3b1bdfa 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -870,11 +870,10 @@ static const MemoryRegionOps tulip_ops = {
 
 static void tulip_idblock_crc(TULIPState *s, uint16_t *srom)
 {
-int word, n;
+int word;
 int bit;
 unsigned char bitval, crc;
 const int len = 9;
-n = 0;
 crc = -1;
 
 for (word = 0; word < len; word++) {
@@ -887,7 +886,6 @@ static void tulip_idblock_crc(TULIPState *s, uint16_t *srom)
 srom[len - 1] = (srom[len - 1] & 0xff00) | (unsigned short)crc;
 break;
 }
-n++;
 bitval = ((srom[word] >> bit) & 1) ^ ((crc >> 7) & 1);
 crc = crc << 1;
 if (bitval == 1) {
-- 
2.31.1

[PATCH 3/4] qemu-img: remove unused variable

[mrezanin]

From: Miroslav Rezanina 

Variable block_count used in img_dd function is only incremented but never read.
This causes 'Unused but set variable' warning on Clang 15.0.1 compiler.

Removing the variable to prevent the warning.

Signed-off-by: Miroslav Rezanina 
---
 qemu-img.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index a3b64c88af..a9b3a8103c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4922,7 +4922,7 @@ static int img_dd(int argc, char **argv)
 const char *out_fmt = "raw";
 const char *fmt = NULL;
 int64_t size = 0;
-int64_t block_count = 0, out_pos, in_pos;
+int64_t out_pos, in_pos;
 bool force_share = false;
 struct DdInfo dd = {
 .flags = 0,
@@ -5122,7 +5122,7 @@ static int img_dd(int argc, char **argv)
 
 in.buf = g_new(uint8_t, in.bsz);
 
-for (out_pos = 0; in_pos < size; block_count++) {
+for (out_pos = 0; in_pos < size; ) {
 int bytes = (in_pos + in.bsz > size) ? size - in_pos : in.bsz;
 
 ret = blk_pread(blk1, in_pos, bytes, in.buf, 0);
-- 
2.31.1

[PATCH 0/4] Removal of several unused variables causing

[mrezanin]

From: Miroslav Rezanina 

When trying to run qemu build using clang 15.0.1 compiler with
--enable-werror option, several 'Unused but set variable' warnings
was breaking the build.

These variables show similar pattern - they are only incremented but
final value of the variable is never used. 

Removing this variables to enable using --enable-werror option
with Clang 15.0.1.

Miroslav Rezanina (4):
  rtl8139: Remove unused variable
  tulip: Remove unused variable
  qemu-img: remove unused variable
  host-libusb: Remove unused variable

 hw/net/rtl8139.c |  2 --
 hw/net/tulip.c   |  4 +---
 hw/usb/host-libusb.c | 15 ---
 qemu-img.c   |  4 ++--
 4 files changed, 3 insertions(+), 22 deletions(-)

-- 
2.31.1

[PATCH 1/4] rtl8139: Remove unused variable

[mrezanin]

From: Miroslav Rezanina 

Variable send_count used in rtl8139_cplus_transmit_one function is only
incremented but never read. This causes 'Unused but set variable' warning
on Clang 15.0.1 compiler.

Removing the variable to prevent the warning.

Signed-off-by: Miroslav Rezanina 
---
 hw/net/rtl8139.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 6b65823b4b..e6643e3c9d 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2156,7 +2156,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 ip_data_len, saved_size - ETH_HLEN, large_send_mss);
 
 int tcp_send_offset = 0;
-int send_count = 0;
 
 /* maximum IP header length is 60 bytes */
 uint8_t saved_ip_header[60];
@@ -2261,7 +2260,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 /* add transferred count to TCP sequence number */
 stl_be_p(&p_tcp_hdr->th_seq,
  chunk_size + ldl_be_p(&p_tcp_hdr->th_seq));
-++send_count;
 }
 
 /* Stop sending this frame */
-- 
2.31.1

[PATCH 4/4] host-libusb: Remove unused variable

[mrezanin]

From: Miroslav Rezanina 

Variable unconnected used in usb_host_auto_check function is only incremented
but never read as line where it is read was disabled since introducing the code.
This causes 'Unused but set variable' warning on Clang 15.0.1 compiler.

Removing the variable and disabled code to prevent the warning.

Signed-off-by: Miroslav Rezanina 
---
 hw/usb/host-libusb.c | 15 ---
 1 file changed, 15 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 28f8af8941..176868d345 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1837,7 +1837,6 @@ static void usb_host_auto_check(void *unused)
 struct USBAutoFilter *f;
 libusb_device **devs = NULL;
 struct libusb_device_descriptor ddesc;
-int unconnected = 0;
 int i, n;
 
 if (usb_host_init() != 0) {
@@ -1897,9 +1896,6 @@ static void usb_host_auto_check(void *unused)
 libusb_free_device_list(devs, 1);
 
 QTAILQ_FOREACH(s, &hostdevs, next) {
-if (s->dh == NULL) {
-unconnected++;
-}
 if (s->seen == 0) {
 if (s->dh) {
 usb_host_close(s);
@@ -1908,17 +1904,6 @@ static void usb_host_auto_check(void *unused)
 }
 s->seen = 0;
 }
-
-#if 0
-if (unconnected == 0) {
-/* nothing to watch */
-if (usb_auto_timer) {
-timer_del(usb_auto_timer);
-trace_usb_host_auto_scan_disabled();
-}
-return;
-}
-#endif
 }
 
 if (!usb_vmstate) {
-- 
2.31.1

Re: [PATCH 06/13] block: Drain invidual nodes during reopen

[Vladimir Sementsov-Ogievskiy]

In subject: individual

On 11/8/22 15:37, Kevin Wolf wrote:

bdrv_reopen() and friends use subtree drains as a lazy way of covering
all the nodes they touch. Turns out that this lazy way is a lot more
complicated than just draining the nodes individually, even not
accounting for the additional complexity in the drain mechanism itself.

Simplify the code by switching to draining the individual nodes that are
already managed in the BlockReopenQueue anyway.

Signed-off-by: Kevin Wolf 
---
  block.c | 11 ---
  block/replication.c |  6 --
  blockdev.c  | 13 -
  3 files changed, 4 insertions(+), 26 deletions(-)



[..]


  bdrv_reopen_queue_free(queue);
-for (p = drained; p; p = p->next) {
-BlockDriverState *bs = p->data;
-AioContext *ctx = bdrv_get_aio_context(bs);
-
-aio_context_acquire(ctx);


In bdrv_reopen_queue_free() we don't have this acquire()/release() pair around 
bdrv_drained_end(). We don't need it anymore?


-bdrv_subtree_drained_end(bs);
-aio_context_release(ctx);
-}
-g_slist_free(drained);
  }
  
  void qmp_blockdev_del(const char *node_name, Error **errp)


--
Best regards,
Vladimir

[PATCH for-8.0 3/9] hw/intc: Convert TYPE_ARM_GIC_COMMON to 3-phase reset

[Peter Maydell]

Convert the TYPE_ARM_GIC_COMMON device to 3-phase reset.  This is a
simple no-behaviour-change conversion.

Signed-off-by: Peter Maydell 
---
 hw/intc/arm_gic_common.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 7b44d5625b6..a379cea3959 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -261,9 +261,9 @@ static inline void arm_gic_common_reset_irq_state(GICState 
*s, int first_cpu,
 }
 }
 
-static void arm_gic_common_reset(DeviceState *dev)
+static void arm_gic_common_reset_hold(Object *obj)
 {
-GICState *s = ARM_GIC_COMMON(dev);
+GICState *s = ARM_GIC_COMMON(obj);
 int i, j;
 int resetprio;
 
@@ -364,9 +364,10 @@ static Property arm_gic_common_properties[] = {
 static void arm_gic_common_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 ARMLinuxBootIfClass *albifc = ARM_LINUX_BOOT_IF_CLASS(klass);
 
-dc->reset = arm_gic_common_reset;
+rc->phases.hold = arm_gic_common_reset_hold;
 dc->realize = arm_gic_common_realize;
 device_class_set_props(dc, arm_gic_common_properties);
 dc->vmsd = &vmstate_gic;
-- 
2.25.1

Re: [PATCH] Add missing pixman dependecy

[Philippe Mathieu-Daudé]

Hi Mirek,

On 9/11/22 16:34, mreza...@redhat.com wrote:

From: Miroslav Rezanina 

Commit cfead31326 'acpi: pc: vga: use AcpiDevAmlIf interface to build VGA 
device descriptors' added
a new file - acpi-vga.c. This file (indirectly) includes pixman.h file so we 
need to ensure pixman
is available when file is compiled.


We don't need this dependency if we move the build_vga_aml() declaration
out of hw/display/vga_int.h, i.e. hw/display/acpi-vga.h.

Regards,

Phil.

[PATCH for-8.0 2/9] hw/arm: Convert TYPE_ARM_SMMUV3 to 3-phase reset

[Peter Maydell]

Convert the TYPE_ARM_SMMUV3 device to 3-phase reset.  The legacy
reset method doesn't do anything that's invalid in the hold phase, so
the conversion only requires changing it to a hold phase method, and
using the 3-phase versions of the "save the parent reset method and
chain to it" code.

Signed-off-by: Peter Maydell 
---
 include/hw/arm/smmuv3.h |  2 +-
 hw/arm/smmuv3.c | 12 
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index c641e60735e..f1921fdf9e7 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -77,7 +77,7 @@ struct SMMUv3Class {
 /*< public >*/
 
 DeviceRealize parent_realize;
-DeviceReset   parent_reset;
+ResettablePhases parent_phases;
 };
 
 #define TYPE_ARM_SMMUV3   "arm-smmuv3"
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index daa80e9c7b6..955b89c8d59 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1431,12 +1431,14 @@ static void smmu_init_irq(SMMUv3State *s, SysBusDevice 
*dev)
 }
 }
 
-static void smmu_reset(DeviceState *dev)
+static void smmu_reset_hold(Object *obj)
 {
-SMMUv3State *s = ARM_SMMUV3(dev);
+SMMUv3State *s = ARM_SMMUV3(obj);
 SMMUv3Class *c = ARM_SMMUV3_GET_CLASS(s);
 
-c->parent_reset(dev);
+if (c->parent_phases.hold) {
+c->parent_phases.hold(obj);
+}
 
 smmuv3_init_regs(s);
 }
@@ -1520,10 +1522,12 @@ static void smmuv3_instance_init(Object *obj)
 static void smmuv3_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 SMMUv3Class *c = ARM_SMMUV3_CLASS(klass);
 
 dc->vmsd = &vmstate_smmuv3;
-device_class_set_parent_reset(dc, smmu_reset, &c->parent_reset);
+resettable_class_set_parent_phases(rc, NULL, smmu_reset_hold, NULL,
+   &c->parent_phases);
 c->parent_realize = dc->realize;
 dc->realize = smmu_realize;
 }
-- 
2.25.1

[PATCH for-8.0 6/9] hw/intc: Convert TYPE_KVM_ARM_GICV3 to 3-phase reset

[Peter Maydell]

Convert the TYPE_KVM_ARM_GICV3 device to 3-phase reset.

Signed-off-by: Peter Maydell 
---
 hw/intc/arm_gicv3_kvm.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 3ca643ecba4..72ad916d3db 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -77,7 +77,7 @@ DECLARE_OBJ_CHECKERS(GICv3State, KVMARMGICv3Class,
 struct KVMARMGICv3Class {
 ARMGICv3CommonClass parent_class;
 DeviceRealize parent_realize;
-void (*parent_reset)(DeviceState *dev);
+ResettablePhases parent_phases;
 };
 
 static void kvm_arm_gicv3_set_irq(void *opaque, int irq, int level)
@@ -703,14 +703,16 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const 
ARMCPRegInfo *ri)
 c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
 }
 
-static void kvm_arm_gicv3_reset(DeviceState *dev)
+static void kvm_arm_gicv3_reset_hold(Object *obj)
 {
-GICv3State *s = ARM_GICV3_COMMON(dev);
+GICv3State *s = ARM_GICV3_COMMON(obj);
 KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
 
 DPRINTF("Reset\n");
 
-kgc->parent_reset(dev);
+if (kgc->parent_phases.hold) {
+kgc->parent_phases.hold(obj);
+}
 
 if (s->migration_blocker) {
 DPRINTF("Cannot put kernel gic state, no kernel interface\n");
@@ -890,6 +892,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error 
**errp)
 static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_CLASS(klass);
 KVMARMGICv3Class *kgc = KVM_ARM_GICV3_CLASS(klass);
 
@@ -897,7 +900,8 @@ static void kvm_arm_gicv3_class_init(ObjectClass *klass, 
void *data)
 agcc->post_load = kvm_arm_gicv3_put;
 device_class_set_parent_realize(dc, kvm_arm_gicv3_realize,
 &kgc->parent_realize);
-device_class_set_parent_reset(dc, kvm_arm_gicv3_reset, &kgc->parent_reset);
+resettable_class_set_parent_phases(rc, NULL, kvm_arm_gicv3_reset_hold, 
NULL,
+   &kgc->parent_phases);
 }
 
 static const TypeInfo kvm_arm_gicv3_info = {
-- 
2.25.1

[PATCH for-8.0 5/9] hw/intc: Convert TYPE_ARM_GICV3_COMMON to 3-phase reset

[Peter Maydell]

Convert the TYPE_ARM_GICV3_COMMON parent class to 3-phase reset.

Signed-off-by: Peter Maydell 
---
 hw/intc/arm_gicv3_common.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 351843db4aa..642a8243ed4 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -450,9 +450,9 @@ static void arm_gicv3_finalize(Object *obj)
 g_free(s->redist_region_count);
 }
 
-static void arm_gicv3_common_reset(DeviceState *dev)
+static void arm_gicv3_common_reset_hold(Object *obj)
 {
-GICv3State *s = ARM_GICV3_COMMON(dev);
+GICv3State *s = ARM_GICV3_COMMON(obj);
 int i;
 
 for (i = 0; i < s->num_cpu; i++) {
@@ -578,9 +578,10 @@ static Property arm_gicv3_common_properties[] = {
 static void arm_gicv3_common_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 ARMLinuxBootIfClass *albifc = ARM_LINUX_BOOT_IF_CLASS(klass);
 
-dc->reset = arm_gicv3_common_reset;
+rc->phases.hold = arm_gicv3_common_reset_hold;
 dc->realize = arm_gicv3_common_realize;
 device_class_set_props(dc, arm_gicv3_common_properties);
 dc->vmsd = &vmstate_gicv3;
-- 
2.25.1

[PATCH for-8.0 0/9] arm: Convert Arm GIC, ITS, SMMU devices to 3-phase reset

[Peter Maydell]

This patchset converts some Arm devices to 3-phase reset.  The
rationale here is that it would be nice to get rid of the
device_class_set_parent_reset() function, which is used by
legacy-reset subclasses which want to chain to their parent's reset
function. There aren't very many of these devices in total, and
if we convert them and their parent classes to 3-phase reset they
can use the 3-phase-reset equivalent function
resettable_class_set_parent_phases().

Eventually this will then let us simplify the transitional
code for handling old-style device reset.

Note that it's necessary to convert the parent class before
the subclass -- the resettable transitional logic will
handle the situation where the subclass is still using
legacy reset and chaining to what it thinks is the parent's
legacy reset function (by doing a 3-phase reset on the parent),
but if the subclass is 3-phase then the parent must be too.

I plan to do the other uses of device_class_set_parent_reset()
too, but since the conversion patchsets don't depend on each
other I'm going to send them out piecemeal so they can be
cc'd to the relevant maintainers, rather than having a
single massive patchset with a billion people on cc.

thanks
-- PMM

Peter Maydell (9):
  hw/arm: Convert TYPE_ARM_SMMU to 3-phase reset
  hw/arm: Convert TYPE_ARM_SMMUV3 to 3-phase reset
  hw/intc: Convert TYPE_ARM_GIC_COMMON to 3-phase reset
  hw/intc: Convert TYPE_ARM_GIC_KVM to 3-phase reset
  hw/intc: Convert TYPE_ARM_GICV3_COMMON to 3-phase reset
  hw/intc: Convert TYPE_KVM_ARM_GICV3 to 3-phase reset
  hw/intc: Convert TYPE_ARM_GICV3_ITS_COMMON to 3-phase reset
  hw/intc: Convert TYPE_ARM_GICV3_ITS to 3-phase reset
  hw/intc: Convert TYPE_KVM_ARM_ITS to 3-phase reset

 include/hw/arm/smmuv3.h|  2 +-
 hw/arm/smmu-common.c   |  7 ---
 hw/arm/smmuv3.c| 12 
 hw/intc/arm_gic_common.c   |  7 ---
 hw/intc/arm_gic_kvm.c  | 14 +-
 hw/intc/arm_gicv3_common.c |  7 ---
 hw/intc/arm_gicv3_its.c| 14 +-
 hw/intc/arm_gicv3_its_common.c |  7 ---
 hw/intc/arm_gicv3_its_kvm.c| 14 +-
 hw/intc/arm_gicv3_kvm.c| 14 +-
 10 files changed, 61 insertions(+), 37 deletions(-)

-- 
2.25.1

[PATCH for-8.0 1/9] hw/arm: Convert TYPE_ARM_SMMU to 3-phase reset

[Peter Maydell]

Convert the TYPE_ARM_SMMU device to 3-phase reset.  The legacy method
doesn't do anything that's invalid in the hold phase, so the
conversion is simple and not a behaviour change.

Note that we must convert this base class before we can convert the
TYPE_ARM_SMMUV3 subclass -- transitional support in Resettable
handles "chain to parent class reset" when the base class is 3-phase
and the subclass is still using legacy reset, but not the other way
around.

Signed-off-by: Peter Maydell 
---
 hw/arm/smmu-common.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index e09b9c13b74..220838525d4 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -526,9 +526,9 @@ static void smmu_base_realize(DeviceState *dev, Error 
**errp)
 }
 }
 
-static void smmu_base_reset(DeviceState *dev)
+static void smmu_base_reset_hold(Object *obj)
 {
-SMMUState *s = ARM_SMMU(dev);
+SMMUState *s = ARM_SMMU(obj);
 
 g_hash_table_remove_all(s->configs);
 g_hash_table_remove_all(s->iotlb);
@@ -543,12 +543,13 @@ static Property smmu_dev_properties[] = {
 static void smmu_base_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 SMMUBaseClass *sbc = ARM_SMMU_CLASS(klass);
 
 device_class_set_props(dc, smmu_dev_properties);
 device_class_set_parent_realize(dc, smmu_base_realize,
 &sbc->parent_realize);
-dc->reset = smmu_base_reset;
+rc->phases.hold = smmu_base_reset_hold;
 }
 
 static const TypeInfo smmu_base_info = {
-- 
2.25.1

[PATCH for-8.0 7/9] hw/intc: Convert TYPE_ARM_GICV3_ITS_COMMON to 3-phase reset

[Peter Maydell]

Convert the TYPE_ARM_GICV3_ITS_COMMON parent class to 3-phase reset.

Signed-off-by: Peter Maydell 
---
 hw/intc/arm_gicv3_its_common.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
index 90b85f1e25c..d7532a7a899 100644
--- a/hw/intc/arm_gicv3_its_common.c
+++ b/hw/intc/arm_gicv3_its_common.c
@@ -122,9 +122,9 @@ void gicv3_its_init_mmio(GICv3ITSState *s, const 
MemoryRegionOps *ops,
 msi_nonbroken = true;
 }
 
-static void gicv3_its_common_reset(DeviceState *dev)
+static void gicv3_its_common_reset_hold(Object *obj)
 {
-GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
+GICv3ITSState *s = ARM_GICV3_ITS_COMMON(obj);
 
 s->ctlr = 0;
 s->cbaser = 0;
@@ -137,8 +137,9 @@ static void gicv3_its_common_reset(DeviceState *dev)
 static void gicv3_its_common_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 
-dc->reset = gicv3_its_common_reset;
+rc->phases.hold = gicv3_its_common_reset_hold;
 dc->vmsd = &vmstate_its;
 }
 
-- 
2.25.1

[PATCH for-8.0 4/9] hw/intc: Convert TYPE_ARM_GIC_KVM to 3-phase reset

[Peter Maydell]

Now we have converted TYPE_ARM_GIC_COMMON, we can convert the
TYPE_ARM_GIC_KVM subclass to 3-phase reset.

Signed-off-by: Peter Maydell 
---
 hw/intc/arm_gic_kvm.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 7d2a13273a4..1d588946bce 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -38,7 +38,7 @@ DECLARE_OBJ_CHECKERS(GICState, KVMARMGICClass,
 struct KVMARMGICClass {
 ARMGICCommonClass parent_class;
 DeviceRealize parent_realize;
-void (*parent_reset)(DeviceState *dev);
+ResettablePhases parent_phases;
 };
 
 void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level)
@@ -473,12 +473,14 @@ static void kvm_arm_gic_get(GICState *s)
 }
 }
 
-static void kvm_arm_gic_reset(DeviceState *dev)
+static void kvm_arm_gic_reset_hold(Object *obj)
 {
-GICState *s = ARM_GIC_COMMON(dev);
+GICState *s = ARM_GIC_COMMON(obj);
 KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
 
-kgc->parent_reset(dev);
+if (kgc->parent_phases.hold) {
+kgc->parent_phases.hold(obj);
+}
 
 if (kvm_arm_gic_can_save_restore(s)) {
 kvm_arm_gic_put(s);
@@ -593,6 +595,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error 
**errp)
 static void kvm_arm_gic_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 ARMGICCommonClass *agcc = ARM_GIC_COMMON_CLASS(klass);
 KVMARMGICClass *kgc = KVM_ARM_GIC_CLASS(klass);
 
@@ -600,7 +603,8 @@ static void kvm_arm_gic_class_init(ObjectClass *klass, void 
*data)
 agcc->post_load = kvm_arm_gic_put;
 device_class_set_parent_realize(dc, kvm_arm_gic_realize,
 &kgc->parent_realize);
-device_class_set_parent_reset(dc, kvm_arm_gic_reset, &kgc->parent_reset);
+resettable_class_set_parent_phases(rc, NULL, kvm_arm_gic_reset_hold, NULL,
+   &kgc->parent_phases);
 }
 
 static const TypeInfo kvm_arm_gic_info = {
-- 
2.25.1

[PATCH for-8.0 9/9] hw/intc: Convert TYPE_KVM_ARM_ITS to 3-phase reset

[Peter Maydell]

Convert the TYPE_KVM_ARM_ITS device to 3-phase reset.

Signed-off-by: Peter Maydell 
---
 hw/intc/arm_gicv3_its_kvm.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index 529c7bd4946..7eda9fb86ea 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -37,7 +37,7 @@ DECLARE_OBJ_CHECKERS(GICv3ITSState, KVMARMITSClass,
 
 struct KVMARMITSClass {
 GICv3ITSCommonClass parent_class;
-void (*parent_reset)(DeviceState *dev);
+ResettablePhases parent_phases;
 };
 
 
@@ -197,13 +197,15 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
   GITS_CTLR, &s->ctlr, true, &error_abort);
 }
 
-static void kvm_arm_its_reset(DeviceState *dev)
+static void kvm_arm_its_reset_hold(Object *obj)
 {
-GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
+GICv3ITSState *s = ARM_GICV3_ITS_COMMON(obj);
 KVMARMITSClass *c = KVM_ARM_ITS_GET_CLASS(s);
 int i;
 
-c->parent_reset(dev);
+if (c->parent_phases.hold) {
+c->parent_phases.hold(obj);
+}
 
 if (kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
KVM_DEV_ARM_ITS_CTRL_RESET)) {
@@ -241,12 +243,14 @@ static Property kvm_arm_its_props[] = {
 static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 GICv3ITSCommonClass *icc = ARM_GICV3_ITS_COMMON_CLASS(klass);
 KVMARMITSClass *ic = KVM_ARM_ITS_CLASS(klass);
 
 dc->realize = kvm_arm_its_realize;
 device_class_set_props(dc, kvm_arm_its_props);
-device_class_set_parent_reset(dc, kvm_arm_its_reset, &ic->parent_reset);
+resettable_class_set_parent_phases(rc, NULL, kvm_arm_its_reset_hold, NULL,
+   &ic->parent_phases);
 icc->send_msi = kvm_its_send_msi;
 icc->pre_save = kvm_arm_its_pre_save;
 icc->post_load = kvm_arm_its_post_load;
-- 
2.25.1

Re: [PATCH 07/13] block: Don't use subtree drains in bdrv_drop_intermediate()

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

Instead of using a subtree drain from the top node (which also drains
child nodes of base that we're not even interested in), use a normal
drain for base, which automatically drains all of the parents, too.

Signed-off-by: Kevin Wolf


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()

[Bin Meng]

On Wed, Nov 9, 2022 at 6:10 PM Mauro Matteo Cascella
 wrote:
>
> On Wed, Nov 9, 2022 at 10:45 AM Siqi Chen  wrote:
> >
> > Hi,
> >
> > >This reproducer does not crash my QEMU. Am I missing anything?
> > I submitted the reproducer. Because the overflow is only one byte, it may 
> > not be detected by the host's heap allocator.  Do you compile your qemu 
> > with sanitizer?  This is my build configuration: "./configure 
> > --target-list=x86_64-softmmu --enable-sanitizers"
>
> Right, you need to recompile QEMU with ASAN support. This is an
> excerpt of the stack trace:

Is this documented somewhere? Is fuzzing.rst the right doc for this
feature? Looking at fuzzing.rst it says --enable-sanitizers is
optional.

Turning on --enable-sanitizers makes the build fail:

FAILED: subprojects/libvduse/libvduse.a.p/libvduse.c.o
cc -m64 -mcx16 -Isubprojects/libvduse/libvduse.a.p
-Isubprojects/libvduse -I../subprojects/libvduse
-fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g
-fsanitize=undefined -fsanitize=address -U_FORTIFY
_SOURCE -D_FORTIFY_SOURCE=2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
-Wold-style-declaration -W
old-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
-Wno-missing-include-dirs -Wn
o-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE
-D_GNU_SOURCE -MD -MQ subprojects/libvduse/libvduse.a.p/libvduse.c.o
-MF subprojects/libvduse/libvduse.a.p/libvduse.c.o.d -o
subprojects/libvduse/libvduse.a
.p/libvduse.c.o -c ../subprojects/libvduse/libvduse.c
In file included from /usr/include/string.h:495,
from ../subprojects/libvduse/libvduse.c:24:
In function ‘strncpy’,
inlined from ‘vduse_dev_create’ at ../subprojects/libvduse/libvduse.c:1312:5:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
‘__builtin_strncpy’ specified bound 256 equals destination size
[-Werror=stringop-truncation]
106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
| ^~
cc1: all warnings being treated as errors

I am using GCC 9.4 on Ubuntu 20.04

>
> =
> ==39159==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x61522880 at pc 0x55b023db5fe1 bp 0x7fffeeef1650 sp
> 0x7fffeeef1648
> WRITE of size 1 at 0x61522880 thread T0
> #0 0x55b023db5fe0 in sdhci_write_dataport ../../hw/sd/sdhci.c:562
> #1 0x55b023dc1cc7 in sdhci_write ../../hw/sd/sdhci.c:1216
> #2 0x55b024521e01 in memory_region_write_accessor 
> ../../softmmu/memory.c:492
> #3 0x55b0245222ab in access_with_adjusted_size ../../softmmu/memory.c:554
> #4 0x55b02452ff15 in memory_region_dispatch_write
> ../../softmmu/memory.c:1514
> #5 0x55b024568c67 in flatview_write_continue ../../softmmu/physmem.c:2814
> #6 0x55b02456908d in flatview_write ../../softmmu/physmem.c:2856
> #7 0x55b024569a74 in address_space_write ../../softmmu/physmem.c:2952
> #8 0x55b02457a63c in qtest_process_command ../../softmmu/qtest.c:538
> #9 0x55b02457ef97 in qtest_process_inbuf ../../softmmu/qtest.c:796
> #10 0x55b02457f089 in qtest_read ../../softmmu/qtest.c:808
> #11 0x55b0249d4372 in qemu_chr_be_write_impl ../../chardev/char.c:201
> #12 0x55b0249d4414 in qemu_chr_be_write ../../chardev/char.c:213
> #13 0x55b0249db586 in fd_chr_read ../../chardev/char-fd.c:72
> #14 0x55b02466ba5b in qio_channel_fd_source_dispatch
> ../../io/channel-watch.c:84
> #15 0x7f88093af0ae in g_main_context_dispatch
> (/lib64/libglib-2.0.so.0+0x550ae)
> #16 0x55b024c5ff14 in glib_pollfds_poll ../../util/main-loop.c:297
> #17 0x55b024c600fa in os_host_main_loop_wait ../../util/main-loop.c:320
> #18 0x55b024c603f3 in main_loop_wait ../../util/main-loop.c:596
> #19 0x55b023fcca21 in qemu_main_loop ../../softmmu/runstate.c:726
> #20 0x55b023679735 in qemu_main ../../softmmu/main.c:36
> #21 0x55b023679766 in main ../../softmmu/main.c:45
> #22 0x7f8808728f5f in __libc_start_call_main (/lib64/libc.so.6+0x40f5f)
> #23 0x7f880872900f in __libc_start_main_impl (/lib64/libc.so.6+0x4100f)
> #24 0x55b023679644 in _start (./qemu-system-x86_64+0x20f2644)
>

Regards,
Bin

[PATCH for-8.0 8/9] hw/intc: Convert TYPE_ARM_GICV3_ITS to 3-phase reset

[Peter Maydell]

Convert the TYPE_ARM_GICV3_ITS device to 3-phase reset.

Signed-off-by: Peter Maydell 
---
 hw/intc/arm_gicv3_its.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 2ff21ed6bbe..57c79da5c55 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -27,7 +27,7 @@ DECLARE_OBJ_CHECKERS(GICv3ITSState, GICv3ITSClass,
 
 struct GICv3ITSClass {
 GICv3ITSCommonClass parent_class;
-void (*parent_reset)(DeviceState *dev);
+ResettablePhases parent_phases;
 };
 
 /*
@@ -1953,12 +1953,14 @@ static void gicv3_arm_its_realize(DeviceState *dev, 
Error **errp)
 }
 }
 
-static void gicv3_its_reset(DeviceState *dev)
+static void gicv3_its_reset_hold(Object *obj)
 {
-GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
+GICv3ITSState *s = ARM_GICV3_ITS_COMMON(obj);
 GICv3ITSClass *c = ARM_GICV3_ITS_GET_CLASS(s);
 
-c->parent_reset(dev);
+if (c->parent_phases.hold) {
+c->parent_phases.hold(obj);
+}
 
 /* Quiescent bit reset to 1 */
 s->ctlr = FIELD_DP32(s->ctlr, GITS_CTLR, QUIESCENT, 1);
@@ -2012,12 +2014,14 @@ static Property gicv3_its_props[] = {
 static void gicv3_its_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 GICv3ITSClass *ic = ARM_GICV3_ITS_CLASS(klass);
 GICv3ITSCommonClass *icc = ARM_GICV3_ITS_COMMON_CLASS(klass);
 
 dc->realize = gicv3_arm_its_realize;
 device_class_set_props(dc, gicv3_its_props);
-device_class_set_parent_reset(dc, gicv3_its_reset, &ic->parent_reset);
+resettable_class_set_parent_phases(rc, NULL, gicv3_its_reset_hold, NULL,
+   &ic->parent_phases);
 icc->post_load = gicv3_its_post_load;
 }
 
-- 
2.25.1

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Ani Sinha]

On Wed, Nov 9, 2022 at 6:09 PM Claudio Fontana  wrote:
>
> On 11/9/22 09:04, Gerd Hoffmann wrote:
>
>
> Thanks Gerd,
>
> I think at least for our packaging purposes we'd rather have it as a 
> configure time option,
> so as to not put functionality in the hands of our users that can potentially 
> lock the guest.
>
> Is someone going to queue this, where?

Unfortunately we are on a hard code freeze at this time for the next
release. It might be better if you can resend the patch again to
remind someone to queue this up once the window opens again after the
release (mid december at the latest).

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Peter Maydell]

On Wed, 9 Nov 2022 at 16:21, Ani Sinha  wrote:
>
> On Wed, Nov 9, 2022 at 6:09 PM Claudio Fontana  wrote:
> >
> > On 11/9/22 09:04, Gerd Hoffmann wrote:
> >
> >
> > Thanks Gerd,
> >
> > I think at least for our packaging purposes we'd rather have it as a 
> > configure time option,
> > so as to not put functionality in the hands of our users that can 
> > potentially lock the guest.
> >
> > Is someone going to queue this, where?
>
> Unfortunately we are on a hard code freeze at this time for the next
> release. It might be better if you can resend the patch again to
> remind someone to queue this up once the window opens again after the
> release (mid december at the latest).

We are in hard freeze, but that just means "no new features".
This patch is fixing, or at least working around, a bug (i.e. that
QEMU can hang), so it's OK to go in during freeze, assuming the
usual code review etc.

thanks
-- PMM

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Ani Sinha]

On Wed, Nov 9, 2022 at 9:52 PM Peter Maydell  wrote:
>
> On Wed, 9 Nov 2022 at 16:21, Ani Sinha  wrote:
> >
> > On Wed, Nov 9, 2022 at 6:09 PM Claudio Fontana  wrote:
> > >
> > > On 11/9/22 09:04, Gerd Hoffmann wrote:
> > >
> > >
> > > Thanks Gerd,
> > >
> > > I think at least for our packaging purposes we'd rather have it as a 
> > > configure time option,
> > > so as to not put functionality in the hands of our users that can 
> > > potentially lock the guest.
> > >
> > > Is someone going to queue this, where?
> >
> > Unfortunately we are on a hard code freeze at this time for the next
> > release. It might be better if you can resend the patch again to
> > remind someone to queue this up once the window opens again after the
> > release (mid december at the latest).
>
> We are in hard freeze, but that just means "no new features".
> This patch is fixing, or at least working around, a bug (i.e. that
> QEMU can hang), so it's OK to go in during freeze, assuming the
> usual code review etc.

yeah just realized it's a bugfix. I was about to respond but you beat
me by a few secs :-)

Re: [QEMU][PATCH v2 3/5] xlnx-zynqmp: Connect Xilinx VERSAL CANFD controllers

[Francisco Iglesias]

Hi Vikram,

In the git summary s/zynqmp/versal/.

On [2022 Oct 21] Fri 22:47:44, Vikram Garhwal wrote:
> Connect CANFD0 and CANFD1 on the Versal-virt machine and update 
> xlnx-versal-virt
> document with CANFD command line examples.
> 
> Signed-off-by: Vikram Garhwal 
> ---
>  docs/system/arm/xlnx-versal-virt.rst | 31 ++
>  hw/arm/xlnx-versal-virt.c| 48 
>  hw/arm/xlnx-versal.c | 37 +
>  include/hw/arm/xlnx-versal.h | 12 +++
>  4 files changed, 128 insertions(+)
> 
> diff --git a/docs/system/arm/xlnx-versal-virt.rst 
> b/docs/system/arm/xlnx-versal-virt.rst
> index 92ad10d2da..372e4249f0 100644
> --- a/docs/system/arm/xlnx-versal-virt.rst
> +++ b/docs/system/arm/xlnx-versal-virt.rst
> @@ -34,6 +34,7 @@ Implemented devices:
>  - DDR memory
>  - BBRAM (36 bytes of Battery-backed RAM)
>  - eFUSE (3072 bytes of one-time field-programmable bit array)
> +- 2 CANFDs
>  
>  QEMU does not yet model any other devices, including the PL and the AI 
> Engine.
>  
> @@ -224,3 +225,33 @@ To use a different index value, N, from default of 1, 
> add:
>  
>Better yet, do not use actual product data when running guest image
>on this Xilinx Versal Virt board.
> +
> +Using CANFDs for Versal Virt
> +
> +Versal CANFD controller is developed based on SocketCAN and QEMU CAN bus
> +implementation. Bus connection and socketCAN connection for each CAN module
> +can be set through command lines.
> +
> +To connect both CANFD0 and CANFD1 on the same bus:
> +
> +.. code-block:: bash
> +
> +-object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus
> +
> +To connect CANFD0 and CANFD1 to separate buses:
> +
> +.. code-block:: bash
> +
> +-object can-bus,id=canbus0 -object can-bus,id=canbus1 \
> +-machine canbus0=canbus0 -machine canbus1=canbus1
> +
> +SocketCAN interface can connect to a Physical or a Virtual CAN interfaces on
> +host machine.

I'm not native english but this sounds better to me:

"The SocketCAN interface can connect to a Physical or a Virtual CAN interface on
the host machine."

> Please check this document to learn about CAN interface on Linux:
> +docs/system/devices/can.rst
> +
> +To connect CANFD0 and CANFD1 to host machine's CAN interface can0:
> +
> +.. code-block:: bash
> +
> +-object can-bus,id=canbus -machine canbus0=canbus -machine canbus1=canbus
> +-object can-host-socketcan,id=canhost0,if=can0,canbus=canbus
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 37fc9b919c..963ace861e 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -40,9 +40,11 @@ struct VersalVirt {
>  uint32_t clk_25Mhz;
>  uint32_t usb;
>  uint32_t dwc;
> +uint32_t canfd[2];
>  } phandle;
>  struct arm_boot_info binfo;
>  
> +CanBusState *canbus[XLNX_VERSAL_NR_CANFD];
>  struct {
>  bool secure;
>  } cfg;
> @@ -235,6 +237,33 @@ static void fdt_add_uart_nodes(VersalVirt *s)
>  }
>  }
>  
> +static void fdt_add_canfd_nodes(VersalVirt *s)
> +{
> +uint64_t addrs[] = { MM_CANFD0, MM_CANFD1 };
> +uint32_t size[] = { MM_CANFD0_SIZE, MM_CANFD1_SIZE };
> +unsigned int irqs[] = { VERSAL_CANFD0_IRQ_0, VERSAL_CANFD1_IRQ_0 };
> +int i;
> +
> +/* Create and connect CANFD0 and CANFD1 nodes to canbus0. */
> +for (i = 0; i < ARRAY_SIZE(addrs); i++) {
> +char *name = g_strdup_printf("/canfd@%" PRIx64, addrs[i]);
> +qemu_fdt_add_subnode(s->fdt, name);
> +qemu_fdt_setprop_cell(s->fdt, name, "rx-fifo0", 0x40);
> +qemu_fdt_setprop_cell(s->fdt, name, "enable-rx-fifo1", 0x1);
> +qemu_fdt_setprop_cell(s->fdt, name, "rx-fifo1", 0x40);
> +
> +qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
> +   GIC_FDT_IRQ_TYPE_SPI, irqs[i],
> +   GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
> + 2, addrs[i], 2, size[i]);
> +qemu_fdt_setprop_string(s->fdt, name, "compatible",
> +"xlnx,versal-canfd");

For the nodes we need to go with something similar to this:
https://github.com/Xilinx/linux-xlnx/blob/master/arch/arm64/boot/dts/xilinx/versal.dtsi#L153

> +
> +g_free(name);
> +}
> +}
> +
>  static void fdt_add_fixed_link_nodes(VersalVirt *s, char *gemname,
>   uint32_t phandle)
>  {
> @@ -639,12 +668,17 @@ static void versal_virt_init(MachineState *machine)
>  TYPE_XLNX_VERSAL);
>  object_property_set_link(OBJECT(&s->soc), "ddr", OBJECT(machine->ram),
>   &error_abort);
> +object_property_set_link(OBJECT(&s->soc), "canbus0", 
> OBJECT(s->canbus[0]),
> + &error_abort);
> +object_property_set_link(OBJECT(&s->soc

Re: [PATCH 08/13] stream: Replace subtree drain with a single node drain

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

The subtree drain was introduced in commit b1e1af394d9 as a way to avoid
graph changes between finding the base node and changing the block graph
as necessary on completion of the image streaming job.

The block graph could change between these two points because
bdrv_set_backing_hd() first drains the parent node, which involved
polling and can do anything.

Subtree draining was an imperfect way to make this less likely (because
with it, fewer callbacks are called during this window). Everyone agreed
that it's not really the right solution, and it was only committed as a
stopgap solution.

This replaces the subtree drain with a solution that simply drains the
parent node before we try to find the base node, and then call a version
of bdrv_set_backing_hd() that doesn't drain, but just asserts that the
parent node is already drained.

This way, any graph changes caused by draining happen before we start
looking at the graph and things stay consistent between finding the base
node and changing the graph.

Signed-off-by: Kevin Wolf 


[..]

  
  base = bdrv_filter_or_cow_bs(s->above_base);

-if (base) {
-bdrv_ref(base);
-}
-
  unfiltered_base = bdrv_skip_filters(base);
  
  if (bdrv_cow_child(unfiltered_bs)) {

@@ -82,7 +85,7 @@ static int stream_prepare(Job *job)
  }
  }
  
-bdrv_set_backing_hd(unfiltered_bs, base, &local_err);

+bdrv_set_backing_hd_drained(unfiltered_bs, base, &local_err);
  ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt, 
false);


If we have yield points / polls during bdrv_set_backing_hd_drained() and 
bdrv_change_backing_file(), it's still bad and another graph-modifying 
operation may interleave. But b1e1af394d9 reports only polling in 
bdrv_set_backing_hd(), so I think it's OK to not care about other cases.


  if (local_err) {
  error_report_err(local_err);
@@ -92,10 +95,7 @@ static int stream_prepare(Job *job)
  }
  
  out:

-if (base) {
-bdrv_unref(base);
-}
-bdrv_subtree_drained_end(s->above_base);
+bdrv_drained_end(unfiltered_bs);
  return ret;
  }
  


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

[PATCH for-7.2 0/5] block/mirror: Do not wait for active writes

[Hanna Reitz]

Hi,

For some reason(TM), the mirror job, when in write-blocking mode, has
decided not to issue background requests while active writes are in
flight.  (Or rather, it was probably me who decided that.)

The problem is that only background requests can really reliably help
you make progress.  When all the mirror job does is to mirror guest
writes to the target, but not copy anything else that remains to be
copied in the disk, it will not make converge.

It is unclear why it is that way, so patch 1 simply drops that
dependency, and attempts to better explain the remaining
wait-dependencies we have between requests (i.e. why active requests
must wait on all overlapping requests for the whole range, but
background requests only wait on any conflicts that concern the
beginning of the range they want to copy).

Patch 2 is clean-up, patch 3 fixes another bug I found while trying to
come up with a working test case.

Patch 4 is that test case (I hope it works on your end, too), and patch
5 is a test case for the fix in patch 3.


Hanna Reitz (5):
  block/mirror: Do not wait for active writes
  block/mirror: Drop mirror_wait_for_any_operation()
  block/mirror: Fix NULL s->job in active writes
  iotests/151: Test that active mirror progresses
  iotests/151: Test active requests on mirror start

 block/mirror.c |  78 -
 tests/qemu-iotests/151 | 227 -
 tests/qemu-iotests/151.out |   4 +-
 3 files changed, 278 insertions(+), 31 deletions(-)

-- 
2.36.1

[PATCH for-7.2 2/5] block/mirror: Drop mirror_wait_for_any_operation()

[Hanna Reitz]

mirror_wait_for_free_in_flight_slot() is the only remaining user of
mirror_wait_for_any_operation(), so inline the latter into the former.

Signed-off-by: Hanna Reitz 
---
 block/mirror.c | 21 -
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index e5467b0053..5b6f42392c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -305,19 +305,21 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t 
*offset,
 }
 
 static inline void coroutine_fn
-mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
+mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
 {
 MirrorOp *op;
 
 QTAILQ_FOREACH(op, &s->ops_in_flight, next) {
-/* Do not wait on pseudo ops, because it may in turn wait on
+/*
+ * Do not wait on pseudo ops, because it may in turn wait on
  * some other operation to start, which may in fact be the
  * caller of this function.  Since there is only one pseudo op
  * at any given time, we will always find some real operation
- * to wait on. */
-if (!op->is_pseudo_op && op->is_in_flight &&
-op->is_active_write == active)
-{
+ * to wait on.
+ * Also, do not wait on active operations, because they do not
+ * use up in-flight slots.
+ */
+if (!op->is_pseudo_op && op->is_in_flight && !op->is_active_write) {
 qemu_co_queue_wait(&op->waiting_requests, NULL);
 return;
 }
@@ -325,13 +327,6 @@ mirror_wait_for_any_operation(MirrorBlockJob *s, bool 
active)
 abort();
 }
 
-static inline void coroutine_fn
-mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
-{
-/* Only non-active operations use up in-flight slots */
-mirror_wait_for_any_operation(s, false);
-}
-
 /* Perform a mirror copy operation.
  *
  * *op->bytes_handled is set to the number of bytes copied after and
-- 
2.36.1

[PATCH for-7.2 1/5] block/mirror: Do not wait for active writes

[Hanna Reitz]

Waiting for all active writes to settle before daring to create a
background copying operation means that we will never do background
operations while the guest does anything (in write-blocking mode), and
therefore cannot converge.  Yes, we also will not diverge, but actually
converging would be even nicer.

It is unclear why we did decide to wait for all active writes to settle
before creating a background operation, but it just does not seem
necessary.  Active writes will put themselves into the in_flight bitmap
and thus properly block actually conflicting background requests.

It is important for active requests to wait on overlapping background
requests, which we do in active_write_prepare().  However, so far it was
not documented why it is important.  Add such documentation now, and
also to the other call of mirror_wait_on_conflicts(), so that it becomes
more clear why and when requests need to actively wait for other
requests to settle.

Another thing to note is that of course we need to ensure that there are
no active requests when the job completes, but that is done by virtue of
the BDS being drained anyway, so there cannot be any active requests at
that point.

With this change, we will need to explicitly keep track of how many
bytes are in flight in active requests so that
job_progress_set_remaining() in mirror_run() can set the correct number
of remaining bytes.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2123297
Signed-off-by: Hanna Reitz 
---
 block/mirror.c | 37 ++---
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 1a75a47cc3..e5467b0053 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -82,6 +82,7 @@ typedef struct MirrorBlockJob {
 int max_iov;
 bool initial_zeroing_ongoing;
 int in_active_write_counter;
+int64_t active_write_bytes_in_flight;
 bool prepared;
 bool in_drain;
 } MirrorBlockJob;
@@ -494,6 +495,13 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 }
 bdrv_dirty_bitmap_unlock(s->dirty_bitmap);
 
+/*
+ * Wait for concurrent requests to @offset.  The next loop will limit the
+ * copied area based on in_flight_bitmap so we only copy an area that does
+ * not overlap with concurrent in-flight requests.  Still, we would like to
+ * copy something, so wait until there are at least no more requests to the
+ * very beginning of the area.
+ */
 mirror_wait_on_conflicts(NULL, s, offset, 1);
 
 job_pause_point(&s->common.job);
@@ -988,12 +996,6 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
 int64_t cnt, delta;
 bool should_complete;
 
-/* Do not start passive operations while there are active
- * writes in progress */
-while (s->in_active_write_counter) {
-mirror_wait_for_any_operation(s, true);
-}
-
 if (s->ret < 0) {
 ret = s->ret;
 goto immediate_exit;
@@ -1010,7 +1012,9 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
 /* cnt is the number of dirty bytes remaining and s->bytes_in_flight is
  * the number of bytes currently being processed; together those are
  * the current remaining operation length */
-job_progress_set_remaining(&s->common.job, s->bytes_in_flight + cnt);
+job_progress_set_remaining(&s->common.job,
+   s->bytes_in_flight + cnt +
+   s->active_write_bytes_in_flight);
 
 /* Note that even when no rate limit is applied we need to yield
  * periodically with no pending I/O so that bdrv_drain_all() returns.
@@ -1071,6 +1075,10 @@ static int coroutine_fn mirror_run(Job *job, Error 
**errp)
 
 s->in_drain = true;
 bdrv_drained_begin(bs);
+
+/* Must be zero because we are drained */
+assert(s->in_active_write_counter == 0);
+
 cnt = bdrv_get_dirty_count(s->dirty_bitmap);
 if (cnt > 0 || mirror_flush(s) < 0) {
 bdrv_drained_end(bs);
@@ -1306,6 +1314,7 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod 
method,
 }
 
 job_progress_increase_remaining(&job->common.job, bytes);
+job->active_write_bytes_in_flight += bytes;
 
 switch (method) {
 case MIRROR_METHOD_COPY:
@@ -1327,6 +1336,7 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod 
method,
 abort();
 }
 
+job->active_write_bytes_in_flight -= bytes;
 if (ret >= 0) {
 job_progress_update(&job->common.job, bytes);
 } else {
@@ -1375,6 +1385,19 @@ static MirrorOp *coroutine_fn 
active_write_prepare(MirrorBlockJob *s,
 
 s->in_active_write_counter++;
 
+/*
+ * Wait for concurrent requests affecting the area.  If there are already
+ * running requests that are copying off now-to-be stale data in the area,
+ * we must wait for them to finish be

[PATCH for-7.2 3/5] block/mirror: Fix NULL s->job in active writes

[Hanna Reitz]

There is a small gap in mirror_start_job() before putting the mirror
filter node into the block graph (bdrv_append() call) and the actual job
being created.  Before the job is created, MirrorBDSOpaque.job is NULL.

It is possible that requests come in when bdrv_drained_end() is called,
and those requests would see MirrorBDSOpaque.job == NULL.  Have our
filter node handle that case gracefully.

Signed-off-by: Hanna Reitz 
---
 block/mirror.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 5b6f42392c..251adc5ae0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1438,11 +1438,13 @@ static int coroutine_fn 
bdrv_mirror_top_do_write(BlockDriverState *bs,
 MirrorOp *op = NULL;
 MirrorBDSOpaque *s = bs->opaque;
 int ret = 0;
-bool copy_to_target;
+bool copy_to_target = false;
 
-copy_to_target = s->job->ret >= 0 &&
- !job_is_cancelled(&s->job->common.job) &&
- s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
+if (s->job) {
+copy_to_target = s->job->ret >= 0 &&
+ !job_is_cancelled(&s->job->common.job) &&
+ s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
+}
 
 if (copy_to_target) {
 op = active_write_prepare(s->job, offset, bytes);
@@ -1487,11 +1489,13 @@ static int coroutine_fn 
bdrv_mirror_top_pwritev(BlockDriverState *bs,
 QEMUIOVector bounce_qiov;
 void *bounce_buf;
 int ret = 0;
-bool copy_to_target;
+bool copy_to_target = false;
 
-copy_to_target = s->job->ret >= 0 &&
- !job_is_cancelled(&s->job->common.job) &&
- s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
+if (s->job) {
+copy_to_target = s->job->ret >= 0 &&
+ !job_is_cancelled(&s->job->common.job) &&
+ s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
+}
 
 if (copy_to_target) {
 /* The guest might concurrently modify the data to write; but
-- 
2.36.1

[PATCH for-7.2 5/5] iotests/151: Test active requests on mirror start

[Hanna Reitz]

Have write requests happen to the source node right when we start a
mirror job.  The mirror filter node may encounter MirrorBDSOpaque.job
being NULL, but this should not cause a segfault.

Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/151 | 53 +++---
 tests/qemu-iotests/151.out |  4 +--
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
index 0a052e5050..b4d1bc2553 100755
--- a/tests/qemu-iotests/151
+++ b/tests/qemu-iotests/151
@@ -22,7 +22,8 @@
 import math
 import os
 import subprocess
-from typing import List
+import time
+from typing import List, Optional
 import iotests
 from iotests import qemu_img
 
@@ -195,12 +196,15 @@ class TestActiveMirror(iotests.QMPTestCase):
 self.potential_writes_in_flight = False
 
 
-class TestThrottledWithNbdExport(iotests.QMPTestCase):
+class TestThrottledWithNbdExportBase(iotests.QMPTestCase):
 image_len = 128 * 1024 * 1024  # MB
-iops = 16
+iops: Optional[int] = None
 background_processes: List['subprocess.Popen[str]'] = []
 
 def setUp(self):
+# Must be set by subclasses
+self.assertIsNotNone(self.iops)
+
 qemu_img('create', '-f', iotests.imgfmt, source_img, '128M')
 qemu_img('create', '-f', iotests.imgfmt, target_img, '128M')
 
@@ -284,6 +288,10 @@ class TestThrottledWithNbdExport(iotests.QMPTestCase):
 os.remove(source_img)
 os.remove(target_img)
 
+
+class TestLowThrottledWithNbdExport(TestThrottledWithNbdExportBase):
+iops = 16
+
 def testUnderLoad(self):
 '''
 Throttle the source node, then issue a whole bunch of external requests
@@ -370,6 +378,45 @@ class TestThrottledWithNbdExport(iotests.QMPTestCase):
 self.assertGreater(start_remaining - end_remaining, 0)
 
 
+class TestHighThrottledWithNbdExport(TestThrottledWithNbdExportBase):
+iops = 1024
+
+def testActiveOnCreation(self):
+'''
+Issue requests on the mirror source node right as the mirror is
+instated.  It's possible that requests occur before the actual job is
+created, but after the node has been put into the graph.  Write
+requests across the node must in that case be forwarded to the source
+node without attempting to mirror them (there is no job object yet, so
+attempting to access it would cause a segfault).
+We do this with a lightly throttled node (i.e. quite high IOPS limit).
+Using throttling seems to increase reproductivity, but if the limit is
+too low, all requests allowed per second will be submitted before
+mirror_start_job() gets to the problematic point.
+'''
+
+# Let qemu-img bench create write requests (enough for two seconds on
+# the virtual clock)
+bench_args = ['bench', '-w', '-d', '1024', '-f', 'nbd',
+  '-c', str(self.iops * 2), self.nbd_url]
+p = iotests.qemu_tool_popen(iotests.qemu_img_args + bench_args)
+self.background_processes += [p]
+
+# Give qemu-img bench time to start up and issue requests
+time.sleep(1.0)
+# Flush the request queue, so new requests can come in right as we
+# start blockdev-mirror
+self.vm.qtest(f'clock_step {1 * 1000 * 1000 * 1000}')
+
+result = self.vm.qmp('blockdev-mirror',
+ job_id='mirror',
+ device='source-node',
+ target='target-node',
+ sync='full',
+ copy_mode='write-blocking')
+self.assert_qmp(result, 'return', {})
+
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'raw'],
  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/151.out b/tests/qemu-iotests/151.out
index 914e3737bd..3f8a935a08 100644
--- a/tests/qemu-iotests/151.out
+++ b/tests/qemu-iotests/151.out
@@ -1,5 +1,5 @@
-.
+..
 --
-Ran 5 tests
+Ran 6 tests
 
 OK
-- 
2.36.1

[PATCH for-7.2 4/5] iotests/151: Test that active mirror progresses

[Hanna Reitz]

Before this series, a mirror job in write-blocking mode would pause
issuing background requests while active requests are in flight.  Thus,
if the source is constantly in use by active requests, no actual
progress can be made.

This series should have fixed that, making the mirror job issue
background requests even while active requests are in flight.

Have a new test case in 151 verify this.

Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/151 | 180 -
 tests/qemu-iotests/151.out |   4 +-
 2 files changed, 181 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
index 93d14193d0..0a052e5050 100755
--- a/tests/qemu-iotests/151
+++ b/tests/qemu-iotests/151
@@ -19,7 +19,10 @@
 # along with this program.  If not, see .
 #
 
+import math
 import os
+import subprocess
+from typing import List
 import iotests
 from iotests import qemu_img
 
@@ -50,7 +53,7 @@ class TestActiveMirror(iotests.QMPTestCase):
 self.vm = iotests.VM()
 self.vm.add_drive_raw(self.vm.qmp_to_opts(blk_source))
 self.vm.add_blockdev(self.vm.qmp_to_opts(blk_target))
-self.vm.add_device('virtio-blk,drive=source')
+self.vm.add_device('virtio-blk,id=vblk,drive=source')
 self.vm.launch()
 
 def tearDown(self):
@@ -192,6 +195,181 @@ class TestActiveMirror(iotests.QMPTestCase):
 self.potential_writes_in_flight = False
 
 
+class TestThrottledWithNbdExport(iotests.QMPTestCase):
+image_len = 128 * 1024 * 1024  # MB
+iops = 16
+background_processes: List['subprocess.Popen[str]'] = []
+
+def setUp(self):
+qemu_img('create', '-f', iotests.imgfmt, source_img, '128M')
+qemu_img('create', '-f', iotests.imgfmt, target_img, '128M')
+
+self.vm = iotests.VM()
+self.vm.launch()
+
+result = self.vm.qmp('object-add', **{
+'qom-type': 'throttle-group',
+'id': 'thrgr',
+'limits': {
+'iops-total': self.iops,
+'iops-total-max': self.iops
+}
+})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-add', **{
+'node-name': 'source-node',
+'driver': 'throttle',
+'throttle-group': 'thrgr',
+'file': {
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': source_img
+}
+}
+})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-add', **{
+'node-name': 'target-node',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': target_img
+}
+})
+self.assert_qmp(result, 'return', {})
+
+self.nbd_sock = iotests.file_path('nbd.sock',
+  base_dir=iotests.sock_dir)
+self.nbd_url = f'nbd+unix:///source-node?socket={self.nbd_sock}'
+
+result = self.vm.qmp('nbd-server-start', addr={
+'type': 'unix',
+'data': {
+'path': self.nbd_sock
+}
+})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('block-export-add', id='exp0', type='nbd',
+ node_name='source-node', writable=True)
+self.assert_qmp(result, 'return', {})
+
+def tearDown(self):
+# Wait for background requests to settle
+try:
+while True:
+p = self.background_processes.pop()
+while True:
+try:
+p.wait(timeout=0.0)
+break
+except subprocess.TimeoutExpired:
+self.vm.qtest(f'clock_step {1 * 1000 * 1000 * 1000}')
+except IndexError:
+pass
+
+# Cancel ongoing block jobs
+for job in self.vm.qmp('query-jobs')['return']:
+self.vm.qmp('block-job-cancel', device=job['id'], force=True)
+
+while True:
+self.vm.qtest(f'clock_step {1 * 1000 * 1000 * 1000}')
+if len(self.vm.qmp('query-jobs')['return']) == 0:
+break
+
+self.vm.shutdown()
+os.remove(source_img)
+os.remove(target_img)
+
+def testUnderLoad(self):
+'''
+Throttle the source node, then issue a whole bunch of external requests
+while the mirror job (in write-blocking mode) is running.  We want to
+see background requests being issued even while the source is under
+full load by active writes, so that progress can be made towards READY.
+'''
+
+# Fill the first half of the source image; do not fill the second half,
+# that is where we will have active requests occur.  This ensures that
+ 

Re: [PATCH v1 1/1] migration: Fix yank on postcopy multifd crashing guest after migration

[Leonardo Bras Soares Passos]

On Wed, Nov 9, 2022 at 10:31 AM Dr. David Alan Gilbert
 wrote:
>
> * Leonardo Bras (leob...@redhat.com) wrote:
> > When multifd and postcopy-ram capabilities are enabled, if a
> > migrate-start-postcopy is attempted, the migration will finish sending the
> > memory pages and then crash with the following error:
>
> How does that happen? Isn't multifd+postcopy still disabled, I see in
> migrate_caps_check
>
> if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> 
> if (cap_list[MIGRATION_CAPABILITY_MULTIFD]) {
> error_setg(errp, "Postcopy is not yet compatible with multifd");
> return false;
> }
> }
>

I can't see this happening in upstream code (v7.2.0-rc0). Could you
please tell me the lines where this happens?

I mean, I see cap_list[MIGRATION_CAPABILITY_MULTIFD] and
cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM] in migrate_caps_check()
but I can't see them nested like this, so I am probably missing
something.

This procedure to reproduce was shared by Xiaohui Li (I added a few tweaks):

1.Boot a guest with any qemu command on source host;
2.Boot a guest with same qemu command but append '-incoming defer' on
destination host;
3.Enable multifd and postcopy capabilities on src and dst hosts:
{"execute":"migrate-set-capabilities","arguments":{"capabilities":[{"capability":"multifd","state":true}]}}
{"execute":"migrate-set-capabilities","arguments":{"capabilities":[{"capability":"postcopy-ram","state":true}]}}
4.During migration is active, switch to postcopy mode:
{"execute":"migrate-start-postcopy"}

Best regards,
Leo


>
> Dave
>
> > qemu-system-x86_64: ../util/yank.c:107: yank_unregister_instance: Assertion
> > `QLIST_EMPTY(&entry->yankfns)' failed.
> >
> > This happens because even though all multifd channels could
> > yank_register_function(), none of them could unregister it before
> > unregistering the MIGRATION_YANK_INSTANCE, causing the assert to fail.
> >
> > Fix that by calling multifd_load_cleanup() on postcopy_ram_listen_thread()
> > before MIGRATION_YANK_INSTANCE is unregistered.
> >
> > Fixes: b5eea99ec2 ("migration: Add yank feature")
> > Reported-by: Li Xiaohui 
> > Signed-off-by: Leonardo Bras 
> > ---
> >  migration/migration.h |  1 +
> >  migration/migration.c | 18 +-
> >  migration/savevm.c|  2 ++
> >  3 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/migration/migration.h b/migration/migration.h
> > index cdad8aceaa..240f64efb0 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -473,6 +473,7 @@ void migration_make_urgent_request(void);
> >  void migration_consume_urgent_request(void);
> >  bool migration_rate_limit(void);
> >  void migration_cancel(const Error *error);
> > +bool migration_load_cleanup(void);
> >
> >  void populate_vfio_info(MigrationInfo *info);
> >  void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 739bb683f3..4f363b2a95 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -486,6 +486,17 @@ void migrate_add_address(SocketAddress *address)
> >QAPI_CLONE(SocketAddress, address));
> >  }
> >
> > +bool migration_load_cleanup(void)
> > +{
> > +Error *local_err = NULL;
> > +
> > +if (multifd_load_cleanup(&local_err)) {
> > +error_report_err(local_err);
> > +return true;
> > +}
> > +return false;
> > +}
> > +
> >  static void qemu_start_incoming_migration(const char *uri, Error **errp)
> >  {
> >  const char *p = NULL;
> > @@ -540,8 +551,7 @@ static void process_incoming_migration_bh(void *opaque)
> >   */
> >  qemu_announce_self(&mis->announce_timer, migrate_announce_params());
> >
> > -if (multifd_load_cleanup(&local_err) != 0) {
> > -error_report_err(local_err);
> > +if (migration_load_cleanup()) {
> >  autostart = false;
> >  }
> >  /* If global state section was not received or we are in running
> > @@ -646,9 +656,7 @@ fail:
> >  migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> >MIGRATION_STATUS_FAILED);
> >  qemu_fclose(mis->from_src_file);
> > -if (multifd_load_cleanup(&local_err) != 0) {
> > -error_report_err(local_err);
> > -}
> > +migration_load_cleanup();
> >  exit(EXIT_FAILURE);
> >  }
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index a0cdb714f7..250caff7f4 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1889,6 +1889,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >  exit(EXIT_FAILURE);
> >  }
> >
> > +migration_load_cleanup();
> > +
> >  migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > MIGRATION_STATUS_COMPLETED);
> >  /*
> > --
> > 2.38.1
> >
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>

[PATCH for-8.0 2/2] hw/input/ps2.c: Convert TYPE_PS2_{KBD, MOUSE}_DEVICE to 3-phase reset

[Peter Maydell]

Convert the child classes TYPE_PS2_KBD_DEVICE and
TYPE_PS2_MOUSE_DEVICE to the 3-phase reset system.  This allows us to
stop using the old device_class_set_parent_reset() function.

We don't need to register an 'exit' phase function for the
subclasses, because they have no work to do in that phase.  Passing
NULL to resettable_class_set_parent_phases() will result in the
parent class method being called for that phase, so we don't need to
register a function purely to chain to the parent 'exit' phase
function.

Signed-off-by: Peter Maydell 
---
 include/hw/input/ps2.h |  2 +-
 hw/input/ps2.c | 31 ---
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index ff777582cd6..cd61a634c39 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -36,7 +36,7 @@
 struct PS2DeviceClass {
 SysBusDeviceClass parent_class;
 
-DeviceReset parent_reset;
+ResettablePhases parent_phases;
 };
 
 /*
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 47a5d68e300..3253ab6a92c 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1042,13 +1042,16 @@ static void ps2_common_post_load(PS2State *s)
 q->cwptr = ccount ? (q->rptr + ccount) & (PS2_BUFFER_SIZE - 1) : -1;
 }
 
-static void ps2_kbd_reset(DeviceState *dev)
+static void ps2_kbd_reset_hold(Object *obj)
 {
-PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(dev);
-PS2KbdState *s = PS2_KBD_DEVICE(dev);
+PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(obj);
+PS2KbdState *s = PS2_KBD_DEVICE(obj);
 
 trace_ps2_kbd_reset(s);
-ps2dc->parent_reset(dev);
+
+if (ps2dc->parent_phases.hold) {
+ps2dc->parent_phases.hold(obj);
+}
 
 s->scan_enabled = 1;
 s->translate = 0;
@@ -1056,13 +1059,16 @@ static void ps2_kbd_reset(DeviceState *dev)
 s->modifiers = 0;
 }
 
-static void ps2_mouse_reset(DeviceState *dev)
+static void ps2_mouse_reset_hold(Object *obj)
 {
-PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(dev);
-PS2MouseState *s = PS2_MOUSE_DEVICE(dev);
+PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(obj);
+PS2MouseState *s = PS2_MOUSE_DEVICE(obj);
 
 trace_ps2_mouse_reset(s);
-ps2dc->parent_reset(dev);
+
+if (ps2dc->parent_phases.hold) {
+ps2dc->parent_phases.hold(obj);
+}
 
 s->mouse_status = 0;
 s->mouse_resolution = 0;
@@ -1245,10 +1251,12 @@ static void ps2_mouse_realize(DeviceState *dev, Error 
**errp)
 static void ps2_kbd_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 PS2DeviceClass *ps2dc = PS2_DEVICE_CLASS(klass);
 
 dc->realize = ps2_kbd_realize;
-device_class_set_parent_reset(dc, ps2_kbd_reset, &ps2dc->parent_reset);
+resettable_class_set_parent_phases(rc, NULL, ps2_kbd_reset_hold, NULL,
+   &ps2dc->parent_phases);
 dc->vmsd = &vmstate_ps2_keyboard;
 }
 
@@ -1262,11 +1270,12 @@ static const TypeInfo ps2_kbd_info = {
 static void ps2_mouse_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 PS2DeviceClass *ps2dc = PS2_DEVICE_CLASS(klass);
 
 dc->realize = ps2_mouse_realize;
-device_class_set_parent_reset(dc, ps2_mouse_reset,
-  &ps2dc->parent_reset);
+resettable_class_set_parent_phases(rc, NULL, ps2_mouse_reset_hold, NULL,
+   &ps2dc->parent_phases);
 dc->vmsd = &vmstate_ps2_mouse;
 }
 
-- 
2.25.1

[PATCH for-8.0 1/2] hw/input/ps2: Convert TYPE_PS2_DEVICE to 3-phase reset

[Peter Maydell]

Convert the parent class TYPE_PS2_DEVICE to 3-phase reset.  Note that
we need an 'exit' phase function as well as the usual 'hold' phase
function, because changing outbound IRQ line state is only permitted
in 'exit'.  (Strictly speaking it's not supposed to be done in a
legacy reset handler either, but you can often get away with it.)

Signed-off-by: Peter Maydell 
---
 hw/input/ps2.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 05cf7111e31..47a5d68e300 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1001,12 +1001,18 @@ void ps2_write_mouse(PS2MouseState *s, int val)
 }
 }
 
-static void ps2_reset(DeviceState *dev)
+static void ps2_reset_hold(Object *obj)
 {
-PS2State *s = PS2_DEVICE(dev);
+PS2State *s = PS2_DEVICE(obj);
 
 s->write_cmd = -1;
 ps2_reset_queue(s);
+}
+
+static void ps2_reset_exit(Object *obj)
+{
+PS2State *s = PS2_DEVICE(obj);
+
 ps2_lower_irq(s);
 }
 
@@ -1281,8 +1287,10 @@ static void ps2_init(Object *obj)
 static void ps2_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 
-dc->reset = ps2_reset;
+rc->phases.hold = ps2_reset_hold;
+rc->phases.exit = ps2_reset_exit;
 set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
-- 
2.25.1

[PATCH for-8.0 0/2] hw/input/ps2: Convert to 3-phase reset

[Peter Maydell]

This patchset converts the ps2 keyboard and mouse devices to 3-phase
reset. The rationale here is that it would be nice to get rid of the
device_class_set_parent_reset() function, which is used by
legacy-reset subclasses which want to chain to their parent's reset
function. There aren't very many of these devices in total, and if we
convert them all to 3-phase reset they can use the 3-phase-reset
equivalent (resettable_class_set_parent_phases()).  Eventually this
will then let us simplify the transitional code for handling old-style
device reset.

This is one of a number of patchsets to do this that I'm planning to
write and send out over the next few weeks. It's all 8.0 material.

thanks
-- PMM

Peter Maydell (2):
  hw/input/ps2: Convert TYPE_PS2_DEVICE to 3-phase reset
  hw/input/ps2.c: Convert TYPE_PS2_{KBD,MOUSE}_DEVICE to 3-phase reset

 include/hw/input/ps2.h |  2 +-
 hw/input/ps2.c | 45 +-
 2 files changed, 32 insertions(+), 15 deletions(-)

-- 
2.25.1

[ANNOUNCE] QEMU 7.2.0-rc0 is now available

[Michael Roth]

Hello,

On behalf of the QEMU Team, I'd like to announce the availability of the
first release candidate for the QEMU 7.2 release. This release is meant
for testing purposes and should not be used in a production environment.

  http://download.qemu-project.org/qemu-7.2.0-rc0.tar.xz
  http://download.qemu-project.org/qemu-7.2.0-rc0.tar.xz.sig

You can help improve the quality of the QEMU 7.2 release by testing this
release and reporting bugs using our GitLab issue tracker:

  https://gitlab.com/qemu-project/qemu/-/milestones/7#tab-issues

The release plan, as well a documented known issues for release
candidates, are available at:

  http://wiki.qemu.org/Planning/7.2

Please add entries to the ChangeLog for the 7.2 release below:

  http://wiki.qemu.org/ChangeLog/7.2

Thank you to everyone involved!

Re: [PATCH 09/13] block: Remove subtree drains

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:

Subtree drains are not used any more. Remove them.

After this, BdrvChildClass.attach/detach() don't poll any more.

Signed-off-by: Kevin Wolf



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: Questions about QEMU exception

[Li, Kevin]

Hi Peter,

We first install via homebrew and then extract the executables (qemu-img and 
qemu-system-xxx) out.
We recently did this, so it should be the latest version of qemu via homebrew, 
and our package is for MacOS, which needs sign and notarize.
If we extract in this way, and use it on other env without signing, it will 
show the unidentified developer, so we sign it along with other executables.
We uses these executables to pack a pkg installer, so first we sign all the 
executables including qemu, and then sign the pkg, and finally notarize the pkg.
I think the previous error I mentioned maybe caused by we didn't sign qemu with 
the entitlement, so I signed with the entitlement with following keys:
com.apple.security.cs.allow-jit

com.apple.security.cs.allow-unsigned-executable-memory

com.apple.security.cs.disable-library-validation

com.apple.security.hypervisor

com.apple.security.inherit

However, even I signed it, qemu still show unidentified developer pop up, which 
doesn't not happen to other executables.
Please let me know if you have any clue or suggestion with it. Thanks in 
advance.

Thanks,
Kevin


On 2022-11-09, 2:10 AM, "Peter Maydell"  wrote:

CAUTION: This email originated from outside of the organization. Do not 
click links or open attachments unless you can confirm the sender and know the 
content is safe.



On Wed, 9 Nov 2022 at 01:53, Li, Kevin  wrote:
>
> Hi qemu community,
>
>
>
> We are working on some open source project which uses qemu on mac, and we 
have some signing process to sign qemu-system-x86_64.
>
> If qemu-system-x86_64 is not signed, we don’t see any problem, but after 
sign it, we got the following error:
>
>
>
> qemu-system-x86_64 -M none -netdev help]: stdout=\"Accelerators supported 
in QEMU binary:\\ntcg\\nhax\\nhvf\\n\", stderr=\"qemu-system-x86_64: allocate 
1073741824 bytes for jit buffer: Invalid argument
>
>
>
> Does anyone has clue about what change may result in this failure?

You don't say which QEMU version you're using. Does it still happen
with the most recent release? Does it still happen if you build
from current head-of-git ?

PS: I think the QEMU build process should already be signing the executable,
so I'm not sure why you need to sign it again (see scripts/entitlement.sh).

thanks
-- PMM